Compute the tier-1 analyze carve-out from issue evidence, so a stale Awaiting cannot hide a qualifying bug - #685
Conversation
Recovered from a session that exited before committing. See #681. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JM9cNxMtHVvZjw84Q9u9jU
…1-stale-awaiting-hides-tier1
…d field The autonomous Stage 2 carve-out was only ever evaluated against items the rhythm pass had already surfaced as actions. Any item that produced no other action was therefore invisible to it -- including #680, a `bug` + `ready-for-analysis` report from an external reporter with a debug bundle attached, which sat unanalysed because its card said `Awaiting: reporter`. A board field that grooming can leave stale silently disabled the one path that fires Stage 2 without a human trigger. `tier1_ready` now derives the condition from the issue's own evidence -- the `bug` label, the `ready-for-analysis` stamp that is triage's own confirmation the log is attached, the absence of any Stage 2 label, and a non-maintainer author -- and emits a dedicated `autonomous_analyze` action that no wait value can suppress. `Awaiting: maintainer` is the deliberate exception: there the loop is held for a decision, so the escalation stays loudest and the spend stands down. The same predicate negates into the `park`, `nudge_reporter` and `surface_discussion` guards. Without that, one pass could tell the PO to chase or bury the exact reporter whose log is already in hand -- the two actions contradict, and sharing one predicate makes contradicting each other impossible rather than merely unlikely. Refs #681 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JM9cNxMtHVvZjw84Q9u9jU
|
@claude-bot review |
There was a problem hiding this comment.
Review
Root cause match: Yes. #681 diagnosed the bug precisely — the autonomous Stage 2 carve-out was only ever evaluated on items the rhythm pass already surfaced, so a stale Awaiting: reporter/discussion board value silently hid a qualifying tier-1 bug (#680) from the pass entirely. The fix implements the issue's own proposed option 1: compute tier1_ready in scripts/backlog-rhythm.sh from issue evidence (bug label, ready-for-analysis label, absence of analyzed/needs-human-review, non-maintainer author, awaiting != "maintainer") and evaluate it independently, rather than depending on the item surfacing via some other rule.
I verified this empirically rather than trusting the description, by fetching the PR branch into a scratch worktree and running the raw script (no pytest available in this environment, but bash+jq are) against a synthetic digest reproducing the #680 shape (bug+ready-for-analysis, Awaiting: reporter, 0 comments):
- On
main(pre-fix): zero actions emitted for the item — confirms the bug is real, not just claimed. - On this branch:
autonomous_analyzefires for it. - A second case with
Awaiting: maintainercorrectly staysescalatedonly (autonomous_analyzedoes not override the deliberate maintainer hold), and adiscussion-awaiting tier-1 bug firesautonomous_analyzewithsurface_discussioncorrectly suppressed — matching the "one predicate, shared by the emitter and the negated chase guards" design described in the PR body.
Test coverage: Strong. Ten new tests in backend/tests/test_backlog_rhythm.py, each exercising the real script via subprocess against a seeded digest (behavior, not implementation — consistent with docs/agents/testing.md). The PR body documents a mutation sweep (each guard clause removed individually, each triggers a distinct test failure) which is exactly the "must be seen to fail without its fix" bar from docs/agents/rules.md Testing section, and is more rigorous than that bar requires.
Minimal / scope: Yes. Touches only scripts/backlog-rhythm.sh (the file that already owns "decide what's due"), its tests, .claude/skills/backlog/SKILL.md docs, and CHANGELOG.md. No new class, no new file, no unrelated cleanup. docs/agents/workflow.md's existing description of ready-for-analysis (label 156: "Debug log present, awaiting @claude-bot analyze") stays consistent with the new evidence-based logic — no doc contradiction introduced.
Rule compliance:
- Workaround check (
rules.mdDebugging Protocol step 8): the fix is direct, not a route-around —tier1_readyis one predicate computed once and shared by the emitter and the three chase-guard negations, not a second construction site or an extra flag bolted onto existing rules. - Separation of concerns:
autonomous_analyzefitsbacklog-rhythm.sh's existing contract (deterministic evidence comparisons that decide what's due) — it doesn't take on any new responsibility the file didn't already have. - jq injection:
--arg owner "$owner"is passed as a bound jq variable, not interpolated into the program string — no injection surface. - No Optional/hasattr/getattr, no new classes, no exception-string matching — none applicable (shell + jq + pytest-style subprocess tests only).
No blockers found. Approving.
Summary
autonomous_analyzeaction inscripts/backlog-rhythm.sh, ranked with the other Analysis actions (rank 5).park/nudge_reporter/surface_discussionfor those items, so one pass cannot both un-hide a bug and tell the PO to bury it.Root cause
From #681:
#680 met every tier-1 criterion —
bug, external reporter (ridax67), debug bundle attached, no prior@claude-bot analyze— and never fired, because its card saidAwaiting: reporterand the pass suppresses every "someone else owes us" wait value.Verifying against current code confirmed the failure is one step broader than the stale field alone: the carve-out was evaluated only against items that produced some other action, so a correctly-groomed but otherwise-quiet card was equally invisible. The live board proves it — #680's
Awaitinghas since been corrected toanalysis, and the pre-fix script still does not surface it (see Test plan).Fix
tier1_readyin the jq program derives the condition from evidence only:buglabelready-for-analysislabelanalyzed/needs-human-reviewlabelready-for-analysison an edited issue, so the label alone would re-fire the $0.50–2 spend every/looptick on items analysis has already settledauthor != $ownerawaiting != "maintainer"Two consumers share that single predicate: the
autonomous_analyzeemitter, and (negated) the three chase guards. Sharing it is the point — with the log in hand the ball is not with the reporter, sopark/nudge_reporter/surface_discussionchasing them in the same pass would contradict the carve-out. One predicate makes that contradiction impossible rather than merely unlikely.The no-prior-analyze check deliberately stays a PO-time step (documented in the action's
detailand in.claude/skills/backlog/SKILL.md), because that is also what catches an analyze still in flight — which keepsready-for-analysisuntil it completes.Documentation
.claude/skills/backlog/SKILL.md— updated: theAwaitingsigned table gains the carve-out note, the action table gains theautonomous_analyzerow, and the Autonomous-spend section is rewritten to describe the evidence basis.docs/agents/bess-knowledge.mdanddocs/SOFTWARE_DESIGN.md— checked, neither mentions anything this diff touches (backlog tooling, not BESS behaviour).docs/agents/workflow.md— checked; itsready-for-analysislabel row stays accurate as written.Test plan
./scripts/quality-check.shpasses locally — 2276 passed, 50 skipped; Black/Ruff/mypy, frontend, permission surface, bot-workflow contracts all green.venv/bin/pytest -m slowpasses — 554 passed, 8 skippedcode-reviewon the diff — zero findingsStep 8, observed against the live board. Fetched a real
backlog-digest.shsnapshot and replayed it through the pre-fix and post-fix scripts:Human-readable output for the new action:
Not a synthetic positive:
gh issue view 680 --json commentsreturns exactly one comment (triage's), so there is no prior analyze and Late switching #680 genuinely still needs Stage 2 fired. Every other action is byte-identical before and after, so the change adds the missing action without perturbing the rest of the pass.Evidence the test discriminates
Ten new tests. The whole fix reverted, then each guard clause removed individually from the fixed tree:
backlog-rhythm.shchange revertedand .awaiting != "maintainer"test_a_tier1_bug_awaiting_the_maintainer_escalates_instead_of_firingFAILED — 1 failed, 83 passedand (.labels | index("analyzed")) == nulltest_a_tier1_bug_with_stage2_history_does_not_fireFAILED — 1 failed, 83 passedand (.labels | index("needs-human-review")) == nulltest_a_tier1_bug_with_stage2_history_does_not_fireFAILED — 1 failed, 83 passedand .author != \$ownertest_a_bug_opened_by_the_maintainer_does_not_fireFAILED — 1 failed, 83 passedand (.labels | index("ready-for-analysis")) != nulltest_a_bug_still_waiting_for_its_log_does_not_fire+test_quiet_backlog_is_a_noopFAILED — 2 failed, 82 passedRestored: tree clean, 84/84 pass. No guard in the predicate is vacuous — every one of them has a test that goes red when it is deleted, which is the property the negative "does not fire" tests would otherwise only appear to have.
Outcome-level coverage
Not a DP/intent/control-mapping change, so
run_scenario_realizeddoes not apply. The outcome pinned is the pass's emitted action set, asserted through the script's real--jsonoutput via the existing_run/_actions_forharness inbackend/tests/test_backlog_rhythm.py— the tests executebacklog-rhythm.shitself against a seeded digest (RHYTHM_DIGEST_FILE), not a reimplementation of its rules. Both halves are covered: the action appears (5 tests), and the contradicting chases disappear (3 of those same tests assertpark/nudge_reporter/surface_discussionabsent).Refs #681