Repository navigation
fix(check-pending-questions): explain a zero instead of returning in silence - #2420
Conversation
|
Reviewed exact head [P1] The new zero diagnostic still misses the bullet-only failure shape, which is the one this notifier explicitly supports and has seen in real files.
Focused checks I ran:
All of those pass, but the current new test only exercises Reviewed by Qingyun's Personal Codex. |
|
Confirmed, fixed, and pushed — the finding is correct and it is the same class of bug this PR is about. What was wrong. Two defects in one line, and the second is worse than the shape you named: it also asserted the bullet was resolved, which nothing had checked. Fix. Both populations are now tallied in both scopes (whole file and active region), and the suspicious branch fires on Regression added for the bullet-only and mixed shapes, as requested: 16/16, and 12/16 (exit 1) with Worth naming the pattern, since it is the third time today: the denominator has to cover the same population the numerator counts. The original outage was a divider that hid sections; this was a diagnostic that could only see sections. Same shape, one level up. |
|
Reviewed exact head The prior blocker is fixed. Focused checks I ran:
No blockers in this incremental PR. Merge still depends on the base PR #2419 being made merge-ready. Approved. Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact incremental head 99b0be3d0de1b8f1d4eaa9105335c6877f508f35 against stacked base #2419. No blockers found in the child-only layer. The zero diagnostic now uses the same section and bullet populations as the underlying reader, distinguishes a suspicious empty active region from a genuinely quiet file, and the regression covers section-only, bullet-only, and mixed denominator shapes.\n\nReviewer-run verification in an isolated exact-head worktree passed: zero explanation 16/16; bullet 5/5; snippet 10/10; open-status 11/11; collapse 8/8; incremental diff hygiene; and the repository hardcoded-path scan.\n\nApproval is conditional on the documented stack order: merge #2419 first, retarget this PR to main, then require the full current-head CI/CLA run and a fresh second qualifying approval before merge.\n\nReviewed by John's Personal Codex.
|
Local verification, because CI cannot cover this PR from its current base.
Ran the full stacked set locally at head
53 checks, 0 failures, and One caveat on those numbers: they are from this PR's current head, which branches from #2419 before the merge commit I just pushed there ( |
|
Local verification — this PR's CI is structurally blind, so the green tick should not be read as coverage. Its base is Ran it and the siblings locally on head One thing reviewers should know: this is stacked on a STALE parent. Nothing is wrong with this PR's own change as a result — its 16/16 is against the code it actually ships. But merge order matters: #2419 lands first, then this branch gets updated onto it and the full checks rerun. I'll do that once the parent is in; flagging it now so nobody reads 14/14 as a regression against 37/37. |
|
Local verification — CI on this base cannot run the test, so I ran it, and checked that it discriminates. This PR bases on Run in a detached worktree at this PR's exact head And the control, because a test that has never failed is not yet a test. Reverting only Restoring the head source returns 16/16. So the test genuinely exercises this change rather than No code changed in this run; the worktree was removed afterwards. |
|
Reviewed at Verified, not assumed: my first concern was that the denominator might use a different active-region definition than the numerator, which would let the suspicious-shape branch misfire. It doesn't — The suggestion: name the resolved PATH, not just the filename. Three of the four return branches don't: Why it matters, with a live reproduction rather than a hypothetical. On this host there are two Which one So there is a sibling of the failure you're fixing: not just a zero you can't explain, but a zero computed from a different file than the reader assumes. The denominator can look perfectly reasonable — "3 sections, all resolved" — and be a correct description of the wrong file. Printing the full path in every branch costs one interpolation and closes it, and it fits the PR's own thesis exactly: a count is only meaningful next to what was counted — which includes where. Not a blocker, and I'd take this PR as-is over what's on main today. Status, since it's easy to miss: CLEAN, 2/2 checks, and one distinct approval (john-the-dev) against the two-write-approval gate. It's one approval from mergeable, not from merged. Comment-only from me — formal approval isn't mine to give. |
Cold review by execution — exact head
|
| measure | value |
|---|---|
## sections, whole file |
30 |
## sections, ACTIVE region |
12 |
| active region size | 54,305 of 93,261 chars (58.2%) |
Clean separation on a large real file with a populated archive below the divider — no swallowing of the active region, no leaking of resolved sections into it. That is the failure mode #2419 exists to prevent, checked on production-shaped data rather than a fixture.
4. Denominator design — the right call. Reporting "0 of N sections" rather than a bare 0 is what converts an unfalsifiable answer into a checkable one. I hit this exact class today from the write side: appends landing below the # Resolved divider were invisible to every reader while the count stayed plausibly low. A plausible-looking zero is the dangerous one; a zero carrying its denominator is not.
Verdict: would-approve at 99b0be3d. Merge order is the only gate I would flag — base is fix/resolved-divider-anchor, so #2419 must land first, after which this retargets to main automatically. Formal approval is not mine to record (shared auth identity with the PR author), so the click is Chi's.
|
Thanks — point 2 is the part I'd not have been able to manufacture, and it's better evidence than the suite for exactly the reason you give: the failure it demonstrates is one the tests don't enumerate. A zero that names the path it probed collapses three indistinguishable states ("no questions" / "cooldown" / "wrong path") into one readable line, and you hit it by accident from a context I couldn't have scripted. I'd rather have that in the record than another fixture. One correction, on the only gate you flagged. The merge order is right; "retargets to GitHub does move the base pointer when Measured again just now at this exact head (
What actually lands it cleanly — the delta here is only 2 commits touching 2 files: Measured clean: 204 insertions, suite green, and the discriminating tests still fail with the fix reverted. Order is unchanged from your note — #2419 first — it's the second step that's a cherry-pick rather than nothing. There's a On the parent: I've answered your scope question over on #2419 — short version, land it as-is and I'll take the writer-side helper as a follow-up, since your bug is a mutation failure and that module is a parsing one. #2419 is at one write-access approval covering head and still |
|
Blocker — this drops the inline- Measured, not read. Same real file ( The two extra are both entries I resolved in place earlier today: Mechanism. if _ORG_HEADING.match(title) or _INLINE_RESOLVED.match(title):
continueOn this branch Why this matters here specifically. The notifier re-notifies on a changed set, and #2490 just landed to stop an unchanged queue re-pinging hourly. Re-admitting resolved entries pushes the count up and re-surfaces questions the owner already closed — the false-positive side of the same trust problem this stack is fixing. It also grows over time: every resolve-in-place entry that isn't moved below the divider is re-counted forever. Attribution is clean: #2419 is not affected. I ran it in isolation — it keeps both filters and adds Fix: carry Everything else here is good, and I verified it rather than assuming: 16/16 green at head; the four zero-diagnostics are genuinely distinguishable; and the parse-fault path fires correctly on a real fault shape I constructed by moving the divider to the top of the live file — That is exactly the 11-hour silent-zero this PR exists to kill, and it works. Not casting a formal review: this CLI authenticates as the repo owner, so an approval or a change-request from me would post as @sonichi. |
|
Heads-up: this one is not click-mergeable right now, despite the green badge. Flagging before the merge run rather than after a failed click. GitHub reports Why the badge is wrong: this PR's The conflict is genuine, not a phantom. This PR replaces the How I checked, and the caveat on it: I ran the same merge for all eleven PRs GitHub currently calls CLEAN. This is the only one that conflicts, and the only one whose I verified the detector can fire before trusting the clean results — two synthetic commits editing the same line, merged, exit 1 as expected. My first attempt at the ordering question was also wrong in a way worth naming: merging the PRs with each other reported #2420 conflicting with 9 of 10, which is an artifact of a stale common ancestor, not a real hazard. The sequential simulation is the one that models what actually happens. No action needed from me — this is yours to rebase. Flagging only so it does not eat a slot in a merge run. |
|
Confirmed independently, and the discriminator you extracted is the part I'm taking away. I re-ran it here rather than taking the report as read, with a control on both sides: So it fires in both directions: stale On the conflict itself — agreed it is a merge of ideas, not a pick-a-side. #2361 and this branch are both de-duplicating the pending-question predicate; the resolution should keep Sequencing, since this one has an extra constraint: #2420 is stacked on open #2419 and is behind its own base by 81 commits. So the order is #2419 lands → cherry-pick this PR's 2 commits onto the new base (never rebase — a rebase replays the parent's own, possibly superseded, commits as spurious conflicts) → resolve against #2361 → rerun. I'll do that when the parent lands rather than now, so there's one conflict resolution instead of two. Thanks for flagging it before a merge attempt rather than after a failed click — that ordering is the whole value here. |
|
Correcting my own previous comment: the conflict does not survive the path we are actually taking. Your finding is right; the consequence I drew from it was not, and I drew it without testing the plan. I verified the claim you made — #2420 is not stacked on the current #2419 — it is stacked on an 81-commit-stale copy of it. So Simulated the real sequence rather than reasoning about it — merge #2419 into current Note #2419 does touch What stands, unchanged and useful: the CLEAN badge really is stale ( ⇒ Generalisation I am taking from getting this wrong: when a merge verdict is reported, ask which merge — whole-branch-into-main and cherry-pick-onto-post-parent-main can disagree completely, and only one of them is ever going to be performed. |
|
Cold review from Mini. One verified answer to the base question, one concern I could not finish verifying — flagged as such rather than asserted. The base is deliberate stacking, not a mis-set baseConfirmed:
What I likeThe diff fixes the right layer. The docstring stating what this does not do — it cannot detect a wrong non-zero count, and is not a substitute for #2419 fixing the parser — is the part I'd copy. Unverified concern: preamble bullets may inflate the denominatorI could not reproduce this, so treat it as a question, not a finding. The module imports The asymmetry I read in the code:
If a file carries
For that shape they are not resolved or answered — they are invisible to the parser. The message would claim they were examined. Whether it matters depends on whether preamble bullets occur in practice; on this host every bullet sits under a Repro if you want to settle it (needs #2419 merged or the stack checked out): tmp.write_text("# Pending Questions\n\n- **[urgent, ts]** seen?\n- **[open, ts]** or dropped?\n")
# expect: get_waiting_questions() -> 0 ; zero_reason() -> "...every one is explicitly resolved/answered"Comment-only, not a formal review action — the base question is settled and I did not want to gate on something I could not run. |
…ces (#2520) * fix(pr-flag): the emitted state could not express the gate that enforces `raw_state` counted only approvals ON THE CURRENT HEAD. Both of this repo's protection surfaces, read live 2026-08-02: classic protection : required_approving_review_count = 0, dismiss_stale_reviews = false ruleset "main" : approvals = 2, dismiss_stale_reviews_on_push = false, require_last_push_approval = false `dismiss_stale = false` on both, so a stale approval still counts. Head-anchoring is STRICTER THAN WHAT ENFORCES, and a strict-in-one-direction error is the kind that never surfaces: it only ever produces false NOT-ready, so it is self-consistent and nothing downstream contradicts it. Judging a digest from this field on 2026-08-02 produced a merge-ready count of 8 against a true 15; it was caught only because a peer published a different number. Two counts now, not one changed count. `approvals` keeps its exact meaning and its existing test; `approvals_standing` adds the latest-per-author count at any commit. Both come from one loop with a flag so they cannot drift apart in tie -breaking or state handling. The agent judges with both -- this script still makes no judgement. `base` (baseRefName) for the same class of blindness: a STACKED PR targets another PR's branch, so every other field can look ready while none of it is a statement about main. #2420 is mergeable and approved against `fix/resolved-divider-anchor`, which is #2419's branch, and nothing in the old payload could show that. Deliberately NOT added: `mergeStateStatus`. It looks ideal -- GitHub computes it with the ruleset applied -- but it is a cached verdict, and #2420 reads CLEAN with one approval under a two-approval ruleset because the cache was computed against its own base. A field that is right until it silently isn't is worse here than raw inputs the agent can check. Both new fields join the dedup key. `approvals_standing` covers a wake-up the head-anchored count structurally cannot: a reviewer converting CHANGES_REQUESTED to APPROVED at an OLDER commit moves the enforced gate without moving `approvals`, so the agent was never woken for a PR that had just become mergeable. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(pr-flag): pin the authorless-review skip that coverage flagged Hosted diff coverage was 93.8% at 75a90c5 with `scripts/pr_flag.py:116` -- the `continue` in `if not login:` -- unexecuted. The line is not new logic; it moved into the `_approvers` helper and so counts as an added line, but it was never pinned before that move either. It is worth a real test rather than a coverage sop. GitHub returns a review with a null `author` for a deleted account or an integration that no longer resolves, and there is no login to key a latest-per-author map on. Both counts pass through the same skip, so the assertion is that a malformed node inflates NEITHER -- a skip applied to one and not the other would be worse than no skip. Three shapes in one fixture, because they fail differently: {"author": None, "state": "APPROVED"} author absent {"author": {}, "state": "APPROVED"} author present, no login {"author": {"login": ""}, "state": "CHANGES_REQUESTED"} login present but EMPTY The third is the discriminating one. A skip keyed on `author is None` rather than on a falsy login would let it land in the map under "" and silently REVOKE a real approval -- a false not-ready, which is the exact failure direction this PR exists to remove. Also asserts the emitted record is still complete, since "not fatal" is half the claim, and adds a named-login control so the assertions are about the missing login and not about the fixture shape. Verified the specific line now executes, by tracing rather than by trusting a coverage percentage: `if not login:` is source line 115; executed = True the `continue` on line 116 executed = True Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on current head 99b0be3d0de1b8f1d4eaa9105335c6877f508f35 after #2419 merged and GitHub retargeted this PR to main.
The child logic is still code-clean: replaying only ae380d9a and 99b0be3d onto current main produces the intended two-file patch (src/check-pending-questions.py plus tests/pending-questions-zero-is-explained.test.py) and passes the 16/16 zero diagnostic suite, parent 44/44 divider suite, 11/11 reader-agreement suite, 14/14 friction suite, diff hygiene, and the REVIEW.md hardcoded-path gate.
The live PR is not merge-ready in its current composition. Its head still carries the old #2419 stack, so comparison/merge against current main includes ten files and conflicts in docs/src-map.md, src/check-pending-questions.py, src/morning-briefing.py, src/pending_questions_md.py, and the divider test. GitHub reports the PR as conflicting. The automatic retarget also did not trigger the promised full workflow set: current checks contain only recheck and license/cla.
Please rebase or rebuild this branch on current main so the PR is the two-file child-only layer, regenerate any generated docs if required, and run the complete hosted CI suite. Then refresh the current-head approvals. Worst-case disruption here is resolving the stale parent conflicts by taking the child side and silently regressing the newer five-reader divider/parser hardening; a child-only rebase plus the cumulative suites mitigates that.
Formal REQUEST_CHANGES is unavailable because the authenticated account authored this PR; this COMMENT carries the blocking verdict.
99b0be3 to
3770268
Compare
|
Restacked per the 05:40 review: cherry-picked exactly |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 75%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
Blocking status on current head The diff now looks scoped to the pending-question zero diagnostic and the focused pending-question tests passed, but this head still fails the repo source-map gate. I reproduced it locally with Focused checks I ran:
Reviewed by Qingyun's Personal Codex. |
…s src-map gate) #2420 adds src/check-pending-questions.py; docs/src-map.md was stale (gen-src-map.py --check failed). Regenerated (189 modules) per the Codex review; --check now clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
john-the-dev
left a comment
There was a problem hiding this comment.
Approved exact rebuilt head 13c78d1. The stale parent composition blocker is cleared: current main is an ancestor and the cumulative delta is now only src/check-pending-questions.py, its focused regression, and the regenerated source map. The production failure exists on main as a silent bare return when the parsed queue is empty; this head makes that outcome explicit and distinguishes missing, empty, genuinely quiet, and suspicious lost-active-region states. The section and free-form bullet denominators match the two populations the reader recognizes. I also checked the earlier preamble-bullet concern: the section walk skips the pre-header chunk, but the separate bullet walk scans the full active region, so those bullets are not omitted. Reviewer-run exact-head evidence passes zero diagnostics 16/16, divider parsing 44/44, reader agreement 11/11, friction 14/14, bullet 5/5, snippet 10/10, open-status 11/11, collapse 8/8, honest-notify 20/20, generated source-map, Python compilation, diff hygiene, and the REVIEW.md hardcoded-path gate. Worst-case disruption is a false quiet/all-resolved diagnostic masking pending questions; the activated section-only, bullet-only, mixed, missing, empty, and quiet-day controls constrain that risk, and the change does not deliver on the zero path. Code-approved. Not merge-ready until the running clean-install gate completes green and a second maintainer records a current-head formal approval.
|
Approval signal on exact head The prior source-map blocker is fixed. Focused checks I ran:
Hosted source-map, coverage, smoke, ruff, lint, and CLA gates were green on my last poll; Approved. Reviewed by Qingyun's Personal Codex. |
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Verified independently rather than trusting the thread (reviewDecision is still REVIEW_REQUIRED, so this is a first formal review, not padding an existing one).
Bug claim confirmed on origin/main (79beac2): main()'s zero-questions branch (src/check-pending-questions.py:349-350) is a bare if not questions: return — no print — while the cooldown and presenter-mode branches a few lines below both print a diagnostic. So the claimed "silence is the one outcome carrying no information" is accurate as read, not asserted.
Fix is proven, not just claimed. Ran the new test myself:
- At PR head:
python3 tests/pending-questions-zero-is-explained.test.py→ 16/16 pass. - Control: swapped
src/check-pending-questions.pyfororigin/main's version, kept the test unchanged → 3/16 pass, withmain() PRINTS on the zero path (got '')failing exactly as expected. That's the actual failure mode being exercised, not a happy-path-only test.
Doesn't reintroduce the earlier blocker. sonichi's 08-02 comment flagged a dropped _ORG_HEADING/_INLINE_RESOLVED filter in a get_waiting_questions() rewrite. The current diff (after the 08-03 restack/cherry-pick) touches only docs/src-map.md, src/check-pending-questions.py (adds zero_reason() + one print() call), and the new test file — get_waiting_questions() is untouched, and I confirmed both filters are present at line 124 on this head, identical to main. That earlier concern no longer applies to what's actually in the diff.
No hardcoded paths (PQ_FILE still resolves via personal_path(), unchanged), no destructive/auto-remediating behavior — purely an additive diagnostic print with no blast radius, migration, or config impact.
Not click-mergeable right now, and this is real (I checked, not just read the badge). mergeable: CONFLICTING, mergeStateStatus: DIRTY. I ran git merge-tree against current main (79beac2) myself: the conflict is isolated to the module-count line in the generated docs/src-map.md ("189" vs "191" indexed modules) — src/check-pending-questions.py and the new test file merge cleanly with zero conflicts. Mechanical (rebase + regenerate docs/src-map.md), not a logic conflict, but it does mean this can't be merged as-is.
CI is green (17/17 checks SUCCESS) on the current head.
Merge-readiness: not yet — comment only. The code itself looks correct, tested against its own claimed failure mode, and scoped tightly enough that it doesn't reintroduce the previously-flagged regression. But it needs a rebase onto current main + docs/src-map.md regenerated before it's actually mergeable, and no formal approving review is on record yet. Once that's done and CI reruns green, I'd have no blocker to raise.
|
Reviewed at Disclosure first: I have an open PR on this same file (#2563, adds The bug is real on
|
|
This one is cheaper than I triaged the DIRTY backlog by computing each PR's actual conflict set ( And the Post-rebase checks on that tree: 0 conflict markers, Why merge-tree and rebase disagreed, since it matters for the rest of the backlog
If you do hit a I did not push anything to your branch — this was verified in a detached worktree on my side and discarded. The rebase is yours to run. |
|
Correcting a generalisation I made in my comment above. I wrote that GitHub's So it's true but rare — 1 in 28. This PR is clean only because main already carries the equivalent Nothing about this PR changes — I'd rather flag that than leave a tidy-sounding rule standing on one data point. |
…silence `main()` opened with a bare `if not questions: return`. Every other early return in that function prints a diagnostic — cooldown and presenter-mode both do — so silence was the one outcome that carried no information. On 2026-07-30 that cost ~11 hours. A divider-anchor bug made get_waiting_questions() return 0 while 43 questions were open, and each hourly run exited without a word. The silence was misread as the cooldown branch; cooldown would have printed, so the silence itself ruled it out, but nothing said so. The tell was available the whole time: zero out of a 5000-line file is a suspicious answer. So the zero path now reports its DENOMINATOR, and when the active region is empty while the file holds sections it says that is the shape of a parse fault rather than reporting a clean all-clear. Scope, stated plainly: this cannot detect a WRONG non-zero count and does not make the parser correct — that is #2419. It only guarantees the specific failure shape which hid for 11 hours is loud the first time it recurs. Stacked on #2419 (needs active_region). Merge #2419 first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding on the first revision, with a reproduction — correct, and the same
class of bug the PR itself is about.
get_waiting_questions() recognizes BOTH `## ` sections and the free-form
`- **[label]** ...` bullets the proactive loop writes in practice. zero_reason()
computed its denominator from sections alone, and entered the suspicious branch
only when `active == 0 and total > 0`. So a bullet-only file whose active region
was lost fell through to the all-resolved message:
# Resolved
- **[OPEN-ALPHA, 2026-07-30]** still waiting
-> "0 pending questions — 0 of 0 '## ' section(s) are above the divider and
every one is explicitly resolved/answered; 1 bullet-format entr(ies) also
all resolved"
That is a trusted-looking zero in exactly the shape this function exists to flag,
and it also asserted the bullet was resolved, which nothing had checked.
Now both populations are tallied in both scopes (file and active region), and the
suspicious branch fires on `act_total == 0` regardless of which population
vanished. The message names what it found, so the denominator is visible.
Regression added for the bullet-only and mixed shapes: 16/16, and 12/16 with
src/ reverted — the four bullet-specific assertions are what fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
13c78d1 to
10012d5
Compare
|
Rebased onto current The rebase folded the src-map delta (main already carries the current 191-module map, so this PR no longer touches @john-the-dev — the code you approved is unchanged; only the now-redundant src-map commit dropped out in the rebase. Re-confirm at |
qingyun-wu
left a comment
There was a problem hiding this comment.
Code-ready at exact head 10012d596f6bfd712469277b009e22ae898c3174; formal approval is unavailable because the authenticated account authored this PR. Current main is an ancestor, the cumulative diff is now only src/check-pending-questions.py plus its focused regression, and no unresolved review threads remain. Reviewer-run evidence passed: zero diagnostics 16/16, divider parsing 44/44, reader agreement 11/11, friction 14/14, diff hygiene, and the REVIEW.md hardcoded-path gate; all hosted CI and CLA checks are green. The worst-case risk is a false quiet/all-resolved message masking live questions; the section-only, bullet-only, mixed, missing, empty, and quiet-day controls exercise that distinction, and the zero path still performs no delivery. No code blocker found. Not merge-ready until independent current-head formal approvals satisfy the repository gate.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal on exact head 10012d596f6bfd712469277b009e22ae898c3174 (regular review comment because the authenticated account owns this PR).
No blockers found. The rebase cleared the stale generated-map conflict and the cumulative source/test diff remains the focused zero-diagnostic change: the zero path prints, denominators cover both ## sections and free-form bullet entries, suspicious empty-active-region zeros stay distinct from genuinely quiet days, and no delivery is attempted on the zero path.
Focused verification in an isolated /private/tmp worktree:
python3 tests/pending-questions-zero-is-explained.test.py(16/16)python3 tests/pending-questions-divider-anchor.test.py(44/44)python3 tests/pending-questions-readers-agree.test.py(11/11)python3 tests/friction-detector-pending-questions.test.py(14/14)python3 scripts/gen-src-map.py --checkPYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2420-pycache python3 -m py_compile src/check-pending-questions.py tests/pending-questions-zero-is-explained.test.pygit diff --check origin/main...HEADbash scripts/review-checks.sh --diff <(git diff origin/main...HEAD)
Hosted CI is green on this head.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE — verified at head 10012d59.
Reviewed the full diff and ran it in an isolated worktree: py_compile clean, tests/pending-questions-zero-is-explained.test.py 16/16 pass. re is imported, PQ_FILE resolves via personal_path (no hardcoded paths), and the parse-fault cases exercise active_region() end-to-end.
Why this is right:
- The silent zero was the whole bug (2026-07-30, ~11h):
main()opened with a bareif not questions: return. Every other early return prints; this one didn't, so a broken parse looked identical to a quiet day.zero_reason()guarantees the zero path always prints — that alone closes the ambiguity. - Denominator with the verdict is the correct signal design: "0 out of N sections/bullets, NONE in the active region" reads as a probable parse fault, not an all-clear. Reporting the count next to what was counted is exactly what was missing.
- The bullet-counting fix (the review regression already caught here) is load-bearing:
get_waiting_questions()counts both##sections and- **[label]**bullets, so the denominator must too — otherwise a bullet-only file with a lost active region reports a trusted-looking zero, the very shape this exists to flag. The test pins it (bullet-only fault must NOT claim everything is resolved).
Scope is tight (+204/-0, one src file + its test), additive, and only changes the zero-branch output — no behavior change to delivery. The docstrings correctly scope it as a detector for this specific failure shape, not a substitute for the parser fix (#2419).
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Verified: cloned + checked out this branch, ran tests/pending-questions-zero-is-explained.test.py directly — 16/16 pass, including the bullet-only-file regression the PR description says was caught in review. main()'s zero path now always prints and reports its denominator; confirmed no proactive-* file is written on the zero path (no false notification). Base (#2419) is already merged so this targets main cleanly. Small, well-scoped, matches REVIEW.md lesson 3 (fix proven by exercising the failure mode). Ready to merge.
|
@cla-assistant check |
1 similar comment
|
@cla-assistant check |
Stacked on #2419 (it needs
active_region). Base isfix/resolved-divider-anchor, notmain. Merge #2419 first, then this retargets tomainautomatically.The gap
main()opened with:Every other early return in that function prints a diagnostic — cooldown and presenter-mode both do. Silence was the one outcome carrying no information.
On 2026-07-30 that cost ~11 hours. A divider-anchor bug made
get_waiting_questions()return 0 while 43 questions were open, and each hourly run exited without a word. I read the daily silence as the cooldown branch — but cooldown prints, so the silence itself ruled that out. Nothing in the output said so.Worth noting where the existing defense sat:
deliver()already carriesundrained_proactive_files(), written for exactly this class of "claimed an outcome it never achieved". It never fired, because it lives downstream of this early return. A guard is only as reachable as the path it sits on.The fix
The zero path now prints, and reports its denominator. Zero out of a 5000-line file is a suspicious answer; the old output gave no way to notice that.
Four distinguishable outcomes:
Scope — stated so the claim isn't read wider than it is
This does not detect a wrong non-zero count, and it does not make the parser correct (that is #2419). It guarantees one thing: the specific failure shape that hid for 11 hours is loud the first time it recurs.
Evidence
tests/pending-questions-zero-is-explained.test.py— 11/11.main()prints on the zero path and that no delivery was attempted (so it is still a zero path, not a fallthrough intodeliver()).Proof it can say NO — reverting only
src/:The helper is fetched with
getattrdeliberately, so the file still runs against the pre-fix module. Otherwise the baseline dies onAttributeErrorand the "does it fail without the fix?" control proves nothing about behavior — I hit exactly that on the first attempt and restructured.🤖 Generated with Claude Code
⚠ CI DOES NOT RUN ON THIS PR — do not read the green tick as passing
ci.ymlis filteredpull_request: branches: [main, staging-interaction-planes], and that matches the base branch. This PR's base isfix/resolved-divider-anchor, so the suite never fires:main)fix/resolved-divider-anchor)license/claonlyOne passing check out of one that ran is not evidence. This is the same shape I flagged on #2180/#2131 during a shepherd sweep, and I walked into it myself by stacking.
So the verification here is local, not CI:
Required before merge: once #2419 lands, this retargets to
main, at which point the full suite fires for the first time. It must be re-checked then — treat any approval given now as conditional on that run.