diff --git a/.changeset/fix-pr-branch-sub-repos-git-c.md b/.changeset/fix-pr-branch-sub-repos-git-c.md new file mode 100644 index 000000000..dfb4ed34a --- /dev/null +++ b/.changeset/fix-pr-branch-sub-repos-git-c.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 667 +--- + +**`/gsd:pr-branch` now handles sub-repos defined in config** — when `planning.sub_repos` is set, the command scans each sub-repo for uncommitted changes and offers to create a branch, commit, push, and open a companion PR per sub-repo. Previously, sub-repos were silently ignored because all git commands ran against the shell's current directory instead of the intended repo path. All sub-repo git operations now use `git -C ` so no shell-state assumptions are made. diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 6475d4a58..cdc82aa12 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -631,7 +631,7 @@ async function main() { // discovery; previously it was a partial subset that didn't include // phase / roadmap / milestone / progress / etc. const TOP_LEVEL_USAGE = 'Usage: gsd-tools [args] [--raw] [--pick ] [--cwd ] [--ws ] [--json-errors]\n' + - 'Commands: agent, agent-skills, audit-open, audit-uat, check, check-commit, commit, commit-to-subrepo, ' + + 'Commands: agent, agent-skills, audit-open, audit-uat, check, check-commit, commit, commit-to-subrepo, pr-subrepo, ' + 'config-ensure-section, config-get, config-new-project, config-path, config-set, migrate-config, ' + 'current-timestamp, detect-custom-files, docs-init, drift-guard, effort, extract-messages, find-phase, ' + 'from-gsd2, frontmatter, gap-analysis, generate-claude-md, generate-claude-profile, ' + @@ -959,6 +959,13 @@ async function runCommand(command, args, cwd, raw, defaultValue, originalCommand break; } + case 'pr-subrepo': { + const message = args[1]; + const { repo, branch } = parseNamedArgs(args, ['repo', 'branch']); + commands.cmdPrSubrepo(cwd, repo, branch, message, raw); + break; + } + case 'verify-summary': { const summaryPath = args[1]; const countIndex = args.indexOf('--check-count'); diff --git a/gsd-core/workflows/pr-branch.md b/gsd-core/workflows/pr-branch.md index 443ebd770..698e69e64 100644 --- a/gsd-core/workflows/pr-branch.md +++ b/gsd-core/workflows/pr-branch.md @@ -43,6 +43,162 @@ Commits: {AHEAD} ahead ``` + +Read the sub-repo list from config using the canonical key path — `planning.sub_repos`. +A non-zero exit code means the key is absent; treat that as "no sub-repos configured". + +```bash +SUB_REPOS_JSON=$(gsd_run query config-get planning.sub_repos 2>/dev/null) +if [ $? -ne 0 ] || [ -z "$SUB_REPOS_JSON" ] || [ "$SUB_REPOS_JSON" = "null" ] || [ "$SUB_REPOS_JSON" = "[]" ]; then + : # Not configured or empty — skip to analyze_commits +fi +``` + +Scan each sub-repo for uncommitted changes using node (always available — avoids undeclared +jq dependency). Write dirty repo names to a temp file so the list survives across +subsequent command executions: + +```bash +ROOT=$(git rev-parse --show-toplevel) +DIRTY_FILE=$(mktemp) + +node -e " + const repos = JSON.parse(process.argv[1]); + const { execFileSync } = require('child_process'); + const path = require('path'); + const fs = require('fs'); + const root = process.argv[2]; + // realpath parity with the pr-subrepo seam's validatePath: resolve $ROOT through + // symlinks once so the containment check below compares real paths, not text. + let realRoot; + try { realRoot = fs.realpathSync(root); } catch (_) { realRoot = path.resolve(root); } + const out = []; + for (const r of repos) { + // Reject before any git invocation: this scan runs on raw config values, + // ahead of the pr-subrepo seam's own validatePath guard. A traversal, + // embedded-newline, or symlink entry here would run git outside the + // workspace, or inject a spurious record into the dirty-file output. + if (typeof r !== 'string' || !/^[A-Za-z0-9._\/-]+$/.test(r)) continue; + // realpathSync follows symlinks — path.resolve only normalizes '..' textually, + // so an in-tree symlink pointing outside root would otherwise smuggle git out. + let resolved; + try { resolved = fs.realpathSync(path.resolve(realRoot, r)); } catch (_) { continue; } + if (resolved !== realRoot && !resolved.startsWith(realRoot + path.sep)) continue; + try { + const res = execFileSync('git', ['-C', resolved, 'status', '--porcelain'], + { encoding: 'utf8', timeout: 10_000 }); + // Exclude untracked-only repos: seam filters ?? lines, so detection must match. + const tracked = res.split('\n').filter(l => l.length > 0 && !l.startsWith('??')); + if (tracked.length > 0) out.push(r); + } catch (_) {} + } + fs.writeFileSync(process.argv[3], out.join('\n')); +" "$SUB_REPOS_JSON" "$ROOT" "$DIRTY_FILE" + +DIRTY_REPOS=$(cat "$DIRTY_FILE") +``` + +If `$DIRTY_REPOS` is empty, remove the temp file and continue to `analyze_commits`. + +Display dirty repos and prompt the user: + +``` +Sub-repos with uncommitted changes: + backend + frontend + +How should sub-repo changes be handled? + 1. all — branch, commit (explicit files only), push -u, open companion PR per repo + 2. select — choose which sub-repos to process + 3. skip — ignore sub-repos, continue with root repo only +``` + +If the user chooses **skip**, remove the temp file and continue to `analyze_commits`. + +For each selected sub-repo `$REPO_REL`, delegate all git work to the `pr-subrepo` query +seam — it stages explicit changed files (never `git add -A`), creates the branch, +commits, and pushes with `--set-upstream`. Branch names include the repo slug to avoid +colliding with the root `PR_BRANCH` that `create_pr_branch` creates later: + +```bash +# Replace path separators to make the name safe as a branch component +REPO_SAFE="${REPO_REL//\//-}" +SUB_BRANCH="${CURRENT_BRANCH}-${REPO_SAFE}-pr" +COMMIT_MSG="fix(${REPO_REL}): sync uncommitted changes for PR" + +RESULT=$(gsd_run query pr-subrepo "$COMMIT_MSG" \ + --repo "$REPO_REL" \ + --branch "$SUB_BRANCH") +SUBREPO_EXIT=$? +``` + +If the seam exited non-zero (stage/commit/push failure), report its error and move on to +the next selected sub-repo. **Do not run the companion-PR step below for this repo** — +the seam's stderr already explains the failure, and the "branch pushed" path would +otherwise contradict it: + +```bash +if [ "$SUBREPO_EXIT" -ne 0 ]; then + echo "pr-subrepo failed for $REPO_REL — see error above; skipping companion PR." >&2 +fi +``` + +Only when `$SUBREPO_EXIT` is `0`, parse the structured result with node and open the +companion PR. If `remote_slug` is null (non-GitHub remote), skip `gh pr create` and show +the push URL instead: + +```bash +REMOTE_SLUG=$(node -e " + try { console.log(JSON.parse(process.argv[1]).remote_slug || ''); } catch(_) {} +" "$RESULT") + +if [ -n "$REMOTE_SLUG" ]; then + # Defense-in-depth: $REPO_REL was already validated by the dirty-scan filter and + # the pr-subrepo seam's validatePath, but these are separate, independent git -C + # invocations on the same value. Resolve it through symlinks with the SAME realpath + # containment the seam uses (path.resolve alone would not catch a symlink escape), + # and run git against the validated absolute path rather than re-concatenating. + SUB_REPO_DIR=$(node -e " + const fs = require('fs'), path = require('path'); + try { + const realRoot = fs.realpathSync(process.argv[1]); + const resolved = fs.realpathSync(path.resolve(realRoot, process.argv[2])); + if (resolved !== realRoot && !resolved.startsWith(realRoot + path.sep)) process.exit(1); + process.stdout.write(resolved); + } catch (_) { process.exit(1); } + " "$ROOT" "$REPO_REL" 2>/dev/null) + + if [ -z "$SUB_REPO_DIR" ]; then + echo "Refusing unsafe sub-repo path: $REPO_REL" >&2 + SUB_TARGET="$TARGET" + else + # Resolve base branch: use $TARGET if it exists in sub-repo, else fall back to + # the sub-repo's own default branch + if git -C "$SUB_REPO_DIR" ls-remote --exit-code --heads origin "$TARGET" \ + > /dev/null 2>&1; then + SUB_TARGET="$TARGET" + else + SUB_TARGET=$(git -C "$SUB_REPO_DIR" remote show origin 2>/dev/null \ + | awk '/HEAD branch/ {print $NF}') + SUB_TARGET="${SUB_TARGET:-main}" + fi + fi + + gh pr create \ + --repo "$REMOTE_SLUG" \ + --base "$SUB_TARGET" \ + --head "$SUB_BRANCH" \ + --title "$COMMIT_MSG" \ + --body "Companion PR for root repo branch \`$CURRENT_BRANCH\`." +else + echo "No GitHub remote detected for $REPO_REL — branch pushed, open PR manually." +fi +``` + +After processing all selected sub-repos, remove the temp file and continue to +`analyze_commits` for the root repo. + + Classify commits: diff --git a/src/commands.cts b/src/commands.cts index 5414e50cc..ff7a8e4a7 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -729,6 +729,171 @@ function cmdCommitToSubrepo(cwd: string, message: string | undefined, files: str output(result, raw, Object.entries(repos).map(([r, v]) => `${r}:${v.hash || 'skip'}`).join(' ')); } +/** + * Prepare a sub-repo for a companion PR branch. + * + * Detects uncommitted changes, creates a new branch, stages every changed + * file explicitly (never git add -A per universal-anti-patterns.md:44), commits, + * and pushes with --set-upstream. Returns a structured result the workflow uses + * to call `gh pr create`. + * + * On a stage/commit failure (nothing committed yet), the branch is deleted and + * the caller is returned to the original HEAD so the repo is left clean. On a + * push failure, the commit already exists — the branch is left in place instead + * so the user's work is not lost; the error includes a retry instruction. + */ +function cmdPrSubrepo( + cwd: string, + repo: string | undefined, + branch: string | undefined, + commitMessage: string | undefined, + raw: boolean, +): void { + if (!repo) { + error('--repo required'); + } + if (!branch) { + error('--branch required'); + } + if (!commitMessage || commitMessage.startsWith('--')) { + error('commit message required'); + } + if ((branch as string).startsWith('-')) { + error(`Branch name must not start with '-': ${branch}`); + } + + // 0. Security: validate repo path is contained within the workspace root. + // Uses security.cjs validatePath (symlink-safe realpathSync + startsWith guard) + // to reject ../escape, absolute paths, and symlink traversal. + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method + const { validatePath } = require('./security.cjs') as { + validatePath(filePath: string, baseDir: string): { safe: boolean; resolved: string; error?: string }; + }; + const pathCheck = validatePath(repo as string, cwd); + if (!pathCheck.safe) { + error(`Sub-repo path is unsafe: ${pathCheck.error}`); + } + const repoCwd = pathCheck.resolved; + if (!fs.existsSync(repoCwd)) { + error(`Sub-repo not found: ${repoCwd}`); + } + + // 1. Collect changed files via porcelain status — explicit, never git add -A. + // ?? (untracked) lines are excluded — only stage tracked modifications. + const statusResult = execGit(['-c', 'core.quotePath=false', 'status', '--porcelain'], { cwd: repoCwd }); + if (statusResult.exitCode !== 0) { + error(`git status failed in ${repo}: ${statusResult.stderr}`); + } + + // Parse porcelain output into two lists: + // changedFiles — all affected paths (old + new for renames) → goes into result.files + // filesToStage — paths to pass to git add (rename old-paths are already staged by + // the rename op and no longer exist in the worktree; only add new paths) + const changedFiles: string[] = []; + const filesToStage: string[] = []; + for (const line of statusResult.stdout.split('\n').filter(Boolean).filter(l => !l.startsWith('??'))) { + // execGit trims the entire stdout string, which may strip the leading X-status + // space from the first output line. Normalize before slicing. + const normalized = line.trimStart(); + const file = normalized.slice(2).trim(); + const arrowIdx = file.indexOf(' -> '); + if (arrowIdx !== -1) { + const oldPath = file.slice(0, arrowIdx).trim(); + const newPath = file.slice(arrowIdx + 4).trim(); + changedFiles.push(oldPath, newPath); + filesToStage.push(newPath); // old path already staged; worktree no longer has it + } else { + changedFiles.push(file); + filesToStage.push(file); + } + } + + if (changedFiles.length === 0) { + output( + { ok: true, repo, branch, committed: false, reason: 'nothing_to_commit', files: [] }, + raw, + 'nothing_to_commit', + ); + return; + } + + // 2. Guard: refuse if branch already exists — checkout -b is non-idempotent + const branchCheck = execGit(['rev-parse', '--verify', branch as string], { cwd: repoCwd }); + if (branchCheck.exitCode === 0) { + error(`Branch already exists in ${repo}: ${branch}. Delete it first or choose a unique name.`); + } + + // Capture current HEAD before switching so rollback can return explicitly. + // git checkout - fails on a fresh single-branch repo with no prior HEAD. + const prevBranchResult = execGit(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: repoCwd }); + const prevBranchName = prevBranchResult.exitCode === 0 ? prevBranchResult.stdout.trim() : null; + + // 3. Create branch + const checkoutResult = execGit(['checkout', '-b', branch as string], { cwd: repoCwd }); + if (checkoutResult.exitCode !== 0) { + error(`Failed to create branch ${branch} in ${repo}: ${checkoutResult.stderr}`); + } + + // Helper: rollback the created branch and return to the previous HEAD. + const rollback = (): void => { + if (prevBranchName) { + execGit(['checkout', prevBranchName], { cwd: repoCwd }); + } + execGit(['branch', '-D', branch as string], { cwd: repoCwd }); + }; + + // 4. Stage explicit files (never git add -A per universal-anti-patterns.md:44) + for (const file of filesToStage) { + const addResult = execGit(['add', '--', file], { cwd: repoCwd }); + if (addResult.exitCode !== 0) { + rollback(); + error(`Failed to stage ${file} in ${repo}: ${addResult.stderr}`); + } + } + + // 5. Commit + const commitResult = execGit(['commit', '-m', commitMessage as string], { cwd: repoCwd }); + if (commitResult.exitCode !== 0) { + rollback(); + error(`Failed to commit in ${repo}: ${commitResult.stderr}`); + } + + // 6. Capture commit hash + const hashResult = execGit(['rev-parse', '--short', 'HEAD'], { cwd: repoCwd }); + const commitHash = hashResult.exitCode === 0 ? hashResult.stdout.trim() : null; + + // 7. Capture remote URL and derive GitHub owner/repo slug for gh pr create + const remoteResult = execGit(['remote', 'get-url', 'origin'], { cwd: repoCwd }); + const remoteUrl = remoteResult.exitCode === 0 ? remoteResult.stdout.trim() : null; + let remoteSlug: string | null = null; + if (remoteUrl) { + const m = remoteUrl.match(/github\.com[:/](.+?)(?:\.git)?$/); + remoteSlug = m ? m[1] : null; + } + + // 8. Push with --set-upstream so gh pr create can find the branch. + // Network operation — use a longer timeout than the default 10 s. + // Do NOT rollback on push failure — the commit already exists on the local branch. + // Deleting the branch here would destroy the only ref holding the user's work. + // Leave the branch in place so the user can retry the push. + const pushResult = execGit(['push', '--set-upstream', 'origin', branch as string], { cwd: repoCwd, timeout: 60_000 }); + if (pushResult.exitCode !== 0) { + error(`Failed to push ${branch} in ${repo}: ${pushResult.stderr}\nBranch ${branch} was created locally — retry with: git -C ${repo} push --set-upstream origin ${branch}`); + } + + const result = { + ok: true, + repo, + branch, + committed: true, + files: changedFiles, + commit_hash: commitHash, + remote_url: remoteUrl, + remote_slug: remoteSlug, + }; + output(result, raw, `${repo}@${commitHash ?? 'unknown'}`); +} + function cmdSummaryExtract(cwd: string, summaryPath: string | undefined, fields: string[] | undefined, raw: boolean): void { if (!summaryPath) { error('summary-path required for summary-extract'); @@ -1421,6 +1586,7 @@ export = { cmdEffortSync, cmdCommit, cmdCommitToSubrepo, + cmdPrSubrepo, cmdSummaryExtract, cmdWebsearch, cmdProgressRender, diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index e61484a27..7df591603 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -8,10 +8,11 @@ const { test, describe, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); -const { execSync } = require('node:child_process'); +const { execSync, execFileSync } = require('node:child_process'); const fs = require('fs'); const path = require('path'); -const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { runGsdTools, createTempProject, createTempDir, cleanup } = require('./helpers.cjs'); +const fc = require('./helpers/fast-check-setup.cjs'); describe('history-digest command', () => { let tmpDir; @@ -2365,3 +2366,415 @@ describe('user-story validate command (bug #1145)', () => { assert.equal(out.valid, true, `minimal valid story should pass: ${JSON.stringify(out)}`); }); }); + +// --------------------------------------------------------------------------- +// pr-subrepo — regressions (#666) + workflow source invariants +// --------------------------------------------------------------------------- + +describe('pr-subrepo', () => { + function writePrSubrepoConfig(dir, obj) { + const planningDir = path.join(dir, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + fs.writeFileSync(path.join(planningDir, 'config.json'), JSON.stringify(obj, null, 2)); + } + + function initPrSubrepo(dir) { + fs.mkdirSync(dir, { recursive: true }); + execFileSync('git', ['init'], { cwd: dir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 'test@example.com'], { cwd: dir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir, stdio: 'pipe' }); + fs.writeFileSync(path.join(dir, '.gitkeep'), ''); + fs.writeFileSync(path.join(dir, 'feature.js'), '// initial\n'); + fs.writeFileSync(path.join(dir, 'a.js'), '// initial\n'); + fs.writeFileSync(path.join(dir, 'b.js'), '// initial\n'); + execFileSync('git', ['add', '.gitkeep', 'feature.js', 'a.js', 'b.js'], { cwd: dir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'chore: initial commit'], { cwd: dir, stdio: 'pipe' }); + } + + function wirePrSubrepoRemote(repoDir, bareDir) { + fs.mkdirSync(bareDir, { recursive: true }); + execFileSync('git', ['init', '--bare'], { cwd: bareDir, stdio: 'pipe' }); + execFileSync('git', ['remote', 'add', 'origin', bareDir], { cwd: repoDir, stdio: 'pipe' }); + const branch = execFileSync('git', ['branch', '--show-current'], { + cwd: repoDir, encoding: 'utf8', + }).trim(); + execFileSync('git', ['push', 'origin', branch], { cwd: repoDir, stdio: 'pipe' }); + } + + describe('regressions (#666 — cmdPrSubrepo seam)', () => { + let rootDir; + let subDir; + let bareDir; + + beforeEach(() => { + rootDir = createTempDir('gsd-666-root-'); + subDir = path.join(rootDir, 'backend'); + bareDir = path.join(rootDir, '_bare-backend.git'); + writePrSubrepoConfig(rootDir, { planning: { sub_repos: ['backend'] } }); + initPrSubrepo(subDir); + wirePrSubrepoRemote(subDir, bareDir); + }); + + afterEach(() => { + cleanup(rootDir); + }); + + test('config-get planning.sub_repos resolves canonical config location', () => { + const res = runGsdTools(['query', 'config-get', 'planning.sub_repos'], rootDir); + assert.ok(res.success, `config-get planning.sub_repos failed: ${res.error}`); + assert.deepStrictEqual(JSON.parse(res.output), ['backend']); + }); + + test('config-get sub_repos (top-level) fails — confirming bug #666 Blocker 1 is gone', () => { + const res = runGsdTools(['query', 'config-get', 'sub_repos'], rootDir); + assert.ok(!res.success, 'top-level sub_repos key must not resolve — fix requires planning.sub_repos'); + }); + + test('pr-subrepo happy path: branch created, files staged explicitly, commit pushed', () => { + fs.writeFileSync(path.join(subDir, 'feature.js'), 'module.exports = 42;\n'); + + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): add feature', + '--repo', 'backend', '--branch', 'fix-666-backend-pr'], + rootDir + ); + assert.ok(res.success, `pr-subrepo failed: ${res.error}`); + + const result = JSON.parse(res.output); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.repo, 'backend'); + assert.strictEqual(result.branch, 'fix-666-backend-pr'); + assert.strictEqual(result.committed, true); + assert.ok(Array.isArray(result.files) && result.files.length > 0); + assert.ok(result.files.includes('feature.js'), `feature.js missing from files: ${JSON.stringify(result.files)}`); + assert.ok(typeof result.commit_hash === 'string' && result.commit_hash.length > 0); + }); + + test('pr-subrepo stages files explicitly — result.files lists every changed file', () => { + fs.writeFileSync(path.join(subDir, 'a.js'), '1\n'); + fs.writeFileSync(path.join(subDir, 'b.js'), '2\n'); + + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): two files', + '--repo', 'backend', '--branch', 'fix-666-explicit-pr'], + rootDir + ); + assert.ok(res.success, `pr-subrepo failed: ${res.error}`); + + const result = JSON.parse(res.output); + assert.ok(result.files.includes('a.js'), 'a.js must be staged'); + assert.ok(result.files.includes('b.js'), 'b.js must be staged'); + }); + + test('pr-subrepo: nothing_to_commit when sub-repo is clean', () => { + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): nothing', + '--repo', 'backend', '--branch', 'fix-666-clean-pr'], + rootDir + ); + assert.ok(res.success, `pr-subrepo should succeed on clean repo: ${res.error}`); + const result = JSON.parse(res.output); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.committed, false); + assert.strictEqual(result.reason, 'nothing_to_commit'); + }); + + test('pr-subrepo: duplicate branch guard — errors when branch already exists', () => { + fs.writeFileSync(path.join(subDir, 'a.js'), '1\n'); + const first = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): first', + '--repo', 'backend', '--branch', 'fix-666-dup-pr'], + rootDir + ); + assert.ok(first.success, `first call failed: ${first.error}`); + + fs.writeFileSync(path.join(subDir, 'b.js'), '2\n'); + const second = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): second', + '--repo', 'backend', '--branch', 'fix-666-dup-pr'], + rootDir + ); + assert.ok(!second.success, 'Expected failure on duplicate branch name'); + assert.ok(second.error.includes('already exists'), `Got: ${second.error}`); + }); + + test('pr-subrepo: missing --repo returns descriptive error', () => { + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix: msg', '--branch', 'some-branch'], + rootDir + ); + assert.ok(!res.success); + assert.ok(res.error.includes('--repo required'), `Got: ${res.error}`); + }); + + test('pr-subrepo: missing --branch returns descriptive error', () => { + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix: msg', '--repo', 'backend'], + rootDir + ); + assert.ok(!res.success); + assert.ok(res.error.includes('--branch required'), `Got: ${res.error}`); + }); + + test('pr-subrepo: missing commit message returns descriptive error', () => { + const res = runGsdTools( + ['query', 'pr-subrepo', '--repo', 'backend', '--branch', 'some-branch'], + rootDir + ); + assert.ok(!res.success); + assert.ok(res.error.includes('commit message required'), `Got: ${res.error}`); + }); + + test('pr-subrepo: non-existent repo path returns descriptive error', () => { + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix: msg', '--repo', 'nonexistent', '--branch', 'some-branch'], + rootDir + ); + assert.ok(!res.success); + assert.ok( + res.error.includes('not found') || res.error.includes('nonexistent'), + `Got: ${res.error}` + ); + }); + + test('pr-subrepo: path traversal (../escape) is rejected', () => { + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix: msg', '--repo', '../escape', '--branch', 'some-branch'], + rootDir + ); + assert.ok(!res.success, 'Expected failure on path traversal attempt'); + assert.ok( + res.error.includes('unsafe') || res.error.includes('escape'), + `Got: ${res.error}` + ); + }); + + test('pr-subrepo push failure: branch+commit survive when push is rejected (no data loss)', () => { + // Reproduce the data-loss scenario flagged in review: a rejecting remote must leave + // the local branch+commit intact so the user can retry git push manually. + const branch = 'fix-666-push-fail-pr'; + + // Wire a bare remote with a pre-receive hook that rejects all pushes. + const rejectingBare = path.join(rootDir, '_rejecting-bare.git'); + fs.mkdirSync(rejectingBare, { recursive: true }); + execFileSync('git', ['init', '--bare'], { cwd: rejectingBare, stdio: 'pipe' }); + const hookPath = path.join(rejectingBare, 'hooks', 'pre-receive'); + fs.writeFileSync(hookPath, '#!/bin/sh\nexit 1\n'); + fs.chmodSync(hookPath, 0o755); + + // Point origin at the rejecting bare (overwrite the working one wired in beforeEach). + execFileSync('git', ['remote', 'set-url', 'origin', rejectingBare], { cwd: subDir, stdio: 'pipe' }); + + fs.writeFileSync(path.join(subDir, 'feature.js'), 'IMPORTANT USER WORK\n'); + + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): push-fail test', + '--repo', 'backend', '--branch', branch], + rootDir + ); + + // Command must fail because push was rejected. + assert.ok(!res.success, `Expected failure on rejected push, got success: ${res.output}`); + + // The local branch must still exist — work must not be lost. + const branches = execFileSync('git', ['branch', '--list', branch], { + cwd: subDir, encoding: 'utf8', + }); + assert.ok(branches.trim().length > 0, `Branch ${branch} was deleted after push failure — user work lost`); + + // The commit on that branch must contain the user's changes. + const log = execFileSync('git', ['log', branch, '--oneline', '-1'], { + cwd: subDir, encoding: 'utf8', + }); + assert.ok(log.trim().length > 0, `No commit on ${branch} — staged work was lost`); + }); + + test('pr-subrepo porcelain: staged rename — both old and new paths in result.files', () => { + // git mv produces "R old -> new" in porcelain v1; both paths must be staged. + execFileSync('git', ['mv', 'feature.js', 'renamed-feature.js'], { cwd: subDir, stdio: 'pipe' }); + + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): rename', + '--repo', 'backend', '--branch', 'fix-666-rename-pr'], + rootDir + ); + assert.ok(res.success, `pr-subrepo failed: ${res.error}`); + const result = JSON.parse(res.output); + assert.ok(result.files.includes('feature.js'), `old path missing: ${JSON.stringify(result.files)}`); + assert.ok(result.files.includes('renamed-feature.js'), `new path missing: ${JSON.stringify(result.files)}`); + }); + + test('pr-subrepo porcelain: non-ASCII filename (core.quotePath=false)', () => { + // Without -c core.quotePath=false, "café.js" is C-escaped → slice(2) parse breaks. + fs.writeFileSync(path.join(subDir, 'café.js'), '// initial\n'); + execFileSync('git', ['add', 'café.js'], { cwd: subDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'chore: add café.js'], { cwd: subDir, stdio: 'pipe' }); + fs.writeFileSync(path.join(subDir, 'café.js'), 'updated\n'); + + const res = runGsdTools( + ['query', 'pr-subrepo', 'fix(backend): non-ascii', + '--repo', 'backend', '--branch', 'fix-666-nonascii-pr'], + rootDir + ); + assert.ok(res.success, `pr-subrepo failed: ${res.error}`); + const result = JSON.parse(res.output); + assert.ok(result.files.includes('café.js'), `non-ASCII file missing: ${JSON.stringify(result.files)}`); + }); + + test('pr-subrepo porcelain: fc property — parsed filenames are always non-empty strings', () => { + // Local mirror of cmdPrSubrepo's porcelain line-parsing logic (commands.cts). + // Tests the transformation contract without needing a real git repo. + function parsePorcelainLine(line) { + const normalized = line.trimStart(); + const file = normalized.slice(2).trim(); + const arrowIdx = file.indexOf(' -> '); + return arrowIdx !== -1 + ? [file.slice(0, arrowIdx).trim(), file.slice(arrowIdx + 4).trim()] + : [file]; + } + + const safeFilename = fc.stringMatching(/^[a-zA-Z0-9._-]+$/); + const xyChar = fc.constantFrom('M', 'A', 'D', 'R', 'C', 'U'); + const normalLine = fc.tuple(xyChar, xyChar, safeFilename) + .map(([x, y, f]) => `${x}${y} ${f}`); + const renameLine = fc.tuple(xyChar, safeFilename, safeFilename) + .map(([x, o, n]) => `${x} ${o} -> ${n}`); + // First-line trim edge case: leading space stripped by execGit global trim + const trimmedLine = fc.tuple(xyChar, safeFilename) + .map(([y, f]) => ` ${y} ${f}`); + + fc.assert(fc.property( + fc.oneof(normalLine, renameLine, trimmedLine), + (line) => { + const files = parsePorcelainLine(line); + return files.length > 0 && files.every(f => typeof f === 'string' && f.length > 0); + } + )); + }); + }); + + describe('workflow source invariants (#666 — pr-branch.md)', () => { + // allow-test-rule: source-text-is-the-product see #666 + // pr-branch.md is a workflow file whose deployed text IS the runtime contract. + const workflowPath = path.resolve(__dirname, '..', 'gsd-core', 'workflows', 'pr-branch.md'); + let wfContent; + + test('setup', () => { + wfContent = fs.readFileSync(workflowPath, 'utf-8'); + assert.ok(wfContent.length > 0); + }); + + test('uses planning.sub_repos (canonical key) — not legacy top-level sub_repos', () => { + wfContent = wfContent || fs.readFileSync(workflowPath, 'utf-8'); + assert.ok(wfContent.includes('planning.sub_repos'), 'must call config-get planning.sub_repos'); + assert.ok( + !/config-get sub_repos(?!\.)/.test(wfContent), + 'must not call config-get sub_repos without the planning. prefix' + ); + }); + + test('delegates git work to gsd_run query pr-subrepo — no inline git add -A in code', () => { + wfContent = wfContent || fs.readFileSync(workflowPath, 'utf-8'); + assert.ok(wfContent.includes('pr-subrepo'), 'must invoke the pr-subrepo seam'); + const hasForbiddenGitAdd = /^\s*git(?:\s+-C\s+\S+)?\s+add\s+(?:-A|\.)\b/m.test(wfContent); + assert.ok(!hasForbiddenGitAdd, 'must not use git add -A or git add . as a shell command'); + }); + + test('persists dirty-repo list without bash arrays (temp file or inline string)', () => { + wfContent = wfContent || fs.readFileSync(workflowPath, 'utf-8'); + assert.ok( + !wfContent.includes('DIRTY_REPOS=()') && !wfContent.includes('DIRTY_REPOS+='), + 'bash arrays must not be used — they do not survive across command blocks' + ); + }); + + test('branch name includes repo-specific slug to avoid root PR_BRANCH collision', () => { + wfContent = wfContent || fs.readFileSync(workflowPath, 'utf-8'); + assert.ok( + /REPO_SAFE|SUB_BRANCH.*REPO/.test(wfContent), + 'sub-repo branch name must embed a repo-specific component' + ); + }); + + test('handle_sub_repos positioned before analyze_commits', () => { + wfContent = wfContent || fs.readFileSync(workflowPath, 'utf-8'); + const a = wfContent.indexOf('handle_sub_repos'); + const b = wfContent.indexOf('analyze_commits'); + assert.ok(a !== -1 && b !== -1 && a < b); + }); + + test('dirty-scan rejects traversal, newline, and symlink entries before invoking git (security)', () => { + // Extracts and executes the ACTUAL node -e script shipped in pr-branch.md — not a + // mirror — so this test fails if the real script regresses, not just a copy of it. + wfContent = wfContent || fs.readFileSync(workflowPath, 'utf-8'); + const match = wfContent.match(/node -e "([\s\S]*?)"\s+"\$SUB_REPOS_JSON" "\$ROOT" "\$DIRTY_FILE"/); + assert.ok(match, 'could not extract dirty-scan node script from pr-branch.md'); + const script = match[1]; + + // Helper: init a git repo with a TRACKED dirty change. An untracked file would be + // filtered by the ?? exclusion and the repo would look clean even without the guard, + // making the assertions vacuous. A tracked modification ensures that WITHOUT the + // guard the repo WOULD be reported dirty, so the test genuinely fails-first. + const initDirtyRepo = (dir, file) => { + execFileSync('git', ['init'], { cwd: dir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 'test@example.com'], { cwd: dir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir, stdio: 'pipe' }); + fs.writeFileSync(path.join(dir, file), 'committed\n'); + execFileSync('git', ['add', file], { cwd: dir, stdio: 'pipe' }); + execFileSync('git', ['-c', 'commit.gpgsign=false', 'commit', '-m', 'init'], { cwd: dir, stdio: 'pipe' }); + fs.writeFileSync(path.join(dir, file), 'modified\n'); + }; + + const scanRoot = createTempDir('gsd-666-scan-root-'); + const outsideDir = createTempDir('gsd-666-scan-outside-'); + initDirtyRepo(outsideDir, 'secret.txt'); + + // Positive control: a legit dirty sub-repo INSIDE the workspace must still be reported, + // so the test can't pass by a guard that simply rejects everything. + const backendDir = path.join(scanRoot, 'backend'); + fs.mkdirSync(backendDir, { recursive: true }); + initDirtyRepo(backendDir, 'app.js'); + + // Symlink escape: an in-tree name with no ".." and no "/" that points outside root. + // path.resolve would keep it "inside"; only realpathSync catches it. Symlink + // creation needs privileges on Windows — skip just this vector if it throws. + let symlinked = true; + try { fs.symlinkSync(outsideDir, path.join(scanRoot, 'evil')); } catch { symlinked = false; } + + const traversalEntry = path.relative(scanRoot, outsideDir); // e.g. "../gsd-666-scan-outside-XXXX" + const newlineEntry = 'good\nbad'; // record-separator injection attempt + const dirtyFile = path.join(scanRoot, '_dirty'); + const entries = symlinked + ? ['evil', traversalEntry, newlineEntry, 'backend'] + : [traversalEntry, newlineEntry, 'backend']; + const subReposJson = JSON.stringify(entries); + + try { + execFileSync('node', ['-e', script, subReposJson, scanRoot, dirtyFile], { stdio: 'pipe' }); + const dirty = fs.existsSync(dirtyFile) ? fs.readFileSync(dirtyFile, 'utf-8') : ''; + const lines = dirty.split('\n').filter(Boolean); + assert.ok( + !dirty.includes(path.basename(outsideDir)), + `Path traversal reached git outside the workspace: ${JSON.stringify(dirty)}` + ); + if (symlinked) { + assert.ok( + !lines.includes('evil'), + `Symlink entry reached git outside the workspace: ${JSON.stringify(dirty)}` + ); + } + assert.ok( + !lines.includes('bad'), + `Embedded-newline entry injected a spurious record: ${JSON.stringify(dirty)}` + ); + assert.deepStrictEqual( + lines, ['backend'], + `Positive control failed — expected only 'backend', got: ${JSON.stringify(lines)}` + ); + } finally { + cleanup(scanRoot); + cleanup(outsideDir); + } + }); + }); +}); diff --git a/tests/workflow-size-baseline.json b/tests/workflow-size-baseline.json index 3a6ad74db..0845b12b9 100644 --- a/tests/workflow-size-baseline.json +++ b/tests/workflow-size-baseline.json @@ -54,7 +54,7 @@ "plan-phase.md": 93166, "plan-review-convergence.md": 23468, "plant-seed.md": 11741, - "pr-branch.md": 9561, + "pr-branch.md": 15919, "profile-user.md": 20650, "progress.md": 29387, "quick.md": 48830,