[codex] p0 foundation tools

[Vasanthdev2004]

What changed

• Added the typecheck package script for the v0.1 foundation validation flow.
• Added tool safety metadata for the core local tools, including bash as a prompt-gated shell tool.
• Added glob and apply_patch tools and registered them in the core tool registry.
• Added a registry-level run path so agent tool execution goes through validation/error handling instead of raw execute dispatch.
• Blocks prompt/deny tools from auto-executing through the registry until a permission grant path exists.
• Keeps apply_patch execution inside the workspace and rejects out-of-workspace cwd values.
• Buffers OpenAI-compatible tool-call argument chunks until the real tool-call id is known, avoiding stranded pending-* ids.
• Removed the optional Tool.run contract and the unused provider-definition helper so registry validation is the single object-literal execution path.
• Fixed glob truncation off-by-one and expanded safety/tool tests.
• Includes the M0 CI/build baseline from main, including build and smoke scripts.

Why

This finishes the next small P0 foundation slice after M0 without pulling in later features like MCP, plugins, subagents, or sandboxing.

Validation

• bun run typecheck passed
• bun test ./tests --timeout 15000 passed: 76 tests, 0 failures
• bun run build passed and compiled zero.exe
• bun run smoke:build passed

Notes

This PR is intentionally small and keeps full permission prompting UI, persistent grants, structured tool results, and session persistence for later v0.1 slices.

[Vasanthdev2004]

Blockers

• src/tools/registry.ts:17 — ToolSafety.permission is metadata only right now. toolRegistry.run() executes permission: 'prompt' and permission: 'deny' tools exactly the same as allow, so the new bash/apply_patch safety labels are not actually prompt-gated. Since this PR registers another write-capable tool and describes bash as prompt-gated, either wire a real permission check before execution or explicitly keep these tools out of auto-execution until the permission slice lands.
• src/tools/apply_patch.ts:23 — cwd is caller-controlled and there is no workspace boundary check. Combined with the missing permission enforcement above, a model-supplied tool call can apply a patch outside the repo by passing an arbitrary cwd. If workspace boundaries are intentionally later, this tool should still refuse non-repo/non-workspace paths or not be registered yet.
• package.json:14 / build validation — in an isolated PR [codex] p0 foundation tools #11 worktree, bun run build fails on the existing Ink react-devtools-core issue. PR Add M0 CI scripts baseline #10 fixes that separately, but this branch currently does not include it, so the PR body’s build-pass claim is not true for this PR as-is. Either merge/rebase after Add M0 CI scripts baseline #10 or remove build from the validation claim.

Non-Blocking

• src/providers/openai.ts:123 — the tool-call ordering fix handles same-chunk id/name/arguments correctly, and the agent loop handles delta-before-start when the delta already has the final id. But if an OpenAI-compatible provider emits arguments before id/name, this provider still emits pending-${index} and later starts the real id, so the arguments will not merge. Worth adding a provider-level regression test or avoiding pending ids if we want that compatibility guarantee.
• .github/CI from Add M0 CI scripts baseline #10 is not in this branch, so PR [codex] p0 foundation tools #11 has no status checks yet. Not a code blocker, but this should probably wait behind Add M0 CI scripts baseline #10 or be rebased after it.

Looks Good

• ToolRegistry.run() is the right direction for centralizing validation/error handling instead of raw execute calls.
• glob and apply_patch are small and well-scoped, with focused tests.
• The new agent-loop test catches the direct delta-before-start ordering case.
• Validation in isolated worktree: bun run typecheck passed and bun test ./tests --timeout 15000 passed with 55/55 tests.

Verdict: Changes Requested — the tool-safety contract is currently misleading because prompt/deny permissions are not enforced, and apply_patch can operate outside the workspace.

[gnanam1990]

Request changes - validation found blockers. toolRegistry.run() executes tools without enforcing permission metadata, so prompt/deny safety labels are currently metadata only. apply_patch accepts arbitrary caller-provided cwd and can apply patches outside the workspace. Build also fails on this branch because react-devtools-core is missing from this branch's dependencies; tests and tsc pass after install, but bun run build does not produce a binary.

[anandh8x]

Code Review: PR #11 — [codex] p0 foundation tools

Author: @Vasanthdev2004 → codex/p0-foundation-tools → main
Scope: 20 files, +326/-40. Adds glob + apply_patch tools, safety metadata on every tool, a registry-level run path, fixes tool-call argument accumulation when providers emit deltas before start events, adds typecheck script, and new test coverage.
Verdict: A solid foundation slice with good test discipline. One real correctness bug in the new tool-call delta handling, a couple of design smells around the new Tool / ToolRegistry.run split, and some gaps in coverage and acknowledged-but-unconstrained safety boundaries. Blocking on the delta-id bug; the rest are important follow-ups.

---

Blocking

1. Tool-call deltas with pending-${index} id are stranded in the agent loop

src/providers/openai.ts:123-129 emits a tool-call-delta event with id: acc.id || \pending-${tc.index}``:

if (tc.function?.arguments) {
  acc.arguments += tc.function.arguments;
  yield {
    type: 'tool-call-delta',
    id: acc.id || `pending-${tc.index}`,
    argumentsFragment: tc.function.arguments,
  };
}

If a provider sends argument fragments before an id arrives (some OpenAI-compatible servers do this, especially on the first chunk), the delta event uses the pending-${index} key. When the id finally arrives at src/providers/openai.ts:113, the subsequent tool-call-start event (line 119) uses the real id. Now the agent loop has two entries:

• One keyed by pending-0 with the accumulated arguments
• One keyed by the real id with name and empty arguments

src/agent/loop.ts:130-138 does:

const existing = toolCallMap.get(event.id) ?? { ... };
existing.arguments += event.argumentsFragment;
toolCallMap.set(event.id, existing);

which never merges the two entries because the keys differ. The end result: the tool call goes to the registry with arguments: '' (the real-id entry) and the model gets an "Invalid arguments" / "Failed to parse arguments" error even though the deltas were captured.

The existing test at tests/agent-loop.test.ts:82-104 only covers the case where the delta and start share the same id — it doesn't exercise the pending-${index} path that this PR introduced. So this is a regression of the very thing the PR claims to fix ("Preserved streamed tool-call arguments when providers emit argument deltas before start events").

Ask: Pick one of:

• (a) Have the provider hold deltas until the id arrives, then emit them with the real id. (Cleanest; the loop doesn't need to know about pending- keys.)
• (b) Have the agent loop look up the pending entry on tool-call-start and merge by some stable key (e.g., a streamIndex on the event).
• (c) At minimum, normalize the id in the loop: if the start event's id doesn't match any existing entry, scan the map for a pending--prefixed entry and merge.

The current behavior silently loses arguments. Add a test for the mismatched-id case before merging.

2. ToolRegistry.run duplicates ToolBase.run and the optional run path is dead

src/tools/registry.ts:18-38 and src/tools/base.ts:34-44 are nearly identical safe-execute implementations:

// ToolBase.run
async run(rawArgs) {
  const parsed = this.parameters.safeParse(rawArgs);
  if (!parsed.success) return `Error: Invalid arguments for ${this.name}: ...`;
  try { return await this.execute(parsed.data); }
  catch (err) { return `Error executing ${this.name}: ...`; }
}

// ToolRegistry.run (the no-tool.run branch)
const parsed = tool.parameters.safeParse(args);
if (!parsed.success) return `Error: Invalid arguments for ${name}: ...`;
try { return await tool.execute(parsed.data); }
catch (err) { return `Error executing ${name}: ...`; }

Every tool in the registry today is a plain object literal (readFileTool, bashTool, etc. — none extend ToolBase). So the if (tool.run) { return tool.run(args); } branch at src/tools/registry.ts:24-26 is never taken in practice. The JSDoc on src/tools/types.ts:25-31 describes the contract as if run is widely used, but it's aspirational.

Ask: Pick a design and commit:

• Option A: Make all tools extend ToolBase. Then ToolRegistry.run is a 3-line shim (lookup + tool.run(args) + unknown-tool error). The duplicate logic in ToolBase.run and the registry's fallback disappear.
• Option B: Keep the object-literal style. Then Tool.run should be removed from the Tool interface, and ToolRegistry.run owns the validation logic. The "preferred over execute" comment in types.ts becomes misleading and should be deleted.

---

Important

3. glob.ts limit check is off-by-one

src/tools/glob.ts:24-30:

for (const match of glob.scanSync({ cwd: root, onlyFiles: !include_dirs })) {
  matches.push(match);
  if (matches.length > limit) break;
}

With limit: 10, the loop pushes 11 items before breaking. The post-loop slice(0, limit) at line 39 hides the off-by-one from callers, but it's fragile. A future refactor that uses matches.length before slicing will see 11.

Ask: Check before pushing: if (matches.length >= limit) { truncated = true; break; } matches.push(match);. Then the truncation message can be set explicitly.

4. grep.ts fallback is a "not implemented" message

src/tools/grep.ts:94-97:

} catch (err) {
  return `ripgrep (rg) not found. Falling back to basic search is not yet implemented. Please install ripgrep for best results.`;
}

The JSDoc on line 95 says "Fallback to simple grep if rg not available (not ideal but works)" — but the fallback does not work. The CI workflow in PR #10 does not install rg, so a fresh CI runner will hit this path. The grep tool is currently unusable on any machine without rg preinstalled.

Ask: Either (a) install rg in the CI workflow (PR #10) before this lands, or (b) actually implement a non-rg fallback (e.g., grep -rn via execa, accepting the perf hit), or (c) mark the tool as requiring rg and document it in the description: "Requires ripgrep on PATH; will return an error otherwise."

5. apply_patch and bash have no workspace boundary enforcement

The PR description acknowledges: "This PR is intentionally small and keeps full permission prompting/enforcement, workspace boundaries, structured tool results, and session persistence for later v0.1 slices."

So the author is aware. Fine for a foundation slice, but:

• apply_patch (src/tools/apply_patch.ts:23-33) runs git apply with the model-supplied cwd. A bad patch from the model can create/delete files anywhere the process can write.
• bash (src/tools/bash.ts:23-27) runs with shell: true and a 2-minute timeout. No command allowlist.

Ask: At minimum, update each tool's description to call out the current safety gap, so the model self-limits. Example for apply_patch:

"Applies a unified diff patch with no path boundary check. Will be constrained to the active workspace in a follow-up — be conservative about which paths you patch."

This is a small JSDoc change but materially helps the model's behavior today.

6. plan tool: global mutable state

src/tools/plan.ts:18: let currentPlan: PlanItem[] = []; is module-level. runAgent clears it on every invocation (src/agent/loop.ts:40), which means concurrent agent runs in the same process will race and clobber each other's plans. Probably fine for the CLI (one process per zero invocation) but the foundation slice should at least document the constraint or scope the state to a PlanSession class that runAgent instantiates.

The test at tests/agent-loop.test.ts:46-80 uses update_plan and reads back via onToolResult — it doesn't exercise the global state path. Add a test that calls runAgent twice and asserts the second call doesn't see the first call's plan.

7. plan tool: model-supplied content not escaped

src/tools/plan.ts:32-36 interpolates item.content and item.notes directly into the result string. A model that emits terminal escape sequences can clobber the user's terminal output. Cheap to strip ANSI or render content via a String(item.content).replace(/[\u0000-\u001f\u007f]/g, '') pass.

8. getDefinitionsForProvider is dead/half-baked code

src/tools/registry.ts:40-46:

getDefinitionsForProvider() {
  return this.getAll().map(tool => ({
    name: tool.name,
    description: tool.description,
    parameters: tool.parameters.shape, // We'll convert properly later
  }));
}

The comment "We'll convert properly later" plus the .shape access (raw Zod fields, not JSON Schema) means this returns something the provider can't use. The agent loop at src/agent/loop.ts:54-74 does its own z.toJSONSchema conversion. If getDefinitionsForProvider is unused, delete it. If it's used elsewhere, fix it to match the loop's behavior.

9. Safety metadata coverage is partial

tests/foundation-tools.test.ts:31-40 only spot-checks four tools' safety fields. The contract (sideEffect: <one-of-five>, permission: <one-of-three>, reason: <non-empty string>) is unenforced for read_file, write_file, edit_file, list-directory, and plan. A single loop test would lock the contract:

for (const name of ['read_file','write_file','edit_file','list_directory','plan','grep','glob','apply_patch','bash']) {
  const safety = toolRegistry.get(name)?.safety;
  expect(safety).toBeDefined();
  expect(['read','write','shell','network','out_of_workspace']).toContain(safety!.sideEffect);
  expect(['allow','prompt','deny']).toContain(safety!.permission);
  expect(safety!.reason.length).toBeGreaterThan(0);
}

10. plan tool sideEffect: 'read' is debatable

The plan tool mutates in-memory state but doesn't touch the filesystem. The sideEffect taxonomy seems to be about durable side effects (read/write/shell/network/out_of_workspace). If so, plan is correctly 'read'. But the description says "Updates in-memory planning state only." which contradicts the "read" classification (reads don't update). Either:

• Move plan to a new sideEffect category like 'state' or 'memory', or
• Update the JSDoc on ToolSideEffect to clarify the criterion is "durable side effect."

---

Suggestions

11. Debug box padding is fragile

src/agent/loop.ts:88-90:

console.log(`│ Messages: ${messages.length}${' '.repeat(40 - String(messages.length).length)}│`);

If messages.length is ≥ 100, the padding goes negative and the box breaks. The toolDefinitions.length > 0 line pads with a fixed 33 spaces regardless of the stringified boolean. Cheap to fix with a helper:

function pad(s: string, width: number) { return s + ' '.repeat(Math.max(0, width - s.length)); }

Minor — only fires in debug mode.

12. toolDefinitions recomputed every turn

src/agent/loop.ts:53-75 runs z.toJSONSchema on every tool's parameters for every turn. The schemas are static. Move the conversion out of the per-turn loop into a one-time setup.

13. Tool<T extends z.ZodObject<any>> is over-restrictive

src/tools/types.ts:19 constrains T to z.ZodObject<any>, but future tools might use z.union(...) or top-level primitives. The current any is a hole, but a better typing would be T extends z.ZodTypeAny. Not blocking — current tools all use z.object.

14. Two different "invalid arguments" error shapes

src/agent/loop.ts:177-179 returns "Error: Failed to parse arguments for X: <json error>" when JSON parse fails, and src/tools/registry.ts:30 / src/tools/base.ts:37 return "Error: Invalid arguments for X: <zod error>" when Zod parse fails. Both end up as tool result strings, but the model has to disambiguate. Consolidate to one error format, e.g., always "Error: invalid arguments for X: <details>".

15. Parallel tool execution may race on filesystem

src/agent/loop.ts:172-197 runs all tool calls in parallel via Promise.all. If the model emits bash("git add .") + edit_file("a.ts") + apply_patch(...) in the same turn, the filesystem mutations are unordered. The model is responsible for sequencing, but consider documenting this or adding a flag for sequential execution.

16. apply_patch test coverage is minimal

tests/foundation-tools.test.ts:54-74 only covers a single-file modify with @@ -1,2 +1,2 @@. Doesn't cover:

• Multi-file patches
• New file (--- /dev/null) / delete file
• Malformed patch
• Patch that doesn't apply (e.g., context mismatch)
• cwd fallback when not specified

For a tool that mutates the filesystem, the failure-mode coverage is thin.

17. No test for glob with include_dirs: true

src/tools/glob.ts:8 accepts include_dirs, but tests/foundation-tools.test.ts:43-52 only exercises the default include_dirs: false path.

18. No test for the new "registry.run" path with a tool that doesn't have a run method

The test at tests/registry.test.ts:55-61 exercises registry.run with a makeTool (object literal, no .run method). It checks the happy path and the Zod-fail path. Add a test for the unknown-tool path (registry.run('nope', {}) returns "Error: Unknown tool").

19. Tool<T> interface leaks Zod details

The parameters: T field on the Tool interface forces every consumer of Tool to know about Zod. If a future provider wants a different schema system, this won't generalize. Not blocking for M0, but worth noting that parameters: unknown (with the Zod-to-JSON conversion in one place) would be more flexible.

---

Questions for the author

1. The toolCallAccumulators map in src/providers/openai.ts:70-75 is keyed by index. What happens if the provider sends a delta with index: 0 after a tool-call-end for index 0? The code emits tool-call-end for index 0 but the same index could be reused for a new tool call later. Test it.
2. The apply_patch tool uses git apply — does the zero CLI assume the workspace is always a git repo? If a user runs zero outside a git checkout, apply_patch will fail. Should the tool check git rev-parse and surface a clearer error?
3. The runAgent function takes provider: Provider but no session / plan / config object. As more state accumulates (plan, conversation history, tool sandbox), is the function signature going to grow? Consider introducing a RunContext now even if it's empty.
4. The toolRegistry singleton in src/tools/index.ts:21 is a module-level mutable. Tests that import toolRegistry share state across test files. Bun's test runner isolates modules per test file, but if any test file mutates the registry (it doesn't today, but a future one might), it'll be a debugging nightmare. Worth passing a registry instance to runAgent instead of importing the singleton.

---

Validation

Local checks claimed in the PR body: bun run typecheck ✓, bun test ./tests --timeout 15000 55/0 ✓, bun run build ✓ produces zero.exe. The new test file tests/agent-loop.test.ts covers the delta-before-start happy path but not the pending-${index} case (the bug I flagged in #1). Math: I traced the new test for tool-call flow (tests/agent-loop.test.ts:45-80) and the order of events checks out for the shared-id case.

---

Recommendation

Request changes. The pending-${index} keying bug in #1 is a real correctness regression of the very thing the PR claims to fix; it needs a regression test before merge. The duplication in #2 is a design smell that compounds as the registry grows — pick one shape now. The unconstrained bash / apply_patch paths are acknowledged in the PR description but should at least get a description-level warning so the model self-limits.

Once #1, #2, #3, and #5 (with a description update) are addressed, this is a clean foundation slice.

[anandh8x]

Blocking — pending-${tc.index} ids are stranded in the agent loop. When the provider sends argument fragments before the id arrives, this yields tool-call-delta events with id: \pending-${tc.index}`. The subsequent tool-call-startat line 119 uses the realid, so the agent loop's toolCallMapends up with two entries: one keyedpending-0(with the accumulated args) and one keyed by the real id (withnamebut emptyarguments). src/agent/loop.ts:130-138does not merge them, so the tool is dispatched witharguments: ''and the model gets an "Invalid arguments" error. The existing test attests/agent-loop.test.ts:82-104only covers the same-id case — it doesn't exercise this regression. Pick one: (a) hold deltas in the provider until the id arrives, then emit with the real id; (b) add astreamIndexto the event and merge in the loop; or (c) scan the map forpending--prefixed entries on tool-call-start`. Add a regression test that exercises the mismatched-id path before merging.

[anandh8x]

Blocking — run duplicates ToolBase.run and the optional run branch is dead code. src/tools/base.ts:34-44 and src/tools/registry.ts:24-37 are nearly identical safe-execute implementations. Every current tool is a plain object literal (no run method), so the if (tool.run) return tool.run(args); branch at line 24-26 is never taken. The JSDoc on src/tools/types.ts:25-31 describes run as the preferred path, but in practice the registry's inline fallback is always used. Pick a design: (a) make all tools extend ToolBase and let the registry be a 3-line shim, or (b) keep object literals and remove run from the Tool interface (with the execute-direct contract). Current state is dead code plus duplication.

[anandh8x]

Important — off-by-one in limit check. if (matches.length > limit) break; runs after the push, so the loop accumulates limit + 1 items before exiting. The post-loop slice(0, limit) at line 39 hides this from callers but it's fragile — any future refactor that uses matches.length before slicing will see 11 with limit: 10. Check before pushing: if (matches.length >= limit) { truncated = true; break; } matches.push(match); and set the truncation message explicitly.

[anandh8x]

Important — call out the missing workspace boundary in the tool description. The PR description acknowledges that workspace boundaries are deferred, but the tool's description (line 15) doesn't mention it. A model that doesn't know there's no path check could patch arbitrary paths. Update the description to: "Applies a unified diff patch with no path boundary check. Will be constrained to the active workspace in a follow-up — be conservative about which paths you patch." Cheap, materially helps the model self-limit today.

[anandh8x]

Important — same concern for bash. shell: true with a model-supplied string and no command allowlist. The description says "Execute a shell command and return the output. Use for running commands, git, tests, etc." — append a one-liner about the current lack of command allowlist so the model self-limits.

[anandh8x]

Suggestion — two different 'invalid arguments' error shapes. JSON parse failure returns "Error: Failed to parse arguments for X: ..." (line 179); Zod parse failure returns "Error: Invalid arguments for X: ..." (src/tools/registry.ts:30 and src/tools/base.ts:37). The model has to disambiguate. Consolidate to one shape, e.g., always "Error: invalid arguments for X: <details>".

[anandh8x]

Suggestion — parallel tool execution may race on filesystem. Promise.all(toolPromises) runs all tool calls in parallel. If the model emits bash('git add .') + edit_file('a.ts') + apply_patch(...) in the same turn, the mutations are unordered. The model is responsible for sequencing, but consider a flag for sequential execution or documenting this constraint.

[anandh8x]

Suggestion — apply_patch test coverage is thin. Only covers single-file modify with @@ -1,2 +1,2 @@. Add coverage for: new file (--- /dev/null), delete file, malformed patch, context mismatch (patch that doesn't apply), and cwd fallback. The tool mutates the filesystem — failure-mode coverage matters.

[anandh8x]

Suggestion — no test for glob with include_dirs: true. Line 44 only exercises the default include_dirs: false path. Add a case where a directory is matched and verify it's in the output.

[anandh8x]

Suggestion — no test for the unknown-tool registry path. Lines 55-61 only exercise the happy path and Zod-fail path. Add expect(await registry.run('missing', {})).toBe('Error: Unknown tool "missing".').

[gnanam1990]

Request changes - new head validates better, but the provider/agent-loop streamed tool-call fix is still incomplete. If an OpenAI-compatible provider emits argument fragments before the final tool-call id, src/providers/openai.ts still yields the early fragment as id pending-, then emits tool-call-start/tool-call-end with the real id. src/agent/loop.ts keeps those as separate map entries, so arguments are stranded and the real tool call is dispatched with incomplete/invalid JSON. I reproduced this locally with a runAgent probe and got repeated JSON parse errors instead of the update_plan args. The existing test only covers delta-before-start when delta and start share the same id, so it misses this path. Please add a regression for the pending-id case and either buffer provider deltas until id/name are known or carry a stable stream index so the loop can merge pending args into the real call. Validation otherwise: git diff --check passed, bun install --frozen-lockfile passed, bun test ./tests --timeout 15000 passed 71/71, tsc --noEmit passed, bun run build passed, ./zero --help passed.

[gnanam1990]

Code Review Summary

Verdict: Changes Requested

Thanks for the update. The latest head fixes the two issues I previously called out:

• OpenAI-compatible streamed tool-call argument fragments are now buffered until the real tool-call id is available, so the agent no longer mixes pending-<index> ids with the final id.
• Tool execution now routes through toolRegistry.run(), so schema validation, unknown-tool handling, and safety metadata are no longer bypassed.

Blocking issue

• src/agent/loop.ts:185 / src/tools/registry.ts:18-25 — Prompt-gated tools are now never executable from the agent loop.

  runAgent() calls toolRegistry.run(tc.name, parsedArgs) without any path to pass permissionGranted: true. Since ToolRegistry.run() rejects every tool whose safety.permission !== 'allow' unless that option is set, tools marked prompt — including bash, apply_patch, write_file, and edit_file — are advertised to the provider but always return Error: Permission required ... was not executed.

  That is safer than the old behavior, but it leaves the new foundation tools effectively unusable in normal agent runs. Please either:

  1. add a real permission request/grant path in the agent loop/UI and pass permissionGranted: true only after approval, or
  2. intentionally keep prompt-gated tools disabled for M0, but then do not advertise them to the provider yet and document that mutating/shell tools are registered but unavailable until permission UX lands.

Looks good

• The streamed tool-call tests cover the prior OpenAI-compatible provider failure mode.
• Registry tests now cover validation, unknown tools, and prompt-gate rejection.
• CI is green across ubuntu, macOS, and Windows.

[anandh8x]

Re-review: APPROVE.

The two blockers and most of the important items from the prior review (00e5c04) are resolved in 6b41fce0. The branch has been rebased onto main and now also includes the M0 CI scripts baseline (PR #10) and the model registry (PR #9) — both already-approved slices riding along.

Status against prior review

Blocking (2/2 resolved):

• ✅ pending-${tc.index} ids stranded — src/providers/openai.ts:115-138 now gates tool-call-delta emission on acc.id && acc.started, so deltas with no real id are buffered and only emitted when id+name are both known. Two new regression tests cover it: tests/openai-provider.test.ts: 'buffers argument deltas until the real tool call id arrives' and tests/agent-loop.test.ts: 'keeps tool arguments when a delta arrives before the start event'.
• ✅ ToolRegistry.run duplicates ToolBase.run — Option B was taken: ToolBase.run is gone, ToolRegistry.run owns the validation, and the Tool interface no longer has an optional run method. The dead getDefinitionsForProvider is also deleted.

Important (6/8 resolved):

• ✅ glob.ts off-by-one — now if (matches.length >= limit) break; before the push.
• ✅ apply_patch workspace boundary — description and code now claim "Paths outside the workspace are rejected"; cwd boundary is enforced and tested ('refuses to apply patches outside the workspace'). Individual file paths inside the patch are still delegated to git apply --directory, which is acceptable for this slice.
• ✅ bash safety description — now reads: "No command allowlist exists yet, so only run conservative workspace-safe commands after permission is granted."
• ✅ Safety metadata contract — tests/foundation-tools.test.ts: 'marks every registered tool with valid safety metadata' iterates every tool and asserts each sideEffect/permission/reason is valid. Contract is now enforced, not spot-checked.
• ⚠️ plan sideEffect: 'read' is debatable — still 'read'. The taxonomy is for durable side effects, which plan doesn't have, so this is a documentation gap more than a code bug. Defer to a follow-up if the taxonomy gets revisited.

Suggestions (2/9 resolved):

• ✅ glob include_dirs test — added ('can include directory matches').
• ✅ Unknown-tool registry test — added ('reports unknown tools from the registry run path').
• ⚠️ Items not addressed: parallel tool race on filesystem, per-turn toolDefinitions recompute, debug-box padding math, two error shapes (JSON-parse vs Zod-parse), apply_patch thin coverage of new-file/delete/malformed cases, Tool<T extends z.ZodObject> over-restrictive type. All reasonable to defer to follow-up slices — none block correctness of the foundation.

Test coverage — tests/foundation-tools.test.ts (108 lines, 9 cases), tests/registry.test.ts adds 5 cases including unknown-tool and prompt-gating, tests/agent-loop.test.ts exercises the system-prompt selection and tool-call flow, tests/openai-provider.test.ts exercises the buffered-delta fix. Solid for an M0 foundation slice.

One bonus fix noticed in passing: the new OpenAIProvider.streamCompletion correctly handles toolCallAccumulators index reuse — if a new id arrives at the same index after a previous one, the old tool call is closed with tool-call-end and a fresh accumulator is created. This is the question I raised in the prior review (Q1); it was answered by code.

Verdict

APPROVE. All blockers and the design issues are addressed. The remaining nits are deferred follow-ups, not regressions. Ready to merge once CI is green and PR #10 / PR #9 are confirmed already on main (the branch includes both — make sure no merge conflict lands).

[Vasanthdev2004]

Pushed fix commit for the latest requested-change blocker.

What changed:

• runAgent() now advertises only safety.permission === 'allow' tools to providers until permission UX exists.
• Prompt/deny tools remain registered and guarded by toolRegistry.run(), but the model is no longer invited to call tools the loop cannot approve.
• Added an agent-loop regression test confirming bash, apply_patch, write_file, and edit_file are not advertised while read-safe tools still are.

Validation run locally:

• bun test tests/agent-loop.test.ts tests/registry.test.ts --timeout 15000 — 13 pass / 0 fail
• bun run typecheck — pass
• bun test ./tests --timeout 15000 — 77 pass / 0 fail
• bun run build — pass, built zero.exe
• bun run smoke:build — pass

This should resolve the prompt-gated-tools blocker without adding premature permission UI.

[gnanam1990]

Code Review Summary

Verdict: Approved

Re-reviewed latest commit 39479d8 after the previous requested-change blocker.

The new fix addresses the remaining issue I raised:

• runAgent() now filters provider tool definitions to only advertise tools with safety.permission === 'allow' until a real permission UX exists.
• Prompt/deny tools remain registered and guarded in ToolRegistry.run(), but the model is no longer invited to call tools the agent loop cannot approve.
• The added agent-loop test covers this behavior and verifies bash, apply_patch, write_file, and edit_file are withheld while read/plan tools remain available.

The prior streamed OpenAI-compatible tool-call fix and registry validation/safety gate changes still look good.

CI is green on ubuntu, macOS, and Windows.