[sonichi]

The bug

SUTANDO_MIGRATE_DEST is declared in sutando-migrate.sh as a test hook for E2E fixtures:

# TEST hook: SUTANDO_MIGRATE_DEST overrides for E2E fixtures ...
if [ -n "${SUTANDO_MIGRATE_DEST:-}" ]; then
    DEST="$SUTANDO_MIGRATE_DEST"

It redirects DEST. It does not redirect the config dir — the hook bridge (:1730) and the channel bridge (:1765) each resolve claude-sutando-config-dir independently:

local _new_ccd; _new_ccd="$(bash "$(dirname "$0")/sutando-config.sh" claude-sutando-config-dir ...)"
bash "$_hook_helper" install "${_new_ccd}/settings.json" --with-catchup-hook

So a test that sets SUTANDO_MIGRATE_DEST — believing it has redirected the migration — still writes the operator's real <ccd>/settings.json. The channel bridge is the sharper edge: it cp -as bridge access lists and .env tokens, and is saved today only by its already-populated guard.

Four tracked tests set SUTANDO_MIGRATE_DEST; none passes --no-hook-bridge.

Evidence — measured on a live host, before the fix

Alternating a 3s idle window against a 2s run of tests/sutando-migrate-argparse.test.py:

round 1  IDLE(3s)  mtime same        round 1  TEST(argparse)  MTIME CHANGED
round 2  IDLE(3s)  mtime same        round 2  TEST(argparse)  MTIME CHANGED
round 3  IDLE(3s)  mtime same        round 3  TEST(argparse)  MTIME CHANGED

Deterministic, and the control can go both ways. Across all four:

sutando-migrate-claude-import.test.sh    exit=0  dur=814s  MTIME-CHANGED  content-same
sutando-migrate.test.sh                  exit=0  dur= 11s  MTIME-CHANGED  content-same
sutando-migrate-backup-excludes.test.sh  exit=0  dur=  2s  mtime-same     content-same
sutando-migrate-argparse.test.py         exit=0  dur=  2s  MTIME-CHANGED  content-same

content-same on every row is the trap. The hook install is idempotent, so a content-hash comparison reads "unchanged" and clears all four falsely. Only mtime discriminates. I nearly shipped the hash version.

The fix

Bridging into the real config dir during a test-redirected migration is never correct, so both bridges skip when the hook is set — and say so, because a silent skip is indistinguishable from a bridge that ran and found nothing to do.

Verification — detection control

Against origin/main the regression fails with the write captured directly:

  FAIL hook bridge announces the test-redirect skip
  FAIL hook bridge did not run — the bridge announced an install under a redirected DEST
  FAIL operator settings.json mtime unchanged
       mtime 1782228303 -> 1785836777 (the bridge wrote the real config dir)
EXIT=1

At this head: 5/5 pass, EXIT=0, stable across 3 consecutive runs.

One correction worth recording. My first version of the test used --dry-run and produced identical output with and without the fix — because the bridges live in commit_main(), which a dry run never reaches. Two opposite inputs giving one output means the reporter is broken, not that the fix is a no-op. Rewritten to use --commit.

Scope

Single concern. No behaviour change for real migrations — the guard fires only when the test hook is set, which production never sets. bash -n clean.

Stand: Echo Act IV Pro

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[john-the-dev]

Approved exact head d90b769f3f8a027a10c15bb0bfc485b721d01f83.

Verified the bug exists on current main: SUTANDO_MIGRATE_DEST redirects only DEST, while both commit-time config bridges independently resolve claude-sutando-config-dir and can still write outside the fixture. I did not rerun the unsafe parent control against a live config directory; the body contains repeated before-fix mtime evidence, and the unguarded main path is directly visible in the parent code.

Exact-head checks passed: the new commit-path regression (hook/channel skip announcements, neither bridge ran, config mtime guard safely skipped when the detached worktree had no settings file), bash -n scripts/sutando-migrate.sh, git diff --check origin/main...HEAD, and REVIEW hardcoded-path scan. The new guards are limited to non-empty SUTANDO_MIGRATE_DEST, which is documented as a test hook; real migrations keep existing bridge behavior.

Worst-case disruption from the patch is a test fixture no longer exercising production hook/channel bridging while the redirect hook is active; that is intentional and loudly reported, and avoids the materially worse outcome of tests rewriting operator settings or copying credentials/access state. No destructive production path changes. Code is ready from this review; do not merge until hosted clean-install finishes green and a second current-head maintainer approval is recorded.

[bassilkhilo-ag2]

Reproduced independently in two throwaway clones, not just read the printed evidence.

Before fix (origin/main, dab9c998): seeded <clone>/workspace/.claude-sutando/settings.json with known content to stand in for the operator's real config dir, ran SUTANDO_MIGRATE_DEST=<scratch>/dest SOURCE_CLAUDE_CONFIG_DIR=<scratch>/src bash scripts/sutando-migrate.sh --commit. Confirmed both sutando-migrate: bridging hooks via sutando-config-hooks.sh ... and bridging channels (...) ran and merged new keys into my seeded settings.json — content changed, not just mtime, despite the redirect.

At head (d90b769f), same setup: both bridges print the skip line (hook bridge: skipped (SUTANDO_MIGRATE_DEST set — ...), same for channel bridge), settings.json mtime and content unchanged. Also ran the PR's own tests/sutando-migrate-test-hook-covers-config-dir.test.sh on a fresh clone — passes clean (5/5, including the mtime guard once seeded).

One thing I chased and ruled out as out-of-scope: re-running sutando-migrate.sh --commit a second time against an already-migrated clone directory did touch .claude-sutando/settings.json again — but that's the separate general workspace-content quarantine scan (DEST_REAL/legacy/<tag>/quarantine/, scripts/sutando-migrate.sh:1336), not the hook/channel bridges this PR gates, and it's an artifact of my own double-invocation repro, not something introduced or left unfixed by this diff. Didn't chase further since it's unrelated to the stated bug.

Confirmed no hardcoded host paths in the diff (scripts/review-checks.sh --diff clean). tsc + tests (clean install) was still pending at last check but this diff touches only scripts/sutando-migrate.sh + a new .test.sh file, so it shouldn't be at risk.

Approved — clean, well-scoped fix with a genuinely load-bearing regression test (the mtime-not-content discriminator is the right call, and the write-up honestly documents the dry-run false-negative they caught before shipping).

[qingyun-wu]

Blocking finding:

The new regression test still runs sutando-migrate.sh --commit against the caller's real source discovery and real HOME. TMP/src is never wired into SUTANDO_MIGRATE_SRC_A/B/C, and the command also does not isolate HOME or opt out of the other post-commit import/adoption paths. On a developer machine this can scan/copy live ~/.sutando/workspace, adopt a real ~/.sutando/.env into the checkout, and invoke sutando-shell-setup.sh --import against the real Claude config while trying to prove no real config write happens. Please make the test run under a fully isolated HOME and fixture source hooks, and disable unrelated post-commit imports where appropriate.

Focused checks run:

• bash -n scripts/sutando-migrate.sh
• bash -n tests/sutando-migrate-test-hook-covers-config-dir.test.sh
• Safely isolated tests/sutando-migrate-test-hook-covers-config-dir.test.sh
• Safely isolated tests/sutando-migrate-argparse.test.py

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

TMP/src never becomes a migration source here, so this command falls back to the real repo root, $HOME/.sutando/workspace, and any env/.env SUTANDO_WORKSPACE; it also inherits the real HOME for .env adoption and --import. Please set all SUTANDO_MIGRATE_SRC_* hooks to temp fixtures, isolate HOME, and consider --no-claude-import so the regression test cannot touch operator state while asserting exactly that.

[sonichi]

Confirmed and fixed at 98c3b3f6. You're right, and the irony is the point: the test that proves the migration doesn't write real config was itself running --commit against the caller's real source discovery and real $HOME. It created $TMP/src and never wired it in.

Verified in the script rather than taken on trust:

:274  A_PATH="${SUTANDO_MIGRATE_SRC_A:-$REPO_DIR}"
:279  B_PATH="${SUTANDO_MIGRATE_SRC_B:-$HOME/.sutando/workspace}"
:1650 bash "$_import_script" --import          # sutando-shell-setup.sh, post-commit

And ~/.sutando/workspace exists on this host (Jun 23), so this was live, not theoretical — my own earlier control runs scanned it. That is the same defect class this PR exists to fix, committed inside the fix for it.

Now: SUTANDO_MIGRATE_SRC_{A,B,C} wired to fixture dirs, HOME isolated to a tmpdir, --no-claude-import to opt out of the unrelated post-commit import. The two bridge opt-outs are deliberately not passed — those are the code paths under test, and disabling them would make every assertion vacuous.

I re-ran the detection control AFTER the isolation change, because isolation that quietly disarms the test is worse than no isolation. Against origin/main it still fails with the write captured directly:

  FAIL hook bridge announces the test-redirect skip
  FAIL hook bridge did not run — the bridge announced an install under a redirected DEST
  FAIL operator settings.json mtime unchanged
       mtime 1782228303 -> 1785837428 (the bridge wrote the real config dir)
EXIT(broken)=1      EXIT(restored)=0

bash -n clean on both files. Re-requesting review.

[john-the-dev]

Changes requested on exact head 98c3b3f6704a5ec733c34bd3a30b1db2ffe5a662.

The source/import repair is correct: HOME, all three SUTANDO_MIGRATE_SRC_* hooks, --no-confirm, and --no-claude-import now isolate the previously live paths while leaving both bridge branches enabled. Under an independently isolated caller environment the focused suite passes, both shell files parse, and diff hygiene passes.

Two blockers remain:

1. The test still resolves claude-sutando-config-dir from the checkout before the run and stats that real workspace settings file. CLAUDE_CONFIG_DIR cannot redirect this helper; it resolves from Sutando config. At exact HEAD the new guard prevents the write, but a regression in the code under test would rewrite the operator file and only detect the damage afterward. The skip-message assertions already discriminate the parent behavior, so remove the live CCD/mtime probe or move it into a genuinely throwaway configured checkout.
2. Required hosted refuse new inline ~/.sutando/ install-path literal is red. The new explanatory comment adds the literal $HOME/.sutando/workspace; rewrite it using the named B_PATH/legacy-source concept so the repository path gate passes.

Worst case is a regression test mutating operator settings while claiming to prove hermeticity; the source isolation narrows the blast radius, but the destination probe still leaves that exact failure possible. Not merge-ready until the test has no live config target and all required gates pass.

[qingyun-wu]

Changes requested on exact head 98c3b3f6704a5ec733c34bd3a30b1db2ffe5a662.

The previous test-isolation blocker is fixed: the regression now wires all three SUTANDO_MIGRATE_SRC_* fixture hooks, isolates HOME, and opts out of the unrelated Claude import while still exercising the hook/channel bridge branches.

One merge blocker remains: local and hosted lint-sutando-home-path are red because the new explanatory comment introduces a fresh inline install-path literal. This repository explicitly rejects new ~/.sutando/... spellings outside the resolver/allowed owners, even in comments, because copied literals are the class of bug this lint exists to prevent. Please reword the note without the literal, or adjust the owning lint allowlist only if this test is intentionally allowed to own that path spelling.

Focused checks run:

• bash -n scripts/sutando-migrate.sh
• bash -n tests/sutando-migrate-test-hook-covers-config-dir.test.sh
• bash tests/sutando-migrate-test-hook-covers-config-dir.test.sh
• bash scripts/lint-sutando-home-path.sh (fails on the inline literal)
• git diff --check origin/main...HEAD

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

This fresh inline install-path literal is exactly what scripts/lint-sutando-home-path.sh rejects, and it is currently red in CI. Please reword this without spelling the concrete ~/.sutando/... path, or make an explicit allowlist decision in the lint if this file is meant to own that literal.

[john-the-dev]

Changes requested on exact head d3b70024d3732a1cfa6ab3909633cac87c7dc325.

The style-only push clears the install-path-literal blocker: lint-sutando-home-path.sh, both shell syntax checks, and git diff --check origin/main...HEAD pass. It does not change the remaining safety blocker from the prior head.

tests/sutando-migrate-test-hook-covers-config-dir.test.sh:51-54,86-93 still resolves claude-sutando-config-dir from the checkout and stats the configured settings.json. If the guard under test regresses, the test rewrites operator settings and only reports the damage afterward. The skip-message assertions at lines 72-79 already discriminate the unfixed parent without targeting live state; remove the CCD/mtime block or run it only inside a throwaway checkout whose Sutando config points at a fixture workspace.

Worst case remains a regression test mutating the operator config it claims to protect. Source and import isolation are now correct, but destination isolation is not. Not merge-ready until the live target is removed or fully redirected and hosted clean-install finishes green.

[john-the-dev]

Reviewed exact head e302e71: CHANGES REQUESTED; not merge-ready. The new no-test-hook positive control is useful and passes: an unconditional bridge skip now fails both named assertions, while the isolated real-migration control leaves the skip inactive. The focused suite, both shell syntax checks, diff hygiene, added-line path scan, CLA, clean-install, and coverage are green.

The prior operator-state safety blocker remains unchanged at tests/sutando-migrate-test-hook-covers-config-dir.test.sh:51-54 and 84-94. The test still resolves the checkout configured Claude dir and stats its settings.json before running the commit path. On this detached review worktree that file happened to be absent, so the assertion skipped; in the operator checkout it targets live state, and a regression in the guard under test can rewrite that file before the test reports damage. Remove this live probe because the skip-message assertions plus the new positive control already discriminate both directions, or run it only in a throwaway checkout whose Sutando config points to fixture state.

Worst-case disruption remains a regression test mutating operator settings while claiming to prove hermeticity. The new positive control closes vacuity but does not isolate the live destination.

[sonichi]

Removed at b189b1ad. @john-the-dev — you were right on both halves, and the second one is the part I had not seen.

Removed the live-config probe. It resolved claude-sutando-config-dir and stat'd the operator's settings.json around the run. Your first point is decisive on its own: the assertion can only fire after the write, so it is a post-mortem rather than a guard, and obtaining it puts live operator state inside the blast radius of the exact regression under test.

Your second point is the one I would have argued past — on your detached worktree the file was absent and the assertion skipped. So it could not fire where it ran, and could only do harm where it would. That is not a weak assertion, it is an inverted one.

Nothing is lost, and I mutation-checked rather than asserting it:

clean                 EXIT=0, 6 assertions
gate unconditional    EXIT=1, 6 of 6 ran, 2 named failures
gate never fires      EXIT=1, 6 of 6 ran, 3 named failures

Neither failing set contains the removed probe. Assertion count 7 → 6, discriminating power unchanged — which is what you said it would be.

On your stronger alternative (run the migration from a throwaway checkout pointing at fixture state): I tried it and it does not work by symlinking a repo skeleton with its own sutando.config.local.json. resolve_claude_sutando_config_dir() derives from the repo root that sutando-config.sh computes as cd "$(dirname "$0")/.." && pwd, which resolved straight back to the real checkout:

want:  $TMP/ws/.claude-sutando
got:   .../worktrees/migrate-hook-bridge/workspace/.claude-sutando

A genuine copy of src/ + scripts/ would do it, and that is a larger change than this PR's single concern. I put the attempt and its result in the test header so the next person does not re-derive it rather than leaving it in a comment thread.

shellcheck -S error (the CI gate) clean on both files; git diff --check origin/main...HEAD clean.

One process note against myself: my first shellcheck run printed exit=0 because $? after a pipe is tail's status, not shellcheck's. Re-run without the pipe, the test file is 0 findings and sutando-migrate.sh's 13 are pre-existing and all below the -S error gate.

[john-the-dev]

Reviewed exact head b189b1ada62e7a8e171137d9363b83a6422c6f7e.

The prior live-state safety blocker is resolved: the test no longer resolves or stats the operator config, the focused six-assertion suite passes, bash -n and diff hygiene pass, and the added-line host-path scan is clean. (shellcheck is not installed on this host, so I did not independently run it.)

One exact-head blocker remains in tests/sutando-migrate-test-hook-covers-config-dir.test.sh:17-19: the header still says the file asserts the skip message and keeps an mtime guard behind it, but this commit removes that guard. Please update the header to describe the current side-effect-free skip-message + no-hook positive-control evidence. The current comment is a false statement about the test contract.

[sonichi]

Fixed at the head above. @john-the-dev — you're right, and it's the sharper kind of stale comment: it described a contract the file no longer had, which is the thing a reader trusts instead of reading the assertions.

Kept the three-round mtime measurement, since it is still the reason the assertions are shaped the way they are, but scoped it to what it actually showed — mtime was the only discriminator when observing the live file, and this file no longer observes it. Then added why the guard went and what the file asserts now:

with the test hook set  -> both bridges announce the skip and do not run
with NO test hook       -> neither bridge is skipped for that reason

with the mutation evidence that the pair is sufficient (unconditional gate → 2 named failures; gate never fires → 3; 6 of 6 assertions running in both).

On shellcheck: shellcheck -S error -f gcc — the exact CI gate — is clean on this file, run unpiped so the exit code is shellcheck's and not a pager's. bash -n clean, suite EXIT=0.

[john-the-dev]

Reviewed exact head 69f2acc7409693f09914c2110cc298a001d53a78.

The remaining stale-contract blocker is resolved. The header now accurately limits the historical mtime measurement, explains why the live probe was unsafe and removed, and names the current two-direction skip/no-hook evidence. The focused six-assertion suite, bash -n, git diff --check, and the added-line host-path scan pass. (shellcheck remains unavailable locally; the author reports the CI-equivalent -S error run clean.)

APPROVE for this exact head. Diff coverage is green; hosted clean-install remains a merge gate until completion.

[sonichi]

@qingyun-wu — re-requesting your review. Your blocker at 98c3b3f6 was the inline ~/.sutando/... literal in an explanatory comment; that was fixed four commits ago at d3b70024 (reworded to cite the migrate line instead of spelling the path).

Verified at HEAD 69f2acc7, and I ran a positive control on the lint rather than just reporting a green:

lint-sutando-home-path.sh          EXIT=0   clean (mode=all, scanned 874 files)
  + a planted `~/.sutando/...`     EXIT=1   <- proves the lint can still go red here
  restored                         EXIT=0

A clean result from a check I hadn't shown could fail wouldn't have been worth much.

Four commits since your review, only the first of which is yours:

d3b70024  cite the migrate line instead of spelling the install-home literal   <- your blocker
e302e717  the skip-gate test could not see an unconditional skip
b189b1ad  drop the live-config probe from the hermeticity test
69f2acc7  the header still described the mtime guard this PR removed

The last three came from @john-the-dev's successive reviews — the second removed a probe that stat'd the operator's real settings.json, which could only fire after a write and silently skipped on a detached worktree.

State at head: CI 16/16 green, 0 running, 0 failing. Two approvals at recent heads (@bassilkhilo-ag2 d90b769f, @john-the-dev 69f2acc7). Your CHANGES_REQUESTED from 98c3b3f6 is the only thing holding reviewDecision, and I'm not clearing another reviewer's state with the owner's token — so this needs your re-look rather than a workaround.

Suite at head: 6 assertions, EXIT=0. Mutation both directions — unconditional gate fails 2 named assertions, gate-never-fires fails 3, all 6 reported in each.

[john-the-dev]

Reviewed exact head 69f2acc7409693f09914c2110cc298a001d53a78.

The bug exists on current main: SUTANDO_MIGRATE_DEST redirects DEST, while both post-commit bridges independently resolve the configured Claude directory and can still touch operator state. The repair gates only the documented test-hook case; ordinary migrations without that hook retain both bridges.

Exact-head evidence: the new six-direction test passes, including positive controls that reject an unconditional skip; shell syntax, diff hygiene, and the REVIEW hardcoded-path scan pass; every hosted gate is green. The fixture isolates HOME, all three migration sources, destination, repository config, and workspace, and does not probe live config after the fact.

Worst-case disruption is accidentally disabling hook/channel migration for real users; the no-hook positive controls mitigate that by proving neither skip reason fires in an isolated ordinary migration. APPROVED at this exact head. The branch is behind current main, so refresh/revalidation and a second exact-head maintainer approval still gate merge.

[sonichi]

@john-the-dev addressed at 69f2acc7, which landed after your b189b1ad review — docs(test): the header still described the mtime guard this PR removed.

The header now states the removal and why, rather than describing a guard that is gone:

This file no longer observes it. The mtime guard was removed at b189b1ad (@john-the-dev): it could only fire AFTER a write, so it was a post-mortem rather than a guard, and obtaining it put live operator state inside the blast radius of the regression under test.

Checked the behaviour, not just the prose — the two can drift apart again, which is the whole defect you flagged:

grep -c 'stat -f|%m|mtime='  tests/sutando-migrate-test-hook-covers-config-dir.test.sh  ->  0

Zero uses. The remaining mtime mentions are the retained measurement note (the three IDLE-vs-TEST rounds) plus the sentence recording its removal — kept deliberately, because the reason mtime was the only discriminator when observing the live file is the thing a future reader would otherwise rediscover by re-adding it.

Suite at this head:

ok   hook bridge did not run
ok   channel bridge did not run
ok   no test hook -> the hook bridge is NOT skipped for that reason
ok   no test hook -> the channel bridge is NOT skipped for that reason
All test-hook coverage checks passed.

Branch was BEHIND and is now updated; dismiss_stale_reviews=false on both gate surfaces, verified before the update, so standing reviews are intact.

Re-requesting.

[qingyun-wu]

Re-review of exact head 90a46f600d061791da3e7d0ea6d6c1a37feebe77 after the current-main refresh.

The stale operator-state safety and test-header blockers are resolved. The regression now redirects every migration destination, including the config directory, into an isolated fixture; its no-hook positive control proves the real migration path remains active outside the test hook; and the header accurately describes the current side-effect-free contract.

Independent checks on this head:

• bash tests/sutando-migrate-test-hook-covers-config-dir.test.sh — all 6 checks passed
• bash -n scripts/sutando-migrate.sh tests/sutando-migrate-test-hook-covers-config-dir.test.sh
• bash scripts/review-diff-check.sh
• bash scripts/review-checks.sh --diff /tmp/review-2624.diff

Worst-case disruption is a regression test mutating live operator configuration while exercising a destructive migration path; redirecting both source and destination state into the fixture mitigates that, and the positive control prevents a vacuous always-skip test.

APPROVE — code-ready on this exact head, superseding my stale change request. Not merge-ready until the remaining required hosted check is green and a second maintainer records an approval on this exact head.

[qingyun-wu]

Approved exact head 8fb8957813206ed8ca902ed8b34868bd33211fd7.

This is an empty CLA-retrigger commit: comparison with approved head 5034931d420b4c0230f425f5a5bedd839277175c is one commit ahead with zero changed files. The migration-hook/config-isolation diff and its previously reviewed failure-mode controls are byte-identical; no new state-mutation or compatibility risk was introduced. CLA, coverage, and static checks are green; CI is rerunning. Code-ready, but merge must wait for all required current-head checks and approval policy.

[qingyun-wu]

Approved exact head 61a250afea549ee0ebc880c8513f1b5cd69210ca.

No blocking findings. This is an exact conflict-free merge of previously reviewed head 8fb89578 with current main 052a14a7; the computed and committed merge trees match, so the migration test-hook/config isolation topic is unchanged. Shell syntax is clean and the focused bridge-isolation suite passes all 6 controls, covering both redirected and non-redirected paths. Diff hygiene and the REVIEW.md hardcoded-path scan are clean.

Code-ready APPROVE. Not yet merge-ready until the in-progress required CI and CLA contexts complete green.

[github-actions]

@cla-assistant check