feat(cli): add reversible full Trellis ablation - #538
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds ChangesReversible Trellis ablation
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Benchmark update — exploratory single paired run I ran a context-retrieval-focused ablation in a medium-large production TypeScript/Next.js application: about 775 tracked files and 130k tracked text lines. The task fixed a presentation-only provider benefit-title formatter in a browser userscript. The visible fixture exposed one realistic footnote-markup regression; a hidden evaluator covered exact one-pass decoding, narrow terminal-marker removal, preservation of unrelated markup-like text and double encoding, empty-result fallback, inert rendering, and unchanged storage identity.
Both conditions used the same history-free source tree, prompt, model/reasoning setting, sandbox, and 600-second budget; native multi-agent was disabled, runs were sequential, and there were no reruns or human interventions. The treatment restored exactly afterward, and the contamination audit found no Git-history, outside-repository, or agent-initiated network reads. Narrow interpretation: in this test, Trellis delivered project-specific edge-case policy that materially changed correctness, at measurable workflow cost. This is evidence for context delivery in one task, not a general performance claim. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (10)
packages/cli/test/utils/uninstall-scrubbers.test.ts (1)
97-111: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case where the manifest does not own the statusline script.
The current test passes only because the command's last token differs from
.claude/hooks/statusline.py. It does not cover the case where a user wrote that exact command and Trellis never installed the script. Add a case that callsscrubHooksJsonwith adeletedPathslist that excludes.claude/hooks/statusline.pyand asserts thestatusLineentry survives. This depends on the ownership fix inpackages/cli/src/utils/uninstall-scrubbers.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/utils/uninstall-scrubbers.test.ts` around lines 97 - 111, Add a test in the statusLine scrubber cases using the exact `.claude/hooks/statusline.py` command while passing a deletedPaths list that excludes that script, then assert scrubHooksJson preserves the statusLine entry and reports fullyEmpty as false. Use the existing scrubHooksJson and CLAUDE_DELETE_PATHS test setup, ensuring the scenario verifies non-ownership behavior.packages/cli/test/utils/managed-removal.test.ts (1)
44-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
it.skipIffor both Windows guards.On Windows, the current tests pass without assertions. Conditional skips report the tests as skipped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/utils/managed-removal.test.ts` around lines 44 - 58, Replace the early-return Windows guards in both tests, “refuses traversal through an external parent symlink” and “treats a leaf symlink as an opaque deletion,” with the test framework’s it.skipIf mechanism so Windows reports them as skipped rather than passing without assertions.packages/cli/src/utils/managed-removal.ts (1)
27-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove shared constants and
cleanupEmptyDirsto a utility module.
utils/managed-removal.tsimportscommands/update.ts, whilecommands/uninstall.tsandcommands/ablate.tsimportutils/managed-removal.ts. Move the shared symbols toutils/and re-export them fromcommands/update.tsto prevent a future load-time cycle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/managed-removal.ts` around lines 27 - 31, Move cleanupEmptyDirs, TRELLIS_BLOCK_END, and TRELLIS_BLOCK_START into a shared utility module under utils, update managed-removal.ts and other consumers to import them there, and re-export the symbols from commands/update.ts to preserve its existing public imports while removing the utility-to-command dependency.packages/cli/src/commands/ablate.ts (1)
466-468: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the duplicated conflict and verification passes.
restoreAblationTransactionalready callscollectRestoreConflictsandverifyRestoredState. This function callscollectRestoreConflictsat Line 442 andverifyRestoredStateagain at Line 467. Each call re-hashes every managed path, andloadAblationTransactionhas already hashed the whole backup tree. For a large.trellistree the command hashes the same content several times.Keep the preflight at Line 442 for the plan output, then rely on the store for the write path.
♻️ Proposed change
restoreAblationTransaction(transaction); - verifyRestoredState(transaction); deleteAblationTransaction(transaction);Remove the now-unused
verifyRestoredStateimport only ifrollbackFailedAblationno longer needs it; it still does, so keep the import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/ablate.ts` around lines 466 - 468, Remove the redundant verifyRestoredState call after restoreAblationTransaction in the affected command flow, relying on restoreAblationTransaction for post-write conflict and verification checks while preserving the earlier collectRestoreConflicts preflight used for plan output. Keep the verifyRestoredState import because rollbackFailedAblation still uses it.packages/cli/src/utils/ablation-store.ts (2)
238-243: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompare fingerprints structurally instead of by
JSON.stringify.
JSON.stringifyequality depends on key insertion order. The order currently matches between the object literals in this module and the objects returned byablationStateSchema.parse. If a future edit reorders a schema field or a literal, every comparison silently reports a mismatch. The failure mode is severe: restore reports conflicts for unchanged paths and refuses to recover the project.♻️ Proposed order-independent comparison
export function fingerprintsEqual( left: PathFingerprint, right: PathFingerprint, ): boolean { - return JSON.stringify(left) === JSON.stringify(right); + if (left.kind !== right.kind) return false; + if (left.kind === "absent") return true; + if (left.kind === "symlink" && right.kind === "symlink") { + return left.target === right.target && left.mode === right.mode; + } + if (left.kind === "file" && right.kind === "file") { + return ( + left.sha256 === right.sha256 && + left.size === right.size && + left.mode === right.mode + ); + } + return ( + left.kind === "directory" && + right.kind === "directory" && + left.sha256 === right.sha256 && + left.mode === right.mode + ); }Note:
hashDirectoryalso embedsJSON.stringify(fingerprint)in the directory hash at Line 184. That usage is self-consistent within one run, so it does not need the same change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/ablation-store.ts` around lines 238 - 243, Update fingerprintsEqual to compare PathFingerprint values structurally and independently of object key insertion order, rather than using JSON.stringify. Preserve equality for identical field values regardless of schema or object-literal property ordering; leave hashDirectory’s JSON.stringify usage unchanged.
158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
z.iso.datetime()instead of the deprecated method form.Zod
^4.4.2supports both APIs, butz.iso.datetime()is the recommended replacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/ablation-store.ts` at line 158, Update the createdAt schema field to use Zod’s z.iso.datetime() API instead of the deprecated z.string().datetime() method form, while preserving its existing validation behavior.packages/cli/test/utils/ablation-store.test.ts (1)
249-263: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for conflict recovery on a
preparingtransaction.This test covers a conflict after
applied. It does not cover a conflict while the status ispreparing. That path writesconflictand then permanently loses thepre-matching relaxation incollectRestoreConflicts. See the comment onpackages/cli/src/utils/ablation-store.tsLines 647-660.Add a test that stages a transaction, edits one managed path, expects
AblationConflictError, reverts the edit, and then expects a successful restore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/utils/ablation-store.test.ts` around lines 249 - 263, Add a regression test alongside the existing conflict test for a transaction still in preparing status: stage it, edit a managed path, assert restoreAblationTransaction throws AblationConflictError, revert the edit to its original content, then assert a subsequent restore succeeds and completes the transaction cleanup.packages/cli/test/commands/ablate.integration.test.ts (2)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the original
isTTYdescriptor inafterEach.
vi.restoreAllMocks()does not revertObject.defineProperty. Theprocessobject is shared between test files that run in the same worker, so the overwrittenisTTYvalue leaks after this file completes.Capture the original descriptor in
beforeEachand reinstate it inafterEach.♻️ Proposed change
+ let originalIsTTY: PropertyDescriptor | undefined; + beforeEach(() => { ... + originalIsTTY = Object.getOwnPropertyDescriptor(process.stdin, "isTTY"); Object.defineProperty(process.stdin, "isTTY", { configurable: true, value: true, }); });afterEach(() => { vi.restoreAllMocks(); + if (originalIsTTY) { + Object.defineProperty(process.stdin, "isTTY", originalIsTTY); + } else { + Reflect.deleteProperty(process.stdin, "isTTY"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/commands/ablate.integration.test.ts` around lines 49 - 53, Update the test setup around the process.stdin isTTY override to capture its original property descriptor in beforeEach and restore that descriptor in afterEach. Keep vi.restoreAllMocks() as-is, but explicitly reinstate the descriptor so the shared process.stdin state does not leak between tests.
167-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
it.skipIf(process.platform === "win32")for tests 7 and 10.Early returns record these Windows tests as passed without execution. Vitest 4 supports
it.skipIf, which reports them as skipped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/commands/ablate.integration.test.ts` around lines 167 - 168, Update tests `#7` and `#10` to use Vitest’s it.skipIf(process.platform === "win32") instead of returning early inside the test bodies, so Windows runs report them as skipped rather than passed without execution..trellis/spec/cli/backend/index.md (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the ablation command to the pre-development checklist.
The new index entry is present, but the checklist does not direct contributors editing
commands/ablate.tsorutils/ablation-store.tsto readcommands-ablate.md. Add a bullet beside the uninstall entry that also points tofilesystem-safety.md.As per path instructions,
.trellis/**work must consultspec/for package- and layer-scoped coding guidelines before writing code in a given layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.trellis/spec/cli/backend/index.md at line 35, Update the pre-development checklist in the backend index to add an ablation entry beside the uninstall entry, directing contributors working on commands/ablate.ts or utils/ablation-store.ts to read commands-ablate.md and filesystem-safety.md, while preserving the existing spec/ guidance for .trellis work.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.trellis/spec/cli/backend/commands-ablate.md:
- Around line 201-206: Clarify the “Privacy and session boundary” policy by
explicitly acknowledging that exact .trellis/ task, spec, and workspace bytes
may contain prompts, responses, or credentials and are copied to the external
state root. Narrow the absolute exclusion guarantee accordingly, or define an
enforceable exclusion, rejection, or encryption mechanism without claiming
redaction while preserving exact restoration.
- Around line 102-110: Update ablate() to call assertExternalStateRoot() before
invoking getTransactionPaths() or checking transactionPaths.transactionDir,
ensuring invalid in-project roots and symlink ancestors are rejected before any
recovery object is read or created. Add a regression test covering this ordering
and verifying that no recovery object is accessed or created.
In @.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/design.md:
- Around line 72-73: Update the rollback flow described in the ablation
procedure to delete the transaction after restoring and verifying the
pre-ablation state; when recovery or verification fails, retain the transaction
and mark it as conflict for later recovery instead of leaving it preparing.
- Around line 61-68: Update restoreAblationTransaction to acquire the project
lock before restore preflight and retain it through state changes, restoration
writes, and final verification. Revalidate all path fingerprints after acquiring
the lock, then abort on conflicts before mutating project files; ensure the lock
is released on every success and failure path.
In @.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.jsonl:
- Line 7: Update both task index entries to use the archived research path for
implementation-evidence.md: change the file value in
.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.jsonl lines
7-7 and .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/check.jsonl
lines 6-6 to
.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/implementation-evidence.md.
In @.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.md:
- Line 37: Replace the <new ablation tests> placeholder in the focused pnpm test
command with the concrete paths of the ablation-store and ablate integration
test files used by the final test run, so the archived plan contains an
executable validation command.
In @.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/prd.md:
- Around line 47-48: Resolve the conflict between full `.trellis/` backups and
the prohibition on storing user data by explicitly defining the allowed recovery
payload and retention policy. Align the ablation design with the user-data paths
identified by the `uninstall` command: either document permission to back up
them or exclude/sanitize them and revise exact-restore requirements. Add a test
covering the selected policy before release.
- Line 62: Update the `trellis ablate` and `restore` behavior specification to
explicitly define exit statuses and user-facing output when `.trellis/` or
recovery state is missing, then add integration assertions covering both
missing-state cases.
In
@.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/castforge-comparison.md:
- Around line 26-30: Revise the comparison paragraph to remove the “48.0%
sooner” completion-time speedup framing. Describe 184.31 seconds only as the
elapsed-time difference between the ablated run and the fixed control cutoff,
explicitly noting that the control did not naturally complete.
In
@.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/local-castforge-validation.md:
- Around line 7-13: Replace the machine-specific /Users/lfan/... paths in the
evidence with redacted or repository-relative placeholders, preserving the
distinctions between mechanical, control, treatment, and recovery-state
locations. Keep the statement that the canonical checkout was unmodified and the
recovery state was empty after final restore.
- Around line 28-31: Replace the Git-status SHA-256 claim in the ablation record
with per-path before-and-after hashes covering file bytes, entry type, symlink
target, and mode; if those measurements are unavailable, describe the unchanged
Git-status hash only as consistent with zero writes rather than proof.
In @.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/task.json:
- Around line 15-18: Update the completed-task metadata to preserve commit
3ea84685: in
.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/task.json lines
15-18, set commit to 3ea84685 or document why null is valid; in
.trellis/workspace/fantasyc/journal-1.md lines 20-24, replace “(see git log)”
with 3ea84685.
In `@packages/cli/src/utils/ablation-store.ts`:
- Around line 647-660: Preserve crash recovery when conflicts occur after a
transaction already matches the restored-state relaxation: in
packages/cli/src/utils/ablation-store.ts lines 647-660, guard the conflict
transition in restoreAblationTransaction with
mayAlreadyBeRestored(transaction.state), and apply the same guard to the
conflict transition in packages/cli/src/commands/ablate.ts lines 442-447;
otherwise leave the transaction’s prior status intact while still throwing
AblationConflictError.
In `@packages/cli/src/utils/managed-removal.ts`:
- Around line 276-284: Update the CLI error handling around
buildManagedRemovalPlan so strict ablation failures include recovery guidance
alongside the existing error. Tell users to remove the stale manifest entry or
restore the managed file, while preserving the current failure and exit
behavior.
In `@packages/cli/src/utils/uninstall-scrubbers.ts`:
- Around line 71-82: Update isTrellisClaudeStatusLine to accept deletedPaths and
return false unless it includes CLAUDE_TRELLIS_STATUSLINE; pass deletedPaths
from its call site in scrubHooksJson. Add coverage in
packages/cli/test/utils/uninstall-scrubbers.test.ts lines 97-111 verifying a
statusLine command for the path is preserved when that path is absent from
deletedPaths.
- Around line 390-422: Update the table-boundary regex in the scan within the
uninstall scrubber to recognize both standard TOML table headers and
array-of-tables headers such as [[agents]]. Ensure the max_depth removal stops
before either header so user-owned settings in subsequent array tables are
preserved.
---
Nitpick comments:
In @.trellis/spec/cli/backend/index.md:
- Line 35: Update the pre-development checklist in the backend index to add an
ablation entry beside the uninstall entry, directing contributors working on
commands/ablate.ts or utils/ablation-store.ts to read commands-ablate.md and
filesystem-safety.md, while preserving the existing spec/ guidance for .trellis
work.
In `@packages/cli/src/commands/ablate.ts`:
- Around line 466-468: Remove the redundant verifyRestoredState call after
restoreAblationTransaction in the affected command flow, relying on
restoreAblationTransaction for post-write conflict and verification checks while
preserving the earlier collectRestoreConflicts preflight used for plan output.
Keep the verifyRestoredState import because rollbackFailedAblation still uses
it.
In `@packages/cli/src/utils/ablation-store.ts`:
- Around line 238-243: Update fingerprintsEqual to compare PathFingerprint
values structurally and independently of object key insertion order, rather than
using JSON.stringify. Preserve equality for identical field values regardless of
schema or object-literal property ordering; leave hashDirectory’s JSON.stringify
usage unchanged.
- Line 158: Update the createdAt schema field to use Zod’s z.iso.datetime() API
instead of the deprecated z.string().datetime() method form, while preserving
its existing validation behavior.
In `@packages/cli/src/utils/managed-removal.ts`:
- Around line 27-31: Move cleanupEmptyDirs, TRELLIS_BLOCK_END, and
TRELLIS_BLOCK_START into a shared utility module under utils, update
managed-removal.ts and other consumers to import them there, and re-export the
symbols from commands/update.ts to preserve its existing public imports while
removing the utility-to-command dependency.
In `@packages/cli/test/commands/ablate.integration.test.ts`:
- Around line 49-53: Update the test setup around the process.stdin isTTY
override to capture its original property descriptor in beforeEach and restore
that descriptor in afterEach. Keep vi.restoreAllMocks() as-is, but explicitly
reinstate the descriptor so the shared process.stdin state does not leak between
tests.
- Around line 167-168: Update tests `#7` and `#10` to use Vitest’s
it.skipIf(process.platform === "win32") instead of returning early inside the
test bodies, so Windows runs report them as skipped rather than passed without
execution.
In `@packages/cli/test/utils/ablation-store.test.ts`:
- Around line 249-263: Add a regression test alongside the existing conflict
test for a transaction still in preparing status: stage it, edit a managed path,
assert restoreAblationTransaction throws AblationConflictError, revert the edit
to its original content, then assert a subsequent restore succeeds and completes
the transaction cleanup.
In `@packages/cli/test/utils/managed-removal.test.ts`:
- Around line 44-58: Replace the early-return Windows guards in both tests,
“refuses traversal through an external parent symlink” and “treats a leaf
symlink as an opaque deletion,” with the test framework’s it.skipIf mechanism so
Windows reports them as skipped rather than passing without assertions.
In `@packages/cli/test/utils/uninstall-scrubbers.test.ts`:
- Around line 97-111: Add a test in the statusLine scrubber cases using the
exact `.claude/hooks/statusline.py` command while passing a deletedPaths list
that excludes that script, then assert scrubHooksJson preserves the statusLine
entry and reports fullyEmpty as false. Use the existing scrubHooksJson and
CLAUDE_DELETE_PATHS test setup, ensuring the scenario verifies non-ownership
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 669466d4-6f89-4726-a0bc-e5d8103e1881
📒 Files selected for processing (29)
.trellis/spec/cli/backend/commands-ablate.md.trellis/spec/cli/backend/commands-uninstall.md.trellis/spec/cli/backend/index.md.trellis/spec/cli/backend/uninstall-scrubbers.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/check.jsonl.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/design.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.jsonl.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/prd.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/castforge-comparison.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/final-review.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/implementation-evidence.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/local-castforge-validation.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/task.json.trellis/workspace/fantasyc/index.md.trellis/workspace/fantasyc/journal-1.mdREADME.mdREADME_CN.mdpackages/cli/src/cli/index.tspackages/cli/src/commands/ablate.tspackages/cli/src/commands/uninstall.tspackages/cli/src/utils/ablation-store.tspackages/cli/src/utils/cwd-guard.tspackages/cli/src/utils/managed-removal.tspackages/cli/src/utils/uninstall-scrubbers.tspackages/cli/test/commands/ablate.integration.test.tspackages/cli/test/utils/ablation-store.test.tspackages/cli/test/utils/managed-removal.test.tspackages/cli/test/utils/uninstall-scrubbers.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/utils/ablation-store.ts (1)
469-542: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHold the project lock across ablation apply and rollback.
ablatestages the transaction and callsapplyAblationPlanwithout acquiring the project lock. A concurrenttrellis restorecan interleave writes or delete the transaction before rollback completes. Acquire the lock before staging and hold it through apply, verification, and state transition. MakerollbackAblationTransactionreuse the held lock because the current lock is not reentrant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/ablation-store.ts` around lines 469 - 542, Update the ablation command flow around stageAblationTransaction and applyAblationPlan to acquire the project lock before staging and retain it through apply, verification, and state transition. Modify rollbackAblationTransaction to reuse the already-held lock rather than attempting a nested acquisition, preserving safe cleanup and release when the full operation completes.
🧹 Nitpick comments (2)
packages/cli/src/utils/ablation-store.ts (1)
639-650: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
releaseProjectLockcan delete a lock owned by another process.The function reads the holder pid, compares it, then unlinks. Another process can replace the lock file between the read and the unlink, and this process then removes the new owner's lock. The window is small, but the failure mode is two concurrent restores on the same project.
An
openSync/fstatSyncinode check, or a per-acquisition unique token compared before unlink, closes the window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/ablation-store.ts` around lines 639 - 650, Make releaseProjectLock atomically verify lock ownership before removal so a replacement lock cannot be deleted after the initial read; use an openSync/fstatSync inode check or the existing acquisition’s unique token, and unlink only when that identity still matches the lock being released.packages/cli/test/utils/managed-removal.test.ts (1)
59-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend this test to execute the plan.
Import and call
executeManagedRemovalPlan(tmpDir, plan). Assert that.codex/hooks.jsonis removed andtarget.jsonretains its original contents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/utils/managed-removal.test.ts` around lines 59 - 77, Extend the “treats a leaf symlink as an opaque deletion” test by calling executeManagedRemovalPlan(tmpDir, plan) after building the plan. Assert that .codex/hooks.json no longer exists and target.json still contains its original contents.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.trellis/spec/cli/backend/commands-ablate.md:
- Around line 25-28: Update the ablate command specification to require an
atomic per-project reservation before snapshot creation, held through staging
and ablation and released on both success and failure; clarify that only one
concurrent invocation may proceed before transaction publication, and add a
separate-process concurrent-ablate test covering this behavior.
- Around line 192-196: Update the restore implementation centered on
restoreEntry() to protect each target path from concurrent edits after
fingerprint validation, using a per-path lock or compare-and-replace check
before replacement. Restore regular files via an atomic temporary-file write and
rename, preserving the existing conflict behavior when validation fails. Ensure
injected write or rename failures clean up temporary files and transition the
transaction out of restoring safely, then add tests covering concurrent races
and injected failures.
In `@packages/cli/src/utils/managed-paths.ts`:
- Around line 10-24: Update cleanupEmptyDirs to validate dirPath before
isManagedPath or filesystem access: reject absolute paths and any "." or ".."
segments, resolve the canonical project-relative path, and require the resolved
location to remain below cwd before constructing fullPath. Preserve managed-root
and recursive parent cleanup behavior, and add regression tests covering
traversal inputs such as .trellis/../user-empty-dir.
---
Outside diff comments:
In `@packages/cli/src/utils/ablation-store.ts`:
- Around line 469-542: Update the ablation command flow around
stageAblationTransaction and applyAblationPlan to acquire the project lock
before staging and retain it through apply, verification, and state transition.
Modify rollbackAblationTransaction to reuse the already-held lock rather than
attempting a nested acquisition, preserving safe cleanup and release when the
full operation completes.
---
Nitpick comments:
In `@packages/cli/src/utils/ablation-store.ts`:
- Around line 639-650: Make releaseProjectLock atomically verify lock ownership
before removal so a replacement lock cannot be deleted after the initial read;
use an openSync/fstatSync inode check or the existing acquisition’s unique
token, and unlink only when that identity still matches the lock being released.
In `@packages/cli/test/utils/managed-removal.test.ts`:
- Around line 59-77: Extend the “treats a leaf symlink as an opaque deletion”
test by calling executeManagedRemovalPlan(tmpDir, plan) after building the plan.
Assert that .codex/hooks.json no longer exists and target.json still contains
its original contents.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c160637-6f6e-4676-aea7-3498a22dd31a
📒 Files selected for processing (24)
.trellis/spec/cli/backend/commands-ablate.md.trellis/spec/cli/backend/index.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/check.jsonl.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/design.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.jsonl.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/prd.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/castforge-comparison.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/local-castforge-validation.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/task.json.trellis/workspace/fantasyc/journal-1.mdREADME.mdREADME_CN.mdpackages/cli/src/commands/ablate.tspackages/cli/src/commands/update.tspackages/cli/src/utils/ablation-store.tspackages/cli/src/utils/managed-paths.tspackages/cli/src/utils/managed-removal.tspackages/cli/src/utils/manifest-prune.tspackages/cli/src/utils/uninstall-scrubbers.tspackages/cli/test/commands/ablate.integration.test.tspackages/cli/test/utils/ablation-store.test.tspackages/cli/test/utils/managed-removal.test.tspackages/cli/test/utils/uninstall-scrubbers.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- README.md
- .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.jsonl
- .trellis/workspace/fantasyc/journal-1.md
- README_CN.md
- .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/local-castforge-validation.md
- .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/task.json
- packages/cli/test/utils/uninstall-scrubbers.test.ts
- .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/research/castforge-comparison.md
- .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/implement.md
- packages/cli/src/utils/uninstall-scrubbers.ts
- .trellis/spec/cli/backend/index.md
- .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/design.md
- packages/cli/src/utils/managed-removal.ts
- .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/check.jsonl
- packages/cli/src/commands/ablate.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/utils/ablation-store.ts (1)
626-646: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe stale-lock path can delete a lock that another process just created.
acquireProjectLockwrites the pid afteropenSync. Between those two calls the lock file exists and is empty. A second process that hitsEEXISTreads an empty file, computesholderPid = 0(Number("")is0), skips thepidAlivecheck, and unlinks the live lock. Both processes then believe they own the project lock, and ablation state plus project mutations can interleave.Treat an unparsable or empty lock as stale only after an age threshold, and confirm the file identity before unlinking.
🔒 Proposed fix
let holderPid = 0; + let lockAgeMs = 0; try { holderPid = Number(fs.readFileSync(paths.lockFile, "utf-8").trim()); + lockAgeMs = Date.now() - fs.statSync(paths.lockFile).mtimeMs; } catch { continue; } if (holderPid && pidAlive(holderPid)) { throw new Error( `Another Trellis ablation operation is already in progress for this project (pid ${holderPid}).`, ); } + if (!Number.isInteger(holderPid) || holderPid <= 0) { + // A concurrent creator has not written its pid yet. Only reclaim the + // lock once it is old enough to be a genuine leftover. + if (lockAgeMs < LOCK_STALE_MS) { + throw new Error( + "Another Trellis ablation operation is already in progress for this project.", + ); + } + } try { fs.unlinkSync(paths.lockFile);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/ablation-store.ts` around lines 626 - 646, Update acquireProjectLock’s stale-lock handling so an empty or unparsable lock file is not immediately deleted: only treat it as stale after the defined age threshold. Before unlinking any stale lock, re-stat or otherwise confirm the file identity has not changed since it was read, then retry acquisition when another process replaced it.
🧹 Nitpick comments (2)
packages/cli/test/utils/ablation-store.test.ts (1)
306-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe ready promise can hang if the helper process exits early.
holder.once("error", reject)fires only for spawn failures. If the child throws after start, for example whenwriteFileSyncwithflag: "wx"hitsEEXIST, the child exits without writing to stdout and the promise never settles. The test then fails by timeout instead of by cause.♻️ Proposed hardening
await new Promise<void>((resolve, reject) => { holder.once("error", reject); + holder.once("exit", (code) => + reject(new Error(`lock holder exited early with code ${code}`)), + ); holder.stdout.once("data", () => resolve()); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/utils/ablation-store.test.ts` around lines 306 - 309, Update the readiness promise around holder in the ablation-store test to reject when the child process exits before emitting stdout, while preserving rejection for spawn errors and resolution on the first stdout data event. Ensure all listeners are cleaned up or the promise settles only once.packages/cli/src/utils/ablation-store.ts (1)
709-733: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated expected-ablated assertion.
The same conflict check appears four times in
restoreEntry. Each copy builds an identicalAblationConflictError. Extract a helper to keep the checks in sync.♻️ Proposed refactor
+function assertExpectedAblated(entry: AblationEntry, destination: string): void { + const actual = fingerprintPath(destination); + if (fingerprintsEqual(actual, entry.expectedAblated)) return; + throw new AblationConflictError([ + { + relativePath: entry.relativePath, + expected: entry.expectedAblated, + actual, + }, + ]); +}Then replace each inline block with
if (verifyExpectedState) assertExpectedAblated(entry, destination);.Also applies to: 756-767, 785-796
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/ablation-store.ts` around lines 709 - 733, In restoreEntry, extract the repeated verifyExpectedState fingerprint comparison and AblationConflictError construction into an assertExpectedAblated helper that accepts entry and destination. Replace all four inline expected-ablated conflict checks with calls to this helper guarded by verifyExpectedState, preserving the existing expected and actual fingerprint values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/prd.md:
- Around line 66-70: Update the restore replacement flow described by
beforeReplace to prevent fs.renameSync from overwriting a target changed by an
external writer: use a no-replace compare-and-swap mechanism, or explicitly
narrow the documented conflict guarantee. Add coverage for a writer acting
between beforeReplace and replacement, verifying the external content remains
intact, temporary files are removed, and the operation can be retried.
---
Outside diff comments:
In `@packages/cli/src/utils/ablation-store.ts`:
- Around line 626-646: Update acquireProjectLock’s stale-lock handling so an
empty or unparsable lock file is not immediately deleted: only treat it as stale
after the defined age threshold. Before unlinking any stale lock, re-stat or
otherwise confirm the file identity has not changed since it was read, then
retry acquisition when another process replaced it.
---
Nitpick comments:
In `@packages/cli/src/utils/ablation-store.ts`:
- Around line 709-733: In restoreEntry, extract the repeated verifyExpectedState
fingerprint comparison and AblationConflictError construction into an
assertExpectedAblated helper that accepts entry and destination. Replace all
four inline expected-ablated conflict checks with calls to this helper guarded
by verifyExpectedState, preserving the existing expected and actual fingerprint
values.
In `@packages/cli/test/utils/ablation-store.test.ts`:
- Around line 306-309: Update the readiness promise around holder in the
ablation-store test to reject when the child process exits before emitting
stdout, while preserving rejection for spawn errors and resolution on the first
stdout data event. Ensure all listeners are cleaned up or the promise settles
only once.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dc9c955-4ffa-4656-bddc-737ffabae344
📒 Files selected for processing (8)
.trellis/spec/cli/backend/commands-ablate.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/design.md.trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/prd.mdpackages/cli/src/commands/ablate.tspackages/cli/src/utils/ablation-store.tspackages/cli/src/utils/managed-paths.tspackages/cli/test/commands/update-internals.test.tspackages/cli/test/utils/ablation-store.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/cli/src/utils/managed-paths.ts
- .trellis/spec/cli/backend/commands-ablate.md
- packages/cli/src/commands/ablate.ts
- .trellis/tasks/archive/2026-08/08-10-full-trellis-ablation/design.md
|
@coderabbitai review |
|
|
First, the part that is on us: you asked for direction in #530, followed up on 2026-08-10 explicitly saying you wanted maintainer input before opening a PR, and got no reply either time. You then scoped the work down yourself, deferred selective capabilities and release baselines, and built it. Waiting on an answer that never came, and being left to guess the boundary, is not the experience we want contributors to have. Sorry. Where this standsThe direction question — whether reversible project mutation belongs in Trellis at all — is still genuinely open on our side and needs a maintainer decision rather than a reviewer's opinion. I am flagging it for that decision now rather than letting it sit again. What I can give you today is the code-level read. Code-level observationsThe scoping you chose looks right: full-only, no selective capabilities, no release baselines, no worktree or agent orchestration. Reusing the v2 managed-file manifest and the uninstall planner rather than inventing a parallel ownership model is the correct instinct, and pulling the shared removal logic out into The all-path conflict preflight before restore, with zero writes on conflict, is the property that makes this safe enough to be worth having. Same for verifying the backup before mutating anything. Two concrete things to fix regardless of which way the direction call goes:
What happens nextThe direction decision comes back to you here, not another round of silence. If it is yes, the code is in reviewable shape and the remaining work is small. If it is no, you will get that answer with the reasoning rather than a stale PR — and in that case the |
2 participants
Summary
trellis ablateandtrellis restorecommandsRelated to #530. This PR intentionally does not implement selective capability targets or release baselines.
Safety model
backup/<project-relative-path>Verification
origin/main: 29 files, 110 symbols, 13 intended ablate/restore/scrub flows; high breadth as expected for a destructive/recovery commandDisposable mechanical validation from one committed revision proved:
A separate context-retrieval-focused comparison used a medium-large production TypeScript/Next.js application (about 775 tracked files and 130k tracked text lines) and a presentation-only provider benefit-title formatter. Two fresh agents received the same history-free source, symptom-level prompt, model/reasoning setting, sandbox, and time budget; native multi-agent was disabled and neither condition was rerun or steered.
This is narrow evidence that Trellis delivered project-specific edge-case policy at measurable workflow cost in one paired run, not a general performance claim.
Scope limits
Environment note
The Codex runtime's pnpm dependency bootstrap blocked esbuild's install script before Husky could run. Commits used
HUSKY=0only after the equivalent package-local lint, typecheck, build, focused tests, and full 1,712-test gate passed.Summary by CodeRabbit
trellis ablateandtrellis restorewith dry-run and confirmation options.