fix(shell_safety): repair two regressions from #18 and the test that missed them

[jasonxi89]

Draft — follow-up to #18, from reviewing my own merged change. Two of these are behavior regressions I introduced there; the third is a test that passed for the wrong reason.

Draft because item 1 is a judgement call about how this bundle is versioned, and I would rather agree on that before it lands.

1. The bundle can now silently disable shell_safety

#18 made this package call register_callback(..., fail_closed=True), a keyword that only exists in code-puppy>=0.0.735. This package declares no dependency on code-puppy at all, so pip install -U code-puppy-core-plugins against an older core is a supported install path today.

What happens then:

TypeError: register_callback() got an unexpected keyword argument 'fail_closed'

and code_puppy/plugins/__init__.py handles a failing entry point like this:

except Exception as exc:
    logger.error("Unexpected error loading installed plugin %s: %s", ...)

Logged, skipped, startup continues. The outcome is not a crash — it is shell_safety not loading at all, which is strictly worse than the fail-open #18 set out to fix.

Fix: declare the floor.

dependencies = [
    "code-puppy>=0.0.734",
    ...
]

0.0.734 is the first published core with the keyword — scanning sdists, 0.0.733 has no occurrence and 0.0.734 has it. Verified against real isolated installs:

install	result
code-puppy==0.0.733 + this bundle	resolver refuses: "your requirements are unsatisfiable"
code-puppy==0.0.734 + this bundle	installs; register_callback(..., fail_closed=True) succeeds
old core already present, bundle installed on top	resolver upgrades the core rather than leaving the broken pair
code-puppy==0.0.734 pinned, floor removed	installs — so the first row's failure comes from the constraint, not from 0.0.734 being unusable

The suite also asserts the capability rather than only the declaration: a dependency line helps only if the resolver acted on it, so a test inspects the installed register_callback for the keyword and for its opt-in default. A wrong floor or a bypassed resolver fails there instead of at a user's registration.

The judgement call for you: a floor is the minimum. If the bundle is expected to track core closely you may want a ceiling or a compatible-release pin instead, and a floor does nothing if a future core removes or renames the keyword. Happy to change it to whatever matches how you release the two together.

I deliberately did not add a try/except TypeError fallback to a two-argument call: that would silently re-register with fail_closed=False, quietly restoring the exact fail-open this was meant to remove.

2. Safety being off could still refuse a command

shell_safety_callback read model before consulting yolo_mode:

current_model = get_global_model_name()      # read first
if is_oauth_model(current_model): return None
yolo_mode = get_yolo_mode()
if not yolo_mode: return None                # abstain — but model was already read

After #18, an unreadable model value therefore refused commands for users who had turned automatic assessment off entirely. With yolo_mode off the user reviews every command by hand, and this guard should not be able to affect the outcome at all.

Fix: read yolo_mode first and abstain before touching anything else. current_model is only used for the OAuth check, so nothing downstream moves. Every read left above the try now happens only when safety is genuinely on — which is exactly the surface fail_closed is meant to cover.

A mirror regression I checked for and could not reproduce. Swapping the order raises the symmetric question: does OAuth model + unreadable yolo_mode now get refused where it previously passed? Measured both orderings — it does not, because an unreadable yolo_mode also fails on the caller's own path, so the command stops regardless of what this guard decides:

config	model-first (before)	yolo-first (after)
OAuth model + yolo_mode=%(nope)s	did not run	did not run
non-OAuth + yolo_mode=%(nope)s	did not run	did not run
OAuth model + unreadable threshold	did not run	did not run

The underlying observation still stands as design feedback: OAuth and yolo_mode=false are two independent reasons this guard does not apply, and ordering them means a broken read for one can mask the other. Making that order-independent is a larger change than this PR, and today it is unobservable. Happy to follow up if you want it.

3. The test in #18 passed for the wrong reason

The fixture used yolo_mode=%. Measured across all three unprotected reads, one isolated process per row:

config	no opt-in	with opt-in
yolo_mode=%	did not run — the exception escaped to the caller instead	refused
model=%(missing)s	ran, unassessed	refused
safety_permission_level=%(nope)s	ran, unassessed	refused

yolo_mode is also read on the caller's own path, so corrupting it stops the command for an unrelated reason and the guard's failure is masked. The other two are not: the rest of the system stays healthy while the assessment dies, and the command runs unchecked.

So the one config the original fixture used is the one config that proves nothing: test_the_command_never_reaches_the_shell asserted an absent sentinel in a case where the sentinel would have been absent anyway. It could not have caught a regression.

Rebuilt around the case that discriminates, plus two things the original lacked:

• a positive control — with a healthy config the same command does create the sentinel, so an absent sentinel means something
• a pre-change assertion — without the opt-in the command runs (assert sentinel.exists()), so the pair brackets the behavior

The yolo_mode=% case is kept, retitled to what it actually shows: the opt-in converts an escaping exception into a structured refusal.

Verification

Every figure below is one measurement per interpreter process, driving the real run_shell_command and watching for a side effect on disk:

measurement	result
positive control — healthy config, no opt-in	ran
safety_permission_level unreadable, no opt-in — the bug	ran, unassessed
safety_permission_level unreadable, with opt-in — the fix	refused
yolo_mode=false + unreadable model, model-first — the regression	refused
yolo_mode=false + unreadable model, yolo-first — item 2	ran

Every table in this description was re-measured this way after the first draft; an earlier version of the item 3 table was taken from a shared-process run and reported model=%(missing)s as not running, which is wrong.

One measurement per process matters here: code_puppy.config caches across reads within a process, so a parameterised matrix sharing one interpreter reports stale values. An earlier draft of this table was measured that way and disagreed with itself between runs; these numbers are from isolated processes and reproduce.

Each of the ten tests also passes when run alone, not only as a suite.

Suite: 1982 passed, 2 skipped. ruff check / ruff format clean.

Framing

Worth being precise about severity: the trigger is a local config that cannot be read, not attacker-controlled input. This is fail-closed hardening and a regression fix, not a remotely exploitable vulnerability — I would rather #18 and this PR were not read as more than they are.