fix(callbacks): a security callback that crashes must not read as approval

[jasonxi89]

Moved here from the Walmart-internal fork at the maintainers' request — the defect is entirely in open-source code.

Problem

_trigger_callbacks isolates callback errors: it logs the exception and appends None to the results. The two phases that carry a {"blocked": True} protocol read None as approval. code_puppy/tools/command_runner.py states it outright:

# Callbacks can return None (allow) or a dict with blocked=True (reject)

A security callback that fails to complete is therefore indistinguishable from one that approved.

Today the outcome depends only on whether each plugin remembered to wrap itself, and the three guards in code_puppy_core_plugins disagree:

Guard	Handles its own exceptions	On crash
shell_safety	yes — returns {"blocked": True}	deny
force_push_guard	only AttributeError/OSError	allow
destructive_command_guard	no	allow

Two of those are deliberate. shell_safety chose deny in code. destructive_command_guard/AGENTS.md chose allow in writing — "fail closed on ambiguity, fail open on our own errors." force_push_guard states nothing. The framework gives no way to express the choice.

It is reachable, not theoretical. shell_safety_callback reads three config values before its own try. ConfigParser interpolates lazily, so a value like yolo_mode=% parses cleanly — the corruption quarantine never sees it — and raises only when the option is read. I verified each of these raises rather than returning a default:

yolo_mode=%                      -> get_yolo_mode()               InterpolationSyntaxError
model=%(missing)s                -> get_global_model_name()       InterpolationMissingOptionError
safety_permission_level=%(nope)s -> get_safety_permission_level()  InterpolationMissingOptionError

The guard raises, the dispatcher returns None, and the command runs with no safety assessment.

Change

register_callback(phase, func, fail_closed=True) lets a callback declare that its failure means deny. The default is unchanged, so every existing registration behaves exactly as before.

• Reported as a result, not by re-raising. The existing raise_on_error cannot serve this: pydantic_patches wrapped the pre_tool_call block in except Exception: pass, so a raised deny was swallowed and the tool ran anyway.
• Keyed by (phase, callback), so one callable registered on several phases can hold a different policy on each.
• Rejected on phases whose consumers do not act on a block result, rather than injecting a dict that load_prompt or git_branch_provider would misread.
• The synthesized message omits the exception text, which reaches the user and the model and may carry paths, command lines, or tokens. Callback name and exception type are included; the full traceback stays in the log.
• The sync trigger's "async callback reached from a running loop" branch is covered too — undecided is not unopposed.

Second, independent fail-open

A hook could already return an explicit {"blocked": True} and still have the tool run, with no crash involved: a non-string reason made "[BLOCKED]" in raw_reason raise inside the same broad except. Reproduced against the pre-change logic:

{"blocked": True, "reason": 123}   -> TOOL ACTUALLY RAN
{"blocked": True, "reason": "..."} -> ERROR: blocked: ...

_block_reason is now total. Extraction, coercion and rendering are all inside the guard, because .get and a plugin object's truthiness can raise as easily as __str__. The block decision moved out of the broad except, so a failure while emitting the warning no longer turns a deny into an allow.

Scope

No plugin in this repository opts in — the guards live in code_puppy_core_plugins, so wiring shell_safety is a separate change I'm happy to open next.

destructive_command_guard should stay as it is regardless: its fail-open is a documented product decision for its owners, not something to change from an infrastructure PR.

Testing

• tests/test_callbacks_fail_closed.py — 25 cases: default behavior unchanged, both dispatchers plus the unawaitable-async branch, phase restriction, per-phase policy isolation, repeat-registration tightening, raise_on_error precedence, no exception text in user-facing output, registry housekeeping, and _block_reason against non-string reasons, a hostile __str__, and a hostile __bool__.
• tests/conftest.py now snapshots and restores _fail_closed_callbacks beside _callbacks; restoring one without the other would hand the next test callbacks whose policy had quietly gone missing.
• Full suite: 7335 passed, 28 skipped. One failure, tests/plugins/test_aws_bedrock.py::TestSupportsAdaptiveThinking::test_supports_adaptive_thinking[opus_4_5_minor_version_not_adaptive], is pre-existing and order-dependent: a clean main full run reproduces it (7310 passed, same 1 failure), while running that file alone passes on both.
• ruff check and ruff format clean on the changed files.

[jasonxi89]

@thomwebb — could I get your eyes on this when you have a moment? (I can't add reviewers directly from a fork.)

Flagging it because unlike a straightforward bug fix this one needs a call from a maintainer rather than just a correctness check:

• It adds a public parameter to register_callback, so the API surface is a decision, not just an implementation detail.
• No plugin in this repo opts in — the guards live in code_puppy_core_plugins. Wiring shell_safety there is a follow-up I'll open once this lands, but that does mean the mechanism arrives without a consumer here.
• It deliberately does not touch destructive_command_guard, whose AGENTS.md documents fail-open on internal errors as intentional. If you'd rather that policy change too, that's a separate conversation with its owners.

The part that is just a bug, and stands on its own regardless of the above: a hook returning {"blocked": True, "reason": <non-string>} currently runs the tool anyway, because the comparison raises inside except Exception: pass. Happy to split that out into its own smaller PR if it would be easier to land first.

CI is green (macos 3.13 / windows-encoding / quality) and the branch is mergeable.