feat(cli): construct skill ship opens a pull request instead of pushing to main - #26
Merged
Conversation
…ng to main #25 made branch + PR the rule for this repo but left `construct skill ship` hard-coding `git push origin main`, so the tool we ship could still bypass the policy the docs state. This closes that gap. `ship` now: enforces bundle-drift and the §5.6 description cap (unchanged), resolves a feature branch, switches to it, stages by explicit name, makes the signed UTC commit, pushes the branch with `-u`, and opens the PR with `gh`. It never pushes to the default branch, and it never merges — that stays the maintainer's call. Branch resolution, in order: - `--branch/-b` when given; - the current branch when it is not the default branch, so a re-run adds a commit to the branch an open PR already tracks rather than fragmenting work; - otherwise a generated `ship/<slug>` derived from the shipped skills. The name is deterministic for the same skill set, which makes the re-run case land on the same branch. The default branch is read from `origin/HEAD`, falling back to `main` when it is unset (fresh clones and fixture repos have no remote refs). `gh` is now a dependency: it is probed *before* the work tree is touched, so a missing `gh` fails with DEPENDENCY_MISSING (127) rather than stranding a committed branch the caller has to finish by hand. If a PR for the branch is already open, `gh pr create` exits non-zero — that is not a ship failure, since the commit is already pushed onto the branch the PR tracks, so the existing URL is looked up and returned. Auto-sync is removed. `ship` no longer lands anything on the default branch, so `nix flake update construct` would have pinned an unrelated revision; run `construct skill sync` after the PR merges. `--no-sync` is kept as a hidden no-op so existing invocations and scripts keep working. Output is now `status: "pull_request_opened"` with `branch`, `base_branch`, and `pull_request_url`; `flake_updated`/`synced_at` are gone. Dry-run reports `default_branch`, `current_branch`, `branch`, `would_create_branch`, and `would_open_pull_request`. manifest.rs updated in step (the `manifest_in_sync_with_cli` test is the guard). Tests: three new cases — shipping from the default branch must plan a `ship/` branch and a PR and never target the default branch; `--branch` wins; an existing feature branch is reused. `--no-sync` dropped from the existing cases to prove it is no longer needed. 9 ship tests, 40 total, all passing. Drive-by, required to get a clean gate: src/sources/skillmd.rs had its `split` helper declared after `mod tests`, which trips clippy's items_after_test_module under `-D warnings`. Pre-existing on main and invisible because CI runs only reuse/config/description checks, not cargo. Moved the test module to the end of the file; no logic change. cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test all clean. AGENTS.md and CONTRIBUTING.md updated to describe ship as the automation of the workflow rather than a tool to avoid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
UnbreakableMJ
added a commit
that referenced
this pull request
Aug 5, 2026
#27) * ci: gate construct-cli with cargo fmt, clippy, test, and an MSRV check construct-cli/ is the only real build surface in this repo — everything else is markdown — and it was entirely ungated. The lint job never invokes cargo, which is how a clippy break (items_after_test_module in src/sources/skillmd.rs) sat unnoticed on main until #26 tripped over it. Two new jobs: - `cargo` — fmt --check, clippy --all-targets -D warnings, test. Runs on stable, since pinning the gate to the MSRV would freeze clippy lints and rustfmt output at whatever 1.82 emitted. - `msrv` — cargo check against the `rust-version` declared in Cargo.toml (1.82), kept separate so an MSRV bump is a visible, deliberate change rather than something that rides along with a dependency update. `check` rather than `test`, because dev-dependencies are not bound by the MSRV the crate advertises to consumers. Both use Swatinem/rust-cache. Deliberately not path-filtered: a filtered job reports nothing when skipped, which makes it unusable as a required status check, and the cache makes a markdown-only PR cheap enough that always running is the better trade. cargo audit is deliberately excluded. A newly published advisory would redden main for a reason unrelated to the change under review; it stays a local step before adding a dependency (Standard §3.3). If it belongs in CI later, a scheduled job is the right shape, not a PR gate. construct-cli/AGENTS.md and CLAUDE.md document what is gated and what is not. They remain identical apart from their self-reference line, as that file pair requires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): correct the declared MSRV to 1.85, the real floor The new msrv job did its job on its first run: `cargo check` under 1.82 failed before compiling a single line of our code. The locked dependency tree pins indexmap 2.14.0, whose manifest requires the `edition2024` Cargo feature — stabilized in 1.85 — so 1.82 could not even parse the tree. `rust-version = "1.82"` was therefore a fiction, and had been for as long as that dependency had been locked. Nothing caught it because nothing ever built the crate on the version it claimed to support. Corrected to 1.85 and the job pinned to match, with the reasoning recorded in Cargo.toml so the next person raising it knows to re-measure rather than guess. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): MSRV is 1.88 — cargo named the packages that set it At 1.85 the resolver got far enough to report the real constraint instead of failing to parse: darling 0.23.0 and instability 0.3.12 — both transitive, via ratatui — require 1.88.0. So the floor is set by the locked dependency tree, not by this crate's own source, and 1.85 was only the first of two hurdles (indexmap's `edition2024` requirement). Recorded both in Cargo.toml so the next reader knows the number tracks dependencies and must be re-measured, not reasoned about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#25 made branch + PR the rule for this repo, but left
construct skill shiphard-codinggit push origin main(ship.rs:235). The tool we ship could still bypass the policy the docs state. This closes that gap.What
shipdoes nowEnforces bundle-drift and the §5.6 description cap (unchanged), then: resolves a feature branch → switches to it → stages by explicit name → signed UTC commit → pushes the branch with
-u→ opens the PR withgh.It never pushes to the default branch, and it never merges — that stays the maintainer's call.
Branch resolution, in order
--branch/-bwhen given.ship/<slug>from the shipped skills. Deterministic for the same skill set, which is what makes case 2's re-run land on the same branch.The default branch comes from
origin/HEAD, falling back tomainwhen unset (fresh clones and fixture repos have no remote refs).ghis now a dependencyProbed before the work tree is touched, so a missing
ghfails withDEPENDENCY_MISSING(127) rather than stranding a committed branch the caller has to finish by hand.If a PR for the branch is already open,
gh pr createexits non-zero. That is not a ship failure — the commit is already pushed onto the branch that PR tracks — so the existing URL is looked up and returned.Auto-sync removed
shipno longer lands anything on the default branch, sonix flake update constructwould have pinned an unrelated revision. Runconstruct skill syncafter the PR merges.--no-syncis kept as a hidden no-op so existing invocations and scripts keep working.Output changes
shippedpull_request_openedbranch,base_branch,pull_request_urlflake_updated,synced_atDry-run additionally reports
default_branch,current_branch,branch,would_create_branch,would_open_pull_request.manifest.rsupdated in step —manifest_in_sync_with_cliis the guard.Tests
Three new cases, and
--no-syncdropped from the existing ones to prove it is no longer needed:ship_from_default_branch_plans_a_feature_branch_and_pr— the core of the rule: must plan aship/branch and a PR, and never target the default branch.ship_honours_explicit_branchship_reuses_the_current_feature_branch9 ship tests, 40 total, all passing.
Drive-by, required to get a clean gate
src/sources/skillmd.rsdeclared itssplithelper aftermod tests, which trips clippy'sitems_after_test_moduleunder-D warnings. Pre-existing onmainand invisible because CI runs only reuse / config / description checks, nevercargo. Moved the test module to the end of the file — no logic change. Without it,cargo clippy --all-targets -- -D warningscannot pass, so this change could not be verified.Verification
cargo fmt --check— cleancargo clippy --all-targets -- -D warnings— cleancargo test— 40 passed, 0 failedmainit plansship/demo,would_create_branch: true,would_open_pull_request: true.reuse lintpass.Note that CI does not build or test
construct-cli— worth a follow-up, since that is why the clippy break above sat unnoticed.🤖 Generated with Claude Code