Skip to content

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

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

fix(core): drop undefined metadata values from permission requests#37679
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.

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