Skip to content

fix(pi): reject null tool arguments before pi-schema coercion - #844

Draft
G0-0000 wants to merge 2 commits into
cortexkit:masterfrom
G0-0000:fix/pi-1.5-null-tool-args
Draft

G0-0000 wants to merge 2 commits into
cortexkit:masterfrom
G0-0000:fix/pi-1.5-null-tool-args

Conversation

@G0-0000

@G0-0000 G0-0000 commented Nov 2, 2026 •

Copy link
Copy Markdown

Refs #843 (kept draft until the issue carries design-approved).

Harness coverage (required)

  • OpenCode 1: not applicable — surface exists, but its zod schema rejects null before the handler; no change needed.
  • OpenCode 2: not applicable — same zod path as OpenCode 1.
  • Pi: covered — prepareArguments added to ctx_note and ctx_memory; tests added for both.
  • OMP: covered — shares the same pi-plugin tool definitions and hook.
  • Rust module: not applicable — no tool-argument schema there.

What

pi 1.5.0 validates tool arguments with @earendil-works/pi-schema, which coerces loosely typed arguments before validation. For required properties, null is not dropped: a string schema receives "null" and an array schema receives [null], whose number items become 0. As a result:

  • ctx_note with content: null writes the literal text "null" as the note body.
  • ctx_memory with content: null writes the literal text "null" as the memory body.
  • ctx_memory with ids: null becomes [0]; get returns nothing instead of rejecting, and update/archive look up id 0.

This change adds ToolDefinition.prepareArguments to both tools. The hook runs before schema validation and before execute(), so it still sees the raw argument:

  • reject content === null in both tools;
  • reject ids when it is present and not an array;
  • accept the literal string "null" unchanged.

It also tightens ctx_memory.ids from Type.Array(Type.Number()) to Type.Array(Type.Integer({ minimum: 1 })), matching ctx_note.note_ids; memory ids start at 1.

Testing

bun test packages/pi-plugin/src/tools/ctx-memory.test.ts packages/pi-plugin/src/tools/ctx-note.test.ts
50 pass / 0 fail

bun test packages/pi-plugin/src/tools/
162 pass / 0 fail

bun run --cwd packages/pi-plugin typecheck
exit 0

biome check (from packages/pi-plugin, repo-local 2.5.1)
Checked 4 files, no fixes applied

The new tests were also run without the source change: exactly the nine new cases fail, so they cover the regression rather than merely passing.


Summary by cubic

Fixes pi 1.5.0's @earendil-works/pi-schema coercion mangling tool arguments in ctx_note and ctx_memory. content: null was written as the literal text "null", ids: null became [0] so get/update/archive targeted id 0, and fractional ids like 1.5 could be truncated into the integer that modified the wrong record.

  • Adds prepareArguments to both tools, which runs before schema validation and execute() and rejects any non-string content and non-array ids; the literal string "null" remains accepted.
  • Rejects fractional ids in execution so they can't modify existing records, and adds minimum: 1 to ctx_memory.ids, matching memory ids starting at 1.

Written for commit bd9edf8. Summary will update on new commits.

View guided diff

pi 1.5.0 validates tool arguments with @earendil-works/pi-schema, which coerces loosely typed values before validation: a required string sent as null becomes "null", and a required array of numbers sent as null becomes [0]. ctx_note and ctx_memory then store or look up garbage instead of rejecting the call.

Add prepareArguments to both tools; it runs before validation and before execute(), so it still sees the raw value. Reject only a raw null for content and a non-array ids. Tighten ctx_memory.ids to Type.Integer({ minimum: 1 }), matching ctx_note.note_ids. The literal string "null" stays accepted.
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

#843 has the design-approved label; this PR can be reviewed.

@magic-alfonso

magic-alfonso Bot commented Nov 6, 2026

Copy link
Copy Markdown

The design is approved (issue 843 now has the design-approved label), with one change: please reject any content that isn't a string, not only null. As the second comment on the issue shows, Pi's coercion also turns a number into text, so typeof content !== "string" (when content is present) covers both. The literal text "null" should stay accepted, as your tests already check. The ids check and the minimum: 1 schema are fine as they are. Once that's in and CI passes, I'll merge it.

@magic-alfonso

magic-alfonso Bot commented Nov 6, 2026

Copy link
Copy Markdown

Thanks for the update. The prepareArguments approach works, but the merge gate fails on three golden tests (smart-notes-dreamer-gate.test.ts, smart-notes-enabled.test.ts, ctx-note-smart.test.ts), because the PR changes the tool schemas. Correcting my earlier comment: please leave the schemas as they are on master after all.

  • Keep both schemas byte-identical to master. Revert note_ids from Type.Number back to Type.Integer, and drop the new minimum: 1 on ctx_memory.ids. Tool definitions are pinned on purpose: a change alters the tool list every Pi session sends, which costs a full prompt-cache rewrite. The pins also keep Pi's tool bytes in step with OpenCode's.
  • Check the ids in prepareArguments instead, on the raw values before Pi's coercion: ids and note_ids, when present, must be an array whose items are all positive integers (Number.isInteger(n) && n >= 1); otherwise throw. That covers null, [null], fractional ids and 0 without touching the schema.
  • Write comments and test names in English, to match the rest of the codebase. Two of the new comments and one test name are in Chinese.

With those changes the golden tests pass unchanged, and I'll merge once CI is green.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants