lucas/omp-deck

Watch

Fork 0

git tools silently ignore unknown parameters #39

New issue

Closed

opened 2026-10-19 14:25:57 +02:00 by omp-deck · 1 comment

omp-deck commented

2026-10-19 14:25:57 +02:00

Collaborator

Implementation: #42

Problem

Calling git_log with {"rev": "53bb57a..origin/main", "max_count": 20} (rev is git_show's parameter name; git_log uses ref) is not rejected. The tool silently ignores rev and returns HEAD's history ("ref": "HEAD"), which looks like a valid answer to the range question. The result is wrong and no error is reported.

Root cause:

The harness (validateToolArguments in @oh-my-pi/pi-ai) passes unknown root keys through to execute unchanged. Verified: {rev:"a..b", max_count:20} arrives in git_log's params as-is. All git tools then ignore keys they don't read.
Making the zod schema .strict() does not fix this. The harness's coercion pass deletes the unrecognized key and retries validation, so the call still succeeds with {}. This was verified as well. So the rejection has to happen inside each tool's execute.

Decision: reject unknown parameters in all 8 git tools (git_status, git_diff, git_log, git_show, git_fetch, git_branch, git_commit, git_push) with an error that names the valid parameters. Do not add a rev alias. Leave git_log's ref output field unchanged.

Plan

Approach

Add a shared guard in apps/server/src/bridge/git-tools/validate.ts, next to assertRefLike:
```
/** Reject parameters the tool does not define; the harness passes unknown keys through silently. */
export function assertKnownParams(params: object, known: readonly string[]): void
```
Collect Object.keys(params) that are not in known, in input order. If there are none, return. Otherwise throw ValidationError with exactly this message:
unknown parameter(s): rev. Valid parameters: ref, max_count, skip, … (unknown keys joined with ", ". Valid keys joined with ", " in schema order. If known is empty, which is the case for git_status, the second sentence is This tool takes no parameters.).
guarded() in run.ts already turns ValidationError into a fail(message) tool error, so no change is needed there.
Wire it into every git tool (status.ts, diff.ts, log.ts, show.ts, fetch.ts, branch.ts, commit.ts, push.ts in apps/server/src/bridge/git-tools/). The omptype zod shim does not expose .shape, so keep the key list in one place:
- Hoist the object literal currently passed to z.object({...}) into const shape = {...};, then const schema = z.object(shape);. Descriptions and validators stay unchanged. For status.ts, use const shape = {};.
- As the first statement inside the guarded(...) callback in execute, call assertKnownParams(params, Object.keys(shape));. This runs before plan*(), resolveRepo, or any git call. In status.ts the parameter is currently named _params, so rename it to params.
- Do not use .strict() for the reason given in the Problem section.

Critical files & anchors

apps/server/src/bridge/git-tools/validate.ts: ValidationError, assertRefLike (around line 24). Put the new helper here.
apps/server/src/bridge/git-tools/run.ts: guarded (around line 244), which converts ValidationError into a tool error.
apps/server/src/bridge/git-tools/log.ts: schema (line 11) and execute (line 36). This is the reported case.

Verification

Add tests to apps/server/src/bridge/git-tools/validate.test.ts (bun:test, same style as the assertRefLike block) for a describe("assertKnownParams") block:
- assertKnownParams({ ref: "x", max_count: 1 }, ["ref", "max_count"]) does not throw.
- assertKnownParams({ rev: "a..b", max_count: 20 }, ["ref", "max_count"]) throws ValidationError with message unknown parameter(s): rev. Valid parameters: ref, max_count.
- assertKnownParams({ x: 1 }, []) throws with message unknown parameter(s): x. This tool takes no parameters.
Run bun test src/bridge/git-tools and bun run typecheck from apps/server.

End-to-end check (a throwaway command, run from apps/server, that confirms the harness path still delivers the key and that the tool now rejects it):

bun -e 'import {validateToolArguments} from "@oh-my-pi/pi-ai"; import {buildLogTool} from "./src/bridge/git-tools/log.ts"; const t=buildLogTool("."); const a=validateToolArguments(t,{type:"toolCall",id:"x",name:"git_log",arguments:{rev:"a..b",max_count:20}}); console.log(JSON.stringify(await t.execute("x",a,undefined,undefined,undefined)));'

Expected result: isError: true, with text starting unknown parameter(s): rev. Valid parameters: ref, max_count, skip,. The same call with {ref:"HEAD~1", max_count:1} returns one commit and no error.

Implementation: #42 ## Problem Calling `git_log` with `{"rev": "53bb57a..origin/main", "max_count": 20}` (`rev` is `git_show`'s parameter name; `git_log` uses `ref`) is not rejected. The tool silently ignores `rev` and returns HEAD's history (`"ref": "HEAD"`), which looks like a valid answer to the range question. The result is wrong and no error is reported. Root cause: - The harness (`validateToolArguments` in `@oh-my-pi/pi-ai`) passes unknown root keys through to `execute` unchanged. Verified: `{rev:"a..b", max_count:20}` arrives in `git_log`'s `params` as-is. All git tools then ignore keys they don't read. - Making the zod schema `.strict()` does **not** fix this. The harness's coercion pass deletes the unrecognized key and retries validation, so the call still succeeds with `{}`. This was verified as well. So the rejection has to happen inside each tool's `execute`. Decision: reject unknown parameters in all 8 git tools (`git_status`, `git_diff`, `git_log`, `git_show`, `git_fetch`, `git_branch`, `git_commit`, `git_push`) with an error that names the valid parameters. Do not add a `rev` alias. Leave `git_log`'s `ref` output field unchanged. ## Plan ### Approach 1. **Add a shared guard** in `apps/server/src/bridge/git-tools/validate.ts`, next to `assertRefLike`: ```ts /** Reject parameters the tool does not define; the harness passes unknown keys through silently. */ export function assertKnownParams(params: object, known: readonly string[]): void ``` Collect `Object.keys(params)` that are not in `known`, in input order. If there are none, return. Otherwise throw `ValidationError` with exactly this message: `unknown parameter(s): rev. Valid parameters: ref, max_count, skip, …` (unknown keys joined with `", "`. Valid keys joined with `", "` in schema order. If `known` is empty, which is the case for `git_status`, the second sentence is `This tool takes no parameters.`). `guarded()` in `run.ts` already turns `ValidationError` into a `fail(message)` tool error, so no change is needed there. 2. **Wire it into every git tool** (`status.ts`, `diff.ts`, `log.ts`, `show.ts`, `fetch.ts`, `branch.ts`, `commit.ts`, `push.ts` in `apps/server/src/bridge/git-tools/`). The omptype zod shim does not expose `.shape`, so keep the key list in one place: - Hoist the object literal currently passed to `z.object({...})` into `const shape = {...};`, then `const schema = z.object(shape);`. Descriptions and validators stay unchanged. For `status.ts`, use `const shape = {};`. - As the **first statement inside** the `guarded(...)` callback in `execute`, call `assertKnownParams(params, Object.keys(shape));`. This runs before `plan*()`, `resolveRepo`, or any git call. In `status.ts` the parameter is currently named `_params`, so rename it to `params`. - Do not use `.strict()` for the reason given in the Problem section. ### Critical files & anchors - `apps/server/src/bridge/git-tools/validate.ts`: `ValidationError`, `assertRefLike` (around line 24). Put the new helper here. - `apps/server/src/bridge/git-tools/run.ts`: `guarded` (around line 244), which converts `ValidationError` into a tool error. - `apps/server/src/bridge/git-tools/log.ts`: `schema` (line 11) and `execute` (line 36). This is the reported case. ### Verification - Add tests to `apps/server/src/bridge/git-tools/validate.test.ts` (bun:test, same style as the `assertRefLike` block) for a `describe("assertKnownParams")` block: - `assertKnownParams({ ref: "x", max_count: 1 }, ["ref", "max_count"])` does not throw. - `assertKnownParams({ rev: "a..b", max_count: 20 }, ["ref", "max_count"])` throws `ValidationError` with message `unknown parameter(s): rev. Valid parameters: ref, max_count`. - `assertKnownParams({ x: 1 }, [])` throws with message `unknown parameter(s): x. This tool takes no parameters.` - Run `bun test src/bridge/git-tools` and `bun run typecheck` from `apps/server`. - End-to-end check (a throwaway command, run from `apps/server`, that confirms the harness path still delivers the key and that the tool now rejects it): ``` bun -e 'import {validateToolArguments} from "@oh-my-pi/pi-ai"; import {buildLogTool} from "./src/bridge/git-tools/log.ts"; const t=buildLogTool("."); const a=validateToolArguments(t,{type:"toolCall",id:"x",name:"git_log",arguments:{rev:"a..b",max_count:20}}); console.log(JSON.stringify(await t.execute("x",a,undefined,undefined,undefined)));' ``` Expected result: `isError: true`, with text starting `unknown parameter(s): rev. Valid parameters: ref, max_count, skip,`. The same call with `{ref:"HEAD~1", max_count:1}` returns one commit and no error.

lucas added the

labels

2026-10-19 14:27:41 +02:00

omp-deck added

and removed

labels

2026-10-20 10:15:18 +02:00

omp-deck commented

2026-10-20 10:15:48 +02:00

Author

Collaborator

Decision: main now also has git_rebase (apps/server/src/bridge/git-tools/rebase.ts), added after this plan was written. It has the same silent-pass-through problem, so I'm applying the same shape hoist + assertKnownParams guard to it as well — 9 tools instead of the 8 listed. Everything else follows the plan as written.

Decision: `main` now also has `git_rebase` (`apps/server/src/bridge/git-tools/rebase.ts`), added after this plan was written. It has the same silent-pass-through problem, so I'm applying the same `shape` hoist + `assertKnownParams` guard to it as well — 9 tools instead of the 8 listed. Everything else follows the plan as written.

omp-deck referenced this issue from a commit

2026-10-20 10:16:48 +02:00

feat(git-tools): add assertKnownParams guard

omp-deck referenced this issue from a commit

2026-10-20 10:16:48 +02:00

fix(git-tools): reject unknown parameters in every git tool

omp-deck referenced this issue from a pull request that will close it,

2026-10-20 10:16:51 +02:00

fix(git-tools): reject unknown parameters in every git tool #42

lucas closed this issue

2026-10-20 11:03:40 +02:00