feat: [de-fork S3] HardPolicy kill gate — non-bypassable DDL denies at every dispatcher#1018
feat: [de-fork S3] HardPolicy kill gate — non-bypassable DDL denies at every dispatcher#1018anandgupta42 wants to merge 4 commits into
Conversation
## Summary Stage **S3** of the de-fork spike — **the kill gate**. Enforces **non-bypassable hard denies** (`sql_execute` DDL: `DROP DATABASE`/`DROP SCHEMA`/`TRUNCATE`; bash DDL) at **every model-invoked tool-execution dispatcher**, relocating today's scattered per-tool safety checks into one audited chokepoint. **Behavior-preserving** — these operations already block; S3 consolidates and closes a bypass. - **NEW `packages/opencode/src/altimate/policy/hard-policy.ts`** — pure, synchronous, **total** `HardPolicy.check()`: malformed/missing args return a deny (`policy_internal_error`), never throw, never implicit-allow. Emits a structured **audit record** on every call — this (not the trace) is the enforcement oracle the tests assert against. - **`HardPolicy.check()` inserted before `tool.execute` at every dispatcher** from the S2 route matrix: D1 (registry loop) + D2 (MCP loop) + D6 (direct Task dispatch) in `session/prompt.ts`, D3/D4 in `session/tools.ts`, D5 (BatchTool inner dispatch) in `tool/batch.ts`. Fail-closed init via `assertInitialized()` at the app-runtime composition seam. - **Closes a real bypass:** the fork's resolvers were *discarding* `tool.execute.before`'s return value, so a plugin hook that mutated args (e.g. rewrote a benign `SELECT` into `DROP DATABASE`) executed the mutation **unchecked**. Every site now captures the post-hook **final args** and checks *those*. The batch inner dispatch checks the **inner** tool ID + args (not `toolID="batch"`), closing the nested-dispatch gap. - **On deny:** returns a `hard_policy_denied` tool-error result (or throws in batch) — `execute` and `tool.execute.after` are both skipped. **Scope preserved:** bash DDL patterns are byte-identical to today's `agent.ts` safety table; the sql check reuses `sql-execute.ts`'s classifier + error string. `rm -rf` / `git push --force` / `git reset --hard` correctly stay **ask-tier** (not promoted to hard deny). Also includes `route-sentinels.test.ts` made **line-number- and arg-agnostic** (matches `args` or S3's `finalArgs`) so it passes with or without S3 applied — shared identically with PR #1014, mergeable in any order. ### Review Two independent adversarial reviews (a Claude reviewer + the orchestrator's own trace of every `.execute()` site) found **no bypass** via malformed args, mutated args, or allow-all permission config, and confirmed all 7 safety properties (coverage, final-args, total-function, deny-terminal, config-independence, fail-closed init, behavior-preservation). ## Test Plan - `bun test test/altimate/defork/hardpolicy.test.ts`: **19 pass / 0 fail** — the bypass matrix across D1–D6: DDL denied + not executed at every route, near-miss controls allowed (proves rules aren't over-broad), allow-all config still denies, malformed args fail closed, deny is terminal, mutated-args checked on the post-hook value (audit `finalArgsDigest` asserted). - Always-allow red-proof: stubbing `check()` to always-allow fails every dispatcher-integration test. - `bunx tsc --noEmit -p packages/opencode`: clean for the changed files. ## Checklist - [x] Tests added/updated - [x] Documentation updated (spec §S3 + route matrix) - [ ] CHANGELOG updated — N/A (internal safety guardrail) Closes #1017 --- _This is a security boundary and the spike's decision checkpoint — **held for human go/no-go; do not auto-merge.**_
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
📝 WalkthroughWalkthroughChangesHardPolicy enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant Plugin
participant HardPolicy
participant ToolDispatcher
Model->>Plugin: tool.execute.before(args)
Plugin-->>Model: finalArgs
Model->>HardPolicy: check(finalArgs)
alt denied
HardPolicy-->>Model: policy denial
else allowed
HardPolicy-->>Model: allow
Model->>ToolDispatcher: execute(finalArgs)
end
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| decision = { allow: false, ruleID: INTERNAL_ERROR_RULE_ID, safeReason: INTERNAL_ERROR_SAFE_REASON } | ||
| } | ||
|
|
||
| emitAudit({ |
There was a problem hiding this comment.
WARNING: emitAudit({...}) is outside the try/catch in check(), so a throwing auditSink callback (or an unlikely push/shift failure) propagates out and breaks the module's stated "TOTAL: never throws" contract.
The dispatchers call HardPolicy.check() without a surrounding try/catch (e.g. session/prompt.ts:1634, session/tools.ts:105, tool/batch.ts:110), so an audit-side throw surfaces as an unexpected tool error instead of the clean hard_policy_denied result the chokepoint is supposed to return. auditSink is null by default in production so the live risk is low, but the contract should not rely on that. Wrapping this emitAudit({...}) call in its own try { ... } catch { /* audit failure must not affect the decision */ } keeps the already-computed decision returned regardless of audit-side failures.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| export interface AuditRecord { | ||
| ruleID: string | ||
| dispatcher: Source |
There was a problem hiding this comment.
SUGGESTION: The dispatcher field is redundant with source.
In emitAudit both dispatcher and source are populated from the same local variable (lines 325-327), and no test ever asserts on dispatcher. Since Input.source is the canonical field name and is what every dispatcher passes, dispatcher can be dropped from AuditRecord (and the emitter) without losing any information.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| function emitAudit(record: AuditRecord): void { | ||
| auditLog.push(record) | ||
| if (auditLog.length > MAX_AUDIT_LOG) auditLog.shift() |
There was a problem hiding this comment.
SUGGESTION: auditLog.shift() is O(n) on every emit once the log is full.
check() runs on every tool execution. Once auditLog.length > 1000, every subsequent call pays an O(1000) shift() just to make room. A bounded ring buffer (index + overwrite) or a batched trim (if (overflow > 0) auditLog.splice(0, overflow) once per push, or every N pushes) would make the cap O(1) amortized. Minor, but it is permanent overhead on the tool-execution hot path with no production reader of the log.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Merge Incremental review of commit Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 3ac7b68)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3ac7b68)Status: 1 Issue Found | Recommendation: Merge Incremental review of commit Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 2884d7e)Status: 1 Issue Found | Recommendation: Merge Incremental review of commit Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 412ed1d)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Security boundary assessmentThe PR's core claim — Reviewed by glm-5.2 · Input: 62K · Output: 12.1K · Cached: 834.8K Review guidance: REVIEW.md from base branch |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/opencode/test/altimate/defork/hardpolicy.test.ts (1)
904-908: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the
FileSystem.FileSystemEffect service over rawfs/promises.
NodeFileSystemis already composed into this file's test layer, sowriteTextcan yieldFileSystem.FileSystemand usefs.writeFileStringinsideEffect.geninstead of dynamically importingfs/promiseswithinEffect.promise.As per coding guidelines: "Prefer
FileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/defork/hardpolicy.test.ts` around lines 904 - 908, Update writeText to use the composed FileSystem.FileSystem service via Effect.gen, yielding the service and calling its writeFileString method; remove the dynamic fs/promises import and Effect.promise implementation while preserving the existing path and content inputs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/policy/hard-policy.ts`:
- Around line 216-220: Update emitAudit to isolate the auditSink(record)
invocation from callers by catching and suppressing any exception it raises.
Preserve auditLog recording and trimming, and ensure check() retains its
never-throws contract even when a sink installed through setAuditSink fails.
In `@packages/opencode/src/session/prompt.ts`:
- Around line 1726-1736: Update the policy-deny return object in the prompt
handling flow to include empty content and attachments arrays, matching the
result shape used by session tools. Preserve the existing title, output,
metadata, and denial values.
In `@packages/opencode/test/altimate/defork/route-sentinels.test.ts`:
- Around line 149-152: Update the D3/D4 “SessionTools.resolve dispatches a tool
when invoked directly” test to import and use tmpdir from fixture/fixture.ts,
create a per-test await using tmp fixture, and pass tmp.path to Instance.provide
instead of the hardcoded /tmp directory.
---
Nitpick comments:
In `@packages/opencode/test/altimate/defork/hardpolicy.test.ts`:
- Around line 904-908: Update writeText to use the composed
FileSystem.FileSystem service via Effect.gen, yielding the service and calling
its writeFileString method; remove the dynamic fs/promises import and
Effect.promise implementation while preserving the existing path and content
inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: afdc53b5-9023-413e-b9ee-e140d07944de
📒 Files selected for processing (7)
packages/opencode/src/altimate/policy/hard-policy.tspackages/opencode/src/effect/app-runtime.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/tools.tspackages/opencode/src/tool/batch.tspackages/opencode/test/altimate/defork/hardpolicy.test.tspackages/opencode/test/altimate/defork/route-sentinels.test.ts
| return { | ||
| title: "Blocked by policy", | ||
| output: policyDecision.safeReason, | ||
| metadata: { | ||
| error: policyDecision.safeReason, | ||
| hard_policy_denied: true, | ||
| code: "hard_policy_denied", | ||
| ruleID: policyDecision.ruleID, | ||
| success: false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm MCP tool consumers read `.content` off the execute() result in prompt.ts
rg -nP -C3 '\.content\b' packages/opencode/src/session/prompt.tsRepository: AltimateAI/altimate-code
Length of output: 1149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- prompt.ts around deny branch ---'
sed -n '1718,1745p' packages/opencode/src/session/prompt.ts
echo
echo '--- tools.ts relevant section ---'
sed -n '170,215p' packages/opencode/src/session/tools.tsRepository: AltimateAI/altimate-code
Length of output: 3301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- prompt.ts execute signature / return shape ---'
rg -n -C4 'return \{|type .*execute|execute\(' packages/opencode/src/session/prompt.ts
echo
echo '--- prompt.ts direct .content / .attachments consumers around execute result ---'
rg -n -C3 'result\.(content|attachments)|attachments:|content:' packages/opencode/src/session/prompt.ts
echo
echo '--- tool result type references in session files ---'
rg -n -C2 'content: \[\]|attachments: \[\]|output: .*safeReason|hard_policy_denied' packages/opencode/src/sessionRepository: AltimateAI/altimate-code
Length of output: 14361
Add empty content and attachments on MCP policy deny
packages/opencode/src/session/prompt.ts:1726-1736 should match session/tools.ts and return content: [] and attachments: [] here; otherwise callers reading result.content can get undefined.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/session/prompt.ts` around lines 1726 - 1736, Update the
policy-deny return object in the prompt handling flow to include empty content
and attachments arrays, matching the result shape used by session tools.
Preserve the existing title, output, metadata, and denial values.
| test("D3/D4: SessionTools.resolve dispatches a tool when invoked directly", async () => { | ||
| await Instance.provide({ | ||
| directory: "/tmp", | ||
| fn: async () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use tmpdir() instead of the hardcoded /tmp directory.
Unlike the D1/D5 tests in this same file, this test provides Instance with a shared, hardcoded /tmp, which has no automatic cleanup and can collide with other runs. Prefer await using tmp = await tmpdir() and pass tmp.path.
Based on learnings: for brand-new test files under packages/opencode/test/altimate/, follow the documented convention — import tmpdir from fixture/fixture.ts and use await using tmp = await tmpdir() with per-test scoping; do not suppress the tmpdir() fixture suggestion for newly added files in this directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/altimate/defork/route-sentinels.test.ts` around lines
149 - 152, Update the D3/D4 “SessionTools.resolve dispatches a tool when invoked
directly” test to import and use tmpdir from fixture/fixture.ts, create a
per-test await using tmp fixture, and pass tmp.path to Instance.provide instead
of the hardcoded /tmp directory.
Source: Learnings
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 412ed1ddf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { Wildcard } from "@/util/wildcard" | ||
| import { classifyAndCheck } from "@/altimate/tools/sql-classify" | ||
|
|
||
| export namespace HardPolicy { |
There was a problem hiding this comment.
The applicable packages/opencode/AGENTS.md explicitly says, “Do not use export namespace Foo { ... },” because this shape prevents tree-shaking and breaks Node's native TypeScript runner. This newly introduced module should expose flat top-level declarations and end with the prescribed export * as HardPolicy from "./hard-policy" self-reexport instead.
Useful? React with 👍 / 👎.
| return out | ||
| } | ||
| try { | ||
| return JSON.stringify(normalize(value)) ?? "null" |
There was a problem hiding this comment.
For calls such as write with .env contents or bash with an authorization token, this stores the first 2,000 characters of the raw JSON arguments—not a digest—and the module retains up to 1,000 such records globally through the exported getAuditLog(). Truncation does not protect secrets near the beginning of the payload; hash or explicitly redact sensitive argument values before placing them in the audit log.
Useful? React with 👍 / 👎.
| const policyDecision = HardPolicy.check({ | ||
| toolID: item.id, | ||
| source: "native", | ||
| args: finalArgs, |
There was a problem hiding this comment.
When item.registrySource === "external", which ToolRegistry.fromPlugin() assigns to user and third-party plugin tools, this records the call as native; the same hard-coded value also appears in session/tools.ts. Consequently the declared plugin source is never emitted and the security audit cannot distinguish plugin execution from built-ins, so derive the policy source from item.registrySource in both registry loops.
Useful? React with 👍 / 👎.
| function emitAudit(record: AuditRecord): void { | ||
| auditLog.push(record) | ||
| if (auditLog.length > MAX_AUDIT_LOG) auditLog.shift() | ||
| if (auditSink) auditSink(record) |
There was a problem hiding this comment.
If a callback installed through setAuditSink() throws, this exception escapes emitAudit() after check()'s catch block, so the advertised total, never-throwing policy function instead aborts the dispatcher without returning its decision. Wrap sink invocation failures so a faulty audit observer cannot disrupt every subsequent tool execution.
Useful? React with 👍 / 👎.
| const policyDecision = HardPolicy.check({ | ||
| toolID: key, | ||
| source: "mcp", | ||
| args: finalArgs, |
There was a problem hiding this comment.
For every real MCP server, MCP.tools() constructs keys as <sanitized-client>_<sanitized-tool> in src/mcp/index.ts:1032-1035, but the policy rules only govern the exact IDs bash and sql_execute. Passing the flattened key here therefore means a server such as warehouse exposing sql_execute is checked as warehouse_sql_execute, receives an allow decision, and executes destructive DDL; use the original MCP tool name or remove the known client prefix before matching. The D4 test misses this because its mock supplies the production-impossible bare key bash.
Useful? React with 👍 / 👎.
| if (!policyDecision.allow) { | ||
| return { | ||
| title: "Blocked by policy", |
There was a problem hiding this comment.
When a built-in bash or sql_execute call is denied, returning this ordinary tool value produces an AI SDK tool-result, which session/processor.ts:258-275 unconditionally stores with status: "completed"; only a rejected execution produces the tool-error branch and an error state. Consequently the UI, transcript, and status-based consumers treat a blocked destructive call as successfully completed despite the metadata, unlike the pre-existing SQL and permission denials that threw; reject with the safe policy error instead.
Useful? React with 👍 / 👎.
| toolID = typeof input.toolID === "string" ? input.toolID : "unknown" | ||
| source = input.source | ||
| sessionID = typeof input.sessionID === "string" ? input.sessionID : "" | ||
| callID = input.callID |
There was a problem hiding this comment.
For an object-shaped but malformed call such as HardPolicy.check({} as any) or one with a non-string toolID, this substitutes "unknown", finds no rules, and returns { allow: true }. That contradicts the stated fail-closed behavior for malformed top-level input and means a dispatcher regression that omits the tool ID silently bypasses the gate; validate required fields such as toolID, source, and sessionID and route invalid values through the internal-error deny.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/session/tools.ts">
<violation number="1" location="packages/opencode/src/session/tools.ts:113">
P3: The HardPolicy deny branch for native tools skips `stampRegistryToolSource`, so denied tool-call results won't have `metadata.source` set (unlike the allow path and the MCP deny branch). This can make the UI badge for hard-policy-denied native tool calls render incorrectly or fall back to a default classification.</violation>
</file>
<file name="packages/opencode/src/altimate/policy/hard-policy.ts">
<violation number="1" location="packages/opencode/src/altimate/policy/hard-policy.ts:27">
P2: This file uses `export namespace HardPolicy { … }` for module organization, which the project conventions explicitly prohibit. Per AGENTS.md: *"Do not use `export namespace Foo { ... }` for module organization. It is not standard ESM, it prevents tree-shaking, and it breaks Node's native TypeScript runner."* The documented alternative is flat top-level exports combined with a self-reexport at the bottom of the file (e.g., `export * as HardPolicy from "./hard-policy"`). This matters for all consumers: anything importing from this file pulls in the entire namespace and prevents tree-shaking from eliminating unused members.
Consider restructuring to use the standard pattern, matching the rest of the codebase.</violation>
<violation number="2" location="packages/opencode/src/altimate/policy/hard-policy.ts:291">
P1: If `input.toolID` is missing or not a string, HardPolicy silently treats it as the ungoverned tool "unknown" and allows the call, instead of failing closed like the rest of the chokepoint does for malformed args. For a security kill-gate whose whole purpose is fail-closed behavior, this is an unintended fail-open path — consider throwing (so it's caught and converted to `policy_internal_error` deny) when `toolID` isn't a non-empty string.</violation>
<violation number="3" location="packages/opencode/src/altimate/policy/hard-policy.ts:292">
P2: In `check()`, the `source` field from `input` is assigned directly without a type guard or fallback, while adjacent fields (`toolID`, `sessionID`) are validated with fallback values. The declared `Input` type constraints provide compile-time safety, but since this is a TOTAL function designed to never throw on any malformed input — and the `catch` block explicitly handles internal failures — the `source` value should be validated at runtime too.
If a caller passes `source: undefined` or an unexpected value through a type bypass, it flows unchecked into the audit record's `dispatcher` field. The audit log is the designated security oracle for this module, so recording a corrupted or missing source value reduces the audit trail's reliability.</violation>
<violation number="4" location="packages/opencode/src/altimate/policy/hard-policy.ts:295">
P3: finalArgsDigest fully stringifies+sorts `input.args` on every single tool dispatch (governed or not) before truncating the result, adding non-trivial overhead on the hot path for tools with large payloads (e.g. file writes, long SQL). Consider gating the digest computation to only governed toolIDs, or truncating/sampling before the full recursive normalize.</violation>
<violation number="5" location="packages/opencode/src/altimate/policy/hard-policy.ts:325">
P2: `dispatcher` and `source` in the emitted AuditRecord are always identical values, so the audit trail can't actually differentiate the D1–D6 call sites the PR claims to verify — consider passing a distinct per-call-site identifier (e.g. "D1", "D2"...) as `dispatcher` if that granularity is meant to be test-observable.</violation>
</file>
<file name="packages/opencode/test/altimate/defork/route-sentinels.test.ts">
<violation number="1" location="packages/opencode/test/altimate/defork/route-sentinels.test.ts:151">
P3: Unlike the D1 and D5 tests in this same file, this test passes a hardcoded `/tmp` to `Instance.provide`, which has no automatic cleanup and can collide with parallel test runs. Consider using `await using tmp = await tmpdir()` and passing `tmp.path` for consistency and isolation.</violation>
<violation number="2" location="packages/opencode/test/altimate/defork/route-sentinels.test.ts:224">
P3: Test uses `permission: {}` (bare object) for `agent.permission`, which is inconsistent with D1's documented practice and the `PermissionV1.Ruleset` type (array of rules). The D1 test at line 95 uses `permission: []` with an explanatory comment that `{}` crashes `Wildcard.match()` via `Permission.evaluate()` in `Skill.available`. Currently works because all downstream services (ToolRegistry, Permission) are mocked — but removing those mocks later or adding a new code path that reads `agent.permission` via `Permission.merge`/`evaluate` would silently fail with no test-preparation surface. Use `permission: []` for consistency and defensive setup.</violation>
</file>
<file name="packages/opencode/src/tool/batch.ts">
<violation number="1" location="packages/opencode/src/tool/batch.ts:117">
P2: Unlike the other HardPolicy checkpoints (D1–D4/D6), the batch dispatcher (D5) surfaces a denial only as a plain error message and drops `ruleID`/`hard_policy_denied` from the batch result metadata. Any downstream code (UI, telemetry, tests) that inspects `metadata.hard_policy_denied`/`ruleID` to detect a hard-policy deny — as the other dispatchers now support — won't see it for batched tool calls, making denial detection for batch results string-matching-dependent instead of structured.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| if (!isRecord(input as unknown)) { | ||
| throw new Error("hard-policy: input is not an object") | ||
| } | ||
| toolID = typeof input.toolID === "string" ? input.toolID : "unknown" |
There was a problem hiding this comment.
P1: If input.toolID is missing or not a string, HardPolicy silently treats it as the ungoverned tool "unknown" and allows the call, instead of failing closed like the rest of the chokepoint does for malformed args. For a security kill-gate whose whole purpose is fail-closed behavior, this is an unintended fail-open path — consider throwing (so it's caught and converted to policy_internal_error deny) when toolID isn't a non-empty string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/policy/hard-policy.ts, line 291:
<comment>If `input.toolID` is missing or not a string, HardPolicy silently treats it as the ungoverned tool "unknown" and allows the call, instead of failing closed like the rest of the chokepoint does for malformed args. For a security kill-gate whose whole purpose is fail-closed behavior, this is an unintended fail-open path — consider throwing (so it's caught and converted to `policy_internal_error` deny) when `toolID` isn't a non-empty string.</comment>
<file context>
@@ -0,0 +1,337 @@
+ if (!isRecord(input as unknown)) {
+ throw new Error("hard-policy: input is not an object")
+ }
+ toolID = typeof input.toolID === "string" ? input.toolID : "unknown"
+ source = input.source
+ sessionID = typeof input.sessionID === "string" ? input.sessionID : ""
</file context>
|
|
||
| emitAudit({ | ||
| ruleID: decision.allow ? "" : decision.ruleID, | ||
| dispatcher: source, |
There was a problem hiding this comment.
P2: dispatcher and source in the emitted AuditRecord are always identical values, so the audit trail can't actually differentiate the D1–D6 call sites the PR claims to verify — consider passing a distinct per-call-site identifier (e.g. "D1", "D2"...) as dispatcher if that granularity is meant to be test-observable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/policy/hard-policy.ts, line 325:
<comment>`dispatcher` and `source` in the emitted AuditRecord are always identical values, so the audit trail can't actually differentiate the D1–D6 call sites the PR claims to verify — consider passing a distinct per-call-site identifier (e.g. "D1", "D2"...) as `dispatcher` if that granularity is meant to be test-observable.</comment>
<file context>
@@ -0,0 +1,337 @@
+
+ emitAudit({
+ ruleID: decision.allow ? "" : decision.ruleID,
+ dispatcher: source,
+ toolID,
+ source,
</file context>
| sessionID: ctx.sessionID, | ||
| callID: partID, | ||
| }) | ||
| if (!policyDecision.allow) { |
There was a problem hiding this comment.
P2: Unlike the other HardPolicy checkpoints (D1–D4/D6), the batch dispatcher (D5) surfaces a denial only as a plain error message and drops ruleID/hard_policy_denied from the batch result metadata. Any downstream code (UI, telemetry, tests) that inspects metadata.hard_policy_denied/ruleID to detect a hard-policy deny — as the other dispatchers now support — won't see it for batched tool calls, making denial detection for batch results string-matching-dependent instead of structured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tool/batch.ts, line 117:
<comment>Unlike the other HardPolicy checkpoints (D1–D4/D6), the batch dispatcher (D5) surfaces a denial only as a plain error message and drops `ruleID`/`hard_policy_denied` from the batch result metadata. Any downstream code (UI, telemetry, tests) that inspects `metadata.hard_policy_denied`/`ruleID` to detect a hard-policy deny — as the other dispatchers now support — won't see it for batched tool calls, making denial detection for batch results string-matching-dependent instead of structured.</comment>
<file context>
@@ -100,6 +104,20 @@ export const BatchTool = Tool.define("batch", {
+ sessionID: ctx.sessionID,
+ callID: partID,
+ })
+ if (!policyDecision.allow) {
+ throw new Error(policyDecision.safeReason)
+ }
</file context>
| throw new Error("hard-policy: input is not an object") | ||
| } | ||
| toolID = typeof input.toolID === "string" ? input.toolID : "unknown" | ||
| source = input.source |
There was a problem hiding this comment.
P2: In check(), the source field from input is assigned directly without a type guard or fallback, while adjacent fields (toolID, sessionID) are validated with fallback values. The declared Input type constraints provide compile-time safety, but since this is a TOTAL function designed to never throw on any malformed input — and the catch block explicitly handles internal failures — the source value should be validated at runtime too.
If a caller passes source: undefined or an unexpected value through a type bypass, it flows unchecked into the audit record's dispatcher field. The audit log is the designated security oracle for this module, so recording a corrupted or missing source value reduces the audit trail's reliability.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/policy/hard-policy.ts, line 292:
<comment>In `check()`, the `source` field from `input` is assigned directly without a type guard or fallback, while adjacent fields (`toolID`, `sessionID`) are validated with fallback values. The declared `Input` type constraints provide compile-time safety, but since this is a TOTAL function designed to never throw on any malformed input — and the `catch` block explicitly handles internal failures — the `source` value should be validated at runtime too.
If a caller passes `source: undefined` or an unexpected value through a type bypass, it flows unchecked into the audit record's `dispatcher` field. The audit log is the designated security oracle for this module, so recording a corrupted or missing source value reduces the audit trail's reliability.</comment>
<file context>
@@ -0,0 +1,337 @@
+ throw new Error("hard-policy: input is not an object")
+ }
+ toolID = typeof input.toolID === "string" ? input.toolID : "unknown"
+ source = input.source
+ sessionID = typeof input.sessionID === "string" ? input.sessionID : ""
+ callID = input.callID
</file context>
| import { Wildcard } from "@/util/wildcard" | ||
| import { classifyAndCheck } from "@/altimate/tools/sql-classify" | ||
|
|
||
| export namespace HardPolicy { |
There was a problem hiding this comment.
P2: This file uses export namespace HardPolicy { … } for module organization, which the project conventions explicitly prohibit. Per AGENTS.md: "Do not use export namespace Foo { ... } for module organization. It is not standard ESM, it prevents tree-shaking, and it breaks Node's native TypeScript runner." The documented alternative is flat top-level exports combined with a self-reexport at the bottom of the file (e.g., export * as HardPolicy from "./hard-policy"). This matters for all consumers: anything importing from this file pulls in the entire namespace and prevents tree-shaking from eliminating unused members.
Consider restructuring to use the standard pattern, matching the rest of the codebase.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/policy/hard-policy.ts, line 27:
<comment>This file uses `export namespace HardPolicy { … }` for module organization, which the project conventions explicitly prohibit. Per AGENTS.md: *"Do not use `export namespace Foo { ... }` for module organization. It is not standard ESM, it prevents tree-shaking, and it breaks Node's native TypeScript runner."* The documented alternative is flat top-level exports combined with a self-reexport at the bottom of the file (e.g., `export * as HardPolicy from "./hard-policy"`). This matters for all consumers: anything importing from this file pulls in the entire namespace and prevents tree-shaking from eliminating unused members.
Consider restructuring to use the standard pattern, matching the rest of the codebase.</comment>
<file context>
@@ -0,0 +1,337 @@
+import { Wildcard } from "@/util/wildcard"
+import { classifyAndCheck } from "@/altimate/tools/sql-classify"
+
+export namespace HardPolicy {
+ // ---------------------------------------------------------------------------------
+ // Public types
</file context>
| callID: ctx.callID, | ||
| }) | ||
| if (!policyDecision.allow) { | ||
| return { |
There was a problem hiding this comment.
P3: The HardPolicy deny branch for native tools skips stampRegistryToolSource, so denied tool-call results won't have metadata.source set (unlike the allow path and the MCP deny branch). This can make the UI badge for hard-policy-denied native tool calls render incorrectly or fall back to a default classification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/session/tools.ts, line 113:
<comment>The HardPolicy deny branch for native tools skips `stampRegistryToolSource`, so denied tool-call results won't have `metadata.source` set (unlike the allow path and the MCP deny branch). This can make the UI badge for hard-policy-denied native tool calls render incorrectly or fall back to a default classification.</comment>
<file context>
@@ -89,12 +92,39 @@ export const resolve = Effect.fn("SessionTools.resolve")(function* (input: {
+ callID: ctx.callID,
+ })
+ if (!policyDecision.allow) {
+ return {
+ title: "Blocked by policy",
+ output: policyDecision.safeReason,
</file context>
| source = input.source | ||
| sessionID = typeof input.sessionID === "string" ? input.sessionID : "" | ||
| callID = input.callID | ||
| digest = finalArgsDigest(input.args) |
There was a problem hiding this comment.
P3: finalArgsDigest fully stringifies+sorts input.args on every single tool dispatch (governed or not) before truncating the result, adding non-trivial overhead on the hot path for tools with large payloads (e.g. file writes, long SQL). Consider gating the digest computation to only governed toolIDs, or truncating/sampling before the full recursive normalize.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/policy/hard-policy.ts, line 295:
<comment>finalArgsDigest fully stringifies+sorts `input.args` on every single tool dispatch (governed or not) before truncating the result, adding non-trivial overhead on the hot path for tools with large payloads (e.g. file writes, long SQL). Consider gating the digest computation to only governed toolIDs, or truncating/sampling before the full recursive normalize.</comment>
<file context>
@@ -0,0 +1,337 @@
+ source = input.source
+ sessionID = typeof input.sessionID === "string" ? input.sessionID : ""
+ callID = input.callID
+ digest = finalArgsDigest(input.args)
+
+ assertInitialized()
</file context>
| } | ||
|
|
||
| const input: any = { | ||
| agent: { name: "build", mode: "primary", permission: {}, options: {} }, |
There was a problem hiding this comment.
P3: Test uses permission: {} (bare object) for agent.permission, which is inconsistent with D1's documented practice and the PermissionV1.Ruleset type (array of rules). The D1 test at line 95 uses permission: [] with an explanatory comment that {} crashes Wildcard.match() via Permission.evaluate() in Skill.available. Currently works because all downstream services (ToolRegistry, Permission) are mocked — but removing those mocks later or adding a new code path that reads agent.permission via Permission.merge/evaluate would silently fail with no test-preparation surface. Use permission: [] for consistency and defensive setup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/altimate/defork/route-sentinels.test.ts, line 224:
<comment>Test uses `permission: {}` (bare object) for `agent.permission`, which is inconsistent with D1's documented practice and the `PermissionV1.Ruleset` type (array of rules). The D1 test at line 95 uses `permission: []` with an explanatory comment that `{}` crashes `Wildcard.match()` via `Permission.evaluate()` in `Skill.available`. Currently works because all downstream services (ToolRegistry, Permission) are mocked — but removing those mocks later or adding a new code path that reads `agent.permission` via `Permission.merge`/`evaluate` would silently fail with no test-preparation surface. Use `permission: []` for consistency and defensive setup.</comment>
<file context>
@@ -0,0 +1,346 @@
+ }
+
+ const input: any = {
+ agent: { name: "build", mode: "primary", permission: {}, options: {} },
+ model: { providerID: "test", api: { id: "test-model" } },
+ session: { id: "ses_fake", permission: [] },
</file context>
|
|
||
| test("D3/D4: SessionTools.resolve dispatches a tool when invoked directly", async () => { | ||
| await Instance.provide({ | ||
| directory: "/tmp", |
There was a problem hiding this comment.
P3: Unlike the D1 and D5 tests in this same file, this test passes a hardcoded /tmp to Instance.provide, which has no automatic cleanup and can collide with parallel test runs. Consider using await using tmp = await tmpdir() and passing tmp.path for consistency and isolation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/altimate/defork/route-sentinels.test.ts, line 151:
<comment>Unlike the D1 and D5 tests in this same file, this test passes a hardcoded `/tmp` to `Instance.provide`, which has no automatic cleanup and can collide with parallel test runs. Consider using `await using tmp = await tmpdir()` and passing `tmp.path` for consistency and isolation.</comment>
<file context>
@@ -0,0 +1,346 @@
+
+test("D3/D4: SessionTools.resolve dispatches a tool when invoked directly", async () => {
+ await Instance.provide({
+ directory: "/tmp",
+ fn: async () => {
+ const invalidInfo = await toolInfo(InvalidTool)
</file context>
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 6 finding(s)
- 6 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/session/tools.ts (L112-L124)
[🟠 MEDIUM] When HardPolicy.check blocks the tool execution and returns early, the tool.execute.after plugin hook is completely skipped.
While it's true the tool itself wasn't executed, any plugins that hooked into tool.execute.before (and perhaps started a span, locked a resource, or initialized state) will never receive the corresponding tool.execute.after event to clean up or log the blocked outcome. If the intention is to completely short-circuit, this might be fine, but typically a paired before/after hook design expects after to be called with the error or blocked result. Please verify if plugins need to be notified of the hard policy denial.
2. packages/opencode/src/session/tools.ts (L94-L103)
[🟠 MEDIUM] You correctly ensured that HardPolicy.check and the tool's execute function use finalArgs. However, ctx is initialized before the plugin hook using the original args.
const ctx = context(args, options)Inside the context factory, args is captured in a closure and used when the tool updates its metadata (e.g., input: args in updateToolCall). This means the system state/UI will record the original unmutated args as the tool's input, even though it actually executed with finalArgs.
If the system state should reflect the mutated arguments, you should create ctx after the hook (using sessionID: input.session.id and callID: options.toolCallId directly for the hook input), or update the context factory to use the latest args.
3. packages/opencode/src/session/tools.ts (L169-L178)
[🟠 MEDIUM] Similarly for MCP tools, ctx captures the original args before the tool.execute.before hook can mutate them. Any calls to ctx.metadata(...) during execution will save the original args to the tool call state rather than finalArgs.
Consider moving const ctx = context(finalArgs, opts) to after the hook, using input.session.id for the hook's sessionID parameter.
4. packages/opencode/src/altimate/policy/hard-policy.ts (L109-L110)
[🔴 HIGH] The Wildcard.match function typically expects a strict prefix match since it compiles patterns with a ^ anchor. Because the BASH_DDL_PATTERNS start directly with the command (e.g., "DROP DATABASE *"), a user could bypass this hard policy by simply prepending whitespace (e.g., " DROP DATABASE prod").
To prevent this easy bypass, consider trimming the command string before matching.
Suggested change:
const command = args.command.trim()
return BASH_DDL_PATTERNS.some((pattern) => Wildcard.match(command, pattern))
5. packages/opencode/src/altimate/policy/hard-policy.ts (L254-L258)
[🟠 MEDIUM] If input.args happens to contain non-plain objects like Map or Set (e.g., mutated by a tool.execute.before hook), Object.keys() will return an empty array, and they will be stringified as {}. This could lead to identical audit digests for different inputs.
Although tool inputs are typically plain JSON objects, consider handling Map and Set or falling back to a more robust serialization if they are expected.
Suggested change:
if (Array.isArray(input)) return input.map(normalize)
if (input instanceof Map) return normalize(Object.fromEntries(input))
if (input instanceof Set) return normalize(Array.from(input))
const record = input as Record<string, unknown>
const out: Record<string, unknown> = {}
for (const key of Object.keys(record).sort()) out[key] = normalize(record[key])
return out
6. packages/opencode/src/altimate/policy/hard-policy.ts (L288-L292)
[🔴 HIGH] If input.toolID is missing or not a string (e.g., due to a caller bug), it defaults to "unknown". Since "unknown" is an ungoverned tool ID, the policy will unconditionally allow the execution. This represents a fail-open behavior on malformed top-level input, which contradicts the fail-closed design contract stated in the comments.
Consider throwing an error here instead of defaulting to "unknown" to ensure any such caller bugs result in a secure deny.
Suggested change:
if (!isRecord(input as unknown)) {
throw new Error("hard-policy: input is not an object")
}
if (typeof input.toolID !== "string") {
throw new Error("hard-policy: input missing string 'toolID'")
}
toolID = input.toolID
source = input.source
| if (!policyDecision.allow) { | ||
| return { | ||
| title: "Blocked by policy", | ||
| output: policyDecision.safeReason, | ||
| metadata: { | ||
| error: policyDecision.safeReason, | ||
| hard_policy_denied: true, | ||
| code: "hard_policy_denied", | ||
| ruleID: policyDecision.ruleID, | ||
| success: false, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] When HardPolicy.check blocks the tool execution and returns early, the tool.execute.after plugin hook is completely skipped.
While it's true the tool itself wasn't executed, any plugins that hooked into tool.execute.before (and perhaps started a span, locked a resource, or initialized state) will never receive the corresponding tool.execute.after event to clean up or log the blocked outcome. If the intention is to completely short-circuit, this might be fine, but typically a paired before/after hook design expects after to be called with the error or blocked result. Please verify if plugins need to be notified of the hard policy denial.
| const ctx = context(args, options) | ||
| yield* plugin.trigger( | ||
| // altimate_change start — HardPolicy enforcement (S3) | ||
| // upstream_fix: plugin.trigger's returned output was previously discarded, so a | ||
| // tool.execute.before hook that mutated `args` had no effect on what actually | ||
| // ran — HardPolicy and execute must see the SAME final, post-hook args. | ||
| const hookOutput = yield* plugin.trigger( | ||
| "tool.execute.before", | ||
| { tool: item.id, sessionID: ctx.sessionID, callID: ctx.callID }, | ||
| { args }, | ||
| ) |
There was a problem hiding this comment.
[🟠 MEDIUM] You correctly ensured that HardPolicy.check and the tool's execute function use finalArgs. However, ctx is initialized before the plugin hook using the original args.
const ctx = context(args, options)Inside the context factory, args is captured in a closure and used when the tool updates its metadata (e.g., input: args in updateToolCall). This means the system state/UI will record the original unmutated args as the tool's input, even though it actually executed with finalArgs.
If the system state should reflect the mutated arguments, you should create ctx after the hook (using sessionID: input.session.id and callID: options.toolCallId directly for the hook input), or update the context factory to use the latest args.
| const ctx = context(args, opts) | ||
| yield* plugin.trigger( | ||
| // altimate_change start — HardPolicy enforcement (S3) | ||
| // upstream_fix: plugin.trigger's returned output was previously discarded, so a | ||
| // tool.execute.before hook that mutated `args` had no effect on what actually | ||
| // ran — HardPolicy and execute must see the SAME final, post-hook args. | ||
| const hookOutput = yield* plugin.trigger( | ||
| "tool.execute.before", | ||
| { tool: key, sessionID: ctx.sessionID, callID: opts.toolCallId }, | ||
| { args }, | ||
| ) |
There was a problem hiding this comment.
[🟠 MEDIUM] Similarly for MCP tools, ctx captures the original args before the tool.execute.before hook can mutate them. Any calls to ctx.metadata(...) during execution will save the original args to the tool call state rather than finalArgs.
Consider moving const ctx = context(finalArgs, opts) to after the hook, using input.session.id for the hook's sessionID parameter.
| const command = args.command | ||
| return BASH_DDL_PATTERNS.some((pattern) => Wildcard.match(command, pattern)) |
There was a problem hiding this comment.
[🔴 HIGH] The Wildcard.match function typically expects a strict prefix match since it compiles patterns with a ^ anchor. Because the BASH_DDL_PATTERNS start directly with the command (e.g., "DROP DATABASE *"), a user could bypass this hard policy by simply prepending whitespace (e.g., " DROP DATABASE prod").
To prevent this easy bypass, consider trimming the command string before matching.
Suggested change:
| const command = args.command | |
| return BASH_DDL_PATTERNS.some((pattern) => Wildcard.match(command, pattern)) | |
| const command = args.command.trim() | |
| return BASH_DDL_PATTERNS.some((pattern) => Wildcard.match(command, pattern)) |
| if (Array.isArray(input)) return input.map(normalize) | ||
| const record = input as Record<string, unknown> | ||
| const out: Record<string, unknown> = {} | ||
| for (const key of Object.keys(record).sort()) out[key] = normalize(record[key]) | ||
| return out |
There was a problem hiding this comment.
[🟠 MEDIUM] If input.args happens to contain non-plain objects like Map or Set (e.g., mutated by a tool.execute.before hook), Object.keys() will return an empty array, and they will be stringified as {}. This could lead to identical audit digests for different inputs.
Although tool inputs are typically plain JSON objects, consider handling Map and Set or falling back to a more robust serialization if they are expected.
Suggested change:
| if (Array.isArray(input)) return input.map(normalize) | |
| const record = input as Record<string, unknown> | |
| const out: Record<string, unknown> = {} | |
| for (const key of Object.keys(record).sort()) out[key] = normalize(record[key]) | |
| return out | |
| if (Array.isArray(input)) return input.map(normalize) | |
| if (input instanceof Map) return normalize(Object.fromEntries(input)) | |
| if (input instanceof Set) return normalize(Array.from(input)) | |
| const record = input as Record<string, unknown> | |
| const out: Record<string, unknown> = {} | |
| for (const key of Object.keys(record).sort()) out[key] = normalize(record[key]) | |
| return out |
| if (!isRecord(input as unknown)) { | ||
| throw new Error("hard-policy: input is not an object") | ||
| } | ||
| toolID = typeof input.toolID === "string" ? input.toolID : "unknown" | ||
| source = input.source |
There was a problem hiding this comment.
[🔴 HIGH] If input.toolID is missing or not a string (e.g., due to a caller bug), it defaults to "unknown". Since "unknown" is an ungoverned tool ID, the policy will unconditionally allow the execution. This represents a fail-open behavior on malformed top-level input, which contradicts the fail-closed design contract stated in the comments.
Consider throwing an error here instead of defaulting to "unknown" to ensure any such caller bugs result in a secure deny.
Suggested change:
| if (!isRecord(input as unknown)) { | |
| throw new Error("hard-policy: input is not an object") | |
| } | |
| toolID = typeof input.toolID === "string" ? input.toolID : "unknown" | |
| source = input.source | |
| if (!isRecord(input as unknown)) { | |
| throw new Error("hard-policy: input is not an object") | |
| } | |
| if (typeof input.toolID !== "string") { | |
| throw new Error("hard-policy: input missing string 'toolID'") | |
| } | |
| toolID = input.toolID | |
| source = input.source |
…throws" contract - `emitAudit` now wraps the external `auditSink(record)` call in try/catch: a throwing installed sink must never propagate out of `check()`, which is contractually total. The decision is already computed and returned regardless of audit outcome (kilo-code-bot + CodeRabbit, both flagged). - Replaced the O(n) `auditLog.shift()`-per-call with a batched splice (O(1) amortized) — `check()` runs on the tool-execution hot path (kilo-code-bot). - Latent-guard test now scans the entire production src tree for `SessionTools.resolve(` callers (matches the S2 PR's version so the shared file merges identically in any order). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/policy/hard-policy.ts">
<violation number="1" location="packages/opencode/src/altimate/policy/hard-policy.ts:227">
P2: The try/catch here only guards against synchronous throws from `auditSink`. Because the sink's declared type (`(record) => void`) is structurally compatible with async functions returning `Promise<void>` in TypeScript, an async sink that rejects would bypass this catch and cause an unhandled rejection, undermining the 'never breaks enforcement' goal for that case.</violation>
</file>
<file name="packages/opencode/test/altimate/defork/route-sentinels.test.ts">
<violation number="1" location="packages/opencode/test/altimate/defork/route-sentinels.test.ts:136">
P3: The isComment heuristic misclassifies lines where a `/* ... */` block comment is followed by actual code on the same line. If someone writes `/* TODO */ SessionTools.resolve(args)`, `t.startsWith("/*")` is true so `isComment = true` and the line is skipped — but the code after the block comment IS executing code, making this a false negative that could silently miss a real caller. The detection treats `t.startsWith("/*")` as a whole-line comment indicator, which is only correct when the `/*` starts a line-spanning or end-of-line comment, not an inline block comment followed by code.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // The decision is already computed and returned regardless of audit outcome. | ||
| if (auditSink) { | ||
| try { | ||
| auditSink(record) |
There was a problem hiding this comment.
P2: The try/catch here only guards against synchronous throws from auditSink. Because the sink's declared type ((record) => void) is structurally compatible with async functions returning Promise<void> in TypeScript, an async sink that rejects would bypass this catch and cause an unhandled rejection, undermining the 'never breaks enforcement' goal for that case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/policy/hard-policy.ts, line 227:
<comment>The try/catch here only guards against synchronous throws from `auditSink`. Because the sink's declared type (`(record) => void`) is structurally compatible with async functions returning `Promise<void>` in TypeScript, an async sink that rejects would bypass this catch and cause an unhandled rejection, undermining the 'never breaks enforcement' goal for that case.</comment>
<file context>
@@ -215,8 +215,20 @@ export namespace HardPolicy {
+ // The decision is already computed and returned regardless of audit outcome.
+ if (auditSink) {
+ try {
+ auditSink(record)
+ } catch {
+ // Intentionally swallowed: audit-side failure cannot affect enforcement.
</file context>
| if (!src.includes("SessionTools.resolve(")) continue | ||
| src.split("\n").forEach((line, i) => { | ||
| const t = line.trim() | ||
| const isComment = t.startsWith("//") || t.startsWith("*") || t.startsWith("/*") |
There was a problem hiding this comment.
P3: The isComment heuristic misclassifies lines where a /* ... */ block comment is followed by actual code on the same line. If someone writes /* TODO */ SessionTools.resolve(args), t.startsWith("/*") is true so isComment = true and the line is skipped — but the code after the block comment IS executing code, making this a false negative that could silently miss a real caller. The detection treats t.startsWith("/*") as a whole-line comment indicator, which is only correct when the /* starts a line-spanning or end-of-line comment, not an inline block comment followed by code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/altimate/defork/route-sentinels.test.ts, line 136:
<comment>The isComment heuristic misclassifies lines where a `/* ... */` block comment is followed by actual code on the same line. If someone writes `/* TODO */ SessionTools.resolve(args)`, `t.startsWith("/*")` is true so `isComment = true` and the line is skipped — but the code after the block comment IS executing code, making this a false negative that could silently miss a real caller. The detection treats `t.startsWith("/*")` as a whole-line comment indicator, which is only correct when the `/*` starts a line-spanning or end-of-line comment, not an inline block comment followed by code.</comment>
<file context>
@@ -120,30 +120,26 @@ test("D1: resolveTools dispatch chokepoint is present and unique", () => {
+ if (!src.includes("SessionTools.resolve(")) continue
+ src.split("\n").forEach((line, i) => {
+ const t = line.trim()
+ const isComment = t.startsWith("//") || t.startsWith("*") || t.startsWith("/*")
+ if (!isComment && line.includes("SessionTools.resolve(")) callers.push(`${rel}:${i + 1}`)
+ })
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2884d7e721
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const str = stableStringify(args) | ||
| return str.length > MAX_DIGEST_LENGTH ? str.slice(0, MAX_DIGEST_LENGTH) : str |
There was a problem hiding this comment.
For tools with large arguments, such as write with a multi-megabyte payload or a large batch call, stableStringify() first recursively copies the entire argument graph and builds the complete JSON string; only afterward is it sliced to 2,000 characters. Because this runs synchronously for every tool dispatch, the advertised digest bound does not limit CPU or peak memory and can stall or exhaust the process on otherwise valid inputs; enforce size/depth limits while traversing or use a bounded/streaming digest.
Useful? React with 👍 / 👎.
| "tool.execute.after", | ||
| { | ||
| tool: "task", | ||
| callID: part.id, |
There was a problem hiding this comment.
On the direct-subtask path, the tool part is created with distinct id and callID values, and the execution context below correctly uses part.callID; recording part.id here therefore makes every task audit record impossible to correlate with the corresponding tool call by call ID. Use part.callID, as the other dispatchers do, so the audit trail retains its stated correlation semantics.
Useful? React with 👍 / 👎.
| // altimate_change end | ||
| // altimate_change start — v1.17.9: Tool.Def.execute returns an Effect | ||
| const result = await AppRuntime.runPromise(item.execute(args, ctx)) | ||
| const result = await AppRuntime.runPromise(item.execute(finalArgs, ctx)) |
There was a problem hiding this comment.
When a tool.execute.before hook replaces output.args with a new object, this now executes finalArgs, but the tool context was created earlier from the original args and its state update still records input: args (context() at lines 1562-1584); the processor likewise began the tool part with the model-supplied input. The session transcript and UI can therefore show benign/original arguments while the tool and HardPolicy audit use different replacement arguments, so update the stored tool input to the post-hook value before dispatch.
Useful? React with 👍 / 👎.
| const INTERNAL_ERROR_RULE_ID = "policy_internal_error" | ||
| const INTERNAL_ERROR_SAFE_REASON = "Policy could not be evaluated; denying for safety." | ||
|
|
||
| export function check(input: Input): Decision { |
There was a problem hiding this comment.
The repository-wide dispatcher search also finds packages/opencode/src/cli/cmd/debug/agent.handler.ts:45-60, where altimate debug agent <name> --tool <id> resolves a registry tool and invokes tool.execute(params, toolCtx) directly without calling this policy. Consequently even a debug invocation of governed tools such as bash or sql_execute produces no HardPolicy audit record, so the audit log cannot serve as the claimed oracle for every execution route; invoke HardPolicy.check() at that dispatcher before execution.
Useful? React with 👍 / 👎.
…ew P1s) Two P1 review findings on the HardPolicy chokepoint: - MCP-prefix bypass (prompt.ts D2 / tools.ts D4 path): real MCP tools are registered with a flattened id `<sanitized-client>_<sanitized-tool>` (see `MCP.tools()` in `src/mcp/index.ts`), but `RULES` are keyed by the bare governed id (`sql_execute`, `bash`). A warehouse server exposing its own `sql_execute` therefore arrived as `warehouse_sql_execute`, matched no rule, and was allowed to run `DROP DATABASE`. Add `resolveGovernedKey()`: for the `mcp` source ONLY, resolve a governed key that is a full `_`-delimited suffix of the flattened id. Native/plugin/batch/task ids keep strict exact-match, so this only ADDS coverage for MCP — it never un-governs an exact match. The audit record still retains the original flattened id for forensics. (The old D4 mock used the production-impossible bare key `bash`, hiding this.) - Audit arg retention: `finalArgsDigest` stored the first 2000 chars of the raw stable-JSON args. Tool args routinely carry secrets (`write` of `.env` contents, `bash` with an auth token) and the audit log is retained (up to MAX_AUDIT_LOG, readable via `getAuditLog()` / an installed sink), so truncation did not protect secrets near the start of the payload. Digest is now a SHA-256 hex over the stable-stringified args — non-reversible, no plaintext retained, correlation property preserved. Export `digestArgs()` so the D3 mutation tests recompute the expected digest instead of substring- matching the (now-hashed) value. Tests: +3 Section-A unit tests (MCP-flattened deny for sql_execute/bash; mcp-only scoping — native suffix + partial `mysql_execute` stay allowed; digest is 64-hex and contains no plaintext). D3 mutation/allow digest assertions rewritten to compare against `digestArgs(...)`. 22 pass, typecheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/policy/hard-policy.ts">
<violation number="1" location="packages/opencode/src/altimate/policy/hard-policy.ts:203">
P2: resolveGovernedKey's suffix match (`toolID.endsWith("_" + governed)`) treats any MCP tool whose flattened id ends in `_bash` or `_sql_execute` as the governed bash/sql_execute tool, not just re-exposed same-named tools. If such an unrelated tool's args don't have the expected `command`/`query` string field, matchBashDdl/matchSqlDdl throws and check() fails closed with `policy_internal_error`, silently blocking a legitimate, unrelated MCP tool call. Consider requiring an exact tool-name segment match (e.g. splitting on the known client-name boundary) rather than a bare string suffix.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // a full `_`-delimited suffix of the flattened id. Native/plugin/batch/task tools keep | ||
| // strict exact-match (their ids are not client-prefixed), so this only ever ADDS coverage | ||
| // for the mcp source; it never un-governs a tool that exact-match already caught. | ||
| function resolveGovernedKey(toolID: string, source: Source): string | undefined { |
There was a problem hiding this comment.
P2: resolveGovernedKey's suffix match (toolID.endsWith("_" + governed)) treats any MCP tool whose flattened id ends in _bash or _sql_execute as the governed bash/sql_execute tool, not just re-exposed same-named tools. If such an unrelated tool's args don't have the expected command/query string field, matchBashDdl/matchSqlDdl throws and check() fails closed with policy_internal_error, silently blocking a legitimate, unrelated MCP tool call. Consider requiring an exact tool-name segment match (e.g. splitting on the known client-name boundary) rather than a bare string suffix.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/policy/hard-policy.ts, line 203:
<comment>resolveGovernedKey's suffix match (`toolID.endsWith("_" + governed)`) treats any MCP tool whose flattened id ends in `_bash` or `_sql_execute` as the governed bash/sql_execute tool, not just re-exposed same-named tools. If such an unrelated tool's args don't have the expected `command`/`query` string field, matchBashDdl/matchSqlDdl throws and check() fails closed with `policy_internal_error`, silently blocking a legitimate, unrelated MCP tool call. Consider requiring an exact tool-name segment match (e.g. splitting on the known client-name boundary) rather than a bare string suffix.</comment>
<file context>
@@ -191,6 +192,23 @@ export namespace HardPolicy {
+ // a full `_`-delimited suffix of the flattened id. Native/plugin/batch/task tools keep
+ // strict exact-match (their ids are not client-prefixed), so this only ever ADDS coverage
+ // for the mcp source; it never un-governs a tool that exact-match already caught.
+ function resolveGovernedKey(toolID: string, source: Source): string | undefined {
+ if (rulesByTool.has(toolID)) return toolID
+ if (source !== "mcp") return undefined
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ac7b68324
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const governed of rulesByTool.keys()) { | ||
| if (toolID.endsWith(`_${governed}`)) return governed |
There was a problem hiding this comment.
For MCP identifiers, suffix matching cannot distinguish the client portion from the tool name. For example, src/mcp/index.ts flattens client warehouse_sql plus tool execute into warehouse_sql_execute; this branch misclassifies it as the governed sql_execute tool and rejects every invocation lacking a query field with policy_internal_error. A tool such as explain_sql_execute is similarly over-governed. Preserve the original MCP tool name or strip the exact sanitized client prefix available at the dispatcher instead of accepting any governed suffix.
Useful? React with 👍 / 👎.
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 3 finding(s)
- 3 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/policy/hard-policy.ts (L24)
[🟠 MEDIUM] This import introduces a strict dependency on Node.js/Bun. If HardPolicy (or any module importing it) is executed in a non-Node environment like a browser, Edge runtime, or a VS Code web extension, this will cause a runtime crash.
Consider using a cross-platform alternative, such as a pure JS implementation of SHA-256 (e.g., crypto-js), using the Web Crypto API (if asynchronous execution is permissible), or injecting the hashing function dynamically based on the environment.
2. packages/opencode/src/altimate/policy/hard-policy.ts (L203-L210)
[🔴 HIGH] The suffix matching loop here depends on the Map insertion order. If there are overlapping suffixes (for instance, rules for both execute and sql_execute), and execute was inserted into RULES first, an MCP tool ID like warehouse_sql_execute would incorrectly match execute instead of sql_execute.
To avoid this vulnerability, sort the governed keys by length in descending order, ensuring the most specific suffix matches first.
Suggested change:
function resolveGovernedKey(toolID: string, source: Source): string | undefined {
if (rulesByTool.has(toolID)) return toolID
if (source !== "mcp") return undefined
// Sort keys by length descending to ensure the most specific suffix matches first
const governedKeys = Array.from(rulesByTool.keys()).sort((a, b) => b.length - a.length)
for (const governed of governedKeys) {
if (toolID.endsWith(`_${governed}`)) return governed
}
return undefined
}
3. packages/opencode/src/altimate/policy/hard-policy.ts (L243-L249)
[🔴 HIGH] The try/catch block only handles synchronous errors. If an externally provided auditSink returns a rejected Promise (e.g., if it's an async function), the rejection will silently bypass the catch block. This leads to an unhandled promise rejection, which can crash the Node.js process.
Safely catch potential Promise rejections to ensure failures on the audit side cannot affect enforcement.
Suggested change:
if (auditSink) {
try {
const result = auditSink(record)
if (result && typeof (result as any).catch === "function") {
(result as any).catch(() => {})
}
} catch {
// Intentionally swallowed: audit-side failure cannot affect enforcement.
}
}
| // absent execute span does NOT prove enforcement. Tests must assert against the audit | ||
| // log and execute-not-called counters. | ||
|
|
||
| import { createHash } from "node:crypto" |
There was a problem hiding this comment.
[🟠 MEDIUM] This import introduces a strict dependency on Node.js/Bun. If HardPolicy (or any module importing it) is executed in a non-Node environment like a browser, Edge runtime, or a VS Code web extension, this will cause a runtime crash.
Consider using a cross-platform alternative, such as a pure JS implementation of SHA-256 (e.g., crypto-js), using the Web Crypto API (if asynchronous execution is permissible), or injecting the hashing function dynamically based on the environment.
| function resolveGovernedKey(toolID: string, source: Source): string | undefined { | ||
| if (rulesByTool.has(toolID)) return toolID | ||
| if (source !== "mcp") return undefined | ||
| for (const governed of rulesByTool.keys()) { | ||
| if (toolID.endsWith(`_${governed}`)) return governed | ||
| } | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
[🔴 HIGH] The suffix matching loop here depends on the Map insertion order. If there are overlapping suffixes (for instance, rules for both execute and sql_execute), and execute was inserted into RULES first, an MCP tool ID like warehouse_sql_execute would incorrectly match execute instead of sql_execute.
To avoid this vulnerability, sort the governed keys by length in descending order, ensuring the most specific suffix matches first.
Suggested change:
| function resolveGovernedKey(toolID: string, source: Source): string | undefined { | |
| if (rulesByTool.has(toolID)) return toolID | |
| if (source !== "mcp") return undefined | |
| for (const governed of rulesByTool.keys()) { | |
| if (toolID.endsWith(`_${governed}`)) return governed | |
| } | |
| return undefined | |
| } | |
| function resolveGovernedKey(toolID: string, source: Source): string | undefined { | |
| if (rulesByTool.has(toolID)) return toolID | |
| if (source !== "mcp") return undefined | |
| // Sort keys by length descending to ensure the most specific suffix matches first | |
| const governedKeys = Array.from(rulesByTool.keys()).sort((a, b) => b.length - a.length) | |
| for (const governed of governedKeys) { | |
| if (toolID.endsWith(`_${governed}`)) return governed | |
| } | |
| return undefined | |
| } |
| if (auditSink) { | ||
| try { | ||
| auditSink(record) | ||
| } catch { | ||
| // Intentionally swallowed: audit-side failure cannot affect enforcement. | ||
| } | ||
| } |
There was a problem hiding this comment.
[🔴 HIGH] The try/catch block only handles synchronous errors. If an externally provided auditSink returns a rejected Promise (e.g., if it's an async function), the rejection will silently bypass the catch block. This leads to an unhandled promise rejection, which can crash the Node.js process.
Safely catch potential Promise rejections to ensure failures on the audit side cannot affect enforcement.
Suggested change:
| if (auditSink) { | |
| try { | |
| auditSink(record) | |
| } catch { | |
| // Intentionally swallowed: audit-side failure cannot affect enforcement. | |
| } | |
| } | |
| if (auditSink) { | |
| try { | |
| const result = auditSink(record) | |
| if (result && typeof (result as any).catch === "function") { | |
| (result as any).catch(() => {}) | |
| } | |
| } catch { | |
| // Intentionally swallowed: audit-side failure cannot affect enforcement. | |
| } | |
| } |
…udit/ctx fidelity
Human review (dev-punia-altimate) + bot findings on the HardPolicy chokepoint:
- HIGH — bash DDL whitespace bypass: Wildcard.match compiles each pattern with a
leading `^` anchor, so `" DROP DATABASE prod"` (or a leading tab/newline) slipped
past `"DROP DATABASE *"` and defeated the hard deny. matchBashDdl now trims the
command first (leading/trailing whitespace is never semantically meaningful here).
+1 test over several whitespace-prefixed DDL forms.
- MEDIUM — audit digest Map/Set collision: Object.keys(new Map()) is [], so distinct
Maps/Sets (a tool.execute.before hook could inject one) both stringified to `{}` and
collided to the same digest. stableStringify now serializes their contents under a
distinguishing tag. +1 test.
- MEDIUM — tool-call metadata recorded pre-hook args: `ctx` was built from the caller's
original `args` before the tool.execute.before hook, so ctx.metadata()'s
updateToolCall recorded the ORIGINAL args while HardPolicy and execute used the
post-hook finalArgs. Both dispatch sites (native + MCP) now build `ctx` from finalArgs
AFTER the hook, passing input.session.id / toolCallId directly to the hook input (both
independent of args). UI/state now reflects what actually ran.
- MEDIUM — tool.execute.after on hard deny: documented that skipping the after-hook on a
policy deny is intentional (the tool never executed; synthesizing an after-event would
hand plugins a result they'd treat as a real post-execute outcome). The security
invariant holds regardless.
24 pass, typecheck clean. Markers clean (changes within altimate_change blocks).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Review comments addressed (2 rounds)Thanks to @dev-punia-altimate, @chatgpt-codex-connector, and @cubic-dev-ai. Round 2 —
Round 3 —
Deferred (with rationale): the 24 pass, typecheck clean, markers clean. |
|
To use Codex here, create an environment for this repo. |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
2 participants
PINEAPPLE
Summary
Stage S3 of the de-fork spike — the kill gate. Enforces non-bypassable hard denies (
sql_executeDDL:DROP DATABASE/DROP SCHEMA/TRUNCATE; bash DDL) at every model-invoked tool-execution dispatcher, relocating today's scattered per-tool safety checks into one audited chokepoint. Behavior-preserving — these operations already block; S3 consolidates and closes a bypass.packages/opencode/src/altimate/policy/hard-policy.ts— pure, synchronous, totalHardPolicy.check(): malformed/missing args return a deny (policy_internal_error), never throw, never implicit-allow. Emits a structured audit record on every call — this (not the trace) is the enforcement oracle the tests assert against.HardPolicy.check()inserted beforetool.executeat every dispatcher from the S2 route matrix: D1 (registry loop) + D2 (MCP loop) + D6 (direct Task dispatch) insession/prompt.ts, D3/D4 insession/tools.ts, D5 (BatchTool inner dispatch) intool/batch.ts. Fail-closed init viaassertInitialized()at the app-runtime composition seam.tool.execute.before's return value, so a plugin hook that mutated args (e.g. rewrote a benignSELECTintoDROP DATABASE) executed the mutation unchecked. Every site now captures the post-hook final args and checks those. The batch inner dispatch checks the inner tool ID + args (nottoolID="batch"), closing the nested-dispatch gap.hard_policy_deniedtool-error result (or throws in batch) —executeandtool.execute.afterare both skipped.Scope preserved: bash DDL patterns are byte-identical to today's
agent.tssafety table; the sql check reusessql-execute.ts's classifier + error string.rm -rf/git push --force/git reset --hardcorrectly stay ask-tier (not promoted to hard deny).Also includes
route-sentinels.test.tsmade line-number- and arg-agnostic (matchesargsor S3'sfinalArgs) so it passes with or without S3 applied — shared identically with PR #1014, mergeable in any order.Review
Two independent adversarial reviews (a Claude reviewer + the orchestrator's own trace of every
.execute()site) found no bypass via malformed args, mutated args, or allow-all permission config, and confirmed all 7 safety properties (coverage, final-args, total-function, deny-terminal, config-independence, fail-closed init, behavior-preservation).Test Plan
bun test test/altimate/defork/hardpolicy.test.ts: 19 pass / 0 fail — the bypass matrix across D1–D6: DDL denied + not executed at every route, near-miss controls allowed (proves rules aren't over-broad), allow-all config still denies, malformed args fail closed, deny is terminal, mutated-args checked on the post-hook value (auditfinalArgsDigestasserted).check()to always-allow fails every dispatcher-integration test.bunx tsc --noEmit -p packages/opencode: clean for the changed files.Checklist
Closes #1017
This is a security boundary and the spike's decision checkpoint — held for human go/no-go; do not auto-merge.
Summary by cubic
Adds a centralized kill gate (
HardPolicy) that enforces non-bypassable denies for destructive SQL/Bash DDL across all tool-execution dispatchers, with audited decisions. Follow-up fixes close whitespace and MCP-prefix bypasses, harden audit hashing, and make tool-call metadata reflect post-hook args. Closes #1017.New Features
packages/opencode/src/altimate/policy/hard-policy.tswith pure, sync, totalHardPolicy.check()that emits an audit record on every call.HardPolicy.assertInitialized()inpackages/opencode/src/effect/app-runtime.ts.executein all routes; on deny, returnhard_policy_denied(or throw in batch) and skipexecute/tool.execute.after.Bug Fixes
_-suffix matching; bash DDL matcher trims leading/trailing whitespace before checks.finalArgsDigestis a SHA-256 of stable-stringified args; stable stringify now encodesMap/Set; audit sink wrapped in try/catch and log trimming is amortized O(1).HardPolicy.check(),execute, audit, andctx.metadata; batch checks the inner tool ID + args.Written for commit 348e930. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests