fix(hooks): read trust state from atomic snapshots - #1210
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: cbdc394063
ℹ️ 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".
| writeFileSync(path, serializeHookTrustState(next), "utf-8"); | ||
| const tempPath = `${path}.${process.pid}.${randomUUID()}.tmp`; | ||
| try { | ||
| writeFileSync(tempPath, serializeHookTrustState(next), "utf-8"); |
There was a problem hiding this comment.
When an existing hooks-state.json has restrictive permissions such as 0600, this temporary file is created using the process umask—typically producing 0644—and the rename replaces the original inode, silently making stored source paths and command previews readable by other local users after /hooks trust, enable, or disable. The previous direct write preserved the existing mode; create the temporary file with the destination's current mode, while using a suitably restrictive default for a new file.
Useful? React with 👍 / 👎.
| read(scope: HookTrustStorageScope): HookTrustState { | ||
| return withHookStateFileLock(statePathForScope(scope, this.globalStatePath, this.projectStatePath), (path) => | ||
| readHookTrustStateJson(existsSync(path) ? readFileSync(path, "utf-8") : undefined), | ||
| ); | ||
| const path = statePathForScope(scope, this.globalStatePath, this.projectStatePath); | ||
| return readHookTrustStateJson(existsSync(path) ? readFileSync(path, "utf-8") : undefined); |
There was a problem hiding this comment.
This changes the behavior of trust-storage.ts, a path absent from the pinned upstream tree, but the commit does not update its nearest tracker, packages/coding-agent/src/core/extensions/builtin/changes.md. Record the lock-free read and atomic-write behavior in that tracker in this increment so the next upstream synchronization does not treat the fork's persistence semantics as undocumented state.
AGENTS.md reference: AGENTS.md:L101-L101
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e54e99133
ℹ️ 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".
| writeFileSync(tempPath, serializeHookTrustState(next), { encoding: "utf-8", mode }); | ||
| renameSync(tempPath, path); |
There was a problem hiding this comment.
When an existing global or project trust-state file has metadata beyond Unix mode bits—for example shared-project ownership/POSIX ACLs or a restrictive Windows DACL—the temporary file is owned by the current process and inherits the directory's ACL, and renameSync replaces the original inode with that metadata. Consequently, /hooks trust, enable, or disable can lock intended collaborators out or widen access even though the numeric mode appears unchanged. Fresh evidence beyond the earlier permissions comment is that the new preservation logic copies only statSync(path).mode & 0o777; preserve the destination's ownership and ACL metadata as well, or use an atomic replacement mechanism that retains it.
Useful? React with 👍 / 👎.
| readHookTrustStateJson(existsSync(path) ? readFileSync(path, "utf-8") : undefined), | ||
| ); | ||
| const path = statePathForScope(scope, this.globalStatePath, this.projectStatePath); | ||
| return readHookTrustStateJson(existsSync(path) ? readFileSync(path, "utf-8") : undefined); |
There was a problem hiding this comment.
When a still-running pre-upgrade Senpi process updates this shared file, it holds the same writer lock but truncates and rewrites hooks-state.json in place; this new reader ignores that lock and can therefore read an empty or partial document. readHookTrustStateJson converts that malformed snapshot to an empty trust state, so a concurrent startup can silently skip previously trusted SessionStart hooks even though the writer is behaving correctly for its version. Retry a malformed read while the writer lock exists, or otherwise retain compatibility with in-place writers during mixed-version concurrency.
Useful? React with 👍 / 👎.
|
Local adversarial review is complete: ultrabrain round 3 returned APPROVE after two REQUEST_CHANGES cycles. All GitHub checks, including the new Windows hooks trust-storage regression and required Check and test fan-in, are green. This account has READ permission on the upstream repository, so immediate merge and auto-merge both fail at the branch-policy permission gate. Maintainer merge is now the only remaining action for this PR. |
|
A downstream packed-install review found a mixed-version stale-read race not covered by the previously approved head: a reader can capture malformed bytes while a legacy writer holds the lock, then observe the lock gone after valid publication and return the stale empty state. A deterministic bounded source regression and protocol fix are now being prepared; prior ultrabrain approval is treated as invalidated until the updated head is reviewed again. Please do not merge the current head. |
…c-snapshot-delivery
Keep complete trust snapshots on the lock-free path while boundedly re-reading malformed or empty bytes observed across the exact legacy writer lock. This prevents a reader from returning stale fail-closed state after a truncate-in-place writer publishes valid JSON and releases its lock. Ultraworked with omo Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
|
Mixed-version stale-read fix pushed at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b5895ea03
ℹ️ 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".
| const lockIsActive = existsSync(lockPath); | ||
| if ((!lockWasActive && !lockIsActive) || attempt === maxAttempts) { | ||
| return emptyHookTrustState(); |
There was a problem hiding this comment.
When a writer acquires the lock after lockWasActive is sampled and finishes before lockIsActive is sampled, both probes are false even though line 38 may have observed a missing or truncated file; this branch then returns an empty trust state and can skip trusted startup hooks. Fresh evidence beyond the earlier mixed-version comment is that the new two-probe implementation still leaves this complete between-probes race for both legacy and atomic writers; re-read the snapshot before treating the state as malformed-and-unlocked.
Useful? React with 👍 / 👎.
| const state = storage.read("global"); | ||
|
|
||
| // Then | ||
| expect(state).toEqual({ version: 1, hooks: {} }); |
There was a problem hiding this comment.
If the retry implementation regresses to return after its first malformed read, this test still passes because it only checks the same final empty state; the 10-second wall-clock timeout proves termination but neither the intended ten attempts nor retry ordering. Mock the clock/filesystem probes and assert the exact attempt sequence so this central concurrency regression cannot silently return.
AGENTS.md reference: packages/coding-agent/test/suite/AGENTS.md:L26-L28
Useful? React with 👍 / 👎.
|
Ultrabrain round 4 returned REQUEST_CHANGES. Green CI missed a deterministic lock ABA interleaving: reader sees lock absent, legacy writer locks/truncates/publishes/unlocks entirely during the read, reader sees lock absent again, and stale empty bytes are returned. The next fix will replace lock-existence sampling with bounded writer-excluding revalidation after malformed reads, add an exact no-sleep ABA regression, and correct tracker wording. Current head remains non-mergeable by our review gate. |
…c-snapshot-delivery
Keep complete snapshots on the lock-free fast path, but acquire the exact bounded writer lock after any incomplete read and re-read under exclusion. This closes the absent-active-absent legacy writer ABA while preserving fail-closed ELOCKED exhaustion. Ultraworked with omo Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
|
Lock ABA repair pushed at |
|
Ultrabrain round 5 returned REQUEST_CHANGES. The ABA repair is sound, but source release remains blocked because inode replacement preserves numeric mode only (not owner/group, POSIX ACLs, or Windows DACL), and lock release failures can mask primary read/publication failures. A deep fix is now investigating metadata-safe atomic replacement with fail-before-replace semantics when preservation is unavailable, plus causal operation/release error aggregation and real POSIX/Windows regressions. Current head remains non-mergeable by our review gate. |
|
Scope decision: hooks trust state is an internal application-owned per-user file, not a general metadata-preserving file replacement API. The PR will not add a mandatory native filesystem subsystem and will not claim preservation of externally assigned owner/group, named POSIX/macOS ACLs, or custom Windows DACLs. The next update will make that contract explicit, correct overbroad permission wording, and fix the valid remaining issue where lock release failures can mask primary read/publication errors with deterministic causal aggregation tests. Merge remains gated on fresh CI and ultrabrain review against this narrowed contract. |
…c-snapshot-delivery # Conflicts: # .github/changes.md # packages/coding-agent/CHANGELOG.md
Use one lock-scope release helper for exclusive reads and serialized writes so release errors cannot mask primary failures. Flatten prior aggregate entries before release failure, pin bounded inherited lease behavior, and document hooks state as ordinary same-account application state with numeric POSIX mode handling only. Ultraworked with omo Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
|
Option-B update pushed at |
1 participant
Problem
Concurrent Senpi sessions share hooks trust-state files.
FileHookStateStorage.read()took the same boundedproper-lockfilewriter lock as updates, so a healthy reader could exhaust the retry window and fail builtin hooks startup withELOCKED.Changes
0600.AggregateError.changes.mdtracker and package changelog.Verification
Lock file is already being held; permission probes widened snapshots; cleanup failure masked publication failure.npm run check, coding-agent build, typecheck, Biome, LSP, changelog/tracker gate, andgit diff --check: passed.local-ignore/qa-evidence/20260831-hooks-writer-lock/raw-receipt.log.Risk
Readers may observe the previous complete snapshot while a writer is in progress, which is intentional. Same-directory rename prevents torn JSON, restrictive metadata is preserved, and the existing writer lock continues to prevent lost writer updates. Crash-before-rename can leave an orphan temp file but cannot corrupt the published snapshot.