Skip to content

fix(cli): revert --project/--schemas global (broke main; clap + positional subcommands)#502

Merged
avrabe merged 1 commit into
mainfrom
fix/revert-req-209-global-args
Jun 6, 2026
Merged

fix(cli): revert --project/--schemas global (broke main; clap + positional subcommands)#502
avrabe merged 1 commit into
mainfrom
fix/revert-req-209-global-args

Conversation

@avrabe
Copy link
Copy Markdown
Contributor

@avrabe avrabe commented Jun 6, 2026

Reverts REQ-209/#501, which broke main.

#501 made --project/--schemas global = true (to fix #500), but clap does not allow a global arg to coexist with subcommands that take positionals (next-id <type>, link <src> <tgt>, snapshot diff <baseline>, …). The result: 3 cli_commands integration tests panic in clap's debug_asserts → the Test gate went red on main.

My fault: pre-merge I only exercised validate (no positional), which parsed fine; the full integration suite (which I didn't run) would have caught it. Lesson logged.

This PR

  • Removes global = true from both args (restores the working pre-subcommand behavior).
  • Removes the now-invalid cli_global_args_tests and the REQ-209 artifact.
  • Documents on the arg why it can't be global, and that --project must precede the subcommand.

So #500 is a clap limitation, not fixable via global — reopening it with this finding; the standard workaround (rivet -p X validate) stands.

Verification

cargo test -p rivet-cli --test cli_commands127 passed, 0 failed (the 3 regressions fixed) · cargo fmt --check + clippy --all-targets -- -D warnings exit 0 · rivet validate PASS.

🤖 Generated with Claude Code

… positional subcommands (reverts REQ-209/#501)

REQ-209 / #501 marked --project/--schemas `global = true` to fix #500, but that
broke main: 3 cli_commands integration tests (next_id_json,
next_id_positional_shorthand, all_json_outputs_are_valid) panicked in clap's
debug_asserts — clap does NOT allow a `global` arg to coexist with subcommands
that take positionals (next-id <type>, link <src> <tgt>, snapshot diff
<baseline>, …). My pre-merge check only exercised `validate` (no positional), so
it passed; the full suite would have caught it.

Reverts the `global = true` on both args, removes the now-invalid
cli_global_args_tests and the REQ-209 artifact. #500's premise (args after the
subcommand) is therefore a clap limitation, not fixable this way — the workaround
is the standard convention: put --project before the subcommand
(`rivet -p X validate`). Documented on the arg + reopening #500.

Confirmed with: cargo test -p rivet-cli --test cli_commands (127 passed, 0
failed — the 3 regressions fixed); cargo fmt --check + clippy --all-targets -- -D
warnings (exit 0); rivet validate PASS.

Refs: REQ-007
@avrabe avrabe merged commit e94ac95 into main Jun 6, 2026
19 of 20 checks passed
@avrabe avrabe deleted the fix/revert-req-209-global-args branch June 6, 2026 03:49
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 6, 2026

📐 Rivet artifact delta

Change Count
Added 0
Removed 1
Modified 0
Downstream impacted (depth ≤ 5) 0

Graph

graph LR
  REQ_209["REQ-209"]:::removed
  classDef added fill:#d4edda,stroke:#28a745,color:#155724
  classDef removed fill:#f8d7da,stroke:#dc3545,color:#721c24
  classDef modified fill:#fff3cd,stroke:#ffc107,color:#856404
  classDef overflow fill:#e2e3e5,stroke:#6c757d,color:#495057,stroke-dasharray: 3 3
Loading
Removed
  • REQ-209

📎 Full HTML dashboard attached as workflow artifact rivet-delta-pr-502download from the workflow run.

Posted by rivet-delta workflow. The graph shows only changed artifacts; open the HTML dashboard (above) for full context.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rivet Criterion Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: d688bc3 Previous: e94ac95 Ratio
store_insert/100 85093 ns/iter (± 475) 67654 ns/iter (± 3608) 1.26
store_insert/1000 897174 ns/iter (± 15410) 724588 ns/iter (± 3390) 1.24
store_lookup/100 2138 ns/iter (± 7) 1544 ns/iter (± 11) 1.38
store_lookup/1000 26590 ns/iter (± 91) 18836 ns/iter (± 320) 1.41
store_lookup/10000 382808 ns/iter (± 2730) 263616 ns/iter (± 2700) 1.45
store_by_type/100 95 ns/iter (± 1) 75 ns/iter (± 0) 1.27
store_by_type/1000 93 ns/iter (± 0) 75 ns/iter (± 0) 1.24
store_by_type/10000 93 ns/iter (± 0) 75 ns/iter (± 0) 1.24
schema_load_and_merge 1465299 ns/iter (± 11560) 1116285 ns/iter (± 18770) 1.31
link_graph_build/100 159145 ns/iter (± 1030) 127342 ns/iter (± 429) 1.25
link_graph_build/1000 1875940 ns/iter (± 16279) 1485700 ns/iter (± 14208) 1.26
validate/100 460209 ns/iter (± 2497) 339829 ns/iter (± 1166) 1.35
validate/1000 17338741 ns/iter (± 183973) 14380052 ns/iter (± 167404) 1.21
validate/10000 1414694058 ns/iter (± 25191822) 1146681493 ns/iter (± 7596817) 1.23
traceability_matrix/100 4550 ns/iter (± 61) 3230 ns/iter (± 7) 1.41
traceability_matrix/1000 60336 ns/iter (± 401) 33990 ns/iter (± 530) 1.78
traceability_matrix/10000 741733 ns/iter (± 6612) 578137 ns/iter (± 2534) 1.28
diff/100 60069 ns/iter (± 183) 44967 ns/iter (± 425) 1.34
diff/1000 687331 ns/iter (± 3856) 518272 ns/iter (± 2497) 1.33
diff/10000 7699296 ns/iter (± 189880) 6244657 ns/iter (± 325897) 1.23
query/100 1179 ns/iter (± 4) 882 ns/iter (± 3) 1.34
query/1000 15277 ns/iter (± 143) 11308 ns/iter (± 17) 1.35
query/10000 319553 ns/iter (± 6208) 175603 ns/iter (± 2580) 1.82
document_parse/10 24078 ns/iter (± 274) 17265 ns/iter (± 41) 1.39
document_parse/100 166729 ns/iter (± 7606) 121441 ns/iter (± 356) 1.37
document_parse/1000 1572198 ns/iter (± 26077) 1148895 ns/iter (± 10832) 1.37

This comment was automatically generated by workflow using github-action-benchmark.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

avrabe added a commit that referenced this pull request Jun 6, 2026
) (#503)

The revert (#502) left clap-internals jargon ("debug-asserts ... next-id/link/
snapshot diff") in the user-facing `--help` for `--project`. Move that rationale
to a plain `//` maintainer comment and make the `///` help user-facing: state
the actionable convention — pass `--project`/`--schemas` BEFORE the subcommand
(`rivet -p <dir> validate`), like `git -C <dir> <cmd>`. Addresses the UX
confusion in #500 (a misplaced `--project` erroring with empty stdout) by
surfacing the convention in `--help`.

Doc-comment only; no logic change. Confirmed with: cargo test -p rivet-cli
--test cli_commands (127 passed, 0 failed — the full suite, incl. help tests);
cargo fmt --check + clippy --all-targets -- -D warnings (exit 0).

Refs: REQ-007
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.

Top-level args (--project / --schemas) aren't globalrivet validate --project X errors instead of working in any position

1 participant