Keep annotated state transparent in argument() and command()#781
Keep annotated state transparent in argument() and command()#781
argument() and command()#781Conversation
Treat annotation-injected initial state as runtime context instead of parser-local progress for argument(), command(), and top-level option value suggestions. This preserves normal parse and suggest behavior in direct parser calls and context-aware runners. Add regression tests for direct parse/suggest flows, static-context runWith()/runWithSync(), and run()/runSync(), and record the user-visible behavior change in CHANGES.md. Fixes #187 Co-Authored-By: OpenAI Codex <codex@openai.com>
Extract annotation-view and state-propagation helpers into a shared internal module so constructs and primitives stay on the same annotation-handling contract. This removes duplicated proxy and wrapper logic that could drift on future fixes. Add wrapper regressions for map(argument()) and group(command()), and restore the changelog reference link so hongdown checks stay clean. Refs #187 Co-Authored-By: OpenAI Codex <codex@openai.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #781 +/- ##
==========================================
+ Coverage 87.63% 87.66% +0.02%
==========================================
Files 44 45 +1
Lines 28416 28523 +107
Branches 6945 6942 -3
==========================================
+ Hits 24902 25004 +102
- Misses 3425 3432 +7
+ Partials 89 87 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 fixes an issue where argument() and command() parsers incorrectly interpreted annotation-injected initial state as real parser-local state. By introducing annotation-state.ts to handle annotation propagation and normalization, the changes ensure that annotations are treated as transparent runtime context. The review feedback identified that using an empty object {} as a target for annotation injection on nullish states breaks transparency for nested parsers; the suggested fix ensures that annotations are injected into the original nullish state to maintain correct normalization back to the sentinel value.
There was a problem hiding this comment.
Pull request overview
Fixes a regression where top-level annotation injection changed primitive parser sentinel state (undefined) into an internal wrapper object, causing argument() and command() to misinterpret progress/state and break parsing and suggestion behavior.
Changes:
- Introduces shared annotation-state helpers to normalize injected wrapper state and consistently propagate annotations to child parser states.
- Updates
argument(),command(), and option suggestion logic to treat annotation-injected initial state as transparent while preserving annotations when rebuilding tuple/array state. - Adds regression tests covering
parse()/suggest()and runner entrypoints (runWith*,run*), plus updates the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/annotation-state.ts | New shared helpers for unwrapping injected annotation wrappers and for child-state propagation (inject vs proxy view). |
| packages/core/src/primitives.ts | Normalizes annotation-injected initial state for argument()/command(), preserves annotations when rebuilding command state, and adjusts option suggestion initial-state checks. |
| packages/core/src/constructs.ts | Switches annotation propagation / annotation-view proxy utilities to the new shared helper module to prevent logic drift. |
| packages/core/src/primitives.test.ts | Adds regression tests ensuring annotations remain transparent for argument()/command() parse and suggest (including wrapper compositions). |
| packages/core/src/facade.test.ts | Adds runner-level regression tests for static contexts with annotations via runWithSync(). |
| packages/run/src/run.test.ts | Adds runner-level regression tests for runSync() and async run() with annotated static contexts. |
| CHANGES.md | Documents the behavioral fix and links the issue/PR. |
Comments suppressed due to low confidence (2)
packages/core/src/primitives.ts:614
- The comment for Scenario 2 still says “state is undefined”, but the condition now also accepts the annotation-wrapped initial state via
isAnnotationWrappedInitialState(context.state). Update the comment to reflect the new behavior (e.g., “state is initial/annotation-wrapped initial”).
} // Scenario 2: Empty buffer, state is undefined, and the prefix is
// not itself starting an option token.
else if (
isAnnotationWrappedInitialState(context.state) &&
context.buffer.length === 0 &&
(context.exec?.path?.length ?? 0) === 0 &&
!(prefix.startsWith("--") || prefix.startsWith("-") ||
prefix.startsWith("/"))
packages/core/src/primitives.ts:794
- The Scenario 2 comment mentions “state is undefined”, but the code now checks
isAnnotationWrappedInitialState(context.state)(i.e., undefined or an annotation-wrapped undefined sentinel). Consider updating the comment to match the actual condition.
} // Scenario 2: Empty buffer, state is undefined, and the prefix is
// not itself starting an option token.
else if (
isAnnotationWrappedInitialState(context.state) &&
context.buffer.length === 0 &&
(context.exec?.path?.length ?? 0) === 0 &&
!(prefix.startsWith("--") || prefix.startsWith("-") ||
prefix.startsWith("/"))
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds packages/core/src/annotation-state.ts to centralize annotation-view proxying and parent→child annotation propagation, refactors packages/core/src/constructs.ts to use the new helpers, and updates packages/core/src/primitives.ts to normalize and wrap annotation-injected initial state when parsing and suggesting (including command child-state handling). It also adds tests in core and run packages validating parse/suggest/run behavior with annotated/static contexts. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/constructs.ts (1)
609-624: 🧹 Nitpick | 🔵 TrivialRoute this helper through the shared annotation-state contract.
getObjectParseChildState()still bakes ingetAnnotations()/inheritAnnotations()locally, soobject()can drift fromgetWrappedChildParseState()/getWrappedChildState()even after the extraction. Please move this wrapper-specific branch behind an exported helper inpackages/core/src/annotation-state.tsinstead of keeping a parallel path here.Based on learnings,
deriveOptionalInnerParseStateregressed when a separate annotation-handling path dropped outer-array annotations on re-entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/constructs.ts` around lines 609 - 624, getObjectParseChildState currently directly uses getAnnotations and inheritAnnotations, causing divergence from the shared contract; extract the wrapper-specific branch into a new exported helper in packages/core/src/annotation-state.ts (e.g., export function reconcileObjectChildState(parentState, childState) or similar), implement the same logic there (checking annotations, null/primitive child, and returning inheritAnnotations when needed), then replace the local calls in getObjectParseChildState with a call to that exported helper so object() flows through the same shared annotation-state contract used by getWrappedChildParseState/getWrappedChildState and avoids regressions like in deriveOptionalInnerParseState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/constructs.ts`:
- Around line 609-624: getObjectParseChildState currently directly uses
getAnnotations and inheritAnnotations, causing divergence from the shared
contract; extract the wrapper-specific branch into a new exported helper in
packages/core/src/annotation-state.ts (e.g., export function
reconcileObjectChildState(parentState, childState) or similar), implement the
same logic there (checking annotations, null/primitive child, and returning
inheritAnnotations when needed), then replace the local calls in
getObjectParseChildState with a call to that exported helper so object() flows
through the same shared annotation-state contract used by
getWrappedChildParseState/getWrappedChildState and avoids regressions like in
deriveOptionalInnerParseState.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82885dad-496a-4fd4-b162-a37d119f5186
📒 Files selected for processing (7)
CHANGES.mdpackages/core/src/annotation-state.tspackages/core/src/constructs.tspackages/core/src/facade.test.tspackages/core/src/primitives.test.tspackages/core/src/primitives.tspackages/run/src/run.test.ts
Move object-owned child-state reconciliation into the shared internal annotation-state module and make constructs.ts call through that helper. This keeps object() on the same annotation propagation contract as the other shared helpers and reduces the risk of future drift. Refs #187 Co-Authored-By: OpenAI Codex <codex@openai.com>
Wrap nullish child states with their original sentinel values so
annotation normalization can recover `undefined` and `null` instead of
an injected `{}` wrapper.
Add direct regression tests for getWrappedChildParseState() and
getWrappedChildState() to lock that behavior down.
Addresses review comments:
- #781 (comment)
- #781 (comment)
Refs #187
Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where argument() and command() primitives misinterpreted annotation-injected initial state as actual parser state. It introduces centralized annotation state utilities in packages/core/src/annotation-state.ts and updates the primitives to normalize and correctly wrap/unwrap states when annotations are present. Extensive tests were added across the core and run packages to verify that positional arguments, subcommands, and value suggestions remain transparent to runtime annotations. I have no feedback to provide.
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ 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". |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/annotation-state.test.ts`:
- Around line 95-103: Replace the strict equality assertion used to test a
truthy annotation with an existence/truthiness assertion: change the
assert.equal(getAnnotations(wrapped)?.[marker], true) check to use assert.ok so
the test asserts the marker value is truthy; update the assertion near the other
checks involving normalizeInjectedAnnotationState and getAnnotations in the same
test where the local variables wrapped and marker are used.
- Around line 68-76: Replace the boolean equality assertion with an assertion
that directly checks truthiness: in the test where
normalizeInjectedAnnotationState and getAnnotations are used, swap the
assert.equal(getAnnotations(wrapped)?.[marker], true) call to use
assert.ok(getAnnotations(wrapped)?.[marker]) (optionally with the same message)
so the test follows the boolean-check guideline; update the assertion in the
block that references wrapped, marker, normalizeInjectedAnnotationState, and
getAnnotations.
🪄 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: f8ef4af3-76fd-46be-a0c7-b79a4606bb79
📒 Files selected for processing (2)
packages/core/src/annotation-state.test.tspackages/core/src/annotation-state.ts
Clarify that isAnnotationWrappedInitialState() checks the initial sentinel after normalization, so plain `undefined` and wrapped `undefined` are treated the same. Addresses review comment: - #781 (comment) Refs #187 Co-Authored-By: OpenAI Codex <codex@openai.com>
Replace boolean equality checks in the new annotation-state regressions with assert.ok() so the tests follow the repository assertion style. Addresses review comments: - #781 (comment) - #781 (comment) Refs #187 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/annotation-state.ts`:
- Around line 15-170: Add explicit JSDoc `@param` and `@returns` tags (and `@throws`
if applicable) to every exported helper in this file: annotationViewTargets,
unwrapAnnotationView, withAnnotationView, normalizeInjectedAnnotationState,
isAnnotationWrappedInitialState, getWrappedChildParseState,
getWrappedChildState, and reconcileObjectChildState; for each function describe
each parameter (e.g., state, annotations, parentState, childState, parser)
including types/nullable semantics and what the function returns (including when
it may return the original input vs. a wrapped/injected view), and note any
exceptional cases (e.g., when injections are not performed) using `@throws` where
a function can propagate errors. Ensure descriptions match the existing
high-level comment text and mention when the return is the original object vs. a
proxy/wrapped state.
🪄 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: 43402b13-acdb-421b-ba08-5b62f6e73a55
📒 Files selected for processing (2)
packages/core/src/annotation-state.test.tspackages/core/src/annotation-state.ts
Add explicit parameter and return documentation to the exported annotation-state helpers and clarify the proxy-target map comment. Addresses review comment: - #781 (comment) Refs #187 Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where argument() and command() primitives misinterpreted annotation-injected initial state as real parser-local state. It introduces a new internal annotation-state.ts module to handle state normalization and propagation, ensuring that annotated calls through various entry points like parse(), suggest(), and run() treat annotations as transparent runtime context. The changes include comprehensive regression tests across facade.test.ts, primitives.test.ts, and run.test.ts. I have no feedback to provide as the implementation correctly addresses the reported issue while maintaining type safety and following the repository's architectural patterns.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/core/src/primitives.ts:615
- The comment for Scenario 2 still says “state is undefined”, but the condition now uses
isAnnotationWrappedInitialState(context.state)(treating plainundefinedand annotation-wrappedundefinedthe same). Updating the comment here would avoid future confusion about why wrapped initial state is included.
} // Scenario 2: Empty buffer, state is undefined, and the prefix is
// not itself starting an option token.
else if (
isAnnotationWrappedInitialState(context.state) &&
context.buffer.length === 0 &&
(context.exec?.path?.length ?? 0) === 0 &&
!(prefix.startsWith("--") || prefix.startsWith("-") ||
prefix.startsWith("/"))
) {
packages/core/src/primitives.ts:795
- The Scenario 2 comment says “state is undefined”, but the code now checks
isAnnotationWrappedInitialState(context.state), which includes annotation-wrapped initialundefinedtoo. Please adjust the comment to match the new behavior so readers don’t assume only plainundefinedis considered.
} // Scenario 2: Empty buffer, state is undefined, and the prefix is
// not itself starting an option token.
else if (
isAnnotationWrappedInitialState(context.state) &&
context.buffer.length === 0 &&
(context.exec?.path?.length ?? 0) === 0 &&
!(prefix.startsWith("--") || prefix.startsWith("-") ||
prefix.startsWith("/"))
) {
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ 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". |
Summary
This fixes a regression where
argument()andcommand()treated annotation-injected initial state as if it were real parser-local progress. The root cause was that both primitives useundefinedas the "no state yet" sentinel, but top-level annotation injection turns that sentinel into an internal wrapper object. Once that happened,argument()thought it had already consumed a value,command()fell out of its valid tuple state machine, and top-level suggestion flows changed as well.The fix makes annotations transparent to these primitives again. packages/core/src/primitives.ts now normalizes injected annotation wrappers before inspecting parser-local state, preserves annotations when
command()rebuilds its tuple state, and forwards child state through a single shared annotation-state contract. I also extracted the shared helper logic into packages/core/src/annotation-state.ts and wired packages/core/src/constructs.ts into the same helper set so the propagation rules do not drift in future changes.This PR adds regression coverage for direct
parse()andsuggest()calls, static-context runner paths throughrunWith(),runWithSync(),run(), andrunSync(), and wrapper compositions that forward primitive state directly. The new tests live in packages/core/src/primitives.test.ts, packages/core/src/facade.test.ts, and packages/run/src/run.test.ts. Because this changes user-visible behavior for annotation users after 0.10.0, CHANGES.md is updated as well.Before this change, these calls could fail with errors like
The argument STRING cannot be used multiple times.orInvalid command state., and the suggestion call could return an empty array. After this change, they behave the same way they do without annotations.Validation
I validated the change with
mise test.Fixes #187