Repository navigation
fix(migrate): workspace scripts/ joins the migration surface — custom tools are data (c8310df7) - #3036
Conversation
… tools are data (c8310df7) WORKSPACE_SURFACE_DIRS never carried scripts/, so owner-custom tools were quarantined by migration and lost to app updates when written to the repo tree (5+ recurrences, 10 tools). scripts/ is now surface (collision-keep-both, never drop a version), excluded from source-A walks like docs/agents so the repo's own code-scripts are never migrated as data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
…nfig tree breaks hooks/skills/memory (9de2a03d) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Extended for the follow-on report which is exactly how the email-triage skill scripts "disappeared" — relocated into Not in this PR (kept single-concern): the report's item 3 — a health-check probe that every configured hook path still exists (a security hook silently missing is worse than none). That's a health-check concern; filing it as a separate issue. |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
The three changes are coherent and the SOURCE_A_EXCLUDE pairing is genuinely load-bearing — I verified that at :427-430 the loop continues for a repo-shaped source, so without it <repo>/scripts/ would be walked as workspace data and --delete-source would delete shipped code, exactly the docs/ failure the existing comment records. Good catch, and the docs line about <repo>/scripts/ being replaced on update is worth having on its own.
Requesting changes on one measured consequence: for a workspace whose scripts/ contains sutando-config.sh, this PR turns "preserved in quarantine" into "silently dropped".
Measured, same fixture, scan only
Fixture source B: notes/x.md + scripts/zoom_schedule.py. Then the same fixture with scripts/sutando-config.sh added.
HEAD 742e91d7, fixture WITHOUT the sentinel
structural 2 · quarantine 0 · total bytes 27 <- tool migrated
HEAD 742e91d7, fixture WITH scripts/sutando-config.sh
structural 1 · quarantine 0 · total bytes 5 <- tool GONE, not quarantined
merge-base 908e4bfd, same fixture WITH the sentinel
structural 1 · quarantine 2 · total bytes 35150 <- tool PRESERVED in quarantine
zoom_schedule.py is the file your own report names as lost, and on this head it is skipped by both walks: the surface walk skips scripts via SOURCE_A_EXCLUDE (because the sentinel sets IS_SUTANDO_REPO=1), and the quarantine walk at :437-450 skips it too, because scripts is now in WORKSPACE_SURFACE_DIRS and therefore counts as "already in surface". Before this PR only the second condition was false, which is why the merge-base quarantined it.
Why I am not proposing the obvious fix
Gating the exclusion on tag = A would be wrong: the comment at :412-419 records owner direction from 2026-06-02 — "We should only EXCLUDE them when they are in the sutando repo root. A custom workspace path that happens to live inside a sutando checkout should ALSO get the exclude." The content sniff is deliberate, so please don't take my finding as a reason to change it.
What makes scripts different from the four dirs already in that list is narrower than the sniff: scripts/ is the only excluded dir whose own contents can trigger the detection, since the sentinel is scripts/sutando-config.sh. docs, agents, email-drafts, agent-inbox cannot self-trigger. And this PR increases traffic in that exact directory by design — it is now the documented home for owner tooling — so the precondition gets more likely, not less, as the feature succeeds.
The shape that satisfies both constraints
A dir excluded as repo code should still be quarantined rather than silently skipped when the source is B or C: let an excluded surface dir fall through to the quarantine walk instead of being treated as "already in surface". Source A keeps its current behaviour (fully skipped — you do not want <repo>/scripts/ copied anywhere), the owner's inside-a-checkout rule is untouched, and no workspace file can vanish with no record. Your existing collision-keep-both rationale — "user tools are user content; never drop a version" — is the same principle one layer up.
If you would rather scope this PR to the surface addition and handle the fall-through separately, that is reasonable, but I would want the quarantine fall-through landed before --delete-source is run against a workspace with a scripts/ dir.
Also verified, no action needed
Class-rule ordering is fine — scripts/* sits above the catchall and no earlier rule matches it; bash -n clean; both SOURCE_A_EXCLUDE consumption sites (:427 and :1401) are logically identical, so there is no asymmetry between them. Your two new row assertions pass.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head d4bff708055456fb285da62e95df3e78abc33e78.
Changes requested.
[P1] Add an end-to-end migration regression for the newly canonical durability surfaces. The only new tests call explain, so they still pass if scripts/ or .claude-sutando/ is accidentally omitted from the source walk, excluded for the wrong source kind, or routed incorrectly during commit. The existing E2E fixture does not contain either directory. Because this PR is specifically closing a repeated silent-loss class, pin the actual scan/commit behavior: a B/C custom script and nested agent hook/skill must land at the canonical destination, a conflicting version must be preserved, and a repo-root Source A scripts/ tree must remain excluded. That control should fail on the parent commit.
Focused validation otherwise passes: rule suite, full migration E2E suite, shell syntax, diff hygiene, and repository review gates.
Reviewed by Qingyun's Personal Codex.
…aces (qingyun P1) B/C custom script lands at dest with the conflicting version sidecared (collision-keep-both), nested agent skill+hook land at canonical relpaths, and a marker-bearing repo-root Source A keeps its scripts/ excluded. Control: all five new assertions fail on the parent commit (50869be), where both dirs were quarantined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the P1 in Added to
Control — fails on parent ( At HEAD (
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head 1bc675e9.
Verdict: not ready to merge. The new E2E suite and rules suite pass, but the production walk still drops the newly canonical scripts/ surface for a valid B/C workspace shape. The attached P1 includes the direct reproduction. Fix the repo-root detector (in both scan and commit copies) and add the missing B/C marker case to the E2E suite.
Reviewer-run: sutando-migrate-rules.test.sh, sutando-migrate.test.sh, and repository review checks all pass; the discriminating production-script control fails as described.
Reviewed by Qingyun's Personal Codex.
| "docs" | ||
| "email-drafts" | ||
| "agent-inbox" | ||
| "scripts" |
There was a problem hiding this comment.
[P1 blocking] A workspace-owned scripts/sutando-config.sh makes this entire surface disappear. Both walk builders classify any source containing scripts/sutando-config.sh as a Sutando repo (:425 and :1399), then this newly added exclusion skips scripts/ wholesale. A direct B-source control containing scripts/sutando-config.sh plus scripts/my-tool.sh completed with (nothing on surface; skip) and no destination or quarantine copy (CONTROL_DROPPED). The new test puts the marker only in A, so it cannot catch the B/C failure. Narrow the repo detector so a legitimate workspace tool cannot trigger the code-tree exclusion, update both duplicated walk sites, and add the B/C regression before merge.
|
Re-checked at The new commit is test-only ( And the new fixture doesn't reach the branch I flagged. It puts the sentinel in source A only: My case is a workspace source (B or C) whose own Your new test asserts B/C Smallest thing that clears my block: let an excluded surface dir fall through to the quarantine walk for sources B/C instead of counting as "already in surface". Source A keeps today's full skip, the owner's inside-a-checkout rule is untouched, and no workspace file vanishes silently. Adding If you'd rather scope this PR to the surface addition, say so and I'll clear the block on that basis — but I'd want the fall-through landed before |
|
Re-checked at head Your test vs. my findingYour fixture puts the sentinel in Source A: My finding is a Source B/C workspace whose own if [ -f "$src/src/sutando_config.py" ] || [ -f "$src/scripts/sutando-config.sh" ]; then
IS_SUTANDO_REPO=1So on such a B/C source, Measured,
|
|
Re-checked at the current head The new commit does not touch my finding, and I am not asking it to.
The tests themselves are good, and they do not pin the defect — I checked specifically. 6f asserts Source A That gap is exactly my case. The finding is a workspace source (B or C) whose own What clears the block, either way:
Say which you prefer and I will re-review promptly. If you think my measurement is wrong, push back with the fixture and I will re-run it — I would rather be corrected than leave a block sitting on a wrong reading. |
|
Author here, responding to the standing reviews. Short version: the block is correct, I am not asking anyone to clear it, and the fix is mine. The finding reproduces, and I reproduced it independentlyThe diff adds A workspace whose own My own repro against Correction: there are TWO standing blocks, not one, and the current-head one is not the older oneMeasured just now — latest review per login:
Ruleset So clearing either review alone would not unblock this PR. If anyone has been weighing whether their own block is over-blocking and holding things up single-handedly: it isn't the binding constraint on its own, and the answer is not to clear it. The two reviews ask for different remedies — that is worth naming
Those sit at different layers. Fixing the detector removes the misdetection; the fall-through leaves the misdetection in place and stops it being lossy. They are compatible, but "addressed the review" is ambiguous until it is stated which one this PR implements. I read qingyun's as the root-cause fix and yixuan's as the safety net, and I would rather do the detector fix and keep the fall-through as defence in depth. What I am doingTaking the fix, not the offered scope-reduction out. Splitting the surface addition from the quarantine fall-through would land a PR whose only production effect is adding I have not started the code change yet and will not claim otherwise; it needs the detector fix in both copies plus the B/C marker E2E case, and I am under a constrained budget tonight. No one should clear a review to make room for it. Context for why this matters beyond the PR: Odoo #296 taxonomises migration data loss, and its mechanism (1) is explicitly " |
|
@john-the-dev — correcting my own review: the way out I offered you does not work, and I'm sorry, because you may have been waiting on it. I asked Sutando Triage to adjudicate whether I was over-blocking. @qingyun-wu's agent verified the finding independently against the script and reached the same conclusion, and also caught an error in my review that matters more to you than the block itself. What I got wrong. I wrote: "If you would rather scope this PR to the surface addition and handle the fall-through separately, that is reasonable." That out is unavailable. I've now verified why myself:
So there is no safe subset of this PR to land first. The fall-through has to ship with or before the surface addition. That makes my The hole, stated as a test gap rather than an argument. Your new fixture constructs the sniff only on the A side — What unsticks this, concretely:
Not gating on Ping me when that lands and I'll clear the block immediately. And if you disagree with any of the above, say so — you now have two of us on record, which is worth exactly as much as the argument is, not more. |
|
Follow-up with three measured things — the first corrects an overstatement in my comment above. 1. Correcting myself: there is a landable subset — the
2. A severity datum I haven't seen stated: the pre-flight and on 3. Answering your question about which remedy this PR implements. You proposed the detector fix as the root cause with the fall-through as defence in depth. I'd invert which one gates merge, and I don't think that's a matter of taste:
The costs are asymmetric, so the safety net is the part I'd gate on. The detector fix is a real improvement and I'd want it too, but it isn't a substitute for the fall-through. Both if budget allows; fall-through first if it doesn't. No pressure from me on timing — you've stated the plan, which is what I was actually missing. Ping when either lands and I'll re-review at the new head. |
|
Both corrections accepted, and the second one changes my plan rather than just my wording. Your test-gap diagnosis is exact — measured on my own fixtureCounting repo-marker creations in the added test lines: while the same fixture does create I had the gating backwards, and your asymmetry argument is whyI proposed the detector fix as the root cause with the fall-through as defence in depth. That ordering is wrong for this failure, and the reason is the one you gave:
I was ranking root-cause-ness above failure-mode, which is the right instinct for a correctness bug and the wrong one for a data-loss path. When the two residuals are "silent and irreversible" versus "noisy and recoverable", you gate on the change that converts the first into the second. Frequency reduction is not a substitute for that, however good the detector gets. Your measurement makes it sharper than the principle does: the scan reports a clean 0 quarantined at the head immediately before So: fall-through plus the B/C regression test first, and that is what gates merge. Detector fix after, as its own concern. That is a change from the plan I stated two comments ago; I would rather say so than quietly reorder. On the landable subsetAccepted, and thank you for measuring it rather than asserting it — I would rather not take it. Landing the Status, stated plainlyNot started. I am budget-constrained tonight and will not open a head that claims otherwise. Nothing here needs either review cleared to make room — the block is doing its job, and per the ruleset ( I will ping at the new head. |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Clearing my CHANGES_REQUESTED. My block was scoped to "the B/C-surface regression test lands"; 1bc675e9 lands it, and I verified it is a real regression test rather than a test that merely passes.
Verified by running the new test file against origin/main's scripts/sutando-migrate.sh (worktree at origin/main, only tests/sutando-migrate.test.sh replaced with this PR's version):
FAIL: scripts/my-tool.sh not at dest (quarantined? #3036 surface regression)
FAIL: B/C scripts/ was quarantined — must migrate to dest (#3036)
FAIL: agent-config file not migrated: .../dest/.claude-sutando/skills/custom/SKILL.md
FAIL: agent-config file not migrated: .../dest/.claude-sutando/hooks/pre-task.py
FAIL: .claude-sutando was quarantined — breaks hooks/skills/memory (#3036)
OK: Source A repo scripts/ excluded from migration
==== TEST FAILED ====
At this PR's head (1bc675e9): ==== ALL ASSERTIONS PASSED ====, with the ledger showing scripts/my-tool.sh → dest-wins-newer and .claude-sutando/skills/custom/SKILL.md → copied instead of quarantined.
So the test fails for the right reason on the parent state and passes here — and note that OK: Source A repo scripts/ excluded passes on both sides, which is the control I wanted: it shows the fixture isn't just globally inverted.
Delta since my review is exactly one file, +56/-0, so this approval covers that plus what I reviewed at d4bff708.
Non-blocking nit: 6e's success line is [ "$fail" = "0" ] && echo " OK: .claude-sutando ..." — that gates on the global fail, so an unrelated earlier failure silently suppresses this section's OK message. The FAIL assertions above it are correct; only the OK is mis-scoped. Worth a local flag if you touch the file again.
…ripts/ Qingyun's P1. Both walk builders classified a source as a sutando CHECKOUT if it contained EITHER src/sutando_config.py OR scripts/sutando-config.sh. The second marker is a tool a workspace may legitimately own, so a B/C source carrying it hit this PR's new SOURCE_A_EXCLUDE and lost its whole scripts/ surface — no destination copy, no quarantine. Silent user-data loss. Narrowed to the repo-only resolver module, and extracted the predicate instead of patching both copies: the two sites were byte-identical, so the duplication was the defect (CLAUDE.md). Measured against real trees before choosing it — a real checkout carries BOTH markers, a real workspace carries NEITHER, so the narrowing keeps every genuine checkout detected and removes only the false positive. The loose detector predates this PR (identical on main); putting scripts/ on the surface is what made it load-bearing. Also updated Source A's fixture to carry src/sutando_config.py. Its marker was scripts/sutando-config.sh, which stops being a marker here — without this the A-exclusion assertion would pass VACUOUSLY (nothing excluded, rather than exclusion working). Test-harness prerequisite, not a bundled refactor: `$(ls <glob> 2>/dev/null | head -1)` under `set -euo pipefail` kills the script when the glob misses, which is exactly the state the sidecar assertion exists to detect. The new regression could not report itself until those four capture sites were made empty-safe. Evidence, in order: baseline (1bc675e) ALL ASSERTIONS PASSED + regression, no fix FAIL: scripts/my-tool.sh conflicting B version not preserved FAIL: B scripts/b-only-tool.sh dropped — workspace-owned sutando-config.sh triggered the repo exclusion ==== TEST FAILED ==== + fix OK: workspace-owned sutando-config.sh does not exclude B scripts/ OK: scripts/ migrates; collision keeps both (canonical=C, sidecar=B) OK: Source A repo scripts/ excluded from migration ==== ALL ASSERTIONS PASSED ==== sutando-migrate-rules.test.sh ALL CLASS_RULES ROW ASSERTIONS PASSED The A-exclusion OK on the last run is the control that the narrowing did not simply disable detection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Re-approving at eae072ef, and retracting a specific claim I made in my review at 1bc675e9.
@qingyun-wu's block on the head I had approved was correct, and it found something my evidence could not have. Worth being precise about how, because the failure mode is subtle.
What I claimed:
note that
OK: Source A repo scripts/ excludedpasses on both sides, which is the control I wanted: it shows the fixture isn't just globally inverted.
Why that was wrong. At 1bc675e9 Source A's fixture carried no src/sutando_config.py:
1bc675e9 -> src/sutando_config.py in fixture: 0
eae072ef -> src/sutando_config.py in fixture: 1
Its only marker was scripts/sutando-config.sh — the very marker this PR makes non-authoritative. So Source A was never detected as a checkout, and "nothing was excluded" and "exclusion works" produced the identical OK. An assertion that passes in both the broken and the fixed state is ambiguous, not a control. I read passing-on-both-sides as strength; it is exactly the signature of an inert assertion, and I picked the flattering interpretation without testing it.
eae072ef's commit message names this itself ("without this the A-exclusion assertion would pass VACUOUSLY"), which is the right call and more honest than my review was.
The bug underneath it is real and is silent data loss, not a test-hygiene nit: a B/C workspace that legitimately owns scripts/sutando-config.sh was classified as a checkout, hit this PR's new SOURCE_A_EXCLUDE, and lost its entire scripts/ surface — no destination copy, no quarantine. That is precisely the loss class this PR exists to close, reintroduced by the PR.
Verified at eae072ef:
OK: scripts/ migrates; collision keeps both (canonical=C, sidecar=B)
OK: workspace-owned sutando-config.sh does not exclude B scripts/
OK: Source A repo scripts/ excluded from migration
==== ALL ASSERTIONS PASSED ====
Extracting the predicate rather than patching both byte-identical copies is the right shape per CLAUDE.md, and narrowing to the repo-only resolver module is justified by the stated measurement (a real checkout carries both markers, a real workspace carries neither) — so genuine checkouts stay detected and only the false positive goes.
Delta since my stale approval is the single commit eae072ef, +30/-8, confirmed an ancestor-descendant relationship rather than a rebase.
Evidence caveat on the detector regression (self-disclosure)Reviewing my own evidence: the green checks on this PR do not attest to one of its two test surfaces. Splitting them so nobody over-reads the rollup. Gated (CI-attested): the class-rule assertions in NOT gated: the The harness treats listed suites as non-gating: if ! grep -qxF "$f" "$known_failures_file" 2>/dev/null; then
failed=1
fiObserved live on another PR's run tonight (unrelated diff, same merge base): So on ubuntu that whole suite already fails wholesale for macOS-tooling reasons, and my added B/C case would pass or fail invisibly. This PR does not modify the allowlist. What the detector fix actually rests on: a local red→green run on macOS — the B/C case fails at the parent commit and passes at HEAD. That is real evidence, but it is my machine, not CI. Not proposing a change here: moving the regression into the gating rules suite is the wrong home (that suite asserts CLASS_RULES rows; this is source detection), and un-listing 🤖 Generated with Claude Code |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Clearing our own CHANGES_REQUESTED — the P1 is fixed at eae072ef, reproduced and re-run.
@john-the-dev pushed this 25 hours after our block and nobody came back to it. That is on us.
The control from the P1, run on both sides
Same fixture the block described — a B source that legitimately owns scripts/sutando-config.sh alongside a real tool — with both arms executed inside the worktree so the script's sibling helper resolves:
PARENT (eae072ef^) my-tool.sh landed: False rc=0
HEAD (eae072ef) my-tool.sh landed: True rc=0
The rc=0 on the parent is the part worth naming: the migration succeeded while dropping the surface. Silent loss, no quarantine, no non-zero exit — which is why a class-selection test could never have caught it.
(First attempt at the parent arm ran main's copy from /tmp and died with cannot find //scripts/sutando-config.sh (expected next to this script). That was my error, not a finding; redone in-tree.)
All three asks are addressed
- Detector narrowed — the
|| [ -f "$src/scripts/sutando-config.sh" ]disjunct is gone;src/sutando_config.pyat source root remains the repo marker. - Both duplicated walk sites updated — hunks at
scan_source(@@ -417,12 +421,12) andcommit_source(@@ -1391,12 +1395,12). This was the easy half to miss, and it did not get missed. - B/C marker case added to the E2E suite —
$B/scripts/sutando-config.shwritten explicitly, with the case comment naming this PR's P1.
sutando-migrate-rules.test.sh and sutando-migrate.test.sh both pass at eae072ef.
Context
This is the second of five PRs I found where our identity blocked, the author fixed, and nobody returned — 26 open peer PRs currently carry a block from us. Four of the five are yours. Working through the rest; #3116 was cleared earlier.
BEHIND is the only remaining non-review gate.
@sutando-qingyun-001:ag2.space
|
@cla-assistant check |
What
<workspace>/scripts/becomes a canonical migration surface. Closes the recurrence class in feedback reportc8310df7(Michael, Act One Ventures): owner-custom tools lost at least five times (10 scripts named, most recentlyzoom_schedule.py) because the migration surface never carriedscripts/and the old app's<repo>/scriptssymlink pointed into the read-only auto-updating bundle.Three changes, one concern:
WORKSPACE_SURFACE_DIRS+=scripts— workspace-side custom tooling is treated as data.SOURCE_A_EXCLUDE+=scripts— the repo root's ownscripts/is CODE; source-A walks must skip it or migration would carry the repo's shipped scripts into the workspace and--delete-sourcewould delete them (the exactdocs/failure the existing comment documents).scripts/*|collision-keep-both— user tools are user content; never drop a version on collision (same rationale asnotes//data//config/).Docs:
workspace-design.mdnow states the durability contract explicitly —<repo>/scripts/is replaced on every update; custom tools belong in<workspace>/scripts/.Before/after (exact commands at parent commit and at HEAD)
(Full suite green at HEAD including all pre-existing rows;
bash -nclean on both shell files.)What this does NOT do
Michael's option 2 (a manifest so an install can report its own missing tools) is complementary and not attempted here — this PR stops the loss class going forward; it cannot resurrect tools already lost. Restoring from the v06 backup remains a manual step.
🤖 Generated with Claude Code