Skip to content

fix(hooks): close two residual secret-disclosure channels to the model - #841

Merged
Kyzcreig merged 3 commits into
mainfrom
wt/t_59c0886e-p1-secrets
Sep 22, 2026
Merged

Kyzcreig merged 3 commits into
mainfrom
wt/t_59c0886e-p1-secrets

Conversation

@Kyzcreig

@Kyzcreig Kyzcreig commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #819 (merged as 846ba49). Closes the three residual P1s
Apollo named on t_59c0886e after the FleetReview record for
hermes-agent-pr-819-7945f423 came back with unresolvedP0P1 = 3.

#819 sanitized the shell-hook callback __name__; these are the two
paths it did not cover, both of which render operator-supplied text into
a model-facing refusal.

Reproduced before fixing

Both on the real paths, with __file__ asserted against the worktree
(the shared venv resolves the LIVE tree — the round-2 measurement trap).
Probe: qa-output/round13/repro_p1a_p1b.py.

before after
P1-a hook sh#8ece5304 failed closed: command "/bin/sh -c 'export TOK=<planted>; echo hi" cannot be parsed: No closing quotation hook sh#8ece5304 failed closed: command cannot be parsed (ValueError)
P1-b pre_tool_call plugin callback functools.partial(<function _slow at 0x…>, '<planted>') timed out after 0.407s pre_tool_call plugin callback partial(_slow) timed out after 0.402s

P1-a — agent/shell_hooks.py

An unparseable command is exactly the shape that still holds an
inline credential (sh -c 'export TOK=… with the quote left open).
_spawn put the raw command into result["error"], and
_evaluate_result hands that to _fail_closed_block as the model-facing
reason — so hook_display_name sanitized the hook's name while the raw
command walked in through the reason. Argus's 15 adversarial shapes in
round 8 were all parseable, so this path was never exercised.

Now reports the exception class only. Swept the two sibling
str(exc) producers (spawn / communicate) for the same reason —
exception text routinely embeds argv. All six result["error"]
producers are now secret-free; the raw command stays on the log channel
(6 sites, unchanged) for operators.

P1-b — hermes_cli/plugins.py

getattr(cb, "__name__", repr(cb)) fell through to repr() for any
callback without a __name__, and functools.partial's repr renders
every bound argument. One _callback_label() helper now covers all
three sites (:5709 model-facing via both fail-closed paths,
:5955/:6141 log): __qualname__/__name__, else the wrapped
function's name for partials, else <type>@id — never repr.

Labels stay diagnostically useful: partial(_policy), hook sh#8ece5304.

P1-c — regressions

Two tests driven through the real consumers
(resolve_pre_tool_block / _evaluate_result), each asserting the
sentinel is absent from both fail-closed paths while the block itself
is preserved and the hook stays identifiable.

Sentinel local is named planted (not secret) per the round-7 gitleaks
convention; .gitleaks.toml is byte-unchanged.

Verification

  • 314/314 across 10 plugin/hook suites (test_plugins,
    test_shell_hooks, test_shell_hooks_fail_closed, _consent,
    _tree_kill, test_plugin_event_bus, test_hooks_cli,
    test_subagent_stop_hook, test_plugin_dev, test_agent_plugins).
  • Mutation, both arms independent — reverting P1-a turns
    test_unparseable_hook_command_is_not_disclosed RED by name while the
    P1-b test stays green, and vice versa for
    test_refusals_do_not_disclose_a_callback_without_a_name. Tree
    restored byte-exact after each (git status --porcelain clean).
  • ruff check clean; gitleaks protect --staged — no leaks.
  • Nothing in the repo pins the changed strings.

Fail-closed policy is unchanged: a hook that cannot be parsed, or a
callback that times out, still blocks — only the wording changed.

Base: fork/main @ a3e3c16, with #819's merge 846ba49 confirmed as
an ancestor.

Card: t_59c0886e. Not enqueued and not marked ready — Apollo lands this
via fleet-merge.sh against the FleetReview record.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

PR #819 sanitized the shell-hook callback __name__ but left two paths
that still render operator-supplied text into MODEL-facing refusals.
Both reproduced on the real paths before fixing (qa-output/round13).

P1-a  agent/shell_hooks.py — an unparseable command (the shape that
      still HOLDS an inline credential: `sh -c 'export TOK=...` with
      the quote left open) put the raw command into result["error"],
      which _evaluate_result hands to _fail_closed_block as the
      model-facing reason. Measured before:
        hook sh#8ece5304 failed closed: command "/bin/sh -c 'export
        TOK=<planted>; echo hi" cannot be parsed: No closing quotation
      Now reports the exception CLASS only. Swept the two sibling
      `str(exc)` producers (spawn / communicate) for the same reason:
      exception text routinely embeds argv. The raw command stays on
      the log channel, unchanged, for operators.

P1-b  hermes_cli/plugins.py — getattr(cb, "__name__", repr(cb)) fell
      through to repr() for any callback without a __name__, and
      functools.partial's repr renders every bound argument. Measured
      before:
        pre_tool_call plugin callback functools.partial(<function
        _slow at 0x...>, '<planted>') timed out after 0.407s
      One _callback_label() helper now covers all three sites
      (:5709 model-facing, :5955/:6141 log): __qualname__/__name__,
      else the WRAPPED function's name for partials, else
      <type>@id — never repr. Labels stay diagnostic:
      `partial(_policy)`, `hook sh#8ece5304`.

P1-c  two regressions, driven through the real consumer
      (resolve_pre_tool_block / _evaluate_result), asserting the
      sentinel is absent from BOTH fail-closed paths while the block
      itself is preserved and the hook stays identifiable.

Verified: 314/314 across 10 plugin/hook suites; ruff clean; gitleaks
clean with .gitleaks.toml byte-unchanged. Mutation of either fix turns
the matching regression RED by name.
@Kyzcreig Kyzcreig closed this Sep 21, 2026
@Kyzcreig Kyzcreig reopened this Sep 21, 2026
… attempts — infra outage shape, not a verdict); tree unchanged
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

FleetReview

Reviewed with 2 of 3 model families — openai unavailable.

Confidence: 3/5

Findings

  • P1 agent/outbound_webhooks.py:399 — New _callback_label still leaks a webhook credential to the model: outbound_webhook callbacks synthesize __name__ from the raw webhook URL
  • P2 agent/shell_hooks.py:564 — Collapsing result["error"] to the exception class name also blinds the operator-facing hermes hooks test / doctor output
  • P2 hermes_cli/plugins.py:504 — _callback_label fallback puts a CPython heap address in a model-facing string and is not "stable" as documented

FleetReview provenance · models: C=claude-code-opus-5, D=grok-4.6, G=grok-4.6 · cost: $6.50 · duration: 12m 42s · rounds: 1 · files examined: 3

…ror channels

FleetReview P1 on #841: a webhook target's URL reached the MODEL through the
fail-closed refusal messages. WebhookTarget.label falls back to self.url when
the optional `name:` is omitted, _make_callback synthesized __name__ from it,
and _callback_label trusts __name__ — so a Slack/Discord bearer URL was
interpolated into both the timeout and suppression blocks. Third sibling of
the shell-hook/partial pair #841 already closed.

Fixed at the PRODUCER, mirroring shell_hooks.hook_display_name:
WebhookTarget.display_label returns the operator's `name:` or `webhook#<sha256
[:8]>`, never the URL. `label` still yields the URL for the log channel.

_callback_label's docstring claimed __name__/__qualname__ "are developer-chosen
identifiers and safe" — false in this repo (two producers synthesize them from
operator config; register_hook accepts any plugin value). Corrected to state
that each producer sanitizes at the source and this is the last line of defence.

P2, same pass:
- shell_hooks now carries TWO error channels. `error` stays redacted
  (exception class only) for the model; new `error_detail` keeps the full
  diagnostic for the log and `hermes hooks test` / `doctor`, which had been
  blinded — shlex "No closing quotation" and spawn EACCES/ENOEXEC were
  indistinguishable. error_detail never reaches a model-facing string.
- The no-name fallback used `id(callback)`, a CPython heap address that is
  per-process and GC-recyclable, in a model-facing string that the docstring
  called "stable". Now a sha256 digest of the fully-qualified type.

Verified: P1 reproduced on the real path (repro LEAKS -> CLEAN) before/after;
205/205 across 6 focused suites; ruff clean.
@Kyzcreig
Kyzcreig added this pull request to the merge queue Sep 22, 2026
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

FleetReview

Reviewed with 2 of 3 model families — openai unavailable.

Confidence: 3/5

Findings

  • P2 agent/outbound_webhooks.py:163 — display_label / hook_display_name digests are unsalted commitments to the secret they replace
  • P2 tests/agent/test_shell_hooks.py:703 — test_spawn_failure_detail_distinguishes_errno is vacuous — its assertions never execute on the path it claims to cover
  • P0 tests/hermes_cli/test_plugins.py:1599 — New anonymous-callback test asserts a branch that class instances never reach (__qualname__ is inherited from the class __dict__)

FleetReview provenance · models: C=claude-code-opus-5, D=grok-4.6, G=grok-4.6 · cost: $10.77 · duration: 1h 29m 37s · rounds: 4 · files examined: 6

Merged via the queue into main with commit 51aba33 Sep 22, 2026
96 of 98 checks passed
@Kyzcreig
Kyzcreig deleted the wt/t_59c0886e-p1-secrets branch September 22, 2026 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant