fix(#2523): normalize --files to repo-relative; reject out-of-repo; gate push on git-add exit (#2638)

* test(#2523): absolute + mixed + out-of-repo --files paths

* fix(#2523): normalize --files to repo-relative; reject out-of-repo; gate push on git-add exit

* chore(#2523): backfill changeset pr to 2638

.changeset/lively-finches-leap.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 2638
+---
+**`query commit --files` now accepts absolute paths** — `cmdCommit` used `path.join(cwd, file)`, which concatenates instead of resetting on an absolute path, so absolute `--files` entries (e.g. the absolute `phase_dir` emitted by `init phase-op` since #2428) were joined to `cwd+absPath` (non-existent) and silently dropped as `nothing_to_commit` — and a mixed relative/absolute list committed the relative entries while reporting `committed:true`. Absolute paths are now normalized to repo-relative before staging/branch-detection, so they commit correctly and the phase-branch detection no longer matches digit-hyphen runs in the absolute prefix. (#2523)

src/commands.cts

@@ -769,6 +769,28 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
     return;
   }
 
+  // Normalize --files to repo-relative paths (#2523). path.join(cwd, absPath)
+  // concatenates instead of resetting (path.resolve does), so absolute phase_dir
+  // paths emitted by init phase-op (#2428) were joined to cwd+absPath (non-existent)
+  // and silently dropped by the existence check — and a mixed relative/absolute list
+  // then committed the relative entries while reporting committed:true. Relative-izing
+  // also keeps the phase-branch detection below from matching digit-hyphen runs in the
+  // absolute prefix (e.g. /proj-7-decoy2/ → wrong phase branch).
+  const filesRel = files && files.length > 0
+    ? files.map(f => toPosixPath(path.relative(cwd, path.resolve(cwd, f))))
+    : [];
+  // Reject --files entries resolving OUTSIDE the project root: fail loudly rather
+  // than silently skip or reach `git add` (which rejects them and would pollute the
+  // index via an unconditional stagedPaths.push) (#2523). `toPosixPath` only flips
+  // backslashes, so a `..` escape is still detectable.
+  for (const rel of filesRel) {
+    if (rel.startsWith('..') || path.isAbsolute(rel)) {
+      const result = { committed: false, hash: null, reason: 'path_outside_repo', path: rel };
+      output(result, raw, 'failed');
+      return;
+    }
+  }
+
   // Ensure branching strategy branch exists before first commit (#1278).
   // Pre-execution workflows (discuss, plan, research) commit artifacts but the branch
   // was previously only created during execute-phase — too late.
@@ -777,7 +799,7 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
     let branchName: string | null = null;
     if (branchingStrategy === 'phase') {
       // Determine which phase we're committing for from the file paths
-      const phaseMatch = (files || []).join(' ').match(/(\d+(?:\.\d+)*)-/);
+      const phaseMatch = (filesRel || []).join(' ').match(/(\d+(?:\.\d+)*)-/);
       if (phaseMatch) {
         const phaseNum = phaseMatch[1];
         const phaseInfo = findPhaseInternal(cwd, phaseNum) as Record<string, unknown> | null;
@@ -809,7 +831,7 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
 
   // Stage files
   const explicitFiles = files && files.length > 0;
-  const filesToStage = explicitFiles ? files : ['.planning/'];
+  const filesToStage = explicitFiles ? filesRel : ['.planning/'];
   const stagedPaths: string[] = [];
   for (const file of filesToStage) {
     const fullPath = path.join(cwd, file);
@@ -824,8 +846,13 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
       // removed planning files are not left dangling in the index.
       execGit(['rm', '--cached', '--ignore-unmatch', file], { cwd });
     } else {
-      execGit(['add', file], { cwd });
-      stagedPaths.push(file);
+      const addResult = execGit(['add', file], { cwd });
+      // Only record paths that actually staged — a failed `git add` (permissions,
+      // out-of-repo edge) must not enter the commit pathspec (#2523). Mirrors
+      // cmdCommitToSubrepo's exitCode-gated push.
+      if (addResult.exitCode === 0) {
+        stagedPaths.push(file);
+      }
     }
   }
 

tests/commit-files-pathspec.test.cjs

@@ -173,4 +173,66 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => {
       'extra.txt should remain staged, not absorbed into a commit',
     );
   });
+
+  test('#2523: absolute --files path inside the repo is committed, not silently dropped', () => {
+    // init phase-op emits phase_dir as an ABSOLUTE path (#2428); cmdCommit must
+    // accept it. The bug was path.join(cwd, absPath) → cwd+absPath (non-existent)
+    // → silently skipped as nothing_to_commit (#2523).
+    fs.writeFileSync(path.join(tmpDir, '.planning', 'A.md'), 'a\n');
+    const absPath = path.join(tmpDir, '.planning', 'A.md');
+    const res = runGsdTools(['commit', 'docs: abs path', '--files', absPath], tmpDir);
+    const parsed = JSON.parse(res.output);
+    assert.strictEqual(parsed.committed, true, `absolute path must commit, not nothing_to_commit: ${res.output}`);
+
+    // The absolute path must land in the commit, normalized to repo-relative.
+    const diff = execSync('git diff HEAD~1 HEAD --name-only', { cwd: tmpDir, encoding: 'utf-8' }).trim();
+    assert.strictEqual(diff, '.planning/A.md', `absolute --files path must be committed (normalized to relative); got: ${diff}`);
+  });
+
+  test('#2523: mixed relative+absolute --files list commits BOTH (no silent partial commit)', () => {
+    // The sharpest symptom: a mixed list committed the relative entry, dropped the
+    // absolute one, and reported committed:true (#2523). Both must land.
+    fs.writeFileSync(path.join(tmpDir, '.planning', 'REL.md'), 'r\n');
+    fs.writeFileSync(path.join(tmpDir, '.planning', 'ABS.md'), 'a\n');
+    const absPath = path.join(tmpDir, '.planning', 'ABS.md');
+    const res = runGsdTools(
+      ['commit', 'docs: mixed', '--files', '.planning/REL.md', absPath],
+      tmpDir,
+    );
+    const parsed = JSON.parse(res.output);
+    assert.strictEqual(parsed.committed, true, `mixed list must commit: ${res.output}`);
+
+    const diff = execSync('git diff HEAD~1 HEAD --name-only', { cwd: tmpDir, encoding: 'utf-8' })
+      .trim().split('\n').sort();
+    assert.deepStrictEqual(
+      diff,
+      ['.planning/ABS.md', '.planning/REL.md'],
+      `mixed relative+absolute list must commit BOTH entries (the bug dropped the absolute one); got: ${diff.join(',')}`,
+    );
+  });
+
+  test('#2523: out-of-repo --files path is rejected loudly (path_outside_repo), no index pollution', (t) => {
+    // An absolute path resolving OUTSIDE the project root must fail loudly, not
+    // silently skip and not pollute the index via a failed git add (#2523).
+    const outsideDir = path.join(tmpDir, '..', `gsd-2523-outside-${process.pid}-${Date.now()}`);
+    fs.mkdirSync(outsideDir, { recursive: true });
+    t.after(() => cleanup(outsideDir));
+    const outsideFile = path.join(outsideDir, 'secret.md');
+    fs.writeFileSync(outsideFile, 's\n');
+
+    const res = runGsdTools(
+      ['commit', 'docs: outside', '--files', path.resolve(outsideFile)],
+      tmpDir,
+    );
+    const parsed = JSON.parse(res.output);
+    assert.strictEqual(parsed.committed, false, 'out-of-repo path must not commit');
+    assert.strictEqual(parsed.reason, 'path_outside_repo', `out-of-repo path must fail loudly: ${res.output}`);
+
+    // No new commit created (still at the single initial commit).
+    const logCount = execSync('git rev-list --count HEAD', { cwd: tmpDir, encoding: 'utf-8' }).trim();
+    assert.strictEqual(logCount, '1', 'no new commit must be created for an out-of-repo path');
+    // Index stays clean (no pollution from a failed git add).
+    const status = execSync('git status --porcelain', { cwd: tmpDir, encoding: 'utf-8' }).trim();
+    assert.strictEqual(status, '', `index must be clean (no pollution): ${status}`);
+  });
 });