Add optional split param to the eval APIs: run or read results by train/val/test - #1621
Closed
scosman wants to merge 70 commits into
Closed
Add optional split param to the eval APIs: run or read results by train/val/test#1621scosman wants to merge 70 commits into
scosman wants to merge 70 commits into
Conversation
Add an EvalJobWorker that wraps the existing EvalRunner so an eval can run
in the background through the job system. Expose a typed, non-streaming
kickoff endpoint POST /api/jobs/evals/run for agents (allow + requires
approval); poll GET /api/jobs/{id} for progress/result instead of SSE. The
endpoint uses a two-segment path so it can never be shadowed by the generic
POST /api/jobs/{type} route.
Flip the UI's SSE eval-run endpoints (run_comparison, run_calibration) to
agent-forbidden, since agents should use the background job API instead.
Also fix a reconcile bug surfaced by review: a worker that can't derive its
error count from entities (failed eval items leave no EvalRun) now returns
JobDerivedState.error=None, and _apply_derived keeps the live reported error
count instead of clobbering it to 0 on reconcile (mirrors total/message).
Regenerated agent-check annotations and the OpenAPI TS schema.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013QNY986xT2VCm7xngUg4tF
Salvages the assistant auto-mode prototype (PR #1451) onto leonard/kil-686-eval-job, dropping the eval/finetune/RAG-via-job refactor (PRs #1436/#1450) that the original branch was stacked on. The auto-mode feature is self-contained: a new chat/auto/ app-server subsystem (auto-run registry/runner/events/SSE + API), enable/disable auto-mode built-in tools in libs/core, and the assistant web UI (auto_run_store, consent dialog, chat history, chat.svelte). Its only coupling to the dropped stack was a generic SSE keepalive helper, which the dropped stack had extracted into jobs/events.py; that helper (KeepalivePing / KEEPALIVE_PING / iter_with_keepalive) is ported here so chat/auto reuses it unchanged. Conflict resolutions vs the newer base: - chat.svelte: took auto-mode's version (the built+tested feature) and re-applied the base's three UI refactors landed since the fork — scrollbar-to-the-side, DaisyUI btn-circle send/stop, and the centering wrapper div (content centered while the scrollbar stays at the edge). - .env.example: kept PUBLIC_ENABLE_JOBS; dropped PUBLIC_SHOW_TOOL_CALL_DETAILS (auto-mode removed that debug feature). Regenerated agent-check annotations and the OpenAPI TS schema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QNY986xT2VCm7xngUg4tF
Low-risk quick-wins from automated review: - jobs/events.py: make iter_with_keepalive generic (TypeVar) so the bytes SSE consumer in chat/auto is typed correctly, not just the JobEvent case. - chat/auto/models.py: pin InboundMessage.role to Literal["user"] so the /message endpoint can't be handed a system/assistant role. - auto_run_store.ts: clear the optimistic working flag when an inject send fails (no burst started, so nothing else would clear it). - chat_session_store.ts: in handleAutoModeConsent, fall back to the last assistant message's trace before continuationTraceId so live-chat consent events with a null payload trace aren't dropped. - chat.svelte: hold consentPending through requestEnable() so a slow enable can't re-enable the button and double-dispatch. - chat_history_row.svelte: reveal the delete action on keyboard focus (group-focus-within), not just hover. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QNY986xT2VCm7xngUg4tF
If the user switches conversations while resyncOnLoad's resolve() or snapshot GET is in flight, the resolved stale run could be hydrated into and attached onto the newly-selected session. Re-check the active trace after each await and bail with a plain return (never detach()/loadSession, since the shared auto_run_store may already be owned by the new session). Addresses the resync race flagged in PR review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QNY986xT2VCm7xngUg4tF
Two fixes to the background-job/eval flow:
- Eval error log: EvalRunner.run() now accepts observers, and EvalJobWorker
passes one that forwards each failed dataset item's exception to
ctx.report_error. Previously only the error COUNT (Progress.errors) was
reported, so GET /api/jobs/{id}/errors showed "no errors recorded" even
when every item failed.
- Multi-job wait: add GET /api/jobs/wait?ids=a&ids=b&timeout=, backed by
JobRegistry.wait_many(), to block until ALL given jobs are terminal and
return their records. Lets a caller that kicked off several eval jobs (one
per run config) wait for them in one call. Declared before /api/jobs/{id}
so "wait" doesn't resolve to {id}. Pure observer, like /{id}/wait.
Regenerated the OpenAPI TS schema and agent-check annotations.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013QNY986xT2VCm7xngUg4tF
Surface the kiln_server context-usage gauge contract through the studio_server
proxy to the web UI (architecture §7):
- Add a ContextUsage Pydantic model and a context_usage field to
ChatSessionSnapshot in the GET /api/chat/sessions/{id} proxy, so a session
reopened from history renders the gauge. All ContextUsage fields are optional
so an older/partial upstream never 500s the proxy.
- Make ChatSessionSnapshot's extra="ignore" explicit (Pydantic v2 default):
unknown upstream keys (notably the server-only compacted_trace) are silently
dropped at the client boundary (functional_spec §7.3 containment). Documented
that extra="forbid" must NOT be used — it would raise/500 on a leaked key.
- SSE proxy needs no change: EventParser forwards complete lines verbatim, so
context_usage on the kiln_chat_trace event passes through untouched. Added a
passthrough test asserting this.
- Tests: round-trip context_usage; drop compacted_trace at the route and via a
direct model-config invariant; tolerate a missing context_usage.
- Regenerated api_schema.d.ts so the web UI types carry context_usage
(Phase 4 depends on it).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNJdjd98QPux2QhReKZY9z
Surface the kiln_server context_usage on the /assistant chat so users get a
glanceable, approximate signal of how full the conversation's context window is.
- streaming_chat.ts: parse context_usage off the kiln_chat_trace snapshot event
(normalizeContextUsage tolerates partial/missing upstream fields) and fire a
new onContextUsage callback alongside onChatTrace.
- chat_session_store.ts: contextUsage in PersistedChatSession (persisted to
sessionStorage), setContextUsage setter wired into the interactive stream, the
auto-run sink, and the resume/handoff path; set on history/resync load; cleared
on reset.
- session_messages.ts: hydrateSessionFromSnapshot returns contextUsage from the
session GET response (threaded through the history apply event into loadSession).
- auto_run_store.ts: AutoRunChatSink gains onContextUsage so the gauge updates
during auto-mode bursts too.
- context_usage_gauge.svelte: compact grey two-div bar (bg-base-content/10 track,
bg-base-content/30 fill, no color ramp) with the percent stacked above and a
tooltip carrying approximate {used}/{total} token counts; hidden when usage is
null. Mounted in the chat input footer row, right-aligned opposite Auto mode.
Tests: streaming_chat (parse + onContextUsage), chat_session_store (set/persist/
load/reset), session_messages (hydrate), auto_run_store sink, and a gauge
component test (markup, grey classes, fill width, percent-above, tooltip tokens,
hidden-when-null).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNJdjd98QPux2QhReKZY9z
…ence (Phase 5) Surface the server's pre-inference compaction window in the assistant UI by handling the new kiln_compaction_status SSE event (architecture.md §8.5, functional_spec.md §9.1). - streaming_chat.ts: parse kiln_compaction_status; add onCompactionStatus through StreamEventProcessor + the interactive/resume option surfaces. Set compacting on "started"; deliberately do NOT clear on "finished" (a fast or buffered started→finished pair would collapse the window) — the indicator is cleared by the first REAL assistant content (text/tool/exec-start/snapshot) and on error. - chat_session_store.ts: runtime-only compacting flag (not persisted) wired through the interactive, resume, and auto-run sink paths; cleared on start/finish/error/reset/loadSession/new-turn/idle/off. - auto_run_store.ts: onCompactionStatus on the sink + processor. - chat.svelte / chat_status_steps.svelte: render the SAME Thinking activity indicator markup with the summarizing label. Because compaction happens before any assistant message exists, render it as a standalone activity row keyed only on compacting (a per-message mount has no DOM anchor in that window), and suppress the empty-message Thinking cursor while compacting. - studio_server: test that kiln_compaction_status passes through the raw SSE proxy untouched (no model change). - Tests: processor set/clear behavior, store flag plumbing + non-persistence, the indicator copy, and an integration test rendering chat.svelte that the summarizing row is visible with compacting=true and NO assistant message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNJdjd98QPux2QhReKZY9z
Addresses Gemini review feedback on the context-usage gauge tooltip: - showTooltip awaits tick() before computePosition, so Floating UI measures the tooltip's real size instead of 0×0 (it's display:none until isVisible flips), fixing wrong initial placement. - Clears any prior autoUpdate registration before re-registering and bails if the tooltip was hidden during the await, avoiding a listener leak / re-init-on-hidden race. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNJdjd98QPux2QhReKZY9z
…ack) Addresses CodeRabbit feedback: the token-count tooltip was reachable only via mouse. Make the meter trigger focusable (tabindex=0) and show/ hide the tooltip on focus/blur as well as mouseenter/mouseleave, so keyboard users get the same info. Also clears the a11y mouse-events warning on this element. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNJdjd98QPux2QhReKZY9z
…pacting indicator
…ction KIL-727: assistant context-usage gauge + compaction indicator (app side)
…om:Kiln-AI/Kiln into leonard/kil-686-eval-job
Drop GET /api/jobs/{id}/wait; the bulk GET /api/jobs/wait?ids=...
endpoint covers the single-job case with one id. Update stale doc
references and regenerate the OpenAPI schema.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JHXGXnj1GKbFretPWeQWgP
Remove POST /api/jobs/{type} (and its wait=true/timeout inline-wait path):
production only runs evals, which use the typed POST /api/jobs/evals/run,
so the generic create endpoint had no real caller (only the temporary
test page + noop test worker). The NoopJobWorker stays as a registry-level
test fixture but is no longer registered in production.
Convert GET /api/jobs/wait to POST with a WaitForJobsRequest body, so the
job ids travel as JSON instead of repeated query params. With the generic
POST gone there's no route collision, so the ordering workaround is dropped.
Rewire test job creation to registry.create(), turn the temporary /jobs
test page into a real jobs panel (the jobs dialog links to it), and
regenerate the OpenAPI schema and agent-check annotations.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JHXGXnj1GKbFretPWeQWgP
Remove single-job wait endpoint in favor of bulk wait
A user message that arrives while an auto-mode burst is in flight was
appended as a raw user turn. The model would reply to it in plain text,
and that text-only turn settles the burst IDLE ("asked_user") — so a
quick aside from the user halted the autonomous run.
Wrap drained mid-burst messages in a system-reminder that tells the model
to weave its reply into ongoing work and keep going in the same turn,
stopping only if explicitly asked or the task is complete. Applies to both
injection sites (append-after-tools and drain-before-idle); the seed
message and idle-wake messages are unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01765EZhz9fQ1mgDvsqFKeQr
Auto mode runs unattended, so a single transient upstream failure (rate
limit, 5xx, connection blip) parked the whole burst IDLE until a human
returned. Retry such failures with bounded full-jitter exponential
backoff (max 10 attempts) before giving up.
- iter_upstream_round gains a defer_terminal_error mode that hands the
error payload + status to the caller instead of emitting it, so the
runner can decide to retry before anything reaches the client.
- RoundState carries the deferred payload, status code, and a retryable
flag (429/500/502/503/504; 4xx excluded). Only failures that streamed
no content are retried, so a retry can never duplicate output.
- New auto-mode-retry SSE event ({attempt, max_attempts, status_code?})
so the UI can show "retrying N/10…".
- Pre-response connection errors are caught in the runner and retried too.
The interactive path is unchanged (defer_terminal_error defaults False).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01765EZhz9fQ1mgDvsqFKeQr
Consume the new auto-mode-retry SSE event in auto_run_store: expose a
`retry` store ({attempt, max}) set on each retry event and cleared by the
next event of any other kind (recovered round, or settled idle/off). The
burst keeps reading as working during retries.
chat.svelte shows a transient "Connection issue — retrying N/M…" affordance
in the transcript (mirrors the reconnecting affordance), so an unattended
run reads as still-working rather than stalled or errored.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01765EZhz9fQ1mgDvsqFKeQr
Add a generic mechanism for job workers to publish static, descriptive properties about their work, derived once from params at create time (mirrors the compute_state pure-read pattern; stored dict-on-the-wire like progress_detail). The registry computes them in create() behind a guard so a failing describe() never breaks job creation. The eval worker is the first implementation: it publishes the eval name, run config (name, model, resolved prompt name, tool/skill counts), and judge (name, algorithm, model) — resolving prompt ids to names the same way the frontend does (generator label, custom id::, reused frozen task_run_config::, fine-tune, local frozen). The jobs table Details cell renders this as a run-config-style summary: "Eval: <name>" header, flat labeled property lines, then a muted date and id. Model names are formatted with the shared model-name helper; long values truncate with tooltips. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YDgjq9EjvM7aYwa2Ccv9L1
Hoist eval_job_properties(job) to a {#each}-level {@const} and gate the cell
on {#if p}, so p is type-narrowed to non-null inside the block. Removes the
?./??/&& noise on guaranteed-present fields (addresses PR review).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDgjq9EjvM7aYwa2Ccv9L1
…on branches
Address deep-review findings:
- Render judge_name in the Judge line ("Judge: <name> (<algorithm>)") instead of
the algorithm alone — the field was published, typed, and tested but unused.
- Add title={job.id} so the truncated id can be read in full (it's the value
users copy for support/debugging).
- Backend tests: cover the previously-dead prompt-resolution branches —
task_run_config:: reused-frozen (no own prompt), fine_tune_prompt::, the
raw-id fallback, and the MCP/non-agent run-config path.
- Registry tests: cover the describe() contract guard — typed-properties happy
path, wrong-type drop, and the raise-is-swallowed path — at the generic
(non-eval) worker level.
- Frontend test: assert the judge name+algorithm line, the model line, and the
non-zero "N available" tool/skill branch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YDgjq9EjvM7aYwa2Ccv9L1
Address deep code review findings:
- Stop pressed while retrying a persistent transient failure now settles
USER_STOPPED (so the supervisor publishes auto-mode-off) instead of
IDLE("error") with the flag left on — restoring the graceful-stop
contract. (Phase 1 moderate)
- Add the missing test for the central anti-duplication guard: a transport
failure after content was streamed must NOT be retried (re-POST would
duplicate output). New FakeUpstreamResponse.raise_transport_error_after_chunks
drives a mid-stream httpx.ReadError. (Phase 1/4 moderate)
- Add a stop-during-retry test; strengthen the retry-then-succeeds test to
assert attempt progression (1, 2) via structured parsing and that backoff
sleep actually ran. (Phase 4 mild)
- UI: "Connection issue" copy mislabeled 429/5xx retries → "Temporary issue",
and guard the degraded 0/0 render. (Phase 3 mild)
- Make _side_note_message a module-level function for consistent call style;
tighten the retry docstring and note the accepted non-idempotent re-POST.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01765EZhz9fQ1mgDvsqFKeQr
Wrap describe(), the contract type-check, and model_dump() in a single try/except so a bad payload or a serialization failure can never break create() — the guard's whole purpose is to keep describe failures non-fatal. Also make the missing-properties_model case explicit (log + drop) instead of relying on model_dump throwing. Adds a registry test for the undeclared-model drop path. (addresses CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YDgjq9EjvM7aYwa2Ccv9L1
Wrap the assertion in try/finally so DescribeWorker.gate is released even on failure, and await terminal status so the spawned job can't leak into teardown. (addresses CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YDgjq9EjvM7aYwa2Ccv9L1
feat(jobs): publish worker properties, render eval summary in jobs table
…est)
- Use round_state.trace_id_for_error for the transport-error give-up payload
so a trace id that streamed in before the error is reflected. (gemini)
- _side_note_message: `base.get("content") or ""` so an explicit None content
can't serialize to the literal "None". (gemini)
- test_stop_requested_during_retry: flip stop_requested from inside the patched
backoff sleep so the test exercises the in-retry transition (one retry
emitted) rather than the pre-run stop path. (coderabbit)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01765EZhz9fQ1mgDvsqFKeQr
…to leonard/kil-692-auto-mode-resilience
…anscripts Auto mode wraps a message injected mid-burst in a <system-reminder> "side note" before sending it upstream, so the framing is persisted in the trace. On reload (History restore / hard-refresh resync) the hydrated user message showed the raw tags, while the live echo renders the unwrapped content. Fix on the client, in hydrateSessionFromSnapshot, mirroring the existing stripAppUiContext handling for the <new_app_ui_context> header: add stripInternalFraming which strips both the app-UI context header and the auto-mode side-note <system-reminder>, so a hydrated transcript shows what the user actually typed. Keeping the framing in the persisted trace is intentional — it's a faithful record of the model's input — so no backend change is needed, and this also cleans up already-persisted conversations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8SUTZJG6Zjp5p5kdxLz6R
… on approval Addresses PR review (Gemini + CodeRabbit): - Data loss: maybeFlush()/sendQueuedNow() cleared queuedMessage before actuallySend() confirmed success, so a declined consent, failed auto-mode injection, or pending armed enable silently dropped the user's typed text. Add dispatchQueued(), which clears optimistically then restores the message to the front of the queue if the send is rejected. - send-now vs tool approval: sendQueuedNow() only treated status !== "ready" as in-flight. Add a toolApprovalWaiter guard so it won't start a competing request while a pending-tool continuation's approval is open (status can be "ready" there); the message stays queued and flushes when that continuation yields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8SUTZJG6Zjp5p5kdxLz6R
…scrollbar) The queued-message preview scrolled inside the text column, so its scrollbar sat to the LEFT of the edit/discard/send buttons and the text scrolled up under that row. Move the label + buttons into a full-width header row and make the scrollable text a full-width body below it, so the (thin, styled) scrollbar sits at the container's right edge instead of beside the buttons, and the buttons no longer overlap the scrolling text. Reuse the chat transcript's thin-scrollbar styling for the banner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8SUTZJG6Zjp5p5kdxLz6R
…plication)
On a hard refresh mid-burst, the injected ("queued") message was duplicated, and
it compounded on each refresh. Cause: resyncOnLoad keeps the optimistic
transcript (which already shows the message), and the re-attach's buffer replay
re-emits the user-message echo for the still-in-flight round — appendEchoedUserMessage
then appended it again.
Give each injected message a stable id and render the echo idempotently:
- Server: InboundMessage gets a generated id (am_…); echo_user_message /
format_user_message include it in the user-message SSE event. (InboundMessage is
internal — built from SendMessageRequest — so the OpenAPI schema is unchanged.)
- Client: ChatMessage carries echoId; the user-message handler forwards it; and
appendEchoedUserMessage skips appending (and opening a new assistant turn) when
a message with that echoId is already present — whether it came from the
optimistic transcript or a buffer replay. This keeps the earlier
keep-optimistic resync fix (no message loss) while eliminating the duplicate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8SUTZJG6Zjp5p5kdxLz6R
…p response loss) On a refresh mid-burst, part of the assistant response following a queued message was lost (recoverable only via History reload). Root cause: live rendering accumulates a whole burst into ONE assistant bubble, but on re-attach the buffer replay carries only the CURRENT in-flight round (the run buffer clears on every kiln_chat_trace) and no fresh assistant turn was opened for it — so the in-flight round's first flush (draft.parts = next, a full overwrite) clobbered the last existing bubble, destroying the rounds it held. Fix: - auto_run_store: on re-attach (resync / History restore), open a fresh assistant turn for the replayed in-flight round — lazily, on its first assistant content (or consumed by an injected-message echo, which opens its own turn), so an idle re-attach leaves no empty bubble. New optional attach(openInflightTurn) arg; resync and History restore pass it, the initial burst attach does not. - resyncOnLoad: revert the keep-optimistic branch back to always adopting the snapshot (per-round bubbles), which composes cleanly with the fresh in-flight turn — the snapshot owns completed rounds, the buffer replay owns the in-flight one. Keeping the multi-round optimistic bubble would instead duplicate the in-flight round. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8SUTZJG6Zjp5p5kdxLz6R
feat(chat): queue messages sent mid-turn with send-now / edit / cancel
…om:Kiln-AI/Kiln into leonard/kil-686-eval-job # Conflicts: # app/desktop/studio_server/jobs/models.py # app/web_ui/src/lib/api_schema.d.ts # app/web_ui/src/lib/components/jobs_table.svelte
Populate Field(description=...) on EvalJobParams, EvalJobResult, and EvalJobProperties in the eval worker. EvalJobParams is the typed request body for POST /api/jobs/evals/run, so its descriptions flow into the generated OpenAPI schema; the result/properties descriptions document the models carried as generic dicts on JobRecord. Regenerated api_schema.d.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Conw7oxpDtZTfhtEtVS9gT
…1536) * Add failing-train-examples eval API for reflective optimization New endpoint POST .../evals/{eval_id}/eval_config/{eval_config_id}/failing_train_examples that samples an Eval's train set, runs its judge (EvalConfig), and returns the datapoints that fail — with the judge's plaintext feedback — to feed GEPA-style reflection loops. - libs/core/.../eval/failing_examples.py: find_failing_train_examples() shuffles the train set (eval.train_set_filter_id), judges items in concurrent batches via the eval_config_eval path, and stops once `count` failures are found or `max_samples` items are judged ("oversample, return the requested amount"). An example fails only when all output scores fall below the bar (normalize_rating < threshold, default 0.75). Results are persisted as EvalRuns and reused on later calls. - eval_api.py: thin endpoint + request/response models, allowed for the Kiln assistant (ALLOW_AGENT) with a detailed OpenAPI description. Committed the agent-policy annotation so the assistant's policy lookup permits the call. - Regenerated app/web_ui api_schema.d.ts. - Tests: 13 core + 6 API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review: reuse loaded eval, resilient persistence, more tests - eval_api.py: resolve the eval config from the already-loaded eval instead of eval_config_from_id (which re-reads the task/eval from disk). Keeps the same 404. - failing_examples.py: wrap EvalRun persistence in its own try/except so a save failure logs and is skipped instead of crashing the whole concurrent batch; the computed scores are still returned. - tests: cover missing scores in example_fails, and judge errors being skipped (still counted as examined) during orchestration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Rework into a persistent Judge Job (config + run + poll) Replace the stateless failing_train_examples endpoint with a durable, runnable JudgeJob model per review feedback. A JudgeJob samples dataset items by tag, judges their existing outputs with an eval config (the judge), and records each item's pass/fail + the judge's feedback as child JudgeJobRuns — surfacing failing examples for reflective optimization. - datamodel/judge_job.py: JudgeJob (child of Task, parent of JudgeJobRun) with target_tags, eval_config_id, run_config_id (metadata), count/max_samples/threshold, latest_status, and an outcome summary. Registered on Task + datamodel __init__. - adapters/eval/judge_job_runner.py: JudgeJobRunner mirrors EvalRunner — judges in eval_config_eval mode, yields Progress for SSE, persists child runs, reuses cached results, and updates status/outcome (running -> succeeded/failed). Keeps the example_fails/score_passes/feedback helpers from the prior engine. - studio_server/judge_job_api.py: create / run (SSE) / create-and-run (SSE) / get / runs / list. The model id is the job id; GET is the poll. Registered in desktop_server; "Judge Jobs" tag added; agent-policy annotations regenerated (ALLOW_AGENT). Removed the standalone endpoint and its annotation; regenerated api_schema.d.ts. - Tests: datamodel, runner, and API (incl. SSE). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review: cancel on disconnect, validation, defensive hardening - judge_job_runner: catch GeneratorExit/CancelledError and mark the job `cancelled` (an SSE disconnect previously left it stuck in `running`); add a test. - judge_job_runner: harden score_passes (catch TypeError), feedback extraction (skip non-str values), and tag matching (None-safe). - judge_job_api: validate run_config_id (if provided) and reject a second run with 409 when the job is already running; add tests. - judge_job datamodel: constrain count/max_samples (ge=1) and threshold (0-1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Make judge jobs synchronous (per Leonard's feedback) The chat tool drops SSE event payloads and a judge minibatch is short, so SSE buys nothing here. Switch run / create-and-run to synchronous JSON and drop the job-status/poll/cancel layer. - judge_job datamodel: remove latest_status, outcome, JudgeJobStatus, JudgeJobOutcome. A JudgeJob is now just a config (+ JudgeJobRun children). - judge_job_runner: run() returns a JudgeJobRunResult (failing_runs + counts) instead of streaming Progress; no status writes, lock, or GeneratorExit handling. - judge_job_api: run / create-and-run block and return JudgeJobRunResponse (judge_job + failing_runs + counts). Counts are FYI for the caller, not persisted. Removed the SSE helper and the 409 already-running guard. - Tests + api_schema.d.ts updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address Leonard's review: rename task_run_id, collect per-item errors - Fix CI lint failure (ruff format test_judge_job.py). - Rename JudgeJobRun.dataset_id -> task_run_id (more descriptive of the TaskRun it points at). Updates model, runner, API, tests, and regenerated TS schema. - Collect per-item judge/save errors during a run and return them in the sync response (new JudgeJobItemError + JudgeJobRunResult.errors / run-response `errors`). Errors no longer silently swallowed: one bad item doesn't abort the run, the item is left un-persisted, and re-running retries only un-persisted items. A non-empty `errors` list signals partial success to the caller. Per-run aggregate counts stay derived (returned, not persisted) — the durable record remains the per-item JudgeJobRun children. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Judge jobs: full-coverage gate + return all judged runs Replace `count` (always early-stops) with `stop_after_failures: int | None`: - None (default) = judge the whole matching set up to max_samples (full coverage), so a val gate can pair results by task_run_id. - set = stop once that many failures are found (the cheap train-signal minibatch). Add `judged_runs` (every item judged this run, pass and fail) to the run result and response, keyed by task_run_id — the piece that makes a paired baseline-vs-candidate gate computable instead of an aggregate-count approximation. hit_cap now means coverage was capped (max_samples reached before stop_after_failures, or the matching set exceeded max_samples). Regenerated api_schema.d.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Judge jobs: retry transient per-item errors Generating/judging invokes a model, so transient rate-limit/connection blips are expected; without a retry they were collected as per-item errors and silently shrank coverage (skewing a gate). Route the judge call through _judge_with_retry, reusing the eval runner's transient-error classification (max_retries=2). Non- transient errors are still collected once, not retried. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Judge jobs: generate-and-judge mode (scoped candidate gate) Add generate_outputs: when true, run run_config_id on each tagged item to produce a fresh output and judge that — gating a candidate config scoped to the tagged items, in one sync call (no run_comparison, no full-eval scores). run_config_id is required in this mode. - Runner resolves the run config's RunConfigProperties + preloads its skills (mirrors EvalRunner.run_job) and instantiates the evaluator with them. - _judge_with_retry branches to run_task_and_eval; the fresh TaskRun is discarded (allow_saving=False) so the dataset is never polluted. - Skip the result cache in generate mode (generation is non-deterministic). - Record run_config_id on each JudgeJobRun for provenance; lower default concurrency when generating. Regenerated api_schema.d.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Judge jobs: address review (approval, robustness, eval-type, naming) Per Leonard's review: - Run / Create-and-run endpoints now require agent approval (agent_policy_require_approval) — they make model calls, so the bot shouldn't kick them off without consent (auto-mode still bypasses). Create + GETs stay ALLOW_AGENT. Regenerated the two annotation files. - run()'s asyncio.gather now uses return_exceptions=True: an unexpected throw in one item is converted to a per-item error instead of discarding the whole chunk's results. - Reject reference-answer evals when generate_outputs=false (no reference to compare a pre-existing output against; the judge would error per item). Validated in the API (422) and the runner constructor (last line of defense). - Rename eval_config_for_id -> eval_config_from_id to match the *_from_id convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Rename JudgeJob -> JudgeFeedbackBatch Avoid collision with the upcoming Job management system (and GEPA's "job"), per Leonard's review. Mechanical rename across the datamodel (JudgeFeedbackBatch / JudgeFeedbackBatchRun), runner, API (paths: /judge_jobs -> /judge_feedback_batches), the Task accessor (judge_feedback_batches()), OpenAPI tag, files, tests. Regenerated api_schema.d.ts and the agent-policy annotations (removed the stale judge_jobs files — the endpoints never shipped). Zero external callers, so no compat shim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Judge feedback batch: return continuous per-dimension scores (P1) The pass/fail bit (example_fails at the 0.75 threshold) discards the continuous signal, so a 2★→3★ improvement reads as zero gradient and a small val slice quantizes to a few loss levels. Add aggregate_normalized_scores() and return mean_normalized_scores (per-dimension mean over judged_runs, 0-1 higher=better) + mean_normalized_score (overall) in the run response — a usable gate/loss metric the caller no longer has to hand-compute from judged_runs[].scores. Part of the loss-function API review; P2/P3/P4/P7 are a separate follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * judge_feedback_batch: surface generation usage (tokens/cost/latency) Stop discarding the candidate run's Usage in generate_outputs mode. JudgeFeedbackBatchRun now carries a per-item `usage`; the runner aggregates it (aggregate_usage) into total_usage / mean_cost / mean_latency_ms on JudgeFeedbackBatchRunResult, and the run API exposes both per-run and aggregate usage. Lets the auto-optimize loop read deterministic cost/latency signals from the same call that gates quality. None on the judge-only path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * judge_feedback_batch: exponential backoff on transient/rate-limit retries Fixed 1s gaps don't let a 429 clear and just re-flood a throttled provider — back off progressively (delay, 2x, 4x) in _judge_with_retry, and raise the default retry_delay 1.0->2.0 so the server-side backoff is gentler. Surfaced by an auto-optimize smoke whose single-sample judge_feedback_batches calls hit provider rate limits (Cerebras/Gemini-Flash candidates). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(jobs): add judge_feedback_batch job type (parallel kick-offs) Make judge feedback batch runs job-backed, alongside the existing synchronous endpoints, so an agent can fire many gates and POST /api/jobs/wait on all of them at once instead of blocking on each synchronous call serially. - JudgeFeedbackBatchJobWorker wraps JudgeFeedbackBatchRunner unchanged (mirrors EvalJobWorker). Single-shot: progress reported once, supports_pause=False. - POST /api/jobs/judge_feedback_batch/run (two-segment, approval-gated) — the job-backed counterpart to POST /judge_feedback_batches/run. - Result carries the aggregate scores/usage/latency + the batch id; per-item runs (with the judge's feedback) are persisted, fetched via .../runs. - Worker tests + regenerated api_schema.d.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(jobs): agent-check annotation for the judge_feedback_batch job route The chat backend gates call_kiln_api via AgentPolicyLookup, which reads the static agent-check annotation JSON (dumped from the OpenAPI), NOT the live spec. Without an annotation the new POST /api/jobs/judge_feedback_batch/run is rejected as "not allowed", so the assistant falls back to the synchronous endpoint. Regenerated via `make annotations`; marks it allow + requires_approval. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Judge feedback batch: job-backed run of a pre-existing batch (eval parity) + deep-review fixes (#1542) * feat(jobs): judge_feedback_batch worker — stream progress, publish props, describe Bring the judge_feedback_batch job worker to parity with the eval job worker (the auto-mode e2e integration flagged three gaps): - Progress: the worker reported progress only once at the end, and used train_set_size as the denominator — so when the matching set exceeded max_samples the bar stalled below 100% even on full success. The runner now takes an optional progress_callback and streams (num_judged, error_count, planned_total) per judged chunk; the worker reports live progress and its final snapshot against the planned (capped) count min(train_set_size, max_samples), so success reaches total on full coverage. - Metadata: publish JudgeFeedbackBatchJobProperties via describe() (mirrors EvalJobProperties) — judge/eval names, algorithm, model, mode, run config, tags, max_samples — and render them in the jobs table (previously just a raw "Judge_feedback_batch" label). - Errors: unchanged wiring already surfaces per-item errors to the View Errors UI; improved message quality by unwrapping KilnRunError.original (mirrors EvalJobWorker._error_detail). - API models: add field descriptions to JudgeFeedbackBatchJobResult and the project_id/task_id params. Regenerated api_schema.d.ts. Tests: runner progress_callback streaming, worker describe() + capped-total progress, frontend judge-property rendering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZXQFWTXbstDj7vBihcx5N * fix(judge-feedback-batch): address deep code review (6 moderate + polish) Deep multi-phase review of the judge_feedback_batch feature (0 critical). Fixes: Moderate: - Progress double-counted errored items: the runner's num_judged already includes errored items, so reporting success=num_judged alongside a separate error count breached the processed=success+error contract and overshot 100%. Report success=num_judged-error_count in both the streamed callback and the final snapshot (worker), matching the eval reference's disjoint counters. - Gate-mode task_run_id pairing was defeated by the random shuffle when the tag set exceeded max_samples (two runs sampled disjoint subsets). Gate mode now selects deterministically (sorted by id) before capping; train-signal mode keeps the random minibatch. - Added a JudgeFeedbackBatch model_validator coupling generate_outputs=True to a required run_config_id and rejecting empty target_tags (defense in depth, mirrors EvalRun) — the API request model already validated both. - Documented that num_judged counts attempts (errors/cache included); use len(judged_runs) for a scored-item count. - Added runner tests for the empty candidate set and gate-mode hit_cap=True (set > max_samples), incl. proof that two gate runs cover the same ids. Polish: - Runner: save-error uses _error_detail; concurrency=max(1, concurrency) guard; mean-of-means comment; "Judge feedback batch" wording. - API: judge_feedback_batch_from_id 404 message fixed + accepts a preloaded task (removes the run endpoint's double task load); run docstring covers generate mode. - UI: surface eval_name + stop_after_failures in the jobs table. - Tests: worker run stub gets the full signature; judge-only describe() test. Full checks.sh green. Regenerated api_schema.d.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZXQFWTXbstDj7vBihcx5N * fix(judge-feedback-batch): report per-item errors live so View Errors works mid-run The streamed progress callback bumps progress.error per chunk, which makes the "View Errors" button appear while the job is still running. But the error MESSAGES were only written to the job error log in a post-run loop over result.errors — after runner.run() returned. So mid-run the button showed but the log was empty ("No error messages recorded"); the messages only appeared once the job completed. This diverged from the eval worker, which logs each error live via an observer. Add an error_callback to JudgeFeedbackBatchRunner.run(), fired the moment each per-item error is collected (both the judge/save error and the unexpected-throw paths), and wire the worker to write it to the error log immediately. Errors still appear in the returned result.errors for the synchronous endpoint (which passes no callback). Removed the worker's post-run loop to avoid double-logging. Tests: runner test asserting errors are delivered live (interleaved with progress, not batched to the end); worker stub updated to emit errors via the callback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZXQFWTXbstDj7vBihcx5N * fix(jobs-table): guard target_tags render against missing properties Defensive: use optional chaining on jp.target_tags so a job record with incomplete properties can't throw a TypeError and crash the whole jobs table render. (Per gemini-code-assist PR review.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZXQFWTXbstDj7vBihcx5N * fix(judge-feedback-batch): wrap batch-config save in git-sync context The batch config was written with a raw save_to_file() outside any atomic_write — in both the job worker and the synchronous create-and-run endpoint. Under auto git-sync that new (untracked) file is dirty working-tree state, so the runner's first per-item child write enters atomic_write, whose ensure_clean() treats it as a crashed session and stashes it away (include_untracked=True). Net effect: the batch config is never committed/pushed and is removed from the working tree, orphaning the committed child runs. Wrap the batch-config save in the same save_context used for the child writes (coalescing None -> no-op default_save_context) so it gets its own atomic_write / commit before the runner starts. The batch save and each child save are separate, non-nested atomic_write blocks, so re-entrancy is not a concern. Mirrors how EvalRunner already wraps every per-item write (the eval worker creates no parent entity, so it was already correct). - worker: judge_feedback_batch.py run() wraps the save. - sync endpoint: create_and_run_judge_feedback_batch (@no_write_lock) wraps it via build_save_context(request). - test: asserts the batch save goes through the git context and closes before the runner runs (enter -> exit -> run). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZXQFWTXbstDj7vBihcx5N * refactor(judge-feedback-batch): job runs a pre-existing batch (eval parity) Reshape the judge_feedback_batch job from "create-and-run" to "run an existing batch", mirroring EvalJobWorker. The batch config is created first via the synchronous POST .../judge_feedback_batches (which persists it under the git-sync write lock), then the job just runs it by id. Why: - The batch id is now caller-supplied and lives in job.params, so it's retrievable via GET /api/jobs/{id} even if the run fails — closing the gap where a create-and-run job only surfaced the minted id in its success result. - The worker no longer writes the config at all, so the earlier "wrap the batch save in the git-sync context" workaround is gone — the create endpoint owns that write. One less special case. - Structurally identical to the eval job (params carry the entity id; results live on disk), which sets up deriving a disk summary + a real compute_state later. Changes: - JudgeFeedbackBatchJobParams: now {project_id, task_id, judge_feedback_batch_id} (was a full CreateJudgeFeedbackBatchRequest). run()/describe() load the batch by id and read its config off disk. - Job result echoes judge_feedback_batch_id from params (no longer "created"). - Route docstring updated; regenerated api_schema.d.ts. - Tests: run loads an existing batch, missing-batch 404s, describe/props derived from the persisted batch. Dropped the now-obsolete config-save-wrap test. The synchronous create-and-run / run endpoints are left in place (now redundant with create + job) for callers not yet migrated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZXQFWTXbstDj7vBihcx5N * chore(judge-feedback-batch): deny agent access to the sync POST endpoints The synchronous create / run / create-and-run endpoints are being superseded by the create + job flow, so mark them DENY_AGENT (the chat assistant should go through POST /api/jobs/judge_feedback_batch/run for dashboard visibility). GETs stay ALLOW_AGENT. Drops the now-unused agent_policy_require_approval import. NOTE: not enforced until the agent-check annotation JSONs are regenerated (the policy lookup reads those, not the live spec). See PR notes re: keeping the create endpoint agent-callable for the new create-then-run-job flow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZXQFWTXbstDj7vBihcx5N * chore(judge-feedback-batch): keep create agent-callable; regenerate annotations Follow-up to the DENY_AGENT change: - Flip the sync create endpoint back to ALLOW_AGENT — the agent creates the batch here (cheap, no model calls) and then runs it via POST /api/jobs/judge_feedback_batch/run. Only the sync run / create-and-run stay DENY_AGENT (superseded by the job). - Regenerate the agent-check annotation JSONs so the policy is actually enforced (the chat backend reads the dumped JSONs, not the live spec) — fixes the check_api_bindings CI job. run / create-and-run now dump as "deny"; create and the GETs stay "allow". Final judge_feedback_batch agent policy: create -> allow run (existing, sync) -> deny create-and-run (sync) -> deny run as job (/api/jobs/...) -> allow + approval list / get / runs (GET) -> allow Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZXQFWTXbstDj7vBihcx5N --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(judge-feedback-batch): expose concurrency in job params Add an optional `concurrency` field to JudgeFeedbackBatchJobParams and forward it to JudgeFeedbackBatchRunner.run. Null keeps the runner's mode-aware default (5 when generating outputs, 25 when judging existing ones); values below 1 are clamped to 1 by the runner. Regenerated api_schema.d.ts and added a param-forwarding test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019w3CFeeewrMyMRdTv3SgQm * feat(judge-feedback-batch): validate concurrency >= 1 at schema level Add ge=1 to the concurrency param so invalid input returns a 422 up front (consistent with max_samples / stop_after_failures) instead of being silently clamped by the runner. Update the description, regenerate api_schema.d.ts, and add a validation test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019w3CFeeewrMyMRdTv3SgQm * feat(eval-job): expose concurrency in job params Add an optional `concurrency` field to EvalJobParams and forward it to EvalRunner.run, mirroring the judge feedback batch job param. Null keeps the runner's default (25); ge=1 rejects invalid values with a 422 at the API boundary (matching max_samples-style validation) rather than a runner-side ValueError. EvalRunner.run now accepts int | None and resolves the default internally, keeping 25 a single source of truth. Regenerated api_schema.d.ts; added forwarding + validation tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019w3CFeeewrMyMRdTv3SgQm * fix(eval-job): CI + review — params round-trip and single-source default - test_api: _EVAL_PARAMS now includes the defaulted concurrency=None, so the stored-params round-trip assertion in test_run_eval_job_creates_typed_eval_job matches again (the new optional field is serialized into job.params). - Address gemini review: extract DEFAULT_EVAL_CONCURRENCY (=25) in eval_runner as the single source of truth for the default, use it in EvalRunner.run and interpolate it into the EvalJobParams.concurrency field description so the doc can't drift from the runner default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019w3CFeeewrMyMRdTv3SgQm --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Leonard Q. Marcq <marcqleonard@gmail.com> Co-authored-by: Leonard Q. Marcq <leonard@getkiln.ai>
…to leonard/kil-686-eval-job
…etry schedule The retry machinery in EvalRunner and JudgeFeedbackBatchRunner never fired in production: the model adapter wraps provider exceptions in KilnRunError, so _is_retryable_error's isinstance checks against litellm error types never matched. Rate-limited items failed on the first attempt and went straight to the job error log. The classifier now unwraps KilnRunError and classifies the underlying error. Retry backoff is now exponential with +/-50% jitter (shared jittered_backoff_delay) instead of a fixed delay, so concurrent workers throttled at the same moment don't retry in lockstep and re-flood the provider. EvalRunner.run() exposes max_retries/retry_delay (keeping the historical defaults of 2 retries), and both background job workers override with a more patient schedule (4 retries, 5s base -> ~5/10/20/40s jittered waits). The RetryableError raised from run_job now carries the underlying provider message instead of KilnRunError's genericized text, keeping View Errors detailed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNMsZVT2b9PuANGAHkPyb1
…d error detail Addresses review: guards a (contract-violating) None original, and keeps the detail extraction from diverging from the classifier on nested wraps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNMsZVT2b9PuANGAHkPyb1
…etries Fix dead transient-error retries; patient backoff for background jobs
# Conflicts: # libs/core/kiln_ai/adapters/eval/test_eval_runner.py
Flag the judge feedback batch feature for design re-validation before it becomes API surface. Add a full design-review TODO block to the datamodel (judge_feedback_batch.py) capturing the open concerns, plus pointer TODOs in the runner and both test files so the concern is visible where the logic lives and each independently trips the debug_detector TODO check. Effective only against main: debug_detector runs on PRs/pushes to main and fails if any TODO/FIXME remains, so the feature cannot be merged toward main until the concerns are resolved and the markers removed. PRs targeting the feature branch are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uooi7AG76fWyV3v93smT23
…ker-uiskhj Merge blocker: gate judge_feedback_batch from main pending design review
Evals previously carried only two split filters: train_set_filter_id and
eval_set_filter_id (which is the test set - the name is legacy). This adds
a validation split so run methods can be compared on held-out data without
touching the test set.
- Add Eval.val_set_filter_id with the same lazy migration as train:
evals loaded without one get tag::val_{name_slug}
- Document on eval_set_filter_id that it is the test set and the name
is legacy
- Mint val_{name_slug} tags/filter ids in spec eval creation
(generate_spec_eval_tags / generate_spec_eval_filter_ids and both
callers: spec_api and copilot_api)
- Deal a val split in copilot synthetic dataset creation: eval keeps half
of the post-golden pool; the other half now splits 2/3 train, 1/3 val
- Regenerate the web client schema for the new Eval field
…in/val/test
Evals carry three dataset splits (train_set_filter_id, val_set_filter_id,
and eval_set_filter_id - the test set, whose name is legacy). This lets the
eval APIs target one of them; omitting the param keeps today's behavior.
- POST /api/jobs/evals/run accepts split. The name resolves to the eval's
stored filter id - 422 at job creation if the eval has no such split,
rather than a doomed background job - and is passed to EvalRunner as a
filter override. Job progress totals follow the split's item universe,
so resumes can't short-circuit against the wrong set.
- GET .../run_config/{run_config_id}/results accepts a split query param,
filtering returned EvalRuns by split membership at query time. No
EvalRun schema change, so it works retroactively on stored results;
omitted returns everything, as today.
- EvalRunner gains eval_set_filter_id_override, replacing the eval-set
filter when collecting task_run_eval jobs; ValueError in
eval_config_eval mode. The item-grained incremental cache is untouched,
so overlapping splits reuse already-scored items.
- Eval.filter_id_for_split maps split names to the stored filter ids
(test always resolves; train/val 422 when unset).
- Regenerate the web client schema for the new params.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
WalkthroughThis PR adds server-owned assistant auto mode, judge feedback batch evaluation, background job workers and waiting, validation dataset splits, retry handling, generated API contracts, and corresponding web UI state, dialogs, session grouping, queued messaging, and context-usage indicators. ChangesAssistant auto mode
Judge feedback batches and jobs
Evaluation split support
Supporting contracts and policy
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChatSessionStore
participant AutoRunStore
participant AutoChatAPI
participant AutoChatRegistry
participant AutoChatRunner
participant UpstreamChat
User->>ChatSessionStore: Enable auto mode or send message
ChatSessionStore->>AutoRunStore: requestEnable() or sendMessage()
AutoRunStore->>AutoChatAPI: POST enable or message
AutoChatAPI->>AutoChatRegistry: Start or enqueue run
AutoChatRegistry->>AutoChatRunner: Supervise burst
AutoChatRunner->>UpstreamChat: Stream chat round and tool continuation
AutoChatRunner-->>AutoChatRegistry: Publish SSE events
AutoChatRegistry-->>AutoRunStore: Stream per-run events
AutoRunStore-->>ChatSessionStore: Update transcript, state, and indicators
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
libs/core/kiln_ai/datamodel/judge_feedback_batch.py (1)
1-42: 📐 Maintainability & Code Quality | 🔵 TrivialUnresolved merge-blocker TODO.
This block states the design must be re-validated (and possibly deleted rather than merged) before it becomes API surface. Flagging so it isn't merged toward main while unresolved. Want me to open a tracking issue capturing the five points and the API-consolidation note?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/kiln_ai/datamodel/judge_feedback_batch.py` around lines 1 - 42, Resolve the merge-blocker TODO before exposing this feature: revalidate or remove the judge feedback batch design, replace tag-based selection with explicit item or split selection, extend EvalRun instead of introducing JudgeFeedbackBatchRun, reuse EvalRunner caching and complete paired coverage, and consolidate to one blessed API path. Remove the TODO only after these design decisions are implemented and the resulting API no longer duplicates or bypasses existing evaluation behavior.libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py (1)
273-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
readonly=Truefor consistency.
task.run_configs()is called withoutreadonly=Truehere, while the same lookup elsewhere in this file (line 327,self.task.runs(readonly=True)) and the mirrored lookup in the worker's_describe_sync(task.run_configs(readonly=True)) both use readonly reads. This is a validation-only read; keeping it readonly avoids unnecessary lock/write semantics.♻️ Suggested fix
- for rc in task.run_configs() + for rc in task.run_configs(readonly=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py` around lines 273 - 280, Update the run-config lookup in the batch runner to call task.run_configs with readonly=True, preserving the existing ID matching and None fallback behavior.app/desktop/studio_server/jobs/models.py (1)
306-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
describe()’s contract with its implementations.“Derived from params only” conflicts with
JudgeFeedbackBatchJobWorker.describe(), which intentionally reads the persisted batch. Document it as a pure, idempotent derivation from params and source-of-truth entities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/desktop/studio_server/jobs/models.py` around lines 306 - 315, Update the base `describe()` docstring to state that its result is a pure, idempotent derivation from `params` and any relevant source-of-truth entities, rather than from params alone. Preserve the existing no-side-effects, static-property, and `None`-when-empty contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/desktop/studio_server/chat/auto/runner.py`:
- Around line 248-251: Filter client_events in the stop-request path before
passing them to _format_tool_calls_pending_sse, excluding enable_auto_mode and
disable_auto_mode signal calls from the pending approval batch. Preserve the
existing USER_STOPPED status and return behavior, while allowing other tool
calls to remain in the emitted batch.
In `@app/desktop/studio_server/jobs/events.py`:
- Around line 59-83: Replace the None end-of-stream marker used by _feed and the
consumer loop with a private, distinct sentinel object or type, and check it by
identity. Preserve valid None subscription items by yielding them normally,
while still terminating iteration only when the sentinel is received.
In `@app/desktop/studio_server/jobs/registry.py`:
- Around line 578-604: Update wait_many() to retain each JobRecord returned by
_require during the initial validation loop, preserving the input order and
duplicates. After waiting for pending completion events, return these captured
records instead of re-indexing self._jobs, so concurrent deletion cannot cause a
KeyError.
In `@app/desktop/studio_server/judge_feedback_batch_api.py`:
- Around line 279-310: The run_judge_feedback_batch flow must validate the
stored run configuration before runner construction. In
run_judge_feedback_batch, call validate_run_config_id(task,
judge_feedback_batch.run_config_id) after resolving the batch and before
delegating to _run_judge_feedback_batch, preserving the existing clean 404
behavior for deleted configurations.
In `@libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py`:
- Around line 1-5: Resolve the outstanding design review documented in the
header of judge_feedback_batch.py before merging the runner. Specifically
validate the parallel JudgeFeedbackBatchRun/EvalRun storage, identical re-run
cache bypass, and paired-gate tag sampling behavior, then remove or update the
merge-blocker TODO only after those concerns are addressed.
- Around line 1-5: Replace the ambiguous en dash characters in the header TODO
comment of the judge feedback batch runner with standard hyphen-minus
characters, preserving the comment’s wording and meaning so Ruff RUF003 passes.
In `@libs/core/kiln_ai/datamodel/judge_feedback_batch.py`:
- Around line 16-28: Replace every Unicode multiplication sign “×” in the
comments of the judge feedback batch module with the ASCII character “x”,
including the eval_config/run_config/item descriptions, while preserving the
comment wording and meaning.
In `@specs/projects/assistant_auto_mode/functional_spec.md`:
- Around line 147-153: Resolve the conflicting Stop behavior between §4.4 and §7
of functional_spec.md by selecting one contract and applying it consistently
across both specification sections, the Stop handling implementation, and
related tests. Update the documented and verified behavior for in-flight output
and tool batches so they no longer describe different cancellation semantics.
In `@specs/projects/assistant_auto_mode/ui_design.md`:
- Around line 100-109: Update the persistent auto-mode consent copy near “Auto
mode turns off automatically” to state that auto-mode remains enabled and
becomes idle when the assistant asks a question or finishes. Clarify that it
only stops when the user clicks Stop or the assistant handles disable_auto_mode,
while preserving the existing warnings about server-side execution and potential
cost.
- Around line 13-16: Update the auto-mode UI specifications throughout the
document, including the sections around the listed ranges, to replace every
green indicator, dot, label, and “auto mode on” treatment with the Revision R1
DaisyUI primary blue treatment using text-primary, bg-primary, or primary as
appropriate. Preserve the requirement that state is also conveyed through text
and ensure no conflicting green references remain.
---
Nitpick comments:
In `@app/desktop/studio_server/jobs/models.py`:
- Around line 306-315: Update the base `describe()` docstring to state that its
result is a pure, idempotent derivation from `params` and any relevant
source-of-truth entities, rather than from params alone. Preserve the existing
no-side-effects, static-property, and `None`-when-empty contract.
In `@libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py`:
- Around line 273-280: Update the run-config lookup in the batch runner to call
task.run_configs with readonly=True, preserving the existing ID matching and
None fallback behavior.
In `@libs/core/kiln_ai/datamodel/judge_feedback_batch.py`:
- Around line 1-42: Resolve the merge-blocker TODO before exposing this feature:
revalidate or remove the judge feedback batch design, replace tag-based
selection with explicit item or split selection, extend EvalRun instead of
introducing JudgeFeedbackBatchRun, reuse EvalRunner caching and complete paired
coverage, and consolidate to one blessed API path. Remove the TODO only after
these design decisions are implemented and the resulting API no longer
duplicates or bypasses existing evaluation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7292650f-e0d3-4c4e-8e3c-0366073a6f56
📒 Files selected for processing (128)
app/desktop/desktop_server.pyapp/desktop/studio_server/chat/__init__.pyapp/desktop/studio_server/chat/auto/__init__.pyapp/desktop/studio_server/chat/auto/api.pyapp/desktop/studio_server/chat/auto/events.pyapp/desktop/studio_server/chat/auto/models.pyapp/desktop/studio_server/chat/auto/registry.pyapp/desktop/studio_server/chat/auto/runner.pyapp/desktop/studio_server/chat/auto/sse.pyapp/desktop/studio_server/chat/auto/test_api.pyapp/desktop/studio_server/chat/auto/test_fakes.pyapp/desktop/studio_server/chat/auto/test_iter_upstream_round.pyapp/desktop/studio_server/chat/auto/test_registry.pyapp/desktop/studio_server/chat/auto/test_runner.pyapp/desktop/studio_server/chat/constants.pyapp/desktop/studio_server/chat/routes.pyapp/desktop/studio_server/chat/stream_session.pyapp/desktop/studio_server/chat/test_routes.pyapp/desktop/studio_server/chat/test_sse_parser.pyapp/desktop/studio_server/chat/test_stream_session.pyapp/desktop/studio_server/copilot_api.pyapp/desktop/studio_server/eval_api.pyapp/desktop/studio_server/jobs/api.pyapp/desktop/studio_server/jobs/events.pyapp/desktop/studio_server/jobs/models.pyapp/desktop/studio_server/jobs/registry.pyapp/desktop/studio_server/jobs/test_api.pyapp/desktop/studio_server/jobs/test_registry.pyapp/desktop/studio_server/jobs/workers/eval.pyapp/desktop/studio_server/jobs/workers/judge_feedback_batch.pyapp/desktop/studio_server/jobs/workers/test_eval.pyapp/desktop/studio_server/jobs/workers/test_judge_feedback_batch.pyapp/desktop/studio_server/judge_feedback_batch_api.pyapp/desktop/studio_server/test_copilot_api.pyapp/desktop/studio_server/test_eval_api.pyapp/desktop/studio_server/test_judge_feedback_batch_api.pyapp/desktop/studio_server/utils/copilot_utils.pyapp/desktop/studio_server/utils/test_copilot_utils.pyapp/web_ui/.env.exampleapp/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/chat/auto_run_store.test.tsapp/web_ui/src/lib/chat/auto_run_store.tsapp/web_ui/src/lib/chat/chat_history_apply.tsapp/web_ui/src/lib/chat/chat_session_store.test.tsapp/web_ui/src/lib/chat/chat_session_store.tsapp/web_ui/src/lib/chat/session_grouping.test.tsapp/web_ui/src/lib/chat/session_grouping.tsapp/web_ui/src/lib/chat/session_messages.test.tsapp/web_ui/src/lib/chat/session_messages.tsapp/web_ui/src/lib/chat/streaming_chat.test.tsapp/web_ui/src/lib/chat/streaming_chat.tsapp/web_ui/src/lib/components/jobs_table.svelteapp/web_ui/src/lib/components/jobs_table.test.tsapp/web_ui/src/lib/stores/jobs_api.test.tsapp/web_ui/src/lib/stores/jobs_api.tsapp/web_ui/src/lib/stores/jobs_store.test.tsapp/web_ui/src/lib/ui/context_usage_gauge.svelteapp/web_ui/src/lib/ui/context_usage_gauge.test.tsapp/web_ui/src/routes/(app)/assistant/+page.svelteapp/web_ui/src/routes/(app)/assistant/auto_mode_consent_dialog.svelteapp/web_ui/src/routes/(app)/assistant/auto_mode_consent_dialog.test.tsapp/web_ui/src/routes/(app)/assistant/auto_mode_stop_dialog.svelteapp/web_ui/src/routes/(app)/assistant/chat.svelteapp/web_ui/src/routes/(app)/assistant/chat_compacting_indicator.test.tsapp/web_ui/src/routes/(app)/assistant/chat_history.svelteapp/web_ui/src/routes/(app)/assistant/chat_history_row.svelteapp/web_ui/src/routes/(app)/assistant/chat_queued_message.test.tsapp/web_ui/src/routes/(app)/assistant/chat_status_steps.svelteapp/web_ui/src/routes/(app)/assistant/chat_status_steps.test.tsapp/web_ui/src/routes/(app)/assistant/chat_step_group.svelteapp/web_ui/src/routes/(app)/jobs/+page.sveltelibs/core/kiln_ai/adapters/eval/eval_runner.pylibs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.pylibs/core/kiln_ai/adapters/eval/test_eval_runner.pylibs/core/kiln_ai/adapters/eval/test_judge_feedback_batch_runner.pylibs/core/kiln_ai/datamodel/__init__.pylibs/core/kiln_ai/datamodel/eval.pylibs/core/kiln_ai/datamodel/judge_feedback_batch.pylibs/core/kiln_ai/datamodel/task.pylibs/core/kiln_ai/datamodel/test_eval_model.pylibs/core/kiln_ai/datamodel/test_judge_feedback_batch.pylibs/core/kiln_ai/datamodel/tool_id.pylibs/core/kiln_ai/tools/built_in_tools/disable_auto_mode_tool.pylibs/core/kiln_ai/tools/built_in_tools/enable_auto_mode_tool.pylibs/core/kiln_ai/tools/built_in_tools/test_disable_auto_mode_tool.pylibs/core/kiln_ai/tools/built_in_tools/test_enable_auto_mode_tool.pylibs/core/kiln_ai/tools/test_tool_registry.pylibs/core/kiln_ai/tools/tool_registry.pylibs/core/kiln_ai/utils/async_job_runner.pylibs/core/kiln_ai/utils/test_async_job_runner.pylibs/server/kiln_server/server.pylibs/server/kiln_server/spec_api.pylibs/server/kiln_server/test_spec_api.pylibs/server/kiln_server/utils/agent_checks/annotations/get_api_chat_auto_resolve.jsonlibs/server/kiln_server/utils/agent_checks/annotations/get_api_chat_auto_run_id_events.jsonlibs/server/kiln_server/utils/agent_checks/annotations/get_api_chat_auto_sessions.jsonlibs/server/kiln_server/utils/agent_checks/annotations/get_api_projects_project_id_tasks_task_id_evals_eval_id_eval_config_eval_config_id_run_comparison.jsonlibs/server/kiln_server/utils/agent_checks/annotations/get_api_projects_project_id_tasks_task_id_evals_eval_id_run_calibration.jsonlibs/server/kiln_server/utils/agent_checks/annotations/get_api_projects_project_id_tasks_task_id_judge_feedback_batches.jsonlibs/server/kiln_server/utils/agent_checks/annotations/get_api_projects_project_id_tasks_task_id_judge_feedback_batches_judge_feedback_batch_id.jsonlibs/server/kiln_server/utils/agent_checks/annotations/get_api_projects_project_id_tasks_task_id_judge_feedback_batches_judge_feedback_batch_id_runs.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_chat_auto_decline.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_chat_auto_enable.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_chat_auto_run_id_message.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_chat_auto_run_id_stop.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_jobs_evals_run.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_jobs_judge_feedback_batch_run.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_jobs_wait.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_projects_project_id_tasks_task_id_judge_feedback_batches.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_projects_project_id_tasks_task_id_judge_feedback_batches_judge_feedback_batch_id_run.jsonlibs/server/kiln_server/utils/agent_checks/annotations/post_api_projects_project_id_tasks_task_id_judge_feedback_batches_run.jsonlibs/server/kiln_server/utils/spec_utils.pylibs/server/kiln_server/utils/test_spec_utils.pyspecs/projects/assistant_auto_mode/architecture.mdspecs/projects/assistant_auto_mode/functional_spec.mdspecs/projects/assistant_auto_mode/implementation_plan.mdspecs/projects/assistant_auto_mode/phase_plans/phase_1.mdspecs/projects/assistant_auto_mode/phase_plans/phase_10.mdspecs/projects/assistant_auto_mode/phase_plans/phase_2.mdspecs/projects/assistant_auto_mode/phase_plans/phase_3.mdspecs/projects/assistant_auto_mode/phase_plans/phase_4.mdspecs/projects/assistant_auto_mode/phase_plans/phase_5.mdspecs/projects/assistant_auto_mode/phase_plans/phase_6.mdspecs/projects/assistant_auto_mode/phase_plans/phase_7.mdspecs/projects/assistant_auto_mode/phase_plans/phase_8.mdspecs/projects/assistant_auto_mode/phase_plans/phase_9.mdspecs/projects/assistant_auto_mode/project_overview.mdspecs/projects/assistant_auto_mode/ui_design.md
💤 Files with no reviewable changes (2)
- app/web_ui/.env.example
- app/web_ui/src/lib/stores/jobs_store.test.ts
Comment on lines
+248
to
+251
| if self.stop_requested and client_events: | ||
| self._emit(_format_tool_calls_pending_sse(client_events)) | ||
| self.status = AutoRunStatus.USER_STOPPED | ||
| return |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'ENABLE_AUTO_MODE_TOOL_NAME|execute_tool_batch|Unknown tool name' app/desktop/studio_server/chat/routes.py app/desktop/studio_server/chat/stream_session.pyRepository: Kiln-AI/Kiln
Length of output: 7167
🏁 Script executed:
#!/bin/bash
set -eu
echo "== runner.py outline =="
ast-grep outline app/desktop/studio_server/chat/auto/runner.py --view compact || true
echo "== runner.py relevant lines 150-330 =="
sed -n '150,330p' app/desktop/studio_server/chat/auto/runner.py | nl -ba -v150
echo "== stream_session.py relevant lines 430-585 =="
sed -n '430,585p' app/desktop/studio_server/chat/stream_session.py | nl -ba -v430
echo "== stream_session.py execute_tool_batch error handling =="
sed -n '680,710p' app/desktop/studio_server/chat/stream_session.py | nl -ba -v680
echo "== references to format_tool_calls_pending_sse and enable/disable tool names =="
rg -n -C3 '_format_tool_calls_pending_sse|ENABLE_AUTO_MODE_TOOL_NAME|DISABLE_AUTO_MODE_TOOL_NAME|toolName|toolCallId' app/desktop/studio_server/chat/auto/runner.py app/desktop/studio_server/chat/stream_session.pyRepository: Kiln-AI/Kiln
Length of output: 404
🏁 Script executed:
#!/bin/bash
set -eu
echo "== runner.py outline =="
ast-grep outline app/desktop/studio_server/chat/auto/runner.py --view expanded || true
echo "== runner.py lines 150-330 =="
sed -n '150,330p' app/desktop/studio_server/chat/auto/runner.py | awk '{printf "%5d\t%s\n", NR+149, $0}'
echo "== stream_session.py lines 430-585 =="
sed -n '430,585p' app/desktop/studio_server/chat/stream_session.py | awk '{printf "%5d\t%s\n", NR+429, $0}'
echo "== stream_session.py execute_tool_batch error handling =="
sed -n '680,710p' app/desktop/studio_server/chat/stream_session.py | awk '{printf "%5d\t%s\n", NR+679, $0}'
echo "== relevant references =="
rg -n -C3 '_format_tool_calls_pending_sse|ENABLE_AUTO_MODE_TOOL_NAME|DISABLE_AUTO_MODE_TOOL_NAME|toolName|toolCallId' app/desktop/studio_server/chat/auto/runner.py app/desktop/studio_server/chat/stream_session.pyRepository: Kiln-AI/Kiln
Length of output: 39275
Exclude auto-mode signal calls from the stop path’s pending approval batch.
At runner.py:248, client_events is emitted without filtering enable_auto_mode/disable_auto_mode, but stream_session.py only intercepts those signals before /execute-tools runs them. If this batch reaches the browser for approval, the signal tool calls would be sent through execute_tool, fail the resolver, and return Unknown tool name instead of being handled as auto-mode signals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/desktop/studio_server/chat/auto/runner.py` around lines 248 - 251, Filter
client_events in the stop-request path before passing them to
_format_tool_calls_pending_sse, excluding enable_auto_mode and disable_auto_mode
signal calls from the pending approval batch. Preserve the existing USER_STOPPED
status and return behavior, while allowing other tool calls to remain in the
emitted batch.
Comment on lines
+59
to
+83
| local_queue: asyncio.Queue[_T | None] = asyncio.Queue() | ||
|
|
||
| async def _feed() -> None: | ||
| try: | ||
| async for event in subscription: | ||
| await local_queue.put(event) | ||
| finally: | ||
| # End-of-stream sentinel. Also reached if the feeder is cancelled | ||
| # during teardown — harmless, since the consumer is gone by then. | ||
| local_queue.put_nowait(None) | ||
|
|
||
| feeder = asyncio.create_task(_feed()) | ||
| try: | ||
| while True: | ||
| try: | ||
| item = await asyncio.wait_for( | ||
| local_queue.get(), timeout=timeout_seconds | ||
| ) | ||
| except asyncio.TimeoutError: | ||
| # Cancels only the throwaway get() above; the feeder (and thus | ||
| # the subscription) is untouched and keeps draining. | ||
| yield KEEPALIVE_PING | ||
| continue | ||
| if item is None: | ||
| break |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a distinct end-of-stream sentinel.
_T is unconstrained, so a valid None item terminates this generic iterator instead of being yielded. Use a private sentinel object/type and identity check rather than None.
Proposed fix
+class _EndOfStream:
+ pass
+
+_END_OF_STREAM = _EndOfStream()
+
- local_queue: asyncio.Queue[_T | None] = asyncio.Queue()
+ local_queue: asyncio.Queue[_T | _EndOfStream] = asyncio.Queue()
...
- local_queue.put_nowait(None)
+ local_queue.put_nowait(_END_OF_STREAM)
...
- if item is None:
+ if item is _END_OF_STREAM:
break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local_queue: asyncio.Queue[_T | None] = asyncio.Queue() | |
| async def _feed() -> None: | |
| try: | |
| async for event in subscription: | |
| await local_queue.put(event) | |
| finally: | |
| # End-of-stream sentinel. Also reached if the feeder is cancelled | |
| # during teardown — harmless, since the consumer is gone by then. | |
| local_queue.put_nowait(None) | |
| feeder = asyncio.create_task(_feed()) | |
| try: | |
| while True: | |
| try: | |
| item = await asyncio.wait_for( | |
| local_queue.get(), timeout=timeout_seconds | |
| ) | |
| except asyncio.TimeoutError: | |
| # Cancels only the throwaway get() above; the feeder (and thus | |
| # the subscription) is untouched and keeps draining. | |
| yield KEEPALIVE_PING | |
| continue | |
| if item is None: | |
| break | |
| class _EndOfStream: | |
| pass | |
| _END_OF_STREAM = _EndOfStream() | |
| local_queue: asyncio.Queue[_T | _EndOfStream] = asyncio.Queue() | |
| async def _feed() -> None: | |
| try: | |
| async for event in subscription: | |
| await local_queue.put(event) | |
| finally: | |
| # End-of-stream sentinel. Also reached if the feeder is cancelled | |
| # during teardown — harmless, since the consumer is gone by then. | |
| local_queue.put_nowait(_END_OF_STREAM) | |
| feeder = asyncio.create_task(_feed()) | |
| try: | |
| while True: | |
| try: | |
| item = await asyncio.wait_for( | |
| local_queue.get(), timeout=timeout_seconds | |
| ) | |
| except asyncio.TimeoutError: | |
| # Cancels only the throwaway get() above; the feeder (and thus | |
| # the subscription) is untouched and keeps draining. | |
| yield KEEPALIVE_PING | |
| continue | |
| if item is _END_OF_STREAM: | |
| break |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/desktop/studio_server/jobs/events.py` around lines 59 - 83, Replace the
None end-of-stream marker used by _feed and the consumer loop with a private,
distinct sentinel object or type, and check it by identity. Preserve valid None
subscription items by yielding them normally, while still terminating iteration
only when the sentinel is received.
Comment on lines
+578
to
+604
| async def wait_many( | ||
| self, job_ids: list[str], timeout: float | None = None | ||
| ) -> list[JobRecord]: | ||
| """Observe several jobs until ALL reach a terminal state, then return | ||
| their records in the order given. | ||
|
|
||
| Same pure-observer semantics as wait(): cancelling this await tears down | ||
| only the awaiter, never the jobs. The single shared `timeout` bounds the | ||
| whole set — on timeout asyncio.wait_for raises asyncio.TimeoutError even | ||
| if some jobs already finished. Raises JobNotFoundError if any id is | ||
| unknown (validated up front, before any waiting). Duplicate ids are fine. | ||
| """ | ||
| # Validate every id and register its event up front, with no await in | ||
| # between, so there's no race window where a job goes terminal before we | ||
| # start observing it (mirrors wait()). | ||
| pending_events: list[asyncio.Event] = [] | ||
| for job_id in job_ids: | ||
| job = self._require(job_id) | ||
| ev = self._completion_events.setdefault(job_id, asyncio.Event()) | ||
| if not job.status.is_terminal: | ||
| pending_events.append(ev) | ||
| if pending_events: | ||
| await asyncio.wait_for( | ||
| asyncio.gather(*(ev.wait() for ev in pending_events)), timeout | ||
| ) | ||
| return [self._jobs[job_id] for job_id in job_ids] | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
wait_many() can raise KeyError if a job is deleted concurrently while other jobs in the batch are still pending.
The validation loop calls self._require(job_id) but discards the returned JobRecord reference (overwritten each iteration). After gather() completes, the function re-indexes self._jobs[job_id] for every id. If a job in the list reaches terminal state and is deleted (via DELETE /api/jobs/{id}, which only requires is_terminal) while wait_many is still awaiting the other jobs in the batch, that lookup raises a raw KeyError instead of returning the already-observed terminal record — crashing POST /api/jobs/wait even though every job did reach a terminal state. wait() avoids this by returning the single reference it captured at the start instead of re-indexing self._jobs.
🔒 Proposed fix: return captured references instead of re-indexing
async def wait_many(
self, job_ids: list[str], timeout: float | None = None
) -> list[JobRecord]:
...
+ jobs: dict[str, JobRecord] = {}
pending_events: list[asyncio.Event] = []
for job_id in job_ids:
job = self._require(job_id)
+ jobs[job_id] = job
ev = self._completion_events.setdefault(job_id, asyncio.Event())
if not job.status.is_terminal:
pending_events.append(ev)
if pending_events:
await asyncio.wait_for(
asyncio.gather(*(ev.wait() for ev in pending_events)), timeout
)
- return [self._jobs[job_id] for job_id in job_ids]
+ return [jobs[job_id] for job_id in job_ids]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def wait_many( | |
| self, job_ids: list[str], timeout: float | None = None | |
| ) -> list[JobRecord]: | |
| """Observe several jobs until ALL reach a terminal state, then return | |
| their records in the order given. | |
| Same pure-observer semantics as wait(): cancelling this await tears down | |
| only the awaiter, never the jobs. The single shared `timeout` bounds the | |
| whole set — on timeout asyncio.wait_for raises asyncio.TimeoutError even | |
| if some jobs already finished. Raises JobNotFoundError if any id is | |
| unknown (validated up front, before any waiting). Duplicate ids are fine. | |
| """ | |
| # Validate every id and register its event up front, with no await in | |
| # between, so there's no race window where a job goes terminal before we | |
| # start observing it (mirrors wait()). | |
| pending_events: list[asyncio.Event] = [] | |
| for job_id in job_ids: | |
| job = self._require(job_id) | |
| ev = self._completion_events.setdefault(job_id, asyncio.Event()) | |
| if not job.status.is_terminal: | |
| pending_events.append(ev) | |
| if pending_events: | |
| await asyncio.wait_for( | |
| asyncio.gather(*(ev.wait() for ev in pending_events)), timeout | |
| ) | |
| return [self._jobs[job_id] for job_id in job_ids] | |
| async def wait_many( | |
| self, job_ids: list[str], timeout: float | None = None | |
| ) -> list[JobRecord]: | |
| """Observe several jobs until ALL reach a terminal state, then return | |
| their records in the order given. | |
| Same pure-observer semantics as wait(): cancelling this await tears down | |
| only the awaiter, never the jobs. The single shared `timeout` bounds the | |
| whole set — on timeout asyncio.wait_for raises asyncio.TimeoutError even | |
| if some jobs already finished. Raises JobNotFoundError if any id is | |
| unknown (validated up front, before any waiting). Duplicate ids are fine. | |
| """ | |
| # Validate every id and register its event up front, with no await in | |
| # between, so there's no race window where a job goes terminal before we | |
| # start observing it (mirrors wait()). | |
| jobs: dict[str, JobRecord] = {} | |
| pending_events: list[asyncio.Event] = [] | |
| for job_id in job_ids: | |
| job = self._require(job_id) | |
| jobs[job_id] = job | |
| ev = self._completion_events.setdefault(job_id, asyncio.Event()) | |
| if not job.status.is_terminal: | |
| pending_events.append(ev) | |
| if pending_events: | |
| await asyncio.wait_for( | |
| asyncio.gather(*(ev.wait() for ev in pending_events)), timeout | |
| ) | |
| return [jobs[job_id] for job_id in job_ids] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/desktop/studio_server/jobs/registry.py` around lines 578 - 604, Update
wait_many() to retain each JobRecord returned by _require during the initial
validation loop, preserving the input order and duplicates. After waiting for
pending completion events, return these captured records instead of re-indexing
self._jobs, so concurrent deletion cannot cause a KeyError.
Comment on lines
+279
to
+310
| async def run_judge_feedback_batch( | ||
| request: Request, | ||
| project_id: Annotated[ | ||
| str, Path(description="The unique identifier of the project.") | ||
| ], | ||
| task_id: Annotated[ | ||
| str, | ||
| Path(description="The unique identifier of the task within the project."), | ||
| ], | ||
| judge_feedback_batch_id: Annotated[ | ||
| str, Path(description="The unique identifier of the judge feedback batch.") | ||
| ], | ||
| ) -> JudgeFeedbackBatchRunResponse: | ||
| """Run a judge feedback batch: sample tagged dataset items, judge their outputs (existing, or | ||
| freshly generated when the batch has generate_outputs=true), and return the failing examples | ||
| + feedback. | ||
|
|
||
| Runs synchronously and returns once judging completes. Each result is persisted as a child | ||
| run (fetch them later via `GET /judge_feedback_batches/{id}/runs`); the returned counts | ||
| (num_judged, failing_count, train_set_size, hit_cap) and any per-item `errors` are FYI for | ||
| the caller's loop. Errors don't abort the run — partial results are still persisted, and | ||
| re-running the job retries only the un-persisted (errored or not-yet-judged) items. | ||
| """ | ||
| task = task_from_id(project_id, task_id) | ||
| judge_feedback_batch = judge_feedback_batch_from_id( | ||
| project_id, task_id, judge_feedback_batch_id, task=task | ||
| ) | ||
| eval_config = eval_config_from_id(task, judge_feedback_batch.eval_config_id) | ||
| validate_judge_eval(eval_config, judge_feedback_batch.generate_outputs) | ||
| return await _run_judge_feedback_batch( | ||
| judge_feedback_batch, eval_config, request | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)judge_feedback_batch_api\.py$|(^|/)judge_feedback_batch\.py$|(^|/)runner\.py$|exception|handler|404|HTTPException' | sed -n '1,200p'
echo "== outline target =="
if [ -f app/desktop/studio_server/judge_feedback_batch_api.py ]; then
ast-grep outline app/desktop/studio_server/judge_feedback_batch_api.py --match run_judge_feedback_batch --view expanded || true
fi
echo "== relevant target section =="
sed -n '250,330p' app/desktop/studio_server/judge_feedback_batch_api.py
echo "== function definitions usages validate_run_config_id =="
rg -n "validate_run_config_id|eval_config_from_id|JudgeFeedbackBatchRunner|run_config_id" app/desktop/studio_server -S
echo "== target runner references =="
rg -n "class JudgeFeedbackBatchRunner|def __init__|run_config_id|ValueError" app/desktop/studio_server -SRepository: Kiln-AI/Kiln
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate exception handlers / error routes =="
rg -n "Exception\(|FastAPI\(|add_exception_handler|HTTPException|status_code=404|raise ValueError|raise HTTPException" app/desktop -S | sed -n '1,240p'
echo "== inspect worker run section =="
sed -n '200,240p' app/desktop/studio_server/jobs/workers/judge_feedback_batch.pyRepository: Kiln-AI/Kiln
Length of output: 25501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== judge_feedback_batch_api helper section =="
sed -n '1,85p' app/desktop/studio_server/judge_feedback_batch_api.py
echo "== runner imports / constructor location =="
rg -n "class JudgeFeedbackBatchRunner|def __init__|def run\(" -S . | sed -n '1,80p'
echo "== candidate runner filenames =="
git ls-files | rg 'judge_feedback_batch_runner|judge.*feedback.*runner|eval.*runner|fine_tune_run_config_id|run_config' | sed -n '1,200p'
echo "== global startup / main files =="
git ls-files | rg 'app/.*main\.py$|app/.*desktop|app/desktop/main\.py' | sed -n '1,100p'
sed -n '1,220p' app/desktop/main.py 2>/dev/null || trueRepository: Kiln-AI/Kiln
Length of output: 21822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== judge_feedback_batch_runner constructor =="
sed -n '1,240p' libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py
echo "== app entrypoints exception handler search =="
python3 - <<'PY'
import pathlib
for p in pathlib.Path('app').rglob('*.py'):
txt = p.read_text(errors='ignore')
if 'add_exception_handler' in txt or 'from fastapi import FastAPI, HTTPException' in txt and 'add_exception_handler' in txt:
lines = txt.splitlines()
for i,l in enumerate(lines):
if 'add_exception_handler' in l or 'FastAPI(' in l:
print(f"{p}:{i+1}:{l.strip()}")
PY
echo "== judge_feedback_batch_runner raise valueerror lines =="
rg -n "ValueError|run_config_id|model_id" libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py -S
echo "== deterministic parse of runner init logic around run_config_id =="
python3 - <<'PY'
from pathlib import Path
p = Path('libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py')
lines = p.read_text().splitlines()
for i,l in enumerate(lines, 1):
if 'run_config_id' in l or 'ValueError' in l:
lo=max(1,i-5); hi=min(len(lines),i+5)
print(f"\n--- {p}:{lo}-{hi} ---")
for n in range(lo,hi+1):
print(f"{n}: {lines[n-1]}")
PYRepository: Kiln-AI/Kiln
Length of output: 18025
Validate run_config_id before constructing JudgeFeedbackBatchRunner.
run_judge_feedback_batch() skips validate_run_config_id(), unlike batch creation, create-and-run, and the worker flow. If judge_feedback_batch.generate_outputs=true and the stored run_config_id was deleted, the runner raises a plain ValueError instead of the clean 404 used by the other resource-resolvers in this file. Add validate_run_config_id(task, judge_feedback_batch.run_config_id) before the runner is constructed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/desktop/studio_server/judge_feedback_batch_api.py` around lines 279 -
310, The run_judge_feedback_batch flow must validate the stored run
configuration before runner construction. In run_judge_feedback_batch, call
validate_run_config_id(task, judge_feedback_batch.run_config_id) after resolving
the batch and before delegating to _run_judge_feedback_batch, preserving the
existing clean 404 behavior for deleted configurations.
Comment on lines
+1
to
+5
| # TODO (merge blocker — do not merge toward main until resolved): this runner is under design | ||
| # review. Concerns 3–5 are implemented here — the parallel eval-result store (JudgeFeedbackBatchRun | ||
| # vs EvalRun), the cache bypass on identical re-runs, and the paired-gate logic retrofitted onto | ||
| # tag-based sampling. Full write-up and rationale in the header of | ||
| # kiln_ai/datamodel/judge_feedback_batch.py. Resolve there before merging toward main. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Explicit merge blocker — do not lose track of this before merging.
The header states outright: this runner is under design review. Concerns 3–5 are implemented here — the parallel eval-result store (JudgeFeedbackBatchRun vs EvalRun), the cache bypass on identical re-runs, and the paired-gate logic retrofitted onto tag-based sampling. This is a self-declared blocker; ensure the linked design review in judge_feedback_batch.py's header is resolved before this lands on main.
🧰 Tools
🪛 GitHub Actions: Format and Lint / 0_Format and Lint Python.txt
[error] 2-4: ruff check (RUF003): Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
🪛 GitHub Actions: Format and Lint / Format and Lint Python
[error] 2-2: Ruff (RUF003): Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py` around lines
1 - 5, Resolve the outstanding design review documented in the header of
judge_feedback_batch.py before merging the runner. Specifically validate the
parallel JudgeFeedbackBatchRun/EvalRun storage, identical re-run cache bypass,
and paired-gate tag sampling behavior, then remove or update the merge-blocker
TODO only after those concerns are addressed.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix ambiguous EN DASH — CI lint is failing.
Pipeline reports Ruff RUF003: comment contains an ambiguous – (EN DASH) instead of - (HYPHEN-MINUS). This blocks CI ("Format and Lint Python").
🔧 Suggested fix
-# TODO (merge blocker — do not merge toward main until resolved): this runner is under design
-# review. Concerns 3–5 are implemented here — the parallel eval-result store (JudgeFeedbackBatchRun
-# vs EvalRun), the cache bypass on identical re-runs, and the paired-gate logic retrofitted onto
-# tag-based sampling. Full write-up and rationale in the header of
+# TODO (merge blocker - do not merge toward main until resolved): this runner is under design
+# review. Concerns 3-5 are implemented here - the parallel eval-result store (JudgeFeedbackBatchRun
+# vs EvalRun), the cache bypass on identical re-runs, and the paired-gate logic retrofitted onto
+# tag-based sampling. Full write-up and rationale in the header of📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # TODO (merge blocker — do not merge toward main until resolved): this runner is under design | |
| # review. Concerns 3–5 are implemented here — the parallel eval-result store (JudgeFeedbackBatchRun | |
| # vs EvalRun), the cache bypass on identical re-runs, and the paired-gate logic retrofitted onto | |
| # tag-based sampling. Full write-up and rationale in the header of | |
| # kiln_ai/datamodel/judge_feedback_batch.py. Resolve there before merging toward main. | |
| # TODO (merge blocker - do not merge toward main until resolved): this runner is under design | |
| # review. Concerns 3-5 are implemented here - the parallel eval-result store (JudgeFeedbackBatchRun | |
| # vs EvalRun), the cache bypass on identical re-runs, and the paired-gate logic retrofitted onto | |
| # tag-based sampling. Full write-up and rationale in the header of | |
| # kiln_ai/datamodel/judge_feedback_batch.py. Resolve there before merging toward main. |
🧰 Tools
🪛 GitHub Actions: Format and Lint / 0_Format and Lint Python.txt
[error] 2-4: ruff check (RUF003): Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
🪛 GitHub Actions: Format and Lint / Format and Lint Python
[error] 2-2: Ruff (RUF003): Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/core/kiln_ai/adapters/eval/judge_feedback_batch_runner.py` around lines
1 - 5, Replace the ambiguous en dash characters in the header TODO comment of
the judge feedback batch runner with standard hyphen-minus characters,
preserving the comment’s wording and meaning so Ruff RUF003 passes.
Source: Pipeline failures
Comment on lines
+16
to
+28
| # EvalRun's job (scores for eval_config × run_config × dataset item) as a parallel, | ||
| # API-shaped model — the name is the smell: it's shaped like a request/response, not | ||
| # like the domain. It is also lossier than what it duplicates: generate mode discards | ||
| # the TaskRun (allow_saving=False), so no input/output/trace/usage survives for | ||
| # debugging an item's failure, while EvalRun keeps all four. And it creates two | ||
| # sources of truth — these scores never feed the eval score summaries, so the same | ||
| # (eval × run config) can answer differently here vs the scorecard. If EvalRun lacks | ||
| # something (e.g. the judge's feedback/reasoning text), EXTEND EvalRun. Extend, | ||
| # don't duplicate. | ||
| # | ||
| # 4. Re-running identical work must hit the cache. Same run config + same input | ||
| # (temperature aside) yields the same result — a "fresh" re-run of an unchanged | ||
| # (eval_config × run_config × item) triple is cost without information. EvalRunner |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
CI failing: replace ambiguous × (U+00D7) with ASCII x.
Ruff RUF003 fails on the × MULTIPLICATION SIGN in these comment lines, blocking the Format and Lint Python job.
🔧 Proposed fix
-# EvalRun's job (scores for eval_config × run_config × dataset item) as a parallel,
+# EvalRun's job (scores for eval_config x run_config x dataset item) as a parallel,-# (eval × run config) can answer differently here vs the scorecard. If EvalRun lacks
+# (eval x run config) can answer differently here vs the scorecard. If EvalRun lacks-# (eval_config × run_config × item) triple is cost without information. EvalRunner
+# (eval_config x run_config x item) triple is cost without information. EvalRunner📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # EvalRun's job (scores for eval_config × run_config × dataset item) as a parallel, | |
| # API-shaped model — the name is the smell: it's shaped like a request/response, not | |
| # like the domain. It is also lossier than what it duplicates: generate mode discards | |
| # the TaskRun (allow_saving=False), so no input/output/trace/usage survives for | |
| # debugging an item's failure, while EvalRun keeps all four. And it creates two | |
| # sources of truth — these scores never feed the eval score summaries, so the same | |
| # (eval × run config) can answer differently here vs the scorecard. If EvalRun lacks | |
| # something (e.g. the judge's feedback/reasoning text), EXTEND EvalRun. Extend, | |
| # don't duplicate. | |
| # | |
| # 4. Re-running identical work must hit the cache. Same run config + same input | |
| # (temperature aside) yields the same result — a "fresh" re-run of an unchanged | |
| # (eval_config × run_config × item) triple is cost without information. EvalRunner | |
| # EvalRun's job (scores for eval_config x run_config x dataset item) as a parallel, | |
| # API-shaped model — the name is the smell: it's shaped like a request/response, not | |
| # like the domain. It is also lossier than what it duplicates: generate mode discards | |
| # the TaskRun (allow_saving=False), so no input/output/trace/usage survives for | |
| # debugging an item's failure, while EvalRun keeps all four. And it creates two | |
| # sources of truth — these scores never feed the eval score summaries, so the same | |
| # (eval x run config) can answer differently here vs the scorecard. If EvalRun lacks | |
| # something (e.g. the judge's feedback/reasoning text), EXTEND EvalRun. Extend, | |
| # don't duplicate. | |
| # | |
| # 4. Re-running identical work must hit the cache. Same run config + same input | |
| # (temperature aside) yields the same result — a "fresh" re-run of an unchanged | |
| # (eval_config x run_config x item) triple is cost without information. EvalRunner |
🧰 Tools
🪛 GitHub Actions: Format and Lint / 0_Format and Lint Python.txt
[error] 16-17: ruff check (RUF003): Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
[error] 16-17: ruff check (RUF003): Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
[error] 22-24: ruff check (RUF003): Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
🪛 GitHub Actions: Format and Lint / Format and Lint Python
[error] 16-16: Ruff (RUF003): Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
[error] 16-16: Ruff (RUF003): Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
[error] 22-22: Ruff (RUF003): Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
[error] 28-28: Ruff (RUF003): Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
[error] 28-28: Ruff (RUF003): Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/core/kiln_ai/datamodel/judge_feedback_batch.py` around lines 16 - 28,
Replace every Unicode multiplication sign “×” in the comments of the judge
feedback batch module with the ASCII character “x”, including the
eval_config/run_config/item descriptions, while preserving the comment wording
and meaning.
Source: Pipeline failures
Comment on lines
+147
to
+153
| 1. **User clicks Stop (graceful).** The **Stop** control adjacent to the auto-mode indicator does | ||
| **not** hard-cancel / cut off the in-flight output. It lets the **current turn finish** | ||
| streaming, then clears the conversation's auto-mode flag and returns to **normal mode**. Any | ||
| tool calls from that final turn (and everything after) are then **subject to the normal approval | ||
| policy** — i.e. if the final turn requested client tool calls, they are surfaced for approval | ||
| (the existing `tool-calls-pending` flow) instead of being auto-executed. Net effect: "finish | ||
| what you're saying, then go back to asking me." (This replaces the earlier prompt-cancel |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)backend|assistant_auto_mode|auto/api.py|chat/auto' || true
echo "== locate functional spec =="
fd -a 'functional_spec.md' . | sed 's#^\./##'
echo "== target snippet =="
python3 - <<'PY'
from pathlib import Path
p=Path('specs/projects/assistant_auto_mode/functional_spec.md')
if p.exists():
lines=p.read_text().splitlines()
for i in range(140, 160):
if i<=len(lines):
print(f"{i}: {lines[i-1]}")
print("----")
for i in range(290, 310):
if i<=len(lines):
print(f"{i}: {lines[i-1]}")
else:
print("missing", p)
PY
echo "== searches for Stop and auto mode behavior =="
rg -n -C 3 -i 'stop|graceful|cooperatively|abandon|pending|tool-calls-pending|auto.mode|auto mode|approval' specs apps app . 2>/dev/null | head -n 200Repository: Kiln-AI/Kiln
Length of output: 24307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== backend auto/api outline =="
ast-grep outline app/desktop/studio_server/chat/auto/api.py --view expanded || true
echo "== api.py relevant sections =="
wc -l app/desktop/studio_server/chat/auto/api.py
rg -n -C 8 -i 'cancel|stop|graceful|abort|completion|tool|pending|auto_mode' app/desktop/studio_server/chat/auto/api.py app/desktop/studio_server/chat/auto/runner.py app/desktop/studio_server/chat/auto/*.py
echo "== tests stop behavior mentions =="
rg -n -C 6 -i 'stop|cancel|cooperatively|abandon|finish' app/desktop/studio_server/chat/auto | head -n 260Repository: Kiln-AI/Kiln
Length of output: 50369
Align the Stop contract at the specification level.
functional_spec.md describes two different R1 behaviors for pressing Stop during a run: §4.4 lets the current turn finish and then switches to normal approval, while §7 treats Stop mid-tool-batch as a cooperative cancellation that may abandon in-flight calls. Pick one and make both sections, the implementation, and tests consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/projects/assistant_auto_mode/functional_spec.md` around lines 147 -
153, Resolve the conflicting Stop behavior between §4.4 and §7 of
functional_spec.md by selecting one contract and applying it consistently across
both specification sections, the Stop handling implementation, and related
tests. Update the documented and verified behavior for in-flight output and tool
batches so they no longer describe different cancellation semantics.
Comment on lines
+13
to
+16
| > **Revision R1 — color.** The auto-mode accent is **blue primary** (`text-primary` / `bg-primary` | ||
| > / DaisyUI `primary`), **not green**. Everywhere this doc says "green" for the indicator, dot, or | ||
| > "auto mode on" treatment, use primary blue instead. State is still conveyed by text too, not | ||
| > color alone. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the Revision R1 blue accent consistently.
Revision R1 explicitly says all green references must become DaisyUI primary blue, but the remaining sections still specify green classes, labels, and indicators. Update those references so the UI contract is unambiguous.
Also applies to: 20-22, 51-66, 126-131, 147-148, 158-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/projects/assistant_auto_mode/ui_design.md` around lines 13 - 16, Update
the auto-mode UI specifications throughout the document, including the sections
around the listed ranges, to replace every green indicator, dot, label, and
“auto mode on” treatment with the Revision R1 DaisyUI primary blue treatment
using text-primary, bg-primary, or primary as appropriate. Preserve the
requirement that state is also conveyed through text and ensure no conflicting
green references remain.
Comment on lines
+100
to
+109
| > While it's on: | ||
| > - It will **run tool calls and Kiln API actions without asking for approval** — including | ||
| > actions you'd normally confirm. | ||
| > - It may **start costly jobs** (for example, reflective optimization runs) that **use tokens and | ||
| > can incur real cost**. | ||
| > - It **keeps working on the server even if you close this window**, until it finishes, needs your | ||
| > input, or you stop it. | ||
| > | ||
| > Auto mode turns off automatically when the assistant has a question for you or is done. You can | ||
| > stop it anytime. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Correct the consent copy for persistent auto-mode.
This text says auto-mode turns off automatically when the assistant asks a question or finishes. Under Revision R1, it remains enabled and becomes idle until the user clicks Stop or the assistant handles disable_auto_mode. Leaving this copy unchanged misrepresents ongoing server-side execution and potential cost to the user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/projects/assistant_auto_mode/ui_design.md` around lines 100 - 109,
Update the persistent auto-mode consent copy near “Auto mode turns off
automatically” to state that auto-mode remains enabled and becomes idle when the
assistant asks a question or finishes. Clarify that it only stops when the user
clicks Stop or the assistant handles disable_auto_mode, while preserving the
existing warnings about server-side execution and potential cost.
2 tasks
scosman
pushed a commit
that referenced
this pull request
Aug 5, 2026
Documents the mis-routing being fixed: evals_v2 dispatches collect_tasks on an eval-level source mode checked before run type, so an EvalInput-backed eval routes judge calibration through the EvalInput path too - despite calibration scoping by the golden set, which is TaskRun-typed. The skipped-EvalRun writing in run_job is the handler for jobs that should never have been collected, so running calibration on such an eval succeeds with a 200 while scoring nothing and leaving a junk record per item. Once source is a property of a split there is no eval-level mode, calibration cannot reach an EvalInput, and the branch handling it is dead code. Records already written are inert under all three readers - checked, not assumed - so no cleanup is specified. Also records that the agi merge backs out #1621 rather than adding to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017oeRgVgXqSeKKb1VacoGyB
scosman
pushed a commit
that referenced
this pull request
Aug 5, 2026
Seven phases: merge, splits datamodel and accessor, creation paths and prompt optimization, EvalRunner, jobs, API and UI, eb-v2 alignment overview. The merge phase drops #1621's split additions rather than reconciling them, since later phases replace all of them - that is what keeps the tree from carrying two split APIs at once, and lets every later phase land green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017oeRgVgXqSeKKb1VacoGyB
scosman
pushed a commit
that referenced
this pull request
Aug 5, 2026
Phase 1 of specs/projects/eval_splits_v1_v2. Merge only - no new behavior, ending on a green tree with no `split` parameter anywhere. Conflicts resolve toward evals_v2 per architecture.md section 10: eval_set_filter_id becomes `DatasetFilterId | None`, the EvalInput model and its collection path stay, and validate_filter_fields keeps the exactly-one-source invariant. This merge is NOT additive - it deliberately deletes PR #1621's split implementation rather than reconciling it. val_set_filter_id, EvalSplitName, Eval.filter_id_for_split, migrate_val_set_filter_id, split_filter_id_from_eval, EvalRunner.eval_set_filter_id_override, the `split` query param on the results endpoint, EvalJobParams.split with the worker's _split_override/_dataset_filter_id, and the jobs-API split pre-resolution are all gone. They are replaced in phases 2-6 by the `splits` dict, resolve_split and ResolvedSplit. Reviewers should compare this against architecture.md, not against #1621. Merge fallout git could not see (the branches merged cleanly but disagree about APIs they do not share): - registry.eval_adapter_from_type was renamed on evals_v2 to legacy_eval_adapter_from_type(eval_config), taking the config rather than its type. judge_feedback_batch_runner.py still imported the old name, which was an import-time crash of the whole server. Renamed, with the three test patch targets updated. The new function raises NotImplementedError for V2 configs - a state the old one could not represent - so judge feedback batches now refuse a V2 judge with a 422 at validate_judge_eval rather than failing mid-run with an internal message. Building V2 dispatch for that runner is judge-axis work, out of scope here. - EvalConfig.model_name/model_provider became `str | None` on evals_v2 (V2 configs carry the model in properties). The job-properties models declare them `str`; blanked with `or ""`, matching how those files already handle an MCP run config that carries no model. - The merge duplicated test_run_job_wrapped_rate_limit_raises_retryable_with_detail in test_eval_runner.py - both branches added it independently. One kept. Also fixes two pre-existing RUF003 lint failures inherited from the integration branch (ambiguous unicode in the judge_feedback_batch design-review comments), since the phase's end state is a green tree. Two tests added, both for raise paths this merge created: a V2 judge is a 422 at batch creation, and the eval job worker raises for an EvalInput-backed eval rather than reporting a zero total that would let a resume short-circuit to complete. Phase 5 replaces the latter with resolve_split. spec_utils keeps its 4-tuples per architecture.md section 8; spec_api and copilot_api hold the val filter id in an unused binding until phase 3 wires it into `splits`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017oeRgVgXqSeKKb1VacoGyB
2 tasks
scosman
pushed a commit
that referenced
this pull request
Aug 6, 2026
The eval job worker had two independent answers to "which items are in scope": the runner's, and the progress universe's. `compute_state` built its universe from `task.runs()` under `eval.eval_set_filter_id`, which is empty for an EvalInput-backed eval — so totals read zero and a resume could short-circuit against the wrong set. - `_resolve_split` is now the one resolution rule, shared by `_build_eval_runner` and `_compute_state_sync`. Both still resolve separately, because the registry calls `compute_state` standalone and a cached universe would answer from a stale snapshot; what matters is that both describe the same split of the same eval. - Membership keys on `ItemKey` rather than bare `dataset_id`, so a TaskRun and an EvalInput sharing a truncated-uuid id can't be conflated. - `EvalJobParams.split` is restored from #1621, and `jobs/api.py` pre-resolves through phase 4's `resolved_split_or_422` so a bad split 422s at request time instead of becoming a doomed background job. The pre-check is guarded on a named split: `Eval` cannot validate without a test split, so resolving unconditionally would enumerate the dataset off disk to refuse nothing. - The error log gains `item_source` alongside `dataset_id`, which now carries EvalInput ids for the first time. Adds `phase5_mutation_sweep.py` — 13 mutations, all killed. Phase 4's sweep drops to 19 (one entry's target line was replaced here) and phases 2 and 3 still kill 30/30 and 23/23 against this tree. Also drops the `agi-anyting_goes_into` merge from the project workflow: this branch goes straight to its PR against `scosman/evals_v2`. Architecture §10's warning survives, generalised — anywhere this branch meets a tree carrying #1621, the diff reads as a large deletion that is correct, not lost work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017oeRgVgXqSeKKb1VacoGyB
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Lets the eval APIs target one of an eval's three dataset splits (
train_set_filter_id,val_set_filter_id, andeval_set_filter_id— the test set, whose name is legacy). Omitting the param keeps today's behavior everywhere.POST /api/jobs/evals/runacceptssplit. The name resolves to the eval's stored filter id — 422 at job creation if the eval has no such split, rather than a doomed background job — and is passed toEvalRunneras a filter override. Job progress totals follow the split's item universe, so resumes can't short-circuit against the wrong set.GET …/run_config/{run_config_id}/resultsaccepts asplitquery param, filtering returnedEvalRuns by split membership at query time. NoEvalRunschema change, so it works retroactively on stored results.EvalRunnergainseval_set_filter_id_override; the item-grained incremental cache is untouched, so overlapping splits reuse already-scored items.Eval.filter_id_for_splitmaps split names to stored filter ids.Note: stacks on the kil-686 eval-job lineage (branched from
leonard/kil-686-eval-job) and includes a cherry-pick of theval_set_filter_idcommit from #1620.Related Issues
Builds on the kil-686 eval-job work; companion to #1620.
Checklists
🤖 Generated with Claude Code
https://claude.ai/code/session_0135zWTBh8MRPwCXiWvuE597
Generated by Claude Code