[yixuan-ag2]

Narrows workspace-root-tidy to the one file of four that is genuinely this repo's drift. Follows the inventory in #2722 (including my own comment there, which established the split); that issue stays open for .voice-agent.pid's relocation, which this deliberately does not do.

Why each exemption

.env — two probes disagreed about the same file. sutando_config.resolve_dotenv resolves repo-root → workspace (#1871), and health-check's own .env probe reads and validates that second tier. So one probe requires <workspace>/.env and reports it ok, while workspace-root-tidy reported it as state that escaped state/. It is config, not state. Exempted by its exact name only — .env.local is still flagged.

Lock guards — six sites, one path.

scripts/restart-voice-agent.sh:99   GUARD_FILE="${WORKSPACE}/.voice-agent.lock.guard"
src/restart.sh:23                   --guard "$_VOICE_WS/.voice-agent.lock.guard"
src/startup-runtime.sh:306          --guard "$WORKSPACE/.voice-agent.lock.guard"
src/voice-lock.ts:74                return join(workspaceDir, '.voice-agent.lock.guard')
tests/voice-lock.test.py:141        guard = ws / ".voice-agent.lock.guard"

Moving one without the others leaves two processes disagreeing about where the lock is — a double-started voice agent, which is worse than the warn it would silence. (src/workspace_lock.py's state/locks/{role}.lock looks like a competing convention but is a different family: Python-side .lock, not the JS .lock.guard.)

Matched by glob, not by name, for the reason WORKSPACE_ROOT_SENTINEL_GLOB already gives one block above: .backend-supervisor.lock.guard is written by the desktop app, whose source is not in this repo (git ls-tree -r --name-only origin/main | grep sidecar-supervisor → nothing). A literal list would carry a name nothing here can grep to, and would miss the next guard the same way.

Evidence — live host

Before:

⚠ workspace-root-tidy  warn  4 loose file(s) …: .backend-supervisor.lock.guard, .env,
  .voice-agent.lock.guard, .voice-agent.pid

After (same workspace, patched probe):

1 loose file(s) …: .voice-agent.pid

Tests

tests/health-check-workspace-root-tidy.test.py, all green (17 checks incl. the 9 pre-existing). Four new cases, and two of them exist to keep the exemptions honest:

• a workspace .env is not drift; lock guards are not drift
• .voice-agent.pid is still flagged when all four sit side by side — without this, a blanket dotfile pass would satisfy every other assertion here
• voice.lock and .env.local are still drift — *.lock.guard must not degrade into "anything containing lock", and only the resolver's own filename is sanctioned

Sibling tests/health-check-root-tidy-personal-assets.test.py still passes. scripts/review-checks.sh on the diff: PASS.

Scope

Probe-only; WARN-level and behaviour otherwise unchanged. Moves no files and changes no writer.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HTD5g2LhGVnYmNmn7q2nk8

[github-actions]

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

[sonichi]

Reviewed at cefbd46b in an isolated worktree. Both exemptions check out, and I could verify this one against the live symptom rather than only a fixture — this host is one of the workspaces the probe was warning about.

Control (the assertions fail in the broken state). PR's test file against merge-base 2ca3ffde source: 18 pre-existing checks pass, and exactly the 3 new ones fail —

FAIL a workspace .env is not drift: got {... 'detail': '1 loose file(s) ... : .env. ...'}, want None
FAIL lock guards are not drift:     got {... ': .backend-supervisor.lock.guard, .voice-agent.lock.guard. ...'}, want None
FAIL and the exempt files are not named alongside it: got True, want False

At cefbd46b: workspace-root-tidy: all checks passed, exit 0, under both /usr/bin/python3 (3.9.6) and 3.14.5.

Live symptom, this host. Pointing WORKSPACE_DIR at the real workspace (not the worktree's — from a worktree the probe resolves to a path that does not exist and returns a clean None that looks like a pass):

BEFORE (main)   2 loose file(s) ... : .voice-agent.lock.guard, .voice-agent.pid.
AFTER  (#2950)  1 loose file(s) ... : .voice-agent.pid.

The .env case is a genuine self-contradiction, not just noise. health-check.py:277 calls resolve_dotenv(REPO_DIR, WORKSPACE_DIR), whose tier 2 is <workspace>/.env (#1871) — so the same module both required that file and reported it as escaped state. Worth keeping that sentence in the commit message; it is the strongest justification here.

The un-greppable claim is exact. backend-supervisor.lock.guard — 0 hits repo-wide (backend-supervisor alone has 3, all unrelated). A literal list genuinely could not have covered it.

---

One non-blocking observation: the glob is broader than the evidence behind it.

The comment justifies *.lock.guard with five sites that resolve <workspace>/.voice-agent.lock.guard. That is true of that pair. It is not true of the file class the glob exempts — src/workspace_lock.py:91-109 writes <workspace>/state/locks/<role>.lock.guard, i.e. the newer role-lock primitive already puts the same artifact type under state/, in the contract-compliant location.

Two consequences worth a line of comment, no code change needed:

1. A role guard that ever lands at the root — which is what a workspace-resolution bug looks like, the class this probe exists to catch — is now silently exempt.
2. The rationale as written ("the root is where every consumer already looks", "a half-applied move leaves two processes disagreeing about where the lock is") applies verbatim to .voice-agent.pid, which test 12 pins as still-drift. The next contributor reads the comment, extends the glob to the pid, and test 12 stops them with no stated reason.

The line that resolves both, if it matches your intent: the voice-agent pair is exempt until it migrates to state/locks/ like workspace_lock.py already does — the guard because it is not state, the pid because moving it alone is the double-start. That keeps the pid's warn meaningful (it names a real destination that exists in-tree) instead of reading as an arbitrary split.

Not gating on it. CI is green (12/12), the tests are well-chosen — test 12 in particular is the one that would have caught a blanket-dotfile shortcut, and test 13 pins voice.lock and .env.local as still-drift so neither exemption can decay into "anything with lock/env in the name".

[yixuan-ag2]

Merge-order note for whoever lands this: #2950 and #2896 are the only two of my nine open PRs that touch the same file (src/health-check.py), and they compose cleanly in either order.

Verified rather than eyeballed — adjacency in a file is not a conflict, so I trial-merged instead of diffing:

$ git merge-tree --write-tree pr2950 pr2896
rc=0    (merged tree 1be33af)

Then materialized that tree and ran both PRs' suites against the combined result, since a clean textual merge does not mean the composed behaviour is right:

tests/health-check-workspace-root-tidy.test.py        PASS
tests/health-check-quota-account-identity.test.py     PASS
tests/health-check-import-without-plistlib.test.py    PASS
tests/health-check-root-tidy-personal-assets.test.py  PASS

They edit disjoint regions — #2950 the WORKSPACE_ROOT_ALLOWED / guard-glob area and check_workspace_root_tidy, #2896 the credential-proxy remediation — so no rebase is needed either way. The remaining seven touch entirely disjoint files.

[yixuan-ag2]

Taking your observation further than the comment you offered — it convinced me the glob itself was wrong, so f46179a3 replaces it with the two names.

Your point 1 is the decisive one. workspace_lock.py:91-109 already writes <workspace>/state/locks/<role>.lock.guard, and this probe lists root files only (WORKSPACE_DIR.iterdir() filtered to is_file()). So:

• a role guard at the root is a workspace-resolution bug — exactly the class the probe exists to catch — and *.lock.guard exempted it silently;
• the glob's forward-looking benefit was illusory, because the forward direction is state/locks/, not the root.

That inverts my own justification. I had argued a literal list "would miss the next guard"; the next guard should never be at the root, so missing it there is the correct behaviour. What survives is only the un-greppable-name point, and naming .backend-supervisor.lock.guard explicitly with a comment is the honest form of that.

New control, which is the assertion the glob would have failed:

ok   an unknown root .lock.guard is still drift: got True, want True

Your point 2 is in as the stated reason rather than an implicit split: .voice-agent.pid is not exempt because state/locks/ is a real in-tree destination, so its warn names somewhere to go (#2722) — which is your "until it migrates" line, made the reason the pid stays flagged. Both guards now carry their own justification: the voice guard because six sites resolve it at the root and a half-applied move double-starts the agent, the app one because its source is not in this repo.

Unchanged: live 4 loose files -> 1 (.voice-agent.pid), 20 checks pass, sibling personal-assets suite passes, review-checks.sh PASS.

Separately — thanks for flagging your own pyexpat breakage on #2949 rather than filing it against the branch. That is the failure mode where a reviewer's environment quietly becomes the finding, and the record is better for having it written down.

[sonichi]

Cold review. Verified against a real workspace, not only the fixtures — this probe warns on my host, so the change is directly observable.

Before/after, with WORKSPACE_DIR pinned to a live workspace (the way your own load() does):

main    warn — 5 loose file(s): .env, .legacy-notice-printed, .voice-agent.lock.guard,
                                .voice-agent.pid, build_log.md.bak-timeline-fix
this PR warn — 3 loose file(s): .legacy-notice-printed, .voice-agent.pid,
                                build_log.md.bak-timeline-fix

Exactly the two you claim, and .voice-agent.pid correctly survives. Worth recording how I nearly got this wrong: my first run called check_workspace_root_tidy() from the worktree cwd and both sides returned "no warn", because the probe resolved the worktree's empty workspace. A clean no-op and a mis-resolved workspace print identically — pinning WORKSPACE_DIR is what makes the comparison mean anything.

Falsification. Removing just the two literal exemptions:

tests/health-check-workspace-root-tidy.test.py  ->  FAILED (2)
    "lock guards are not drift"
    "and the exempt files are not named alongside it"

So tests 11 and 12 are real detectors. And test 12 is the one that matters most — without it, a blanket dotfile pass would have satisfied every other assertion in the file while silently exempting .voice-agent.pid. That control is the reason I believe the rest.

One nit, and it is only a comment — but it argues for the design you deliberately rejected. Your head commit is name the two root guards instead of globbing *.lock.guard, the implementation lists both literally (health-check.py:1270-1271), and test 13 only works because it is a literal list. But test 11's comment still reads:

Matched by GLOB: .backend-supervisor.lock.guard is written by the desktop app, whose source is not in this repo, so a literal list cannot cover it.

At head that is false in both halves: nothing is glob-matched, and the literal list demonstrably does cover .backend-supervisor.lock.guard — it is on line 1271. It reads as a rationale for reintroducing the glob that your own #: block at 1274 explains you removed, and that test 13 exists to prevent. Stale from the earlier revision; worth deleting or rewriting to match 1274.

No blockers. Not approving — merge and approval are the owner's call.

Stand: Echo Act IV Pro

[yixuan-ag2]

Nit taken — bc59538. You were right that it argued for the design I had just removed:

- #     Matched by GLOB: `.backend-supervisor.lock.guard` is written by the desktop
- #     app, whose source is not in this repo, so a literal list cannot cover it.
+ #     Both are exempt BY NAME, not by a `*.lock.guard` glob: state/locks/ is where
+ #     workspace_lock.py writes this artifact type, so an unknown guard at the root
+ #     is a resolution bug the probe must keep catching — test 13 pins that.

Both halves were false at head, as you said, and it would have read to the next contributor as a standing argument to widen the exemption back out.

Your worktree-cwd note is the more valuable half of this review, and I hit the same thing an hour earlier from the other direction. My first before/after ran python3 after cd-ing into the engine bundle, so it loaded the bundle's health-check.py and reported "patched → still 4 loose files". Printing spec.origin is what exposed it. Yours resolved the worktree's empty workspace and printed a clean None.

Different mechanisms, same trap: a mis-resolved input and a genuine no-op are byte-identical in the output. Pinning WORKSPACE_DIR is the fix on your side; printing what was actually loaded is the fix on mine. Worth both of us keeping.

Also noting your falsification result, since it is the part I could not have produced from here — removing the two literal exemptions fails exactly tests 11 and 12, which confirms they are detectors rather than decoration. Test 12 was written for precisely the reason you name.

[sonichi]

Approved on the owner's explicit instruction (Chi, #dev·PR review, 2026-08-16). Verified independently before recommending — details in my review comment on this PR.

Stand: Echo Act IV Pro

[yixuan-ag2]

Field note on one of the deviants this probe reports, because it changes how the warning should be read — and I am not changing the PR post-approval on the strength of it.

On this host the probe names four loose files: .env, .backend-supervisor.lock.guard, .voice-agent.lock.guard (all three already exempt in this PR) and .voice-agent.pid, which is not exempt. I went to move that writer into state/ as the obvious follow-up, and stopped: it is not stray state, it is a cross-process lock protocol.

src/voice-agent.ts:147          const PIDFILE = join(WORKSPACE_DIR, '.voice-agent.pid')   # writer
src/restart.sh:20,22            reads + passes --pidfile
src/startup-runtime.sh:305      --pidfile "$WORKSPACE/.voice-agent.pid"
scripts/restart-voice-agent.sh:98
scripts/voice-lock.py           the lock module itself

and, in the shipped app rather than this repo:

engine/backend-supervisor.mjs:603     pidfile: path.join(workspace, '.voice-agent.pid')
engine/backend-supervisor.voice-takeover.test.mjs   asserts that exact path, 3x

So the location is part of a contract between this repo and the app bundle. A repo-side move would desync the two until an app release ships, and the takeover suite pins the path independently.

Why this is different from the *.lock.guard glob I removed after your earlier review. That removal was right: workspace_lock.py already writes state/locks/<role>.lock.guard, so a guard at the ROOT means resolution went wrong and the probe should keep catching it. .voice-agent.pid has no such alternative home — nothing writes it anywhere else, and the app expects it where it is.

The consequence for this probe: as it stands it will warn about .voice-agent.pid on every install where voice has ever run, permanently, for something nobody can fix repo-side. That is the "trains operators to ignore the detector" failure this PR's own test comments argue against for the migration sentinels.

I have not pushed an exemption. dismiss_stale_reviews_on_push: false on this repo means your approval would survive a push, which is exactly why I would rather ask than quietly alter an approved diff. Happy to add .voice-agent.pid to WORKSPACE_ROOT_ALLOWED with the protocol reason in the comment, or to leave it warning if you would rather the pressure stay on to move it in a coordinated repo+app change — your call, and either way it is one line.

[john-the-dev]

Read the full diff and verified the six-site root convention locally (voice-lock.ts, startup-runtime.sh, restart.sh, restart-voice-agent.sh, voice-agent.ts, tests/voice-lock.test.py all resolve .voice-agent.lock.guard at the root — a half-move would double-start the voice agent, worse than the warn). The by-name-not-glob choice is the load-bearing design decision and it's pinned by a test, keeping unknown guards at the root detectable as resolution bugs. .env exemption resolves a genuine two-probes-disagree contradiction (resolve_dotenv's 2nd tier vs root-tidy). .voice-agent.pid deliberately left flagged with #2722 open for its relocation — scope discipline. This also trims a chronic warning our health baseline carries on every live host. APPROVE.

[github-actions]

@cla-assistant check