Skip to content

fix(core): drop undefined metadata values from permission requests - #37679

Closed
rvaccone wants to merge 1 commit into
anomalyco:v2from
rvaccone:fix/tool-metadata-undefined
Closed

rvaccone wants to merge 1 commit into
anomalyco:v2from
rvaccone:fix/tool-metadata-undefined

Conversation

@rvaccone

Copy link
Copy Markdown

Issue for this PR

Closes #37650

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Pending glob and grep permissions store absent optional inputs as undefined inside permission metadata (for example metadata: { path: input.path, limit: input.limit }). The metadata record is Schema.Record(Schema.String, Schema.Unknown) and the wire codec serializes Unknown as a JSON value, so session.permission.list fails to encode its response with Expected JSON value, got undefined.

This strips undefined values in request() in packages/core/src/permission.ts, the one place every pending request is constructed for both assert and ask. That fixes both tools at once and protects the event publish path as well as the list and get endpoints. I audited every permission.assert and permission.ask call site: these two tools are the only ones producing undefined values and all metadata is flat, so a shallow strip is sufficient. A deep JSON round trip was rejected because it would silently normalize values that should fail loudly.

How did you verify your code works?

Added one regression test: ask for a permission whose metadata contains undefined values, then assert the pending request omits those keys. It fails without the fix and passes with it. I also reproduced the reported error (Expected JSON value, got undefined at ["path"]) against the JSON value codec using the old metadata shape and confirmed the stripped shape encodes cleanly. The permission and tool test suites in packages/core (111 tests) and tsgo --noEmit all pass.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@rvaccone

Copy link
Copy Markdown
Author

The linked issue is #37650, referenced with Closes #37650 in the description. GitHub only registers a formal closing link when a PR targets the default branch, so this check reports zero linked issues for any PR based on v2 (for example #37479 and #37480 currently carry the same label). Happy to adjust if there is a preferred way to reference issues on v2 PRs.

@rvaccone

Copy link
Copy Markdown
Author

#37965 fixes the same two tools on dev at the source. This PR targets v2, which is the tree #37650 was reported against, so the two cover different branches rather than duplicating each other. Happy to close this one if you would rather take the source level approach in both places.

@rvaccone

rvaccone commented Aug 4, 2026

Copy link
Copy Markdown
Author

Bumping this. The needs:issue label is a false positive. The description links #37650, but the bot only detects closing links on PRs that target the default branch. On direction, I can keep this fix as is or port the source level approach from #37965 to v2 and close this PR. Let me know which you prefer.

@github-actions

Copy link
Copy Markdown
Contributor

Automated PR Cleanup

Thank you for contributing to opencode.

Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions.

This PR was closed because it matched the following cleanup criteria:

  • The PR was created more than 1 month ago
  • The PR had fewer than 2 positive reactions
  • Positive reactions are counted as thumbs-up, heart, celebration, or rocket reactions on the PR

PRs created within the last month are not affected by this cleanup.

If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate.

Thanks again for taking the time to contribute.

@rvaccone

Copy link
Copy Markdown
Author

Requesting a reopen. The bug is still live on v2 at HEAD: packages/core/src/tool/plugin/glob.ts:76 and grep.ts:91 still put optional path, limit, and include into permission metadata, and request() in packages/core/src/permission.ts:204 passes it through unchanged, so session.permission.list fails with Expected JSON value, got undefined. #37650 is open and #37965 is unmerged, so neither branch is covered.

This PR was never reviewed, so the reaction count says nothing about whether the fix is needed.

One question: keep this fix, or port the approach from #37965 to v2? I'll do either, or close this if you only want it on dev.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant