docs: require branch + PR in Construct, withdrawing the auto-push exemption - #25
Merged
Conversation
…mption The two repos disagreed about workflow. /spacecraft-software/standard/CLAUDE.md states "Branch + PR, never push to main" for **both** the Standard and Construct repos, while Construct's own maintainer-local CLAUDE.md claimed this repo was "pre-authorised for auto-push on skill-directory changes" and CONTRIBUTING.md step 5 was a bare `git push … main`. Construct's actual recent history (#11, #21, #22) is squash-merged PRs, so the auto-push note was the outlier. Resolved in favour of branch + PR: - AGENTS.md gains a "Branch + PR — never push to main" hard rule (it had no push rule at all, only a pointer to CONTRIBUTING.md). - CONTRIBUTING.md step 5 replaced: feature branch → PR → squash-merge → delete branch, with the gh invocation. Noted as a two-repo rule binding human and assistant-driven changes alike. - Both state that an agent stops at opening the PR; merging is the maintainer's call and an agent never merges its own PR. - The maintainer-local CLAUDE.md (gitignored) is updated to match. Also flagged, not fixed: `construct skill ship` hard-codes `git push origin main` (construct-cli/src/commands/ship.rs:235), so the shipped tool can still bypass the rule. All three docs now say not to use it to publish until it is reworked — branch + `gh pr create`, or stop after the signed commit. That is a CLI behaviour change with schema, help-text, and test impact, so it is left as a separate decision. No skill directories touched, so no bundle rebuild. Description cap and reuse lint pass. 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
…ng to main (#26) #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>
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
1 participant
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
The two repos disagreed about workflow, and I hit the conflict while landing the v1.42 §13 change across both.
/spacecraft-software/standard/CLAUDE.md— "Branch + PR, never push tomain. Every change — even a one-line version bump — goes through a feature branch → PR → squash-merge → delete branch, in both the Standard and Construct repos."CLAUDE.md— "Push toorigin/mainwith no confirmation prompt — this repo is pre-authorised for auto-push on skill-directory changes."CONTRIBUTING.mdstep 5 — a baregit push … main.AGENTS.md— no push rule at all, only a pointer toCONTRIBUTING.md.Construct's own recent history (#11, #21, #22) is squash-merged PRs, so the auto-push note was the outlier rather than the policy.
What changed
Resolved in favour of branch + PR:
AGENTS.mdgains aBranch + PR — never push to mainhard rule alongside the existing bundle/staging/UTC-signing rules.CONTRIBUTING.mdstep 5 replaced — feature branch → PR → squash-merge → delete branch, with thegh pr createinvocation and the HTTPS-over-SSH note preserved.CLAUDE.md(gitignored, not in this diff) is updated to match.Flagged, deliberately not fixed here
construct skill shiphard-codesgit push origin main—construct-cli/src/commands/ship.rs:235— so the tool this repo ships can still bypass the rule the docs now state.All three docs now say not to use it to publish until it is reworked. The fix is a real CLI behaviour change (branch +
gh pr create, or stop after the signed commit) with knock-on effects on thepushedoutput field,manifest.rsdescriptions,cli.rshelp text, andtests/ship.rs— so it wants its own decision and its own PR. Its §5.6 description-cap gate and bundle-drift check remain useful meanwhile;--dry-run --no-syncexercises them without pushing.Verification
.githooks/check-description-length.pyover everySKILL.md→ pass.reuse lint→ compliant.Note on the preceding commits
The three commits that landed earlier today (
094c806,86b57d1, and the v1.42 skill sync) went directly tomainunder the now-withdrawn auto-push note. I have not rewritten that history — rewriting pushedmainis worse than the inconsistency. This PR is the first Construct change to follow the new rule.🤖 Generated with Claude Code