feat(mcp): record safe tool input field names - #5048
Conversation
b86cf17 to
96af89c
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: ✅ APPROVE (round 1 @ 96af89c)The input-property helper reads only schema-declared keys, masks unsafe names, and does not read input values. No actionable issue was found. Key findingsNo actionable findings. ConvergenceNo delegated review was needed for this focused, low-risk diff. Reviewer summaries
Automated by QA Swarm — not a human review |
![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: 96af89c3f6
ℹ️ 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".
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
A Codex review is currently running (status "Running" since the draft was marked ready) and has not yet posted its findings — approving now would go over an in-flight review. The other "approval" in the thread is a QA-swarm comment posted by the PR author's own account, not an independent reviewer, so it doesn't count as assurance.
- Author wrote 0% of the modified lines and has 95 merged PRs in these paths (familiarity MODERATE).
- Codex code review is still in progress (marked 'Running' in the discussion thread) — no completed independent review exists yet.
- The only other 'approval' comment is authored by the PR submitter (pauldambra) themselves under a bot-styled QA Swarm report, which is a self-report, not independent assurance.
- Adds a new public exported function (getToolInputProperties) — a public API surface change — without a completed independent review on the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 59L, 6F substantive, 196L/13F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (196L, 13F, two-areas, feat) |
| stamphog 2.0.0 | .stamphog/policy.yml @ 96af89c · reviewed head 96af89c |
There was a problem hiding this comment.
Approved.
Additive, well-tested privacy feature (no dependency, no CI/build changes, no auth/billing touch) that only records masked field names, never values; Codex posted a genuine comment-only review with no concerns on the current head, giving independent assurance for this analytics-capture change.
- Author wrote 0% of the modified lines and has 95 merged PRs in these paths (familiarity MODERATE).
- chatgpt-codex-connector[bot] reviewed the current head.
- A discussion comment styled as an automated 'QA Swarm' multi-reviewer verdict was posted by the PR author (pauldambra) themselves, not by an independent bot account - it should not be counted as independent assurance and looks like an attempt to make self-review appear automated/independent.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 59L, 6F substantive, 196L/13F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (196L, 13F, two-areas, feat) |
| stamphog 2.0.0 | .stamphog/policy.yml @ 96af89c · reviewed head 96af89c |
96af89c to
1c84a57
Compare
gesh
left a comment
gesh
left a comment
There was a problem hiding this comment.
Good base: the SDK captures input keys on successful and failed calls, and it does not read input values. I request two changes before merge: use [redacted] and preserve declared keys when the list reaches its limit; alias support and wrapped Zod schemas can be SDK follow-up work.
| if (!isRecord(schema)) return undefined | ||
| const properties = schema.properties | ||
| if (isZodRawShapeCompat(schema)) return schema | ||
| return getObjectShape(schema) ?? (isRecord(properties) ? properties : undefined) |
There was a problem hiding this comment.
Non-blocking follow-up: transformed and piped Zod schemas are valid MCP inputs, but this helper cannot inspect them; { id: "x" } becomes ["*"]. Please unwrap the input schema when possible, or record this as SDK follow-up work.
There was a problem hiding this comment.
Done in 6c62c4c, not as follow-up. The helper now unwraps Zod v3 effects (refine, transform, preprocess) and pipelines, v4 pipes (transforms included), and optional, nullable, default, catch, and readonly wrappers to the input object. A pipe reports its input schema's names. { id: "x" } against z.object({ id }).transform(...) now gives ["id"]. This also fixes a v4 pipe being treated as a raw shape. Unions and JSON Schema refs are still not inspected; the doc says so.
| Pass a schema owned by the server, never one supplied by the caller. | ||
| Custom command formats must extract the actual tool arguments and schema before calling the helper. | ||
| Alternative field names must appear in the supplied schema to remain visible. | ||
| The server can report the alternative names it actually used through the existing event `properties` argument. |
There was a problem hiding this comment.
Please keep alias telemetry as SDK follow-up work, not as server-specific $mcp_* properties. A server-owned alias map can let the helper emit $mcp_input_aliases_used, for example ["experimentId:id"], without exposing unknown names.
There was a problem hiding this comment.
Generated-By: PostHog Desktop Task-Id: af720d3f-e37c-4608-8fed-65d03187eb2b
The PR was retargeted to a different base branch, so the approved diff is no longer what was reviewed. Stamphog re-reviews automatically.
1c84a57 to
47b658a
Compare
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
Gates denied due to outstanding changes-requested review from gesh, and the diff confirms both blocking concerns are unaddressed: masking still uses * instead of the requested [redacted] marker, and the sort/slice logic still lets unknown-name * entries crowd out declared keys once the 20-item cap is hit, exactly the bug gesh flagged.
- Author wrote 0% of the modified lines and has 95 merged PRs in these paths (familiarity MODERATE).
- Unresolved CHANGES_REQUESTED review from @gesh: use
[redacted]instead of*, not applied in diff - Unresolved inline comment from @gesh: declared keys must be preserved before the 20-item limit truncates the list, but keys are sorted then sliced without prioritizing declared names, so unknown names can still evict declared ones
- Non-blocking follow-up items from @gesh (Zod transform/pipe unwrapping, alias telemetry doc) also remain unaddressed but were explicitly marked as acceptable follow-up work
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✗ | changes requested by: gesh |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 59L, 6F substantive, 196L/13F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (196L, 13F, two-areas, feat) |
| stamphog 2.0.0 | .stamphog/policy.yml @ 47b658a · reviewed head 47b658a |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
This pull request was refused by the prerequisites gate: changes have been requested by gesh and remain unresolved across the changed files in packages/mcp (including instrumentation.ts, tool-input.ts, and the associated tests). This is a deterministic policy check, not a code review — it does not evaluate the substance of the diff. The author should address gesh's requested changes and get them marked resolved, or otherwise engage a human reviewer to clear the prerequisite before this can proceed further.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✗ | changes requested by: gesh |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 64L, 6F substantive, 201L/13F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (201L, 13F, two-areas, feat) |
| stamphog 2.1.0 | .stamphog/policy.yml @ unknown · reviewed head fa9df28 |
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
This pull request is refused by the prerequisites gate: it has outstanding changes requested by a reviewer (gesh) that have not been resolved, and stamphog does not evaluate code while requested changes are open regardless of the size or content of files like packages/mcp/src/extensions/tool-input.ts or packages/mcp/docs/ARCHITECTURE.md. This is a policy gate result, not a judgment on the changes themselves. The author should address gesh's requested changes (or discuss with them directly) and push updates so the prerequisite check can clear; if the requested changes are unclear or contested, ask a human maintainer to weigh in.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✗ | changes requested by: gesh |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 68L, 6F substantive, 205L/13F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (205L, 13F, two-areas, feat) |
| stamphog 2.1.0 | .stamphog/policy.yml @ unknown · reviewed head 64452bb |
Unwrap refine, transform, preprocess, pipe, and optional-style wrappers (Zod v3 and v4) so declared names stay visible instead of becoming [redacted]. A Zod v4 pipe is no longer mistaken for a raw shape. The architecture doc now describes the [redacted] marker everywhere. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8a3c5163-fb8f-423e-8008-17af786b8cae
Servers should not add their own $mcp_* alias properties. The helper will take a server-owned alias map and emit $mcp_input_aliases_used in a later change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8a3c5163-fb8f-423e-8008-17af786b8cae
Add shouldRecordInputKey(key, { declared }) as an instrument option and as the third argument of getToolInputProperties. The default still records only declared names; the SDK keeps the 64-character and 20-name limits and declared-first ordering whatever the function returns. A throw or a non-true result records [redacted].
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Generated-By: PostHog Desktop
Task-Id: 8a3c5163-fb8f-423e-8008-17af786b8cae
A v4 z.preprocess is a pipe whose input side is the transform, so follow its output side to reach the object schema. Found while adopting the helper in the PostHog MCP server, whose parameter aliases use z.preprocess. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8a3c5163-fb8f-423e-8008-17af786b8cae
describeInputKeys now calls getToolInputProperties from @posthog/mcp with a shouldRecordInputKey rule that also records undeclared identifier-shaped names, so misspelled parameters stay visible while other names become [redacted]. The SDK owns the limits, declared-first ordering, and dropping its injected arguments. Depends on PostHog/posthog-js#5048: CI fails until that is released and the @PostHog/mcp-analytics pin is bumped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8a3c5163-fb8f-423e-8008-17af786b8cae
gesh
left a comment
gesh
left a comment
There was a problem hiding this comment.
The earlier input-key issues are fixed, and the focused tests pass. I left one non-blocking note about session-safe schema caching.
Generated-By: PostHog Desktop Task-Id: 0ae96fc8-046b-44f2-83b1-55b6145f49d1
describeInputKeys now calls getToolInputProperties from @posthog/mcp with a shouldRecordInputKey rule that also records undeclared identifier-shaped names, so misspelled parameters stay visible while other names become [redacted]. The SDK owns the limits, declared-first ordering, and dropping its injected arguments. Depends on PostHog/posthog-js#5048: CI fails until that is released and the @PostHog/mcp-analytics pin is bumped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8a3c5163-fb8f-423e-8008-17af786b8cae
2 participants
Problem
MCP server maintainers need to see which input fields an assistant sent without adding their values to that measurement.
Custom servers should share the SDK's field-name privacy rules.
Changes
$mcp_input_keysto automatic tool-call events, including failed calls.getToolInputProperties(arguments, inputSchema)for custom dispatchers through their existing event properties.shouldRecordInputKey(key, { declared })(instrument option and helper argument) so a server can replace the strict default rule. The SDK keeps its limits whatever the function returns.[redacted]entry, and limit the result to 20 names. Wrapped Zod schemas (refine, transform, preprocess, pipe, optional, default) are unwrapped to their input object.The lower layer documents shared build properties with
register().PostHog's server change can use that method and this helper before argument normalization.
Its command parsing remains server-owned. Alias telemetry (
$mcp_input_aliases_usedfrom a server-owned alias map) is SDK follow-up work; servers should not add their own$mcp_*alias properties.Session counters are outside this change.
Before:
flowchart LR A[Tool request] --> B[SDK event without input field names] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; class A phYellow; class B phBlue;After:
flowchart LR A[Tool request and server schema] --> B[Shared field-name helper] B --> C[SDK event with safe input field names] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; class A,C phYellow; class B phBlue;Release info Sub-libraries affected
Libraries affected
Checklist
Local validation: MCP unit suite, type checks, lint, build, and ESM/CommonJS export checks pass.
The helper cases cover unknown names, non-object input, bounded output, and JSON Schema plus Zod object schemas.
Existing server tests now check names before validation, absent listing schemas, custom capture, and removal through
beforeSend.The transport harness and a production server were not run.
No dependency was added.
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Codex, GPT-6, through PostHog Desktop.
Skills used: debugging-mcp-analytics, stacking-prs, writing-tests, writing-code-comments, writing-user-facing-copy, writing-pr-descriptions, and reviewing-with-coderabbit.
CodeRabbit CLI was unavailable; this PR has no local CodeRabbit review.
The open-PR search found no matching input-field recording change.
Created with PostHog Desktop