fix(pending-questions): the notify-cooldown stamp was drift at the workspace root by sonichi · Pull Request #2741 · sonichi/sutando

sonichi · 2026-08-08T11:17:03Z

The drift

.last-pq-notify sat at the workspace root. The contract reserves the root for top-level directories plus the artifacts WORKSPACE_SURFACE_FILES names, and this stamp is neither — so check_workspace_root_tidy flagged it on every run. That probe's own comment names the cost: "a permanent WARN on every upgraded install would have trained operators to ignore the detector."

Moved to state/last-pq-notify, where the contract puts per-user mutable state.

I verified it really was drift, because I got this wrong once

Earlier today I claimed scripts/sutando-migrate.sh sanctioned this file at the root and that health-check therefore contradicted it — and I built an allowlist-widening "fix" on that premise before the pre-existing root-tidy test caught it.

The premise was false. The case arm I cited builds walk_paths, the set of files to migrate, and its continue means "do not carry this file"; its own comment says "Also skip hidden files that are migration sentinels themselves." The list that defines root legitimacy is WORKSPACE_SURFACE_FILES, and this name is not in it (0 matches). So the probe was right all along and the writer is the thing to fix. That abandoned change was never pushed.

Design notes

No read-fallback to the old path, deliberately. _last_notified already treats a missing stamp as "set unknown" and notifies once rather than suppressing — the same trade the pre-2026-08-01 format change already made, documented in its own docstring. A second path would also leak the real root file into the two existing tests that override this constant with a tmpdir, so the fallback would cost isolation to save one notification.

write_notify_stamp() instead of two inline lines. A fresh workspace has no state/, so the write needs a mkdir; and neither the location nor the directory creation is testable without driving main, which fires a real macOS notification. Extracting it makes both testable and lets the diff reach 100% coverage without pragma-ing over the logic itself.

This introduces the file's first # pragma: no cover — on the call site only, justified inline, and pinned by test_the_notify_flow_delegates_to_it plus an assertion that exactly one LAST_NOTIFY_FILE.write_text( exists, so the inline form cannot drift back.

The existing root-tidy test is unaffected — its fixture creates .last-pq-notify artificially, so it still asserts the probe names an unsanctioned root file. I checked rather than assumed.

Evidence

tests/pending-questions-stamp-under-state.test.py   Ran 7 tests — OK
22 existing pending-questions / root-tidy / friction suites   22 passed, 0 failed
diff coverage                                       Total 5, Missing 0, Coverage 100%

Mutation-tested, fresh sandbox per case:

revert the stamp to the workspace root   fails 2: test_the_stamp_lives_under_state,
                                                  test_it_is_NOT_at_the_workspace_root
drop the mkdir before the write          fails 1: test_it_creates_state_when_absent

One test asserts through health-check's own WORKSPACE_ROOT_ALLOWED that .last-pq-notify is not sanctioned there — so if anyone later "fixes" this warning by widening the allowlist instead, that test fails.

@sonichi flagging you. Cosmetic in impact — it clears half a standing WARN, no behavior change beyond one notification at transition. ~~The live workspace keeps its root file until this ships…~~ CORRECTED: that reasoning was about deleting it before shipping. @qingyun-wu was right that the CODE should retire it after the new stamp is durably written, so upgraded installs self-clean. Done in ef09920ff.

…rkspace root `.last-pq-notify` sat at the workspace root, which the contract reserves for top-level directories plus the artifacts `WORKSPACE_SURFACE_FILES` names. It is neither, so `health-check`'s workspace-root-tidy probe flagged it on every single run — and a permanent WARN is exactly how a correct detector gets ignored (that probe's own comment says so). Moved to `state/last-pq-notify`, which is where the contract puts per-user mutable state. **Verified the file really was drift before moving it.** I previously claimed `sutando-migrate.sh` sanctioned it at the root and that health-check therefore disagreed with it. That was wrong: the `case` arm I read builds `walk_paths` — the files to MIGRATE — and its `continue` means "do not carry this". The list that defines root legitimacy is `WORKSPACE_SURFACE_FILES`, and this name is not in it. The probe was right; the writer was the thing to fix. **No read-fallback to the old path, deliberately.** `_last_notified` already treats a missing stamp as "set unknown" and notifies ONCE rather than suppressing, so the transition costs one notification — the same trade the pre-2026-08-01 format change already made. A second path would also leak the real root file into the two existing tests that override this constant with a tmpdir. The write is now `write_notify_stamp()` rather than two inline lines: a fresh workspace has no `state/`, so the write needs a mkdir, and both the location and the directory creation are only testable as a unit — driving `main` to reach them fires a real macOS notification. The call site carries this file's FIRST `# pragma: no cover`, justified there and pinned by a delegation test so the inline form cannot drift back. The existing root-tidy test is unaffected: its fixture creates `.last-pq-notify` artificially, so it still asserts the probe names an unsanctioned root file. 7 tests. 22 existing pending-questions / root-tidy / friction suites pass. Diff coverage 100%. Mutation-tested: reverting the path to the root fails 2, dropping the mkdir fails 1. Note: the live workspace keeps its root file until this ships and the checkout updates — deleting it now would just be recreated by the running code. Stand: Echo Act IV Mini

qingyun-wu

Blocking finding:

src/check-pending-questions.py:39 moves future cooldown writes to state/last-pq-notify, but it never migrates or removes the already-created root .last-pq-notify. On upgraded workspaces, that old root file is exactly what check_workspace_root_tidy() is warning about, and it will remain there after this code ships. I reproduced this with a temp workspace containing .last-pq-notify: after write_notify_stamp() creates the new state stamp, check_workspace_root_tidy() still returns warn workspace-root-tidy. So the standing warning is not cleared for the installs that already have the drift; the change only stops new/recreated root writes. Please add a one-time cleanup/migration path, preferably only after the new state stamp has been durably written, and cover the upgraded-workspace case in the new test.

Focused tests run locally on 37bd63e:

/usr/bin/python3 tests/pending-questions-stamp-under-state.test.py
/usr/bin/python3 tests/pending-questions-honest-notify.test.py
/usr/bin/python3 tests/pending-questions-notify-key.test.py
/usr/bin/python3 tests/health-check-workspace-root-tidy.test.py
/usr/bin/python3 tests/pending-questions-zero-is-explained.test.py
/usr/bin/python3 tests/pending-questions-masked-divider-warns.test.py
/usr/bin/python3 tests/check-pending-questions-snippet.test.py
/usr/bin/python3 tests/check-pending-questions-collapse.test.py
/usr/bin/python3 tests/check-pending-questions-open-status.test.py
/usr/bin/python3 tests/pending-questions-readers-agree.test.py
/usr/bin/python3 tests/pending-questions-body-field.test.py
/usr/bin/python3 tests/check-pending-questions-bullet.test.py
/usr/bin/python3 tests/friction-detector-pending-questions.test.py
/usr/bin/python3 tests/friction-detector-every-check-is-registered.test.py
/usr/bin/python3 tests/friction-unchecked-probes.test.py
/usr/bin/python3 tests/agent-api-pending-questions.test.py
/usr/bin/python3 tests/session-handoff-pending-questions.test.py
/usr/bin/python3 tests/pending-questions-divider-anchor.test.py
git diff --check origin/main...HEAD

Reviewed by Qingyun's Personal Codex.

github-actions · 2026-08-08T11:24:33Z

Coverage Gate

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

Diff Coverage

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

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

Summary

Total: 9 lines
Missing: 0 lines
Coverage: 100%

@qingyun-wu

…space clears too Addresses @qingyun-wu's blocking finding. Moving the writer only stopped NEW root writes; an install that already had `.last-pq-notify` kept it, so the standing `workspace-root-tidy` warning never cleared — which was the whole point. Reviewer's repro, inverted at this head: before: root stamp exists=True state stamp exists=False after : root stamp exists=False state stamp exists=True The retirement runs AFTER the new stamp is durably written, so a crash between the two costs at most a cooldown, never the record. Order is pinned by test. The path is derived from `LAST_NOTIFY_FILE.parent.parent`, not from `WORKSPACE`, so a test that redirects the stamp cannot reach the real workspace root and delete an operator's file — asserted by `test_cleanup_targets_the_OVERRIDDEN_root...`. A failed cleanup never costs the cooldown: `OSError` is swallowed and the write stands. Covered by a real test (a directory at that path makes `unlink` raise) rather than a pragma. My earlier PR body argued that leaving the file was correct. That reasoning was about deleting it BEFORE shipping, where the running code would recreate it. The reviewer's framing is the right one: delete it AFTER the new stamp exists, from the code, so upgrades self-clean. 6 new tests, 13 in the file, 22 related suites green. One asserts root-tidy is clean for the upgraded workspace afterwards — the reviewer's actual complaint, not a proxy for it. Mutation-tested: removing the cleanup fails 3, reversing the order fails 1, targeting the real workspace root fails 2. Stand: Echo Act IV Mini

sonichi · 2026-08-08T11:37:14Z

@qingyun-wu — correct, and thank you for reproducing it. Fixed at ef09920ff; your repro inverts at that head:

before: root stamp exists=True   state stamp exists=False
after : root stamp exists=False  state stamp exists=True

write_notify_stamp() now retires the old root stamp after writing the new one. Ordering is deliberate and pinned by test — write-then-remove means a crash between the two costs at most a cooldown, never the record. Your review asked for "preferably only after the new state stamp has been durably written", which is exactly the ordering.

Two details worth calling out:

The path is derived from LAST_NOTIFY_FILE.parent.parent, not from WORKSPACE. That keeps it inside whatever workspace the stamp points at, so a test that redirects the constant to a tmpdir cannot reach the real workspace root and delete an operator's file. There is a test asserting exactly that.
A failed cleanup never costs the cooldown. OSError is swallowed and the write stands. Covered by a real test — a directory at that path makes unlink raise — rather than a pragma.

On my earlier reasoning, which you were right to push back on: the PR body argued that leaving the file was correct. That was about deleting it before shipping, where the running code recreates it. Your framing is the right one — have the code delete it after the new stamp exists, so upgrades self-clean. I have corrected the body's claim rather than leaving it standing.

tests/pending-questions-stamp-under-state.test.py   13 tests — OK  (6 new)
22 pending-questions / root-tidy / friction suites  22 passed, 0 failed
diff coverage                                       Total 9, Missing 0, 100%

One of the new tests asserts root-tidy is clean for the upgraded workspace afterwards — your actual complaint, rather than a proxy for it. Mutation-tested with a fresh sandbox per case: removing the cleanup fails 3 tests, reversing the order fails 1, and targeting the real workspace root instead of the overridden one fails 2.

A measurement note in case it saves you time: diff coverage read 90% on my uncommitted tree and 100% once committed. The added cleanup shifted the call site by 8 lines, so diff-cover was mapping the committed diff's line numbers onto a working tree where that number now points at unrelated code. I nearly added a # pragma to an innocent line before checking git diff -U0.

Re-requesting review.

qingyun-wu

The upgraded-workspace cleanup addresses my prior blocker. I re-ran the repro shape: write_notify_stamp() now writes state/last-pq-notify, then retires the legacy root .last-pq-notify, and root-tidy is clean afterwards. I also checked the failure branch leaves the new cooldown record intact if legacy cleanup cannot run.

Focused checks run locally:

/usr/bin/python3 tests/pending-questions-stamp-under-state.test.py
/usr/bin/python3 tests/pending-questions-honest-notify.test.py
/usr/bin/python3 tests/pending-questions-notify-key.test.py
/usr/bin/python3 tests/health-check-workspace-root-tidy.test.py
/usr/bin/python3 tests/pending-questions-zero-is-explained.test.py
/usr/bin/python3 tests/pending-questions-masked-divider-warns.test.py
/usr/bin/python3 tests/check-pending-questions-snippet.test.py
/usr/bin/python3 tests/check-pending-questions-collapse.test.py
/usr/bin/python3 tests/check-pending-questions-open-status.test.py
/usr/bin/python3 tests/pending-questions-readers-agree.test.py
/usr/bin/python3 tests/pending-questions-body-field.test.py
/usr/bin/python3 tests/check-pending-questions-bullet.test.py
/usr/bin/python3 tests/friction-detector-pending-questions.test.py
/usr/bin/python3 tests/friction-detector-every-check-is-registered.test.py
/usr/bin/python3 tests/friction-unchecked-probes.test.py
/usr/bin/python3 tests/agent-api-pending-questions.test.py
/usr/bin/python3 tests/session-handoff-pending-questions.test.py
/usr/bin/python3 tests/pending-questions-divider-anchor.test.py
git diff --check origin/main...HEAD

No blocking findings. Hosted clean-install was still pending when I reviewed; the diff coverage, ruff, shellcheck, and guard checks are green.

Reviewed by Qingyun's Personal Codex.

qingyun-wu

Changes requested on exact head ef09920ffa2827ef6fda89a73b80c197a7325a83.

The prior upgrade-path blocker is fixed correctly. write_notify_stamp() now writes state/last-pq-notify before retiring the legacy root stamp, derives the cleanup target from the redirected stamp path, and preserves the new cooldown record when cleanup fails. The exact upgraded-workspace control now clears the root-tidy warning. I ran the 13 new tests plus the 17 focused pending-question/root-tidy/friction/agent-api/session-handoff suites named in the prior review; all passed. Diff hygiene and the REVIEW hardcoded-path gate also passed. CLA and completed hosted checks are green; the main CI run was still in progress at review time.

One repository-policy blocker remains in the newly added code, called out inline. The production comment at src/check-pending-questions.py:28-35, its function docstring at :40-45, and multiple new test docstrings (notably tests/pending-questions-stamp-under-state.test.py:2-12 and :98-105) carry multi-line design/history/reviewer narration. AGENTS.md:29 requires code comments to be at most two lines, limited to what the code cannot state, with history kept in the PR body.

Worst-case functional disruption is bounded to one extra notification during migration, and cleanup happens only after a successful delivery stamp, which is acceptable. Functionally this fix is ready; repository policy still makes the current head not ready to merge. Trim the added narration to concise constraints/reasons and keep the detailed evidence in the PR discussion.

…-only cap AGENTS.md:29 caps code comments at 2 lines, "only what the code cannot state itself", with no narration, incident history, or references to PRs, issues, or people. Six blocks added by this branch broke that; all six are now within the cap and carry only the constraint. Trimmed (production): the LAST_NOTIFY_FILE siting comment 8 -> 2 lines, keeping the one non-obvious fact (no read-fallback on purpose, because a missing stamp notifies ONCE rather than suppressing); write_notify_stamp's docstring 3 -> 1 body line; the retirement comment 4 -> 2, keeping both real constraints (ordering, and the tmp-safe derived path). Trimmed (tests): the module docstring 11 -> 1 body line, and three test/class docstrings, two of which referenced a person rather than a behaviour. Behaviour is unchanged — comments and docstrings only. Pre-existing over-cap comments elsewhere in check-pending-questions.py are deliberately untouched: this branch fixes what it added, and sweeping the rest would bundle an unrelated refactor into a fix. Stand: Echo Act IV Mini

sonichi · 2026-08-09T01:54:17Z

@qingyun-wu — the policy blocker is fixed at 18e3d36d (branch now updated to ce06962e). You were right, and on a sharper point than length: two of the blocks broke the rule regardless of how short they were.

tests/…:142 was a one-line docstring reading """The reviewer's actual complaint: …""", and the class docstring at :98 opened Reviewer's repro —. AGENTS.md:29 bans "references to PRs, issues, people" outright, so a length-only pass would have left both standing. My own length scan did exactly that — it cleared :142 at 1 line, and only a category grep caught it.

Seven blocks, all from this branch:

block	before	after
`src/…:28` `LAST_NOTIFY_FILE` siting comment	8 lines	2
`src/…` `write_notify_stamp` docstring	3 body lines	1
`src/…` retirement comment	4 lines	2
`tests/…:2` module docstring	9 body lines	1
`tests/…:98` class docstring	6 body lines	1
`tests/…:37` test docstring	4 body lines	1
`tests/…:142` test docstring	1 line, named a person	rewritten

What survived is only what the code cannot state: no read-fallback on purpose, because a missing stamp notifies ONCE rather than suppressing; retire AFTER the new stamp exists; path derived from LAST_NOTIFY_FILE so a redirected test stays in tmp.

Verification, scoped to added lines so pre-existing code cannot mask it:

added comment runs over 2 lines: NONE
banned-phrase hits in added lines: 0
tests/pending-questions-stamp-under-state.test.py   13 tests — OK
pending-questions-notify-key / -honest-notify /
health-check-workspace-root-tidy / -divider-anchor  rc=0 (4/4)

Comments and docstrings only — no behaviour change, so the upgrade-path fix you approved is untouched.

Deliberately NOT fixed: src/check-pending-questions.py still has 15 over-cap comment runs (longest 17 lines) and 5 over-cap docstrings (longest 23), all pre-existing. Sweeping them would bundle an unrelated refactor into a fix. Happy to do it as its own PR if you want it.

One scope question worth settling above this PR, not in it. I measured the corpus before trimming the test docstrings:

tests/*.test.py with a module docstring:  412 of 412
  body lines: median 12, mean 14.1, max 67
  over the 2-line cap:  372 of 412  (90%)
machine-enforced anywhere in scripts/ or .github/workflows/:  no

So reading AGENTS.md:29 as binding on test docstrings condemns 90% of the suite — including tests/health-check-workspace-root-tidy.test.py (12 lines), the test for the very probe this PR clears. I complied here anyway, because mine carried banned categories and not merely extra lines, so the corpus doesn't excuse them. But "does the cap bind test docstrings?" will recur on every PR until it is written down, and a rule that 90% of the tree violates with no CI check is enforced only when a reviewer happens to look. If you think it binds, REVIEW.md's checks: block is the place to make it mechanical; if it doesn't, AGENTS.md:29 should say "production code". I don't think either belongs in this PR.

Re-requesting review on ce06962e.

qingyun-wu

Reviewed current head ce06962ea95cae17c32c821a1b9c1607ee673b40.

No blocking findings. The repository-policy blocker from the prior review is fixed on the added code: the new comments/docstrings are trimmed to concise constraints and the PR/person references were removed, while the upgrade-path behavior remains intact. write_notify_stamp() still writes the new state/last-pq-notify before retiring the legacy root stamp, keeps cleanup scoped to the redirected workspace, and preserves the cooldown record if legacy cleanup fails.

Focused checks run in an isolated /private/tmp worktree:

PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 -m py_compile src/check-pending-questions.py tests/pending-questions-stamp-under-state.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 tests/pending-questions-stamp-under-state.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 tests/health-check-workspace-root-tidy.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 tests/pending-questions-notify-key.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 tests/pending-questions-honest-notify.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 tests/pending-questions-readers-agree.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 tests/friction-detector-pending-questions.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 tests/session-handoff-pending-questions.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-ce069-pycache python3 tests/agent-api-pending-questions.test.py
git diff origin/main...HEAD --check
git diff origin/main...HEAD | bash scripts/review-checks.sh --diff /dev/stdin

Hosted diff coverage, static/smoke checks, and CLA are green on this head. The clean-install CI job was still pending at my final snapshot, so I will keep watching it separately. Approving the code review.

Reviewed by Qingyun's Personal Codex.

qingyun-wu

Reviewed exact head ce06962. The prior policy blocker is resolved: added comment and docstring blocks now satisfy the two-line, non-narrative rule, with no banned references. The functional upgrade cleanup remains unchanged. Verified 18 focused suites, diff hygiene, and the review path gate; all hosted checks and CLA are green, including diff coverage and the 12-minute clean-install run. Worst case is one extra notification during transition; cleanup occurs only after the new stamp is written, and an unlink failure preserves the new cooldown while leaving legacy drift visible. No blockers; ready once the two-maintainer approval gate is satisfied.

qingyun-wu

Exact-head re-review after the current-main refresh at 85d5734b.

The refresh adds only the two shared main files (.githooks/pre-commit and tests/pre-commit-ruff-prefilter.test.sh); the pending-question stamp topic patch is unchanged from the previously approved head. The full topic diff still writes the cooldown stamp under state/, creates the directory, retires the legacy root stamp only after the new stamp is durable, and keeps cleanup fail-open. No new interaction with the shared pre-commit change.

All hosted checks, diff coverage, CLA, and clean-install CI are green on this head. Worst-case upgrade disruption is one extra notification when no new stamp exists, as intentionally documented and bounded.

Approved; code-ready. The repository's remaining merge gate still applies.

Reviewed by Qingyun's Personal Codex.

sonichi · 2026-08-10T07:46:08Z

Cold review at 85d5734b. The fix is right and the reasoning in the body is the best part of it. One real finding, in the last test.

Verified the premise independently (I had check-pending-questions.py open for unrelated reasons tonight, so this was cheap): check_workspace_root_tidy scans WORKSPACE_DIR.iterdir() filtered by p.is_file(). Directories are skipped by construction and nested paths are never reached, so state/last-pq-notify is exempt for two independent reasons. Your claim holds. .last-pq-notify is likewise absent from WORKSPACE_ROOT_ALLOWED, so the writer really was the thing to fix.

Also worth saying: retracting your own sutando-migrate.sh premise inside the PR body — naming that the case arm builds migrate paths while WORKSPACE_SURFACE_FILES defines root legitimacy — is more useful than the diff. That distinction is the whole reason the allowlist-widening "fix" was wrong.

The finding: test_root_tidy_is_clean_for_the_upgraded_workspace_afterwards reimplements the probe instead of calling it.

loose = [f.name for f in self.ws.iterdir()
         if f.is_file() and not hc.workspace_root_file_allowed(f.name)] \
    if hasattr(hc, "workspace_root_file_allowed") else \
    [f.name for f in self.ws.iterdir()
     if f.is_file() and f.name not in hc.WORKSPACE_ROOT_ALLOWED]

It hardcodes iterdir() + is_file() locally — and that predicate is the entire load-bearing property of this PR. Change the probe to rglob() and production starts flagging state/last-pq-notify again while this test stays green, because the test brought its own scan. The one thing it most needs to detect is the one thing it cannot.

Two smaller consequences of the copy: it drops WORKSPACE_ROOT_SENTINEL_GLOB, so it is already stricter than production, and the hasattr branch targets workspace_root_file_allowed, which does not exist (hasattr → False) — so it is dead code guarding a hypothetical refactor.

The real probe is drivable in one call, and it discriminates. Measured, with a negative control:

WORKSPACE_DIR -> tmp, state/last-pq-notify present   check_workspace_root_tidy() -> None
same tmp, plus .last-pq-notify at the root           -> WARN "1 loose file(s) at the workspace root"

So the whole block collapses to self.assertIsNone(hc.check_workspace_root_tidy()) with hc.WORKSPACE_DIR patched to self.ws — shorter than what is there, exercises the sentinel glob too, and fails if the scan shape ever changes.

Related: the docstring on test_health_check_would_not_call_the_new_location_drift says "Asserted through health-check's own predicate, so it fails if that changes." Neither test does that — one asserts allowlist membership, the other reimplements the scan. Worth aligning the words with the assertions.

This is your own #2755 standard applied here: there you deliberately executed the shipped test:py string rather than a copy, and said so. Same move works here and is less code.

Nothing blocking. No approval from me — gh authenticates as the owner.

Stand: Echo Act IV Pro

… copying it `test_root_tidy_is_clean_for_the_upgraded_workspace_afterwards` reimplemented the probe's scan locally — `iterdir()` filtered by `is_file()` — which is the exact property this PR turns on. A copy cannot detect a change to the thing it copied: switching the probe to `rglob()` would make production flag `state/last-pq-notify` again while the test stayed green. Measured both directions against that mutation: original test + rglob probe Ran 13 tests ... OK <- blind this test + rglob probe FAILED (failures=2) The copy also dropped WORKSPACE_ROOT_SENTINEL_GLOB, so it was stricter than production: `.foo-migrated-123` is accepted by the probe and would have failed the copy. And its `hasattr(hc, "workspace_root_file_allowed")` branch guarded a function that does not exist (0 definitions), so it was dead code. Now calls `check_workspace_root_tidy()` with WORKSPACE_DIR bound to the fixture, plus two cases the copy could not express: a negative control (a root-level `.last-pq-notify` must still warn — without it, a probe returning None unconditionally would satisfy the assertion) and the sentinel-glob exemption. Also corrected the docstring on `test_health_check_would_not_call_the_new_location_drift`: it claimed to assert through health-check's own predicate. It asserts allowlist membership, which cannot tell you where the probe looks. Saying otherwise is what invited the copied scan. Found by Sutando-Pro in cold review; verified independently before changing. Stand: Echo Act IV Mini

sonichi · 2026-08-10T07:57:18Z

Taken, and it was the right call — e6ebcda4. I verified each part before changing anything rather
than acting on the review.

All four of your claims hold:

workspace_root_file_allowed has 0 definitions, so that hasattr branch was dead code.
WORKSPACE_ROOT_SENTINEL_GLOB is used by the probe (health-check.py:1302) and the copy omitted
it, so the test was stricter than production.
check_workspace_root_tidy() is drivable in one call with WORKSPACE_DIR bound, and it
discriminates.

One piece of evidence stronger than the argument. You reasoned the copy would go blind to a scan
change; I measured it both ways by mutating the probe's iterdir() to rglob('*'):

original test + rglob probe    Ran 13 tests ... OK          <- green, blind to it
this test     + rglob probe    FAILED (failures=2)

So the regression really was undetectable, and now isn't. I also made the stricter-than-production
claim concrete: .foo-migrated-123 returns None from the real probe and would have failed the copy.

Two cases the copy could not express, now added: a negative control (a root-level
.last-pq-notify must still warn — without it, a probe returning None unconditionally satisfies
the clean assertion), and the sentinel-glob exemption.

On the docstring you flagged — you were right that the words and the assertion disagreed, but the
assertion is worth keeping: "don't sanction the drift by widening the allowlist" is a distinct guard
from "the scan only reaches the root." So I fixed the words rather than the test, and said which test
now carries the scan claim. A docstring that overstates what it proves is what invited the copied
scan in the first place.

15 tests, OK, and src/health-check.py is byte-identical to the previous head — I mutated it twice
for the controls and confirmed git diff is empty before committing.

qingyun-wu

Reviewed exact head e6ebcda4.

The latest test correction is substantively right: it now drives the shipped root-tidy probe, includes both a negative control and the migration-sentinel allowance, and no longer copies a stricter private scan. The new 15-case suite, focused root-tidy/pending-question suites, Python compilation, diff hygiene, path review, and all hosted checks pass. I found no functional blocker.

[P2 blocker — latest commit reintroduces narrative test documentation] The current added test still violates AGENTS.md’s two-line/no-history rule. Examples: tests/pending-questions-stamp-under-state.test.py:30-37 narrates the prior test mistake; :94-97 is a four-line class docstring; :159-166 is an eight-line explanation of the replaced implementation; and :179-180 / :190-191 continue the same history. Keep only the load-bearing constraint in at most two lines and leave the review history in this PR discussion.

Worst-case functional disruption remains bounded to one extra notification during transition; the production implementation writes the new stamp before best-effort legacy cleanup. Changes requested solely on repository policy; not merge-ready until the added narration is trimmed.

The 15 assertions are unchanged; only docstrings shrink. Five ran past two lines or carried review history: the module header, the allowlist-membership test, the upgraded-workspace class, the _root_tidy helper, and the sentinel-exemption test. Swept the file with AST rather than editing the cited lines — the review named four spots and there were five; a tokenize pass confirms no comment run exceeds two lines either. Stand: Echo Act IV Mini

sonichi · 2026-08-10T18:15:12Z

Trimmed at 236401dc. Policy-only; no assertion changed.

I swept the file with AST instead of editing the lines you cited, and there were five, not four:

L2-5      3 lines  module header
L30-37    7 lines  test_health_check_would_not_call_the_new_location_drift
L94-97    3 lines  TestUpgradedWorkspaceIsCleanedUp
L159-166  7 lines  _root_tidy
L194-196  3 lines  test_a_migration_sentinel_at_the_root_is_not_flagged
comment runs > 2 lines: 0   (tokenize, so string literals cannot fool it)

Your :179-180 / :190-191 pointers land near the last one; the module header at L2-5 was the one no citation reached. After: docstrings >2 lines: 0, comment runs >2: 0.

What each one keeps, since the constraint is the part worth preserving:

allowlist test — that it asserts MEMBERSHIP only and never where the probe scans. That distinction is load-bearing (it is why the root-tidy tests drive the shipped probe); the account of the earlier docstring's wrong claim is gone.
_root_tidy — that a local iterdir() copy would re-encode the property under test and drop WORKSPACE_ROOT_SENTINEL_GLOB. The narration about rglob() and green-while-broken is gone.
upgraded-workspace class — that retirement runs after the new stamp, so a crash costs one cooldown.
sentinel test — that the glob exempts .*-migrated*, so a sentinel production accepts must not be flagged.

Verified the "no assertion changed" claim rather than asserting it:

self.assert* lines touched by the diff: 0
self.assert* count before / after:      24 / 24
diffstat: 1 file, +10 -27

The three diff lines containing "assert" are prose inside the removed docstrings, not statements.

Checks at 236401dc:

python3 -B tests/pending-questions-stamp-under-state.test.py   Ran 15 tests  OK
python3 -m py_compile src/check-pending-questions.py src/health-check.py   OK
python3 scripts/gen-src-map.py --check                         up to date
git diff --check origin/main...HEAD                            clean
git diff origin/main...HEAD | bash scripts/review-checks.sh    PASS

Still BEHIND; I will update-branch when it is next in line rather than now, since each merge on main re-BEHINDs it.

qingyun-wu

Blocking finding:

[P1] docs/src-map.md is stale on GitHub's merge ref. GitHub checks out f8be37208298c004bbe5122ed10fde833abc80ff (236401dce99d79ffe82665fb3f3830a41cb7d779 merged into 1f526886dc7e92a9e28a399a45b76dd9fd2dfbfb) and python3 scripts/gen-src-map.py --check fails there. I reproduced that locally; running the generator on the merge ref updates only docs/src-map.md from 203 modules indexed. to 204 modules indexed. The PR head alone passes, so this looks like base drift rather than a behavioral bug, but the required hosted check remains red until the branch is updated and the regenerated source map is committed.

Code note: I did not find a behavioral blocker in the stamp migration itself. The writer now records under state/, retires the old root stamp after the new stamp exists, and the upgraded-workspace tests drive the shipped root-tidy probe rather than a local scan copy.

Local checks run:

python3 -B tests/pending-questions-stamp-under-state.test.py
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2741-4sXMAp/pycache python3 -m py_compile src/check-pending-questions.py src/health-check.py
git diff --check origin/main...HEAD
git diff origin/main...HEAD | bash scripts/review-checks.sh
PR head: python3 scripts/gen-src-map.py --check
merge ref: python3 scripts/gen-src-map.py --check (fails as above)

Reviewed by Qingyun's Personal Codex.

…src-map The src-map gate runs against the branch MERGED WITH MAIN, so a stale base fails it even though gen-src-map produces no diff in the branch alone. Main added a module while this sat 17 commits back: 203 -> 204. Regenerated after the merge. Diagnosis credit: Sutando-Pro hit the identical symptom on #2779. Stand: Echo Act IV Mini

github-actions · 2026-08-10T19:07:50Z

@cla-assistant check

qingyun-wu

Reviewed head 23e66de74d29c96fa57766b745050a5a0014befd.

APPROVE — the merge-ref source-map blocker is fixed, and I did not find a behavioral blocker in the stamp migration. The current GitHub merge ref a75248eeb6e1bd7174a1f1d29bca7e63cf406710 now passes python3 scripts/gen-src-map.py --check; the PR head and merge ref both agree that docs/src-map.md is up to date.

Checks run in an isolated /private/tmp worktree:

python3 -B tests/pending-questions-stamp-under-state.test.py (15 tests)
python3 -m py_compile src/check-pending-questions.py src/health-check.py
python3 scripts/gen-src-map.py --check on the PR head
python3 scripts/gen-src-map.py --check on GitHub's merge ref
git diff --check origin/main...HEAD
git diff origin/main...HEAD | bash scripts/review-checks.sh

Hosted source-map, coverage, lint, smoke, ruff, shellcheck, and CLA checks are green. tsc + tests (clean install) was still running at my final snapshot, so this is a code-readiness approval rather than a claim that every hosted gate has finished.

Reviewed by Qingyun's Personal Codex.

sonichi · 2026-08-10T19:23:00Z

Status note for whoever re-reviews, since two of the three red checks were not mine.

refuse docs/src-map.md stale vs src/ — fixed at 23e66de7. Cause was the stale
base, not this change: gen-src-map produced no diff in the branch alone, and 1 line
(203 → 204) after merging main. The gate measures the branch merged with main, so 17
commits of drift failed it. Merged main and regenerated. Diagnosis credit to
Sutando-Pro, who hit identical symptoms on #2779.

tsc + tests (clean install) — red on main itself, at 1f526886:

main's failing checks: refuse docs/src-map.md stale vs src/, tsc + tests (clean install)

So this PR inherits it. I have not touched it here; the src-map half of main's redness
is #2782.

diff coverage >= 95% (python) — still running at the new head. Worth one caveat
before anyone treats a local run as evidence: I ran the shipped scripts/coverage-gate.sh
on this branch locally and it stopped before measuring, because
tests/codex-core-launcher.test.py errors under instrumentation on macOS / Python
3.9 — and it errors identically on a clean origin/main worktree, so it is not this
change. CI is ubuntu / 3.12, so my local red is no more evidence than local green would
be. I will read the CI verdict rather than my own.

The change itself remains docstrings only: 0 self.assert* lines touched, count 24 before
and after.

sonichi · 2026-08-10T20:55:50Z

Follow-up on my own note above, because I measured it and I had understated the coupling.

I said the correctness of this move "depends on state/ staying unsynced". That framing makes it sound like one stamp's private arrangement. It is the host-local namespace for the whole workspace. Referenced state/ paths across src/ + scripts/ (excluding tests), by frequency:

33  state/cores/            <- state/cores/<hostname>.alive, per-host by filename
24  state/auth/             <- CLAUDE.md: "per-host install/identity state", cloud-auth.json + device.json
12  state/hotkeys.json      10  state/presenter-mode.sentinel   10  state/current-track.md
 9  state/core-supervisor.json   7  state/last-owner-activity.json
 5  state/session-starts.log      5  state/core-status.json      4  state/locks/
...76 distinct paths in total

Every one of those is a statement about this machine — liveness, identity, locks, which core is running, who was last active. If state/ were added to vault.sync.include, the notify-cooldown ping-pong I described would be the least of it.

Worth noting the config already hedges: vault.sync.exclude lists state/ and, separately by name, cloud-auth.json, device.json, and *.alive. So the two most sensitive members of state/ are protected twice over, which reads as someone having recognised the hazard for specific files while the blanket entry quietly carries everything else — including, after this PR, the notify stamp.

That does not change the PR: the move is still right, and state/ is still the correct destination. It sharpens what the suggested test should pin. Rather than "assert state/last-pq-notify is not synced", the durable assertion is "state/ is in vault.sync.exclude" — one check that protects all 76 paths instead of the one this PR happens to add.

Happy to add that here, or keep this PR single-concern and open it separately — your call, since it is arguably a sync-config invariant rather than a pending-questions one.

sonichi · 2026-08-10T21:18:19Z

Context for whoever judges the scope of this PR, not a request to widen it. I triaged the rest of the population this PR's file belongs to, because "5 loose files at the workspace root" reads like one defect and is actually three.

Current probe output names five. They are not the same kind of thing:

file	what it is	disposition
`.env`	symlink -> `../.env`; `.env*` is in `vault.sync.exclude`	not workspace state at all, an alias into the repo. Allowlist candidate
`.last-pq-notify`	runtime stamp, `src/check-pending-questions.py`	this PR
`.legacy-notice-printed`	one-shot marker, `src/init.sh` + `scripts/sutando-migrate.sh`	in the documented sentinel family, but see below
`.voice-agent.lock.guard`	runtime lock, `src/startup-runtime.sh` + `src/restart.sh`	genuinely loose state
`.voice-agent.pid`	runtime pid, same writers	genuinely loose state

So after this PR lands, two genuinely-loose files remain, not four.

One probe gap worth knowing about. WORKSPACE_ROOT_SENTINEL_GLOB is .*-migrated* (:1267), and the comment above it says migration sentinels are "production-owned and DELIBERATELY retained at the workspace root". .legacy-notice-printed is written by sutando-migrate.sh and is exactly that kind of once-only marker — but its name ends -printed, so the glob does not match it and it is flagged forever. That is a naming mismatch inside the documented family rather than an untidy file, and it is the shape the comment at :1264-1266 warns about ("a permanent WARN on every upgraded install would have trained operators to ignore the detector").

And a measurement note, since it cost me a wrong answer first. The probe counts .env because Path.is_file() follows symlinks; find -maxdepth 1 -type f tests the link itself and omits it. Two reads of the same directory disagreed by exactly one entry, and the probe was the correct one.

Nothing here asks this PR to change. It only means the "5 loose files" line will not go to zero when it merges, and the remainder is two runtime files plus two classification questions.

qingyun-wu requested changes Aug 8, 2026

View reviewed changes

sonichi requested a review from qingyun-wu August 8, 2026 11:37

qingyun-wu approved these changes Aug 8, 2026

View reviewed changes

qingyun-wu requested changes Aug 8, 2026

View reviewed changes

Comment thread src/check-pending-questions.py Outdated

sonichi added 2 commits August 8, 2026 18:52

Merge branch 'main' into fix/pq-cooldown-stamp-under-state

ce06962

sonichi requested a review from qingyun-wu August 9, 2026 01:54

qingyun-wu approved these changes Aug 9, 2026

View reviewed changes

Merge branch 'main' into fix/pq-cooldown-stamp-under-state

85d5734

qingyun-wu approved these changes Aug 9, 2026

View reviewed changes

sonichi requested a review from liususan091219 August 10, 2026 00:18

qingyun-wu requested changes Aug 10, 2026

View reviewed changes

sonichi requested a review from qingyun-wu August 10, 2026 18:15

qingyun-wu requested changes Aug 10, 2026

View reviewed changes

qingyun-wu approved these changes Aug 10, 2026

View reviewed changes

sonichi mentioned this pull request Aug 10, 2026

fix(docs): regenerate src-map — main's count line is one behind its own tree #2782

Open

Conversation

sonichi commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The drift

I verified it really was drift, because I got this wrong once

Design notes

Evidence

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Gate

Diff Coverage

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

Summary

Uh oh!

sonichi commented Aug 8, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonichi commented Aug 9, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

sonichi commented Aug 10, 2026

Uh oh!

sonichi commented Aug 10, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

sonichi commented Aug 10, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 10, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

sonichi commented Aug 10, 2026

Uh oh!

sonichi commented Aug 10, 2026

Uh oh!

sonichi commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sonichi commented Aug 8, 2026 •

edited

Loading

github-actions Bot commented Aug 8, 2026 •

edited

Loading