Skip to content

fix(check-pending-questions): a fully resolved file is not a parse fault - #3569

Closed
TustinOC wants to merge 4 commits into
sonichi:mainfrom
TustinOC:fix/zero-reason-resolved-file
Closed

TustinOC wants to merge 4 commits into
sonichi:mainfrom
TustinOC:fix/zero-reason-resolved-file

Conversation

@TustinOC

Copy link
Copy Markdown
Contributor

What

zero_reason() explains a zero so a parse fault cannot pass as a quiet day. It treats an empty active region as evidence of a mis-anchored divider — but that region is also empty when every question has been answered and archived. The success state was unreachable in the message logic.

Reproduction on origin/main (f34b5a5)

A file whose entries all carry this repo's own documented [RESOLVED] marker:

f.write_text("# Pending Questions\n\n# Resolved\n\n"
             "## [RESOLVED] should the digest run daily?\nYes.\n\n"
             "## [RESOLVED] archive the old notes?\nDone.\n")
waiting       : 0
zero_reason() : 0 pending questions, but pending-questions.md holds 2 '## ' section(s)
                and NONE are in the active region (above the archive divider). That is
                the shape of a parse fault, not a quiet day — check the '# Resolved'
                divider before trusting this zero.

Nothing is wrong with that file.

Why it matters

The guard runs on a schedule (*/30 here). On a host that actually reaches zero open questions it fires every run, forever, and it is the loudest message the checker emits. That is how a real detector gets tuned out — and this one exists because a silent zero once cost ~11 hours.

Fix

An empty active region has two causes and they are distinguishable: ask whether the archived entries are marked resolved.

  • all marked → This zero is affirmative.
  • any unmarked entry below the divider → still named a parse fault, and now it names the first offending entry, which is strictly more actionable than before.

The archive is judged by the same predicate the active region uses. Rather than restate it, I extracted the per-section decision into one owner (_section_is_waiting) that both callers use, so the two cannot drift about what an archived entry means. Behaviour above the divider is unchanged.

It counts bullets as well as sections. A section-only scan would call a bullet-only fault affirmative — the same blind spot this function was already fixed for once (the REVIEW REGRESSION block at line 86). I reintroduced exactly that bug on the first attempt and tests/pending-questions-zero-is-explained.test.py caught it:

FAIL bullet-only file with a lost active region is NAMED a parse fault

Evidence — new tests fail at the parent, pass at HEAD

At origin/main:

FAIL an all-[RESOLVED] archive is NOT called a parse fault
FAIL an all-[RESOLVED] archive is called affirmative
FAIL a **Status:** resolved archive is NOT called a parse fault
FAIL the fault names the offending entry so it is actionable
17/21 passed

At HEAD (40d80282):

21/21 passed

Every existing pending-questions / briefing / friction suite exits 0 (18 files, checked by exit code, not by grepping output).

review-checks.sh: PASS (hardcoded-paths + root-artifacts + prose-cap clean).

How I hit it

Not from reading the code — the checker told me. It reported this on a real host, I went to confirm the fault, and found 55 archived sections and an # Open region that was legitimately empty. The zero was real. Worth saying plainly: the guard did its job, and the only thing wrong was that it could not express the good outcome.

zero_reason() treats an empty active region as proof of a mis-anchored
divider. That region is also empty when every question has been answered and
archived, so the success state was unreachable in the message logic: a file
whose entries all carry the repo's own [RESOLVED] marker still reported "the
shape of a parse fault, not a quiet day".

The guard runs on a schedule, so on a host that reaches zero open questions it
becomes a permanent false alarm — which is how a real detector gets ignored.

An empty active region has two causes and they are distinguishable: ask
whether the archived entries are marked resolved. All marked means the zero is
affirmative; any unmarked entry below the divider is the 2026-07-30 shape and
still names the fault, now also naming the first offending entry so the reader
knows where to look.

The archive is judged by the same predicate the active region uses, extracted
to one owner rather than restated, and it counts bullets as well as sections —
a section-only scan would call a bullet-only fault affirmative, the same blind
spot this function was already fixed for once.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/check-pending-questions.py (100%)

Summary

  • Total: 13 lines
  • Missing: 0 lines
  • Coverage: 100%

@TustinOC

Copy link
Copy Markdown
Contributor Author

Follow-up evidence: verified against the real host file this was found on, not only the fixtures.

The production pending-questions.md here is 889 lines, 55 archived sections, active region legitimately empty. Same file, two readers:

# current main
0 pending questions, but pending-questions.md holds 55 '## ' section(s) and NONE are in
the active region (above the archive divider). That is the shape of a parse fault, not a
quiet day — check the '# Resolved' divider before trusting this zero.

# this PR
0 pending questions — the active region is empty and all 55 '## ' section(s) below the
archive divider are explicitly resolved. This zero is affirmative.

One thing that got there is worth naming, because it is the more useful half of this change.

When I first ran the new code against this host it did not say "affirmative" — it correctly reported that 37 of 55 archived entries carried no machine-readable resolution marker. That was true. Those entries close in prose (— CLOSED 2026-08-28 by Max: …), which reads fine to a human and is invisible to every parser here.

That is not cosmetic. Those 37 sit below the divider only because the divider is where it should be. The failure this function exists to catch — 2026-07-30 — is precisely a divider that moves. Had it moved on this host, 37 long-closed items would have surfaced as live questions and notified the owner, and nothing would have flagged it, because "no marker" and "still open" are the same state to the reader.

So the fix pays twice: the affirmative branch removes a permanent false alarm, and the fault branch names the entries whose resolution is not machine-readable — which is a real latent problem it can now report instead of hiding behind a generic verdict. I marked those 37 on this host and re-ran; that is what produced the "affirmative" line above.

Worth stating plainly for the reviewer: on a host whose archive is not marked up, this PR does not go quiet. It still says "parse fault" and now tells you which entry to look at first. Silencing that would have been the wrong fix.

@yixuan-ag2 yixuan-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 40d80282. Not approving and not requesting changes — this is a strict improvement over main and I would not hold it, but the headline claim is narrower than it reads, and I have a measurement that I think changes what you'd want the final shape to be.

What holds

_section_is_waiting() is a real single-owner extraction, not a cosmetic one, and it is equivalence-preserving: the old inline form skipped on _ORG_HEADING or _INLINE_RESOLVED, then skipped when a **Status:** line failed the unanswered|waiting|open prefix, and fell through to notify otherwise. The new predicate returns exactly that in all three arms.

The bullet arm in the archive scan is the part I would have missed. You caught your own blind spot in the comment — an archive walk over ## sections alone would call a bullet-only fault affirmative, which is the same denominator/numerator mismatch the file's existing test comment was written about.

And archived = text[len(active_text):] is sound, which is worth saying because it is the line that looks dangerous. I checked the producer rather than the name: active_region() in pending_questions_md.py:349 returns text[:m.start()] on a hit and text unchanged on a miss, so it is always a literal prefix and the slice is a correct offset. The no-divider case cannot reach this branch at all (active_text is text ⇒ act_total == file_total, and file_total == 0 returned earlier).

The finding: the affirmative branch does not fire on a real archive

Your test comment names the defect precisely — "the success state was unreachable in the message logic, so a host at zero got a permanent false alarm." After this change it is reachable in the message logic. On real data it still is not.

I ran your archive scan verbatim against this host's live pending-questions.md (315 KB, 110 KB below the divider):

ARCHIVED sections: 12   flagged "unmarked" by the PR's rule: 12
ARCHIVED bullets : 39   flagged "unmarked":                  32
=> affirmative branch reachable on this file?  NO

Control, so the 12/12 is not an artifact of a predicate that only ever returns True:

_section_is_waiting('[RESOLVED] x', '')        -> False
_section_is_waiting('RESOLVED — y', '')        -> False
_section_is_waiting('z', '**Status:** done')   -> False
_section_is_waiting('live q', '')              -> True

The predicate discriminates. The archive just does not speak its dialect. Isolating why:

title shape _ORG_HEADING _INLINE_RESOLVED recognised
RESOLVED — foo ✅ ❌ yes
[RESOLVED] foo ❌ ✅ yes
[RESOLVED 2026-08-27] foo ❌ ✅ yes
RESOLVED 2026-08-27 — foo ❌ ❌ no
foo — RESOLVED 2026-08-27 ❌ ❌ no
RETIRED 2026-08-26 — foo ❌ ❌ no

_ORG_HEADING needs the delimiter immediately after the keyword (RESOLVED\s*[—–\-:]), so a date between them defeats it. Both patterns are anchored, so a marker at the end of a title — the form people reach for when they resolve an entry in place — never matches. RETIRED is not in the vocabulary at all.

All three unrecognised shapes are present in this host's archive. Your fixtures use the two canonical forms, so the tests pass and the branch is green while never firing in the field.

So the unreachable success state moves rather than dies, and it lands somewhere harder to see: main says "parse fault" unconditionally, which is at least obviously unconditional. After this, the same host gets "parse fault, and 44 entries below the divider carry no resolution marker" — a specific, quantified, and wrong claim that reads like it was measured.

What I would suggest, weakly

I do not think the fix is to widen the regexes until they cover today's archive; that is a vocabulary chase and the next writer invents a new form.

The line I would re-examine is the docstring's premise: "zero_reason() must apply the SAME rule below the divider that this applies above it." I am not sure that is right, and it is the load-bearing choice here. Above the divider, "no marker" means live — the safe default, because an unlabelled active entry is probably a real question. Below it, "no marker" means someone moved it into the archive deliberately, so the safe default inverts. Being below the divider is itself the resolution marker for most entries; that is what the divider is for.

If the archive predicate were "has a marker OR is simply below the divider" — i.e. only flag entries carrying a positive live signal (**Status:** unanswered|waiting|open) — then the fault still fires for the case you are guarding (live entries stranded below a mis-anchored divider will usually carry their status line), and the affirmative branch becomes reachable on a normal file. That reading also matches what the divider means to a person editing the file by hand.

That is a design call and it is yours; I would take either version over main. Flagging it because the current shape passes its tests for a reason that does not survive contact with a real archive, and that is exactly the failure mode the surrounding test file exists to document.

Happy to re-run the measurement against any predicate you want to try.

…olved

Review measured that on a real archive the flagged entries are resolved in
prose the parser does not read, so "carry no resolution marker" is a specific,
quantified claim that reads as measured and is wrong about their status.

What the check actually knows is that it does not recognise them. Say that.

The classification itself is unchanged and deliberately so: the reviewer's
alternative -- treat an archived entry as resolved unless it carries a positive
live status -- silences the guard on the repo's own PARSE_FAULT fixture, whose
sections are prose with no status field, exactly like the stranded-live entries
the branch exists to catch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@TustinOC

Copy link
Copy Markdown
Contributor Author

Thank you — the measurement is the useful kind, and you found a real defect in the message. I've taken half of it and I'm declining the other half, with a measurement rather than an argument.

Taken: the claim overreaches, and it's now fixed (7a97e707)

You're right that this is the damaging part:

a specific, quantified, and wrong claim that reads like it was measured

It said carry no resolution marker. The check does not know that. What it knows is that it does not recognise them. That distinction is exactly what I landed on #3557 an hour earlier — "not one this check recognises" — and I failed to carry it across to my own next PR. Now:

... while 44 entr(ies) BELOW it carry no marker this check RECOGNISES
(they may be resolved in prose), the first being '...'

Same information, no false claim about their status.

Declined: inverting the archive default silences the guard on the repo's own fixture

I implemented your proposal exactly — flag an archived entry only if it carries a positive **Status:** unanswered|waiting|open — and ran the existing suite:

FAIL parse fault is NAMED as a probable fault
FAIL parse fault warns against trusting the zero
FAIL one unmarked entry below the divider STILL names the fault
FAIL the fault names the offending entry so it is actionable
17/21 passed

The fixture that breaks is PARSE_FAULT, line 68, predating this PR:

PARSE_FAULT = "# Resolved\n\n## Q1\n\nprose\n\n## Q2\n\nprose\n"

Prose sections, no status field. And that is not an accident of fixture-writing — it is the documented convention, from get_waiting_questions()'s own docstring:

the free-form prose format used in practice never writes a status field; sections are deleted when resolved, not marked done

So free-form live questions structurally do not carry a status line. Under the inversion, the 2026-07-30 population — live questions stranded below a mis-anchored divider — is precisely the set that gets classified resolved and goes silent. That is the failure the function exists to prevent, reintroduced at the one place it is guaranteed to matter.

Where I think the disagreement actually sits

Your premise is that below the divider, position is the resolution marker. For a human editing by hand, agreed — that is what the divider means. But the fault branch only runs when the divider is suspected wrong, and in that world position has already stopped being evidence. Reading it as a marker there assumes the thing under test.

What your finding does leave standing

The permanent false alarm persists on an unmarked archive — the message is honest now, but it still says "parse fault" every run. That is real and I am not claiming otherwise. I do not think the vocabulary chase fixes it and I do not think the inversion is safe, so the direction I would look next is structural rather than lexical: when the cut is genuinely mis-anchored, the real divider is still in the file below the stranded entries, so "entries between the computed cut and a later top-level divider" discriminates without reading anyone's prose. That is a different change and I would not grow this PR into it.

I would rather land this narrower version and open that as its own issue, unless you read it as leaving the alarm in a worse place than main. main says "parse fault" unconditionally and never says anything else; this says it conditionally, names what it could not read, and can say "affirmative". Happy to be argued out of that ordering.

21/21 at 7a97e707, review-checks.sh PASS. And I'd genuinely welcome you re-running your host measurement against the new wording — if 44 entries still read badly to you in the field, that is worth knowing before this lands.

…e cap

The replacement comment was 3 lines. Local verification piped the incremental
`git diff`, which showed only the changed lines; CI scans the cumulative
origin/main...HEAD diff, where the whole block reads as added.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The extraction is right and I ran a live parity test main's CI cannot.

_section_is_waiting() becoming the single place a ## section is judged live/resolved is the correct shape — it is the repo's own "duplicated policy is a defect in its own right" rule applied to a real instance. The old code judged liveness inline above the divider while zero_reason() assumed any empty active region meant a mis-anchored divider, so the legitimate success state — every question answered and archived — was unreachable in the message logic. Two places, two rules, one of them wrong.

Live parity test on a real production file

Unit fixtures cannot show that the refactor preserves behaviour on a file that grew organically. Mine has 32 open questions across all three formats this parser handles — legacy ## Q1 — Title sections, - **Status:** entries, and the free-form bullets the proactive-loop actually writes — plus an archive region below the divider.

Ran your parser against it with my own util_paths on the path, so it resolved and read the real file rather than a fixture:

main's reader        : 32 waiting
PR #3569's reader    : 32 waiting
top-3 titles         : identical, same order
VISIBLE_PREFIX       : 5 (unchanged)
_section_is_waiting  : present
zero_reason          : present

Read-only; I did not modify the file. Same count, same ordering, so the extraction is behaviour-preserving on a corpus that exercises every branch of the old inline rule.

Worst-case disruption

The consumer population matters here, and it is wide — check-pending-questions.py is read by the notifier, morning-briefing, agent-api, friction-detector and the dashboard. A parser regression would silently drop live questions from every one of them at once, which is the failure this file exists to prevent.

The change narrows rather than widens that risk: open used to fall through to the resolved-skip, so a question written with the word writers most naturally reach for was filed as answered and never surfaced. That is a live-entry-loss bug, and it is fixed here.

CI 20/20 green. I have not exercised the dashboard or agent-api consumers directly — only the shared reader they all call.

@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@TustinOC

TustinOC commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Following up because I addressed the review and a push notifies nobody. @yixuan-ag2 raised a real defect in the message text on 2026-08-30 and I took half of it; my reply at 14:54Z that day says which half and why I did not take the other. That has been sitting green ever since, and from where you sit an addressed finding and an ignored one look identical.

Current state: 64+/15- over 2 files, CI all green, one approval (@john-the-dev). Needs a second.

@yixuan-ag2 — you found the original defect, so if the half I declined still bothers you, a REQUEST_CHANGES saying so is more useful to me than silence. @bassilkhilo-ag2 @qingyun-wu — if @yixuan-ag2 is out, this is a small one.

The claim to check is narrow: a pending-questions.md whose questions have all been resolved parses to zero waiting items, and the reader was treating that as a parse fault rather than the legitimate empty state. The distinction matters because a parse fault is loud and a resolved file is the goal state, so the reader was loudest exactly when the user had done everything right.

Shows BEHIND; leaving it, for the reason in my #3647 comment.

@TustinOC

TustinOC commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

First live occurrence of this, on my own host, twenty minutes ago — and it happened to me rather than to a fixture.

Max approved a pending question in Slack. I moved it into the Resolved region, which took the file to genuinely zero open items. The very next cron fire printed:

0 pending questions, but pending-questions.md holds 57 '## ' section(s) and NONE are in the
active region (above the archive divider). That is the shape of a parse fault, not a quiet day —
check the '# Resolved' divider before trusting this zero.

Nothing was broken. I verified the file rather than trusting either the warning or my own edit:

'# Resolved' dividers : 1
'## ' sections total  : 57   (all below the divider — correct)
git diff --stat        : 1 file changed, 10 insertions(+), 3 deletions(-)   <- a clean move, no loss

So the reader is currently unable to distinguish "every question has been answered" from "the parser lost the divider", and it resolves that ambiguity toward alarm. That is the right default when the two are indistinguishable — but it means the goal state produces the scariest output the tool emits, which is what this PR fixes.

Why I think this is worth adding rather than just being a +1: the warning fires precisely when someone has finished the work. On a host that reaches zero, every subsequent fire says "parse fault" until a new question arrives. I nearly went and audited my own edit for damage on the strength of it — and the audit came back clean, which is the cost this PR removes.

Not re-soliciting on the approval count and not asking anyone to hurry. Posting because a real instance is better evidence than the constructed one in the body, and I had it.

(Also worth recording for whoever picks this up: my host reached zero because the last open item was answered, not because anything went wrong. If you want a reproduction, resolving the final question in a file with a large archive is the whole recipe.)

@TustinOC

TustinOC commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Closing this. main solved the same user-visible problem a different way while this sat, and rebasing it mechanically would now be a regression, not an update.

What changed under it. #3622 landed section_is_waiting() on main — public, with a fuller docstring, and with a guard this branch does not have:

# origin/main:123
if not title or _ORG_HEADING.match(title) or _INLINE_RESOLVED.match(title):

This branch's _section_is_waiting() is the same rule, privately named, without the not title guard. So the merge diff is almost entirely a rename in the wrong direction:

$ git diff origin/main:src/check-pending-questions.py <branch>:src/check-pending-questions.py --stat
 src/check-pending-questions.py | 71 ++++++++++++++--------------
 1 file changed, 37 insertions(+), 34 deletions(-)

And the headline behaviour is already fixed on main. Running both versions against a live 57-section, fully-resolved file:

main : "0 pending questions — the active region is empty and all 57 '## ' section(s) sit
        below the archive divider are explicitly resolved. This zero is affirmative."
PR   : "0 pending questions, but ... NONE are in the active region ... while 1 entr(ies)
        BELOW it carry no marker this check RECOGNISES ..."

Main reaches the affirmative answer through _active_region_lost(active_text), reading the header to tell a mis-anchored divider from a genuinely answered file. This PR reaches it by scanning the archive for entries carrying no recognised resolved-marker. Both work; main's is merged.

The one idea here that main does not have, recorded so it is not lost with the branch: the archive scan produces an independent second signal, naming how many entries below the divider carry no marker the checker recognises and quoting the first. On the host above that is 1 entr(ies). Main's header-based check cannot produce that number. If anyone wants it, the right shape is a small addition on top of main's existing section_is_waiting(), not this branch.

Closing rather than reworking, since the problem that motivated it is fixed and the residual is a nice-to-have. Reopen-able if the second signal turns out to be wanted.

@TustinOC

TustinOC commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Superseded in substance by #3622; see analysis above.

@TustinOC TustinOC closed this Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants