Skip to content

[eas-cli] Preserve literal simulator control arguments - #4682

Open
gwdp wants to merge 4 commits into
mainfrom
gwdp/simulator-control-literal-values
Open

gwdp wants to merge 4 commits into
mainfrom
gwdp/simulator-control-literal-values

Conversation

@gwdp

@gwdp gwdp commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Why

Shell quotes do not keep text=hello literal: the CLI reads it as a named argument. -- marks positional literals, and repeated values must keep their input order.

Fixes #4676.

How

  • Use oclif's ordered raw argument tokens for action values and the literal boundary.
  • Preserve named arguments before --, including when a later literal repeats an earlier token or flag value. Keep -- consumed as an option value distinct from a separator.
  • Keep the full usage block together and show the literal note below it. Explain the named-versus-literal change in the changeset.

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 named text=hello, literal text=hello, and literal --help at 269c6bdbc through unpublished integration dc6bfa8d6. 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

@gwdp

This comment has been minimized.

@devin-ai-integration

This comment has been minimized.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@codecov

codecov Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.44%. Comparing base (e8ca106) to head (e849ca5).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gwdp

This comment has been minimized.

@devin-ai-integration

This comment has been minimized.

@gwdp

This comment has been minimized.

@devin-ai-integration

This comment has been minimized.

@gwdp

gwdp commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor Author

Gabe confirmed review of the final changes at 269c6bdbc and requested Brent and Krystof for review. Codex implemented the parser changes, regressions, and labeled oclif examples under Gabe’s direction. No code changed during this handoff.

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

@gwdp
gwdp marked this pull request as ready for review October 18, 2026 04:45
@github-actions

Copy link
Copy Markdown

Subscribed to pull request

File Patterns Mentions
packages/eas-cli/** @douglowder

Generated by CodeMention

Warning: The preamble and epilogue options in commentConfiguration are deprecated. Use template instead.

@gwdp

gwdp commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Hosted QA: Codex verified named text=hello, literal text=hello after --, and literal --help through native clipboard readback on a real EAS session. The tested local integration dc6bfa8d6 contains this exact tip and the other CLI fixes; it remains unpublished.

— Codex

@brentvatne brentvatne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=value before the separator becomes a literal when a later literal repeats an earlier token: permissions.set --id s action=grant -- s camera sends action: "s", service: "action=grant"; main sends action: "grant".
  • A literal -- works only as the first literal. permissions.set --id s -- grant -- com.app sends action: "--", because the second -- shares indexOf with 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 :206 prints between the USAGE line and its continuation at :208, so [--id <session-id>] [--json] ends up under the note.
  • name=value after -- 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 char or 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.

@gwdp

gwdp commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks @brentvatne for reproducing the parser cases and checking the oclif source. Codex addressed the actionable points in e849ca5c3:

  • Action values and the literal boundary now use oclif's ordered raw argument tokens. The tests cover a literal repeating a flag value, a later literal --, a literal repeating the action name, and the inherited duplicate-ordering bug without a separator. They also cover an empty literal and a separator before the action.
  • The usage continuation stays directly below the command; the literal note follows the complete usage block. Both --help and -h layout assertions failed before the fix and now pass.
  • The changeset now states that name=value after -- is literal and named arguments belong before it.

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 main, the CLI build and package typecheck, and changed-file type-aware lint/format checks; all pass. Six regression assertions failed before the fix. I also inspected the compiled action-help output. Earlier hosted clipboard QA remains tied to 269c6bdbc; this follow-up was validated locally, with current-head CI now passing on Node 22 and Node 24. Codecov reports all modified coverable lines covered. The latest Devin report predates this follow-up.

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 main without changing those files.

Existing approvals and reviewer requests are preserved.

— Codex

gwdp and others added 4 commits October 22, 2026 22:47
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>
@gwdp
gwdp force-pushed the gwdp/simulator-control-literal-values branch from 96b0212 to e849ca5 Compare October 23, 2026 05:48
@github-actions

Copy link
Copy Markdown

✅ Thank you for adding a changeset!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

simulator:control: values that start with <property>= can't be passed as-is

2 participants