From 4320af4d9be152e0e608e6c4c22bbd3ccb75bfd1 Mon Sep 17 00:00:00 2001 From: UnbreakableMJ Date: Wed, 5 Aug 2026 09:04:05 +0000 Subject: [PATCH] feat(cli): construct skill ship opens a pull request instead of pushing to main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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/` 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) --- AGENTS.md | 7 +- CONTRIBUTING.md | 17 +- construct-cli/src/cli.rs | 15 +- construct-cli/src/commands/ship.rs | 349 ++++++++++++++++++++++++--- construct-cli/src/manifest.rs | 23 +- construct-cli/src/sources/skillmd.rs | 37 +-- construct-cli/tests/ship.rs | 120 ++++++++- 7 files changed, 479 insertions(+), 89 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 55fe560..b813587 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,7 +16,7 @@ rules that bite. (A maintainer-local `CLAUDE.md` overlay adds host-specific note rendered length is what counts, not the raw line count. Re-check after any edit. Normative as Standard §5.6, and gated in CI (`SKILL.md description cap` step, every `SKILL.md` in the tree) and by `construct skill ship`, which refuses to - stage or push an over-cap skill (exit 5, `CONFLICT`). The `.githooks/pre-commit` + stage or ship an over-cap skill (exit 5, `CONFLICT`). The `.githooks/pre-commit` hook is the fast local signal only — it is opt-in per clone (`git config core.hooksPath .githooks`), so trim *before* packing rather than relying on it. @@ -35,9 +35,8 @@ rules that bite. (A maintainer-local `CLAUDE.md` overlay adds host-specific note assistant-driven changes alike. There is **no auto-push exemption** for skill-directory edits. An agent's work ends at opening the PR — **merging is the maintainer's call**, and an agent never merges its own PR. - **`construct skill ship` predates this rule and hard-codes - `git push origin main`** — do not use it to publish until it is reworked; run - the workflow in [`CONTRIBUTING.md`](CONTRIBUTING.md) by hand instead. + `construct skill ship` implements this end-to-end — branch, signed commit, + push, `gh pr create` — and never pushes to the default branch. - **Keep the README §2 catalogue row in sync** when adding, removing, or re-scoping a skill. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 97f2ecf..d593473 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -100,12 +100,15 @@ diverge. Verify with `unzip -l .zip` before committing. Contributing agents stop at opening the PR — **merging is the maintainer's call**, and an agent never merges its own PR. - > **`construct skill ship` does not yet follow this rule.** - > `construct-cli/src/commands/ship.rs` hard-codes `git push origin main`, so - > the command publishes straight to the default branch. Until it is reworked - > (branch + `gh pr create`, or stop after the signed commit), do not use it - > to publish — follow steps 1–5 by hand. `--dry-run --no-sync` still - > exercises its §5.6 description-cap gate and bundle-drift check safely. + > `construct skill ship` automates steps 1–5: it enforces bundle-drift and + > the §5.6 description cap, switches to a feature branch (generated from the + > shipped skills, or `--branch`), stages by explicit name, makes the signed + > UTC commit, pushes the branch, and opens the PR with `gh`. It never pushes + > to the default branch and never merges. `--dry-run` reports the full plan, + > branch included, without changing anything. + > + > Because nothing lands on the default branch, `ship` does not sync — run + > `construct skill sync` after the PR merges. Never let `git status` show a skill-dir change without its matching bundle change. @@ -207,7 +210,7 @@ consolidated zip intentionally differs from any on-disk tree). snippet above is only for a quick manual count: - **CI** — the `SKILL.md description cap` step runs the checker over every `SKILL.md` in the tree on each PR and push to `main`. This is the gate. - - **`construct skill ship`** — refuses to stage, commit, or push a skill whose + - **`construct skill ship`** — refuses to stage, commit, or open a PR for a skill whose description is over the cap, before any bundle is shipped (exit 5, `CONFLICT`, with an `oversized_skills` list naming each offender). - **The [pre-commit hook](#pre-commit-hook)** — the fast local signal. It is diff --git a/construct-cli/src/cli.rs b/construct-cli/src/cli.rs index c6415c8..0a8fd6f 100644 --- a/construct-cli/src/cli.rs +++ b/construct-cli/src/cli.rs @@ -212,9 +212,9 @@ pub(crate) enum SkillCommand { )] Sync(SyncArgs), - /// Ship local skill edits: commit (signed) + push, then sync. + /// Ship local skill edits: branch + commit (signed) + open a pull request. #[command( - after_help = "Examples:\n construct skill ship --dry-run\n construct skill ship --skills spacecraft-rust-guidelines\n construct skill ship --message \"docs: clarify X\" --json" + after_help = "Never pushes to the default branch — every change goes through a\nfeature branch and a pull request, and merging is the maintainer's call.\nRun `construct skill sync` after the PR merges.\n\nExamples:\n construct skill ship --dry-run\n construct skill ship --skills spacecraft-rust-guidelines\n construct skill ship --branch ship/palette-fix --message \"docs: clarify X\" --json" )] Ship(ShipArgs), } @@ -373,8 +373,15 @@ pub(crate) struct ShipArgs { #[arg(short = 'm', long = "message", value_name = "MSG")] pub(crate) message: Option, - /// Commit and push but skip the final `skill sync` step. - #[arg(long)] + /// Feature branch to commit onto (default: derived from the shipped + /// skills, or the current branch when it is not the default branch). + #[arg(short = 'b', long = "branch", value_name = "NAME")] + pub(crate) branch: Option, + + /// Deprecated no-op. `ship` opens a pull request instead of pushing to the + /// default branch, so nothing lands to sync; run `construct skill sync` + /// after the PR merges. + #[arg(long, hide = true)] pub(crate) no_sync: bool, } diff --git a/construct-cli/src/commands/ship.rs b/construct-cli/src/commands/ship.rs index 2b61f41..a52ce57 100644 --- a/construct-cli/src/commands/ship.rs +++ b/construct-cli/src/commands/ship.rs @@ -1,15 +1,28 @@ // SPDX-FileCopyrightText: 2026 Mohamed Hammad // SPDX-License-Identifier: GPL-3.0-or-later -//! `construct skill ship` — the edit→ship→sync loop for the Construct catalogue. +//! `construct skill ship` — the edit→branch→commit→PR loop for the Construct +//! catalogue. //! //! It detects local skill edits in the construct clone, **enforces** the //! `.zip`/`.skill` bundling discipline (refusing to commit a skill-dir change //! whose bundles weren't rebuilt — it does not rebuild them itself), stages the //! shipped paths **explicitly by name** (never `git add -A`), creates a signed -//! UTC commit (via the repo's gitway signing config), pushes `origin main`, and -//! finally runs `skill sync` to repoint the consuming flake. All git operations -//! shell out to the system `git` so signing and credentials work transparently. +//! UTC commit (via the repo's gitway signing config), pushes a **feature +//! branch**, and opens a **pull request**. +//! +//! It never pushes to the default branch. `CONTRIBUTING.md` requires every +//! change — including a one-line version bump — to go through a feature branch +//! → pull request → squash-merge → delete branch, in both the Standard and +//! Construct repositories. Merging is the maintainer's call: this command +//! opens the PR and stops. +//! +//! Because nothing lands on the default branch here, `ship` no longer runs +//! `skill sync` — repointing a consumer flake before the PR merges would pin an +//! unrelated revision. Run `construct skill sync` once the PR is merged. +//! +//! All git operations shell out to the system `git` so signing and credentials +//! work transparently; the pull request is opened with `gh`. use std::collections::{BTreeMap, BTreeSet}; use std::path::{Path, PathBuf}; @@ -18,12 +31,11 @@ use std::process::Command as Proc; use serde_json::{json, Value}; use crate::cli::ShipArgs; -use crate::commands::sync; use crate::context::Context; use crate::install::plan::NON_SKILL_DIRS; use crate::output::error::{AppError, ErrorCode}; -use crate::sources::skillmd; use crate::output::{CommandOutput, HumanRender}; +use crate::sources::skillmd; /// Default catalogue clone to ship from. const DEFAULT_REPO: &str = "/spacecraft-software/construct"; @@ -43,6 +55,12 @@ const EXPECTED_REMOTE: &str = "Spacecraft-Software/Construct"; const DESCRIPTION_CAP: usize = 1000; /// Assistant co-authorship trailer (CONTRIBUTING §4). const COAUTHOR: &str = "Co-Authored-By: Claude Opus 4.8 (1M context) "; +/// `owner/repo` slug passed to `gh --repo`, derived from [`EXPECTED_REMOTE`]. +const REPO_SLUG: &str = EXPECTED_REMOTE; +/// Fallback default branch when `origin/HEAD` is not configured locally. +const FALLBACK_DEFAULT_BRANCH: &str = "main"; +/// Prefix for generated feature branches. +const BRANCH_PREFIX: &str = "ship/"; /// Parsed working-tree changes, grouped for shipping decisions. #[derive(Debug, Default)] @@ -60,7 +78,7 @@ struct Changes { /// Run the ship loop (or, under `--dry-run`, report the exact plan). #[allow( clippy::too_many_lines, - reason = "a single linear detect -> enforce -> stage -> commit -> push -> sync flow; clearer unsplit" + reason = "a single linear detect -> enforce -> branch -> stage -> commit -> push -> PR flow; clearer unsplit" )] pub(crate) fn run(ctx: &Context, args: &ShipArgs) -> Result { let repo = args @@ -178,18 +196,29 @@ pub(crate) fn run(ctx: &Context, args: &ShipArgs) -> Result 0 { "planned_push" } else { "nothing_to_ship" }, + "status": if will_commit { "planned" } else if ahead > 0 { "planned_pull_request" } else { "nothing_to_ship" }, "shipped_skills": shipped, "would_stage": stage, "left_unstaged": left_unstaged, "would_commit": will_commit, "commit_message": message, - "would_push": will_commit || ahead > 0, - "would_sync": !args.no_sync, + "default_branch": default_branch, + "current_branch": current, + "branch": branch, + "would_create_branch": needs_switch, + "would_push_branch": will_commit || ahead > 0, + "would_open_pull_request": will_commit || ahead > 0, "unpushed_commits": ahead, }); let human = HumanRender::Message(plan_summary( @@ -198,29 +227,37 @@ pub(crate) fn run(ctx: &Context, args: &ShipArgs) -> Result Result Result AppError { // ── misc helpers ─────────────────────────────────────────────────────────--- -/// Run the flake sync unless `--no-sync`; returns the sync timestamp. -fn maybe_sync(ctx: &Context, args: &ShipArgs) -> Result, AppError> { - if args.no_sync { - return Ok(None); +/// The repository's default branch, via `origin/HEAD` when it is configured. +/// +/// A fresh clone that never ran `git remote set-head` has no `origin/HEAD`, and +/// a throwaway fixture repo has no remote refs at all — both fall back to +/// [`FALLBACK_DEFAULT_BRANCH`]. +fn default_branch(ctx: &Context, repo: &Path) -> String { + git_capture( + ctx, + repo, + &["symbolic-ref", "--short", "refs/remotes/origin/HEAD"], + ) + .ok() + .and_then(|s| { + s.trim() + .rsplit_once('/') + .map(|(_, name)| name.to_owned()) + .filter(|name| !name.is_empty()) + }) + .unwrap_or_else(|| FALLBACK_DEFAULT_BRANCH.to_owned()) +} + +/// The currently checked-out branch name. +fn current_branch(ctx: &Context, repo: &Path) -> Result { + Ok( + git_capture(ctx, repo, &["rev-parse", "--abbrev-ref", "HEAD"])? + .trim() + .to_owned(), + ) +} + +/// Decide which branch the work belongs on. +/// +/// An explicit `--branch` always wins. Otherwise, if the caller already put +/// themselves on a feature branch, respect it — re-running `ship` should add to +/// the open PR rather than fragment the work across branches. Only when sitting +/// on the default branch is a name generated. +fn resolve_branch( + args: &ShipArgs, + shipped: &[String], + current: &str, + default_branch: &str, +) -> String { + if let Some(explicit) = args.branch.as_deref().map(str::trim) { + if !explicit.is_empty() { + return explicit.to_owned(); + } + } + if current != default_branch && current != "HEAD" { + return current.to_owned(); + } + generated_branch(shipped) +} + +/// Generate a feature-branch name from the shipped skills. +/// +/// Re-running with the same skills yields the same name on purpose: the second +/// run adds a commit to the branch the PR is already tracking. +fn generated_branch(shipped: &[String]) -> String { + let slug = match shipped { + [] => "catalogue".to_owned(), + [only] => slugify(only), + [first, rest @ ..] => format!("{}-plus-{}", slugify(first), rest.len()), + }; + format!("{BRANCH_PREFIX}{slug}") +} + +/// Reduce a skill id to characters that are safe in a git ref. +fn slugify(name: &str) -> String { + let cleaned: String = name + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || c == '-' || c == '_' { + c.to_ascii_lowercase() + } else { + '-' + } + }) + .collect(); + let trimmed = cleaned.trim_matches('-').to_owned(); + if trimmed.is_empty() { + "catalogue".to_owned() + } else { + trimmed + } +} + +/// Check out `branch`, creating it if it does not exist yet. +fn switch_branch(ctx: &Context, repo: &Path, branch: &str) -> Result<(), AppError> { + let exists = git_capture( + ctx, + repo, + &[ + "rev-parse", + "--verify", + "--quiet", + &format!("refs/heads/{branch}"), + ], + ) + .is_ok(); + let args: Vec<&str> = if exists { + vec!["switch", branch] + } else { + vec!["switch", "-c", branch] + }; + git_capture(ctx, repo, &args).map(|_| ()) +} + +/// Fail early if `gh` is not available, so we never strand a committed branch. +fn require_gh(ctx: &Context) -> Result<(), AppError> { + match Proc::new("gh").arg("--version").output() { + Ok(_) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Err(AppError::dependency_missing( + ctx, + "`gh` was not found on PATH — it is required to open the pull request", + "gh --version # install the GitHub CLI, then re-run construct skill ship", + )), + Err(e) => Err(AppError::general( + ctx, + ErrorCode::InternalError, + format!("failed to launch gh: {e}"), + "verify the GitHub CLI is installed and on PATH", + )), + } +} + +/// Open the pull request and return its URL. +/// +/// If a PR for this branch is already open, `gh` exits non-zero; that is not a +/// failure of the ship — the new commit is already pushed onto the branch the +/// existing PR tracks — so the existing URL is looked up and returned instead. +fn open_pull_request( + ctx: &Context, + repo: &Path, + branch: &str, + base: &str, + title: &str, + shipped: &[String], +) -> Result { + let body = pr_body(shipped); + let output = Proc::new("gh") + .arg("-C") + .arg(repo) + .args([ + "pr", + "create", + "--repo", + REPO_SLUG, + "--base", + base, + "--head", + branch, + "--title", + title, + "--body", + body.as_str(), + ]) + .output() + .map_err(|e| gh_launch_error(ctx, &e))?; + + if output.status.success() { + return Ok(String::from_utf8_lossy(&output.stdout).trim().to_owned()); + } + if let Some(url) = existing_pr_url(repo, branch) { + return Ok(url); + } + Err(AppError::general( + ctx, + ErrorCode::InternalError, + format!("gh pr create failed: {}", tail(&output.stderr)), + format!( + "cd {} && gh pr create --base {base} --head {branch}", + repo.display() + ), + )) +} + +/// URL of an already-open PR for `branch`, if there is one. +fn existing_pr_url(repo: &Path, branch: &str) -> Option { + let output = Proc::new("gh") + .arg("-C") + .arg(repo) + .args([ + "pr", "view", branch, "--repo", REPO_SLUG, "--json", "url", "--jq", ".url", + ]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + let url = String::from_utf8_lossy(&output.stdout).trim().to_owned(); + (!url.is_empty()).then_some(url) +} + +/// Pull-request body describing what is being shipped. +fn pr_body(shipped: &[String]) -> String { + let mut lines = vec![ + "Opened by `construct skill ship`.".to_owned(), + String::new(), + ]; + if shipped.is_empty() { + lines.push("Ships pending catalogue commits.".to_owned()); + } else { + lines.push("Skills shipped:".to_owned()); + lines.push(String::new()); + for skill in shipped { + lines.push(format!( + "- `{skill}` — source and both bundles (`.zip` + `.skill`) in the same commit" + )); + } + } + lines.push(String::new()); + lines.push( + "Bundle-drift and the Standard §5.6 description cap were enforced before staging." + .to_owned(), + ); + lines.push(String::new()); + lines.push( + "After this merges, run `construct skill sync` to repoint the consumer flake.".to_owned(), + ); + lines.join("\n") +} + +/// Map a `gh` launch error to an `AppError`. +fn gh_launch_error(ctx: &Context, err: &std::io::Error) -> AppError { + if err.kind() == std::io::ErrorKind::NotFound { + AppError::dependency_missing( + ctx, + "`gh` was not found on PATH — it is required to open the pull request", + "gh --version # install the GitHub CLI, then re-run construct skill ship", + ) + } else { + AppError::general( + ctx, + ErrorCode::InternalError, + format!("failed to launch gh: {err}"), + "verify the GitHub CLI is installed and on PATH", + ) } - let synced_at = sync::flake_update(ctx, &PathBuf::from(sync::DEFAULT_FLAKE_DIR))?; - Ok(Some(synced_at)) } /// The exact bundle-rebuild command for a drifted skill (a runnable hint). @@ -519,24 +789,25 @@ fn plan_summary( stage: &[String], will_commit: bool, ahead: u32, - no_sync: bool, + branch: &str, + needs_switch: bool, ) -> String { let mut lines = vec![format!("[dry-run] ship from {}", repo.display())]; if will_commit { lines.push(format!(" would stage: {}", stage.join(", "))); - lines.push(format!( - " would commit + push skills: {}", - shipped.join(", ") - )); + lines.push(format!(" would commit skills: {}", shipped.join(", "))); } else if ahead > 0 { lines.push(format!(" would push {ahead} existing commit(s)")); } else { lines.push(" nothing to commit or push".to_owned()); } - lines.push(format!( - " would sync: {}", - if no_sync { "no" } else { "yes" } - )); + if will_commit || ahead > 0 { + lines.push(format!( + " would {} branch: {branch}", + if needs_switch { "create" } else { "reuse" } + )); + lines.push(" would open a pull request (never pushes to the default branch)".to_owned()); + } lines.join("\n") } diff --git a/construct-cli/src/manifest.rs b/construct-cli/src/manifest.rs index 4b40167..3688b54 100644 --- a/construct-cli/src/manifest.rs +++ b/construct-cli/src/manifest.rs @@ -191,15 +191,17 @@ pub(crate) fn commands() -> Vec { name: "construct skill ship".to_owned(), noun: "skill".to_owned(), verb: "ship".to_owned(), - description: "Commit (signed) + push local skill edits, then sync".to_owned(), + description: "Branch + commit (signed) local skill edits, then open a pull request" + .to_owned(), parameters: json!({ "type": "object", "additionalProperties": false, "properties": { "repo": { "type": "string", "description": "Construct clone to ship from" }, "skills": { "type": "array", "items": { "type": "string" }, "description": "Restrict to these skills (default: all changed)" }, - "message": { "type": "string", "description": "Commit message subject" }, - "no_sync": { "type": "boolean", "default": false, "description": "Skip the final flake sync" } + "message": { "type": "string", "description": "Commit message subject, also the pull-request title" }, + "branch": { "type": "string", "description": "Feature branch to commit onto (default: derived from the shipped skills, or the current branch when it is not the default branch)" }, + "no_sync": { "type": "boolean", "default": false, "description": "Deprecated no-op — ship opens a pull request, so nothing lands to sync" } } }), output_data: json!({ @@ -211,30 +213,31 @@ pub(crate) fn commands() -> Vec { "committed": { "type": "boolean" }, "commit_sha": { "type": "string" }, "signed": { "type": "boolean" }, + "branch": { "type": "string" }, + "base_branch": { "type": "string" }, "pushed": { "type": "boolean" }, - "flake_updated": { "type": "boolean" }, - "synced_at": { "type": ["string", "null"] } + "pull_request_url": { "type": ["string", "null"] } } }), exit_codes: pairs(&[ - ("0", "SUCCESS — shipped (or planned under --dry-run)"), - ("1", "GENERAL_FAILURE — a git or nix command failed"), + ("0", "SUCCESS — pull request opened (or planned under --dry-run)"), + ("1", "GENERAL_FAILURE — a git or gh command failed"), ("2", "USAGE_ERROR — repo is not the Construct work tree"), ("3", "NOT_FOUND — repo path does not exist"), ( "5", "CONFLICT — skill source changed without rebuilt .zip/.skill bundles, or a SKILL.md description exceeds the 1000-character cap (Standard §5.6)", ), - ("127", "DEPENDENCY_MISSING — git or nix not on PATH"), + ("127", "DEPENDENCY_MISSING — git or gh not on PATH"), ]), examples: pairs(&[ ( "construct skill ship --dry-run", - "Preview what would be committed and pushed", + "Preview the branch, commit, and pull request without changing anything", ), ( "construct skill ship --skills spacecraft-rust-guidelines", - "Ship one skill's edits", + "Ship one skill's edits on a generated branch and open its PR", ), ]), supports_json: true, diff --git a/construct-cli/src/sources/skillmd.rs b/construct-cli/src/sources/skillmd.rs index dc22cb2..1d8b26f 100644 --- a/construct-cli/src/sources/skillmd.rs +++ b/construct-cli/src/sources/skillmd.rs @@ -57,6 +57,22 @@ pub(crate) fn body(skill_md: &Path) -> String { } } +/// Split `---\n\n---\n` into `(frontmatter, body)`. +fn split(content: &str) -> Option<(&str, &str)> { + let rest = content + .strip_prefix("---\n") + .or_else(|| content.strip_prefix("---\r\n"))?; + let idx = rest.find("\n---")?; + // Inclusive of the newline before the closing fence: without it a block + // scalar that is the *last* frontmatter key loses its trailing newline, and + // `description_len` would under-count by one against the loader. + let fm = &rest[..=idx]; + // Body begins after the closing fence line. + let after = &rest[idx + 1..]; // at the closing "---" + let body = after.split_once('\n').map_or("", |(_, b)| b); + Some((fm, body)) +} + #[cfg(test)] mod tests { use std::io::Write as _; @@ -74,7 +90,10 @@ mod tests { fn folded_scalar_keeps_its_trailing_newline_as_the_last_key() { // The closing `---` fence ends the block. The loader still counts the // newline that terminates the folded content, so this is 4 chars. - assert_eq!(len_of("---\nname: d\ndescription: >\n abc\n---\nb\n"), Some(4)); + assert_eq!( + len_of("---\nname: d\ndescription: >\n abc\n---\nb\n"), + Some(4) + ); } #[test] @@ -107,19 +126,3 @@ mod tests { assert_eq!(len_of("no frontmatter here\n"), None); } } - -/// Split `---\n\n---\n` into `(frontmatter, body)`. -fn split(content: &str) -> Option<(&str, &str)> { - let rest = content - .strip_prefix("---\n") - .or_else(|| content.strip_prefix("---\r\n"))?; - let idx = rest.find("\n---")?; - // Inclusive of the newline before the closing fence: without it a block - // scalar that is the *last* frontmatter key loses its trailing newline, and - // `description_len` would under-count by one against the loader. - let fm = &rest[..=idx]; - // Body begins after the closing fence line. - let after = &rest[idx + 1..]; // at the closing "---" - let body = after.split_once('\n').map_or("", |(_, b)| b); - Some((fm, body)) -} diff --git a/construct-cli/tests/ship.rs b/construct-cli/tests/ship.rs index 8b4b936..724df5a 100644 --- a/construct-cli/tests/ship.rs +++ b/construct-cli/tests/ship.rs @@ -2,8 +2,9 @@ // SPDX-License-Identifier: GPL-3.0-or-later //! Black-box tests for `construct skill ship` against throwaway git fixtures. -//! All use `--dry-run --no-sync`, so no commit, push, or flake update happens — -//! they exercise detection, the bundle-drift refusal, and remote validation. +//! All use `--dry-run`, so nothing is committed, branched, pushed, or opened — +//! they exercise detection, the bundle-drift refusal, the §5.6 description cap, +//! remote validation, and the branch/pull-request plan. use std::fs; use std::path::Path; @@ -70,7 +71,6 @@ fn ship_dry_run_reports_plan_when_bundles_rebuilt() { "ship", "--repo", p.to_str().unwrap(), - "--no-sync", "--dry-run", "--json", ]) @@ -89,6 +89,115 @@ fn ship_dry_run_reports_plan_when_bundles_rebuilt() { assert!(stage.iter().any(|s| s == "demo/SKILL.md")); } +/// The core of the branch+PR rule: shipping from the default branch must plan a +/// feature branch and a pull request, never a push to the default branch. +#[test] +fn ship_from_default_branch_plans_a_feature_branch_and_pr() { + let repo = fixture(REMOTE); + let p = repo.path(); + write(p, "demo/SKILL.md", "---\nname: demo\n---\nv1\n"); + write(p, "demo.zip", "z1"); + write(p, "demo.skill", "s1"); + run_git(p, &["add", "demo/SKILL.md", "demo.zip", "demo.skill"]); + run_git(p, &["commit", "-qm", "init"]); + write(p, "demo/SKILL.md", "---\nname: demo\n---\nv2\n"); + write(p, "demo.zip", "z2"); + write(p, "demo.skill", "s2"); + + let out = bin() + .args([ + "skill", + "ship", + "--repo", + p.to_str().unwrap(), + "--dry-run", + "--json", + ]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let v: Value = serde_json::from_slice(&out).expect("valid JSON"); + let branch = v["data"]["branch"].as_str().unwrap(); + assert_eq!(v["data"]["current_branch"], "main"); + assert_eq!(v["data"]["default_branch"], "main"); + assert_ne!(branch, "main", "must never target the default branch"); + assert!(branch.starts_with("ship/"), "generated branch: {branch}"); + assert!(branch.contains("demo")); + assert_eq!(v["data"]["would_create_branch"], true); + assert_eq!(v["data"]["would_open_pull_request"], true); +} + +/// An explicit `--branch` wins over the generated name. +#[test] +fn ship_honours_explicit_branch() { + let repo = fixture(REMOTE); + let p = repo.path(); + write(p, "demo/SKILL.md", "---\nname: demo\n---\nv1\n"); + write(p, "demo.zip", "z1"); + write(p, "demo.skill", "s1"); + run_git(p, &["add", "demo/SKILL.md", "demo.zip", "demo.skill"]); + run_git(p, &["commit", "-qm", "init"]); + write(p, "demo/SKILL.md", "---\nname: demo\n---\nv2\n"); + write(p, "demo.zip", "z2"); + write(p, "demo.skill", "s2"); + + let out = bin() + .args([ + "skill", + "ship", + "--repo", + p.to_str().unwrap(), + "--branch", + "ship/custom-name", + "--dry-run", + "--json", + ]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let v: Value = serde_json::from_slice(&out).expect("valid JSON"); + assert_eq!(v["data"]["branch"], "ship/custom-name"); +} + +/// Already on a feature branch: reuse it, so a re-run adds to the open PR +/// rather than fragmenting the work across branches. +#[test] +fn ship_reuses_the_current_feature_branch() { + let repo = fixture(REMOTE); + let p = repo.path(); + write(p, "demo/SKILL.md", "---\nname: demo\n---\nv1\n"); + write(p, "demo.zip", "z1"); + write(p, "demo.skill", "s1"); + run_git(p, &["add", "demo/SKILL.md", "demo.zip", "demo.skill"]); + run_git(p, &["commit", "-qm", "init"]); + run_git(p, &["switch", "-q", "-c", "ship/already-here"]); + write(p, "demo/SKILL.md", "---\nname: demo\n---\nv2\n"); + write(p, "demo.zip", "z2"); + write(p, "demo.skill", "s2"); + + let out = bin() + .args([ + "skill", + "ship", + "--repo", + p.to_str().unwrap(), + "--dry-run", + "--json", + ]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let v: Value = serde_json::from_slice(&out).expect("valid JSON"); + assert_eq!(v["data"]["branch"], "ship/already-here"); + assert_eq!(v["data"]["would_create_branch"], false); +} + #[test] fn ship_refuses_bundle_drift() { let repo = fixture(REMOTE); @@ -107,7 +216,6 @@ fn ship_refuses_bundle_drift() { "ship", "--repo", p.to_str().unwrap(), - "--no-sync", "--dry-run", "--json", ]) @@ -150,7 +258,6 @@ fn ship_refuses_oversized_description() { "ship", "--repo", p.to_str().unwrap(), - "--no-sync", "--dry-run", "--json", ]) @@ -184,7 +291,6 @@ fn ship_allows_description_exactly_at_cap() { "ship", "--repo", p.to_str().unwrap(), - "--no-sync", "--dry-run", "--json", ]) @@ -208,7 +314,6 @@ fn ship_rejects_wrong_remote() { "ship", "--repo", p.to_str().unwrap(), - "--no-sync", "--dry-run", "--json", ]) @@ -230,7 +335,6 @@ fn ship_nothing_to_ship_on_clean_repo() { "ship", "--repo", p.to_str().unwrap(), - "--no-sync", "--dry-run", "--json", ])