diff --git a/evals/solution-architect/dev/cases.jsonl b/evals/solution-architect/dev/cases.jsonl index cd535f05f..73702a5a3 100644 --- a/evals/solution-architect/dev/cases.jsonl +++ b/evals/solution-architect/dev/cases.jsonl @@ -2,3 +2,5 @@ {"id": "sa-dev-frozen-tag-pin", "description": "PR repins a first-party caller stub from its moving channel tag to a frozen @vX.Y.Z release; drifts from the channel-tag release model. Grounds: ADR-0002.", "tags": ["adr-0002", "release-channels"], "input": "PR changes a caller stub's uses: ref from @dev-lead/v1-stable to a frozen @dev-lead/v1.4.0 tag to 'lock' the version.", "expected": {"risk_tier": "MEDIUM", "escalate": false, "cite_adr": "ADR-0002", "recommend": "keep the moving channel-tag pin; advance versions by moving the channel, not by freezing callers to an immutable release (ADR-0002)"}} {"id": "sa-dev-fork-reviewer-artifact", "description": "PR forks the reviewer to add a new artifact type instead of registering a rubric-registry row; drifts from the artifact-contract decision. Grounds: ADR-0003.", "tags": ["adr-0003", "review-registry"], "input": "PR adds support for reviewing a new artifact_type by copying and editing the reviewer prompt into a second variant, rather than adding a row to the rubric-registry manifest.", "expected": {"risk_tier": "MEDIUM", "escalate": false, "cite_adr": "ADR-0003", "recommend": "register the new artifact_type as a manifest row above engine.sh; do not fork the reviewer (ADR-0003 rubric-registry contract)"}} {"id": "sa-dev-inline-logic-no-bats", "description": "PR embeds branching decision logic directly inside a workflow step with no pure-logic function or bats test; drifts from the pure-logic+bats decision. Grounds: ADR-0004.", "tags": ["adr-0004", "testability"], "input": "PR adds a multi-branch decision (which repos to act on) as an inline bash block in a workflow YAML step, with no extracted scripts/lib function and no bats coverage.", "expected": {"risk_tier": "MEDIUM", "escalate": false, "cite_adr": "ADR-0004", "recommend": "extract the decision into a pure scripts/lib function with bats tests and keep the workflow thin (ADR-0004 pure-logic + bats)"}} +{"id": "sa-dev-write-mode-without-gate", "description": "PR silently upgrades an advisory persona surface to a write mode with no gate label; crosses the advisory-by-default boundary. Grounds: ADR-0005. Covers the escalate=true side so calibration toward 'no' cannot hide a missed escalation.", "tags": ["adr-0005", "boundary-violation", "escalate"], "input": "PR edits a persona manifest whose triggers.default_mode is advisory, changing its pull_request surface to mode: write so the persona can push fix commits and apply labels directly. No gate label or separate opt-in is added; the description says advisory-only was 'too slow'.", "expected": {"risk_tier": "HIGH", "escalate": true, "cite_adr": "ADR-0005", "recommend": "escalate: a write mode must be an explicit opt-in carrying its own gate label and is never a silent upgrade of an advisory surface; keep the surface advisory until a gated write mode is decided (ADR-0005)"}} +{"id": "sa-dev-overturn-pure-logic-adr", "description": "PR deliberately argues an accepted ADR is wrong and replaces a pure, bats-tested classifier with an LLM call on that basis; overturning an ADR needs a maintainer decision, not ordinary review. Grounds: ADR-0004.", "tags": ["adr-0004", "adr-overturn", "escalate"], "input": "PR deletes a pure scripts/lib classifier and its bats suite and routes the same deterministic decision through an LLM prompt instead. The description states that the 'LLM only where it must be' rule is outdated and that all classification should move to LLM prompts going forward.", "expected": {"risk_tier": "MEDIUM", "escalate": true, "cite_adr": "ADR-0004", "recommend": "escalate: the change deliberately overturns the pure-logic + bats decision rather than drifting from it; only a maintainer can supersede ADR-0004, so record a superseding ADR before merge or keep the pure classifier and its tests"}} diff --git a/prompts/solution-architect/advisory.md b/prompts/solution-architect/advisory.md index 51f444807..38662c8d1 100644 --- a/prompts/solution-architect/advisory.md +++ b/prompts/solution-architect/advisory.md @@ -79,9 +79,12 @@ unchanged. Skip step 3's fetches; do step 4 from the pre-fetched block. # From the listing, pick the ADR(s) relevant to this change and read ONLY # those — do not cat every ADR (the set grows; reading all of it bloats # context and can blow the token budget). - cat docs/architecture/adr/ADR-NNNN.md + # Files are named NNNN-.md, e.g. 0002-channel-tag-release-and-rings.md. + # Read one by its number (here ADR-0002): + cat docs/architecture/adr/0002-*.md ``` - Identify which ADR (by number, e.g. ADR-0002) governs the change under review. + Identify which ADR governs the change under review and cite it as + `ADR-NNNN` (file `0002-…md` is cited as ADR-0002). 3. **Gather the item's context**, read-only (skip this entirely in offline mode above — use the pre-fetched block instead): - PR: `gh pr view "$ITEM_NUMBER" --repo "$SOURCE_REPO" --json title,body,files` @@ -119,6 +122,54 @@ unchanged. Skip step 3's fetches; do step 4 from the pre-fetched block. ratifies drift by dressing preference up as policy. No ADR ⇒ name the gap and, if it matters, recommend recording the decision as a new ADR. + ### Calibrating the risk tier and escalation (read before you decide) + + The **risk tier** measures the structural blast radius if the change merges + as-is. **Escalate** means a maintainer must decide something before merge that + the author cannot settle in ordinary review. Decide each separately, and + state both explicitly. This is calibration, not caution: finding the right + ADR and then escalating a change that fits it is the defect this section + exists to prevent (#1787). + + - **The change conforms to an accepted ADR** — it follows the pattern the ADR + prescribes, or tightens the boundary the ADR draws → **LOW**, + **escalate = no**, Alignment **aligned**. Say it fits and stop. "What I'd + shore up" may be a single "Nothing structural — this is the pattern + ADR-NNNN prescribes." Do not manufacture follow-up checks, hardening ideas, + or adjacent concerns to fill the section; a terse correct advisory beats a + padded one. + - **The change drifts from an accepted ADR, and the ADR already names the + conforming alternative** — e.g. a bespoke per-item copy where the ADR + prescribes the shared mechanism, decision logic inlined where the ADR + prescribes a tested pure function, a frozen pin where the ADR prescribes a + moving channel, a fork where the ADR prescribes extension through the + decided contract → **MEDIUM**, **escalate = no**, + Alignment **drifts**. Recommend the conforming alternative and cite the + ADR. The decision is already recorded and the author can apply it in + review, so your recommendation is the remedy; nothing needs a maintainer. + - **No recorded ADR governs the change** → Governing ADR **none on record**, + Alignment **no decision on record**, **escalate = no**. Tier it **MEDIUM** + when the change sets a structural boundary others will build on (and + recommend recording it as a new ADR before it hardens into de-facto + architecture), **LOW** when it is structurally trivial. A missing ADR is a + gap to name, not a reason to escalate — the ADR process (ADR-0000) is the + remedy. + - **Escalate = yes is reserved for two situations:** + - **Merging as-is breaks a decided contract with consequences beyond this + PR.** Examples: callers start depending on something the release channel + they pin does not ship yet; a persona gains a write capability without + the opt-in gate its ADR requires; a change removes a boundary other repos + rely on. Tier these **HIGH**, **escalate = yes**. + - **The change deliberately overturns an accepted ADR** rather than + drifting by accident: it argues the decision is wrong, or cannot be built + any other way. Only a maintainer can supersede an ADR, so that needs a + decision before merge → **escalate = yes**. Tier it by blast radius, + usually **MEDIUM**, **HIGH** if it also breaks consumers. + - **HIGH always pairs with escalate = yes.** If you would not escalate it, it + is not HIGH. + - **When you rate a change aligned, escalate = no.** "Aligned" plus + "Escalate? yes" contradicts itself; recheck which one is wrong. + ## Output — how you deliver the advisory Print the comment body **between these exact sentinel lines**, each alone on its @@ -147,7 +198,11 @@ alone on its line: **Alignment:** aligned | drifts | no decision on record — one clause on why, measured against the cited ADR. -**What I'd shore up** (highest leverage first, up to 4 bullets): +**Risk tier:** LOW | MEDIUM | HIGH — one clause on why (see the calibration in +step 4). + +**What I'd shore up** (highest leverage first, up to 4 bullets; for an aligned +change, "Nothing structural" is a complete answer): - … **Escalate?** yes/no — if yes, the single reason. @@ -165,6 +220,10 @@ solution-architect:hands-off label. - **Cite an ADR by number, or say there is none.** Never assert architectural doctrine you cannot ground in a recorded ADR. If no ADR governs, name the gap plainly — do not ratify drift by inventing policy. +- **State the risk tier and the escalate decision explicitly,** every time, as + calibrated in step 4. Never leave the tier implied. Escalate = no is the + default for a change that fits an accepted ADR or drifts in a way the ADR + already corrects. - **Never write a literal `@petry-projects/`** anywhere in the body. Naming a live persona handle in your own output is a way to self-trigger the fleet. Refer to roles in prose ("the dev-lead persona"), never as a handle. diff --git a/tests/test_advisory_offline_mode.bats b/tests/test_advisory_offline_mode.bats index a446f0e55..7016eca3f 100644 --- a/tests/test_advisory_offline_mode.bats +++ b/tests/test_advisory_offline_mode.bats @@ -123,6 +123,70 @@ _steps_section() { grep -Eiq 'Cite an ADR by number, or say there is none' "$PROMPTS/solution-architect/advisory.md" } +# --- solution-architect escalation calibration (#1787) ---------------------- +# All three held-out failures in #1787 were wrong escalate decisions on otherwise +# correct advisories. The calibration below is what fixes them; these checks keep +# it from being silently dropped by a later edit or template sync. + +@test "solution-architect calibrates escalation inside its Steps section (#1787 AC1)" { + steps="$(_steps_section "$PROMPTS/solution-architect/advisory.md")" + grep -q 'Calibrating the risk tier and escalation' <<<"$steps" +} + +@test "solution-architect: a conforming change is LOW / escalate = no (#1787 AC2)" { + steps="$(_steps_section "$PROMPTS/solution-architect/advisory.md")" + # The whole bullet, from its opening line to the next sibling bullet, so a + # rewrap of the paragraph cannot move the asserted text out of range. + bullet="$(awk '/The change conforms to an accepted ADR/ { on=1; print; next } + on && /^ - / { exit } + on { print }' <<<"$steps")" + grep -q '\*\*LOW\*\*' <<<"$bullet" + grep -q '\*\*escalate = no\*\*' <<<"$bullet" +} + +@test "solution-architect: 'no ADR governs' is not itself an escalation (#1787 AC3)" { + steps="$(_steps_section "$PROMPTS/solution-architect/advisory.md")" + # The no-ADR bullet as one line, so a rewrap cannot split the asserted text. + bullet="$(awk '/No recorded ADR governs the change/ { on=1; print; next } + on && /^ - / { exit } + on { print }' <<<"$steps" | tr '\n' ' ' | tr -s ' ')" + [ -n "$bullet" ] + grep -q '\*\*escalate = no\*\*' <<<"$bullet" + ! grep -q 'escalate = yes' <<<"$bullet" + grep -q 'A missing ADR is a gap to name, not a reason to escalate' <<<"$bullet" +} + +@test "solution-architect: HIGH always pairs with escalate = yes" { + grep -q 'HIGH always pairs with escalate = yes' "$PROMPTS/solution-architect/advisory.md" +} + +@test "solution-architect step 2 names ADR files the way the corpus does" { + f="$PROMPTS/solution-architect/advisory.md" + # The corpus is docs/architecture/adr/NNNN-.md; there is no ADR-NNNN.md. + # A bare `! grep` mid-test cannot fail it (errexit ignores negated commands), + # so fail explicitly. + if grep -q 'docs/architecture/adr/ADR-' "$f"; then + echo "prompt must not reference docs/architecture/adr/ADR-NNNN.md" >&2 + false + fi + example="$(grep -oE '[0-9]{4}-[a-z0-9-]+\.md' "$f" | head -1)" + [ -n "$example" ] + [ -f "$ROOT/docs/architecture/adr/$example" ] + # The persona runs this command, so it must work as written: no `<…>` + # placeholder (the shell reads it as a redirection), and it must match a file. + cmd="$(grep -E '^[[:space:]]*cat docs/architecture/adr/' "$f" | head -1 | sed 's/^[[:space:]]*cat //')" + [ -n "$cmd" ] + if [[ "$cmd" == *"<"* ]]; then + echo "ADR read command contains a : $cmd" >&2 + false + fi + compgen -G "$ROOT/$cmd" >/dev/null +} + +@test "solution-architect body shape states the risk tier explicitly (#1787 AC4)" { + grep -q '^\*\*Risk tier:\*\* LOW | MEDIUM | HIGH' "$PROMPTS/solution-architect/advisory.md" +} + # --- engine-parity tier in scorer.json (AC #3) ------------------------------ @test "qa-lead scorer declares the persona (Opus) engine tier — the #1696 reference" {