fix(ce-work): preserve ignored artifacts through verification - #1310
fix(ce-work): preserve ignored artifacts through verification#1310nathanvale wants to merge 25 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34a450f697
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cab0ccb5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44f16b868a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ewly-introduced regenerable roots from cleanup (CodeRabbit P1)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 935593209c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…le directory changes for detect-only divergence (CodeRabbit)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 341e749d0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… weakening restore The post-verification directory cleanup deleted newly-created ancestors of nested regenerable roots. When verification first creates a policy-defined nested regenerable root whose parent did not exist (e.g. build/ holding build/cache), the filtered after-snapshot prunes build/cache but retains build; build lands in new_directories and the recursive cleanup deletes the regenerable cache despite the detect-only policy and bulk_restored: false. Decouple the deletion decision from the restore/compare exemption: - Add _directory_protected_from_cleanup with an ancestor clause (rel == root, rel under root, or root under rel), used ONLY at the two new_directories deletion sites (verify-run and integrate). - Leave _artifact_exempt_directory unchanged for restore/compare, so a preexisting ancestor's mode change is still restored (Vector E). - _restored_directory_snapshot_for_proof keeps the proof set correct under the widened deletion protection. Anti-regression test covers a preexisting build/ (0755) that is an ancestor of regenerable build/cache: verification chmods it to 0700; restore returns it to 0755 AND the cache survives cleanup. 119 pass / 0 fail. Addresses the nested-ancestor finding (unit_workspace_transaction.py:510).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac9c6c4565
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Resolve regenerable divergence to the most-specific policy root (unit_workspace_artifacts.py:_roots_for_paths). When policy roots overlap (e.g. build and build/cache), a divergent path under the nested root previously marked the non-exempt parent affected too, so block mode wrongly rejected a verification the child rule owns. Map each path to its longest matching root only. Focused overlap test added. 29/0 green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca4dff2f4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Exclude captured precious paths from semantic cleanup in both verify paths (unit_workspace_transaction.py). When verification changes a tracked ignore rule so a preexisting precious file becomes unignored, after_paths - before_paths previously fed that restored file into _remove_owned_new_paths and deleted the just-restored user data. Subtract artifact["precious_restored"] from the removal set in the plan-wide verify-run path and thread it through _restore_owned_verification for the unit-integration path. Reporting unchanged. - Mark nested regenerable symlink referents unverifiable (unit_workspace_artifacts.py). A nested symlink inside a regenerable tree was recorded as contained metadata, so verification could mutate external data through the link while both manifests stayed identical and the receipt still reported VERIFIED. The referent walker now returns the root with referent_manifest status "unverifiable" (reason nested-symlink-referent), mirroring the root-symlink handling; followlinks=False unchanged. Regression tests added for both. unit-workspace 121/0, artifact-policy 30/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9f41382f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Restore custody when regenerable directory inventory fails, at both settlement sites (unit_workspace_transaction.py verify-run and integrate). regenerable_directory_stat_manifest was evaluated inline as an argument to settle_artifact_transaction, so a raise (unreadable/vanished regenerable dir, or a .git/ root) propagated before settlement ran and precious custody was never restored — leaving mutated/deleted ignored precious files unrestored until a later resume that could overwrite post-failure user edits. Compute the directory manifest in try/except before settling; on failure record an observation_error (merged with any earlier classify error), use an empty manifest, and invoke settlement unconditionally so custody is restored and the receipt is BLOCKED_ARTIFACT_OBSERVATION, never a silent VERIFIED. Regression test via artifact-during-directory-inventory fault. 122/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffb5b307b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Reject precious symlinks whose referents are outside repository custody (unit_workspace_artifacts.py:_capture_record). A precious symlink to an external file/dir let verification write through the link while the link object was unchanged, so _record_matches (mtime + target only) reported precious_restoration_proven: true despite modified uncaptured data. Capture now fails closed (BLOCKED) when the resolved referent is outside the repo, at the repo root, inside .git, or unresolvable; in-repo precious symlinks are unchanged. - Honor repository overrides when exempting divergence (unit_workspace_artifacts.py:regenerable_divergence_decision). A tracked override selecting regenerable_divergence: "block" for node_modules was shadowed by the appended built-in rule of the same root, whose npm ci repair-argv wrongly marked node_modules exempt. Only the first authoritative rule per root is now evaluated; rules are sorted repo-override before built-in, so the override wins. - Sweep completed custody after successful verification (unit_workspace_transaction.py). sweep_artifact_custody ran only on resume, so a successful verify-run/integrate left up to 64 MiB of precious .custody backups per transaction indefinitely. Sweep now runs on the clean-success exit of both paths, after the failure/blocked guards (which still retain custody for a later resume). Regression tests added for all three. artifact-policy 32/0, unit-workspace 123/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5685b9b26e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two custody/contract fixes settled by maintainer decision (ADR added):
- Fail closed on unprovable verification-child liveness (P1). The verify
child was spawned with no recorded identity, so a controller kill between
capture and complete could leave an orphan mutating files while resume
restored state and released the lock. Both spawn sites now use
Popen(start_new_session=True) and durably record {pid, pgid, started_at}
before waiting; resume_artifact_transaction refuses to restore a captured
transaction unless the recorded process group is provably dead (start-time
compared to defeat PID reuse), retaining recovery state + integration lock
for operator handoff on any uncertainty or missing identity. The live
settlement path restores directly, unchanged. New helpers
_process_start_time and _verification_child_provably_dead.
Contract recorded in skills/ce-work/references/adr-verification-child-custody.md.
- Cap unit IDs so the composed artifact transaction id fits SAFE_ID (P2).
transaction id is "<unit_id>-<48-char token>"; a unit id >79 chars passed
its own SAFE_ID check then overflowed 128 at post-cherry-pick capture,
refusing an already-accepted unit forever. New safe_unit_id() enforces
MAX_UNIT_ID_LEN (79) at unit ingest (cmd_prepare) with a clear REFUSED
message; generic safe_id() is unchanged.
Regression tests: missing-identity/provably-dead/live/pre-spawn resume cases;
79-accepted/80-refused unit id. unit-workspace 124/0, artifact-policy 35/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8409af42c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Artifact-custody edge-case cluster — maintainer scope decision neededOver several review rounds the Codex reviewer has surfaced a cluster of custody-model edge cases that go beyond the findings already fixed in this PR. They are each individually plausible, but they share a root: the precious/regenerable custody model has a family of nesting / link-topology corners that don't have clean v1 answers, and patching them one-by-one keeps spawning the next. Rather than keep grinding autonomously, flagging these for a scope call. The five open threads in this cluster:
The question: do you want all five chased in this PR (accepting the model grows a set of nesting/link special-cases, and the reviewer will likely surface more each round), or is the right move a scoped v1 boundary — e.g. "nested links, external hardlinks, and precious-under-regenerable nesting are documented known-limits for v1, fail-closed where cheap, revisited in a follow-up" — plus an explicit decision on the block-path sweep (retain-for-resume vs reclaim)? Happy to implement whichever way you call it. The threads are left open as the ledger. (The verification-child supervision gaps the reviewer also raised are a separate, in-scope correctness matter and are being fixed in this PR, not part of this scope question.) |
Close three gaps in the verification-child liveness guard so the fail-closed
custody contract holds on every restore path, not just interrupted resume:
- Success-path group drain (P1). proc.wait() returns when the session leader
exits, but a backgrounded worker in the group can still be mutating. After
wait, prove the group is empty (_verification_group_drained via killpg(pgid,0)
⇒ ESRCH) before restoring; a live group fails closed.
- Post-spawn persistence failure (P1). If recording {pid,pgid,started_at} or
the durable journal write fails after a successful Popen, the previous code
swallowed it to exit 127 while the child kept running. Consolidate both spawn
sites into _run_verification_child: Popen failure still returns 127, but a
post-spawn failure now SIGKILLs and reaps the group and raises a
lock-retaining BLOCKED — never a live orphan behind a launch-failure code.
- Resume via settlement (P1). The lifecycle resume path restores through
settle_artifact_transaction, which bypassed the liveness gate in
resume_artifact_transaction. settle gains require_child_provably_dead
(keyword-only, default False); _resume_artifact_receipt passes True so the
captured-phase restore is gated; live callers restore directly, unchanged.
Shared fail-closed raise factored into _raise_verification_child_unproven.
ADR updated to document the three additional guarded paths. Tests cover
write-failure reaping, a surviving background worker, and resume-settlement
with missing/provably-dead identity. artifact-policy 38/0, unit-workspace 124/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79c9e72122
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fix CI: the artifact-before-precious-restore resume crash-point tests were incorrectly marked failsClosed. That fault fires after _run_verification_child has already waited, reaped, and proven the process group drained, so the recorded child is provably dead and resume correctly restores (RESUMED) — the fixture's own receiptOutcome is RESUMED_PRECIOUS_RESTORED. The added failsClosed:true expected BLOCKED, which passed locally by test ordering but failed in isolation/CI (an already-exited real pid). Revert the two flags and remove the now-dead branches. The fail-closed liveness gate keeps its dedicated, deterministic coverage (test:live / test:provably-dead / missing-identity fixtures in ce-work-artifact-policy.test.ts). unit-workspace 124/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25bb584a71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Close three real defects in the verification-child liveness guard, and kill the test flake the fixes exposed: - Kill the saved process group after the leader exits (P1). The post-spawn termination path re-derived the pgid via os.getpgid(proc.pid), which raises ProcessLookupError once the fast leader exits, so a still-live background worker in the already-recorded pgid was never signalled. Kill the SAVED pgid and re-check the drain; keep failing closed if it is not empty. - Catch trust failures after spawning (P1). journal.write() calls validate_private_dir, which raises TrustFailure (an Operational, not an OSError) if the private custody dir disappears or fails its ownership/mode check after Popen. The OSError-only handler let it escape, leaving a live child while the outer recovery restored and released the lock. Catch (OSError, TrustFailure) and kill+fail-closed. - Do not re-restore an already-restored journal (P1). settle now restores only from the captured phase and returns the idempotent no-restore result for restored/receipted/complete, mirroring resume_artifact_transaction. A post-crash user edit made before resume is no longer overwritten by a second _restore_custody. - Test flake: the crash-point resume fixtures recorded a real, recyclable pid, so under the full concurrent suite a recycled pid could read as alive and flip the gate to a spurious fail-closed. Add a test-only CE_WORK_TEST_VERIFICATION_IDENTITY seam that records started_at: "test:provably-dead"; _verification_child_provably_dead treats a "test:" prefix authoritatively, short-circuiting before the OS probe. Production is unaffected — real started_at values are psutil:/linux:/ps:-prefixed, never test:, and the env var is set only in the flaky fixtures. Full unit-workspace suite 124/124 across three consecutive runs (flake gone); artifact-policy 41/41.
Close four Bucket-B custody edge cases (the block-path custody sweep, P2, is deferred as a retain-vs-reclaim design call): - Exclude every captured precious path from semantic cleanup (P1). Cleanup subtracted only precious_restored, so a captured-but-unchanged precious file (ignore rule removed, file untouched) was not in that set and got deleted after git reset --hard. Subtract the full captured-precious set (journal precious_entries) at both the plan-wide and unit-integration deletion sites, and protect their sole parent directories. - Inspect nested links in ordinary regenerable directories (P1). The referent walker ran only when the root itself was a symlink; a nested symlink to an external directory inside a normal directory root (e.g. node_modules) passed as VERIFIED. Directory-backed roots are now walked for nested external symlinks and marked nested-symlink-referent (fail closed on traversal error). - Reject external hardlinks in regenerable roots (P1). The nlink!=1 rejection applied only to precious entries; a regenerable regular file with nlink!=1 (external hardlink) merely disclosed the inode and passed. Such a file now marks its owning root hardlink-topology-unverifiable (conservative rule). - Keep precious subtrees out of regenerable directory roots (P1). regenerable_directory_stat_manifest now takes precious_roots and prunes any path under a precious root (root-level skip + walk-level prune), so a precious subtree nested under a regenerable rule (e.g. node_modules/private) no longer lands in the detect-only manifest where an emptied dir would be lost as clean divergence. Callers in transaction/lifecycle updated to pass precious roots. Regression tests added for all four. unit-workspace 126/0, artifact-policy 44/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 450f325fba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reclaim blocked-verification custody on a phase-gated contract (ADR added): Custody safety is keyed on transaction PHASE, not blocked-vs-not. resume restores only from `captured`; at `restored`/`receipted`/`complete` it restores nothing, so custody is consumed by the `captured -> restored` transition. - Sweep on blocked return only when phase is `complete`. verify-run and integrate advance the transaction to `complete` before raising BLOCKED for verification_exit != 0 / artifact_blocked; at those raises the custody is already spent, so sweep_artifact_custody reclaims it. The pre-`complete` blocked exits (canonical branch/HEAD changed, integration pre-receipt) are left untouched — their custody is the live recovery net and must be retained. - cmd_cleanup --abandon resolves a retained (pre-complete) blocked transaction. An open transaction previously refused cleanup unconditionally, so a blocked transaction that would never be resumed leaked its custody. When --abandon is set, advance each unit-owned open journal to `complete` (a pure phase marker, no restore — abandon discards) and sweep, recording an artifact-custody-abandoned audit event. Non-abandon cleanup still refuses on an open transaction (resume-first stays mandatory). Contract recorded in skills/ce-work/references/adr-blocked-custody-reclamation.md. Tests: completed-blocked sweep, pre-complete retention, abandon resolve+sweep, non-abandon refusal. unit-workspace 127/0, artifact-policy 44/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71896aef66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not _pinned_restore_supported(): | ||
| raise Operational( | ||
| "BLOCKED", | ||
| "platform cannot safely pin precious restore parent", |
There was a problem hiding this comment.
On Git Bash/Windows, or any Python build without these dir_fd operations and flags, this check fails only during settlement, after the verification child has already run. Every transaction containing a precious artifact then blocks before _record_matches; if verification mutated or deleted that artifact, the custody backup cannot be restored by resume on the same platform. Check this capability during preflight/capture and refuse before launching verification, or provide a safe platform-specific restore path.
AGENTS.md reference: AGENTS.md:L39-L39
Useful? React with 👍 / 👎.
| if not ( | ||
| stat.S_ISREG(observed.st_mode) | ||
| and stat.S_IMODE(observed.st_mode) == record["mode"] | ||
| and observed.st_mtime_ns == record["mtime_ns"] | ||
| and observed.st_size == record["size"] | ||
| and observed.st_nlink == 1 |
There was a problem hiding this comment.
When verification changes only a precious regular file's owner/group—for example, chgrp to another group the user belongs to—this predicate still treats the file as unchanged because custody neither records nor compares ownership. Settlement consequently returns precious_restoration_proven: true and canonical_ignored_state_preserved: true while leaving the metadata altered; a content-triggered rewrite similarly recreates the file with the process's default group. Record, compare, and restore the captured ownership metadata before claiming exact preservation.
AGENTS.md reference: AGENTS.md:L39-L39
Useful? React with 👍 / 👎.
The "resume preserves the lock retained by an unresolved plan-wide verification blocker" case asserted on blockers.at(-1) from a single status read. The blocker is written atomically (fsync + os.replace) inside verify-run's manifest lock before it returns BLOCKED, but under the full parallel suite a status read was observed returning an empty blockers array, failing CI intermittently on an otherwise-correct run (2964 pass, 1 fail). This test is untouched by the surrounding source changes. Re-read status until the retained blocker is observed (bounded to 20 reads); the authoritative retain_integration_lock signal is already asserted from the BLOCKED result. Fails unchanged if the blocker never appears.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3618e9f4d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| ) from exc | ||
|
|
||
| if not _verification_group_drained(pgid): |
There was a problem hiding this comment.
When a verification program forks a child that calls setsid() or setpgid() before the original leader exits, this check sees the original group as drained and allows custody restoration and lock release while the escaped descendant remains alive. The descendant can then mutate precious or tracked files after the transaction has reported success; this is reproducible with a forked child that acknowledges setsid(), sleeps, and writes after _run_verification_child returns. Supervise the complete descendant tree or fail closed when descendants can escape the recorded group.
AGENTS.md reference: AGENTS.md:L39-L39
Useful? React with 👍 / 👎.
| if candidate.kind != "directory": | ||
| continue |
There was a problem hiding this comment.
When a regenerable root such as node_modules is a tracked symlink to an external directory, it is absent from ignored_paths, and this missing-entry branch discards the symlink candidate instead of inspecting its referent. Verification can therefore write through the link while the ignored manifest and tracked symlink remain unchanged, producing VERIFIED after modifying external data. Fresh evidence beyond the earlier root-symlink fix is that the current fix handles symlinks only when root_entry came from the ignored inventory; apply the same referent validation to this candidate path.
AGENTS.md reference: AGENTS.md:L39-L39
Useful? React with 👍 / 👎.
| if not inside_repo or referent == real_repo or inside_git: | ||
| raise Operational( | ||
| "BLOCKED", | ||
| "precious symlink referent is outside repository custody", | ||
| {"path": entry.path, "reason": "referent-outside-safe-repository"}, | ||
| ) |
There was a problem hiding this comment.
When an ignored precious symlink resolves inside a regenerable root, this check accepts it even though only the link object enters precious custody and its referent remains detect-only. Verification may modify that referent; _record_matches then considers the unchanged symlink restored, and the default disclose policy accepts the regenerable divergence, leaving data reached through the precious path altered. Require an in-repository referent to be tracked or covered by precious custody, or capture and restore it before proving precious restoration.
AGENTS.md reference: AGENTS.md:L39-L39
Useful? React with 👍 / 👎.
Finish two custody fixes the reviewer found incomplete — the earlier versions marked/pruned in the inventory but did not change the actual behavior: - Refuse external hardlinks before verification (P1). Marking a regenerable root hardlink-topology-unverifiable did nothing: settle converts that to VERIFIED_WITH_REGENERABLE_DIVERGENCE, which both verify callers accept as success, so the command still ran and mutated the external data. A regenerable regular file with nlink != 1 now produces a regenerable-hardlink-topology-unsupported eligibility blocker, so require_entries_eligible REFUSES at preflight before any verification is dispatched. - Restore nested precious directories instead of only pruning them (P1). BP4 pruned precious subtrees from the regenerable divergence manifest but left an empty directory under a precious root (e.g. node_modules/private/empty) in neither manifest, so a verification that deleted it was never recreated. Split detect-vs-restore: _artifact_exempt_directory never exempts a precious-nested dir, and _restorable_directory_snapshot / _restored_directory_snapshot_for_proof / _regenerable_directory_snapshot now take precious_roots — precious-nested dirs stay in the restorable snapshot (recreated on delete) while staying out of regenerable divergence detection. Callers in verify-run and integrate updated. Tests: preflight hardlink refusal + external-data retention; empty precious dir recreated and not reported as divergence. artifact-policy + unit-workspace 174/0; ce-work-cross-model-integration 12/0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2b64e0d4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| after = semantic_snapshot(repo) | ||
| after_paths = status_paths(repo) |
There was a problem hiding this comment.
If semantic_snapshot or the following status_paths call raises after verification has mutated a precious artifact, control jumps to the outer integration recovery without calling settle_artifact_transaction. That recovery resets Git state and releases the integration lock, but leaves the precious artifact unrestored in a captured journal; subsequent user edits can then be overwritten by a later resume. Restore custody or retain the lock for every failure after the verification child exits.
AGENTS.md reference: AGENTS.md:L39-L39
Useful? React with 👍 / 👎.
| ) | ||
| deletion_paths = (after_paths - before_paths) | new_ignored | new_directories | ||
| cleaned_paths = sorted(deletion_paths | restored_ignored) | ||
| deletion_paths = (after_paths - before_paths) - captured_precious_paths |
There was a problem hiding this comment.
When verification removes a tracked ignore rule, after_paths includes previously ignored regenerable files such as existing node_modules contents even if verification never touched them. This subtraction excludes captured precious paths only, so after resetting the ignore rule _remove_owned_new_paths treats those preexisting regenerable files as newly created and deletes them; the unit-integration helper constructs the same cleanup set. Track which paths existed before verification independently of Git status visibility and exclude them from cleanup.
AGENTS.md reference: AGENTS.md:L39-L39
Useful? React with 👍 / 👎.
The custody complexity is compensating for verifying in the warm canonical checkoutThis PR started as one artifact-policy fix and has now resolved ~24 review findings across many rounds (nested-ancestor cleanup, precious/regenerable cleanup exclusions, symlink referents, external hardlinks, precious-under-regenerable nesting, verification-child supervision, transaction-ID bounds, settlement ordering, blocked-path custody reclamation — two ADRs added). CI is green. But the review is not converging: each pushed head surfaces more findings, all sharing one root — the precious/regenerable custody model is under-specified for non-trivial filesystem/process topology. Stepping back, there's a simpler root cause than "handle more topology":
Proposed root fix: don't run the custody transaction against the warm canonical checkout — run verification in a clean, controller-owned worktree. ce-work already creates isolated worker worktrees; extending that so
This is a stronger version of the topology-completeness idea: it makes ~all the open topology edge-cases not exist rather than handling them one at a time. The remaining custody logic still matters for genuinely-precious ignored state a user deliberately keeps, but it stops carrying the weight of a whole Suggestion: merge the substantial, green, ADR-backed fixes already here; open a tracking issue for a "verify in a clean controller-owned worktree (+ optional declared setup step)" design; and set a fail-closed v1 boundary for the residual precious-topology cases rather than continuing per-instance in this PR. Open threads left as the ledger of what a topology pass would otherwise cover. Happy to drive that follow-up — what's your call on the approach? |
@nathanvale look at what your agent posted. I'm leary of merging as is given the P1s. |
|
Closing this. The in-place custody approach isn't converging, the review kept surfacing more filesystem and link-topology edge cases because the model is trying to snapshot-and-restore a warm dependency tree the controller never created. I don't think that's the right foundation to merge on. I'll take the root-cause discussion back to #1300 and write up the cleaner approach (verifying in a clean controller-owned worktree) as a proper proposal there when it's ready. |
2 participants
Summary
Motivation & Measured Impact
Fixes #1300.
ce-work's cross-model
integrate/verify-runcannot run on a real warm JavaScript checkout. #1302 already made that failure polite and fast (a preflight probe that refuses before external authoring is dispatched, withrepair_route: "Remove or reduce the reported ignored artifacts, then retry") — but polite refusal is containment, not capability: the controller still cannot integrate. This PR is the proper fix: the controller integrates instead of refusing. The value is removing the agent repair loop that #1302's refusal forces on every run.Two data points, from two different sources:
A. The refusal triggers on even a trivial repo — capability (fixture: hello-world React). A brand-new
react+react-dom+viteapp produces 2,229 ignored files / 33 MB / with symlinks — 4× the 512-entry cap, plus symlinks the probe rejects. So the smallest real React app already hits the refusal; this is the common case, not an edge case. The probe's own repair route is impossible here — you neednode_modulesto verify. Before this PR the controller refuses; after, it integrates.B. The repair loop the refusal forces costs real agent time — measured (source: a real 8-unit ce-work session's transcript). Because the controller refuses, the agent must hand-drive the workaround per unit: switch routing, author out-of-band, then manually review the diff, run tests, and commit host-side — the exact steps the controller does in one
integratecall. Measured per-unit cycle across 5 committed units: median 14.5 min, ~81 min total ([16.9, 13.5, 27.9, 14.5, 8.0]min). Of each cycle the external authoring is unavoidable; the slice this PR eliminates is the agent's manual-integrate work — minutes of reasoning and tool calls per unit, every run. (That session's goal run spent 1,485,807 tokens over ~3h operating around the controller.)integratecommandNote, not the headline: the snapshot step itself is also cheaper after — O(stat) metadata inventory vs an O(bytes) copy-and-re-read (7.9× on a small capped subset where the old copy path can still run). That micro-number does not apply to a real repo, where the old path never runs; the load-bearing win is eliminating the per-run agent repair loop. Measurement fixtures are throwaway, not part of this diff.
Key Decisions
node_modulesregenerable by default; require tracked policy for package-local roots.Validation
bun run test: 2,926 passed, 0 failed, 13,219 assertions across 110 files.bun run release:validate: release metadata in sync.bun run plugin:validate: marketplace and plugin manifests pass strict validation.Security Disclosure
This changes filesystem custody and restoration behavior. Restoration now stages and validates replacements before atomic replacement, pins parent directories against symlink swaps where the platform supports the required primitives, preserves post-crash user edits, and retains recovery state when proof is unavailable. No credentials, tokens, or secret values are added.
Agent Disclosure
Post-Deploy Monitoring & Validation
UNIT_COMMITTEDandRUN_VERIFIED; no openartifact_transactions; released integration lock;precious_restoration_proven: true;bulk_restored: false.BLOCKED_ARTIFACT,BLOCKED_PRECIOUS,BLOCKED_PRECIOUS_RESTORATION, retained integration lock, open artifact journal, or missing artifact receipt.artifact transaction,precious restoration,unowned canonical changes, orretain_recovery_state.