Skip to content

fix(cli): widen rivet add --id to the shapes the loader loads (#1088) - #1116

Open
avrabe wants to merge 3 commits into
mainfrom
fix/issue-1088-widen-id-suffix
Open

avrabe wants to merge 3 commits into
mainfrom
fix/issue-1088-widen-id-suffix

Conversation

@avrabe

@avrabe avrabe commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Closes #1088

What changed

validate_explicit_id (rivet-cli/src/main.rs:19206) previously required
every --id to end in an all-digit suffix. The artifact loader accepts
any uppercase-alphanumeric segment, so a store carrying
SWREQ-FALCON-PREARM-P<nn> (meld, 277 artifacts) or REQ-README-ROSTER
(pulseengine.eu) could load those ids but rivet add --id refused them,
forcing the hand-written YAML workaround the issue is about. The check
now accepts whatever the loader accepts.

The commit-ref-shape warning in validate.rs:678-688 is unchanged, so
projects that rely on commit-trailer coverage still get told their ids
aren't grep-matchable from a trailer — the two checks are now decoupled
as the issue's third suggestion asked for.

Scope — Fix 1 only

The prior triage comment on #1088 sketched three fix paths. This PR
lands Fix 1 (widen validate_explicit_id to match the loader) because
it is the smallest, most contained change that unblocks the reporter's
immediate case. Fix 2 (sibling-aware next-id so rivet next-id --type sw-req returns SWREQ-FALCON-PREARM-P04 instead of SWREQ-001) and
Fix 3 (per-project suppression of commit-ref-shape) remain open
follow-ups on the same issue.

How each ask in #1088 is addressed

  • "Accept a trailing P<nn>-style segment in --id, since the loader
    already does."
    — Done. The suffix no longer has to be all-digit; it
    can be any uppercase-alphanumeric segment, matching the loader's
    acceptance.
  • "Make next-id derive from existing IDs." — Deferred to Fix 2
    (open on rivet add --id rejects, and next-id cannot mint, the ID convention the store already loads (forces hand-written YAML) #1088). This PR is scoped to the write-side --id shape
    check.
  • "Decouple the trailer warning from --id acceptance." — Done.
    validate_explicit_id no longer co-enforces the trailer shape; the
    commit-ref-shape warning at validate.rs:680 is now the only place
    that speaks about trailer discoverability. A project that wants to
    suppress those warnings in bulk is the Fix 3 follow-up.

Test plan

  • cargo test -p rivet-cli --bins validate_explicit_id_tests — the
    new in-module unit tests pass; both the accepted shapes
    (SWREQ-FALCON-PREARM-P04, REQ-README-ROSTER, VER-REVIEW-V0-43-0,
    and historical ones) and the still-rejected shapes (lowercase,
    invalid chars, dash-position errors, no uppercase) are covered.
  • cargo test -p rivet-cli --test cli_commands add_id_flag — all
    four end-to-end rivet add --id tests pass, including the new
    add_id_flag_accepts_loader_shapes.
  • cargo test -p rivet-cli --bins — full 168-test bin suite green.
  • cargo fmt --all -- --check clean.
  • cargo clippy -p rivet-cli --all-targets -- -D warnings clean.
  • rivet validate on this repo: PASS (unchanged, 844 warnings
    baseline).
  • Live repro: rivet add --type requirement --id SWREQ-FALCON-PREARM-P04 --title Example --status proposed on a
    fresh rivet init project succeeds and the file carries the id
    verbatim. Repeated with REQ-README-ROSTER. Negative controls
    (req-001, 1-2) still rejected.

Generated by Claude Code

`validate_explicit_id` required every `--id` to end in an all-digit
suffix, but the artifact loader accepts any uppercase-alphanumeric
segments. Stores with id conventions like `SWREQ-FALCON-PREARM-P<nn>`
(meld, 277 artifacts) or `REQ-README-ROSTER` (pulseengine.eu) load and
resolve those ids, yet `rivet add --id` refused them — so the only way
to add the next sibling was to hand-write YAML, exactly the workaround
`add` is meant to replace. The reporter hit this twice more after
filing, both times falling back to hand-written YAML and once ending
up with two conventions in one repo.

The write-path check now accepts whatever the loader accepts
(uppercase-alphanumeric segments separated by single dashes, with at
least one dash and at least one uppercase letter somewhere). The
`commit-ref-shape` warning emitted by validate still fires on ids whose
last segment isn't all-digit, so a project that relies on commit-
trailer coverage still gets told its ids can't be grep-matched by a
trailer parser — the two checks are now decoupled, as the issue asked.

Lands Fix 1 only. Fix 2 (sibling-aware next-id) and Fix 3 (per-project
suppression of commit-ref-shape) remain open follow-ups to #1088; Fix
1 alone unblocks the reporter's immediate case.

New in-module unit tests cover both the newly-accepted shapes (the two
from the body plus a few historical ones) and the shapes still
rejected (lowercase, invalid chars, dash-position errors, no
uppercase). A new integration test exercises the full `rivet add --id`
path for the newly-accepted shapes. The error-message needle for the
no-dash case moved from "numeric suffix" to "at least one '-'", so the
existing integration-test case updated to match.

Fixes: REQ-007
Refs: REQ-330, #1088, #880

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018W8367wHNy5cYzU8yqjAre
@github-actions

Copy link
Copy Markdown

📐 Rivet artifact delta

No artifact changes in this PR. Code-only changes (renderer, CLI wiring, tests) don't touch the artifact graph.

Playwright E2E was failing on this PR too for the same reason main is
red: `ubuntu-latest` now resolves to Ubuntu 26.04, and the pinned
Playwright release's `install --with-deps chromium` fails before any
test runs with "Playwright does not support chromium on
ubuntu26.04-x64". The failure is base-branch state, not this PR's diff
(which only touches `rivet-cli/src/main.rs` and
`rivet-cli/tests/cli_commands.rs`).

Porting the one-line pin from #1114 so the Playwright gate on THIS PR
sees the test body instead of the install error. No-ops once #1114
merges.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018W8367wHNy5cYzU8yqjAre

avrabe commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Playwright E2E failed on 161d9e9 with the install-step error:

ERROR: Playwright does not support chromium on ubuntu26.04-x64

Not this PR's diff — this touches rivet-cli/src/main.rs and rivet-cli/tests/cli_commands.rs only, no Playwright or workflow exposure. Same failure already hits every PR and main since GitHub's ubuntu-latest moved to Ubuntu 26.04 (#1109). The fix is open at #1114 (one-line pin to ubuntu-24.04).

Ported #1114 verbatim in e65f902: .github/workflows/ci.yml:558 becomes runs-on: ubuntu-24.04 with the explanatory comment above. No-ops once #1114 merges. The next CI run against this head should get the Playwright E2E job to the test body.


Generated by Claude Code

avrabe commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Second CI failure on e65f902: Semver Checks (rivet-core public API) reported function_missing: pub fn rivet_core::test_scanner::scan_source_files_in_project removed or renamed.

Not this PR's diff — this touches only rivet-cli/src/main.rs, rivet-cli/tests/cli_commands.rs, and .github/workflows/ci.yml. The actual cause is that origin/main moved while CI was running: PR #1111 merged as 9e6fb60 and added scan_source_files_in_project to rivet-core/src/test_scanner.rs. The semver check builds its baseline from origin/main at check-run time, so my branch (based on the older cc3b807) looked like it had removed a pub fn it never had.

Fix: merged origin/main into this branch at b45f7ed. Clean auto-merge (no conflicts), fmt / clippy / my new unit tests / the four add_id_flag_* integration tests all green on the merged tree.


Generated by Claude Code

avrabe commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Test failed on b45f7ed with 17 rivet-cli::serve_integration tests reporting "server did not become healthy within 30 seconds" across all three nextest retries (another 25 tests in the same module recovered on retry — the "FLAKY 2/3" / "FLAKY 3/3" lines above the summary).

Not this PR's diff — validate_explicit_id + two test blocks + a workflow-file pin, zero exposure to the serve subsystem. This is the pattern tracked in #1112 (CI runner pool liveness alert): the test_<thing>_present style tests spin up a server on an ephemeral port and poll /healthz for 30s; under runner saturation, port allocation + binary startup + first-request response takes longer and the 30s guard trips.

Base-branch confirmation: every scheduled main CI run since v0.41.0 shipped on 2026-10-15 has concluded failure (runs 2151, 2152, 2153, 2154, 2155, 2160 — six consecutive failures over seven days). The last green main run is 38813250234 on the v0.41.0 release push itself. The push-event run for 9e6fb60 (today's main, after #1111) is still queued after 4h+ — same runner-pool starvation.

Spending the one re-run allotted on this failure to see whether the Test job on this SHA goes green with a less-saturated runner. If it still fails, keeping the PR watched; the root fix lives on #1112, not this PR's diff.


Generated by Claude Code

@codecov

codecov Bot commented Oct 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

This branch has not been deployed

No deployments
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.

rivet add --id rejects, and next-id cannot mint, the ID convention the store already loads (forces hand-written YAML)

2 participants