Skip to content

feat(miner-governor): PreToolUse-hook-enforced house rules (#2343) - #5031

Merged
JSONbored merged 2 commits into
mainfrom
feat/miner-pretooluse-hook-house-rules-2343
Jul 11, 2026
Merged

feat(miner-governor): PreToolUse-hook-enforced house rules (#2343)#5031
JSONbored merged 2 commits into
mainfrom
feat/miner-pretooluse-hook-house-rules-2343

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • This is a hard security boundary, not a bookkeeping task, per the issue's own framing: prompt-level instructions alone are not a safety boundary for a coding agent with arbitrary tool-use ability, and a bypassPermissions-configured session ignores interactive approval prompts entirely.
  • Checked for existing scaffold first, per the issue's explicit instruction — and found two directly relevant pieces already shipped:
  • Verified the "even under bypassPermissions" claim against Anthropic's own current documentation (code.claude.com/docs/en/agent-sdk/permissions) rather than assuming: hooks are evaluated first, before deny rules, ask rules, the permission-mode check, and allow rules. Quoting directly: "Bypass permissions mode... Hooks still execute and can block operations if needed." and "Deny rules (disallowed_tools), explicit ask rules, and hooks are evaluated before the mode check and can still block a tool." This module does not implement that guarantee — the SDK does. This module's only job is to return a correctly-shaped, fail-closed deny decision every time.
  • New packages/gittensory-miner/lib/pretooluse-hook.js exports buildHouseRulesPreToolUseHook(config, options), which returns a real Agent SDK PreToolUse hook callback: translates the SDK's { tool_name, tool_input } input shape into evaluateDenyHooks's { name, input } shape, returns the exact documented hookSpecificOutput: { hookEventName: "PreToolUse", permissionDecision: "deny", permissionDecisionReason } shape on a match (or {} to allow), records every denial to the governor ledger with the specific rule's reason, and fails closed (denies) on any internal error — including a ledger-append failure, which must never downgrade a security decision to allow.
  • House rules are sourced from the single, already-auditable DEFAULT_DENY_RULES list by default, or any caller-supplied effective rule set (e.g. resolveEffectiveDenyRules from deny-hook-synthesis.js, merging in maintainer-approved synthesized rules) — this module composes whatever rule set it's given; it does not own deriving one.
  • Also fixes a small pre-existing gap noticed in passing: deny-hook-synthesis.js was missing from the miner package's node --check build-validation chain (added alongside the new file, in the same line I was already touching).

Scope

  • Conventional Commit title.
  • Focused: one new file + its test + a two-line build-script fix for a file directly in this issue's own dependency chain.
  • Follows CONTRIBUTING.md.
  • Owner PR — linked issue eligibility gate does not apply. Closes maintainer: PreToolUse-hook-enforced house rules (deny even under bypassPermissions) #2343.
  • No dependency on the other open Miner Wave 3 PRs — this branches directly off main and only imports pre-existing deny-hooks.js/governor-ledger.js.

Validation

  • git diff --check
  • npm run typecheck — clean.
  • npm run build in packages/gittensory-miner (node --check, including the new pretooluse-hook.js and the now-checked deny-hook-synthesis.js) — clean.
  • vitest run on the new suite plus every deny-hooks-adjacent suite (miner-pretooluse-hook, miner-deny-hooks, deny-hooks-fixtures, miner-cli-deny-check, miner-deny-hook-synthesis, miner-governor-ledger) — 64/64 passing.
  • npm run test:coverage, test:workers, build:mcp, test:mcp-pack, ui:*, npm audit — not re-run locally; this PR touches only packages/gittensory-miner/** and test/unit/**, and per CONTRIBUTING.md only src/** is Codecov-measured. CI runs them authoritatively.

Safety

  • No secrets, wallet/hotkey/trust-score/reward data anywhere.
  • Fail-closed is tested, not just asserted: one test forces a real exception through the actual evaluateDenyHooks call (a Proxy whose matcher getter throws, rather than a test-only seam added to production code) and confirms a deny; another forces the governor-ledger append itself to throw and confirms the deny still happens.
  • Reuses the SDK's own documented deny/allow output shape exactly (verified against current docs, not recalled from training data) — a malformed hookSpecificOutput shape would silently fail to block anything, which would be far worse than an obviously-broken build.
  • This PR builds the interception primitive but does not wire it as a default anywhere — no current call site constructs a live coding-agent attempt end-to-end yet (that's the not-yet-built orchestration loop, maintainer: local create->score->self-review iterate-loop orchestrator (the control-flow core) #2333/maintainer: wire the submission-gate trigger into the harness driving loop #2337). driver-factory.ts's existing optional hooks parameter is the natural attachment point once that loop exists; forcing a default wiring now would mean editing a file with no real caller yet, which isn't this issue's job.
  • No UI changes. No docs/schema changes needed (no new config surface).

Notes

Wraps the existing pure evaluateDenyHooks decision function (deny-hooks.js,
interception point CodingAgentDriver sessions register via
options.hooks.PreToolUse (driver-factory.ts and agent-sdk-driver.ts already
thread an optional `hooks` parameter through for exactly this purpose,
explicitly citing #2343 as the attachment point).

Per Anthropic's own documented permission-evaluation order, hooks run FIRST
-- before deny rules, ask rules, the permission-mode check, and allow rules
-- and hook denials are enforced even under permissionMode: bypassPermissions
("Hooks still execute and can block operations if needed"). This module
does not implement that guarantee; the SDK does. Its own job is only to
return a correctly-shaped, fail-closed deny decision every time.

House rules are sourced from the existing single, auditable
DEFAULT_DENY_RULES list (or a caller-supplied effective set, e.g. merging in
maintainer-approved rules from deny-hook-synthesis.js) -- not duplicated
here. Every denial is recorded to the governor ledger with the specific
rule's reason; an internal error (including a ledger append failure) fails
closed to deny, never silently allows.

Also fixes a pre-existing gap: deny-hook-synthesis.js was missing from the
miner package's node --check build-validation chain.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 11, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui db87103 Commit Preview URL

Branch Preview URL
Jul 11 2026, 10:03 AM

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.36%. Comparing base (dcc1601) to head (db87103).
⚠️ Report is 8 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5031   +/-   ##
=======================================
  Coverage   94.36%   94.36%           
=======================================
  Files         458      458           
  Lines       39255    39255           
  Branches    14322    14322           
=======================================
  Hits        37042    37042           
  Misses       1558     1558           
  Partials      655      655           
Flag Coverage Δ
shard-1 46.73% <ø> (-0.02%) ⬇️
shard-2 33.27% <ø> (-0.12%) ⬇️
shard-3 31.26% <ø> (-0.22%) ⬇️
shard-4 32.58% <ø> (-0.22%) ⬇️
shard-5 34.48% <ø> (+1.00%) ⬆️
shard-6 45.33% <ø> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-11 10:09:29 UTC

4 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This adds a `PreToolUse` hook callback (`buildHouseRulesPreToolUseHook`) that wraps the existing `evaluateDenyHooks` decision function and ledger-logs denials, intended to close #2343 by providing the live interception point for house-rule enforcement even under `bypassPermissions`. The module itself is correct and fails closed on internal/ledger errors, but the diff never wires this hook into any actual driver construction call site (e.g. passing it to `createAgentSdkCodingAgentDriver({ hooks })`), so the claimed security boundary is not actually active anywhere yet. The `package.json` build-script change also silently drops the `node --check lib/governor-kill-switch.js` syntax check, an unrelated regression in coverage for a security-relevant module.

Blockers

  • The diff only adds `pretooluse-hook.js` and its test but never calls `buildHouseRulesPreToolUseHook()` from any real driver-construction site (e.g. wherever `createAgentSdkCodingAgentDriver` is invoked in the miner runtime), so despite the PR's framing as 'the live interception wiring itself,' the hook is built but never registered — the house-rule enforcement described is not actually active on any real session.
  • packages/gittensory-miner/package.json's `build` script silently removes `node --check lib/governor-kill-switch.js` while adding the two new file checks, dropping syntax-check coverage for an unrelated, security-relevant kill-switch module with no explanation in the PR description.
Nits — 6 non-blocking
  • pretooluse-hook.js:70 nests to depth 5 (try/if/ternary chain) per the flagged control-flow depth — consider extracting the `toolName`/`toolInput` extraction into a small helper to flatten it.
  • The extensive block comments in pretooluse-hook.js re-explain SDK behavior (hooks run before bypassPermissions) that lives in Anthropic's docs, not this repo's invariants — consider trimming to the parts this module actually guarantees.
  • denyOutput()'s `reason` string is echoed directly from `verdict.blockedBy?.reason`; worth confirming deny-hooks.js's reasons never embed untrusted tool-input content that could bloat/pollute the governor ledger payload.
  • Add (or point to) the call site in agent-sdk-driver.ts / wherever the miner CLI starts an agent session that actually passes `{ hooks: { PreToolUse: [{ hooks: [buildHouseRulesPreToolUseHook(...)] }] } }` — without it this PR doesn't close maintainer: PreToolUse-hook-enforced house rules (deny even under bypassPermissions) #2343, it only stages the piece.
  • Restore `node --check lib/governor-kill-switch.js` in the build script alongside the new entries, or explain in the PR description why it was intentionally dropped.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2343
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 47 registered-repo PR(s), 39 merged, 428 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 47 PR(s), 428 issue(s).
Gate result ✅ Passing No configured blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 47 PR(s), 428 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

An AI reviewer flagged a likely defect, but its confidence was below this repository's configured close-confidence floor, so this is held for a maintainer to confirm instead of closing automatically. Resolve the flagged defect (see the review notes), or ask a maintainer to override.

…etooluse-hook

Measured coverage (vitest v8, scoped include override -- this file
sits outside the repo's default coverage.include) showed
pretooluse-hook.js at 76.92% branch. Two of the six gaps were real:

- The `input && typeof input === "object"` boundary check (input is
  typed `unknown`) never saw a malformed non-object input exercised.
- A custom rule omitting `reason` never exercised the generic-message
  fallback.
- A deny with a non-string tool_name (matcher: "*" still fires --
  matcherMatches substitutes "" before testing) never exercised the
  ledger payload's null-toolName fallback.
- The internal-error catch's `error instanceof Error` check was only
  ever exercised with real Error instances, never the String(error)
  fallback for a non-Error thrown value.

The remaining two (both on `blockedBy?.matcher ?? null`) were dead
code, not undertested: ruleMatches gates every match on
`typeof rule.matcher === "string"`, so a rule can never become
`blockedBy` without a defined string matcher -- the fallback could
never fire. Simplified to `verdict.blockedBy.matcher` per the
project's "don't add validation for scenarios that can't happen"
convention, rather than papering over it with a contrived test.

pretooluse-hook.js now measures 100/100/100/100
(stmts/branch/funcs/lines), up from 76.92% branch (24 real branches,
down from 26 after removing the two dead ones).
@JSONbored

Copy link
Copy Markdown
Owner Author

Pushed a coverage-only follow-up commit (db87103): measured coverage (vitest v8, scoped include override since this file sits outside the repo's default `coverage.include`) found `pretooluse-hook.js` at 76.92% branch.

Four of the six gaps were real and got tests:

  • The `input && typeof input === "object"` boundary check (input is typed `unknown`) never saw a malformed non-object input.
  • A custom rule omitting `reason` never exercised the generic-message fallback.
  • A deny with a non-string `tool_name` (`matcher: "*"` still fires) never exercised the ledger payload's null-toolName fallback.
  • The internal-error catch's `error instanceof Error` check was only ever exercised with real Error instances, never the `String(error)` fallback.

The remaining two (both on `blockedBy?.matcher ?? null`) were dead code, not undertested: `ruleMatches` gates every match on `typeof rule.matcher === "string"`, so a rule can never become `blockedBy` without a defined string matcher -- the fallback could never fire. Simplified to `verdict.blockedBy.matcher` rather than writing a contrived test to hit unreachable code.

`pretooluse-hook.js` now measures 100/100/100/100 (stmts/branch/funcs/lines). 56/56 tests pass across this file and its siblings (deny-hooks, deny-hook-synthesis, deny-hooks-fixtures).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

maintainer: PreToolUse-hook-enforced house rules (deny even under bypassPermissions)

1 participant