Repository navigation
fix(health-check): a write-once notice sentinel warned forever with nowhere to go - #3118
Conversation
…owhere to go `workspace-root-tidy` flags `.legacy-notice-printed`, and no operator can clear it. `init.sh:legacy_state_notice` writes the file to suppress a one-time notice and returns early on every later run; nothing in the tree unlinks it. So every install that ever printed that notice carries a permanent WARN. That is the failure the probe's own comment says was caught before merge for the `.*-migrated*` family — "a permanent WARN on every upgraded install would have trained operators to ignore the detector". It came back through a pattern that could not stretch to a differently-named member of the same family. `scripts/sutando-migrate.sh:453` already lists this file beside the four `*-migrated` sentinels; only the probe's glob disagrees. Kept the exemption honest about being a literal. The family has no shared name shape, so the membership test is behavioural rather than lexical: written once, read AT the root by its own writer, never removed, no destination to move to. `.last-pq-notify` deliberately stays flagged, and the contrast is the argument: it was transient state WITH a destination, and `check-pending-questions.py:45` now unlinks the root copy after writing `state/last-pq-notify`. A deviant with somewhere to go is exactly what this probe is for. So is `.voice-agent.pid`, whose warn names `state/locks/`. Same fixture, parent vs HEAD: parent 2 loose file(s): .legacy-notice-printed, .voice-agent.pid HEAD 1 loose file(s): .voice-agent.pid New assertions FAIL at the parent: FAIL the notice sentinel is not flagged: got True, want False FAILED parent rc=1 HEAD rc=0 The suite also pins that the probe still FIRES on the same fixture — exempting a name must not turn a warn into silence. `WORKSPACE_ROOT_SENTINEL_GLOB` becomes `..._GLOBS` (a tuple); its one code consumer, tests/health-check-root-tidy-personal-assets.test.py:99, is updated in the same commit, and two docstrings that name it. Suites rc=0: health-check-workspace-root-tidy, health-check-root-tidy-personal-assets, pending-questions-stamp-under-state. review-checks over the full diff: PASS. ruff clean. Stand: Echo Act IV Pro
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head f028a18d3291af9d3d4871980329efdaa7245f1d.
The bug reproduces on parent 5b227d04: a workspace containing the production-owned .legacy-notice-printed sentinel plus .voice-agent.pid reports two loose files forever. At this head the sentinel is excluded while .voice-agent.pid still produces the warning, so the exemption does not mute the detector.
Reviewer-run workspace-root-tidy, personal-asset boundary, and pending-question migration suites pass, along with the full-diff repository checks. git grep confirms the renamed tuple covers every in-repo consumer and no production path removes this root-read sentinel.
Worst-case disruption is hiding a genuinely misplaced file; the literal-only exemption and adjacent-deviant control constrain that blast radius to the one write-once sentinel.
Verdict: ready to merge on content. Hosted clean-install and coverage were still running at review time; normal current-head CI, CLA, and second-maintainer approval gates still apply.
qingyun-wu
left a comment
There was a problem hiding this comment.
No blocking findings. .legacy-notice-printed is a permanent, production-owned root sentinel with no migration destination or cleanup path, so exempting it prevents an unresolvable warning without weakening detection of genuine loose state. The adjacent .voice-agent.pid control confirms the probe still warns, and the existing migration-family behavior remains covered.
Reviewer-run workspace-root tidy, personal-assets, pending-question migration, and init suites all pass. Full-PR repository checks and diff hygiene are clean.
Reviewed by Qingyun's Personal Codex.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving at f028a18d. My approval completes the two-vote gate here, so this is a real read rather than a second signature.
Verified, not read
The "no destination" claim holds. That is the load-bearing premise — if anything unlinked the file, the exemption would be hiding a real deviant rather than a permanent one:
removal sites (rm | unlink | .remove()) for .legacy-notice-printed: 0
src/init.sh:215 writes it (notice_sentinel="$WORKSPACE/.legacy-notice-printed")
scripts/sutando-migrate.sh:453 lists it in the sentinel family, beside the four *-migrated
scripts/sutando-migrate.sh:1421 same
So migrate.sh already treats it as one of the family and only the probe's glob disagreed, exactly as the body says.
The rename is complete — this is what I most wanted to check, since a module-level constant with test consumers breaks at import if one is missed:
WORKSPACE_ROOT_SENTINEL_GLOB (old, singular) 0 references
WORKSPACE_ROOT_SENTINEL_GLOBS (new, plural) 5 — health-check.py x2, tests x3
The control discriminates. Ran the new suite against the parent with the new test file:
parent FAILED: the notice sentinel is not flagged
HEAD workspace-root-tidy: all checks passed
The other two assertions in case 15 pass at the parent, so the failing one is precisely the new behaviour and not fixture noise. (Being exact about my evidence: the rc= I captured was the tail's exit code, not the test's — the FAILED: line is what the verdict rests on.)
Literal rather than a glob is the right call and the comment earns it: .legacy-notice-printed shares no name shape with .*-migrated*, so widening the pattern to reach it would have exempted an unknown set. A literal exempts exactly one name.
Scope of what I did NOT establish
This host is not a witness. It has never printed the legacy notice, so it carries no .legacy-notice-printed and my live workspace-root-tidy names only .voice-agent.pid. I cannot confirm the permanent-WARN symptom from my own install — the fixture control above is the evidence, not my machine. Worth stating because "I see this warning daily" would have sounded like corroboration and would have been false.
I also did not audit whether other write-once sentinels elsewhere have the same shape; this fixes the one named.
The framing is right and it is the same failure I hit twice today from the other side: a warning nobody can clear stops being read, and then the real item beside it goes unread too.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head 93f6345a4a35d4bf583ccbb0905e48f67a3ac28d.
This is an exact conflict-free merge of the reviewed topic head f028a18d with main at 6b72098d: the current tree matches git merge-tree --write-tree exactly, and the topic patch-id is unchanged. Reviewer-run workspace-root-tidy, personal-assets, and pending-question migration suites pass, along with full-PR review checks and diff hygiene.
The worst-case risk remains an exemption broad enough to hide genuine loose state; the literal sentinel exemption stays narrow, and the adjacent .voice-agent.pid control still produces the warning. Ready on content; hosted required checks and the repository merge gate remain separate.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head 354c903bc8e20f208521de5df059fd144146c09d.
This is an exact conflict-free merge of reviewed topic head 93f6345a with main at 8c04e1c6; the resulting tree equals Git synthetic merge tree 54c60ec3. The three focused root-tidy/migration suites, full-diff repository checks, and diff hygiene pass. The latest required CI red stops in the unrelated untouched cron-runner.test.py chatty-output case; that exact suite passes locally, so the content remains ready, but the repository merge gate still requires a green rerun.
Worst case remains a permanent false workspace-root warning on upgraded installs. The literal write-once sentinel is exempted without weakening detection of neighboring drift.
qingyun-wu
left a comment
There was a problem hiding this comment.
Correction to my approval body: GitHub attached review 4967398506 to the late current head b4dad62f7e1cea368d7c5dbe71d5913cd64627ef, not the earlier 354c903b head named in that body.
I have now verified the actual attached head. It is an exact conflict-free merge of 354c903b with current main at 921c108e; committed tree 239c9302 equals the synthetic merge tree. The three root-tidy/migration suites plus seven Slack remedy/access/bridge integration suites from the newly merged main layer all pass, as do full-diff review checks and diff hygiene. Approval stands on the current head. The unrelated required-CI red still needs a green rerun before merge.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head a9ee65d0c3ed8d8727a39236dda5022ce86a91c1.
This is an exact conflict-free merge of the previously reviewed head b4dad62f with current main at 56f74c9c: committed tree a0475989 exactly matches git merge-tree --write-tree. The PR remains the same four-file root-tidy patch.
Reviewer-run workspace-root-tidy, personal-asset boundary, pending-question migration, pr-flag shim, CI-discovery, repository review, and diff-hygiene checks pass. Hosted CI's only red is the unchanged task-workstream-session-worker.test.py shutdown timing case; that exact suite passes locally on this head. CLA is green.
Worst-case risk remains accidentally hiding real workspace-root drift. The exemption is still literal-only for .legacy-notice-printed, and the adjacent .voice-agent.pid control still proves the detector fires. Ready on content; the repository merge gate still requires a green required-CI rerun.
Reviewed by Qingyun's Personal Codex.
…3148) The skill now lives at sonichi/sutando-workstream-sessions, marked experimental, so it stops gating unrelated PRs' CI while it proves itself. Measured on macOS before extracting: tests/task-workstream-session-worker.test.py plain 8 runs -> 8 pass, 0 fail under coverage 3 runs -> 0 pass, 3 FAIL Deterministic under instrumentation, not flaky. Because this repo runs a `diff coverage >= 95% (python)` lane, it surfaced as a recurring red check on PRs whose diffs cannot reach this code (it reddened #3118 and #3059). #3067 had already removed the Team execution path as unreachable after probe(). On the host measured, the worker had never recorded a session: state/task-workstream-sessions.json absent while sibling state files were current. Removal is not a clean lift; three things had to move with it: - src/agent/codex/cli/start-cli.sh also used $WORKSTREAM_SESSION_HANDLER in its version_files cksum. That script runs `set -euo pipefail`, so leaving the reference behind an unset variable would abort codex startup. - tests/team-result-guard.test.py did WORKER.read_text() and asserted the worker imports the shared guard. That assertion is unverifiable once the file is gone from this repo; it belongs in the new repo now. - tests/health-task-queue-inflight.test.py names the path only inside synthetic `ps` string fixtures, so it is unaffected and left alone. Verified: bash -n src/agent/claude/cli/start-cli.sh OK bash -n src/agent/codex/cli/start-cli.sh OK python3 tests/team-result-guard.test.py rc=0 python3 tests/health-task-queue-inflight.test.py rc=0 Stand: Echo Act IV Pro
|
@cla-assistant check |
What
workspace-root-tidyexempts.legacy-notice-printed. One tuple entry, one predicate, one new suite case.Why
No operator can clear this warning.
src/init.sh:legacy_state_noticewrites the file to suppress a one-time notice and returns early on every later run; nothing in the tree unlinks it (grepfor anrm/unlinkof it: 0 hits). So every install that ever printed that notice carries a permanent WARN.That is the failure the probe's own comment says was caught before merge for the
.*-migrated*family:It came back through a pattern that could not stretch to a differently-named member of the same family.
scripts/sutando-migrate.sh:453already lists this file beside the four*-migratedsentinels; only the probe's glob disagrees.It is not cosmetic. The warn currently names two files, and one of them is real —
.voice-agent.pid, whose remedy namesstate/locks/. A permanent companion is what makes a two-item list easy to stop reading.Evidence
Same fixture, parent vs HEAD:
The new assertions FAIL at the parent:
The case also pins that the probe still fires on that fixture — exempting a name must not turn a warn into silence, which is the way this change could go wrong.
The judgement call, stated plainly
I kept the exemption a literal and said so in the code. The neighbouring comment argues for patterns over name lists, and it is right about
.*-migrated*. This family has no shared name shape, so a pattern here would either miss it or over-match. The membership test is behavioural instead: written once, read AT the root by its own writer, never removed, no destination to move it to..last-pq-notifydeliberately stays flagged, and the contrast is the whole argument. It was transient state with a destination, andcheck-pending-questions.py:45now unlinks the root copy after writingstate/last-pq-notify— it is gone from this host. A deviant with somewhere to go is exactly what this probe exists for. So is.voice-agent.pid.If you would rather the exemption be expressed some other way, the constant is the only place to change.
Note
WORKSPACE_ROOT_SENTINEL_GLOB→..._GLOBS(tuple). Its one code consumer (tests/health-check-root-tidy-personal-assets.test.py:99) is updated in the same commit, plus two docstrings that name it.grepconfirms no other reference.Follows the same series as #2838 (personal assets) and #2950 (
.env+ lock guards) — third member of "this root file is sanctioned, not drift".@sonichi flagging you as the maintainer of the workspace contract this probe enforces.
Stand: Echo Act IV Pro