From b0072107a10672403f5fd520632eda235938cc3c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Apr 2026 03:25:54 +0000 Subject: [PATCH 1/6] ghcp-handoff: require heading match for PR body section check checkPrBody previously did a case-insensitive substring search, so a sentence like "this PR includes a Summary of changes" counted as the required Summary section. Match Markdown headings and bold labels only (e.g. ## Summary, **Summary:**, **Summary**:), and add tests for the prose-only and bold-label cases. --- ghcp-handoff/bin/ghcp-verify-pr-body.ts | 29 +++++++++++++---- ghcp-handoff/test/ghcp-verify-pr-body.test.ts | 32 +++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/ghcp-handoff/bin/ghcp-verify-pr-body.ts b/ghcp-handoff/bin/ghcp-verify-pr-body.ts index da1c0686f3..810fd4124f 100644 --- a/ghcp-handoff/bin/ghcp-verify-pr-body.ts +++ b/ghcp-handoff/bin/ghcp-verify-pr-body.ts @@ -26,22 +26,37 @@ export interface PrBodyResult { /** * Check the PR body for required sections. - * Searches for each section name as a case-insensitive substring. + * + * A section counts as present only when its name appears as a Markdown + * heading (`#`, `##`, …) or as a bolded label at the start of a line + * (`**Summary**` or `**Summary:**`). Matching is case-insensitive. Plain + * prose mentions like "this PR includes a summary of changes" do NOT count. */ export function checkPrBody( prBody: string, requiredSections: string[] ): PrBodyResult { - const bodyLower = prBody.toLowerCase(); + const lines = prBody.split("\n"); const missing: string[] = []; const present: string[] = []; for (const section of requiredSections) { - if (bodyLower.includes(section.toLowerCase())) { - present.push(section); - } else { - missing.push(section); - } + const sectionLower = section.toLowerCase(); + const found = lines.some((line) => { + const trimmed = line.trim().toLowerCase(); + // Markdown heading: ^#{1,6}\s+
[optional trailing punctuation]$ + const headingMatch = trimmed.match(/^#{1,6}\s+(.+?)[:.\s]*$/); + if (headingMatch && headingMatch[1].trim() === sectionLower) return true; + // Bold label: ^\*\*
[:]?\*\*[:]? (e.g. **Summary**, **Summary:**, **Summary**:) + const boldMatch = trimmed.match(/^\*\*(.+?)\*\*/); + if (boldMatch) { + const label = boldMatch[1].replace(/[:.\s]+$/, "").trim(); + if (label === sectionLower) return true; + } + return false; + }); + if (found) present.push(section); + else missing.push(section); } return { diff --git a/ghcp-handoff/test/ghcp-verify-pr-body.test.ts b/ghcp-handoff/test/ghcp-verify-pr-body.test.ts index e385a259e1..9f2b845480 100644 --- a/ghcp-handoff/test/ghcp-verify-pr-body.test.ts +++ b/ghcp-handoff/test/ghcp-verify-pr-body.test.ts @@ -74,4 +74,36 @@ Deviations from plan documented below, and Flagged uncertainties at the end.`; expect(result.missingSections).toEqual(requiredSections); expect(result.presentSections).toHaveLength(0); }); + + test("bold-label sections (e.g. **Summary**:) count as present", () => { + const body = `**Summary:** Shipped the scaffold. + +**Manual followups:** Hook up CI secrets. + +**Deviations from plan:** None. + +**Flagged uncertainties** +Unsure about vitest config.`; + + const result = checkPrBody(body, requiredSections); + expect(result.missingSections).toHaveLength(0); + expect(result.presentSections).toHaveLength(4); + }); + + test("headings with trailing punctuation still match", () => { + const body = `### Summary: +Done. + +### Manual followups. +None. + +### Deviations from plan +None. + +### Flagged uncertainties +None.`; + + const result = checkPrBody(body, requiredSections); + expect(result.missingSections).toHaveLength(0); + }); }); From 12a483f5fc6a8cd780d2d494367e983ee3e2e4f5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Apr 2026 03:26:08 +0000 Subject: [PATCH 2/6] ghcp-handoff: parse manifests structurally instead of regex-matching diffs The previous dep-check regex-matched + lines in unified diffs, so any top-level package.json key (author, repository, engines, keywords, etc.) and any TOML key in any pyproject.toml / Cargo.toml section got flagged as a new dependency. Fetch before/after file contents via git show, then parse them structurally: - package.json: JSON.parse both sides, diff keys across dependencies, devDependencies, peerDependencies, and optionalDependencies. - pyproject.toml / Cargo.toml: walk lines tracking the active [section] header, collect keys only when the section matches a dep-section regex (tool.poetry.*dependencies, dependencies, dev-dependencies, build-dependencies, target.*.dependencies, etc.). - go.mod: match require-block module lines. - Gemfile: match gem "name" entries. Replaces extractAddedDeps with parseManifestDeps + diffManifestDeps. Tests cover the metadata-key false-positive case and each supported manifest. --- ghcp-handoff/bin/ghcp-verify-boundaries.ts | 160 +++++++++++++----- .../test/ghcp-verify-boundaries.test.ts | 159 +++++++++++++++-- 2 files changed, 258 insertions(+), 61 deletions(-) diff --git a/ghcp-handoff/bin/ghcp-verify-boundaries.ts b/ghcp-handoff/bin/ghcp-verify-boundaries.ts index 311d48cf3b..25324a670e 100644 --- a/ghcp-handoff/bin/ghcp-verify-boundaries.ts +++ b/ghcp-handoff/bin/ghcp-verify-boundaries.ts @@ -107,55 +107,115 @@ export function checkBoundaries( } /** - * Simple dep manifest diff parser. - * Returns list of added dependency names from a unified diff of a manifest file. + * Dep-section names for each supported manifest. Keys outside these + * sections (e.g. `name`, `version`, `author`, `repository` in package.json; + * `[tool.poetry]` metadata in pyproject.toml) must NOT be flagged as deps. */ -export function extractAddedDeps( - diff: string, - manifestName: string -): string[] { - const added: string[] = []; - - // package.json: lines like + "foo": "^1.0.0" - if (manifestName === "package.json") { - const matches = diff.matchAll(/^\+\s+"([^"]+)":\s*"/gm); - for (const m of matches) { - // Skip metadata keys - if ( - !["name", "version", "private", "scripts", "description"].includes( - m[1] - ) - ) { - added.push(m[1]); +const PACKAGE_JSON_DEP_SECTIONS = [ + "dependencies", + "devDependencies", + "peerDependencies", + "optionalDependencies", +] as const; + +const CARGO_DEP_SECTION_RE = /^(?:dependencies|dev-dependencies|build-dependencies|target\..+?\.dependencies)$/; +const PYPROJECT_DEP_SECTION_RE = /^(?:dependencies|tool\.poetry\.dependencies|tool\.poetry\.dev-dependencies|tool\.poetry\.group\..+?\.dependencies)$/; + +function parsePackageJsonDeps(content: string): Set { + const deps = new Set(); + try { + const parsed = JSON.parse(content); + for (const section of PACKAGE_JSON_DEP_SECTIONS) { + const block = parsed?.[section]; + if (block && typeof block === "object") { + for (const name of Object.keys(block)) deps.add(name); } } + } catch { + // malformed JSON — nothing we can flag with confidence } + return deps; +} - // pyproject.toml: lines like +foo = ">=1.0" - if (manifestName === "pyproject.toml") { - const matches = diff.matchAll(/^\+([a-zA-Z0-9_-]+)\s*=/gm); - for (const m of matches) added.push(m[1]); +/** + * Walk TOML-ish content line by line tracking the active `[section]` header + * and collect `key = ...` entries only when the section matches a dep regex. + * This is intentionally minimal (no full TOML parse) — good enough to catch + * the common case without a new dependency. + */ +function parseTomlDeps(content: string, sectionRe: RegExp): Set { + const deps = new Set(); + let section = ""; + for (const raw of content.split("\n")) { + const line = raw.trim(); + if (!line || line.startsWith("#")) continue; + const header = line.match(/^\[(.+?)\]$/); + if (header) { + section = header[1].trim(); + continue; + } + if (!sectionRe.test(section)) continue; + const kv = line.match(/^([A-Za-z0-9_-]+)\s*=/); + if (kv) deps.add(kv[1]); } + return deps; +} - // go.mod: lines like +\trequire foo v1.0.0 - if (manifestName === "go.mod") { - const matches = diff.matchAll(/^\+\t([^\s]+)\s+v/gm); - for (const m of matches) added.push(m[1]); +function parseGoModDeps(content: string): Set { + const deps = new Set(); + // Matches both `require module v...` (single) and lines inside `require ( ... )` blocks. + for (const raw of content.split("\n")) { + const line = raw.trim(); + const m = line.match(/^(?:require\s+)?([^\s()]+)\s+v\d/); + if (m && m[1] !== "require") deps.add(m[1]); } + return deps; +} - // Cargo.toml: like package.json pattern - if (manifestName === "Cargo.toml") { - const matches = diff.matchAll(/^\+([a-zA-Z0-9_-]+)\s*=/gm); - for (const m of matches) added.push(m[1]); +function parseGemfileDeps(content: string): Set { + const deps = new Set(); + for (const m of content.matchAll(/^\s*gem\s+['"]([^'"]+)['"]/gm)) { + deps.add(m[1]); } + return deps; +} - // Gemfile: lines like +gem "foo" - if (manifestName === "Gemfile") { - const matches = diff.matchAll(/^\+gem\s+"([^"]+)"/gm); - for (const m of matches) added.push(m[1]); +/** + * Parse a manifest's current dependency names. Returns an empty set for + * unsupported manifests (callers should gate on the supported list). + */ +export function parseManifestDeps( + content: string, + manifestName: string +): Set { + switch (manifestName) { + case "package.json": + return parsePackageJsonDeps(content); + case "pyproject.toml": + return parseTomlDeps(content, PYPROJECT_DEP_SECTION_RE); + case "Cargo.toml": + return parseTomlDeps(content, CARGO_DEP_SECTION_RE); + case "go.mod": + return parseGoModDeps(content); + case "Gemfile": + return parseGemfileDeps(content); + default: + return new Set(); } +} - return added; +/** + * Return dependency names present in `after` but not in `before`. + * `before` may be empty string (file added on this branch). + */ +export function diffManifestDeps( + before: string, + after: string, + manifestName: string +): string[] { + const beforeDeps = parseManifestDeps(before, manifestName); + const afterDeps = parseManifestDeps(after, manifestName); + return [...afterDeps].filter((d) => !beforeDeps.has(d)); } /** @@ -235,19 +295,25 @@ if (import.meta.main) { const allAddedDeps: { name: string; manifest: string }[] = []; for (const manifest of manifests) { - if (changedFiles.includes(manifest)) { - const mDiff = Bun.spawnSync([ + if (!changedFiles.includes(manifest)) continue; + + const fetchAt = (ref: string): string => { + const proc = Bun.spawnSync([ "git", - "diff", - `${baseBranch}...${prBranch}`, - "--", - manifest, + "show", + `${ref}:${manifest}`, ]); - const diffText = new TextDecoder().decode(mDiff.stdout); - const added = extractAddedDeps(diffText, manifest); - for (const name of added) { - allAddedDeps.push({ name, manifest }); - } + // Exit code is non-zero when the file did not exist at that ref. + return proc.exitCode === 0 + ? new TextDecoder().decode(proc.stdout) + : ""; + }; + + const before = fetchAt(baseBranch); + const after = fetchAt(prBranch); + const added = diffManifestDeps(before, after, manifest); + for (const name of added) { + allAddedDeps.push({ name, manifest }); } } diff --git a/ghcp-handoff/test/ghcp-verify-boundaries.test.ts b/ghcp-handoff/test/ghcp-verify-boundaries.test.ts index f9f01aa317..44b249f955 100644 --- a/ghcp-handoff/test/ghcp-verify-boundaries.test.ts +++ b/ghcp-handoff/test/ghcp-verify-boundaries.test.ts @@ -3,7 +3,8 @@ import path from "node:path"; import { parseMetadata, checkBoundaries, - extractAddedDeps, + diffManifestDeps, + parseManifestDeps, checkDeps, type Metadata, } from "../bin/ghcp-verify-boundaries"; @@ -119,23 +120,153 @@ describe("checkBoundaries", () => { }); }); -describe("extractAddedDeps", () => { - test("extracts added deps from package.json diff", () => { - const diff = ` -+ "minimatch": "^10.0.0", -+ "yaml": "^2.7.0", -+ "name": "my-project", -`; - const added = extractAddedDeps(diff, "package.json"); +describe("diffManifestDeps — package.json", () => { + const before = JSON.stringify({ + name: "my-project", + version: "1.0.0", + dependencies: { foo: "^1.0.0" }, + devDependencies: { bar: "^2.0.0" }, + }); + + test("flags newly added deps in dependencies + devDependencies", () => { + const after = JSON.stringify({ + name: "my-project", + version: "1.0.0", + dependencies: { foo: "^1.0.0", minimatch: "^10.0.0" }, + devDependencies: { bar: "^2.0.0", yaml: "^2.7.0" }, + }); + const added = diffManifestDeps(before, after, "package.json"); expect(added).toContain("minimatch"); expect(added).toContain("yaml"); - expect(added).not.toContain("name"); + expect(added).toHaveLength(2); + }); + + test("ignores top-level metadata keys like author, repository, keywords", () => { + const after = JSON.stringify({ + name: "my-project", + version: "2.0.0", + author: "New Author", + repository: { type: "git", url: "..." }, + keywords: ["a", "b"], + engines: { node: ">=20" }, + dependencies: { foo: "^1.0.0" }, + devDependencies: { bar: "^2.0.0" }, + }); + const added = diffManifestDeps(before, after, "package.json"); + expect(added).toHaveLength(0); + }); + + test("handles added manifest (before = empty string)", () => { + const after = JSON.stringify({ + name: "new-project", + dependencies: { lodash: "^4.0.0" }, + }); + const added = diffManifestDeps("", after, "package.json"); + expect(added).toEqual(["lodash"]); + }); + + test("flags peerDependencies + optionalDependencies", () => { + const after = JSON.stringify({ + dependencies: { foo: "^1.0.0" }, + devDependencies: { bar: "^2.0.0" }, + peerDependencies: { react: "^19.0.0" }, + optionalDependencies: { fsevents: "^2.0.0" }, + }); + const added = diffManifestDeps(before, after, "package.json"); + expect(added).toContain("react"); + expect(added).toContain("fsevents"); + }); +}); + +describe("parseManifestDeps — pyproject.toml section awareness", () => { + test("only collects keys inside dep sections", () => { + const content = ` +[project] +name = "my-pkg" +version = "1.0.0" +description = "not a dep" + +[tool.poetry] +name = "my-pkg" +description = "also not a dep" + +[tool.poetry.dependencies] +python = "^3.11" +requests = "^2.31" + +[tool.poetry.dev-dependencies] +pytest = "^8.0" +`; + const deps = parseManifestDeps(content, "pyproject.toml"); + expect(deps.has("requests")).toBe(true); + expect(deps.has("pytest")).toBe(true); + expect(deps.has("python")).toBe(true); + // Metadata keys outside dep sections must NOT be collected. + expect(deps.has("name")).toBe(false); + expect(deps.has("version")).toBe(false); + expect(deps.has("description")).toBe(false); }); +}); + +describe("parseManifestDeps — Cargo.toml section awareness", () => { + test("only collects keys inside dep sections", () => { + const content = ` +[package] +name = "my-crate" +version = "0.1.0" +edition = "2021" + +[dependencies] +serde = "1.0" +tokio = { version = "1", features = ["full"] } + +[dev-dependencies] +criterion = "0.5" + +[package.metadata.docs.rs] +all-features = true +`; + const deps = parseManifestDeps(content, "Cargo.toml"); + expect(deps.has("serde")).toBe(true); + expect(deps.has("tokio")).toBe(true); + expect(deps.has("criterion")).toBe(true); + expect(deps.has("name")).toBe(false); + expect(deps.has("edition")).toBe(false); + expect(deps.has("all-features")).toBe(false); + }); +}); - test("extracts from Gemfile diff", () => { - const diff = `+gem "rails"\n+gem "puma"`; - const added = extractAddedDeps(diff, "Gemfile"); - expect(added).toEqual(["rails", "puma"]); +describe("parseManifestDeps — go.mod", () => { + test("extracts require block modules", () => { + const content = `module example.com/foo + +go 1.22 + +require ( +\tgithub.com/spf13/cobra v1.8.0 +\tgithub.com/stretchr/testify v1.9.0 +) + +require github.com/single/pkg v0.1.0 +`; + const deps = parseManifestDeps(content, "go.mod"); + expect(deps.has("github.com/spf13/cobra")).toBe(true); + expect(deps.has("github.com/stretchr/testify")).toBe(true); + expect(deps.has("github.com/single/pkg")).toBe(true); + }); +}); + +describe("parseManifestDeps — Gemfile", () => { + test("extracts gem entries", () => { + const content = `source "https://rubygems.org" +gem "rails", "~> 7.0" +gem "puma" +gem 'sidekiq' +`; + const deps = parseManifestDeps(content, "Gemfile"); + expect(deps.has("rails")).toBe(true); + expect(deps.has("puma")).toBe(true); + expect(deps.has("sidekiq")).toBe(true); }); }); From 951126268834fa8b31267b1c4829f414841045e3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Apr 2026 03:26:15 +0000 Subject: [PATCH 3/6] ghcp-handoff: drop misleading gh copilot fallback claim in detect script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment promised a "gh copilot" fallback that was never implemented. It would have been wrong anyway — the gh extension doesn't support the -p --allow-all-tools --output-format json --no-ask-user flags the handoff invocation requires. Make the comment match reality: only the standalone copilot CLI counts as available. --- ghcp-handoff/bin/ghcp-detect.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ghcp-handoff/bin/ghcp-detect.sh b/ghcp-handoff/bin/ghcp-detect.sh index 7c70bc60dc..2625317ed7 100644 --- a/ghcp-handoff/bin/ghcp-detect.sh +++ b/ghcp-handoff/bin/ghcp-detect.sh @@ -1,15 +1,18 @@ #!/usr/bin/env bash -# ghcp-detect.sh — Detect copilot CLI binary and print version. -# Exit 0 with version on stdout if found; exit 1 with empty stdout if not. +# ghcp-detect.sh — Detect the standalone GitHub Copilot CLI (`copilot`) and +# print its version. Exits 0 with version on stdout if found; exit 1 with +# empty stdout if not. +# +# Note: the `gh copilot` extension is intentionally NOT a fallback. The +# handoff workflow invokes `copilot -p ... --allow-all-tools --output-format +# json --no-ask-user`, flags that only the standalone CLI supports. set -euo pipefail -# Try 'copilot' first, then 'gh copilot' as fallback if command -v copilot &>/dev/null; then VERSION=$(copilot --version 2>/dev/null | head -1 || echo "unknown") echo "$VERSION" exit 0 fi -# Not found exit 1 From 8a35981aa1827ad83e57bdd71e42f15dfa2e7037 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Apr 2026 03:26:27 +0000 Subject: [PATCH 4/6] ghcp-handoff: drop unused template placeholders, document the rendered set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit prompt-template.md referenced {{project_name}}, {{repo_abs_path}}, {{user_name}}, and {{forbidden_branches}} — none of which Step 2B's guided-mode question flow collected. On render they would have been left as literal text in the prompt sent to Copilot. Replace them with concrete wording that matches the sample fixture ("the repo root", "the reviewer", explicit forbidden-branch list), and update the Render step in SKILL.md.tmpl to enumerate the actual placeholder set plus a pre-flight grep for any remaining {{...}} markers. --- ghcp-handoff/SKILL.md.tmpl | 20 ++++++++++++++++---- ghcp-handoff/templates/prompt-template.md | 8 ++++---- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/ghcp-handoff/SKILL.md.tmpl b/ghcp-handoff/SKILL.md.tmpl index 156e299bbc..e7f69ad4af 100644 --- a/ghcp-handoff/SKILL.md.tmpl +++ b/ghcp-handoff/SKILL.md.tmpl @@ -182,10 +182,22 @@ AskUserQuestion: "PR description sections — accept defaults or customize?" ### Render After all answers: 1. Read `${CLAUDE_SKILL_DIR}/templates/prompt-template.md`. -2. Fill all double-brace-style template variables (e.g. `task_summary`, `target_branch`, `stream_blocks`) from collected answers. -3. Write to `.gstack/ghcp-handoff/.md` + `.meta.json`. -4. Show the rendered prompt in a fenced block. -5. Proceed to **Step 2-exec**. +2. Substitute every double-brace placeholder in that file from the collected + answers. The full placeholder set is: + - `task_summary` — answer to Q1 + - `phase_note` + `stub_policy` — answers to Q4 + - `target_branch` — answer to Q2 + - `context_files_bullets` — rendered as `- ` per line from Q3 + - `numbered_boundaries` — one numbered line per entry from Q6 + - `stream_blocks` — each stream rendered as the `STREAM N — ` block (Q5) + - `not_in_scope_paths` / `not_in_scope_reasons` — YAML list items from Q7, + each prefixed with `- ` and indented 4 spaces to sit under the `paths:` / `reasons:` keys + - `forbidden_deps_list` — YAML list items from Q8 indented 2 spaces (or `[]` if none) +3. Before writing, grep the rendered output for any remaining double-brace + placeholders — if found, stop and tell the user which placeholder failed to substitute. +4. Write to `.gstack/ghcp-handoff/.md` + `.meta.json`. +5. Show the rendered prompt in a fenced block. +6. Proceed to **Step 2-exec**. --- diff --git a/ghcp-handoff/templates/prompt-template.md b/ghcp-handoff/templates/prompt-template.md index 0d2c6a05b1..689e58fc49 100644 --- a/ghcp-handoff/templates/prompt-template.md +++ b/ghcp-handoff/templates/prompt-template.md @@ -1,11 +1,11 @@ -You are implementing {{task_summary}} for {{project_name}}. {{phase_note}}. +You are implementing {{task_summary}}. {{phase_note}}. DO NOT implement anything beyond {{stub_policy}}. IMPORTANT: Work in a clean git worktree on the branch `{{target_branch}}`. Create the branch from the repo's default branch if it doesn't exist. Do not work directly in the main worktree — other agents may be active there. -Context files to read first (in the repo at {{repo_abs_path}}): +Context files to read first (in the repo root): {{context_files_bullets}} Hard boundaries (IF YOU CROSS THESE, THE PR WILL BE REJECTED): @@ -17,12 +17,12 @@ What to deliver in one PR against branch `{{target_branch}}`: PR description must include: - Summary of what shipped. -- Manual followups required from {{user_name}}. +- Manual followups required from the reviewer. - Any dependency decisions that deviated from the plan. - Any interface signatures you were unsure about (flag them for review). Open the PR against `{{target_branch}}` (create the branch if it doesn't exist). -DO NOT merge to {{forbidden_branches}}. +DO NOT merge to main, master, dev, develop, production, or staging.