Skip to content

Fail closed by default - #495

Closed
bar-capsule wants to merge 4 commits into
refimpl/adapters-14-review-fixesfrom
refimpl/adapters-15-fail-closed
Closed

bar-capsule wants to merge 4 commits into
refimpl/adapters-14-review-fixesfrom
refimpl/adapters-15-fail-closed

Conversation

@bar-capsule

@bar-capsule bar-capsule commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #489. Last slice of the stack. The full stack is listed in #22.

What changed

The leads decided on 3 November that the default is fail-closed, configurable either way.

  • A session whose handshake gets no answer refuses its steps unless the hook sets ACS_STARTUP_POSTURE=proceed (§4.1). Before, an unset startup posture followed ACS_DEFAULT_DENY and proceeded. A hook whose adapter cannot load follows the same rule.
  • The example Guardian declares on_decision_failure: deny; ACS_ON_DECISION_FAILURE=proceed makes it declare proceed.
  • In a negotiated session, a step with no usable decision follows the posture the Guardian declares, as §6.4 says, and ACS_DEFAULT_DENY=1 denies it whatever the Guardian declares. The wired gate hooks keep ACS_DEFAULT_DENY=1 in their command.
  • acs-nat-wire writes default_deny: true, and --all-fail-open writes false.
  • --all-fail-open in acs-claude-code-wire and acs-cursor-wire writes ACS_DEFAULT_DENY=0 ACS_STARTUP_POSTURE=proceed on every hook; before, it wrote no posture.
  • Every posture table, setting and walkthrough in the docs states the new default, and the two Guardian-down walkthroughs show the output of a real run.
  • A Claude Code PreToolUse deny also carries its reason in additionalContext. When several PreToolUse hooks deny, Claude reads only the permissionDecisionReason of the one that finishes last, so a project's own hook could hide the ACS reason; live, with a second hook that denied 2 s after ACS, Claude received only the project's reason before the change and both after it.

§6.4 and handshake.json still give proceed as the default of on_decision_failure, so a ServerHello without the field still negotiates proceed; that changes with the §6.4 PR for #32.

Which issue does this implement

Part of #134

I tested this

At this slice's top (243b2682), with claude removed from PATH and ACS_LIVE_CLAUDE and ACS_SPEC_DIR unset: 648 passed and 3 expected skips with the NAT extra, 510 passed and 3 expected skips without it. The skips are the two opt-in live Claude Code tests and the manual Cursor procedure. The installed-wheel check passes on Python 3.11 and 3.12, lint-imports keeps both contracts, the repository suite passes 1205 with 1 skip, and mkdocs build --strict passes. CI passes on this PR.

Live run of the wheel built from this slice's code (sha256 12ccf4d71445b7617e03e9eede82a62f4391dddb22d566c2eadd3aab204cef0c) in Claude Code 2.1.318 and NAT 1.7.0, every request through the recording proxy and the checker: with the Guardian down when the session starts, every hook refused its step (session_not_started_fail_closed) and the command did not run; with the Guardian silent on tool results, Claude's reply left out the file's contents when the Guardian declared deny (decision_failure_fail_closed) and included them when it declared proceed (fail_open_bypass). Across the full harness, 15 recordings, the checker found 0 problems.

Base branch

Stacked on #489, not on integration. It lands through #22; see the stack in #22.

Type of change

  • Reference implementation or adapter

Checklist

  • Commits are signed off with git commit -s (required by the DCO)
  • Prose follows STYLE.md
  • No secrets, tokens, or internal URLs in the diff

Security

A session with no negotiated Guardian, and a hook whose adapter cannot load, deny their steps by default, and the example Guardian declares on_decision_failure: deny.

A session whose handshake gets no answer, and a hook whose adapter cannot load, refuse their steps unless ACS_STARTUP_POSTURE=proceed. The example Guardian declares on_decision_failure: deny unless ACS_ON_DECISION_FAILURE=proceed, acs-nat-wire writes default_deny: true, and --all-fail-open in acs-claude-code-wire and acs-cursor-wire writes ACS_DEFAULT_DENY=0 and ACS_STARTUP_POSTURE=proceed. In a negotiated session, a step with no usable decision follows the Guardian's declared posture, or is denied under ACS_DEFAULT_DENY=1.

Signed-off-by: Bar Kaduri <bar@capsule.security>
It denies the step unless the hook sets ACS_STARTUP_POSTURE=proceed.

Signed-off-by: Bar Kaduri <bar@capsule.security>
When several PreToolUse hooks deny, Claude reads only the reason of the one that finishes last, so another hook's deny hid the ACS reason. The deny now also carries it in additionalContext, which reaches Claude either way.

Signed-off-by: Bar Kaduri <bar@capsule.security>

@drk-ram drk-ram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at head 243b2682e20c1894aa4e0b4493540bd5256518b2. Approving this slice.

Scope. This is the adapters' half of the #32 default ruling (the leads' 3 November decision, restated in Bar's comment on #32). All 31 changed files are under adapters/. No schema or spec text moves here: §4.1, §6.4, §17.1, the on_decision_failure default in handshake.json, and error.signature in response-envelope.json are untouched, and the description says so. Those belong to the spec PR Bar said he would write for #32 (item 4 of his answer), so I reviewed this for what it does: whether the adapters and the example Guardian fail closed by default, whether the docs say the same thing, and whether it keeps the invariants that ruling needs.

What I checked

  • Config.startup_proceeds now returns true only for a trimmed, case-insensitive proceed. Unset, empty, refuse and a typo such as procede all refuse. The table in test_config.py covers each.
  • _bootstrap._settle, the path for a hook whose adapter cannot load, now denies when fail_closed, posture.deny_on_failure or not startup_proceeds. I drove run_hook directly with an unloadable adapter module and an event on stdin. At this head: no setting denies; ACS_DEFAULT_DENY=0 and ACS_DEFAULT_DENY=1 deny; ACS_STARTUP_POSTURE=proceed proceeds, with or without ACS_DEFAULT_DENY=0; proceed with ACS_DEFAULT_DENY=1 denies; ACS_STARTUP_POSTURE=refuse denies; ACS_DISABLED=1 still bypasses. At the base (fcc58fe) no setting, ACS_DEFAULT_DENY=0 and an explicit ACS_STARTUP_POSTURE=refuse all proceeded. The last one was a real hole: the hook ignored a posture the operator had set to refuse. This slice closes it.
  • The example Guardian reads ACS_ON_DECISION_FAILURE once at import and only a trimmed proceed opens it. "", deny, open and 0 all stay deny, and the new test checks that in a subprocess so the import-time read is real. The ServerHello it builds carries the value.
  • Wiring: --all-fail-open writes both ACS_DEFAULT_DENY=0 and ACS_STARTUP_POSTURE=proceed on every Claude Code and Cursor hook, gates included, and the default wiring writes neither (AllFailOpenOpensEveryHook asserts both directions). acs-nat-wire writes default_deny: true by default and false under --all-fail-open; the two flags are now a mutually exclusive group and --default-deny stays accepted, so existing scripts do not break.
  • Opting out takes both sides: with the example Guardian declaring deny, --all-fail-open still denies a step with no usable decision, and the help text and the Claude Code and Cursor READMEs say so.
  • The PreToolUse additionalContext is added only on deny, and is built from the same reason string as permissionDecisionReason, so it exposes nothing the Guardian's reason did not already carry. The test pins the equality for both a Guardian deny and a decision failure.
  • Every posture table, setting row and walkthrough I found in the adapter READMEs, mapping.md files, settings.json.example and the wire.py help text now states the new default. I found no stale "fail-open is the §6.4 default" claim left in the changed docs.

Validation at this head (Windows host, uv, Python 3.11.9, claude not on PATH, ACS_LIVE_CLAUDE and ACS_SPEC_DIR unset)

  • Repository root, uv run pytest -q: 1185 passed, 1 skipped, 10 failed, 20 errors. All 30 are in tests/test_closing_choice.py, test_fetch_roadmap.py, test_overrides.py, test_pr_intake_workflow.py and test_publish_schemas.py, files this PR does not touch. The same five files give the same 10 failed and 20 errors on integration at bba1b84, so I treat them as host effects.
  • uv run --no-dev mkdocs build --strict: exit 0.
  • Adapters suite with the nat extra: this host cannot run it as shipped, because example_guardian.py imports fcntl and acs.testing.harness uses select.select on a pipe, and the .venv\Scripts console scripts have an .exe suffix the security tests do not expect. With two stand-ins kept outside the tree (a no-op fcntl and a select fallback), twelve of the thirteen test files this PR touches (I left the conformance module out) give 229 passed, 30 failed and 338 subtests passed at this head, against 217 passed and 35 failed at the base. Every failure at this head also fails at the base, so the slice adds none. Selecting the PR's own new or changed tests by name (posture, AllFailOpen, unanswered-handshake, declared-proceed, additional-context, startup-posture and cannot_load cases) gave 15 passed and 4 failed. The 4 are the two cannot_load tests, which need the POSIX console script, and two parallel-startup subtests; all four fail the same way at the base. I replaced the cannot_load pair with the direct run_hook probe above. I did not reproduce the 648/510 counts in the description, and CI is the right witness for those: package (3.11), package (3.12), tests and tests-without-nat are green on this PR.

Non-blocking observations

  1. In both Guardian-down walkthroughs the stdout line sits outside its code fence. adapters/docs/claude_code/README.md (the {"hookSpecificOutput": ...} line right after the closing fence, about line 223) and adapters/docs/cursor/README.md (the {"permission": "deny", ...} line, about line 234) both close the fence after the two ACS_AUDIT lines and leave the JSON as loose paragraph text. Move the closing fence below the JSON line.
  2. ACS_STARTUP_POSTURE=proceed and ACS_DEFAULT_DENY=1 combine differently on the two failure paths. For a session whose handshake got no answer, proceed wins (test_startup_posture pins {"proceed", "DEFAULT_DENY": "1"} as true). For a hook whose adapter cannot load, ACS_DEFAULT_DENY=1 wins (my probe above). It is probably intended, since the second path has no session to apply a posture to, but a reader of the ACS_DEFAULT_DENY row, which says "fail-closed whatever the Guardian declares", would not guess it. One sentence in the settings table would settle it.
  3. The description is right that a ServerHello with no on_decision_failure still negotiates proceed until the §6.4 and handshake.json PR lands. Until then a third-party Guardian that omits the field leaves the non-gate hooks with no local floor. That is the intended staging, but I would link the spec PR from the adapter README's degraded-Guardian paragraph once it exists, so the fallback is visibly temporary.

The behaviour is correct, the opt-out is explicit on both sides, and the tests pin both directions of every change. Approving.

@acs-board-reconciler acs-board-reconciler Bot moved this from Needs triage to In progress in ACS Project Tracker Nov 5, 2026
…e NAT middleware by _type

An unset ACS_DEFAULT_DENY, and NAT's default_deny, now mean deny. Only
ACS_DEFAULT_DENY=0 (default_deny: false) opts into proceed, and a
ServerHello that declares deny still overrides it, so the Guardian can
only tighten the posture.

A Cursor preToolUse MODIFY answers updated_input with no permission key,
so Cursor keeps its sandbox and approval for the rewritten call.

acs-nat-wire finds the ACS middleware by _type: acs_guardian under any
name and lists that name, so an existing guard is never added twice.
--write stops when acs_guardian names a middleware of another type.

Signed-off-by: Bar Kaduri <bar@capsule.security>
@rocklambros

Copy link
Copy Markdown
Collaborator

Closing this slice because PR #22 (the reference adapters for Claude Code, Cursor and NAT) merged into integration as 2630ec2, and that merge carries every slice in this stack. The top of the stack had the same files as PR #22's final commit, so nothing in this slice was left behind. Thanks, Bar, for cutting the adapters into pieces this size, because it made the review possible.

@rocklambros rocklambros closed this Nov 5, 2026
@github-project-automation github-project-automation Bot moved this from In progress to Done in ACS Project Tracker Nov 5, 2026
@bar-capsule
bar-capsule deleted the refimpl/adapters-15-fail-closed branch November 8, 2026 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants