Repository navigation
Keep each session handshake's own failure posture when hooks resolve in parallel - #180
Merged
Merged
Conversation
…in parallel `resolveSessionConfig()` read the session store twice: once to decide whether to negotiate, and again after the handshake to choose what to return. Claude Code runs hooks in parallel and every hook in a session shares one store, so a second resolution could publish its own config between those reads. The first resolution then returned the other exchange's config, including its `on_decision_failure`. A step whose own handshake negotiated `deny` could fail by `proceed` if its decision delivery later failed. Reported privately. The store is now read once, before anything awaits. A config already stored ends the call as before, with no handshake. Otherwise the config this exchange negotiated wins, including one it negotiated but could not store, which is what the function's existing comment already promised: the value in hand is authoritative for the step that negotiated it. The store gets a second look only when this exchange negotiated nothing, for example when its own ServerHello was unusable. A config another resolution stored from the same Guardian is a better posture than the ACS default there. Returning only this exchange's value would have closed the race and failed open in that case instead. Two tests in `packages/host-adapter/test/client.test.ts` pin both halves against a real handshake and a real memory store: - An overlapping resolution publishes `proceed` right after this exchange stores `deny`. The call must return `deny`. It failed on the old code with `proceed`. - This exchange's ServerHello is unusable while another resolution stores `deny`. The call must return `deny`. It passes on the old code and failed when the last line was changed to return only this exchange's value, so it guards the fallback. `bun run typecheck` is clean and `bun test` gives 1112 pass, 1 skip, 0 fail across 1113 tests. The root README said the suite has 1,111 tests, which these two made false, so it now drops the total and keeps the count that matters: without `trash` on `PATH`, 26 tests still fail, verified by running the suite with it hidden. `uv run pytest` gives 278 passed, 1 skipped, and `uv run mkdocs build --strict` is clean. Signed-off-by: Rock Lambros <rock@rockcyber.com>
rocklambros
requested review from
GangGreenTemperTatum,
afogel,
almogbhl,
artmaro,
bar-capsule,
fewdisc,
mamicidal,
sclintonowasp and
stefanoamorelli
as code owners
September 22, 2026 03:40
This comment has been minimized.
This comment has been minimized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolveSessionConfig()read the session store twice: once to decide whether to negotiate, and again after the handshake to choose what to return. Claude Code runs hooks in parallel and every hook in a session shares one store, so a second resolution could publish its own config between those reads. The first resolution then returned the other exchange's config, including itson_decision_failure. A step whose own handshake negotiateddenycould fail byproceedif its decision delivery later failed. Reported privately.The store is now read once, before anything awaits. A config already stored ends the call as before, with no handshake. Otherwise the config this exchange negotiated wins, including one it negotiated but could not store, which is what the function's existing comment already promised: the value in hand is authoritative for the step that negotiated it.
The store gets a second look only when this exchange negotiated nothing, for example when its own ServerHello was unusable. A config another resolution stored from the same Guardian is a better posture than the ACS default there. Returning only this exchange's value would have closed the race and failed open in that case instead.
Two tests in
packages/host-adapter/test/client.test.tspin both halves against a real handshake and a real memory store:proceedright after this exchange storesdeny. The call must returndeny. It failed on the old code withproceed.deny. The call must returndeny. It passes on the old code and failed when the last line was changed to return only this exchange's value, so it guards the fallback.bun run typecheckis clean andbun testgives 1112 pass, 1 skip, 0 fail across 1113 tests. The root README said the suite has 1,111 tests, which these two made false, so it now drops the total and keeps the count that matters: withouttrashonPATH, 26 tests still fail, verified by running the suite with it hidden.uv run pytestgives 278 passed, 1 skipped, anduv run mkdocs build --strictis clean.Signed-off-by: Rock Lambros rock@rockcyber.com