Stop Stage 2 reading the sub-agent's own system prompt, and give step 4 a check that fires - #655
Conversation
Stage 2's main agent listed `.claude/agents/bess-analyst.md` (25,681 B) as REQUIRED READING item 3. That file IS the bess-analyst sub-agent's system prompt -- frontmatter `name: bess-analyst`, `tools: Read, Grep, Glob, Bash, WebFetch` -- so the main agent was loading a second copy of another agent's instructions into its own context, on every turn. Nothing consumed it. The six PROCESS steps are: get issue context, identify the current problem, delegate, verify the cited file:line, publish, label. No step applies a "domain expertise checklist". The one distinctive task, step 4, needs the cited code -- not a triage procedure the main agent never runs. The content was in fact loaded three times. The section that could plausibly serve as a judging standard, "Separate Evidence from Claims", is already restated almost verbatim in the sub-agent task the same prompt passes: hypothesis-not-diagnosis, triage-the-bundle-first, verify-the-code-path- matches-this-user's-setup, intentional-design-is-not-a-bug. Sub-agent system prompt, main-agent file read, and inline paraphrase. All four landed Stage 2 analyses (#118, #252, #624, #627) use the workflow's four headings, never bess-analyst.md's seven-item Output Format or its six-item type-B shape -- the main agent follows the prompt, not the file. So item 3 is removed, with an explicit note saying why, and step 4 gains the check that was missing: a report fails verification if it claims a CODE bug with no file:line, if a cited location does not say what is claimed, or if it blends P-optimality / forecast error / control noise into one verdict when actual behavior diverged from plan. Dead weight becomes an instruction that fires. Three things the first draft got wrong, found in review: - "cites no file:line" alone would have rejected CORRECT diagnoses. The same prompt tells the analyst that unavailable sensors, wrong inverter type or an HA integration mismatch are likely the real cause, and those cite no code. Now qualified to code-bug claims only. - The inconclusive path never removed `ready-for-analysis`, and backlog-digest.sh checks that label BEFORE `needs-human-review`, so the backlog pass reports the issue as un-analysed and re-dispatches analyze -- re-billing the stage. Widening that path without fixing it would have made a cost bug worse. It now clears the label. - The new criterion used vocabulary defined only in the file this diff stops the agent reading. P-optimality had a gloss; P≠R never expanded P or R and control noise had no definition. All three are now glossed inline. quality-check.sh gains a context contract gate, written RED first: no workflow may list a `.claude/agents/*.md` file as a numbered required read, and Stage 2 must still delegate. Both assertions were mutation-tested -- removing the delegation reddens it, and reformatting `subagent_type` with backticks or quotes does not, which the first bare-substring version got wrong in both directions. Saving is up to 25,681 B off the main agent per turn; the prompt itself grew 7,098 -> 7,821 B, so net is roughly -24.9 KB. "Up to", because whether a run obeyed the read instruction is not observable -- #646 showed it need not. No CHANGELOG entry: agent-facing, zero user-visible effect, as #649 and #651. Closes #654
|
@claude-bot review |
There was a problem hiding this comment.
Summary
This matches the linked issue's (#654) root cause: .github/workflows/issue-analyze.yml:59 previously listed .claude/agents/bess-analyst.md as the main agent's REQUIRED READING item 3, but that file is the bess-analyst sub-agent's own system prompt — a second, unused copy loaded into a different context window on every Stage 2 turn. I independently confirmed the two load-bearing claims from the PR body rather than taking them on faith:
- The
## CRITICAL: Separate Evidence from Claimssection ofbess-analyst.mdis indeed already restated almost verbatim in the sub-agent task text atissue-analyze.yml:79-113(reporter-hypothesis framing, triage-before-code-reading order, HA-integration/inverter-type check, "intentional per design docs" check) — the duplication claim holds up. scripts/backlog-digest.sh:275does checkready-for-analysisbeforeneeds-human-reviewinawaiting_from_labels, confirming the claimed bug: the inconclusive path (issue-analyze.yml:202, pre-fix) addedneeds-human-reviewwithout removingready-for-analysis, which would leave the issue reading as "not yet analyzed" to the digest and risk re-dispatching (re-billing) the stage. The fix adds--remove-label ready-for-analysisto that path, matching what the success path (issue-analyze.yml:187) already does.
Test coverage
The new scripts/quality-check.sh gate (a python heredoc checking (a) no numbered .claude/agents/*.md read in any workflow, (b) subagent_type still references bess-analyst inside the prompt: block) is real, discriminating coverage, not a tautology. I extracted the gate logic and ran it standalone against both branch states:
- Against
main(pre-fix): RED — correctly flags.github/workflows/issue-analyze.yml:59: main agent told to read a sub-agent definition: 3. .claude/agents/bess-analyst.md — domain expertise checklist. - Against the PR branch: GREEN.
I also re-verified the specific mutation claims in the PR description directly:
- Backtick form
subagent_type: `bess-analyst`(the actual form used atissue-analyze.yml:79) → matches. - Quoted form
subagent_type: "bess-analyst"→ matches. - The delegation string present only outside the
prompt: |block (simulating an emptyprompt_block) → does not match, i.e. correctly fails the gate.
So yes — this test would catch a regression on either half of the fix (the read-list removal being un-done, or the delegation being silently dropped), not just the one that motivated it.
Minimality / scope
Diff touches exactly the two files described: .github/workflows/issue-analyze.yml (+32/−2) and scripts/quality-check.sh (+62/−0). Nothing else changed. The PR body states the scope assessment explicitly per rules.md's Debugging Protocol step 9: "local — Stage 2's prompt and one gate," and I agree with that classification — no new parameter/flag/second construction site, the inconclusive-path label fix reuses the existing gh issue edit call rather than adding a new mechanism.
bash -n on the modified quality-check.sh passes (no syntax errors), and the heredoc uses a quoted delimiter (<<'PY_CTX') so there's no shell-injection surface from the embedded regex/backticks. CI is green (Code quality, Fast tests, E2E tests, Frontend checks, Merge gate all pass).
Nit (non-blocking)
The PR body itself flags, under "Documentation check," that this diff creates a second, abbreviated definition site for the P-optimality / forecast-error / control-noise taxonomy (now inline in issue-analyze.yml, previously only in bess-analyst.md), which can drift from the original. That's called out as accepted and out of scope here, and I agree it's reasonable to leave for a follow-up rather than block this PR on it.
Verdict
No rule violations found (no Optional[x]/hasattr/getattr-default patterns, no new classes, no exception-string matching, no hardcoded entity IDs — none of these apply to this diff's surface, which is workflow prompt text and a bash/python quality gate). Root cause matches the issue, the new test is genuinely RED→GREEN discriminating (verified by mutation, not just claimed), and the change is minimal.
Summary
.claude/agents/bess-analyst.md(25,681 B) — that file is the sub-agent's own system promptquality-check.shgains a context-contract gate (written RED first, both assertions mutation-tested)Root cause
.claude/agents/bess-analyst.mdis thebess-analystsub-agent's system prompt — frontmattername: bess-analyst,tools: Read, Grep, Glob, Bash, WebFetch. Listing it as the main agent's REQUIRED READING loaded a second copy of another agent's instructions into a different context window, on every turn.Nothing consumed it. The six PROCESS steps: get issue context → identify the current problem → delegate → verify the cited
file:line→ publish → label. No step applies a "domain expertise checklist"; step 4 needs the cited code, not a triage procedure the main agent never runs.It was loaded three times. The section most plausibly serving as a judging standard,
## CRITICAL: Separate Evidence from Claims, is already restated almost verbatim in the sub-agent task the same prompt passes:bess-analyst.mdThe artifacts agree. All four landed Stage 2 analyses (#118, #252, #624, #627) use the workflow's four headings — never
bess-analyst.md's seven-item Output Format or its six-item type-B shape. The main agent follows the prompt, not the file.Fix
Remove item 3, with an explicit note saying why, and give step 4 the check that was missing. A report fails verification if it claims a code bug with no
file:line, if a cited location doesn't say what's claimed, or if it blends P-optimality / forecast error / control noise into one verdict when actual behavior diverged from plan. A failed report routes to the existing inconclusive path rather than being dressed up as a root cause — no new mechanism.Review findings fixed
Three were defects in my own first draft:
file:line" would reject correct diagnosesready-for-analysisbacklog-digest.sh:275checks that label beforeneeds-human-review, so the issue reads as un-analysed and gets re-dispatched — re-billing the stage. Widening that path without fixing it would have made a cost bug worse. Verified against the source; now clears the label.P≠Rnever expandedPorR; control noise had none. All three glossed inline.Two hardened the gate:
WF.glob("*.yml")missed.yamlworkflows, and the delegation pin was a bare substring over the whole file — a benign reformat would raise a hard ERROR falsely claiming the delegation was gone, while a leftover mention in a comment would satisfy it after the real instruction was deleted.Test plan
./scripts/quality-check.sh— Errors: 0, Warnings: 0.venv/bin/pytest -m slow— 554 passed, 8 skipped (419 s)No mock-HA surface — this is a prompt change with no runtime behaviour. What was observed instead is the gate's RED→GREEN transition, the mutation results below, and the parsed prompt.
Evidence the test discriminates
Both assertions were written before the fix and mutation-tested:
Read-list assertion
3. .claude/agents/bess-analyst.md — domain expertise checklist→ gate REDDelegation assertion
subagent_type: bess-analyst→general-purpose: gate RED,restored: Truesubagent_type: `bess-analyst`: passes (no false ERROR)subagent_type: "bess-analyst": passesprompt:block: fails (a comment can't satisfy it)The last three are the ones the first bare-substring version got wrong.
Outcome-level coverage
quality-check.sh's context-contract gate pins both halves: no sub-agent definition as a numbered required read in any workflow, and Stage 2 still delegating. No optimizer fixture or golden applies — the diff touches no runtime code.<path>" instruction would slip through. That's accepted — the prompt now carries an explicit "Do NOT read" note, and a matcher broad enough to catch every phrasing would flag that note too, i.e. fight its own documentation.Documentation check
Neither
docs/agents/bess-knowledge.mdnordocs/SOFTWARE_DESIGN.mddocuments the P-optimality / forecast-error / control-noise taxonomy — it lives only in.claude/agents/bess-analyst.md, so there was nothing to update. Worth flagging: this diff creates a second, abbreviated definition site in the workflow prompt, which can drift from the analyst file. Consolidating the taxonomy intobess-knowledge.mdand referencing it from both would fix that, and is out of scope here.Size
Saving is up to 25,681 B off the main agent per turn. The prompt itself grew 7,098 → 7,821 B, so net is ≈ −24.9 KB. "Up to", because whether a given run obeyed the read instruction isn't observable — #646 showed it need not.
Scope assessment
Adds no parameter, flag, default-fallback, second construction site or extra trigger. The failed-verification path reuses the existing inconclusive route rather than inventing a retry loop. Scope is local — Stage 2's prompt and one gate.
Closes #654