Protect annotations from parser mutation - #787
Conversation
Expose annotations through protected read-only views so custom parsers can no longer mutate caller- or context-owned annotation payloads through parser state. This adds regression coverage for the public parser entrypoints and runWith() paths, updates the annotation contract in docs, and documents the fix in CHANGES.md. The verification workflow also needed stabilization in this workspace: mise test now runs its component tasks sequentially, and pwsh completion tests skip when this environment can invoke pwsh but cannot load the generated completion script correctly. Fixes #491 Co-Authored-By: OpenAI Codex <codex@openai.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #787 +/- ##
==========================================
- Coverage 87.39% 86.87% -0.53%
==========================================
Files 44 44
Lines 28558 29563 +1005
Branches 6936 7123 +187
==========================================
+ Hits 24959 25682 +723
- Misses 3514 3793 +279
- Partials 85 88 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 3f55aecc7a
ℹ️ 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".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements annotation isolation by exposing annotations through protected read-only views using Proxies, preventing custom parsers from mutating caller-owned data (addressing issues #491 and #787). The implementation covers various container types like Maps, Sets, and URLs, and includes comprehensive tests and documentation updates. Review feedback identifies a leak in the URLSearchParams proxy where the original target is passed to callbacks, and a violation of the repository style guide regarding the use of the system temporary directory instead of a project-local one for test scripts.
There was a problem hiding this comment.
Pull request overview
This PR prevents caller- and context-supplied annotations from being mutated by custom parsers by changing getAnnotations(state) to return protected, read-only views (with memoized protected views for common container types). It also updates tests, public exports, and documentation to reflect the new contract and adds a small local verification workflow tweak.
Changes:
- Introduces a protection layer in
packages/core/src/annotations.tsthat exposes annotations via read-only proxy views (including nested container protections) and updatesgetAnnotations()to returnReadonlyAnnotations. - Adds/updates regression and unit tests to ensure annotation mutation attempts throw and do not affect caller/context-owned objects.
- Updates exports/docs/changelog and stabilizes local verification (
mise.toml) + PowerShell completion integration tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/annotations.ts | Core change: protected read-only views for annotations; getAnnotations() now returns ReadonlyAnnotations. |
| packages/core/src/annotations.test.ts | Adds focused unit tests for stable protected views and mutation failures across representative container types. |
| packages/core/src/parser.test.ts | Adds regression coverage ensuring public parser/doc entrypoints don’t allow mutation of caller-owned annotations. |
| packages/core/src/facade.test.ts | Adds regression coverage ensuring runWith()/runWithSync() don’t allow mutation of context-owned annotations. |
| packages/core/src/modifiers.test.ts | Updates assertions to avoid relying on raw annotation object identity. |
| packages/core/src/index.ts | Re-exports ReadonlyAnnotations from the public core entrypoint. |
| packages/core/src/context.ts | Re-exports ReadonlyAnnotations for context consumers. |
| packages/core/src/completion.test.ts | Skips pwsh completion integration when pwsh can’t load the generated script in the current env. |
| docs/concepts/extend.md | Documents the new read-only-view contract for getAnnotations() and the fail-fast mutation behavior. |
| CHANGES.md | Adds changelog entry describing the annotation isolation fix and links to the issue/PR. |
| mise.toml | Runs the top-level test workflow sequentially for more stable local verification. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPublic parser entrypoints and runner-driven context injection now wrap caller-supplied annotations in protected, read-only views before attaching them to parser state. The change adds Proxy/readonly wrappers and specialized protections for Map, Set, Date, RegExp, URL, and URLSearchParams that throw TypeError on mutation attempts, caches protected view identity, and ensures isolation between runs. A new ReadonlyAnnotations type is exported and getAnnotations/injectAnnotations signatures were updated; docs and tests were revised to assert protected-view semantics across parse/suggest/getDocPage and runWith entrypoints. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/facade.test.ts`:
- Line 10273: The current assertion only checks phase2 is truthy and can miss
phase1 leaking through; update the check on the test's result variable so it
asserts both that phase2 === true and that phase1 is not present/true. Replace
the current assert.equal((result as { readonly phase2?: boolean }).phase2, true)
with a stricter assertion such as assert.ok((result as { readonly phase2?:
boolean; readonly phase1?: boolean }).phase2 === true && !((result as
any).phase1)), ensuring you reference the same result cast used in the test so
phase-one leakage is caught.
In `@packages/core/src/modifiers.test.ts`:
- Around line 7519-7520: Replace the boolean equality assertions using
assert.equal(..., true) with assert.ok(...) in the test: locate the marker
retrieval (const marker = Object.getOwnPropertySymbols(annotations)[0]) and the
assertion that checks getAnnotations(seenState)?.[marker] and change it to
assert.ok(getAnnotations(seenState)?.[marker]); also update the other similar
check around the getAnnotations(seenState) usage (the pair at lines shown) to
use assert.ok(...) (or assert.ok(!...) if checking false) so the tests follow
the assertion guideline.
In `@packages/core/src/parser.test.ts`:
- Around line 2737-2741: The async mutation test fixture's complete() currently
returns Promise.resolve({...}) which prevents synchronous throws from
mutateAnnotationPayload(state, marker) from becoming rejected promises; change
the complete method to be declared async (async complete() { /* deno-lint-ignore
require-await */) and return the result object directly (return { success: true
as const, value: mutateAnnotationPayload(state, marker) }); so synchronous
exceptions are converted to rejected promises and the fixture matches the
pattern used by constructs.test.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9f9c459-e2b3-4d23-afdb-fa7df4d8a95c
📒 Files selected for processing (11)
CHANGES.mddocs/concepts/extend.mdmise.tomlpackages/core/src/annotations.test.tspackages/core/src/annotations.tspackages/core/src/completion.test.tspackages/core/src/context.tspackages/core/src/facade.test.tspackages/core/src/index.tspackages/core/src/modifiers.test.tspackages/core/src/parser.test.ts
Address follow-up review findings in the protected annotation view layer. Plain object and array views now use detached read-only snapshots with throwing setters, which avoids proxy invariant violations on frozen inputs while still failing fast on mutation attempts. URLSearchParams views now also keep callback ownership on the protected view instead of leaking the mutable target. Co-Authored-By: OpenAI Codex <codex@openai.com>
Address follow-up review comments on the updated test expectations. The phase-two refinement assertions now explicitly guard against phase-one leakage, and the annotation-marker checks in modifiers tests now use assert.ok() instead of boolean assert.equal() patterns. Co-Authored-By: OpenAI Codex <codex@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/annotations.ts`:
- Around line 91-103: The cacheProtectedMethod helper uses Map<PropertyKey,
unknown>, losing compile-time type safety; update its signature to accept a
generic value type parameter (e.g., cache: Map<PropertyKey, V>, factory: () =>
V, and function generic <T, V = T> or simply <V>) so the cached value keeps its
concrete type, update occurrences calling cacheProtectedMethod to provide or
infer the type parameter, and ensure return type and local variables (cached,
created) are typed to V instead of unknown to restore stronger typing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24473a14-c381-407c-afed-b7cb75a9d7f1
📒 Files selected for processing (2)
packages/core/src/annotations.test.tspackages/core/src/annotations.ts
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/facade.test.ts`:
- Around line 10344-10353: The current assert.rejects only checks for any
TypeError and can catch unrelated failures; update the assertion in the runWith
test using createMutatingAnnotationRunnerParser(marker) to also validate the
error message from the protected-view trap (e.g., assert.rejects(..., err =>
err.name === 'TypeError' && /protected.*mutation|protected
view/i.test(err.message))). Ensure the same tighter message-based assertion is
applied to the duplicate test block around lines 11957-11966 that also tests
protected annotation mutation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c5ca0e0-cb7f-4190-91f1-235884672c89
📒 Files selected for processing (2)
packages/core/src/facade.test.tspackages/core/src/modifiers.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6b0226054
ℹ️ 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".
There was a problem hiding this comment.
Code Review
This pull request implements annotation isolation within @optique/core to prevent parsers from mutating caller-owned or context-owned objects. Public entry points now expose annotations through protected read-only views, utilizing Proxies for containers like Map, Set, Date, and URL. Documentation and tests have been updated to reflect these changes. A potential issue was identified in the array protection logic where using new Array() might lose prototype information or internal slots for certain objects.
Protected RegExp annotations now isolate lastIndex-changing method calls from the caller-owned object, and the annotation test suite now covers that behavior together with frozen-input invariants. Co-Authored-By: OpenAI Codex <codex@openai.com>
Make the runWith() and runWithSync() regression tests assert against the read-only annotation mutation message so they only pass for the intended protected-view failure path. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4e193fad6
ℹ️ 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".
There was a problem hiding this comment.
Code Review
This pull request implements annotation isolation across public parser entrypoints and runner-driven context injection to prevent callers or custom parsers from accidentally mutating shared annotation data. It introduces protected read-only views for supported container types (Map, Set, Date, RegExp, URL, etc.) that throw a TypeError on mutation attempts. The feedback focuses on ensuring the robustness of property iteration in the new protection logic and maintaining the proxy-view fallback for custom class instances to ensure annotations remain accessible to inner parsers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8b0c4ec44
ℹ️ 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".
Protected URL views still bypassed the shared clone-property helper, so constructor lookup and generic method identity remained unstable there even after the other built-in views were fixed. In addition, annotation-view internals still accepted only mutable annotation records in a few spots despite the new read-only annotation contract, and subclass clone helpers were not validating that constructor/species clones actually preserved the source payload. Route URL clone-backed views through the same cached clone-property helper, widen internal annotation-view plumbing to accept read-only annotation records, and validate constructor/species-based clone parity before accepting subclass clones. Add regression coverage for URL method identity stability and for clone helpers that would otherwise silently drop payload. See #787 (comment) See #787 (comment) See #787 (comment) See #787 (comment) Fixes #491 Co-Authored-By: Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements annotation isolation to prevent parsers from mutating caller-owned objects. It introduces protected read-only views for common container types like Map, Set, Date, and RegExp using Proxies and cloning. The implementation ensures that mutation attempts throw a TypeError, maintaining the immutability of input annotations across parse runs. Documentation and tests have been updated to reflect these changes. Feedback is provided regarding the consistency of the valueOf trap in the URL protected view, suggesting that the method should be cached to ensure stable identity.
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The URL protected view still returned a fresh valueOf closure on every property read, so its method identity was inconsistent with the rest of the clone-backed built-ins. In the same area, internal annotation-view plumbing still needed to accept read-only annotation records explicitly. Route URL clone-backed views through the shared cached valueOf path, keep URL constructor/method identity aligned with the other protected built-ins, and widen the remaining internal annotation-view helper to accept readonly annotation records. Add regression coverage for stable URL valueOf identity. See #787 (comment) See #787 (comment) See #787 (comment) Fixes #491 Co-Authored-By: Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cc974b735
ℹ️ 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".
There was a problem hiding this comment.
Code Review
This pull request implements annotation isolation across public parser entrypoints and runner-driven context injection. By exposing annotations through protected read-only views instead of leaking caller-owned objects by reference, the system now prevents accidental mutation of original annotation payloads. The changes include a comprehensive protection system for various container types (Map, Set, Date, RegExp, URL, etc.) and updated documentation and tests to reflect these safety improvements. Feedback was provided regarding a potential performance regression when setting prototypes on arrays and a recommendation to use Reflect.ownKeys() for more robust property iteration.
Clone-backed protected annotation views were still violating proxy invariants for non-configurable, non-writable own function properties by rebinding them through the generic clone-property helper. That made otherwise valid frozen annotation payloads throw at read time. Return the exact stored function for frozen own data properties before applying any binding logic, and add regression coverage for readonly function properties on clone-backed views. See #787 (comment) Fixes #491 Co-Authored-By: Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements annotation isolation and protection against mutation in @optique/core to address issue #491. It introduces ReadonlyAnnotations and ensures that getAnnotations() returns protected views for supported container types, such as Map, Set, and RegExp, which now throw a TypeError upon mutation. The changes also include updated documentation and extensive test coverage for these protections. A review comment identifies a style guide violation in packages/core/src/completion.test.ts, where a system temporary directory is used instead of the required project-local tmp/ directory.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The pwsh availability probe still created its temporary directory outside the protected cleanup path, so temp-directory creation failures could abort test definition instead of cleanly disabling the pwsh-specific integration checks. Create the temporary directory inside the try block and guard cleanup so probe failures degrade to an unavailable pwsh completion environment instead of crashing the suite. See #787 (comment) Co-Authored-By: Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements annotation isolation to prevent the leakage of caller- or context-owned objects by reference, addressing issues #491 and #787. It introduces protected read-only views for annotations, ensuring that mutation attempts against supported container types fail with a TypeError. The PR also updates documentation, integration examples, and adds comprehensive tests for these isolation guarantees. Review feedback suggests removing redundant global checks for URL types and adopting a more robust approach for identifying plain objects across different JavaScript realms.
| if ( | ||
| typeof URLSearchParams === "function" && | ||
| target instanceof URLSearchParams | ||
| ) { | ||
| return createProtectedURLSearchParamsView(target, context) as T; | ||
| } | ||
| if (typeof URL === "function" && target instanceof URL) { | ||
| return createProtectedURLView(target, context) as T; | ||
| } |
There was a problem hiding this comment.
The typeof checks for URLSearchParams and URL are redundant given the project's target runtimes (Node.js, Deno, Bun) where these globals are always available. Removing them would improve consistency with the instanceof checks used for other built-in types like Map and Set in the same function. This ensures that these object-shaped initial states correctly receive annotations as per the project's propagation rules.
| if ( | |
| typeof URLSearchParams === "function" && | |
| target instanceof URLSearchParams | |
| ) { | |
| return createProtectedURLSearchParamsView(target, context) as T; | |
| } | |
| if (typeof URL === "function" && target instanceof URL) { | |
| return createProtectedURLView(target, context) as T; | |
| } | |
| if (target instanceof URLSearchParams) { | |
| return createProtectedURLSearchParamsView(target, context) as T; | |
| } | |
| if (target instanceof URL) { | |
| return createProtectedURLView(target, context) as T; | |
| } |
References
- When propagating annotations, ensure that object-shaped initial states (e.g., boolean flag options) also receive annotations, not just null or undefined states.
| const proto = Object.getPrototypeOf(target); | ||
| if (proto === Object.prototype || proto === null) { | ||
| return createProtectedObjectView(target, context) as T; | ||
| } |
There was a problem hiding this comment.
The check Object.getPrototypeOf(target) === Object.prototype might fail for plain objects created in a different JavaScript realm (e.g., a different Node.js vm context), as Object.prototype is unique to each realm. While likely not an issue in this monorepo, a more robust check for plain objects across realms would be to check if the prototype's constructor name is 'Object' or if the prototype is null.
| ), | ||
| { | ||
| name: "TypeError", | ||
| message: "Cannot mutate read-only annotation data.", |
There was a problem hiding this comment.
The assertion here matches the exact TypeError.message string. Unless the error message is intended to be part of the public contract, this makes the regression test unnecessarily brittle (small wording changes will fail the test while behavior is still correct). Consider asserting only { name: "TypeError" } (or checking the message via a looser predicate) and keeping the message string covered by unit tests in annotations.test.ts instead.
| message: "Cannot mutate read-only annotation data.", |
| ), | ||
| { | ||
| name: "TypeError", | ||
| message: "Cannot mutate read-only annotation data.", |
There was a problem hiding this comment.
Same as above: this test asserts an exact TypeError.message, which is brittle unless the message is part of the API contract. Prefer asserting only the error type/name (or using a looser message check) so the test continues to validate isolation without coupling to wording.
| message: "Cannot mutate read-only annotation data.", |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This PR fixes annotation isolation across Optique’s public parser entrypoints and runner-driven context injection. Before this change, caller-supplied or context-supplied annotation objects could be exposed by reference through
getAnnotations(state), which meant a custom parser could mutate data that it did not own. That behavior was reproducible throughparse(),parseSync(),parseAsync(),suggest(),suggestSync(),suggestAsync(),getDocPage(),getDocPageSync(),getDocPageAsync(),runWith(), andrunWithSync(). This PR closes that gap by moving the fix to the shared annotation boundary instead of patching individual parser families.The core change is in packages/core/src/annotations.ts. Annotations are now exposed through protected read-only views owned by the current parse run.
getAnnotations()returnsReadonlyAnnotations, and supported nested container values such as plain objects, arrays,Map,Set,Date,RegExp,URL, andURLSearchParamsare surfaced through memoized protected views that fail fast on ordinary mutation attempts withTypeError. This preserves the existing “annotations as runtime context” model for live objects and functions, while preventing low-level parsers from mutating caller-owned annotation payloads through parser state.This PR also expands regression coverage around the issue. packages/core/src/parser.test.ts now verifies that custom parsers cannot mutate caller-owned annotations through the public parser and documentation entrypoints. packages/core/src/facade.test.ts adds the equivalent coverage for runner-collected context annotations. packages/core/src/annotations.test.ts adds focused unit coverage for the new protection layer, including stable view reuse and mutation failures for representative container types. Existing tests that previously assumed raw object identity have been updated to assert the new read-only-view contract instead.
The public API and docs were updated to match the new behavior. packages/core/src/index.ts now re-exports
ReadonlyAnnotations, packages/core/src/context.ts re-exports the readonly type for context consumers, docs/concepts/extend.md now documentsgetAnnotations()as returning a protected read-only view, and CHANGES.md includes a changelog entry for this fix. The docs now make the contract explicit: supported mutation attempts fail fast, while opaque live objects remain reference-preserving and keep their own runtime behavior.While validating the change, I also tightened the verification workflow in this workspace. mise.toml now runs the top-level test workflow sequentially instead of depending on parallel subtask expansion, which avoids misleading aggregate failures during local verification. In addition, packages/core/src/completion.test.ts now skips PowerShell completion integration checks when
pwshis present but cannot actually load the generated completion script in the current environment. That keepsmise teststable without weakening the normal completion coverage.A minimal example of the new behavior looks like this:
Fixes #491
Verification
mise testnow passes on this branch.pnpm buildin docs/ also passes.