refactor(cli): use Node argument parsing - #231
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @kyle-sexton's task in 2m 40s —— View job PR Review
|
…true form (#294) ## Summary `components/claude-lanes/claude-review.yml` justified its job-level queue block with a key-level claim: > `queue:` cannot share a concurrency block with cancel-in-progress GitHub's prohibition is **value-level**, not key-level. Verbatim, from the reusable that renders into both `#concurrency` and `#jobsjob_idconcurrency`: > The combination of `queue: max` and `cancel-in-progress: true` is not allowed > and will result in a workflow validation error. Same file, restated in the example prose: > Note that `queue: max` cannot be combined with `cancel-in-progress: true`, > because the two options describe conflicting behaviors for handling > in-progress runs. - Source pinned at the commit read: [`github/docs@336b7f5` `data/reusables/actions/actions-group-concurrency.md`](https://github.com/github/docs/blob/336b7f546d9443dab4e1fa4f0f470e45448c7abc/data/reusables/actions/actions-group-concurrency.md) (lines 20 and 126). - Rendered page carrying the same sentence, confirmed by fetch: <https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#concurrency> `queue: max` + `cancel-in-progress: false` is legal and runs — the key-level wording forbids a shape GitHub permits. The correction narrows the claim to the documented pair and adds the second, independent reason the two blocks stay separate: this group is job-scoped and repo-wide, not per-PR. Without that clause a reader who sets `cancel-in-progress: false` could conclude the blocks may now be merged, which would silently collapse the per-PR supersede group into the repo-wide queue. The authoritative URL is cited **inline, in the tracked bytes** (`565559a`, added after review). Because this component is materialized verbatim downstream, a citation living only in a commit message never reaches the maintainers who read the claim — a correct-but-uncited assertion would repeat this PR's own failure mode at lower severity. The link sits at the sentence that defers to it per `conventions/engineering/documentation-and-citations.md`, and it is a living URL rather than a pinned snapshot because the same convention prefers fetching at read time over storing a snapshot with no recheck trigger. The job-level anchor (`#jobsjob_idconcurrency`) is cited rather than the workflow-level one because the comment annotates a `jobs.<id>.concurrency` block; both anchors were confirmed live to exist on the rendered page and to carry the rule sentence. ### Why this races the open sync PRs This component is sync-managed: its bytes are copied verbatim into every consumer's `.github/workflows/claude-review.yml`. The wording has not landed anywhere yet, and four open sync PRs are carrying it right now. Regenerated from commands, not recalled: ```console $ grep -n '^targets:' distribution/sync-manifest.yml 216:targets: $ awk 'NR>=216' distribution/sync-manifest.yml | grep -c '^ - claude-review-caller$' 5 ``` Managed targets: `claude-code-plugins`, `dotfiles`, `github-iac`, `medley`, `provisioning`. Each consumer's live default-branch file was fetched and counted — the file was read, not grepped on a ref for text expected to be there: | Repo | `queue: max` on default branch | old wording on default branch | open sync PR head carries old wording | | --- | --- | --- | --- | | `claude-code-plugins` | 0 | 0 | no open sync PR | | `dotfiles` | 0 | 0 | #361 (`df035f9`) — yes | | `github-iac` | 0 | 0 | #244 (`0b8124d`) — yes | | `medley` | 0 | 0 | #1676 (`d107862`) — yes | | `provisioning` | 0 | 0 | #231 (`7b9d016`) — yes | Landing this before those PRs merge means the correct text reaches every consumer on first contact. Landing it after means a false statement propagates fleet-wide and needs a second sync to retract. **Hold on the four sync PRs until this merges.** The rollout-window gate is intact — `grep -c '^ automerge: false$' distribution/sync-manifest.yml` returns `8` against `8` total targets — and each of the four PRs was queried live (`gh pr view --json autoMergeRequest`): **none is armed**. So nothing merges them without a human, and a human merging any of them before #294 lands is the only thing that defeats this PR. Merging #294 does **not** require closing them. Verified in the engine at the SHA this repository pins (`ci-workflows@ac223bb`, `.github/workflows/standards-sync.yml:454-461`): it uses `peter-evans/create-pull-request` against a fixed `branch: chore/standards-sync`, which is the head branch on all four PRs — so a subsequent real run refreshes each existing PR in place rather than opening a new one, which is also why the engine guards auto-merge arming on `pull-request-operation == 'created'`. `sync.yml` runs on `push: branches: [main]`, so merging this PR is itself the refresh trigger. ### Sibling component: checked, no change `components/claude-lanes/claude-security-review.yml` was read in full, not assumed to match. It makes no key-level claim. Its one queue-adjacent statement — "a full queue CANCELS new arrivals" — is accurate: > `max`: Up to 100 jobs or workflow runs can be `pending` in the concurrency > group. When the queue is full, any additional jobs or workflow runs are > canceled. A repo-wide grep confirms the defect had exactly one site: ```console $ grep -rn 'share a concurrency block\|cannot share' . | grep -v '^\./\.git/' ./components/claude-lanes/claude-review.yml:87: ... ``` This repository's own `.github/workflows/claude-review.yml` sets no caller-level concurrency and no `queue:`, so it never carried the claim. ### Deliberate non-change Independent verification surfaced a separate omission, not a falsehood: the comment does not mention that `queue: max` caps at 100 pending and cancels arrivals beyond that. Left out on purpose — this PR narrows a false claim and should stay a one-hunk diff while it races the sync PRs. The overflow-wedge argument that makes the cap load-bearing in the security lane does not transfer here, and that was checked rather than assumed: every ruleset on all five managed targets was enumerated and its `required_status_checks` contexts read, and **no target requires any `claude`-named context today**. ```console $ # per target: enumerate rulesets, union their required contexts, count claude ones claude-code-plugins: total_required=4 claude_required=0 dotfiles: total_required=3 claude_required=0 github-iac: total_required=3 claude_required=0 medley: total_required=3 claude_required=0 provisioning: total_required=3 claude_required=0 ``` So overflow cancellation on this lane cannot void a required check — the same premise that already makes this lane's deliberate `cancel-in-progress: true` safe. If a consumer later promotes the code-review context to required, the cap becomes load-bearing and the comment should gain it. Worth a follow-up on its own merits, not a blocker for this one. ## Test plan - `components/claude-lanes/claude-lanes.test.sh`, counted from the run rather than eyeballed, and reproduced by two independent runs: ```console $ bash components/claude-lanes/claude-lanes.test.sh > run.txt 2>&1; echo "exit=$?" exit=0 $ grep -cE '^PASS' run.txt; grep -cE '^FAIL' run.txt 30 0 ``` That includes `[29] a synced lane caller fails actionlint without the suppression` and `[30] control run reports the suppressed message`. Note for anyone re-running: the harness materializes from the git **index**, so the change must be staged or every target reports `source worktree bytes differ from the indexed object` and the suite fails for that reason alone. - Comment-only change: no YAML key, value, group expression, or pin is touched. Confirmed by `git diff --stat origin/main...HEAD` — 6 insertions, 3 deletions, every line inside a `#` comment block. - Independent verification by a fresh-context agent with the rationale withheld (given the final file text and asked whether every claim is true, and whether the constraint is stated at the correct level of generality). Verdict on the corrected wording: **ACCURATE** — "The comment restates the constraint at precisely the docs' generality — the docs name the literal `queue: max` + `cancel-in-progress: true` pair and nothing wider. Neither broader nor narrower. I would not reword this clause." It also independently confirmed `queue:` is valid at job level, via `data/reusables/actions/jobs/section-using-concurrency-jobs.md` including the same reusable. - Repository CI on this PR: every check in the `pass` bucket, zero non-pass, verified by `gh pr checks 294 --json name,bucket` on each pushed head (`a2a8228` and `565559a`). That includes the Claude review lane (`review / review`) — run once per head, never re-run to chase green. ## Related - #286 — the PR that introduced these caller components and the wording corrected here. - Open sync PRs carrying the pre-correction text: melodic-software/dotfiles#361, melodic-software/provisioning#231, melodic-software/github-iac#244, melodic-software/medley#1676. - Two sibling sites for the same claim live in `ci-workflows` and are tracked there (ci-workflows#296 for the workflow; ci-workflows#302, merged, for `PLAN.md`). Deliberately untouched by this PR. No linked issue. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
node:util.parseArgsconfigurationsBehavior
The standard parser intentionally adds normal Node CLI syntax and strictness:
--is an option terminator; policy CLIs accept it with no trailing positional, while package paths after it remain positional--name=value--root=-repo); the separated form is rejected as ambiguous--Repeated options remain last-wins. The policy CLIs still wrap argument failures as
ConfigurationErrorand exit 2 with their established prefixes; package-lifecycle still exits 1 and preservesPACKAGE_BASE_REFfallback.Research
The implementation and tests follow the stable Node.js
util.parseArgscontract, including strict unknown-option/type validation, string and boolean option definitions, defaults, positionals, inline values, and the option terminator.Verification
Passed on Node 24.18.0:
distribution/sync-manifest.sh validatewas attempted against the staged change but exceeded the 60-second local Windows cap without producing a finding; CI remains the authoritative run for that gate. The managed concurrency, Dependabot, and runner source files will propagate through the existing manifest; their tests and the package harness remain repository-local.Related
Closes #218