Skip to content

fix(cli): reject excess positionals - #1433

Merged
thymikee merged 2 commits into
callstack:mainfrom
Bortlesboat:fix/1406-positional-arity
Jul 27, 2026
Merged

fix(cli): reject excess positionals#1433
thymikee merged 2 commits into
callstack:mainfrom
Bortlesboat:fix/1406-positional-arity

Conversation

@Bortlesboat

Copy link
Copy Markdown
Contributor

Summary

Bounded CLI commands now fail closed when a plan supplies extra positionals. For example, open app url close previously swallowed close; it now returns INVALID_ARGS before command execution.

  • derive one positional limit from CommandSchema and enforce it in normal parsing, deprecated legacy batch steps, and help-conformance validation
  • keep explicitly variadic commands unchanged, while declaring the existing optional connect provider and swipe duration inputs accurately
  • add schema-exhaustive and legacy-batch regressions for the motivating failure

This production-contract slice changes 8 files and stays within the CLI schema/parser, legacy batch, and deterministic validator family. It does not modify guidance or SkillGym while the active harness split moves command-planning coverage out of the oversized suite; that coverage remains follow-up work.

Validation

The focused parser, interaction, session, batch, and help-conformance suites pass (133 tests). Formatting, typechecking, linting, layering, Fallow, build, and generated MCP metadata checks are green locally.

The native-Windows full unit sweep reached 4,249 passing and 8 skipped tests, with 257 existing environment-sensitive failures involving platform paths, unavailable host tools, symlinks, and fixture checksums. All changed-path suites pass in isolation; CI remains the cross-platform authority. This is parser-only behavior, so no device session was opened.

Related: #1406, #1411

@thymikee

Copy link
Copy Markdown
Member

Reviewed exact head 08fa2464. P1 — get is incorrectly bounded to two positionals. The new shared arity assertion rejects valid existing syntax such as agent-device get text @e5~s3 Title before the production reader runs, even though readElementTargetFromPositionals(positionals.slice(1)) deliberately consumes a ref plus its optional trailing label, and replay serialization emits that form. Legacy batch get steps with labels are likewise rejected. Mark get variadic for its label grammar (or model that grammar correctly) and add a normal-parser regression with a multiword ref label.

The exhaustive test only proves declared schemas are enforced; it does not prove each schema admits the existing command grammar, so it misses this regression. Other reviewed routes and all exact-head checks are clean; keep ready-for-human withheld.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 27, 2026
@thymikee

Copy link
Copy Markdown
Member

Re-reviewed exact head e6a288662e732219fc939711f267376c02eaf7b7. Clean and ready.

The follow-up fixes the prior get @ref label regression without weakening the bounded-command contract: get is explicitly variadic for its established ref-label grammar, while the shared parser guard still derives bounded arity from CommandSchema. The exhaustive listCliCommandNames() regression is non-vacuous, and the legacy batch test proves both rejection before daemon projection and preservation of a multiword ref label. The help-conformance validator now consumes the production parser guard rather than retaining a parallel arity map.

Trace confirms normal CLI parsing reaches the guard before daemon execution; legacy batch applies it before projection. This aligns with #1406/#1412 and ADR 0008’s command-family CLI-schema ownership. Exact-head focused suites passed 57/57. GitHub reports the PR clean/mergeable with 18 checks passing; Bundle Size is intentionally skipped. No labels changed.

@thymikee
thymikee merged commit 4c02b6a into callstack:main Jul 27, 2026
19 checks passed
@Bortlesboat
Bortlesboat deleted the fix/1406-positional-arity branch July 27, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants