Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4682 +/- ##
==========================================
+ Coverage 75.43% 75.44% +0.01%
==========================================
Files 1055 1055
Lines 52404 52420 +16
Branches 11437 11443 +6
==========================================
+ Hits 39526 39542 +16
Misses 11891 11891
Partials 987 987 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Gabe confirmed review of the final changes at Ready for review with Brent and Krystof requested. CI passed on the reviewed head, including Node 22/24; Devin completed analysis on the same head, with no open review threads. All modified coverable lines are tested, and stale review notices are hidden. — Codex |
|
Subscribed to pull request
Generated by CodeMention Warning: The preamble and epilogue options in commentConfiguration are deprecated. Use template instead. |
|
Hosted QA: Codex verified named — Codex |
brentvatne
left a comment
There was a problem hiding this comment.
Reviewed at 269c6bd with Codex, Grok and Claude reviewers; I checked each finding below against the code and ran the parser cases with the installed @oclif/core 4.8.3.
1. The literal boundary is applied to oclif's reordered argv (low-medium)
control.ts:94-99 counts raw tokens after the real -- and applies that count to oclif's parsed argv:
literalFrom = argv.length - 1 - (this.argv.length - index - 1);oclif sorts parsed argv by first occurrence in the raw input (lib/parser/parse.js:191):
argv: unsortedArgv.sort((a, b) => originalArgv.indexOf(a) - originalArgv.indexOf(b)),so a token that also appears earlier, as a positional or a flag value, moves forward, and the boundary lands in the wrong place. Most resulting misplacements already happen on main, which uses the same sorted argv. Two consequences are specific to this PR:
- A
name=valuebefore the separator becomes a literal when a later literal repeats an earlier token:permissions.set --id s action=grant -- s camerasendsaction: "s",service: "action=grant";mainsendsaction: "grant". - A literal
--works only as the first literal.permissions.set --id s -- grant -- com.appsendsaction: "--", because the second--sharesindexOfwith the separator.
Fix: take positional values from the parse result's raw tokens (type === 'arg'), which keep input order, and compute the boundary against that list. Tests where a literal repeats an earlier token or flag value would cover it.
2. Smaller items
- The help note at
:206prints between the USAGE line and its continuation at:208, so[--id <session-id>] [--json]ends up under the note. name=valueafter--was previously still parsed as named and is now literal. That is the intended fix for #4676; a changeset sentence would tell anyone relying on the old reading.- The separator scan recognizes only long option flags. No option flag has a
charor alias today, so this is latent.
Checked and fine: invocations without -- are unchanged; --id --, --id=-- and --output -- are treated as option values, not separators; -- --help reaches the command rather than oclif's help.
|
Thanks @brentvatne for reproducing the parser cases and checking the oclif source. Codex addressed the actionable points in
The short-option/alias observation is latent: neither option flag currently declares one. This follow-up adds no flag aliases; supporting future option aliases remains outside it. Codex ran 521 simulator/help tests across 22 suites after rebasing onto current The rebase preserved all four PR patches. The previous changeset check compared the old base SHA against a merge containing newer release changelogs; the rebased branch includes current Existing approvals and reviewer requests are preserved. — Codex |
Read action values from ordered raw oclif tokens, keep the complete usage block together, and explain named arguments before the literal separator. Add repeated-token and help-layout regressions. Co-authored-by: Codex <codex@openai.com>
96b0212 to
e849ca5
Compare
|
✅ Thank you for adding a changeset! |
Why
Shell quotes do not keep
text=helloliteral: the CLI reads it as a named argument.--marks positional literals, and repeated values must keep their input order.Fixes #4676.
How
--, including when a later literal repeats an earlier token or flag value. Keep--consumed as an option value distinct from a separator.Test Plan
Codex ran 521 simulator/help tests across 22 suites after rebasing onto current
main, the CLI build, package typecheck, and changed-file type-aware lint/format checks. All pass. Six regression assertions failed before the follow-up fix. Codex also checked the compiled action-help output and generic command help.CI passes at
e849ca5c3, including Node 22 and Node 24 tests. Codecov reports all modified coverable lines covered. The latest Devin report predates this follow-up. Earlier hosted clipboard readback verified namedtext=hello, literaltext=hello, and literal--helpat269c6bdbcthrough unpublished integrationdc6bfa8d6. Hosted QA was not repeated for this parser follow-up.Authorship and LLM use
Gabe directed the fixes, requested separate PRs, and reviewed the earlier diff at
269c6bdbc. Brent reviewed that revision and found the ordering and help-layout bugs. Codex implemented and locally reviewed the follow-up, tests, changeset and description. Human review of the follow-up is pending.— Codex