fix(sage): explain the discriminated review-failure causes - #7686
Conversation
github-actions
Bot
added
the
readiness: checking
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Each new failure sentence names what happened and the remedy, matches sibling [UX-REVIEWED] de185f8 |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Right fix for these four strings, but the design guarantees the same bug recurs: translation keyed on backend prose the API has already stripped of its enum. Watch
Suggestions
[DESIGN-REVIEWED] de185f8 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of Everything checks out: the four backend strings exist verbatim where the description says ( First-Principles-Verdict: PASS Untranslated backend English on the one card meant to explain itself is a named, user-visible harm; every item traces to one of the four real strings. What this change shipsIntent: make Code Review Sage's four new "reviewer never ran / stopped short" failures render as actionable, translated sentences instead of raw backend English — a FIX.
Duplication counted: zero — no existing branch or key covers these strings ( [FIRST-PRINCIPLES-REVIEWED] de185f8 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
|
Dispositioning the three advisory suggestions on UX - append a next step to The absence of a next step there is the honest content, and the two cases where Design - expose the First Principles - "Watch": eight prose-keyed regexes drift silently on a |
|
The red Frontend Lint & Type Check on
Main never went red for it because of the failure mode #7511 already describes - Not folding a main-side CI fix into this PR unilaterally. The one-line |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
aa0a449 to
fefc45b
Compare
github-actions
Bot
added
readiness: checking
PR #7240 gave the Code Review Sage backend four new failure strings, and the frontend cause translator had a branch for none of them, so a reviewer that never started rendered as raw lowercase backend English in the run card. The four strings are the two answers review_pool.runtime_preflight can give (no kiro-cli executable, ACP runtime not importable), the driver's per-change wording for a record written without completing the review, and the run-level sentence routes.py _first_change_error maps `runtime_unavailable` to. Each now has its own branch and its own localized sentence in 13 catalogs, keyed on the specific phrase rather than on the word `kiro-cli`: a missing agent spec also names kiro-cli and already carries its own repair command, so a broad pattern would have replaced useful text with wrong guidance. A test pins that case. The screenshot harness answers the dashboard's boot endpoints through the shared scripts/lib/stub-dashboard-api.mjs rather than carrying its own copy of them, which is what that helper exists for. Fixes #7242 Refs #7233
fefc45b to
de185f8
Compare
|
Design re-raised the token-vs-prose class on Two things I would add to the record rather than argue:
Not folding the payload change into this PR: it is a backend contract change on a |
github-actions
Bot
added
readiness: passed
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.
github-actions
Bot
removed
the
readiness: passed
A Sage run's failure cause exists as a stable token in the backend -- `skipped_reason` is one of `no_review_recorded`, `review_record_incomplete`, `runtime_unavailable`, `review_failed` -- but nothing in the payload the dashboard receives named it. `routes.py::_first_change_error` mapped the token to an English sentence before the run was serialized, and each per-change `progress` entry carried prose while the token sat on the driver-internal record beside it. So the dashboard's translator had to recognize causes by their wording, and rewording any backend message silently reverted its card to untranslated pass-through with no test going red -- the frontend's fixtures are its own copies of the backend's strings. That drift is not hypothetical. It has already happened and is on main now: `_first_change_error` maps `no_review_recorded` to "the reviewer finished but wrote no findings record", and NONE of the translator's eight prose branches matches it -- the no-record branch keys on "no result record", the driver's per-change wording for the same cause. The sibling branch for `review_record_incomplete` deliberately covers both of ITS wordings, so this is an asymmetry rather than a decision. That card renders raw backend English today; with the token it explains the cause. The token is now carried BESIDE the sentence, never instead of it: - `_first_change_failure` returns `(sentence, token)` from ONE pass over `per_change`. One pass rather than two functions because the sentence is often built from a record's `deep_error` while the token is that record's `skipped_reason`; an independent scan for the token would return a LATER record's token and label this failure with an unrelated cause. `_first_change_error` keeps its `-> str` signature as a thin wrapper, so every existing caller and its eight assertions across two test files stay untouched -- the change is additive, not a signature migration. - The run record gains `reason` on both error branches. Absent, never blank, when no record carried a token, so a reader cannot mistake one for the other. - Each of the driver's five failure paths puts its token on the `progress` entry it already wrote prose to, agreeing with the `skipped_reason` it sets on the same path. - `failureReason` resolves the token through the same per-change-then-run precedence it already uses for the prose, and consults it exactly where the verbatim pass-through used to be -- which is where a reword lands. The prose path is PERMANENT, not transitional: `progress` lives inside a persisted run record, so every run already on disk has no token and never will. Prose matching is the compatibility path for those runs for good. Two deliberate narrowings, both with a test pinning them: The token is consulted AFTER the prose branches, not before. For one cause the prose is strictly more specific than the token: all three runtime-preflight messages carry the single token `runtime_unavailable`, so a token-first lookup would replace "no kiro-cli executable was found" and "the ACP runtime is not importable" -- one of which carries the exact repair command -- with the generic sentence. The translator's own comment on those branches already makes this argument about a bare /kiro-cli/ pattern; keying on the token first would reintroduce the loss it warns about by another route. `review_failed` is emitted but is NOT in the translator's token table. The other three label prose the backend wrote, a closed set of fixed sentences the token faithfully stands in for. `review_failed` labels whatever the failed dispatch returned, so its prose is arbitrary and usually far more specific: the missing-agent-spec message arrives on that path carrying its own `kirocrew setup --agent-only --clean` repair command. Adding it to the table reddens the verbatim guard, which is how that was caught. The screenshot harness grew the reword case, and an assertion that made it observable: its per-card check finds a card BY its expected sentence, so it could not see a regression on a cause whose sentence another fixture also renders -- the `find` returned the healthy sibling. It now also asserts no cause's raw backend wording appears anywhere in the list. The two committed frames are regenerated from it; `0-before-*` is left alone, being the historical pre-fix frame from #7686. Refs #7688
…8186) A Sage run's failure cause exists as a stable token in the backend -- `skipped_reason` is one of `no_review_recorded`, `review_record_incomplete`, `runtime_unavailable`, `review_failed` -- but nothing in the payload the dashboard receives named it. `routes.py::_first_change_error` mapped the token to an English sentence before the run was serialized, and each per-change `progress` entry carried prose while the token sat on the driver-internal record beside it. So the dashboard's translator had to recognize causes by their wording, and rewording any backend message silently reverted its card to untranslated pass-through with no test going red -- the frontend's fixtures are its own copies of the backend's strings. That drift is not hypothetical. It has already happened and is on main now: `_first_change_error` maps `no_review_recorded` to "the reviewer finished but wrote no findings record", and NONE of the translator's eight prose branches matches it -- the no-record branch keys on "no result record", the driver's per-change wording for the same cause. The sibling branch for `review_record_incomplete` deliberately covers both of ITS wordings, so this is an asymmetry rather than a decision. That card renders raw backend English today; with the token it explains the cause. The token is now carried BESIDE the sentence, never instead of it: - `_first_change_failure` returns `(sentence, token)` from ONE pass over `per_change`. One pass rather than two functions because the sentence is often built from a record's `deep_error` while the token is that record's `skipped_reason`; an independent scan for the token would return a LATER record's token and label this failure with an unrelated cause. `_first_change_error` keeps its `-> str` signature as a thin wrapper, so every existing caller and its eight assertions across two test files stay untouched -- the change is additive, not a signature migration. - The run record gains `reason` on both error branches. Absent, never blank, when no record carried a token, so a reader cannot mistake one for the other. - Each of the driver's five failure paths puts its token on the `progress` entry it already wrote prose to, agreeing with the `skipped_reason` it sets on the same path. - `failureReason` resolves the token through the same per-change-then-run precedence it already uses for the prose, and consults it exactly where the verbatim pass-through used to be -- which is where a reword lands. The prose path is PERMANENT, not transitional: `progress` lives inside a persisted run record, so every run already on disk has no token and never will. Prose matching is the compatibility path for those runs for good. Two deliberate narrowings, both with a test pinning them: The token is consulted AFTER the prose branches, not before. For one cause the prose is strictly more specific than the token: all three runtime-preflight messages carry the single token `runtime_unavailable`, so a token-first lookup would replace "no kiro-cli executable was found" and "the ACP runtime is not importable" -- one of which carries the exact repair command -- with the generic sentence. The translator's own comment on those branches already makes this argument about a bare /kiro-cli/ pattern; keying on the token first would reintroduce the loss it warns about by another route. `review_failed` is emitted but is NOT in the translator's token table. The other three label prose the backend wrote, a closed set of fixed sentences the token faithfully stands in for. `review_failed` labels whatever the failed dispatch returned, so its prose is arbitrary and usually far more specific: the missing-agent-spec message arrives on that path carrying its own `kirocrew setup --agent-only --clean` repair command. Adding it to the table reddens the verbatim guard, which is how that was caught. The screenshot harness grew the reword case, and an assertion that made it observable: its per-card check finds a card BY its expected sentence, so it could not see a regression on a cause whose sentence another fixture also renders -- the `find` returned the healthy sibling. It now also asserts no cause's raw backend wording appears anywhere in the list. The two committed frames are regenerated from it; `0-before-*` is left alone, being the historical pre-fix frame from #7686. Refs #7688 Co-authored-by: gh-autofix#2887 <chenmingwei23@users.noreply.github.com>
2 participants
Problem / Motivation
PR #7240 discriminated Code Review Sage's failure causes in the backend, so a
host that cannot spawn a reviewer now says so instead of reporting an
untriageable "produced no result record". The frontend cause translator
(
website/src/apps/code-review-sage/lib/format.ts,failureReason) has nobranch for any of the new strings, so each one reaches the run card as raw
lowercase backend English:
the reviewer cannot run: no kiro-cli executable was found on this host (the reviewer session is driven by kiro-cli - install it or add it to PATH)the reviewer cannot run: the ACP runtime (kiro_crew.acp.runtime) is not importable in this installthe reviewer never ran: its agent runtime is unavailable on this hostreview wrote a result record but never completed the reviewWhy it matters
These are exactly the failures a reader most needs help with: the review did not
fail, it never ran, and the fix is on the host. Rendering the driver's own
sentence leaves that unsaid in every language, and leaves a non-English reader
with untranslated English on the one card that is supposed to explain itself.
The existing
/no result record|no_review_recorded/branch deliberately does notmatch the incomplete-record message, so nothing catches it by accident.
What changed (motivation -> approach -> change)
Four branches, not the two the issue proposed, because four distinct strings
reach this function. The issue's set missed two of them:
review_pool.runtime_preflightno kiro-cli executable was foundkiro-cli)review_pool.runtime_preflightthe ACP runtime (...) is not importablekiro-cli, noruntime_unavailableroutes.py::_first_change_errorits agent runtime is unavailable on this hostreview_driver.run_review/_first_change_error... never completed the reviewThe run-level miss is the one worth naming:
_first_change_errormapsruntime_unavailableto prose, so a translator keyed on the enum token neversees it.
The patterns are also narrower than the issue asked for. A branch on the bare
word
kiro-cliwould swallowacp/runtime.py's missing-agent-spec message -Agent spec '...' is not installed: kiro-cli found no '....json' in ... - repair with \kirocrew setup --agent-only --clean`- which reaches this function through the pool's spawn error and already carries the exact repair command. Replacing it with "install kiro-cli" would be worse than today's pass-through, so the branch matchesno kiro-cli executable` and a test pins the agent-spec message asverbatim.
Each cause gets its own sentence rather than one shared "runtime unavailable",
because the remedies differ: install the CLI / put it on PATH, repair the
install, or nothing actionable named yet (the raw line under the sentence still
carries the specifics). Four new keys in all 13 catalogs, en-XA regenerated.
Tests
website/src/test/CodeReviewSageRunList.test.tsx, five cases, every fixture thestring the backend really emits:
and do not fall into the "recorded no findings" branch
PATH one (different remedy, so it must not share a key)
runtime is unavailablesentence and the bare enum both resolvecommand intact
Mutation-verified, one test killed per mutation: broadening
/no kiro-cli executable/to/kiro-cli/(the pattern the issue prescribed)fails the agent-spec guard; disabling each of the other three branches fails
exactly its own case.
Manual verification
node scripts/i18n-check.mjswithI18N_BASE_REFat the PR base: 19/19 PASS(4 new English keys, 0 badly shaped, 0 QA findings, 0 values still reading as
English, key-refs resolve, en-XA matches).
vitest run src/i18n/660 passing, incl. catalog parity, dead keys, and theper-language style suites;
vitest run src/test/CodeReviewSageRunList.test.tsx40 passing;
tsc --noEmitandeslintclean on the touched files.the base: all pass.
website/scripts/capture-sage-cause-translator.mjs,a new harness that runs the real built SPA with the backend answered from
fixtures. It asserts the rendered text rather than just saving a PNG: each card
shows its own sentence, no translated card still contains the backend wording,
and the detail notice keeps the driver's raw line.
Screenshots / video
Before - every new cause as raw backend English (this same harness on the
pre-fix translator):
After - one sentence per cause. The last card is the missing-agent-spec message,
unchanged on purpose:
The failure notice in the detail pane: sentence, then the driver's own wording
Related Issues
Closes #7242
Refs #7233
Pattern harvest
Rule candidate: review-prompt
Pattern: a frontend translator keyed on a backend enum token that the backend
maps to prose before it leaves the API - the token never reaches the client, so
the branch is dead and the prose renders untranslated. Same shape as matching an
error by a word (
kiro-cli) that a second, unrelated error also contains.Checklist