fix: enforce mypy on changed files, and make the PR review bot always submit a verdict - #614
Conversation
Three gaps found while merging #612: 1. rules.md requires mypy but nothing ran it -- not quality-check.sh, not CI, and it was not in requirements-dev.txt. Repo-wide is not an option: measured 2914 errors across 191 files (417 outside tests). So the gate now runs mypy against files changed vs origin/main, enforcing the rule going forward while the legacy backlog burns down as files get edited. Untracked files are included -- a new file is untracked until its first commit, and this gate runs before that commit; leaving them out let a probe file with a known error pass silently. 2. pr-review.yml told the bot to submit a summary review but did not make it mandatory, so a clean run posted nothing. GitHub blocks on the LAST explicit verdict, which means a silent clean run can never clear a prior REQUEST_CHANGES -- #612 had to be dismissed by hand. Now required in every case, with the reason stated. 3. CLAUDE.md: a failed keychain read makes gh fall back to a scopeless token silently, so the symptom is Forbidden on an operation you do have rights to. Records how to tell the two apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZWVYVveH7RFMTe9ikVfaQ
backlog-board-init.sh creates the Projects v2 board, idempotently. It runs as the MAINTAINER, not the PO: a user cannot create a project inside another user's account, and the board deliberately lives under the maintainer's account since it is their backlog. Verified: the PO token gets 'unknown owner type' addressing johanzander's projects, and cannot even list its own without read:org + read:discussion -- scopes not worth granting for a CLI quirk. Also closes the design's last unverified assumption. Against the real board (project 1, issue #611 set to P1), gh project item-list --format json returns each custom single-select field as a top-level key on the item -- priority: "P1" -- alongside status and content.number. The digest read it back correctly end to end, so the assumed jq path was right. Both the script comment and the skill's Prerequisites now record the confirmed shape instead of warning about an open question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZWVYVveH7RFMTe9ikVfaQ
Two things only a real board could reveal: 1. The board uses 'Ready for Dev' and 'In Progress', not the spec's 'Ready' and 'In progress'. Columns are compared as strings, so the mismatch would have stranded every card. The board is authoritative for its own column names, so the digest, skill and tests adopt them. 2. The PO cannot use 'gh project --owner johanzander' at all -- the CLI resolves the owner via an API needing read:org, which the PO token deliberately lacks. Granting project access does not change this. Board reads and writes as the PO go through gh api graphql addressed by node id instead; verified working for both. No scope widening needed. Also records why this jq program may contain no apostrophes: it is a single-quoted shell string, and the one I introduced truncated the whole program, leaving only 'Top-level program not given'. Live digest against project 1: Analysis 15, Backlog 13, In Progress 4, In review 3, Ready for Dev 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZWVYVveH7RFMTe9ikVfaQ
The board column is 'In Review', not 'In review' — read back from the board itself, which is authoritative. Fixed in the digest, the skill, the tests and the board-init instructions. Also fixes a real bug in the mypy gate added earlier this branch: a file both committed on the branch and dirty in the working tree appeared in two of the three sources, and mypy rejects a duplicate path with 'Duplicate module named ...'. sort -u. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZWVYVveH7RFMTe9ikVfaQ
The skill claimed the PO moves cards to Done when their issue closes. It cannot: the digest fetches only open issues, so a closed one leaves the evidence entirely and Done is underivable by construction. Five columns are derived; Done is filled by the board built-in item-closed workflow and ages out via auto-archive, which is why a Done card is invisible to the agent. Removes the impossible reconcile rule and states the division in both the skill and the digest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZWVYVveH7RFMTe9ikVfaQ
|
@claude-bot review |
| echo "🔸 Checking mypy on changed files..." | ||
| base=$(git merge-base origin/main HEAD 2>/dev/null || echo "") | ||
| if [ -z "$base" ]; then | ||
| echo "⚠️ Cannot resolve origin/main — skipping mypy (run: git fetch origin main)" |
There was a problem hiding this comment.
An unresolvable origin/main downgrades the whole mypy gate to a WARNING, and WARNINGS never fail the script (see the exit logic at the bottom: elif [ $ERRORS -eq 0 ]; then ... exit 0). So in a shallow clone or a worktree that hasn't fetched origin/main, quality-check.sh prints Errors: 0 and exits 0 having never run mypy at all.
That's exactly the failure mode this same diff calls out 20 lines above for the missing-tool case:
# A missing tool is an ERROR, not a warning: this script is the pre-commit
# gate, and skipping its three most important checks while printing
# "Errors: 0" reports success for a run that verified nothing.
The mypy branch should follow the same rule — ERRORS=$((ERRORS + 1)) here, not WARNINGS.
|
|
||
| scripts/gh-agent.sh --as po api graphql -f query='...' | ||
| project id PVT_kwHOACEigM4Bgiwa | ||
| Priority PVTSSF_lAHOACEigM4Bgiwazhfh7NQ (P0 da61340b, P1 131c5c2f, P2 107b9947) |
There was a problem hiding this comment.
This line (new in this PR) still lists P0 da61340b as a live Priority option:
Priority PVTSSF_lAHOACEigM4Bgiwazhfh7NQ (P0 da61340b, P1 131c5c2f, P2 107b9947)
But project_board_field_ids.md, added by this same PR, says the opposite:
Priority field: ... options are P1
131c5c2f, P2107b9947, P36d4b1494, P44d153125. There is no P0 option — the backlog skill/task text that references "P0 da61340b" is stale; that option id does not exist on the live field.
This PR is introducing the stale claim into SKILL.md in the same diff that documents it as stale elsewhere. Should be fixed to match the confirmed P1–P4 values before merge.
| # of a built-in single-select field. | ||
| gh project field-create "$number" --owner "$owner" \ | ||
| --name "Priority" --data-type SINGLE_SELECT \ | ||
| --single-select-options "P0,P1,P2" >/dev/null |
There was a problem hiding this comment.
--single-select-options "P0,P1,P2" hardcodes a 3-option Priority field (P0/P1/P2). Per project_board_field_ids.md (added in this same PR), the confirmed live board's Priority field has P1–P4, with no P0 option at all. If this script is ever run to (re)create the board, it produces a field that doesn't match what the rest of this PR documents as ground truth — including the field IDs referenced in SKILL.md and the memory files.
There was a problem hiding this comment.
Summary
Scope: this PR does not match its own description, and that's the primary blocker. The title and body describe three things: mypy enforcement in quality-check.sh, mandatory verdict submission in pr-review.yml, and the gh scopeless-token-fallback note in CLAUDE.md. Those three are fine. But the diff also contains, entirely undescribed:
.claude/skills/backlog/SKILL.md— substantial rewrite of board column names/workflow (Ready→Ready for Dev,In progress→In Progress, GraphQL-vs-CLI guidance, a new "Board writes go through GraphQL" section)- Two new PO memory files under
.claude/agent-memory/product-owner/ - A new script,
scripts/backlog-board-init.sh(86 lines) scripts/backlog-digest.shcolumn-name changes + matching test updates inbackend/tests/test_backlog_digest.py
None of this is mentioned in the PR body. This is more than half the diff by file count, touching a completely orthogonal concern (PO board bootstrapping/reconciliation) from the stated mypy/review-bot fixes. Per CLAUDE.md's Scope Discipline ("After editing, list every file and symbol changed so the user can confirm nothing unrelated was touched") and the checklist's "whether the change is minimal (no scope creep)," this needs to be split into two PRs, or the description needs to fully account for the board changes so they can be reviewed on their own merits.
And the undisclosed portion has a real internal contradiction (see inline comments): SKILL.md and scripts/backlog-board-init.sh both encode a P0 Priority option that this PR's own new memory file (project_board_field_ids.md) says doesn't exist on the live board ("There is no P0 option ... the backlog skill/task text that references 'P0 da61340b' is stale"). The PR introduces the stale claim in the same diff that documents it as stale — evidence this bundle wasn't reviewed as a unit before being folded into this PR.
On the stated changes themselves:
- Root cause match: yes — mypy genuinely wasn't wired into any gate, and
pr-review.ymlgenuinely lacked a mandatory-verdict instruction, matching the described gaps from landing #612. - Test coverage: no new automated test proves the mypy gate's behavior (the PR body describes manual verification of the three states — clean, dirty, no-Python-changed — but nothing in
backend/tests/exercisesquality-check.sh). Thepr-review.ymlandCLAUDE.mdchanges are prompt/doc text, not independently testable. - Minimal/scope: no, per above — the mypy/review-bot fix is minimal, but it isn't the whole PR.
- Scope-assessment statement (
rules.mdDebugging Protocol step 9): the PR body states the mypy scoping rationale (changed-files-only vs repo-wide) reasonably well for that piece, but says nothing about the backlog/board changes' scope or why they're bundled here.
Bug found in the stated change: scripts/quality-check.sh line 120 — when origin/main can't be resolved, the mypy check degrades to a WARNING, and warnings don't fail the script (exit 0 on the warnings-only path at the bottom of the file). That silently reports Errors: 0 for a run that never checked types — the exact anti-pattern this same diff's comment above the missing-tool branch calls out ("skipping its three most important checks while printing 'Errors: 0' reports success for a run that verified nothing"). Should be ERRORS, not WARNINGS.
Requesting changes: split out the backlog/board changes (or fully describe and justify bundling them), fix the stale P0 reference, and fix the mypy warning-vs-error severity.
…Step 0) Sessions die mid-issue routinely and nothing picked them up. A fleet audit found 34 worktrees whose sessions had exited: 8 with real unpushed commits and no PR (one with 32 commits), plus three PRs sitting green-or-reviewed with no owner left. #615 was APPROVED and still a draft the next morning; #614 carried CHANGES_REQUESTED with nobody to act on it. `sweep-prs` refuses that job by design, so the work simply stopped. This lives in `implement-issue` rather than a new skill because the loop that acts on review feedback is Step 11 and already lives here. A second skill would duplicate it, and duplicating a review loop is how one of them goes stale. Step 0 keys off state observable from OUTSIDE the dead session — branch, worktree, commits, PR body sections, CI status, review verdict — and re-enters at the earliest incomplete step. The one thing that dies with the session is Step 2's diagnosis, which Step 11 depends on holding; it is recoverable only because this skill already forces it to be written down (the Stage 2 analyze comment, and the PR body's `## Scope assessment` and `## Test plan`). When those do not reconstruct a coherent approach, Step 0 STOPS rather than re-diagnosing on top of commits encoding decisions it cannot see. Hard rules, each from an observed failure: - never run Step 4's fresh-from-origin/main worktree when a branch for the issue already has commits — that deletes them - never reset or force-push a resumed branch; its commits are the only copy - check for a live session unscoped AND unsandboxed: a sandboxed `claude agents --json` returned 1 session where the truth was 17, because ~/.claude/jobs is sandbox-denied, so every other session read as dead - treat uncommitted tracked changes as unfinished work; WIP-commit first - if the same issue has died twice, say so and stop CI mode gets a Step 0 row too: Stage 3 is re-triggered by hand, so a second `@claude-bot fix` on an issue that already has a has-fix-pr PR is a resume, not a restart, and must not open a second PR.
Session died without starting — picking this up needs a fresh runA background session ( Recording the work here so the next attempt does not have to reconstruct it from a dead session's transcript. Decision taken: split this PR in twoPer the review's primary blocker (undescribed, orthogonal board changes making up over half the diff): Stays in #614 — the changes this PR actually describes:
Moves to a new branch off
Two defects to fix while splitting
Note for whoever picks this upThe board changes overlap with pending work: the digest currently derives Both branches must pass |
…sume for dead sessions (#622) * fix: wait for a terminal review verdict, not the bot's placeholder `request-pr-review.sh` took the LAST review newer than its trigger and called it the verdict. The review bot posts its inline notes first, as a COMMENTED review whose body is "Inline notes below; summary review to follow.", then submits the real APPROVED/CHANGES_REQUESTED summary seconds later. Measured on PR #617: placeholder at 06:57:13Z, APPROVED at 06:58:03Z — 50 seconds apart. Any poll landing in that window returned COMMENTED. `implement-issue` Step 11 then saw a non-APPROVED verdict and skipped `gh pr ready`, so an approved PR stayed a draft with nothing left to do but the merge. PR #615 sat that way overnight: CHANGES_REQUESTED, fixed, APPROVED at 21:13, still a draft the next morning. Filter on state BEFORE taking `last`, so only APPROVED or CHANGES_REQUESTED ends the wait. Verified against #617's real review history by simulating a poll at 06:57:30Z, when the placeholder was the newest review: the old expression returns COMMENTED, the new one returns empty and keeps waiting. The timeout path also conflated two opposite faults that printed the same message — a review that started and never summarised, versus a trigger that never reached the workflow. It now reports which one happened. PR #619 is currently the second kind, and that was invisible before. Step 11's own text told the agent to act on `COMMENTED`, so it is corrected to match; a bare COMMENTED can no longer reach the caller at all. * feat: resume an issue whose session died mid-flight (implement-issue Step 0) Sessions die mid-issue routinely and nothing picked them up. A fleet audit found 34 worktrees whose sessions had exited: 8 with real unpushed commits and no PR (one with 32 commits), plus three PRs sitting green-or-reviewed with no owner left. #615 was APPROVED and still a draft the next morning; #614 carried CHANGES_REQUESTED with nobody to act on it. `sweep-prs` refuses that job by design, so the work simply stopped. This lives in `implement-issue` rather than a new skill because the loop that acts on review feedback is Step 11 and already lives here. A second skill would duplicate it, and duplicating a review loop is how one of them goes stale. Step 0 keys off state observable from OUTSIDE the dead session — branch, worktree, commits, PR body sections, CI status, review verdict — and re-enters at the earliest incomplete step. The one thing that dies with the session is Step 2's diagnosis, which Step 11 depends on holding; it is recoverable only because this skill already forces it to be written down (the Stage 2 analyze comment, and the PR body's `## Scope assessment` and `## Test plan`). When those do not reconstruct a coherent approach, Step 0 STOPS rather than re-diagnosing on top of commits encoding decisions it cannot see. Hard rules, each from an observed failure: - never run Step 4's fresh-from-origin/main worktree when a branch for the issue already has commits — that deletes them - never reset or force-push a resumed branch; its commits are the only copy - check for a live session unscoped AND unsandboxed: a sandboxed `claude agents --json` returned 1 session where the truth was 17, because ~/.claude/jobs is sandbox-denied, so every other session read as dead - treat uncommitted tracked changes as unfinished work; WIP-commit first - if the same issue has died twice, say so and stop CI mode gets a Step 0 row too: Stage 3 is re-triggered by hand, so a second `@claude-bot fix` on an issue that already has a has-fix-pr PR is a resume, not a restart, and must not open a second PR. * fix: resolve an ambiguous COMMENTED review by time, and stop emitting it Review of #622 found the previous commit's fix incomplete, and it was right. `pr-review.yml:75` documents COMMENT as a legitimate FINAL verdict ("questions/observations only"), submitted with `gh pr review --comment`, which produces the same state == "COMMENTED" as the bot's inline-notes placeholder. Treating every COMMENTED as non-terminal therefore swallowed a real COMMENT verdict: the loop waited out the full timeout and reported "never submitted a summary" while a summary with findings sat on the PR. That over-generalised "the placeholder is COMMENTED" into "COMMENTED is always the placeholder". Fixed at the source and in the consumer. Source: pr-review.yml step 3 permitted `gh pr review` for inline notes, and that is what submits the extra review. It now requires `gh api .../pulls/N/comments`, so exactly ONE review is submitted per run -- the step 4 summary. No placeholder means no ambiguity. Consumer: the script no longer decides by state alone, and deliberately does NOT parse the placeholder's body -- that text is bot-generated prose with no contract behind it. APPROVED/CHANGES_REQUESTED return immediately; a COMMENTED-only state is held `grace` seconds (180, against observed placeholder-to-summary gaps of 16s on #622 and 50s on #617) to let a summary supersede it, and is returned as the verdict if none does. The grace window is what keeps this correct for reviews already on older PRs and if the bot regresses. Verified against #622's real review history: with both reviews visible the decisive branch returns CHANGES_REQUESTED and grace is never entered; in a window containing only the placeholder the COMMENTED branch finds it while the decisive branch is empty, so grace holds. The timeout message is also now correct rather than merely different: a COMMENTED-only run can no longer reach it, so reaching it means no review of any state was submitted -- a trigger fault, which is what #619 hit twice. SKILL.md's Step 11 said "It will never hand you COMMENTED"; a COMMENTED that now reaches the caller IS the verdict, so it is documented as carrying findings and not earning the ready flag, same as CHANGES_REQUESTED. * feat: Step 0 resolves a bare number to an issue OR a pull request This skill is used for TODO.md items and for refactors that never had an issue, so "issue number" was too narrow a contract. Step 0 already keys off observable state; a PR is simply another entry point to it, and the stronger one -- it carries the branch, the diff, the `## Scope assessment` and the review verdict, which is everything Step 0 reads. No flag is needed. GitHub numbers issues and PRs from ONE sequence per repository, so a bare number is unambiguous: try `gh pr view <n>`, fall back to `gh issue view <n>`. An earlier draft used `--pr <n>`; that distinction carries no information. Why it matters beyond tidiness: scripts/backlog-rhythm.sh hands unfinished drafts back to this skill, and for a PR with no linked issue it had nothing to hand -- it reported "no issue references this PR; finish it by hand". That left every self-directed PR with no owner in the loop, which is how #620, #622 and #623 all ended up driven by hand in one session. Where an issue IS linked, nothing changes: it is still read for the diagnosis. Where none is, Step 2's root cause comes from the maintainer's own framing rather than a Stage 2 comment, and Step 9 records it in the PR body as usual. * fix: decide a COMMENTED review by run state, and test the decision Third round on the same finding, and the previous two "fixes" were asserted rather than demonstrated -- verification was quality-check.sh plus `bash -n`, neither of which executes the decision path. So this commit changes the mechanism AND adds the missing tests. The grace window was the wrong instrument, and the review named the reason: it competed against the ORIGINAL deadline instead of extending it, so a COMMENTED first seen in the last `grace` seconds of the window could never satisfy the `elapsed >= grace` branch -- the loop exited first and reported "no review landed", which was false. Observed live on #622: the stub landed 12:15:14, the real CHANGES_REQUESTED 12:16:39, and the script returned the stub. Sizing it differently would not have helped. The gap that matters is not placeholder-to-summary (16s on #622, 50s on #617) but placeholder-to-END-OF-RUN: the bot posts an early permission-check comment within a couple of minutes and works for five to eight more. No constant is both short enough to return a real COMMENT promptly and long enough never to pre-empt a summary. So ask the run instead. `review_run_state` reads the PR Review workflow run started since the trigger: running -> a COMMENTED decides nothing; keep waiting finished -> a COMMENTED last word IS the verdict, per pr-review.yml's own three-verdict contract (APPROVE / REQUEST_CHANGES / COMMENT) failed -> report at once; do not burn the timeout on a dead run none -> the trigger never reached the workflow, a different fault That last one matters as much as the first. A dead run and a thinking one are both silence if you only poll for reviews, which is how #623's run -- already failed on "Reached maximum number of turns (60)" -- was waited on for 16 minutes. Also from this review round: - pr-review.yml no longer REQUIRES `gh api` for inline comments. The reviewer reported that `gh api` is permission-gated and unavailable to it, and that it probed with `gh pr review` to find out -- which submits, and is where the stray "test permission check" reviews came from. The prompt now states the one hard rule (submit exactly ONE review, never probe with it), prefers `gh api` for inline notes, and says to fold findings into the summary with file:line when it is unavailable, rather than falling back to a second review. - SKILL.md Step 0 keyed a resume signal on `## Scope assessment`, which only CI mode writes into the PR body. An interactive-mode PR never carries it, so the row would not match for most PRs this skill opens. It now keys on the PR existing, which is what actually proves Step 9 was reached. 7 new tests, REVIEW_POLL_INTERVAL added as their seam. Two of them are the discriminating pair: identical reviews (COMMENTED only), opposite outcomes, differing only in run state -- so the decision is provably driven by the new signal and not by timing. The shim applies `--jq` like real gh does; an earlier version echoed raw JSON and the script reported it as a verdict. * fix: give the verdict tests their own env file so they pass in CI The new test file could not pass in CI, and the review caught it with the failing run: `Fast tests` was red on this PR while local `quality-check.sh` was green. Reproduced both sides before fixing. Cause: request-pr-review.sh posts its trigger through scripts/gh-agent.sh, which reads a real BESS_AGENT_TOKEN and exits 1 before `gh` is reached. Shimming `gh` on PATH does not help -- gh-agent.sh is invoked by a repo-relative path, not looked up on PATH. Worse, it resolves its env file from the MAIN checkout (`dirname $(git rev-parse --git-common-dir)`), so this worktree having no `.env` of its own was irrelevant: the developer's real token was read anyway. CI provisions no `.env` and no such secret, so the suite failed unconditionally there. The abandoned `(d / "scripts").mkdir(...)` in the fixture was an attempt at this that did nothing. Fixed with the seam gh-agent.sh already documents for exactly this (`BESS_ENV_FILE`, "a seam tests use to point at a fixture .env instead"), so no production code changes: each test now supplies its own env file carrying a dummy token. Also adds a test that PINS that dependency, because the fix is otherwise invisible and could be dropped again silently: point the seam at an empty file and the script must fail before polling, naming the missing token. Worth recording how I nearly mis-verified this: my first attempt set BESS_ENV_FILE from OUTSIDE pytest and saw the tests still pass, which looked like proof of CI-safety. It was not -- the test sets that variable in the subprocess env, so it overrides any outer value and the experiment could not fail. The in-test pin above is the version that can actually discriminate. Full backend suite: 507 passed. * fix: an unreadable run state must not promote a placeholder to a verdict Review of #622 found a real correctness bug, reproduced against the shipped script: `review_run_state` falls back to `unknown` whenever `gh run list` itself fails -- network blip, rate limit, transient auth error -- and the COMMENTED branch tested only `state = running`, so `unknown` fell through to the else and reported the bot's placeholder as the verdict. That re-opens the exact race this script exists to close, gated on API flakiness instead of timing. "I could not tell whether the reviewer is still working" must never mean "it finished". It now waits, same as `running`: waiting costs one more poll, and a genuine COMMENT verdict still returns as soon as the state resolves. A test drives it, with the shim making `run list` exit 1 rather than returning a run shape -- which is what the failure actually looks like. That test also exposed a second defect in the same path: the timeout branch ends with `gh run list ... >&2` for diagnostics, and under `set -e` a failing `gh` -- precisely the `unknown` case -- aborted the script with exit 1 instead of the exit 2 that means "no verdict". Diagnostics must not decide the exit code, so it is `|| true` now. Also fixes the doc/implementation mismatch the review flagged: SKILL.md still described the 180s hold from the superseded commit, which would actively mislead the next reader about how Step 11 decides. It now describes the run-state mechanism, including that an unreadable state waits. 9 tests. Worth noting the `pr-review.yml` change from this PR is already working: this round the bot folded its findings into a single summary review and posted no stray placeholder.
…e approvals as merge-ready Two defects, both in the same direction: the surface told the maintainer to act on work that was already handled. REVIEW STATE (addresses the Stage 4 review on this PR, which reproduced it). The previous commit asked "is there an APPROVED review anywhere" before consulting `reviewDecision`. GitHub never rewrites an old review when a later round requests changes, so an approved-then-reworked PR keeps its stale APPROVED entry forever and was reported "nothing left but your merge" — the exact failure the commit set out to close, reintroduced by the fix for it. The review proposed keying on `reviewDecision` instead. That alone is also wrong here, and measurably: `reviewDecision` is only populated when the repo REQUIRES reviews, and this one does not. It reads CHANGES_REQUESTED for #619/#620/#614 but "" for #490, which carries two genuine APPROVED reviews — so keying on it alone reports an approved PR as never reviewed. Neither signal is sufficient, and each fails toward "merge it", so the order is the whole content of the rule: trust `reviewDecision` when set, otherwise fall back to the LAST non-COMMENTED review. Last, not any — same staleness trap. SESSION LIVENESS. `resume_implementation` keyed off `session == null`, and `session` comes from `claude agents`, which lists BACKGROUND agents only. A session started in the terminal — `claude`, then `/implement-issue <n>` — is a foreground session and never appears; even a background agent carries a generated descriptive name rather than the `issue-<n>` the dispatch convention promises. Measured: 41 worktrees on disk, `claude agents --json` returning one entry. So every worktree read as abandoned, and #624 was reported "no live session, /implement-issue 624 to resume" while actively being worked — routing a second session onto a branch the advice itself calls the only copy. The worktree LOCK is what tracks a live session: git records `locked claude session <name> (pid N start ...)`, and 4 of those 41 were locked — exactly the four live sessions, foreground and background alike. Live effect: #626 moves request_review -> rework_review (the review landed), #490 stays awaiting_maintainer despite its empty reviewDecision, and #624 is no longer reported as stalled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR
* fix: make a backlog pass runnable without a shell preamble `/loop /backlog` died on its first line, every tick. `backlog-digest.sh` required PROJECT_NUMBER in the environment, but it lives in the gitignored `.env` that nothing exports — so the script exited "the backlog board has not been created yet", which is the most misleading message it could emit: the board is Project #1, populated, and entirely fine. The documented workaround (`set -a; . ./.env; set +a; ...`) cannot survive an unattended pass, because the caller is a skill and not a shell someone typed into. The digest now sources `.env` itself, located via `git rev-parse --path-format=absolute --git-common-dir`. That is one rule rather than a search over candidate paths: `.env` is gitignored, so it exists only in the main checkout and never in a worktree, and the common dir resolves to the main checkout from either. `--path-format=absolute` is load-bearing — git otherwise answers the relative `.git`, whose dirname is `.`, silently resolving against the caller's cwd. The environment still wins over the file, or an explicit `PROJECT_NUMBER=2` would be un-overridable. Two gaps the skill could not close without a second API call by hand: - `board_status` — where the card sits NOW, alongside `column` (where the evidence says it belongs), in the same six Status strings. The `board` verb says "reconcile every card against the derived column, the digest always wins" and had nothing to reconcile against. Seven live mismatches were invisible. - `issue_no_card` — an open issue with no card at all. `Ready for Dev` requires a Priority and priority is a board field, so an off-board issue can never become dispatchable however well analysed, while reading as an ordinary Backlog item. #621 and #624 were both in that state. The rhythm pass told the PO to "set Priority" on those two, i.e. to set a field on a card that does not exist; it now asks for `add_card`, and reports `move_card` for a card the evidence disagrees with. Tests pin all of it. The git shim answers `rev-parse` so `.env` discovery is deterministic — letting the real git through would read the maintainer's own gitignored file, which CI does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR * fix: never report an unreviewed PR as ready to merge The rhythm surface resolved any non-draft PR to `awaiting_maintainer`, "nothing left but your merge". That rests on an assumption that does not hold: that only Step 11 clears the draft flag, and only after an APPROVED verdict. #626 broke it in the obvious way. It was flipped out of draft by hand because it LOOKED stuck, had zero reviews at the time, and the next pass duly reported it as ready to merge. A Stage 3 CI-mode PR has the same hole from the other direction — it never runs Step 11 at all. Stage 4 is the gate the entire pipeline is built around, so a surface that routes around it is worse than no surface. An APPROVED review is now the bar, matching Step 11 exactly. Not `reviewDecision`, which reads "" both for never-reviewed and for approved-then-dismissed; and not COMMENTED, which the bot also posts as a placeholder before its real verdict. Two new actions carry the cases that used to be silently collapsed into "merge it": `request_review` (Stage 4 never ran) and `rework_review` (out of draft with changes requested). Both fields were already fetched by `gh pr list` and simply unused. Live effect on the current fleet: #490 stays `awaiting_maintainer` — it carries real APPROVED reviews — and #626 becomes `request_review`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR * fix: stop the rhythm surface reporting live work as stalled, and stale approvals as merge-ready Two defects, both in the same direction: the surface told the maintainer to act on work that was already handled. REVIEW STATE (addresses the Stage 4 review on this PR, which reproduced it). The previous commit asked "is there an APPROVED review anywhere" before consulting `reviewDecision`. GitHub never rewrites an old review when a later round requests changes, so an approved-then-reworked PR keeps its stale APPROVED entry forever and was reported "nothing left but your merge" — the exact failure the commit set out to close, reintroduced by the fix for it. The review proposed keying on `reviewDecision` instead. That alone is also wrong here, and measurably: `reviewDecision` is only populated when the repo REQUIRES reviews, and this one does not. It reads CHANGES_REQUESTED for #619/#620/#614 but "" for #490, which carries two genuine APPROVED reviews — so keying on it alone reports an approved PR as never reviewed. Neither signal is sufficient, and each fails toward "merge it", so the order is the whole content of the rule: trust `reviewDecision` when set, otherwise fall back to the LAST non-COMMENTED review. Last, not any — same staleness trap. SESSION LIVENESS. `resume_implementation` keyed off `session == null`, and `session` comes from `claude agents`, which lists BACKGROUND agents only. A session started in the terminal — `claude`, then `/implement-issue <n>` — is a foreground session and never appears; even a background agent carries a generated descriptive name rather than the `issue-<n>` the dispatch convention promises. Measured: 41 worktrees on disk, `claude agents --json` returning one entry. So every worktree read as abandoned, and #624 was reported "no live session, /implement-issue 624 to resume" while actively being worked — routing a second session onto a branch the advice itself calls the only copy. The worktree LOCK is what tracks a live session: git records `locked claude session <name> (pid N start ...)`, and 4 of those 41 were locked — exactly the four live sessions, foreground and background alike. Live effect: #626 moves request_review -> rework_review (the review landed), #490 stays awaiting_maintainer despite its empty reviewDecision, and #624 is no longer reported as stalled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iewer" The Stage 4 bot only ever acts when triggered by an `@claude-bot review` comment. So "green, no verdict yet" has two completely different meanings, and only one of them belongs to the reviewer: request NEWER than last push -> the bot is genuinely working: reviewer request OLDER, or absent -> nobody has asked: DISPATCHER Collapsing those parked six of eleven open PRs on someone who had not been asked and was never going to act. Measured on the live fleet: #637, #635 never requested at all #620 requested 17:40:53, pushed 18:33:05 #619 requested 10:50:40, pushed 21:53:00 #614 requested 06:55:38, pushed 07:08:20 #490 requested 08-15 13:41, pushed 08-16 15:30 Every one had been reported as `awaiting-review [reviewer]`. After this change the same fleet shows zero PRs waiting on the reviewer and zero waiting on the maintainer — the pipeline owes an action on all of them. That is the failure this whole branch is about, seen from the other side. The gate stops a confused loop asking too often; this surfaces the loop that stopped asking at all. Both are the same lost state — whether the last verdict has been consumed — and both are recoverable from the PR rather than from a session that died. `comments` joins the field set for this. It fits inside the GraphQL node budget at --limit 30, which is already bounded by `commits`. Verified by mutation: disabling the request-feed check reddens test_a_review_never_requested_is_the_dispatchers_turn_not_the_reviewers and test_a_push_after_the_last_request_owes_a_new_round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… branch Two halves of the same failure: work landing outside a worktree, and nobody noticing when it does. ## The hook CLAUDE.md has said "never edit any file on main, even a one-line doc fix" unconditionally for a long time, and it keeps being skipped. The reason is structural, not carelessness: it is prose, so it has to be REMEMBERED at the moment of the first edit — and that is exactly the moment a session which opened as a question has no reason to reconsider it. Six live sessions currently sit in the main checkout for perfectly good read-only reasons; nothing catches the one that quietly starts editing. check-worktree-path.sh already guarded CROSS-checkout edits and passed same-checkout ones, so main-to-main sailed through. It now also refuses any edit made from the main checkout, detected by --git-dir equalling --git-common-dir. That is a path comparison, the only shape docs/agents/rules.md sanctions here — it never guesses what a command will touch. Linked worktrees and sibling checkouts both differ, so both still work; the rule is "be in a worktree", not "be under .claude/". The denial names the remedy (EnterWorktree) and says what the main checkout still does — questions, gh, backlog, dispatch — because a block without a next move gets worked around. Residual gap, stated plainly: this governs Edit/Write/NotebookEdit. A Bash `sed -i` still writes. Guarding that would mean parsing command strings, which rules.md forbids for this hook and which has produced false positives here four times. ## The detector pr-state.sh gains a local-writer section. GitHub cannot see this: a branch with two writers looks normal through the API, because the divergence exists only between a local checkout and the remote and it collapses into an ordinary merge the moment someone reconciles. #619 is the worked example. One writer took the branch at 08:09 and worked from that base; another pushed 23031e7 at 09:34. The reviewer reviewed 23031e7 three times, twice with blocking findings, while the first line never held that commit. Fifteen hours later it landed as `Merge remote-tracking branch 'origin/fix/...' into fix/...` — a branch merged into itself, which is the fingerprint. `git rev-list --left-right` would have caught it at 09:34. Run against the live fleet it also surfaces the precursor state: #437 is 5 commits behind its own remote and #614 is 3, so a commit in either worktree diverges immediately. Skipped LOUDLY outside a checkout, since this script is also meant for a container fleet where a silent skip would read as "no divergence found". Verified by mutation: disabling the main-checkout guard reddens 2 of the 6 hook tests. The divergence detector is tested against a real two-clone scenario, not fixtures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main moved the local-environment section out of CLAUDE.md (#651), so the gh scopeless-token note lands in docs/agents/local-agent-environment.md instead. The backlog/board half of this branch is removed here, not lost: it is preserved at eb4d00e and moves to its own PR, per the review's primary blocker (undescribed, orthogonal changes making up over half the diff). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt
The changed-files mypy check degraded to a WARNING when `git merge-base origin/main HEAD` could not resolve. Warnings exit 0, so a run that type-checked nothing printed "Errors: 0" and reported success — the same anti-pattern the comment above the missing-tool branch already calls out. Covered by a test that drives the script with a git shim whose merge-base fails, asserting the error count rises by exactly one against an otherwise identical run. Verified it reddens when the severity is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt
…r/bess-manager into worktree-po-followups
|
@claude-bot review |
There was a problem hiding this comment.
Summary
No linked issue — the PR body frames this as three gaps self-discovered while landing #612 (mypy unenforced, the review bot going silent on clean runs, and gh's silent scope fallback). All three are narrowly scoped to their stated problem; no scope creep observed (the unrelated backlog/board bundle was already split out to #656 per the description).
Root cause / fix fit: Each of the three fixes matches the gap it describes — mypy is now gated on files changed vs origin/main (avoiding the 2914-error repo-wide blocker), the severity bug (WARNING→ERROR on unresolvable origin/main) is fixed, and pr-review.yml now makes a summary review mandatory every run.
Blocking: test-coverage gap in the new mypy-gate test
backend/tests/test_quality_check_mypy_gate.py:25-33 (GIT_RESOLVES) and :35-40 (GIT_CANNOT_RESOLVE):
case "$1 $2" in
"merge-base origin/main") echo deadbeef ;;
"diff --name-only") ;;
"ls-files --others") ;;
*) ;;
esac
Both shims match git diff --name-only ... and git ls-files --others ... but their case arms are empty — no echo of any filename. That means scripts/quality-check.sh's changed variable is always empty in both tests, regardless of the mod.py fixture file _run() creates on disk.
I reproduced this outside the PR's test harness by checking out pr-614's quality-check.sh and instrumenting the git/mypy shims directly: with the exact GIT_RESOLVES shim body, mypy is never invoked, and the script prints ✅ mypy OK (no changed Python files) — not ✅ mypy OK (changed files) — even on the "resolvable" run with mod.py present.
So test_resolvable_origin_main_runs_mypy (:97-101), which is docstringed as "The control: with the ref resolvable the gate actually checks types," only asserts two log strings ("Checking mypy on changed files" in stdout and "Cannot resolve origin/main" not in stdout) that are true whether or not mypy ever runs against a file. Neither this test nor test_unresolvable_origin_main_fails_the_gate exercises the actual "$MYPY" --explicit-package-bases --ignore-missing-imports $changed invocation in scripts/quality-check.sh (the new block starting around line 99) — which is the primary new behavior this PR adds (enforcing mypy on changed files). A regression there (wrong flags, a quoting break, wrong path resolution, changed never populated) would pass this suite silently.
This is exactly the anti-pattern docs/agents/testing.md calls out ("a test has passed while proving less than claimed... a fixture that could not reach the branch it named") — on a PR whose whole point is closing gaps like this one.
Suggested fix: make GIT_RESOLVES's "ls-files --others" (or "diff --name-only") arm actually echo a filename, add a fixture .py file with a genuine type error, and assert the gate reports it as an error (with a companion well-typed fixture proving the pass case) — a true RED/GREEN pair for the "changed files, real type error" path, not just the two branches around it.
Other checklist items
- Scope assessment statement (rules.md Debugging Protocol step 9): not explicitly stated as local/structural in the PR description, but these are new-capability additions (a new gate, a new mandatory-review rule) rather than root-cause bug fixes in existing methods, so the local/structural framing doesn't cleanly apply here — not treating this as a blocker.
.github/workflows/pr-review.ymlandscripts/request-pr-review.shchanges are comment/prompt-only, low risk, and consistent with the existing "exactly one review" constraint (reinforces it rather than conflicting).- Minor nit:
scripts/request-pr-review.sh's new paragraph is inserted between the2and1entries of the "Exit codes:" list, splitting it awkwardly (0,2, [prose],1). Cosmetic only. - Architecture/error-handling rules (no
Optional[x], nohasattr/getattrfallbacks, no exception-string matching, no new classes) don't apply — this PR touches only shell, YAML, Markdown, and one new test file with no domain logic. - Could not independently verify Black/Ruff/mypy pass on the new test file (no toolchain available in this review environment); trusting the PR's stated local verification (
0 errors, 0 warnings).
Requesting changes on the test-coverage gap above — the fix logic itself looks sound, but the test suite doesn't actually prove it.
The Stage 4 review caught the test proving less than it claimed. Both git shims matched 'ls-files --others' with an empty arm, so the gate's file list was always empty and it took its 'no changed Python files' path — mypy was never invoked in either run. The control asserting 'the gate actually checks types' was two log-string assertions that held whether or not it did. Now the git shim names the changed file, mypy is the real one rather than an exit-0 shim, and the primary case is a genuine type error in that file. A third test pins that the well-typed run reached the checked-files path, which is the vacuity the delta assertion cannot see by itself: with an empty list both runs agree and the delta is simply zero. Also unsplit the exit-code list in request-pr-review.sh, which had prose between entries 2 and 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt
|
@claude-bot review |
There was a problem hiding this comment.
Summary
Verified all three fixes locally, not just by reading the diff.
1. mypy gate (scripts/quality-check.sh, backend/tests/test_quality_check_mypy_gate.py, requirements-dev.txt)
- Installed mypy/pytest in a fresh venv and ran
backend/tests/test_quality_check_mypy_gate.py— all 3 tests pass. - Reverted the severity fix (
ERRORS=$((ERRORS + 1))→WARNINGS=$((WARNINGS + 1))on line 126) and rerantest_unresolvable_origin_main_fails_the_gate— it goes RED (assert 2 == (2 + 1)), confirming the test genuinely proves the severity behavior, not just log text. This matches the "must be seen to fail without its fix" rule indocs/agents/rules.md. - Ran the gate's actual invocation against the real PR branch (
git merge-base origin/main HEAD, the three-source diff, dedup, mypy call): it correctly scoped to exactlybackend/tests/test_quality_check_mypy_gate.pyand passed cleanly — matches the PR's "0 errors, 0 warnings" claim. - The vacuity gap flagged in the prior review round (git shims never actually exercising mypy) is fixed in
ebd9349—mypyis now the real binary via passthrough, andILL_TYPED/WELL_TYPEDfixtures drive a genuine type error through it. Confirmed by reading that commit's diff directly.
2. Mandatory verdict (pr-review.yml, scripts/request-pr-review.sh)
- The new "mandatory in every case" clause in
pr-review.ymlis additive to, not in conflict with, the existing "exactly one review" constraint — together they read as "exactly one, and never zero." Correctly targets the root cause from the PR body: a clean run previously posted nothing, and GitHub blocks a merge on the last explicit verdict, so a staleREQUEST_CHANGEScould never self-clear. request-pr-review.sh's exit-code doc reorder and rationale addition are consistent with the behavior change — a timeout is a real signal again now that silence is disallowed.
3. gh scopeless-token fallback note (docs/agents/local-agent-environment.md)
- Correctly placed under the
network.allowMachLookupfinding it extends. ConfirmedCLAUDE.mdhas no leftover/duplicate copy of this content (consistent with #651 having already relocated the local-environment section out).
Scope
gh pr view --json filesshows exactly the 6 files the PR describes:pr-review.yml, the new test, the doc note,requirements-dev.txt,scripts/quality-check.sh,scripts/request-pr-review.sh. No backlog/board files present — matches the PR body's claim that content was split out to #656.- No architecture-rule violations: no
Optional[x], nohasattr/getattr-with-default, no new classes, no exception-string matching, no hardcoded entity IDs (none of this touches HA/sensor code). black/ruff/mypyall pass on the new test file (verified directly, not just assumed).
Root cause vs. linked issue: no linked issue — PR is self-reported gaps found while landing #612. All three fixes map directly to the three problems described, each with a plausible/verified causal chain (measured error counts, actual GitHub merge-block behavior, a real diagnosed Forbidden mismatch).
Regression coverage: yes — the mypy severity test is RED-verified above. The other two changes are prompt/doc-level and have no automated test, which is appropriate for their nature (a workflow prompt and a documentation note aren't unit-testable, and this PR doesn't claim otherwise).
No blockers. Approving.
The new gate test passed locally and failed on CI. The shim exec'd REPO_ROOT/.venv/bin/mypy, which CI does not have — so mypy failed identically in every run, both delta assertions collapsed to zero, and the control never saw 'mypy OK (changed files)'. Reaching mypy as '<the interpreter running pytest> -m mypy' works under both layouts, and importorskip makes a genuinely absent mypy a skip rather than an assertion against a gate that cannot run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt
The review was right that 'this cannot be tested' does not hold against this repo's own precedent — test_backlog_digest.py shims gh on PATH for exactly this shape of script, and #614 used the same technique. Three things are worth pinning and all are visible from the arguments the script passes: - Priority is created as P1,P2,P3,P4. It is a bare string literal that already drifted once (P0,P1,P2), and nothing else would catch it. - An existing board creates nothing at all. - A failed lookup does not become a second board. That last one is also a fix, not just a test. The lookup ended in '|| true', which makes 'no such board' and 'the lookup failed' indistinguishable — a rate limit or transient GraphQL error would have created a duplicate 'BESS Manager Backlog', contradicting the idempotence the header promises. It now stops instead. Each test verified to redden under the matching mutation: restoring the P0 tiers fails the first, restoring '|| true' fails the third. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt
|
@claude-bot review |
There was a problem hiding this comment.
Summary
This PR is well-scoped (the unrelated board/backlog bundle was correctly split into #656) and the three remaining changes are each individually reasonable: a mypy gate in scripts/quality-check.sh, a mandatory summary-review requirement in pr-review.yml, and a doc note about gh's scopeless-token fallback. The new test (backend/tests/test_quality_check_mypy_gate.py) is genuinely good — it runs the real script against real mypy, asserts on a delta rather than an absolute count, and the PR's own commit history shows it was mutation-tested (reverted the severity fix, watched it go red, restored it).
However, the PR's central claim — that mypy is now enforced where it previously wasn't, including CI — does not hold for the path that actually gates merges to main.
Blocking: the required CI check still never runs mypy
Part 1 of the PR description opens with: "docs/agents/rules.md says code must pass mypy with zero errors. Nothing ran it: not quality-check.sh, not CI, and it was not even in requirements-dev.txt."
This PR adds the gate to scripts/quality-check.sh (new code at scripts/quality-check.sh around the new mypy block) and wires requirements-dev.txt — but it does not touch .github/workflows/ci.yml. The quality job there (ci.yml, "Code quality gate: linting + formatting") is unchanged by this diff:
quality:
name: Code quality
...
- name: Install tools
run: pip install black ruff
- name: Black formatting
run: black --check . --exclude="/(build|\.venv|node_modules)/"
- name: Ruff linting
run: ruff check . --exclude="build,.venv,node_modules"No mypy step. Per docs/agents/workflow.md:84, this ci.yml job set is "the single required status check" that gates merges — not scripts/quality-check.sh, which is only invoked by local developers and by the Stage 3 issue-fix.yml agent flow (issue-fix.yml:45-47).
This is directly verifiable on this PR itself: gh pr checks 614 shows "Code quality pass 18s" — far too fast to have run mypy over this diff's own new Python file (backend/tests/test_quality_check_mypy_gate.py), confirming the required check still only runs Black and Ruff.
Practically: the exact scenario the PR opens with — a mypy violation reaching main via a normally-reviewed PR, caught only because "the Stage 4 review bot happens to run mypy independently" — is still possible after this merges. Stage 4's mypy check is an AI-judgment checklist item (.github/claude-bot.md → Code quality → "Python: Black, Ruff, mypy must pass"), not an automated, always-run gate; it can be missed. The one automated, always-run, merge-blocking check (ci.yml) still doesn't run mypy at all, for any PR not produced by the Stage 3 flow — which includes ordinary human-authored PRs and worktree-driven agent PRs like this one.
The PR explicitly measured why repo-wide mypy in CI isn't viable (2914 errors / 191 files), and built the changed-files-only scoping specifically to make it viable — but then didn't apply that same scoped logic to the one place that was named as broken in the problem statement. Either ci.yml's quality job needs a scoped-mypy step (reusing the same changed-vs-origin/main logic already built and tested here), or the PR description needs to explicitly acknowledge that CI enforcement is deliberately out of scope for this PR and explain why — per the Debugging Protocol's scope-assessment requirement (docs/agents/rules.md step 9), a structural-looking gap in coverage shouldn't be left unstated.
Checklist per the review process
- Matches root cause? Partially — fixes local/Stage-3 enforcement, not the general-PR CI enforcement the PR itself identifies as broken (see above).
- Would the test catch a regression? Yes, for the code path it covers (
scripts/quality-check.sh). It cannot catch a regression inci.yml, becauseci.ymlis untouched. - Minimal / no scope creep? Yes — good split of the unrelated board work into #656.
- Rule violations? None found in the diff itself (no
Optional[x], nohasattr/getattrfallbacks, no new classes, comments are WHY not WHAT, no secrets).
Non-blocking nit
scripts/quality-check.sh, the new mypy block: changed is built as an unquoted, space-joined string and passed unquoted to "$MYPY" --explicit-package-bases --ignore-missing-imports $changed. This relies on no changed .py path ever containing a space or glob character. Unlikely in this repo, but worth a printf '%s\0' / array-based rewrite if that's ever not true. Not blocking given the codebase's existing filenames.
Review round 3 found the PR only half-delivered its own opening claim. The gate went into quality-check.sh, which runs locally and in the Stage 3 flow — but ci.yml's Code quality job installs only black and ruff, and that job aggregates into the Merge gate, the single required status check on main (docs/agents/workflow.md). So the exact scenario this change exists to prevent, a mypy violation reaching main through an ordinary PR, was still open after it merged. The CI step reuses the same scoping rather than a second policy: changed against the merge-base with main, deletions dropped, repo-wide left alone (2914 errors across 191 files). The job checks out full history because a shallow clone has no merge-base to compute. Also takes the review's nit, which the CI step made worth doing: the file list is an array in both places now, so a path containing a space stays one argument instead of splitting into several names that do not exist. Covered by a test using a spaced path, verified to redden when the list goes back to being space-joined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt
…rd actually has (#656) * feat: add the board bootstrap script and the PO's board memory Split out of #614, whose review's primary blocker was that this board work was undescribed and orthogonal to that PR's mypy/review-bot fixes. Most of the original bundle is now redundant: main already carries the 'Ready for Dev'/'In Progress' column names, the awaiting-before-analyzed precedence fix, and the 'there is no P0' correction in the backlog skill. What was left unlanded is the bootstrap script and the two PO memory files. backlog-board-init.sh created the Priority field with P0,P1,P2 — a tier the live board does not have and no consumer reads. It now creates P1-P4, which is what the board carries, what the skill documents and what the digest ranks on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt * fix: keep the board memory to its durable lesson, not a state snapshot The Stage 4 review found this violates the backlog skill's own rule ('State lives on GitHub, nowhere else' — never write a local file that mirrors board or issue state). The per-issue triage list was exactly that, and was already stale two days after being written. What stays is the generalizable lesson: the digest derives a column for an issue whether or not a card exists, so board presence has to be checked against gh project item-list directly. The name and index line now say that rather than framing the file as a snapshot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt * test: pin the board bootstrap's tiers, idempotence and failed lookup The review was right that 'this cannot be tested' does not hold against this repo's own precedent — test_backlog_digest.py shims gh on PATH for exactly this shape of script, and #614 used the same technique. Three things are worth pinning and all are visible from the arguments the script passes: - Priority is created as P1,P2,P3,P4. It is a bare string literal that already drifted once (P0,P1,P2), and nothing else would catch it. - An existing board creates nothing at all. - A failed lookup does not become a second board. That last one is also a fix, not just a test. The lookup ended in '|| true', which makes 'no such board' and 'the lookup failed' indistinguishable — a rate limit or transient GraphQL error would have created a duplicate 'BESS Manager Backlog', contradicting the idempotence the header promises. It now stops instead. Each test verified to redden under the matching mutation: restoring the P0 tiers fails the first, restoring '|| true' fails the third. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vHvj9qtD1jDYVnm3LjTnt --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@claude-bot review |
There was a problem hiding this comment.
Summary
Root cause fit: No linked issue — self-reported gaps found while landing #612 (mypy unenforced anywhere, the review bot going silent on clean runs, gh's silent scopeless-token fallback). All three fixes map directly to the described gap.
This round specifically closes the prior blocker. The last CHANGES_REQUESTED review found that scripts/quality-check.sh gained a mypy gate but .github/workflows/ci.yml's quality job — the job that feeds ci-gate ("Merge gate"), the single required status check (docs/agents/workflow.md:83-84) — still only ran pip install black ruff. That gap is now fixed: .github/workflows/ci.yml adds fetch-depth: 0 to checkout, installs mypy, and adds a mypy on changed files step that fetches origin/main, computes git merge-base FETCH_HEAD HEAD, and runs mypy --explicit-package-bases --ignore-missing-imports over the resulting file list.
I verified this isn't just diff-reading — pulled the actual run log for this PR's own Code quality job (run 32215786243, job 95956939894):
Checking 1 file(s):
backend/tests/test_quality_check_mypy_gate.py
Success: no issues found in 1 source file
So the step genuinely executes and scopes correctly, not a step that trivially passes because the file list came back empty (the exact vacuity failure mode round 2's review caught in the test itself). quality is listed in ci-gate's needs: (ci.yml:529-536), confirming this job's result is load-bearing for the merge gate, matching the PR's claim.
Test coverage: backend/tests/test_quality_check_mypy_gate.py runs the real quality-check.sh against real mypy (not shimmed), asserts on a delta between well-typed/ill-typed fixture runs rather than an absolute count, and per the PR's commit history was mutation-tested (severity fix reverted → target test goes red, restored → green). This would catch a regression in the gate's actual behavior, not just its log text.
Minimal / scope: Yes. gh pr view --json files shows exactly the 7 files the description covers (ci.yml, pr-review.yml, the new test, local-agent-environment.md, requirements-dev.txt, quality-check.sh, request-pr-review.sh). The previously-flagged undescribed board/backlog bundle is confirmed absent — split to #656 as stated.
Scope-assessment statement (rules.md Debugging Protocol step 9): present and clear — the PR body states the changed-files-only scoping rationale (2914 repo-wide errors vs. 417 production-only) and explicitly frames this as closing a gap rather than a structural redesign.
Rule violations found: none. No Optional[x], hasattr/getattr-with-default, new classes, exception-string matching, or hardcoded entity IDs — none of this diff touches HA/sensor/application code. Black/Ruff/mypy all pass per the Code quality job's own log.
Minor, non-blocking: the mypy run emits a config note unrelated to this diff — pyproject.toml: note: unused section(s): module = ['apscheduler.*', 'loguru.*', 'tests.*', 'uvicorn.*'] — harmless (exit 0, doesn't affect the Errors: count) but worth a follow-up cleanup of pyproject.toml's mypy overrides section at some point.
No blockers. Approving.
…5 minutes (#667) PriceManager.check_health() delegated unconditionally to price_source.perform_health_check(), and every source implements that as a live fetch of today's prices — bypassing PriceManager's own cache. With refresh_health_check on a */5 cron, that is 288 live Nordpool service calls a day for data that changes once a day. The cost is not just the calls. A single transient HA 500 flipped the "Electricity Price Data" component ERROR, and the next check five minutes later flipped it back, which HealthRecoveryTracker turned into a "recovered from an earlier issue" banner. Users saw one most days, for a system that had today's prices cached throughout and never missed an optimization. Holding today's prices already answers the only question this check asks, so report OK from the cache and probe the source only when the cache is cold — startup, date rollover, or clear_cache() after a settings or provider change. A cold probe that fails is still ERROR: without prices the system genuinely cannot optimize. get_price_data()'s today branch now goes through the same _cached_today_prices() helper, so the two notions of "the cache is warm" cannot drift apart. Mypy annotations in price_manager.py are the changed-files gate from #614 pulling this file's legacy backlog into scope; they are annotations only, no behaviour change. Closes #662 Claude-Session: https://claude.ai/code/session_01Kf5wtkJiPQQ5tJnmfxQA3j Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: give the mypy gate the same environment locally and in CI
The Code quality job installed `black ruff mypy` and nothing else, so mypy
resolved `pytest` to Any there while the local `.venv` had the real package.
That divergence is the opposite of what the step promises ("a green local
gate and a green CI gate mean the same thing"), and it is not closeable by
annotating: with an untyped `pytest`, annotating a decorated test function
only converts `no-untyped-def` into `untyped-decorator`. Measured on
test_agent_permissions.py -- 6 errors before, 5 after. Install the dev
requirements instead, which is where pytest, black and ruff are already
pinned.
With the environments matched, annotate the functions the ratchet had no
baseline for. These files predate the gate (#614), so nothing charged them
until a release PR compared them against a stale mirror:
- test_agent_permissions.py and test_vpp_idle_at_reserve_floor.py are new
files, so every error in them counts; both are now clean.
- vpp_simulator.py's `_simulate` gains the callback type its docstring
already describes.
- the four functions #619 added to test_vpp_simulator_branches.py and the
two it added to test_solax_modbus_growatt_vpp.py get return types. The
pre-existing untyped functions in those two files are left alone -- the
ratchet does not charge them, and burning them down is separate work.
Narrowing `_inverter_controller` surfaced a real mismatch the `| None` error
had been masking: `current_schedule` is a `DPSchedule`, and the test assigns
a `SimpleNamespace` stub. Cast it, with a note that only `.actions` is read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gRPJGGJvK1ZH5DjpZnxSJ
* fix: install the app requirements too, or the gate still diverges
Installing only requirements-dev.txt closed the divergence for `pytest` and
left the identical one for everything in backend/requirements.txt. Every
other Python job in this workflow installs both files; this one now does too.
It cuts both ways, so neither half is optional:
- Missing `fastapi` makes `@router.get` untyped exactly as missing `pytest`
made `@pytest.fixture` untyped. backend/api.py reports 53 errors without
site-packages against 43 with, and the 10-error delta is entirely
`untyped-decorator` -- so a new annotated endpoint would pass locally and
fail here, unfixable by annotating.
- Missing `numpy` MASKS errors instead. core/bess/pwl_window_dp.py reports
7 errors with it installed and 3 without, so a genuine type error in a
numpy-using optimizer file would clear the merge gate and surface only on
the maintainer's machine.
Found in review of the first commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gRPJGGJvK1ZH5DjpZnxSJ
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: assert the gh api guard by behaviour, not by spelling `quality-check.sh` failed on a clean `origin/main`. Its permission gate demanded the literal strings `Bash(gh api)` and `Bash(gh api *)` in `permissions.ask`; #657 replaced that blanket with one rule per write flag so `gh api` reads would stop prompting. The property the gate cared about still held — a `gh api` write still could not reach `allow` — but the spelling it pinned was gone, so it reported main as broken. Pinning a spelling was the wrong assertion. Whether a write can reach `allow` is already asserted by command string in MUST_BE_GUARDED, and those pins survive a respelling. Drop the two literal entries from REQUIRED["ask"] and keep the behavioural ones. Relaxing it uncovered a live hole. `gh api --help` documents `-F, --field` and `-f, --raw-field`; only the short forms were enumerated, so gh api repos/o/r/releases --field tag_name=v1 resolved to `allow` — a GitHub write that never prompted. Four command strings now pin both long forms in both argument positions, and `settings.json` gains the matching ask rules. Also annotates the six test functions in test_agent_permissions.py, which #614's mypy-on-changed-files gate requires once the file is touched, and corrects two claims in local-agent-environment.md that this makes false: that every `gh api` asks, and that the gate requires the blanket spelling. No CHANGELOG entry: agent tooling, no user-visible effect on the add-on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Y74iknEwCAGYGMwDM4NRc * fix: pin attached-value gh api write spellings so they prompt (#665) cobra/pflag accepts `--flag=value`, `-fvalue` and `-f=value` as readily as `--flag value`, but the space-separated globs in settings.json end in ` --field ` (with a space) and could not reach them -- so a write like `gh api repos/o/r/releases --field=body=hi` resolved to `allow` and never prompted. Add the attached-value ask patterns in both argument positions, pin the same spellings in MUST_BE_GUARDED, and extend the permission test so a respelling can't reopen the leak. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Three gaps found while landing #612. None were caused by that PR — it just surfaced them.
Scope note: this PR previously also carried an undescribed backlog/board bundle. Per the review and the maintainer's direction on this thread, that half has been split out to #656; nothing was lost (its full state is preserved at
eb4d00e9). This PR is now exactly the three changes it describes, plus the test and severity fix the review asked for.1. mypy was required but enforced nowhere
docs/agents/rules.mdsays code must pass mypy with zero errors. Nothing ran it: notquality-check.sh, not CI, and it was not even inrequirements-dev.txt. A violation reachedmainin #612 and was only caught because the Stage 4 review bot happens to run mypy independently.Turning it on repo-wide is not viable. Measured:
core+backend+scriptsSo the gate runs mypy against files changed vs
origin/main. Code you touch must type-check; the legacy backlog burns down as files get edited. Untracked files are included deliberately — a new file is untracked until its first commit, and this gate runs before that commit.mypyadded torequirements-dev.txt.And on CI, which is the half that actually gates merges
Review round 3 found the first version of this PR only half-delivered its own opening claim. The gate went into
quality-check.sh— which runs locally and in the Stage 3 flow — whileci.yml'sCode qualityjob still installed onlyblack ruff. That job aggregates into the Merge gate, the single required status check onmain(docs/agents/workflow.md), so the exact scenario described above was still reachable through any ordinary PR.ci.ymlnow runs the same scoped check, reusing the same policy rather than inventing a second one: changed against the merge-base withmain, deletions dropped, repo-wide left alone. The job checks out full history because a shallow clone has no merge-base to compute.Verified from the run log rather than the check colour — "Code quality pass" was the reviewer's own evidence that a passing job proves nothing here:
The severity bug the review found — fixed
When
origin/maincould not be resolved, the check degraded to aWARNING, and warnings exit 0. That printedErrors: 0for a run that type-checked nothing — the exact anti-pattern the comment above the missing-tool branch already calls out. It is now anERROR, with the reasoning recorded next to it.And the test coverage the review found missing — added
backend/tests/test_quality_check_mypy_gate.pydrives the real script in a throwaway directory withgit,black,ruff,mypyandpytestas PATH shims (same approach astest_backlog_digest.py). Only thegitshim'smerge-basearm differs between the two runs.2. The review bot went silent when it found nothing
pr-review.ymltold the bot to end with a summary review but didn't make it mandatory, so a clean run posted nothing at all.That's worse than cosmetic: GitHub blocks a merge on the last explicit verdict. A prior
REQUEST_CHANGEStherefore survives every subsequent clean run, and no amount of re-reviewing can clear it — #612 had to be dismissed by hand to merge. Now mandatory in every case, with the reason stated in the prompt so it doesn't get "simplified" away later.request-pr-review.sh's header updated: a timeout is once again a real signal rather than the usual clean case.3.
ghfails open to a scopeless tokenWhen a keychain read fails,
ghdoesn't error — it silently falls back to a token with fewer scopes. The symptom isForbiddenon an operation you do have rights to, which reads as a permissions problem and isn't. Cost a wrong diagnosis during #612: the samegh apicall returnedForbiddenin-sandbox and succeeded outside it, same account.Recorded with how to tell the two apart (
gh auth status—✓ ... (keyring)vsFailed to log in ... (keyring)). Since main moved that whole section out ofCLAUDE.md(#651) while this PR sat, the note now lands indocs/agents/local-agent-environment.mdalongside the sandbox findings it belongs with.Test plan
./scripts/quality-check.shpasses locally: 0 errors, 0 warningsEvidence the test discriminates
Four mutations, each reddening its own test and only its own:
test_type_error_in_a_changed_file_fails_the_gateFAILEDtest_unresolvable_origin_main_fails_the_gateFAILEDtest_a_changed_path_with_a_space_is_still_checkedFAILEDRestored after each: tree clean, 4 passed.
The second one matters most, because it is the defect review round 2 caught. The first version of this test shimmed
mypytoexit 0and left the git shim's file-list arm empty, so the gate took its "no changed Python files" path and mypy was never invoked — the control asserting "the gate actually checks types" was two log-string assertions true either way. Now mypy is the real binary and the file list is non-empty, and that mutation reddens two tests instead of passing silently.One further correction worth recording: the rewritten test passed locally and failed CI, because it reached mypy through
.venv/bin/mypy, which CI has no such layout for. mypy then failed identically in every run, both deltas collapsed to zero, and the control never saw its string. It now goes through<the interpreter running pytest> -m mypy, which holds under both layouts.Outcome-level coverage
The outcome asserted is the gate's exit behaviour — the error count and a non-zero exit — not the text it prints. That is the whole behaviour at issue: warnings exit 0, so severity is the outcome here. No optimizer fixture or golden applies; this diff touches no DP, intent or control/rate path.
Documentation check
Grepped
docs/agents/bess-knowledge.mdanddocs/SOFTWARE_DESIGN.mdfor everything this diff touches (quality-check.sh, mypy,pr-review.yml, the review verdict flow): neither file mentions any of it — they document optimizer behaviour and system design.docs/agents/local-agent-environment.mdis the file that does cover this ground, and it is updated here.Local verification only — no app behaviour changes, so there is no mock-HA scenario to observe; the gate itself was run directly, which is the equivalent.
No
CHANGELOG.mdentry: all three changes are developer tooling with zero user-visible effect on the add-on.