test: [de-fork S2] execution-route matrix + sentinel tests#1014
test: [de-fork S2] execution-route matrix + sentinel tests#1014anandgupta42 wants to merge 4 commits into
Conversation
Stage S2 of the de-fork spike: the authoritative map of every tool-execution DISPATCHER (the point where a resolved tool's `.execute()` actually runs), so S3's HardPolicy knows every chokepoint it must cover. Analysis + executable sentinel tests only — zero product-code change. - `docs/internal/defork-route-matrix.md` — ingress surfaces vs execution dispatchers (D1-D8), each classified active / latent / out-of-scope with file:line evidence. Key findings: `SessionTools.resolve` (D3/D4) is LATENT (zero production callers at HEAD); the real bypass is BatchTool inner dispatch (`tool/batch.ts:104`) which skips `Plugin.trigger`; all active dispatchers live in 3 files. Also flags a pre-existing security gap: the server-v2 `shell` HTTP route has no permission check. - `packages/opencode/test/altimate/defork/route-sentinels.test.ts` — an executable sentinel per active route, driving a harmless tool through the real dispatch code and asserting execution reaches the documented line. Test Plan: - `bun test test/altimate/defork/route-sentinels.test.ts`: 9 pass, 0 fail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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. |
📝 WalkthroughWalkthroughThe change adds a de-fork spike specification, an S2 execution-route matrix, and sentinel tests covering selected direct, batched, and task-tool dispatchers through runtime probes and source-text assertions. ChangesDe-fork spike specification
De-fork route coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/internal/defork-route-matrix.md (1)
57-57: 🔒 Security & Privacy | 🔵 TrivialConsider filing a tracking issue for the flagged
shellHTTP permission gap.This row correctly surfaces that the server-v2
shellroute only checks session existence, with no permission/ruleset gate — a real gap even though it's out of this spike's scope. Since it's documented here but not tracked anywhere actionable, worth opening a follow-up issue so it doesn't get lost.🤖 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 `@docs/internal/defork-route-matrix.md` at line 57, File a follow-up tracking issue for the server v2 shell HTTP route identified in the SessionPrompt.shell row, documenting that its requireSession check lacks a permission or Ruleset gate around raw command execution. Keep the documentation row’s out-of-scope classification unchanged and reference the new issue in the row so the security gap has an actionable owner.
🤖 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.
Nitpick comments:
In `@docs/internal/defork-route-matrix.md`:
- Line 57: File a follow-up tracking issue for the server v2 shell HTTP route
identified in the SessionPrompt.shell row, documenting that its requireSession
check lacks a permission or Ruleset gate around raw command execution. Keep the
documentation row’s out-of-scope classification unchanged and reference the new
issue in the row so the security gap has an actionable owner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf40245f-9130-4bad-a91f-3d4f6b9e6523
📒 Files selected for processing (2)
docs/internal/defork-route-matrix.mdpackages/opencode/test/altimate/defork/route-sentinels.test.ts
![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: d3b811963f
ℹ️ 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 promptSrc = readSrc("session/prompt.ts") | ||
| const toolSourceSrc = readSrc("altimate/tool-source.ts") |
There was a problem hiding this comment.
This latent-route guard only examines session/prompt.ts and altimate/tool-source.ts, so a future production caller added in any other source file will leave the test green and D3/D4 incorrectly classified as latent. Since the stated purpose is to fail as soon as the resolver gains a real caller, search all production TypeScript sources for SessionTools.resolve or imports of session/tools, excluding the definition itself and comments.
Useful? React with 👍 / 👎.
| This document separates **ingress surfaces** (how a request enters the system) | ||
| from **execution dispatchers** (where a tool call is actually dispatched), and | ||
| records, for every dispatcher, whether it is currently reachable in production | ||
| (`active`), wired but with no current caller (`latent`), or intentionally | ||
| outside HardPolicy's model-invoked boundary (`out-of-scope`). |
There was a problem hiding this comment.
The matrix says every resolved Tool.Info execution point is classified, but createUserMessage directly initializes and executes ReadTool for text and directory file parts at src/session/prompt.ts:2001 and :2070, with ask: () => Effect.void. These user-attachment routes do not funnel through any listed dispatcher; even if they are intentionally outside the model-invoked HardPolicy boundary, they need an explicit out-of-scope entry so the S3 audit is complete.
Useful? React with 👍 / 👎.
| Status: draft for S3 handoff. Analysis-only; no product code was modified to | ||
| produce this document. Governed by S2 of `docs/internal/2026-07-18-defork-spike-spec.md` | ||
| (rev 4, Codex-consensus-approved). |
There was a problem hiding this comment.
The referenced docs/internal/2026-07-18-defork-spike-spec.md exists in neither this commit nor its parent, and a repo-wide path/content search finds no available copy. Consequently reviewers cannot verify the declared S2 requirements or the later citation to line 61; commit the specification or replace these references with the actual accessible source.
Useful? React with 👍 / 👎.
| | D2 | `SessionPrompt.resolveTools` — MCP-tools loop | `src/session/prompt.ts:1630-1739`, dispatch `const result = await execute(args, opts)` at `1669` | Real, but wildcard: `AppRuntime.runPromise(ctx.ask({permission: key, patterns: ["*"], always: ["*"]}))` at `1656-1667` — always-allow-with-audit-record, not a fine-grained deny-capable gate. Comment documents a **previously fixed bug**: `await ctx.ask(...)` alone only awaited the Effect *object*, never ran `PermissionNext.ask` — MCP tools executed with no permission check until this was corrected (`upstream_fix` marker). | Same merge as D1 (wildcard pattern) | Yes / Yes (`1644-1654`, `1671-1680`) | Yes, if any MCP server is configured | **active** | | ||
| | D3 | `SessionTools.resolve` — registry-tools loop | `src/session/tools.ts:27` (fn), context/`ask` at `44-75`, dispatch `yield* item.execute(args, ctx)` at `97` | Real: `permission.ask({..., ruleset}).pipe(Effect.orDie)` at `66-74` | `Permission.merge(input.agent.permission, input.session.permission ?? [])` | Yes / Yes (`92-96`, `109-113`) | N/A — **zero production callers**. Exhaustive grep for `SessionTools` outside its own definition file returns only two doc-comments in `src/session/prompt.ts` (noting the shared `stampRegistryToolSource`/`describeMcpTool` helpers) and two doc-comment references in `src/altimate/tool-source.ts`. No call site exists at HEAD. | **latent** | | ||
| | D4 | `SessionTools.resolve` — MCP-tools loop | `src/session/tools.ts:124-216`, dispatch `Effect.promise(() => execute(args, opts))` at `145` | Real (with same wildcard-pattern caveat as D2) at `144` | Same merge as D3 | Yes / Yes (`138-142`, `156-160`) | N/A, same as D3 | **latent** (bundled with D3 — same unused resolver) | | ||
| | D5 | `BatchTool` inner dispatch | `src/tool/batch.ts:104` — `await AppRuntime.runPromise(tool.execute(validatedParams, toEffectContext(ctx, partID)))` | **Forwarded, not independently gated.** `toEffectContext` (`batch.ts:16-28`) passes the *same* `ctx.ask` the outer dispatcher (D1/D2/D3/D4/D6, whichever invoked "batch") supplied — BatchTool adds no wrapper-level, tool-specific `ask` of its own. Per-inner-tool permission enforcement therefore depends entirely on whether that inner tool's own implementation calls `ctx.ask` internally (many built-ins, e.g. `bash`, do). | Same ruleset as whichever outer dispatcher invoked "batch" (forwarded, not re-derived) | **No** — confirmed by full read of `tool/batch.ts`: no `Plugin.trigger` call anywhere in the file. `stampRegistryToolSource` is also never applied to inner results. This is the genuine, precisely-scoped bypass: batched inner tool calls skip plugin hooks and source-stamping, not permission checks outright. | Yes — the model can invoke `batch` from any agent that has it enabled, and can batch any tool present in `ToolRegistry.tools(...)` except `batch` itself (`DISALLOWED`, `batch.ts:11`) | **active** | |
There was a problem hiding this comment.
When batch remains allowed but an inner tool is denied, this route can bypass that deny rather than merely skipping hooks and source stamps. BatchTool obtains every registry tool without the current agent at src/tool/batch.ts:62 and invokes it directly; inner tools such as warehouse_remove (src/altimate/tools/warehouse-remove.ts:10-35) never call ctx.ask, so a configured warehouse_remove: deny rule is never evaluated. The matrix should identify this as a permission bypass so S3 gates every inner dispatch independently.
Useful? React with 👍 / 👎.
| | # | Dispatcher | File:line | `ctx.ask` wiring | Ruleset | `Plugin.trigger` before/after | Reachable under allow-all | State | | ||
| |---|---|---|---|---|---|---|---| | ||
| | D1 | `SessionPrompt.resolveTools` — registry-tools loop | `src/session/prompt.ts:1521` (fn), context/`ask` at `1562-1572`, dispatch `await AppRuntime.runPromise(item.execute(args, ctx))` at `1601`, source-stamp `1614` | Real: `PermissionNext.ask({..., ruleset})` wrapped in `Effect.promise` | `PermissionNext.merge(input.agent.permission, input.session.permission ?? [])` | Yes / Yes (`1589-1599`, `1615-1624`) | Yes — this is the primary, always-reachable per-turn tool dispatch path (called from `loop()` at `918`) | **active** | | ||
| | D2 | `SessionPrompt.resolveTools` — MCP-tools loop | `src/session/prompt.ts:1630-1739`, dispatch `const result = await execute(args, opts)` at `1669` | Real, but wildcard: `AppRuntime.runPromise(ctx.ask({permission: key, patterns: ["*"], always: ["*"]}))` at `1656-1667` — always-allow-with-audit-record, not a fine-grained deny-capable gate. Comment documents a **previously fixed bug**: `await ctx.ask(...)` alone only awaited the Effect *object*, never ran `PermissionNext.ask` — MCP tools executed with no permission check until this was corrected (`upstream_fix` marker). | Same merge as D1 (wildcard pattern) | Yes / Yes (`1644-1654`, `1671-1680`) | Yes, if any MCP server is configured | **active** | |
There was a problem hiding this comment.
The MCP call is not “always-allow-with-audit-record”: PermissionNext.ask evaluates the MCP tool key and the "*" pattern against the merged ruleset, throws on an explicit deny, and blocks for a reply when the result is ask (src/permission/next.ts:138-168). The always field is only used to persist approval patterns after an always reply; the wildcard merely prevents argument-level distinctions. Misclassifying this as non-deny-capable can lead S3 to preserve the wrong policy behavior.
Useful? React with 👍 / 👎.
| | D3 | `SessionTools.resolve` — registry-tools loop | `src/session/tools.ts:27` (fn), context/`ask` at `44-75`, dispatch `yield* item.execute(args, ctx)` at `97` | Real: `permission.ask({..., ruleset}).pipe(Effect.orDie)` at `66-74` | `Permission.merge(input.agent.permission, input.session.permission ?? [])` | Yes / Yes (`92-96`, `109-113`) | N/A — **zero production callers**. Exhaustive grep for `SessionTools` outside its own definition file returns only two doc-comments in `src/session/prompt.ts` (noting the shared `stampRegistryToolSource`/`describeMcpTool` helpers) and two doc-comment references in `src/altimate/tool-source.ts`. No call site exists at HEAD. | **latent** | | ||
| | D4 | `SessionTools.resolve` — MCP-tools loop | `src/session/tools.ts:124-216`, dispatch `Effect.promise(() => execute(args, opts))` at `145` | Real (with same wildcard-pattern caveat as D2) at `144` | Same merge as D3 | Yes / Yes (`138-142`, `156-160`) | N/A, same as D3 | **latent** (bundled with D3 — same unused resolver) | | ||
| | D5 | `BatchTool` inner dispatch | `src/tool/batch.ts:104` — `await AppRuntime.runPromise(tool.execute(validatedParams, toEffectContext(ctx, partID)))` | **Forwarded, not independently gated.** `toEffectContext` (`batch.ts:16-28`) passes the *same* `ctx.ask` the outer dispatcher (D1/D2/D3/D4/D6, whichever invoked "batch") supplied — BatchTool adds no wrapper-level, tool-specific `ask` of its own. Per-inner-tool permission enforcement therefore depends entirely on whether that inner tool's own implementation calls `ctx.ask` internally (many built-ins, e.g. `bash`, do). | Same ruleset as whichever outer dispatcher invoked "batch" (forwarded, not re-derived) | **No** — confirmed by full read of `tool/batch.ts`: no `Plugin.trigger` call anywhere in the file. `stampRegistryToolSource` is also never applied to inner results. This is the genuine, precisely-scoped bypass: batched inner tool calls skip plugin hooks and source-stamping, not permission checks outright. | Yes — the model can invoke `batch` from any agent that has it enabled, and can batch any tool present in `ToolRegistry.tools(...)` except `batch` itself (`DISALLOWED`, `batch.ts:11`) | **active** | | ||
| | D6 | Direct Task-tool dispatch (subtask replay/continuation) | `src/session/prompt.ts:517-625` inside `loop()`; `Plugin.trigger("tool.execute.before", ...)` at `580-588`, dispatch `await AppRuntime.runPromise(taskTool.execute(taskArgs, taskCtx))` at `625`, `Plugin.trigger("tool.execute.after", ...)` at `636-644` | Real: `PermissionNext.ask({..., ruleset})` at `612-621` | `PermissionNext.merge(taskAgent.permission, session.permission ?? [])` | Yes / Yes | Yes — fires whenever a pending `task`-type item is popped off the session's task queue (subtask continuation), independent of the current turn's LLM tool_calls | **active** | |
There was a problem hiding this comment.
For this exact pending-subtask route, taskCtx.extra.bypassAgentCheck is always set to true at src/session/prompt.ts:597, and TaskTool.execute only calls ctx.ask when that flag is false (src/tool/task.ts:96-107). Therefore the documented PermissionNext.ask closure is never exercised by D6, so describing it as a real permission gate hides that these direct task executions are currently ungated and need dispatcher-level policy enforcement.
Useful? React with 👍 / 👎.
| All test names below are verified verbatim against `route-sentinels.test.ts` | ||
| as committed (9 tests, all passing via `bun test | ||
| packages/opencode/test/altimate/defork/route-sentinels.test.ts`). |
There was a problem hiding this comment.
The documented command cannot produce the claimed nine passing tests from either plausible working directory: at the repository root, the root bunfig.toml sets the test root to the deliberately nonexistent do-not-run-tests-from-root, while from packages/opencode the prefixed path resolves to a nonexistent nested packages/opencode/... path. Document the package-local invocation, such as cd packages/opencode && bun test test/altimate/defork/route-sentinels.test.ts, so the S2 result is reproducible.
Useful? React with 👍 / 👎.
| | D6 | Direct Task-tool dispatch (subtask replay/continuation) | `src/session/prompt.ts:517-625` inside `loop()`; `Plugin.trigger("tool.execute.before", ...)` at `580-588`, dispatch `await AppRuntime.runPromise(taskTool.execute(taskArgs, taskCtx))` at `625`, `Plugin.trigger("tool.execute.after", ...)` at `636-644` | Real: `PermissionNext.ask({..., ruleset})` at `612-621` | `PermissionNext.merge(taskAgent.permission, session.permission ?? [])` | Yes / Yes | Yes — fires whenever a pending `task`-type item is popped off the session's task queue (subtask continuation), independent of the current turn's LLM tool_calls | **active** | | ||
| | D7 | `TaskTool`'s own recursive prompt call | `src/tool/task.ts` → recursively invokes `SessionPrompt.prompt` for the spawned subagent session | N/A — delegates to D1/D2 for the child session's own tool loop | Child session's own agent/session ruleset | N/A at this layer (delegates) | Yes, whenever `task`/subagent tool is invoked | **active** (not a distinct dispatch primitive — routes back into D1/D2 for the child session) | | ||
| | D8 | CLI `debug agent --tool <id>` handler | `src/cli/cmd/debug/agent.handler.ts:60` — `const result = yield* tool.execute(params, toolCtx)` | Real: `ctx.ask` at `196-205` evaluates `Permission.evaluate(req.permission, pattern, ruleset)` and throws `PermissionV1.DeniedError` on `deny` | `Permission.merge(agent.permission, session.permission ?? [])` (`186`) | **No** — full-file read confirms zero `Plugin` import/usage in `agent.handler.ts`. Inconsistent with every other dispatcher above. | Only reachable via the `opencode debug agent` developer CLI command, not via any model-facing surface | **active, but out-of-scope** per the spec's "arbitrary code outside HardPolicy's model-invoked boundary" carve-out — it is a human-operated debug utility, not something the model can trigger. Flagged here because the missing `Plugin.trigger` is a real inconsistency worth fixing regardless of HardPolicy scope. | | ||
| | — | `SessionPrompt.shell` | `src/session/prompt.ts:2418-2664`, raw process spawn at `2580` (`spawn(shell, args, {...})`) | N/A — never touches `ToolRegistry`/`resolveTools`/any `Tool.Info.execute` | N/A | Only extensibility hook is `Plugin.trigger("shell.env", {cwd, sessionID, callID}, {env: {}})` at `2575-2579` — this is the spec's own named `out-of-scope` example, matched verbatim. | N/A | **out-of-scope** (matches spec's own example). **Flag:** the server v2 `shell` HTTP route (`src/server/routes/instance/httpapi/handlers/session.ts:367-388`) gates this with **only** `requireSession(ctx.params.sessionID)` (`371`) — a session-existence check, **no permission/Ruleset check of any kind**. Formally out of HardPolicy's model-invoked-tool boundary, but worth a security follow-up outside this spike since it is a raw-exec HTTP route with no ask-gate. | |
There was a problem hiding this comment.
The same ungated raw-shell path is exposed by the v1 Hono endpoint at src/server/routes/session.ts:1004-1034, whose handler directly calls SessionPrompt.shell without any permission/Ruleset check. Restricting the warning to the v2 handler leaves users of the legacy /session/:sessionID/shell route out of the stated security follow-up even though it reaches the identical process spawn.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // Documented as `latent`: wired but with zero production callers at HEAD. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| test("D3/D4: SessionTools.resolve has no production caller (latent guard)", () => { |
There was a problem hiding this comment.
SUGGESTION: Latent guard only inspects two known files — a new caller anywhere else in src/ would slip through
The test name and inline comment say this guards SessionTools.resolve against gaining "a real call site outside src/session/tools.ts itself", but the implementation only reads session/prompt.ts and altimate/tool-source.ts — the two files where doc-comment references were already known to exist. A future PR that calls SessionTools.resolve(...) in any other file (e.g. under src/acp/, src/server/, or a brand-new module) would not trip this guard, so D3/D4's latent classification could silently become wrong. Consider scanning every .ts file under src/ for SessionTools.resolve( (e.g. via Bun.Glob or a recursive readdir) and asserting the only hit is inside src/session/tools.ts.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| const input: any = { | ||
| agent: { name: "build", mode: "primary", permission: {}, options: {} }, |
There was a problem hiding this comment.
SUGGESTION: Use [] instead of {} for agent.permission to match the documented PermissionV1.Ruleset shape
The D1 test in this same file (lines 75-79) explicitly documents that permission is a PermissionV1.Ruleset (array of rules, not a bare object — confirmed at src/agent/agent.ts:55), and that passing {} instead of [] makes rule.permission/rule.pattern undefined and crashes Wildcard.match() deep inside Skill.available. The D3/D4 test passes here only because every service that would consume input.agent.permission is mocked (Permission.Service.ask returns Effect.void, and InvalidTool never calls ctx.ask), so Permission.merge({}, []) is never computed. Aligning on [] keeps the synthetic input shape-consistent with the rest of the file and prevents a confusing crash if the mocks are ever loosened.
| agent: { name: "build", mode: "primary", permission: {}, options: {} }, | |
| agent: { name: "build", mode: "primary", permission: [], options: {} }, |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Incremental Review (26db15d → 235295a)The incremental diff (2 files, +34/−6):
The two prior SUGGESTIONs on lines 222 and 338 remain open on unchanged lines (current-code re-verified). Files in incremental diff:
Files Reviewed (2 files in incremental diff)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 26db15d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 26db15d)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Incremental Review (970c043 → 26db15d)The incremental diff (3 files, +142/−29):
The two prior SUGGESTIONs on lines 220 and 336 remain open on unchanged lines. Files in incremental diff:
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 970c043)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Incremental Review (d3b8119 → 970c043)The incremental diff refactors three structural-pin tests from hardcoded line numbers to content/uniqueness-based assertions (merge-safe with S3 Files in incremental diff:
Fix these issues in Kilo Cloud Previous review (commit d3b8119)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Reviewed by glm-5.2 · Input: 58.4K · Output: 6.8K · Cached: 479.6K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 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/test/altimate/defork/route-sentinels.test.ts">
<violation number="1" location="packages/opencode/test/altimate/defork/route-sentinels.test.ts:148">
P2: Hardcoded `/tmp` instead of a disposable temp directory. D1 and D5 tests in this same file use `await using tmp = await tmpdir(...)` for isolation, but D3/D4 hardcodes `/tmp` — a shared system path. `Instance.provide` caches instances by directory, so parallel runs or other tests targeting `/tmp` could collide or reuse stale state.</violation>
<violation number="2" location="packages/opencode/test/altimate/defork/route-sentinels.test.ts:221">
P2: The `permission` field here is `{}` (an empty object), but the same file's D1 test (lines 75–79) explicitly documents that `permission` is a `PermissionV1.Ruleset` — an array of rules — and that passing `{}` instead of `[]` causes `rule.permission`/`rule.pattern` to be undefined, crashing `Wildcard.match()` deep inside `Skill.available`. This test only passes because every service that would consume `agent.permission` is mocked away. Aligning on `[]` keeps the synthetic input consistent with the documented shape and prevents a confusing failure if mocks are loosened.</violation>
</file>
<file name="docs/internal/defork-route-matrix.md">
<violation number="1" location="docs/internal/defork-route-matrix.md:4">
P2: The referenced `docs/internal/2026-07-18-defork-spike-spec.md` does not appear to exist in this commit or its parent. The matrix cites it here as its governing specification, and the D6 test section references a specific line (`:61`) from it. If this file hasn't been committed, reviewers cannot verify the declared S2 requirements or the cited line. Either commit the specification in this PR or replace these references with an accessible source.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| const input: any = { | ||
| agent: { name: "build", mode: "primary", permission: {}, options: {} }, |
There was a problem hiding this comment.
P2: The permission field here is {} (an empty object), but the same file's D1 test (lines 75–79) explicitly documents that permission is a PermissionV1.Ruleset — an array of rules — and that passing {} instead of [] causes rule.permission/rule.pattern to be undefined, crashing Wildcard.match() deep inside Skill.available. This test only passes because every service that would consume agent.permission is mocked away. Aligning on [] keeps the synthetic input consistent with the documented shape and prevents a confusing failure if mocks are loosened.
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 221:
<comment>The `permission` field here is `{}` (an empty object), but the same file's D1 test (lines 75–79) explicitly documents that `permission` is a `PermissionV1.Ruleset` — an array of rules — and that passing `{}` instead of `[]` causes `rule.permission`/`rule.pattern` to be undefined, crashing `Wildcard.match()` deep inside `Skill.available`. This test only passes because every service that would consume `agent.permission` is mocked away. Aligning on `[]` keeps the synthetic input consistent with the documented shape and prevents a confusing failure if mocks are loosened.</comment>
<file context>
@@ -0,0 +1,345 @@
+ }
+
+ 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.
P2: Hardcoded /tmp instead of a disposable temp directory. D1 and D5 tests in this same file use await using tmp = await tmpdir(...) for isolation, but D3/D4 hardcodes /tmp — a shared system path. Instance.provide caches instances by directory, so parallel runs or other tests targeting /tmp could collide or reuse stale state.
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 148:
<comment>Hardcoded `/tmp` instead of a disposable temp directory. D1 and D5 tests in this same file use `await using tmp = await tmpdir(...)` for isolation, but D3/D4 hardcodes `/tmp` — a shared system path. `Instance.provide` caches instances by directory, so parallel runs or other tests targeting `/tmp` could collide or reuse stale state.</comment>
<file context>
@@ -0,0 +1,345 @@
+
+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>
| # De-fork spike — S2 execution-route trace + bypass matrix | ||
|
|
||
| Status: draft for S3 handoff. Analysis-only; no product code was modified to | ||
| produce this document. Governed by S2 of `docs/internal/2026-07-18-defork-spike-spec.md` |
There was a problem hiding this comment.
P2: The referenced docs/internal/2026-07-18-defork-spike-spec.md does not appear to exist in this commit or its parent. The matrix cites it here as its governing specification, and the D6 test section references a specific line (:61) from it. If this file hasn't been committed, reviewers cannot verify the declared S2 requirements or the cited line. Either commit the specification in this PR or replace these references with an accessible source.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/internal/defork-route-matrix.md, line 4:
<comment>The referenced `docs/internal/2026-07-18-defork-spike-spec.md` does not appear to exist in this commit or its parent. The matrix cites it here as its governing specification, and the D6 test section references a specific line (`:61`) from it. If this file hasn't been committed, reviewers cannot verify the declared S2 requirements or the cited line. Either commit the specification in this PR or replace these references with an accessible source.</comment>
<file context>
@@ -0,0 +1,112 @@
+# De-fork spike — S2 execution-route trace + bypass matrix
+
+Status: draft for S3 handoff. Analysis-only; no product code was modified to
+produce this document. Governed by S2 of `docs/internal/2026-07-18-defork-spike-spec.md`
+(rev 4, Codex-consensus-approved).
+
</file context>
The D1/D3/D4/D6 structural pins asserted the dispatch chokepoint at a hardcoded line index and with the exact arg name. S3's HardPolicy insertions shift those lines and rename the dispatched args to `finalArgs` (it must check the post-`tool.execute.before` args), which would break these pins once S3 lands. Rewrite them to assert the chokepoint pattern occurs exactly once, matching both `args` and `finalArgs`, so this file passes identically with or without S3 applied — mergeable 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 970c043d12
ℹ️ 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".
| | D5 `BatchTool` inner dispatch | "D5: BatchTool dispatches inner tool.execute without Plugin.trigger" (real execution, via `initTool(BatchTool)`) + "D5: BatchTool never calls Plugin.trigger for inner tool calls (structural bypass)" (structural) | | ||
| | D6 direct Task dispatch | "D6: direct Task dispatch chokepoints are pinned at the documented lines" (structural) + "D6: existing real-execution proof exists in test/session/prompt.test.ts" (citation of `it.instance("failed subtask preserves metadata on error tool state"`) — **partial compliance, flagged**: this route does not have a fresh, self-contained real-execution sentinel in `route-sentinels.test.ts` itself; see the compliance note in that file's D6 section for rationale (reconstructing the cited test's harness would duplicate ~250 lines of module-private test infrastructure from `prompt.test.ts` that isn't exported anywhere). Team-lead should decide whether to accept this as a documented S2 exception or require a follow-up to export a shared harness. | | ||
|
|
||
| **D2 (`resolveTools` MCP-tools loop) is intentionally absent from this table.** |
There was a problem hiding this comment.
When the MCP-only permission or dispatch code at src/session/prompt.ts:1656-1669 regresses, the documented nine-test suite still passes because D2 is intentionally omitted here. The D1 test cannot cover that branch: it invokes the registry-backed InvalidTool, whose implementation never calls ctx.ask, so it exercises neither the MCP wrapper nor its permission gate. Since D2 is classified as active and this document promises an executable sentinel for every active route, add a fake-MCP execution test that asserts both the ask and the underlying execute call.
Useful? React with 👍 / 👎.
| test("D6: existing real-execution proof exists in test/session/prompt.test.ts", () => { | ||
| const promptTestSrc = readFileSync(join(SRC, "..", "test", "session", "prompt.test.ts"), "utf8") | ||
| expect(promptTestSrc).toContain('it.instance("failed subtask preserves metadata on error tool state"') |
There was a problem hiding this comment.
This assertion only proves that a test declaration string exists; the documented command runs this sentinel file alone and never executes test/session/prompt.test.ts. It therefore remains green if the cited test stops reaching taskTool.execute while retaining its name—for example, that test currently returns without failure when errorTool(taskMsg.parts) is absent at prompt.test.ts:837-838. Run a shared D6 harness or otherwise execute and assert the dispatch here so this active route has the promised real-execution proof.
Useful? React with 👍 / 👎.
| const lines = readSrc("session/prompt.ts").split("\n") | ||
| // Content-based + arg-agnostic (matches `taskArgs` or S3's `finalTaskArgs`) — see D1's note. | ||
| expect(lines.filter((l) => l.includes("PermissionNext.merge(taskAgent.permission, session.permission ?? []),")).length).toBe(1) | ||
| expect(lines.filter((l) => l.includes("AppRuntime.runPromise(taskTool.execute(") && l.includes(", taskCtx)")).length).toBe(1) |
There was a problem hiding this comment.
SUGGESTION: Dropped Plugin.trigger assertions weaken the D5-vs-D6 structural distinction
The previous form of this test asserted "tool.execute.before" (old line 580) and "tool.execute.after" (old line 636) flank the task dispatch. Those markers are exactly what the matrix uses to tell D6 apart from D5 — the BatchTool bypass whose defining property is no Plugin.trigger anywhere in tool/batch.ts (matrix line 53, still structurally enforced by the D5 sentinel at line 305 of this file; D6 is documented Plugin.trigger Yes / Yes at matrix line 54). After this refactor, stripping Plugin.trigger from D6 would invalidate the matrix's D6 row but leave this sentinel green, and the cited real-execution test (prompt.test.ts:806) doesn't assert on Plugin.trigger either. If the assertions were dropped only because those strings aren't unique enough for .length).toBe(1) (they appear 6x in session/prompt.ts), consider an indexOf-style check that the unique dispatch line is wrapped by some preceding tool.execute.before and following tool.execute.after, so the D5/D6 distinction stays enforced on both sides.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…mit spec - Commit the governing spec (`2026-07-18-defork-spike-spec.md`) the matrix references, so reviewers can verify the S2 requirements (Codex). - D2/D4 MCP: corrected — `PermissionNext.ask` IS deny-capable (evaluates key + `*` pattern, throws on deny, blocks on `ask`); the wildcard only removes argument-level distinctions, it isn't "always-allow" (Codex). - D5 batch: reframed as a genuine PERMISSION bypass — inner tools that don't self-call `ctx.ask` (e.g. `warehouse_remove`) escape their deny rules when reached via `batch`; this is why S3 gates each inner dispatch (Codex). - D6: noted `bypassAgentCheck=true` means the task tool's own `ctx.ask` never fires, so these executions are ungated today — the reason S3 adds a dispatcher-level check at `prompt.ts:625` (Codex). - Added the direct `ReadTool` (user-attachment) dispatches as an explicit out-of-scope entry so the audit covers every `Tool.Info.execute` site (Codex). - Included the v1 Hono `shell` endpoint in the security note; filed issue #1019 to track both v1+v2 ungated shell routes (Codex + CodeRabbit). - Fixed the runnable sentinel-test command (must run from `packages/opencode`). - Latent guard now scans the ENTIRE production src tree for `SessionTools.resolve(` callers, not just two files (Codex). 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.
Actionable comments posted: 1
🤖 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 `@docs/internal/defork-route-matrix.md`:
- Line 50: The route matrix’s “every active route” coverage claim is
inconsistent with D2 and D6 lacking dedicated executable sentinels. Update
route-sentinels.test.ts to add runtime sentinels for D2 and D6, or revise the
matrix coverage contract to explicitly identify these structural/citation-only
exceptions and avoid overstating S3 HardPolicy coverage.
🪄 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: 0fab006d-a7cf-4d6c-a76f-7d4219fab8d8
📒 Files selected for processing (3)
docs/internal/2026-07-18-defork-spike-spec.mddocs/internal/defork-route-matrix.mdpackages/opencode/test/altimate/defork/route-sentinels.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/altimate/defork/route-sentinels.test.ts
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // `active` so HardPolicy coverage there is re-verified. We look for a real | ||
| // `SessionTools.resolve(` CALL on a non-comment line, excluding the resolver's | ||
| // own definition file (session/tools.ts) where the name is declared. | ||
| const glob = new Bun.Glob("**/*.ts") |
There was a problem hiding this comment.
SUGGESTION: Glob **/*.ts skips the 12 .tsx files under src/
The refactor's stated intent ("Scan the ENTIRE production src tree") is undercut by the glob pattern: .tsx files (TUI components in plugin/tui/altimate/*.tsx and cli/cmd/run/*.tsx) aren't scanned, so a SessionTools.resolve( caller added in any of them would slip through — the same shape of gap this refactor just closed for the two-file case. None of the 12 currently reference SessionTools, but the latent guard's whole purpose is to flip the moment one appears anywhere in src/.
| const glob = new Bun.Glob("**/*.ts") | |
| const glob = new Bun.Glob("**/*.{ts,tsx}") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26db15dfa4
ℹ️ 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 src = readFileSync(join(SRC, rel), "utf8") | ||
| 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}`) |
There was a problem hiding this comment.
The broadened full-tree scan still detects only the literal spelling SessionTools.resolve(. Because session/tools.ts directly exports resolve, a production caller can naturally use import { resolve } from "@/session/tools" (or alias the namespace) and invoke it without this guard noticing, leaving D3/D4 classified as latent and outside S3 coverage. This revision's new full-tree implementation is fresh evidence that the earlier guard was expanded but still does not inspect imports or other call spellings.
Useful? React with 👍 / 👎.
|
|
||
| | # | Dispatcher | File:line | `ctx.ask` wiring | Ruleset | `Plugin.trigger` before/after | Reachable under allow-all | State | | ||
| |---|---|---|---|---|---|---|---| | ||
| | D1 | `SessionPrompt.resolveTools` — registry-tools loop | `src/session/prompt.ts:1521` (fn), context/`ask` at `1562-1572`, dispatch `await AppRuntime.runPromise(item.execute(args, ctx))` at `1601`, source-stamp `1614` | Real: `PermissionNext.ask({..., ruleset})` wrapped in `Effect.promise` | `PermissionNext.merge(input.agent.permission, input.session.permission ?? [])` | Yes / Yes (`1589-1599`, `1615-1624`) | Yes — this is the primary, always-reachable per-turn tool dispatch path (called from `loop()` at `918`) | **active** | |
There was a problem hiding this comment.
For the active D1 path, the merged ruleset is only installed on ctx.ask; it is not automatically evaluated at this dispatch. The production prefilter in src/session/llm.ts:309-316 checks only input.agent.permission, so a session-level deny can still expose and execute a tool whose implementation never calls ctx.ask—for example, warehouse_remove at src/altimate/tools/warehouse-remove.ts:10-35. Presenting the merged ruleset as applied without documenting this condition hides an active permission bypass from the S3 handoff.
Useful? React with 👍 / 👎.
| test("D5: BatchTool never calls Plugin.trigger for inner tool calls (structural bypass)", () => { | ||
| const src = readSrc("tool/batch.ts") | ||
| expect(src).not.toContain("Plugin.trigger(") | ||
| expect(src).toContain("await AppRuntime.runPromise(tool.execute(validatedParams, toEffectContext(ctx, partID)))") |
There was a problem hiding this comment.
This sentinel remains green if BatchTool starts using the established Effect-service form plugin.trigger(...) instead of the static Plugin.trigger(...) spelling used by D1. That form already exists in src/session/tools.ts:92-96, so a migration of batch.ts to an injected Plugin.Service would fix the documented hook bypass without invalidating this test or forcing the matrix to be updated. Check for execution-hook calls independent of receiver spelling, or assert hook behavior dynamically.
Useful? React with 👍 / 👎.
| | D2 | `SessionPrompt.resolveTools` — MCP-tools loop | `src/session/prompt.ts:1630-1739`, dispatch `const result = await execute(args, opts)` at `1669` | Real and **deny-capable**: `AppRuntime.runPromise(ctx.ask({permission: key, patterns: ["*"], always: ["*"]}))` at `1656-1667`. Correction: `PermissionNext.ask` DOES evaluate the tool key + `"*"` pattern against the merged ruleset — it throws on an explicit `deny` and blocks for a reply when the result is `ask` (`src/permission/next.ts:138-168`). The `always: ["*"]` field only persists approval patterns after an `always` reply; the wildcard `patterns` merely prevents *argument-level* distinctions — it does NOT make the gate non-deny-capable. Comment documents a **previously fixed bug**: `await ctx.ask(...)` alone only awaited the Effect *object*, never ran `PermissionNext.ask` — MCP tools executed with no permission check until this was corrected (`upstream_fix` marker). | Same merge as D1 (wildcard pattern) | Yes / Yes (`1644-1654`, `1671-1680`) | Yes, if any MCP server is configured | **active** | | ||
| | D3 | `SessionTools.resolve` — registry-tools loop | `src/session/tools.ts:27` (fn), context/`ask` at `44-75`, dispatch `yield* item.execute(args, ctx)` at `97` | Real: `permission.ask({..., ruleset}).pipe(Effect.orDie)` at `66-74` | `Permission.merge(input.agent.permission, input.session.permission ?? [])` | Yes / Yes (`92-96`, `109-113`) | N/A — **zero production callers**. Exhaustive grep for `SessionTools` outside its own definition file returns only two doc-comments in `src/session/prompt.ts` (noting the shared `stampRegistryToolSource`/`describeMcpTool` helpers) and two doc-comment references in `src/altimate/tool-source.ts`. No call site exists at HEAD. | **latent** | | ||
| | D4 | `SessionTools.resolve` — MCP-tools loop | `src/session/tools.ts:124-216`, dispatch `Effect.promise(() => execute(args, opts))` at `145` | Real (with same wildcard-pattern caveat as D2) at `144` | Same merge as D3 | Yes / Yes (`138-142`, `156-160`) | N/A, same as D3 | **latent** (bundled with D3 — same unused resolver) | | ||
| | D5 | `BatchTool` inner dispatch | `src/tool/batch.ts:104` — `await AppRuntime.runPromise(tool.execute(validatedParams, toEffectContext(ctx, partID)))` | **Forwarded, not independently gated.** `toEffectContext` (`batch.ts:16-28`) passes the *same* `ctx.ask` the outer dispatcher (D1/D2/D3/D4/D6, whichever invoked "batch") supplied — BatchTool adds no wrapper-level, tool-specific `ask` of its own. Per-inner-tool permission enforcement therefore depends entirely on whether that inner tool's own implementation calls `ctx.ask` internally (many built-ins, e.g. `bash`, do). | Same ruleset as whichever outer dispatcher invoked "batch" (forwarded, not re-derived) | **No** — confirmed by full read of `tool/batch.ts`: no `Plugin.trigger` call anywhere in the file. `stampRegistryToolSource` is also never applied to inner results. **This is also a genuine PERMISSION bypass, not merely a hooks/stamping gap:** `BatchTool` resolves every registry tool WITHOUT the current agent (`batch.ts:62`) and invokes it directly, so an inner tool that does not call `ctx.ask` internally (e.g. `warehouse_remove`, `src/altimate/tools/warehouse-remove.ts`) is executed with NO permission evaluation — a configured `warehouse_remove: deny` rule is never applied when the tool is reached via `batch`. This is exactly why S3 gates each inner dispatch independently at `batch.ts:104` rather than relying on inner tools' own `ctx.ask`. | Yes — the model can invoke `batch` from any agent that has it enabled, and can batch any tool present in `ToolRegistry.tools(...)` except `batch` itself (`DISALLOWED`, `batch.ts:11`) | **active** | |
There was a problem hiding this comment.
BatchTool cannot be invoked by all of the listed outer dispatchers: D2/D4 iterate only MCP-provided tools, while D6 initializes and executes TaskTool explicitly at src/session/prompt.ts:524-625. The registry-backed BatchTool can currently arrive only through D1 (or D3 if that latent resolver is activated), so attributing its forwarded context and ruleset to D2/D4/D6 creates nonexistent route combinations in the authoritative S3 coverage matrix.
Useful? React with 👍 / 👎.
…tsx callers
- coderabbit MAJOR: the matrix claimed "every active route has an executable
sentinel test", which overstates coverage — D2 (MCP resolveTools loop) has only
structural pins + transitive coverage via D1, and D6 (direct Task dispatch) has
structural pins + a citation of an existing prompt.test.ts test rather than a
fresh self-contained sentinel. Replace the blanket claim with an explicit
"Coverage contract" section that states exactly what holds: dedicated
real-execution sentinels for D1 and D5; D2 and D6 as documented
transitive/citation exceptions; latent-guard for D3/D4; out-of-scope routes
listed for completeness. No longer reads as full dedicated-sentinel coverage.
- kilo P2: the D3/D4 latent-guard's caller scan globbed `**/*.ts`, skipping the
~12 `.tsx` files under `src/` — a new `SessionTools.resolve(` caller added in a
TUI `.tsx` file would slip past the guard. Broaden to `**/*.{ts,tsx}`. 9 pass.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 235295a65e
ℹ️ 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".
| `src/server/routes/experimental.ts` contains only a stale comment; no live | ||
| route. |
There was a problem hiding this comment.
At this commit, src/server/routes/experimental.ts defines numerous live Hono endpoints and is mounted at /experimental by both src/server/server.ts:324 and src/server/instance.ts:60, so it is not a file containing only a stale comment. Although the current endpoints do not execute tools, this authoritative route inventory should record that the router was inspected and has no dispatch point rather than incorrectly declaring the entire production surface inactive.
Useful? React with 👍 / 👎.
|
|
||
| ## S2 — Execution-route inventory + sentinel tests (DETAILED, Codex contract adopted) | ||
|
|
||
| 1. **Separate ingress surfaces from execution dispatchers.** Ingress: CLI `run`, TUI, server v1 routes, server v2 HttpApi, ACP, GitHub/GitLab handlers. Dispatchers (the things HardPolicy must cover): `session/tools.ts` resolver wrapper, `session/prompt.ts` parallel resolver, **BatchTool's direct registry execution (`tool/batch.ts:104` — bypasses both wrappers)**, **direct Task dispatch in `session/prompt.ts:580`**, MCP wrapper path, plus anything discovered (debug/internal direct calls). |
There was a problem hiding this comment.
In the reviewed tree, src/session/prompt.ts:580 is the tool.execute.before plugin trigger, while the direct Task dispatch defined by this requirement is taskTool.execute(...) at line 625. Since S2 explicitly inventories the seams where execution actually occurs so S3 can place HardPolicy there, citing the hook instead of the execute call contradicts the matrix and can send an implementation to the wrong location.
Useful? React with 👍 / 👎.
Review comments addressedThanks to @coderabbitai, @chatgpt-codex-connector, @cubic-dev-ai, and @kilo-code-bot. Round 3 —
Earlier doc corrections (dispatcher semantics D1/D2/D5/D6, ReadTool out-of-scope row, v1 shell endpoint + #1019, runnable command) are in |
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
PINEAPPLE
Summary
Stage S2 of the de-fork spike: the authoritative map of every tool-execution dispatcher (where a resolved tool's
.execute()actually runs), so stage S3's HardPolicy knows every chokepoint it must cover. Analysis + executable sentinel tests only — zero product-code change.Deliverables:
docs/internal/defork-route-matrix.md— ingress surfaces vs execution dispatchers (D1–D8), each classifiedactive/latent/out-of-scopewith file:line evidence.packages/opencode/test/altimate/defork/route-sentinels.test.ts— an executable sentinel per active route, driving a harmless tool through the real dispatch code and asserting execution reaches the documented line.Key findings (these shape S3's HardPolicy)
SessionTools.resolve(D3/D4) is latent — zero production callers at HEAD (a guard test flips if that changes), so it isn't a current bypass.tool/batch.ts:104) — it skipsPlugin.triggerand source-stamping.session/prompt.ts,tool/batch.ts,session/tools.ts) → S3's ≤3-file audited-patch budget is feasible.shellHTTP route has only a session-existence check, no permission gate.Test Plan
bun test test/altimate/defork/route-sentinels.test.ts: 9 pass / 0 fail (verified in the primary dev worktree).Checklist
Closes #1011
Summary by cubic
Maps all tool-execution dispatchers and adds sentinel tests to pin and exercise each route; commits the S2 spec and clarifies MCP checks and the
BatchToolpermission bypass. Adds an explicit coverage contract and expands latent-guard scanning; no product code changes. Closes #1011.docs/internal/defork-route-matrix.mdupdated with a precise “Coverage contract” (D1/D5 have real-execution sentinels; D2/D6 use documented transitive/citation coverage), plus out-of-scope entries and the v1+v2shellroute gap (tracked in Security: shell HTTP routes (v1 + v2) spawn processes with no permission gate #1019); committeddocs/internal/2026-07-18-defork-spike-spec.md.packages/opencode/test/altimate/defork/route-sentinels.test.ts(9 passing) with line- and arg-agnostic chokepoint pins; latent guard now scans the entiresrctree including.tsxforSessionTools.resolve(callers; D1 and D5 exercise real dispatch.PermissionNext.askis deny-capable with wildcard patterns; D5BatchToolinner dispatch is a real permission bypass; D6 direct task dispatch is ungated today (bypassAgentCheck=true). All active dispatchers remain insession/prompt.ts,tool/batch.ts,session/tools.ts.Written for commit 235295a. Summary will update on new commits.
Summary by CodeRabbit
Documentation
Tests