Repository navigation
feat: implement issue #1787 — calibrate solution-architect escalation so conforming changes are not escalated - #2205
Merged
Merged
Conversation
… so conforming changes are not escalated All three held-out failures in #1787 were wrong escalate decisions on otherwise correct advisories: the persona cited the right ADR, then escalated a change that fits it. The prompt had only a one-line "do not manufacture concern" sentiment and no calibration. - Add a "Calibrating the risk tier and escalation" section to step 4, in the shape qa-lead's prompt uses: conforming -> LOW / no; drift the ADR already corrects -> MEDIUM / no; no ADR governs -> name the gap, no escalation; escalate = yes only when merging breaks a decided contract beyond the PR or deliberately overturns an accepted ADR. HIGH always pairs with escalate = yes. - Add an explicit **Risk tier:** line to the comment body and a rule that tier and escalate are always stated (AC4). - Runner path unchanged: sentinels, persona marker, read-only gh, and the cite-an-ADR-or-say-none rule (AC5). No eval case or threshold touched (AC7). - Guard the calibration with bats checks in test_advisory_offline_mode.bats; they fail on the previous prompt and pass on this one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dijra3xKt8dUEtEm2DK9W5
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 30 minutes. View limit details
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 |
Collaborator
Author
|
|
Contributor
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-10T17:57:21Z. |
Collaborator
Author
QA Lead — test-risk advisoryRisk tier: LOW. This only changes the prompt of an advisory-only persona that can't write or block anything, and a revert fully undoes it. What I'd shore up (highest leverage first):
Escalate? no Advisory only — I comment, I do not change code. Ground: BMAD Test Architecture (bmad-tea v1.19.0). Opt out on this item with the qa-lead:hands-off label. |
Collaborator
Author
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Step 2 told the persona to `cat docs/architecture/adr/ADR-NNNN.md`, but the corpus is named `NNNN-<slug>.md` (e.g. 0002-channel-tag-release-and-rings.md), so the example path never resolved. Show the real naming and how a file maps to its `ADR-NNNN` citation, and add a bats check that the example names a file that exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dijra3xKt8dUEtEm2DK9W5
don-petry
enabled auto-merge
October 10, 2026 16:57
Contributor
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-10T17:57:32Z. |
CodeAnt Nitpicks1 code suggestion1. This assertion never verifies the no-ADR branch says
|
CodeAnt PR Risk: Medium Risk
Assessed commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new section on calibrating risk tiers and escalation rules in the solution-architect advisory prompt, along with corresponding BATS tests to ensure these rules are correctly documented and enforced. The review feedback highlights opportunities to improve consistency in the prompt's wording and to make the BATS tests more robust and portable by avoiding fragile line-count grep assertions and non-POSIX flags.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
don-petry
disabled auto-merge
October 10, 2026 16:59
… AC2 check From the qa-lead advisory on this PR: - The dev and holdout sets expected escalate=false in all but one case, so calibrating toward "no" could hide a missed escalation. Add two dev cases (holdout untouched): a write mode added to an advisory persona surface with no gate label (ADR-0005, HIGH / escalate) and a PR that deliberately overturns ADR-0004 (MEDIUM / escalate), the new calibration row no case covered. - The AC2 bats check read the conforming bullet with `grep -A3`, which a rewrap would break. Extract the whole bullet up to the next sibling instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dijra3xKt8dUEtEm2DK9W5
This comment has been minimized.
This comment has been minimized.
…y fail on regression - The ADR-path check used `! grep -q` mid-test; errexit ignores a negated command, so it could never fail (Graphite). Fail explicitly instead. A prompt that re-adds `adr/ADR-NNNN.md` next to a valid example now fails. - The "no ADR governs" check never asserted that bullet says `escalate = no`, and matched line-wrapped fragments (CodeAnt, Gemini). Extract the bullet, flatten it, and assert both. Flipping that bullet to yes now fails. - Wording: "escalate is no" -> "escalate = no", matching the other rules (Gemini). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dijra3xKt8dUEtEm2DK9W5
don-petry
enabled auto-merge
October 10, 2026 17:01
Contributor
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-10T18:09:10Z. |
Contributor
|
No description provided. |
Contributor
|
No description provided. |
Contributor
|
No description provided. |
don-petry
disabled auto-merge
October 10, 2026 18:00
This comment has been minimized.
This comment has been minimized.
… written `cat docs/architecture/adr/NNNN-<slug>.md` breaks if the persona runs it verbatim: the shell reads `<slug>` as an input redirection (CodeAnt). Use a real lookup by number, `cat docs/architecture/adr/0002-*.md`, and keep the naming explained in the comment above it. The bats check now also fails if the command carries a `<placeholder>` or its glob matches no ADR file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dijra3xKt8dUEtEm2DK9W5
Collaborator
Author
|
No description provided. |
don-petry
disabled auto-merge
October 10, 2026 21:12
Collaborator
Author
|
Fixed in tests/test_advisory_offline_mode.bats:154: added explicit check to verify the no-ADR branch specifically says escalate = no, preventing the decision from being changed to escalate = yes while still passing the test. |
Contributor
|
No description provided. |
Added explicit check to verify the no-ADR branch does not contain escalate = yes. This prevents changing the no-ADR decision to escalate = yes while still passing the regression guard. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
don-petry
enabled auto-merge
October 10, 2026 21:34
Collaborator
Author
|
No description provided. |
|
Contributor
|
No description provided. |
Collaborator
Author
|
No description provided. |
This was referenced Oct 10, 2026
Merged
don-petry
added a commit
that referenced
this pull request
Oct 11, 2026
- bcr_retry_run_ids reads a retry's run inside the lost-retry backoff as well as the pending window. With a backoff longer than the pending window a lost run past the pending window was never read, so the backoff could not hold. - bcr_run_state no longer counts a job whose only steps were skipped as started. A cancelled step, or one with no conclusion, still counts as run. - record_bot_comment_retry_run records a run only when two looks in a row show the same single candidate. One look could record another PR's retry run that was listed before this dispatch's own. The second look narrows that race; a run of ours listed later than both looks still leaves the other as the only candidate. - BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC (default 1200): past it a dispatch records no run instead of waiting, so lookup waits cannot use up the retry workflow's 30-minute limit. - Tests: 7 new, 5 of which fail on the previous scripts. One runs the pairing and the run-state reader against the Actions API's real answers for PR #2205's cancelled retry (tests/fixtures/bot-comment-retry/). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Oct 11, 2026
…er holds the pending window or an attempt (#2262) (#2267) * fix(dev-lead): a bot-comment retry cancelled before it starts no longer holds the pending window or an attempt (#2262) A fix-bot-comment retry joins the PR's concurrency lane, where GitHub keeps one pending run, so a newer PR event cancels it before a step runs. The sweep only saw its own marker: the cancelled retry stayed "pending" for 150 minutes and used up one of the two attempts. The sweep now records the run each dispatch created on the retry marker (run=<id>, by editing the marker) and reads recorded runs back on later scans: - a run cancelled with no job step run is lost: not pending, and not counted toward BOT_COMMENT_RETRY_MAX_ATTEMPTS. It still counts toward BOT_COMMENT_RETRY_MAX_TOTAL, and the PR's next retry waits out a short backoff, so a lane that keeps cancelling stays bounded; - a run still queued or running holds every comment on the PR, so one comment's retry no longer cancels another's queued run (the PR #2205 sequence); - a run that cannot be paired with its dispatch (none, or more than one candidate) or cannot be read keeps the time-based behaviour. An API error is never read as "cancelled". bcr_retry_decisions stays pure: run states are passed in as data. Markers without run= parse as before, and an older parser reads a marker with run= unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dev-lead): close the retry-run gaps review found (#2262) - bcr_retry_run_ids reads a retry's run inside the lost-retry backoff as well as the pending window. With a backoff longer than the pending window a lost run past the pending window was never read, so the backoff could not hold. - bcr_run_state no longer counts a job whose only steps were skipped as started. A cancelled step, or one with no conclusion, still counts as run. - record_bot_comment_retry_run records a run only when two looks in a row show the same single candidate. One look could record another PR's retry run that was listed before this dispatch's own. The second look narrows that race; a run of ours listed later than both looks still leaves the other as the only candidate. - BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC (default 1200): past it a dispatch records no run instead of waiting, so lookup waits cannot use up the retry workflow's 30-minute limit. - Tests: 7 new, 5 of which fail on the previous scripts. One runs the pairing and the run-state reader against the Actions API's real answers for PR #2205's cancelled retry (tests/fixtures/bot-comment-retry/). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 task done
don-petry
added a commit
that referenced
this pull request
Oct 11, 2026
… replies posted as PR comments (#2261) (#2270) * fix(dev-lead): disposition a fixed bot issue comment and keep its sha valid across a guard rebase (#2261) On PR #2205 a fix-bot-comment pass fixed a CodeAnt finding posted as a PR issue comment and answered it like a review thread. The maintainer-comment gate reads only a comment-disposition reply, so the comment was never dispositioned, and the claim in that reply cited a commit the push guard had rebased away. - Prompt: fix-bot-comment now says, for every reviewer bot, that a finding fixed from an issue comment gets exactly one comment-disposition `fixed sha=` reply, and that the addressed-marker and the claim are for review threads only. - resolve_dispositioned_comments: a `fixed` sha that is not on the pushed head but maps through _PUSH_GUARD_REWRITES is judged by its rebased successor with the same rdc_fixed_verdict, and our reply is edited in place to cite it. An unmapped sha, a successor that is not on the head or fails the content rule, or a reply that cannot be edited stays open. - retract_unlanded_claims: also sweeps this pass's claim replies posted as PR issue comments. One is re-pointed when its commit was rebased and landed, and retracted otherwise. A comment-disposition marker in the same reply is kept. Not changed: the #2004 content rule. A fix that only adds lines, for a finding that names a token, still does not verify (the #2205 fix itself). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dev-lead): remove a claim whose JSON spans lines when retracting it (#2261) cl_strip_claim_markers stopped at the first newline, while acv_parse_claim reads a claim through its first ` -->` across lines. A retracted reply with a multi-line claim kept the claim comment in its quoted original, so the next sweep selected and retracted it again. The pattern now runs to the first `-->` across lines and still keeps a later marker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dev-lead): an issue-comment listing that is not a list no longer fails the claim sweep (#2261) cl_without_fixed_dispositions rejected a JSON payload that was not an array, so retract_unlanded_claims returned non-zero and a pushed pass was reported as failed. CI caught it in "resolve_deferred_bot_threads (#2045): after a pushed commit, the resolve-failed marker is keyed to the pushed head", whose gh stub answers the listing with an object. The helper now passes such a payload through, and the claim selector reads it as "no replies", as it and main's `fixed`-disposition sweep already do. A payload that is not JSON still fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Oct 11, 2026
…#2269) The comment names a test file, so the pass also goes through the test-tamper guard. The test serves the guard's base-branch lookup and gives the repo an origin to fetch main from, then checks the disposition verifies and the comment is minimized. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Oct 11, 2026
…ng names (#2269) (#2303) * fix(dev-lead): verify an additive fix a pass made in a file the finding names (#2269) A `fixed` comment disposition verified only when the cited diff removed a token the finding names (#2004). A fix that only adds lines, such as a missing assertion or guard, removes nothing, so it could never verify and its comment stayed open (PR #2205, commit 3e5025b). Maintainer ruling on #2269: option 1. A commit that removes no named token now verifies when this pass produced it and its diff changes a file the finding names by its full repo path (`this-pass-touches-named-file`). It must still be on the pushed head, off the base branch, non-empty and dated after the finding. A commit from an earlier pass, or one outside the named files, keeps the removed-token rule. This is the rule a review-thread claim already passes. - cdv_finding_paths, cdv_touches_named_file: pure helpers. - cdv_fixed_verdict takes an eighth argument, <touches_named>. - rdc_fixed_verdict passes the commit's own files against the finding's paths. - Both prompts and docs/pr-review-agent/maintainer-comment-gate.md state it. - Tests: 8 pure (one on the real PR #2205 comment and the diff of 3e5025b, saved under tests/fixtures/comment-disposition/) and 4 through the full script. 9 of the 12 fail on main's sources; the other 3 are the guards that an unrelated or earlier commit still does not verify. Closes #2269 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dev-lead): a diff that adds a named token never verifies as an additive fix (#2269) Review (CodeAnt, QA Lead): the new path accepted `content-adds-token` as well as `content-no-token-removed`. For a finding like "remove `eval` from x.sh", a commit from this pass that adds another `eval` to that file would have verified. The path is now limited to a diff that neither removes nor adds a token the finding names; `content-adds-token` fails whatever pass made the commit, as before this branch. - Prompts and the gate doc say so. - Tests: the boundary at the pure level and through the full script; a table of the forms reviewer bots write a location in; renamed and deleted files; and one that pins the accepted limit that a comment-only line in the named file verifies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(dev-lead): run the real PR #2205 comment through the full script (#2269) The comment names a test file, so the pass also goes through the test-tamper guard. The test serves the guard's base-branch lookup and gives the repo an origin to fetch main from, then checks the disposition verifies and the comment is minimized. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Summary
Adds escalation calibration to the solution-architect advisory prompt, using the worked-example shape
prompts/qa-lead/advisory.mdalready uses. It also makes the risk tier an explicit line in the comment body. New bats checks guard the calibration against a later edit or template sync.It also fixes the ADR path in step 2. The prompt said
cat docs/architecture/adr/ADR-NNNN.md, but the files are namedNNNN-<slug>.md(for example0002-channel-tag-release-and-rings.md), so the example never resolved. Step 2 now shows the real naming and how a file maps to itsADR-NNNNcitation.Closes #1787
Problem
#1787: solution-architect scored 0.625 against the 0.7 gate. All three held-out failures were wrong escalate decisions on otherwise correct advisories: the persona cited the right ADR, then escalated a change that fits it.
The prompt had only one generic line ("do not manufacture concern") and nothing tying a class of structural finding to a tier and an escalate decision. Two of the failures also lost points because the risk tier was never stated.
Calibration added to step 4 of the prompt:
Two consistency rules are also added: HIGH always pairs with escalate = yes, and an advisory rated "aligned" must not escalate.
The examples are written at class level. They don't restate any held-out case, so the prompt adapts to the classes the cases exercise rather than to the cases themselves (AC7).
Risk
Low. This changes the prompt text of an advisory-only persona that can't block or write anything. The runner path is unchanged (AC5): the sentinels, the
<!-- persona:solution-architect -->marker, the read-onlyghfetches and the "cite an ADR or say there is none" rule are all as before. No eval case, judge or threshold is touched (AC7).Test plan
bats tests/test_advisory_offline_mode.bats: 18/18 pass. The 6 new checks cover:**Risk tier:**;docs/architecture/adr/.main's prompt.ADR-NNNN.mdpath.bats tests/test_skill_evals.bats(45),tests/test_validate_cases.bats(21) andtests/test_score_gate.bats(30) all pass.prompts/is excluded from markdownlint by config.Rollback
Revert this PR. There are no non-revertible side effects.
Monitoring
skill-eval-report.ymlforsolution-architectand record the score beside0.25 → 0.625 → 0.875 → ?(AC6).solution-architect(holdout) #2063, taken without any prompt change.sa-hold-eval-immutability-gate-no-adrandsa-hold-repin-channel-to-frozen-tag.solution-architect(holdout) #2063 failure,sa-hold-onboard-advisory-persona-router, wasgot null(no parsed output) rather than a wrong escalation. Watch whether it recovers.solution-architect(holdout) #2063 auto-closes on recovery.Interaction contract
Checklist
shellcheck scripts/*.shpasses for any changed shell scripts (none changed).Related: #2203. That issue would make persona escalations hold PRs, so this calibration should land first.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Dijra3xKt8dUEtEm2DK9W5