fix: an open PR suppresses the reporter chase, which was oscillating - #640
Conversation
Parking an item that has an open PR does not settle it -- it OSCILLATES. `park` says move the card to Backlog; the column derivation says an item with an open PR is In Review; so the next pass reports `move_card` to put it back, and the pass after that parks it again. Forever. #162 did exactly this and it was watched happen: parked at 55 quiet days, immediately reported as a mis-placed card, restored, reported as needing a park again. That is the "same conversation every tick" failure the board work exists to end, reappearing between two rules inside this script rather than between the script and the maintainer. The rule is also right on its own terms, independent of the loop. An issue with a PR in flight is not a reporter chase whatever its `Awaiting` field says: the work exists, and the wait that matters is the review, which the PR half of this pass already reports. Nudging the reporter of a 46-day-stale conflicted PR asks the wrong person about the wrong thing. So `.pr != null` suppresses both `park` and `nudge_reporter`. The suppression does not reach the case the rule exists for: an issue with no PR still parks at 28 quiet days, pinned by `test_the_chase_still_fires_without_a_pr`, which passes both before and after the change. Verified the oscillation test fails against the pre-fix script and passes after. Live effect: 23 actions -> 22, with no `park`/`move_card` pair left to trade #162 back and forth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR
|
@claude-bot review |
There was a problem hiding this comment.
Review: fix/park-vs-open-pr
Root cause matches the diagnosis. scripts/backlog-digest.sh:339 derives column as "In Review" whenever .pr != null. Before this fix, scripts/backlog-rhythm.sh's park/nudge rule ignored .pr entirely and fired park purely off awaiting == "reporter" + quiet days. Once park moved the card to Backlog, the next pass's board_status != .column check (scripts/backlog-rhythm.sh:146) emitted move_card back to "In Review", and the pass after that parked it again — the exact loop #162 hit. .pr != null then empty as the first branch of the if/elif chain at scripts/backlog-rhythm.sh:109 correctly suppresses both park and nudge_reporter in one guard, matching the PR description.
Independently reproduced, not just trusted. I extracted both the pre-fix (main) and fixed (origin/fix/park-vs-open-pr) versions of backlog-rhythm.sh and ran them directly against hand-built digest fixtures (bypassing pytest, since this is pure bash+jq):
- Matched-column case (
column/board_statusboth"In Review",pr: 167,awaiting: "reporter", 28 quiet days): pre-fix →["park"]; fixed →[]. Confirms the fix. - No-PR guard case (
pr: null, same quiet days): fixed script still returns["park"]. Confirms the suppression doesn't swallow the case the rule exists for.
This matches the PR's own "revert to HEAD, re-run" evidence and CI (all green, including Fast tests / Code quality).
Fitness of approach:
- Best available fix, not just better-than-before — it directly encodes the real rule ("a PR in flight is not a reporter chase") rather than routing around the oscillation via e.g. an already-parked flag. No workaround shape (no new parameter/flag/second construction site) — straight condition added to the existing rule block it belongs in.
- Holds for valid inputs generally:
.pris sourced only fromgh pr list --state open(scripts/backlog-digest.sh:58), so it's null once a PR merges or closes — the suppression lifts naturally rather than persisting forever via a stale value. - Separation of concerns: the added branch lives inside the same "chase once, then park" jq expression it needs to gate — no responsibility leaked into an unrelated method.
- Scope is minimal and matches the PR description exactly: 2 files (
scripts/backlog-rhythm.sh+15/-1,backend/tests/test_backlog_rhythm.py+33), confirmed viagh pr view --json files. No scope creep. - This is clearly a local fix (one guard clause within an existing rule's existing contract), so the absence of an explicit "local vs structural" statement in the description is fine — not a structural-looking change that needed one.
Test quality: test_an_open_pr_suppresses_the_chase_and_the_park loops over both the park threshold (28d) and nudge threshold (14d) — matching the script's actual defaults (NUDGE_DAYS=14, PARK_DAYS=28 at scripts/backlog-rhythm.sh:39-40) — and asserts on the resulting action set (behavior), not implementation details. test_the_chase_still_fires_without_a_pr is the correct negative-space guard. Both would catch a regression; I confirmed this myself against the pre-fix script rather than relying on the PR's claim.
Rule violations: none found. No Optional[x], no hasattr/getattr fallbacks, no new classes, no exception-string matching, no API-layer or sensor-access code touched. Comments in the diff explain non-obvious WHY (the oscillation mechanism, tied to #162), consistent with the file's existing style.
No blockers. Approving.
Problem
Parking an item that has an open PR does not settle it — it oscillates.
park— awaiting reporter, quiet 55dmove_card, put it backparkagain#162 did exactly this, and it was watched happening during a backlog pass: parked at 55 quiet days, immediately reported as a mis-placed card, restored, reported as needing a park again.
That is the "same conversation every tick" failure the board work (#636) exists to end — reappearing between two rules inside this script, rather than between the script and the maintainer.
Fix
.pr != nullsuppresses bothparkandnudge_reporter.The rule is right on its own terms too, independent of the loop: an issue with a PR in flight is not a reporter chase, whatever its
Awaitingfield says. The work exists, and the wait that matters is the review — which the PR half of this pass already reports. Nudging the reporter of a 46-day-stale conflicted PR asks the wrong person about the wrong thing.Evidence the test discriminates
Reverted the script to
HEADand re-ran the two new tests:move_cardremains to oscillate against, at both the 14-day and 28-day thresholds.Test plan
./scripts/quality-check.shgreenbackend/tests/test_backlog_rhythm.pypark/move_cardpair left trading Addexternal_solar_modebattery setting for AC-coupled PV systems #162 back and forth.Outcome-level coverage
Not applicable — no optimizer path touched. The pins above are the coverage.
Scope
scripts/backlog-rhythm.shand its test file. No CHANGELOG entry: agent tooling, no user-visible effect.🤖 Generated with Claude Code