* fix(pr-branch): handle sub_repos from config with git -C (#666) Adds a `handle_sub_repos` step between `detect_state` and `analyze_commits`. When `planning.sub_repos` is set in config, the workflow now: - Reads sub-repo paths via `gsd_run query config-get sub_repos` - Skips the step entirely when the list is empty/null/[] - Scans each repo with `git -C "$REPO" status --porcelain` - Offers the user all/select/skip choices - For selected repos: creates a PR branch, commits all staged/unstaged changes, pushes, and opens a companion PR via `gh pr create` All git commands use `git -C "$REPO"` — never `cd "$REPO"` — because shell state does not persist between agent-executed commands. Closes #666 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: update changeset pr number to 667 * fix(pr-branch): address maintainer review — correct seam, behavioral tests, robustness Resolves all three blockers and seven robustness issues raised in PR #667 review: Blockers: - Use `planning.sub_repos` (not top-level `sub_repos`) so config-get actually resolves - Replace prose grep test with behavioral fixture tests using runGsdTools + local bare repo - Extract sub-repo git work into new `cmdPrSubrepo` seam in src/commands.cts; never uses git add -A — stages explicit files only (universal-anti-patterns.md:44) Robustness: - Dirty-repo list persisted via mktemp/cat, not bash arrays (cross-block safe) - Branch name embeds repo slug (${CURRENT_BRANCH}-${REPO_SAFE}-pr) to avoid collision - push --set-upstream so gh pr create finds the branch - Sub-repo base branch resolved via ls-remote with fallback to repo's default branch - Remote slug parsed with /github\.com[:/]/ (handles SSH + HTTPS + .git-less URLs) - rollback() cleans up branch on any mid-sequence failure - node -e replaces jq (always available, no undeclared hard dep) Refs: #666 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(pr-branch): security guard, push timeout, rollback fix, porcelain fix Security (Blocker 1): - Use security.cjs validatePath() in cmdPrSubrepo for symlink-safe workspace containment check — rejects ../escape, absolute paths, and symlink traversal - Add negative regression test: '../escape' repo path must be rejected Robustness: - Push uses timeout: 60_000 ms (network op needs more than the 10 s default) - Capture prevBranchName before checkout -b so rollback uses explicit name instead of git checkout - (fails on fresh single-branch repos) - Porcelain path parse: line.trimStart().slice(2).trim() handles all XY combinations and the execGit global-trim edge case uniformly Tests: 17/17 pass, lint: 0 errors Refs: #666 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(pr-branch): move regression tests to commands.test.cjs, add core.quotePath=false - Move cmdPrSubrepo behavioral + workflow source-invariant tests from standalone bug-666-*.test.cjs into tests/commands.test.cjs under describe('pr-subrepo') per TESTING-SUITES.md policy (no new bug-* files). Adds allow-test-rule: source-text-is-the-product see #666 for the workflow-source-invariant suite. - Add -c core.quotePath=false to git status --porcelain call so non-ASCII filenames (e.g. café) are not C-escaped, keeping slice(2) parse correct. * fix(pr-branch): remove obsolete regression tests for sub-repos handling * fix(pr-branch): update workflow-size-baseline, add dirty-scan timeout - Regenerate tests/workflow-size-baseline.json for pr-branch.md growth (+handle_sub_repos step, +timeout addition). - Add { timeout: 10_000 } to the execFileSync git status --porcelain call in the handle_sub_repos dirty-scan (repo convention: every git subprocess is bounded, never hangs). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: regenerate INVENTORY-MANIFEST after rebase onto next Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#666): handle rename staging and split changedFiles from filesToStage For git mv renames, the old path no longer exists in the worktree after the move — staging it with git add fails. Split parsing into changedFiles (both paths, for result.files) and filesToStage (new path only for renames; old is already staged by git mv). Also adds porcelain tests for staged renames, non-ASCII filenames, and a fast-check property test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#666): rollback on push failure in cmdPrSubrepo If push fails the branch only exists locally; rollback cleans it up so the sub-repo is not left in a half-committed state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#666): do not rollback after commit on push failure; add push-fail regression test Post-commit push failures are network/auth/policy issues — the user's work is already committed on the local branch. Calling rollback() at that point force-deletes the only ref holding the commit (data loss). Leave the branch in place and emit a retry instruction instead. Adds a regression test (pre-receive hook that rejects all pushes) asserting the branch and commit survive a push rejection so the failure path stays covered going forward. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: regenerate INVENTORY-MANIFEST after rebase onto next Rebased onto current next (#1267 retired core.cjs). Stale tsbuildinfo and a leftover bin/lib/core.cjs build artifact were masking the drift — wiped both, rebuilt clean, and regenerated the manifest. gen-inventory-manifest --check now exits 0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#666): validate sub-repo paths before git invocation in pr-branch.md The handle_sub_repos workflow ran git -C on raw planning.sub_repos config values at two points before the pr-subrepo seam's validatePath guard ever ran: the dirty-scan detection (git status) and the base-branch resolution (git ls-remote / remote show). A traversal entry could point git outside the workspace; an embedded newline could inject a spurious record into the newline-joined dirty-file output and into the shell-interpolated commit message. Adds a containment check + character allowlist to the dirty-scan node script (reject before any execFileSync), and a defense-in-depth shell case guard on the same value before the second, independent git -C invocation in the base-branch resolution block. Adds a behavioral test that extracts and executes the actual shipped node script from pr-branch.md (not a mirror) against a real traversal target and an embedded-newline entry, asserting neither reaches git or the dirty-file output. Also updates the stale cmdPrSubrepo doc comment: push failures no longer delete the branch (see prior commit), only stage/commit failures do. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(#666): make sub-repo traversal scan test genuinely fail-first The outside repo's only change was an untracked file, which the ?? filter excludes — so the repo looked clean even with the guard removed, making the traversal assertion vacuous (it passed against a neutered guard). Commit the file first, then modify it, so the outside repo has a tracked dirty change: without the path guard it WOULD be reported dirty, so the test now fails-first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#666): symlink-safe (realpath) sub-repo containment in pr-branch.md Finding A from re-review: the workflow guard used path.resolve, which only normalizes '..' textually and does not follow symlinks — so an in-tree symlink whose name has no '..' or '/' (e.g. "evil" -> /outside) passed both the charset filter and the resolve+startsWith check, letting git status / ls-remote / remote show run against a directory outside the workspace. The pr-subrepo seam already used fs.realpathSync (validatePath); this brings the workflow layer to parity. - dirty-scan: realpathSync the root once, and realpathSync each candidate before the containment check; skip on throw. - base-branch resolution: replace the weak `case *..*|/*` guard with a realpath containment check that yields a validated absolute SUB_REPO_DIR, and run git -C against that instead of re-concatenating $ROOT/$REPO_REL. - security test: add a symlink-escape entry and a positive control (legit in-root backend must still be reported). Confirmed fails-first — regressing the scan to path.resolve makes the symlink case leak. Also fixes a misleading-fallback minor: the workflow now checks the seam's exit status and skips the companion-PR step on failure, instead of printing "branch pushed, open PR manually" after a real stage/commit/push failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#666): harden pr-branch sub-repo flow against round-12 edge cases Pre-emptive hardening of the workflow changes from the symlink fix: - continue-outside-loop: the "skip companion PR on seam failure" block used a bash `continue`, but the per-sub-repo iteration is prose-driven (the agent loops, not a literal `for`), so `continue` would warn and no-op. Reframed as prose-gated control flow keyed on $SUBREPO_EXIT — no bash loop assumption. - Windows portability: the new symlink security case now degrades gracefully (try/catch around fs.symlinkSync; skip just the symlink assertion when symlink creation lacks privileges) so it doesn't hard-fail on Windows CI. Verified: seam exits 1 on error / 0 on success (error() → process.exit(1), propagated through the shim), so the $SUBREPO_EXIT check is meaningful; bash -n clean on the touched blocks; commands 156/156; lint:ci green; manifest in sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Enes Yağız
166
src/commands.cts
166
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,
|
||||
|
||||
Reference in New Issue
Block a user