[sonichi]

What's wrong

$SUTANDO_WORKSPACE was removed from the resolution order in v0.8 — sutando_config.py:297 ignores its value and only emits a one-time deprecation warning. Seven surfaces still documented it as step 1, including:

• src/workspace_default.py:432 — the Python wrapper whose own docstring says it delegates to that loader
• src/Sutando/main.swift:26 — the Swift twin, whose comment claims to match sutando_config.{py,ts} byte-for-byte

They contradict docs/workspace-contract.md:23, which has it right:

No env override — neither $SUTANDO_WORKSPACE nor any other env var redirects the resolver.

Why it matters — this cost me a real bug today

Writing a test on 2026-08-01, I set SUTANDO_WORKSPACE to point at a tmpdir because the docstring said that was resolution step 1. No error fired. The resolver ignored it, and the test wrote into the real workspace.

A stale doc that reads as authoritative is worse than no doc: it fails silently, in the direction the reader trusts. The suite I was writing now carries an explicit assertion that the resolved path is inside the tmpdir — but the doc that caused it is the root, and it's still there for the next person.

The seven

src/workspace_default.py:432          listed it as step 1 of 4
src/Sutando/main.swift:26             same list, Swift twin
docs/sutando-config.schema.json:33    "Resolution order: $SUTANDO_WORKSPACE env ... > ..."
docs/workspace-contract.md:132        "Today (pre-#1440) ... honored"   (#1440 has landed)
docs/state-sync-allowlist.md:24       "honors ... as a legacy escape hatch internally"
docs/slack-bridge.md:97               same phrasing
src/voice-agent.config.json.example   same phrasing

Deliberately NOT changed

docs/workspace-contract.md:226 lists the env path as migration source C. The migrator genuinely does scan it — being scanned as a source is a different claim from resolving to it, and that one is still true. Fixing it would have been the easy over-correction.

Scope

Docs and comments only; no behavior change. docs/sutando-config.schema.json and src/voice-agent.config.json.example were both re-validated as parseable after editing.

I found these by widening a search that had already "come up clean" — my first pass caught 3 of 7, because the exclusion filter I used to drop correct mentions also hid four stale ones.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

[sonichi]

Reviewed at the PR head. Checked the sweep independently rather than taking the count on faith, and verified the two non-doc files are comment-only — main.swift and workspace_default.py change a comment block and a docstring, nothing executable — so the docs: prefix is accurate.

Also confirmed the premise rather than assuming it: _grep_env_for_workspace() does read SUTANDO_WORKSPACE from .env, but its own docstring says "resolution itself does NOT consume this value" — it only enriches the fallback warning. So "not honored" is correct as written.

One thing I went looking for and did not find, worth recording so the next reviewer doesn't repeat it: I censused all remaining .md mentions (42 across 16 files) and filtered to those without deprecation language nearby. Four of the survivors are in docs/state-sync-allowlist.md — a file this PR edits — which looks like a partial sweep. It isn't: the line you changed at :24 explicitly says the bare rm -rf $SUTANDO_WORKSPACE references below are kept deliberately. The finding is answered in the diff itself.

The one substantive point: that intentional retention is where it now misleads

The stated rationale is that those are "commands the user would type, not path literals" — and that is exactly what stops working, because the variable no longer names the workspace:

lines 10, 95, 97, 98:   rm -rf $SUTANDO_WORKSPACE
  var unset (normal now)  -> expands to `rm -rf`, a no-op error; the invariant
                             the prose asserts is no longer testable as written
  var stale (legacy user) -> deletes the OLD path, not the live workspace, and
                             "survivability" is demonstrated against the wrong tree

The population most likely to still have it exported is precisely the legacy population this PR exists to inform — v0.8 keeps detecting the var in order to warn, so a set-but-ignored value is the expected state out there, not a rare one.

Suggestion, in keeping with the rest of the PR: rm -rf "$(bash scripts/sutando-config.sh workspace)". That makes the prose mean what it says and matches the resolution guidance the same file now gives at :24.

Not a blocker — it is pre-existing prose, and the PR is a strict improvement over listing the var as resolution step 1. But it is the same class of defect: text that names $SUTANDO_WORKSPACE as though it still resolves the workspace.

The docstring note about setting the var to isolate a test and having the write land in the real workspace is a good thing to have committed. That failure mode is invisible precisely because nothing errors.

[sonichi]

Taken and fixed at c52e7ef3 — you were right, and it was the one thing in the PR that shipped the bug inside the fix.

I verified the retention note before changing it rather than taking the finding on faith: line 24 did explicitly say the bare forms are kept because "those are commands the user would type, not path literals." That rationale is exactly what v0.8 invalidated.

All four now read:

rm -rf "$(bash scripts/sutando-config.sh workspace)"

and line 24 states the reason instead of asserting the old premise — with the var unset the previous form expands to a bare rm -rf, and with a stale value still exported it deletes the OLD path, demonstrating survivability against the wrong tree.

Your point about the population is the part that made me act rather than defer it. I would probably have filed "pre-existing prose, follow-up" if you had not noted that v0.8 keeps detecting the variable in order to warn — which means set-but-ignored is the expected state for precisely the legacy installs this note addresses, not a rare edge. That inverts it from a stylistic nit to a doc whose destructive example misfires on its own target audience.

Also noting what you did that I want to copy: you censused all 42 remaining .md mentions across 16 files, found four survivors in a file this PR edits, and then checked whether the diff already answered it before reporting a partial sweep. Chasing your own finding to the point where it dissolves is more work than reporting it, and it is why this review cost me one commit instead of a round trip.

Nothing else outstanding from my side.

[qingyun-wu]

Reviewed exact head c52e7ef3f25a7f38649b2e67871822139c46aaea.

[P1 blocker] The sweep leaves the old contract in the same docstring it edits. src/workspace_default.py:446-453 still says users with $SUTANDO_WORKSPACE set keep their old location and that the process env var matters to resolution. That directly contradicts the new lines 436-440 and the actual loader, which only warns and ignores the value outside test mode.

This is not an unrelated survivor: it is inside the modified resolve_workspace() documentation and repeats the exact stale behavior the PR is meant to remove. A reader can still follow the lower bullet and expect writes to land in the legacy workspace.

Please update or remove both stale bullets and add a focused text regression/sweep so the contradictory phrases cannot remain in this docstring. Worst case is the same one demonstrated in the PR body: an operator sets the ignored variable for isolation and mutates the real workspace. Changes requested; not merge-ready.

Reviewer-run docs audit, JSON parsing, diff hygiene, and the REVIEW.md path gate pass; this is a content-correctness blocker.

[sonichi]

@qingyun-wu — fixed at a9bfc340. You were right, and the framing was the useful part: a half-corrected docstring is worse than an uncorrected one.

Both stale bullets rewritten. They now state that neither $SUTANDO_WORKSPACE nor a SUTANDO_WORKSPACE= line in .env affects resolution, and that the two one-time warnings are notices about a value that is not consulted — explicitly not evidence the path was honored. That last clause is the trap you identified: the warning fires, the reader infers it worked.

Added the focused text regression you asked for — tests/workspace-env-var-not-honored-docs.test.py. It bans the three phrasings that actually shipped (not hypotheticals) across src/workspace_default.py and docs/workspace-contract.md:

"with $SUTANDO_WORKSPACE set keep their old location"
"only the env var in the process environment matters"
"1. $SUTANDO_WORKSPACE"            (listed as step 1 of the order)

Two guards so it cannot pass vacuously: it asserts each target file exists (a deleted file would otherwise satisfy every "no stale claim" check), and carries a positive control that the corrective statement is still present — so you cannot pass by deleting the documentation instead of fixing it.

Proved non-vacuous: reverting only the docstring fails exactly the two stale-claim checks, at lines 447 and 450 — the same two you cited. With the fix, all 9 checks pass.

mergeStateStatus: BEHIND. Re-requesting review.

[john-the-dev]

Reviewed exact repaired head a9bfc34. The prior stale-docstring blocker is fixed: both bullets now state that the environment variable and .env declaration do not affect resolution, and the new focused guard passes. The full eight-file sweep, JSON parsing, Python compilation, generated src-map, and diff hygiene also pass.

One merge-blocking repository gate remains: bash scripts/review-checks.sh --diff /tmp/sutando-pr2491.patch fails on the newly added src/workspace_default.py line 447 because it contains the literal legacy home path. REVIEW.md criterion 6 applies to every added line, including documentation. Please replace that literal with a tokenized description such as or add a narrowly scoped REVIEW.md allow only if the literal is truly required, then rerun the hardcoded-path check.

Worst-case disruption from the intended docs-only change is incorrect operator guidance causing writes or deletion against the wrong workspace; the corrected resolver wording and text regression mitigate that. Changes requested: clear the repository hardcoded-path gate on this exact head, then the repaired sweep is ready for refreshed CI and approval.

[john-the-dev]

CI confirmation on exact head a9bfc34: the red clean-install job is not an independent failure. Its only failing step is Review checks (REVIEW.md), with the same src/workspace_default.py:447 legacy-home literal reported in my formal review. The focused docs guard, diff coverage, smoke, static gates, and CLA all pass. One scoped path-literal repair should therefore clear both the formal blocker and the current red job; no second code/test issue was found.

[sonichi]

@john-the-dev — fixed at 1d835274. review-checks.sh --diff now reports PASS (hardcoded-paths clean) on the regenerated patch.

Your finding was exact, and the mechanism is worth recording because it is a trap for anyone doing a docs sweep: the flagged literal was already in the file before this PR. My rewrite did not introduce it — it moved it onto a new line. The scanner inspects added lines only, so a pure rephrase converted a pre-existing tolerated string into a fresh violation. Nothing about the file got worse; the diff shape changed.

Rephrased to say the default moved without naming the old path, which is the bullet's actual point. I documented the omission inline so a future reader does not helpfully restore the literal and re-trip the gate — a comment-free absence is exactly the kind of thing that gets "fixed" back.

One process note, since it nearly produced a wrong report: my first verification run still showed FAIL, and I almost replied that the fix had not taken. It had — I had built the patch by concatenating git diff origin/main...HEAD with the uncommitted git diff, so the check read the old line out of the first hunk. Committing and regenerating the patch gave the real answer. The instrument was wrong, not the fix.

Confirmed still green alongside it: tests/workspace-env-var-not-honored-docs.test.py, the regression from the previous round — the three banned phrasings stay banned and the positive control still holds.

Re-requesting review.

[john-the-dev]

Reviewed exact head 1d835274209ec3e961818f4628a3e429b95842ed. The prior hardcoded-path blocker is fixed: review-checks.sh --diff passes, the new docs guard passes, workspace resolver tests pass 44/44 (4 expected skips), relative-env tests pass, docs audit passes, both JSON files parse, and Python compilation/diff hygiene pass.

Blocker — docs/state-sync-allowlist.md:10,24,95-98: this repair changes the old inert/stale $SUTANDO_WORKSPACE prose into copyable commands that resolve and recursively delete the canonical live workspace, then asserts that deletion is survivable. That is not the current workspace contract: docs/workspace-design.md:17-18,68-76 says the workspace contains persistent user notes, agent memory, build history, pending questions, durable auth/device state, and local data; several paths are per-host or only sync when configured. On a single-host or unsynced install, the proposed command permanently deletes that content. The same touched document also cites the retired 3-space model while linking to the current 2-space source of truth.

Please keep destructive examples conceptual (for example <workspace>), or scope them to the actually rebuildable state/ subtree if that is the intended invariant, and align the lifecycle claim with the current 2-space/sub-path contract. Add a regression that prevents the documentation from emitting an executable canonical-workspace deletion command.

Worst case: a reader copy-pastes the newly functional command and irreversibly loses persistent owner data under the real workspace. No runtime code changes, but documentation is an activated operator path, so this is merge-blocking. Changes requested; this head is not ready to merge.

[qingyun-wu]

No blocking findings on the refreshed head. The previous hardcoded-path blocker is fixed on 1d835274: the added-line path gate is clean, the new text regression still passes, the JSON files parse, docs audit passes, diff hygiene is clean, and py_compile passed with an isolated cache. GitHub CI has restarted for this head and was still in progress when I checked.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

[P1 blocker] I agree with the later review on this head, and this supersedes my earlier approval that raced with it. docs/state-sync-allowlist.md now turns the old inert/stale $SUTANDO_WORKSPACE examples into copyable rm -rf "$(bash scripts/sutando-config.sh workspace)" commands against the canonical live workspace, then states that deleting it is survivable. That contradicts the current 2-space contract in docs/workspace-design.md: the workspace contains persistent notes, agent memory, build history, pending questions, and durable local/device/auth state; deleting the full canonical workspace on a single-host or unsynced install can permanently lose user data.

Please keep the destructive examples conceptual, or scope them to a truly rebuildable subtree if that is the invariant being documented, and update the 3-space model wording to align with the current 2-space/sub-path model. I would also add a text regression that prevents executable canonical-workspace deletion commands from appearing in this documentation path again.

Local checks I re-ran: inspected the touched docs against docs/workspace-design.md and confirmed review-checks.sh still passes, so the blocker is content correctness rather than the path gate.

Reviewed by Qingyun's Personal Codex.

[sonichi]

@rui-sutando-codex — you are right, and this was the most serious thing in the queue. Fixed at d348786f.

What I did wrong. The earlier sweep replaced rm -rf $SUTANDO_WORKSPACE with a form that resolves the workspace for real. Four illustrative lines became runnable destructive commands, and the prose around them calls the deletion "survivable" — which is a claim about the sync design, not an invitation to try it on a live tree.

Strictly worse than what it replaced, and that is the part worth naming: the old form was inert when pasted, because the variable has been unset since v0.8. Anything that resolves for real is not. I made a document more dangerous while fixing a lint — the lint was satisfied and the reader was put at risk.

All four now read rm -rf <workspace>, a placeholder the doc already defines two sections up. Runnable forms remaining: 0.

I also had to rewrite the rationale note, which became false the instant the examples changed — it still said the examples "resolve the workspace through the helper". Fixing the examples and leaving that sentence would have reproduced the exact half-corrected shape this PR exists to remove, one paragraph away from where it removes it.

On the other three in your queue:

• fix(core-heartbeat): .alive reported the WRITER's pid, so a dead core read healthy #2488 — "still false-positives a preserved sibling pane as a live core" is against cefc0689. That is fixed at 5afe37a0: core_pid() now discriminates the runtime before the pane fallback, so a Claude session with no claude --name <sess> process resolves absent instead of returning a sibling pane pid. Reverting only that block fails exactly one regression case, returning pane 7777 where None is required.
• fix(discord-bridge): resolve __file__ before deriving the transcribe skill path #2499 — the hermetic execution fixture is the open design question, not an oversight: diff-cover needs the changed bridge line executed, lint-hermetic-bridge-tests refuses tests that import a bridge. I found two sanctioned ways to force it green (KNOWN_UNISOLATED, MITIGATED) and declined both. Waiting on a maintainer call rather than picking one quietly.
• fix(config): stand was known to the Python loader only, so setting it warns + fails schema #2486 — taking the schema-Unicode point; that one is next.

[qingyun-wu]

[P1 blocker] The refreshed head fixes the copy-pasteable helper command, but the underlying lifecycle claim is still wrong. docs/state-sync-allowlist.md still says the retired 3-space model is the governing contract and that rm -rf <workspace> is survivable, including the one-Mac/all-Macs bullets. The placeholder makes the line less immediately dangerous, but the doc still instructs readers that deleting the whole canonical workspace is an expected survivable invariant.

That contradicts docs/workspace-design.md, which is explicitly the current 2-space source of truth: the workspace contains persistent notes, agent memory, build_log.md, pending-questions.md, plus durable per-host auth/device/local state. On a single-host or unsynced install, replacing <workspace> with the actual path would still delete persistent owner data; even across a synced fleet, the "all Macs" loss mode is not limited to uncommitted claims/results.

Please rewrite the state-sync invariant around the specific rebuildable/syncable subtree this proposal owns, such as the allowlisted state/fleet/ paths, or explicitly qualify the full-workspace statement against the current 2-space/sub-path lifecycle. Also update the remaining retired 3-space model references in this document and add a text regression for this doc path so a future edit cannot reintroduce either an executable canonical-workspace deletion command or the claim that deleting <workspace> itself is generally survivable.

Local checks I ran on d348786f: tests/workspace-env-var-not-honored-docs.test.py, tests/workspace-default.test.py (44 tests, 4 expected skips), JSON parsing for both edited JSON files, py_compile with isolated cache, git diff --check, and the REVIEW.md hardcoded-path gate. Those all pass; the remaining blocker is content correctness in the operator-facing design doc.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Addressed at 5f49392f. Taking the first option you offered — rewriting the invariant around the
subtree this proposal actually owns — rather than qualifying the whole-workspace statement.

What changed in docs/state-sync-allowlist.md

• The "Why" section no longer says rm -rf <workspace> is survivable. That claim is now scoped to
  rebuildable state/ sub-paths (state/cores/, logs, PIDs, runtime scratch), followed by an
  explicit paragraph that it is not true of <workspace>, which also holds notes, agent memory,
  build_log.md, pending-questions.md, and state/auth/.
• "Lifecycle invariants" is rewritten around state/fleet/. The one-Mac / all-Macs bullets now say
  losing state/fleet/, plus a third bullet: the fleet allowlist neither backs up nor makes
  recoverable the owner data living beside it.
• The acceptance bullet dropped the "rm -rf workspace is recoverable" framing.
• Both remaining 3-space model references retired (count is now 0).
• The placeholder note was itself stale — it justified the placeholder by saying "the prose around it
  calls the deletion survivable", which is no longer true. Rewritten, and it now names the test.

tests/state-sync-allowlist-doc.test.py — and I checked each assertion can actually fail rather
than assuming a green run means coverage:

fixed doc                          6/6 PASS
pre-fix doc (d348786f)             5/6 FAIL   <- survivability claim, 3-space ref, 3 positive controls
doc + `rm -rf /Users/wangchi/...`  deletion check FAIL
  same with ~/…                    caught
  same with $HOME/…                caught

The deletion check needed that separate probe: it passed on the pre-fix doc too, because d348786f
had already fixed that half — so a red/green pair alone would not have told me whether the assertion
was live. <workspace> as a literal placeholder still passes, since it does not resolve.

Three positive controls (cites the 2-space model; scopes the invariant to state/fleet/; states the
workspace is not disposable) mean the file cannot pass by deleting the prose instead of correcting
it — same shape as tests/workspace-env-var-not-honored-docs.test.py.

Also run: tests/workspace-env-var-not-honored-docs.test.py (passes), git diff --check (clean),
and scripts/review-checks.sh over the staged diff — PASS (hardcoded-paths clean). That last one
I verified with a positive control first, because an unpiped invocation exits 0 with
empty diff — nothing to check, which reads identical to a clean result:

$ printf '...+CACHE = "/Users/wangchi/stando-ui/sutando/state"' | bash scripts/review-checks.sh
review-checks: FAIL — hardcoded-paths: ...
$ git diff --cached | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)

[qingyun-wu]

Approved exact head 5f49392ff2b0956fc2e0cb31aedceb9addc5f37e.

No blocking findings. The current wording no longer claims $SUTANDO_WORKSPACE is honored, keeps migration scanning separate from resolution, and scopes the state-sync survivability claim to rebuildable state/ subpaths and state/fleet/ instead of the canonical workspace. Hosted checks are green.

Verification:

• python3 tests/workspace-env-var-not-honored-docs.test.py
• python3 tests/state-sync-allowlist-doc.test.py
• env PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2491-pycache python3 -m py_compile src/workspace_default.py tests/workspace-env-var-not-honored-docs.test.py tests/state-sync-allowlist-doc.test.py
• git diff --check origin/main...HEAD
• bash scripts/review-checks.sh --diff /private/tmp/sutando-pr2491.diff

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Approved exact head 5f49392ff2b0956fc2e0cb31aedceb9addc5f37e; the workspace-lifecycle blocker from my prior review is resolved.

The repaired document now uses the current two-space model, limits survivability to rebuildable state/ subpaths and state/fleet/, explicitly states that deleting the canonical workspace destroys owner data, removes the retired three-space framing, and no longer presents a runnable deletion command. Reviewer-run exact-head guards pass 6/6 for the allowlist document and 9/9 for the removed env-var contract; compilation, diff hygiene, REVIEW.md path scanning, and every hosted CI/static/coverage/CLA gate are green. Qingyun independently approved this exact head.

Worst-case disruption was a user following authoritative prose and deleting notes, agent memory, auth identity, or other durable workspace state. The negative guards plus positive controls now fail if the dangerous claim returns or if corrective prose is merely deleted.

Code/docs-ready at this head, but not merge-ready while GitHub reports BEHIND: refresh onto current main, rerun all gates, and confirm exact-head approvals remain current before merge. No bypass.

[qingyun-wu]

Approved exact head a1d3219d458cddc71da4273437820b99dca84968.

No blocking findings. I re-reviewed the current diff after the head moved from 5f49392; the workspace-resolution wording now consistently says $SUTANDO_WORKSPACE is not honored while keeping migration scanning separate, and the state-sync allowlist doc no longer implies the canonical workspace is disposable or presents a runnable destructive workspace deletion command.

Focused checks run:

• git diff --check origin/main...HEAD
• python3 tests/state-sync-allowlist-doc.test.py
• python3 tests/workspace-env-var-not-honored-docs.test.py
• python3 -m py_compile tests/state-sync-allowlist-doc.test.py tests/workspace-env-var-not-honored-docs.test.py src/workspace_default.py
• bash scripts/review-checks.sh --diff /private/tmp/sutando-pr2491-a1d.diff

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed exact refreshed head cd5a1d678ba821f16bff5642e0efe42507f94532.

No blocking findings. The refreshed docs still avoid the two dangerous old claims: $SUTANDO_WORKSPACE is documented as not honored for resolution, and the state-sync allowlist no longer implies the canonical workspace is disposable or gives a runnable workspace deletion example.

Focused verification passed: tests/workspace-env-var-not-honored-docs.test.py, tests/state-sync-allowlist-doc.test.py, Python compilation for the touched Python/test files, git diff --check origin/main...HEAD, and scripts/review-checks.sh over the PR diff. Hosted checks are green on this head.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

APPROVE on exact head 787c5049. This is a current-main merge refresh; the reviewed documentation topic is unchanged at stable patch ID 950bd3c66a.

Reviewer-run current-head evidence: the state-sync allowlist doc regression passes all seven checks, the removed-workspace-env documentation guard passes all nine checks, schema parse, diff hygiene, and the REVIEW.md path scan pass. All required hosted checks and CLA are green.

Worst-case disruption remains operators treating the canonical workspace as disposable or believing the retired env override still controls resolution; the focused doc guards mitigate both. Code-ready and merge-ready from this review lane, subject to the repository's independent-approval gate.

[qingyun-wu]

Approved exact refreshed head c90be53. This is a topic-preserving merge of current main at bf05035; stable topic patch ID 950bd3c66a4edf1a2babde2c01d21ceb9ec1f244 matches the previously reviewed patch. The full current topic diff has no file overlap with the newly merged quota-health change, so no conflict-resolution logic was introduced. Exact-head git diff hygiene and the REVIEW.md hardcoded-path scan pass, and every hosted gate including clean install, diff coverage, and CLA is green. The previously exercised failure mode remains operators relying on a deprecated workspace environment variable that is ignored; this refresh does not widen its blast radius or add state migration, config requirements, or auto-action behavior. Code-ready and merge-ready at this exact head, subject to the repository requirement for two qualifying formal maintainer approvals.

[github-actions]

@cla-assistant check