v1.58.0.0 feat: /pr-prep — pre-PR upstream duplicate audit + /ship Step 1.5 gate - #1696
BenjaminDSmithy wants to merge 20 commits into
Conversation
8d43b46 to
2a5fa08
Compare
|
Updated this branch. Rebased onto Commit messages now follow the repo convention across every commit: New — Step 4.6 in Test + registration fixes so the branch is green:
|
Dogfooded
|
908509a to
f3a7e53
Compare
f3a7e53 to
29e4c17
Compare
|
@16francej — recommend closing this PR. Thanks for putting this together. We aren’t accepting new standalone skills right now, so we’re going to pass on this proposal. We can revisit the area later if it becomes part of GStack’s core workflow. |
38e813c to
e024d11
Compare
@time-attack @16francej @garrytan: tagging all three, since this is a policy call rather than a code review. Understood on the policy, and the reasoning holds: a new skill is a permanent cost to the discovery catalog that every user pays whether they run it or not. Two things first. Split for reviewThis PR had accumulated work that has nothing to do with Why this one is worth reconsideringIt isn't a workflow skill competing for catalog space. It reduces your triage load, and your own release notes make the case:
Three waves in five weeks. A real share of that queue was people solving what someone else had already solved. Each one cost a contributor hours and cost you a triage slot. The data that would have prevented it was public and one That's the gap. Before a branch becomes a PR, Wider point: this isn't a gstack problem. Every project with an active tracker and drive-by contributions has it, and no tooling checks. gstack is already what runs at the moment a PR gets opened, which makes it the one place the check actually fires. Cutting duplicate-PR waste across open source looks on-mission for a workflow tool, and nothing else ships it. Taking your condition literallyYou said you'd revisit it "if it becomes part of GStack's core workflow". So: if the blocker is the catalog entry, I'll ship it as State of the branchAlready landed here:
Ready to swap in: a branch cut straight from
A PR can't be repointed at another branch, so swapping means opening a replacement and closing this one. Tell me which version you want, standalone skill or ship-only, and it'll be up the same day. One note: I ran the audit against this PR before writing this. It found #1696. |
|
Opened #2642 against this PR's surface: It came out of using this skill in anger today. Filing a Claude Code bug report ( The issue has a proposed shape that reuses Step 3 and I would be glad to build it, folded into this PR or as a follow-up. I would rather wait for a steer on this PR first, though: whether you want |
|
Dogfooded this skill against its own branch and it found a false positive in its own Step 4 bucketing. Fixed in d1cd1eb, now part of this PR. The {"bucket":"OVERLAP","topScore":0.05,"openIssueCount":4,"reasons":["OVERLAP: 4 open issues"]}The four issues it counted were a triggers-frontmatter bug, a review-bias issue, It never false-blocked — OVERLAP is informational and only EXACT_DUP refuses — but a bucket that always fires stops being read, which defeats the point of the audit. The fix adds Same dogfood pass after the fix: 4 OVERLAP → 1, and the survivor is Worth flagging that the existing unit test asserted the old behaviour with three issues at Jaccard 0 — it encoded the noise as signal. I retargeted it at genuinely related issues (each scoring 0.2, between the floor and the single-hit threshold) rather than deleting it, and added a regression case built from the four real issue titles above. To be clear on scope: this is a fix to code already in this PR, not the issue-mode work in #2642. That one is still parked pending your steer on whether |
d1cd1eb to
7d3c026
Compare
Walks `git log base..HEAD`, derives search keywords per commit from subject + changed file paths, queries upstream issues + PRs via `gh`, scores each commit against upstream collisions (EXACT_DUP / OVERLAP / SIBLING / CLEAN) on a title-token + file-overlap Jaccard, and refuses to proceed when EXACT_DUP found. Designed to slot into `/ship` as a Step 0 hook (env `GSTACK_FROM_SHIP=1` switches to JSON output + skips interactive prompts). Motivating case (real, 2026-05-26): a contributor branch had 8 commits ready for upstream PRs; 4 of 4 unverified commits would have duplicated already-open upstream issues. pr-prep catches all in ~30-60s of `gh` queries, before any noise PR or reviewer triage round. v0.1.0 ships inline bash in SKILL.md (reviewable in one file). Out of scope: diff-content similarity, cross-repo audit, LLM-judged semantic dup detection, auto-comment on upstream PRs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`/ship` now invokes `/pr-prep --base $BASE_BRANCH --json` with `GSTACK_FROM_SHIP=1` before any of: merge base branch, run tests, version bump, push. If pr-prep returns EXACT_DUP (exit 1), ship aborts with a pinpoint message naming the upstream PR + resolution paths (close mine / cherry-pick unique parts / coordinate + retry with `--skip-pr-prep`). Skip conditions: no upstream remote (solo-repo case), `--skip-pr-prep` flag, or pr-prep skill not installed (older gstack — stderr warn + continue). SIBLING / OVERLAP / CLEAN buckets do not block. The JSON report is written to `/tmp/ship-pr-prep.json` so PR body assembly can render upstream context as a collapsed section. Fails fast before any test run wastes time on a branch that duplicates already-open upstream work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… per commit Step 1.4: fetch `CONTRIBUTING.md` (case-insensitive) via gh api from the upstream repo, cache to /tmp, extract pre-push commands + test layout conventions + branch naming rules + banned patterns. The agent uses these inline when writing PR bodies. Step 4.5: annotate each CLEAN/OVERLAP/SIBLING commit row with the required pre-push gate (e.g. `bun run verify`), whether changed files trigger special test paths (eval-replay for retrieval), and whether the commit added tests. Soft warning on missing-tests when not-required is unclear — don't block, let the human decide. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mmits Before filing each CLEAN commit (not bucketed EXACT_DUP / OVERLAP / SIBLING), invoke `codex review` for an independent second opinion. Codex CLI uses a different model family (OpenAI vs Claude), so the signal is genuinely independent and catches structural bugs the author missed during write-up. Optional: if `codex` is not on PATH, emit a soft warning and continue; never block on tool availability. Severity escalation: P0/P1 findings bump the commit from CLEAN to OVERLAP (don't file until addressed); P2 stays CLEAN at author discretion (fix-before-file or note in PR body). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new `## Step 1.5` heading in `ship/SKILL.md.tmpl` (the pr-prep gate) tripped the step-numbering checks in `skill-validation`, which only permit a closed set of fractional sub-steps. Add `1.5` to `ALLOWED_SUBSTEPS` so the gate heading is recognised as intentional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The doc-inventory cross-check requires every skill directory to appear in both `AGENTS.md` and `docs/skills.md`. Add a `/pr-prep` row to each so the new skill is documented and the check passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The committed `pr-prep/SKILL.md` was generated against an older preamble and had drifted from `bun run gen:skill-docs` output. Regenerate it so it picks up the current shared resolvers: the `SESSION_KIND` and `GSTACK_PLAN_MODE` preamble lines, the `/spec` routing entry, the AskUserQuestion failure fallback, the 5+-option split rule, and the Boil-the-Ocean rename. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Step 4.6: a per-commit conformance check that holds the branch's commit messages to the UPSTREAM repo's convention, not the contributor's own house style. A fork PR whose commits read in a different voice than the project reads as a drive-by and burns reviewer goodwill before the diff is read. The authoritative style is resolved once (Step 1.4): the upstream CONTRIBUTING.md commit rules if present, else the de-facto shape sampled from `git log upstream/$BASE_BRANCH --no-merges`, else the conventional-commits baseline. Step 4.6 then flags subject-shape drift, a personal body template (emoji/bullets) where upstream uses prose, a subject that promises content it lacks (e.g. "+ tests" with no tests), and a missing-or-extra trailer relative to upstream (e.g. a `Co-Authored-By:` line upstream carries on every commit). Surfaced as a soft warning in the report, never a block — style is not a duplicate, but it is the cheapest goodwill win in the audit and far cheaper to fix before the PR exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The golden-file regression check pins the generated per-host ship skill (Claude, Codex, Factory) against committed snapshots. Adding the Step 1.5 pr-prep gate to `ship/SKILL.md.tmpl` changed all three generated outputs, so the snapshots no longer matched. Regenerate them via `gen:skill-docs --host all` and re-capture; the diff is exactly the Step 1.5 block, identical across hosts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The coverage gate (skill-coverage-matrix + skill-coverage-floor) requires every skill on disk to have a registry entry with at least one gate-tier test. The new pr-prep skill had none, failing both checks. Register it with the structural floor test as its gate-tier minimum, matching how other audit/report skills (qa-only, investigate) are covered until a behavioral E2E is written. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Step 4 bucketing (title/file Jaccard, state weighting, EXACT_DUP / OVERLAP / SIBLING / CLEAN precedence) lived only as inline bash in the skill, so it had no behavioral coverage. Extract it into a pure, deterministic CLI, `bin/gstack-pr-prep-score`, and pin every bucket threshold in `test/pr-prep-score.test.ts` (13 cases, free, gate-tier). The skill's Step 4 now points at the scorer as the canonical implementation rather than re-deriving the thresholds inline, and the coverage matrix gates pr-prep on the new behavioral test. This is the v0.2.0 extraction the skill flagged, scoped to the scoring core. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Measured the {{PREAMBLE}} block that scripts/gen-skill-docs.ts injects into
every generated SKILL.md: 46KB per file. Deliberately made no code change.
Upstream churns the resolvers and the generated output every release (one
delta was 51 files and 1,469 insertions), so a fork-side trim would
conflict on every merge and would be the wrong place to fix it anyway.
- PREAMBLE-BLOAT-NOTES.md records per-tier preamble sizes, the section
breakdown, and a top-ten offender table.
- The safe path it documents is an upstream change extending the existing
sections/ carve-out to move roughly 20.5KB of tier-2 and above
interaction prose behind on-demand loading, worth about 5,100 tokens per
invocation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The decision-brief preamble never said whether options were mutually exclusive, so multiSelect was being set on questions whose options were bundles of the same underlying items. Scoring those per sub-item produces a brief that cannot be acted on. - generate-ask-user-format.ts now states that options are mutually exclusive by default, and that multiSelect: true is legal only when every option is an independently-selectable atom whose pro, con and effort stand alone. - Bundles and combinations (E1+E3, All three, E1 only, Defer all) are mutually exclusive by construction, so they take multiSelect: false and score Completeness per option letter rather than per sub-item. - Names the diagnostic tell: a defer/none option or a do-everything option in the list proves the question is single-select. - Routes five or more independent atoms to the existing split chain instead of multiSelect, so the four-option cap is never silently exceeded. The rule lives in the preamble resolver rather than in any one skill, so every skill that renders a decision brief inherits it from one source. The 44 regenerated SKILL.md files are build output. Verified with bun test over host-config, skill-validation, audit-compliance, question-log-hook and gstack-question-preference: 387 pass, 0 fail. The three golden-file regressions were already red at HEAD and are untouched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every skill description sits in Claude Code's always-loaded prompt, whether or not the model can invoke that skill. Skills that are only ever reached by an explicit slash command were paying that cost for nothing, about 1,050 tokens per request here. The flag lives in each SKILL.md.tmpl; generated SKILL.md files are regenerated with gen:skill-docs. The claude host uses denylist frontmatter mode, so the key passes through untouched. - Flagged (36): benchmark, benchmark-models, canary, careful, codex, cso, design-html, design-shotgun, devex-review, diagram, document-generate, document-release, freeze, gstack-upgrade, guard, health, ios-clean, ios-design-review, ios-fix, ios-qa, ios-sync, landing-report, make-pdf, open-gstack-browser (covering the connect-chrome alias), pair-agent, plan-devex-review, plan-tune, pr-prep, retro, scrape, setup-browser-cookies, setup-deploy, setup-gbrain, skillify, spec, unfreeze. - Left model-invocable on purpose: every skill named in a CLAUDE.md skill-routing table, being autoplan, browse, context-restore, context-save, design-consultation, design-review, investigate, land-and-deploy, learn, office-hours, plan-ceo-review, plan-design-review, plan-eng-review, qa, qa-only, review, ship and sync-gbrain, plus the _gstack-command router. - Tier-1 suites green (733 pass, 0 fail). The browse/test/sidebar-ux.test.ts failures reproduce with this change stashed, so they are pre-existing and need a live browse daemon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Upstream issue and PR titles are attacker-authorable text, and the audit
asks a model to judge them for similarity. They were landing in context
raw, which is exactly the ingress the v1.66.1.0 trust envelope exists
for. test/tracker-guard-wiring.test.ts flagged both gh issue list reads.
- Step 3's four fetches now tee raw JSON to a mktemp -d scratch dir as
mechanical scorer input and pipe the human-readable line through
bin/gstack-issue-guard. The scanner does not match gh pr list, but
those are the same class of ingress, so all four are enveloped.
- Documents the envelope as a health signal: "(empty body)" means zero
matches, while no envelope at all means the pipeline failed and must
not clear a commit.
- Step 7 now names the report path and its shape, {summary, worst,
commits[]}, with worst as the highest-severity bucket across all
commits. That is the field /ship's Step 1.5 gate branches on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 1.5 shelled out to ~/.claude/skills/gstack/bin/gstack-skill, a binary that has never existed in this tree or upstream, so the gate would have failed at runtime on every ship. test/setup-claude-skill-assets.test.ts was failing on that dangling reference. Replaced with the real invocation: a shell precondition probe, an inline read-and-follow of pr-prep/SKILL.md, then a shell gate over the JSON report. - The probe prints PR_PREP_GATE: run|skip and clears any stale report first, so a leftover from an earlier ship cannot decide this one. - The gate reads .worst via jq, defaulting to CLEAN. A missing report warns to stderr and continues rather than hard-failing ship on an audit that never ran. - GSTACK_FROM_SHIP becomes a behavioural contract (suppress pr-prep's own confirmations, ship owns the decision) instead of an env var handed to a process that never ran. - Step 19's PR-body read of the report is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ">=3 OPEN issues" clause in the Step 4 bucketing had no score floor, so it counted raw gh full-text hits. Dogfooding the audit over this branch's 24 non-merge commits, three chore(build) commits bucketed OVERLAP off a topScore of 0.05: keywords like "regenerate skill merge" pull three unrelated open issues on any repo with a busy tracker, so those commits would have read OVERLAP forever. It never false-blocked, since OVERLAP is informational and only EXACT_DUP refuses, but a bucket that always fires stops being read. Add RELATED_OPEN_ISSUE_FLOOR = 0.15 and count only open issues that clear it. The floor sits below the single-hit OVERLAP threshold (0.3) so a genuinely crowded topic still trips the clause when no single issue is a strong match, which is the case the clause exists for. An open issue carries state weight 0.7, so 0.15 requires title Jaccard >=0.214, a real shared-token overlap rather than one incidental word. openIssueCount keeps its old meaning and a new relatedOpenIssueCount carries what the clause counted, so the JSON output stays additive for consumers. Reason strings now name the floor. The existing unit test asserted the old behaviour with three issues at Jaccard 0, encoding the noise as signal. Retarget it at genuinely related issues, each scoring 0.2, and add a regression case built from the four real issue titles that caused the false positive. Same dogfood pass after the fix: 4 OVERLAP to 1, and the survivor trips the legitimate single-hit threshold rather than the count clause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerated with `bun run gen:skill-docs` after rebasing onto upstream/main (v1.87.4.0). Replaces four stale `chore(build)` checkpoint commits that carried generated output from merges of older upstream snapshots; those conflicted wholesale against the current base and were dropped rather than hand-merged. Also dropped: the commit registering /pr-prep in scripts/proactive-suggestions.json, since upstream retired that registry in the v1.64 waves and deleted the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The branch carried two ceiling commits pinned against an older upstream. Neither side's numbers survive a rebase onto v1.87.4.0, so both were dropped and every ceiling is re-pinned from an actual `bun test test/parity-suite.test.ts` run against this tree. Attribution checked rather than assumed: every skill's upstream/main size sits under its existing ceiling (cso 17,986 < 18,000; review 60,940 < 61,500; ship 77,680 < 79,300), so the overage is the fork's own — +805B of single-select AskUserQuestion doctrine in each affected preamble, and +3,421B in ship for that plus the Step 1.5 pr-prep gate. Each new ceiling records its measured figure. Result: 21/21 skills pass parity vs the v1.64.1.0 baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both ratchets failed on the rebased branch, and both for the reason their protocol names as legitimate growth: this branch adds a skill and a shared preamble paragraph. catalog-budget: 1,161 token-equivalents against a 1,150 ceiling. Upstream sat one token under it at 53 skills; /pr-prep is the 54th. Ceiling raised to 1,200 with the derivation re-measured in place, keeping roughly the ~4% headroom the previous derivation kept. context-budget: re-captured with `bun test/helpers/capture-context-budget.ts` as the ratchet protocol prescribes. alwaysOnTotal was 6,453 against 6,397; health's eager ledger 10,891 against 10,805; pr-prep had no eager ceiling. Every eager ceiling moves because the single-select AskUserQuestion paragraph lands in each skill's shared preamble (about +202 tokens where the full preamble is carried, +8 on small skills). Ceilings are the tool's own actual x1.05 / x1.10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7d3c026 to
47087b1
Compare
Summary
New
/pr-prepskill: a pre-PR upstream duplicate audit. It walksgit log base..HEAD, derives search keywords per commit from the subject + changed filepaths, queries the upstream repo's open issues and PRs via
gh, and scores eachcommit against open work (EXACT_DUP / OVERLAP / SIBLING / CLEAN). EXACT_DUP
refuses to proceed.
/shipruns it as a Step 1.5 gate before any merge, test,or version bump.
Why it helps you, the maintainer: it kills duplicate community PRs before
they're filed — the triage you do most. (Dogfood result below.)
What it does
CONTRIBUTING.md: pre-push gates, testlayout, branch/commit conventions, banned patterns. Used inline when writing
the PR body.
bucketing is implemented in
bin/gstack-pr-prep-score(pure, unit-tested) andreferenced by the skill as the canonical scorer.
codexsecond opinion on CLEAN commits (different modelfamily for independent signal; soft-skips if
codexis absent).convention (sampled from its
CONTRIBUTING.md+git log), not thecontributor's personal house style.
GSTACK_FROM_SHIP=1; EXACT_DUP abortsship with resolution paths; skips cleanly on solo repos and
--skip-pr-prep.Dogfooded on itself
Ran
/pr-prepon this branch againstgarrytan/gstack@main: CLEAN — 13/13commits, no EXACT_DUP, no OVERLAP, in ~30-60s of
ghqueries. No existingpr-prep / duplicate-audit skill upstream; the open
shipPRs all touch unrelatedparts of the flow. Full audit is in a comment on this PR.
Tests + release
test/pr-prep-score.test.ts— 13 deterministic cases pinning the scorer'sbucket thresholds (gate-tier, free).
AGENTS.md,docs/skills.md,scripts/proactive-suggestions.json,and
test/skill-coverage-matrix.ts.ALLOWED_SUBSTEPSupdated for the new Step 1.5 sub-step.
VERSION/package.jsonsynced.bun testis green except environment-specific cases (local Swift toolchain,gbrain config, local git state) that also fail on a clean
main.secrets, so those will error regardless of code; happy to push the branch to a
base-repo branch and re-target if you'd prefer green eval CI.
Motivation
Real case (2026-05-26): a contributor branch on
garrytan/gbrainhad 8 commitsready for upstream PRs. Without pr-prep, 4 of 8 unverified commits would have been
duplicates of already-open work:
e96332c5(reindex CLI_ONLY one-char fix) → setup: add GSTACK_SKIP_PLAYWRIGHT to skip Chromium install #913, open 14 days, same fix74819cec(sourceId fallback) → fix: make bin root detection CDPATH-safe #836, open787da2af+829099f9(synopsis env-override) → setup --host cursor errors out despite README + hosts/cursor.ts declaring full Cursor support #1358, open, same patterne0133d8a(LM Studio recipe + Ollama dims_options) → fix: normalize line endings in template reads for cross-platform generator #1051 + Multiple Claude Code security hook triggers in/codexand/autoplanskill templates #1329, crowded2db6b6d4(--lock-durationflag) → plan-eng-review: add throughput planning readout #1014 issue + Fix gstack build when Conductor creates a repo with unborn HEAD #1177 PREach would have cost a triage round and a close. pr-prep catches all of them in
one
ghquery pass before anything is filed.