golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(3678): executor must respect commit_docs:false; teach SDK skip envelope (#3679)

Browse Source 
* fix(3678): executor must respect commit_docs:false; teach SDK skip envelope

Closes #3678

When `commit_docs: false` in `.planning/config.json`, the SDK's
`cmdCommit` correctly short-circuits and returns
`{committed: false, hash: null, reason: 'skipped_commit_docs_false'}`
without staging or committing anything. The agent prompt at
`agents/gsd-executor.md:710-720` (final_commit block) tells the executor
to call `gsd-sdk query commit "docs(...)" --files .planning/...` but says
NOTHING about how to interpret a skipped return. With no explicit
instruction, the LLM improvises raw `git add` / `git add -f` / `git commit`
to "fulfill" the per-plan commit step it was told to make, which leaks
gitignored `.planning/` artifacts into the user's git history (exactly
what the reporter observed).

Three coordinated fixes:

1. **agents/gsd-executor.md final_commit block** — adds explicit handling
   text for all three SDK return envelopes (`committed:true`, `skipped:true
   commit_docs`, `skipped:true gitignored`, `committed:false other reasons`).
   States plainly: "Do not fall back to raw `git add` / `git commit` /
   `git add -f` when the SDK returns `skipped: true`."

2. **get-shit-done/bin/lib/commands.cjs cmdCommit** — adds `skipped: true`
   to both skip-path envelopes so agents see "skipped" as a first-class
   success signal rather than inferring "no commit happened, I must
   improvise" from absent `hash` / `committed:false`. Backward-compatible:
   existing callers reading `committed` / `hash` / `reason` are unaffected.

3. **tests/bug-3678-executor-commit-docs-respect.test.cjs** — 7-test
   regression covering:
   - A1/A2: agent prompt mentions the skip envelope AND explicitly forbids
     raw-git fallback (`source-text-is-the-product` exception)
   - B1: SDK envelope carries `committed:false`, `skipped:true`, canonical
     `reason: 'skipped_commit_docs_false'` (frozen enum)
   - B2: git index empty after commit_docs:false skip (no `.planning/` staged)
   - B3: HEAD unchanged after commit_docs:false skip
   - C1/C2: structural ban on `git add -f` / `git add --force` in any agent
     or workflow body (prohibition-sentence exception preserves audit prose)

Verification:
- node --test tests/bug-3678-*: 7/7 pass
- Targeted regression (10 commit/executor-adjacent files): 135/135 pass
- Full docker suite (gsd-test-summary): 11751/11740 pass / 0 fail
  (the 11 added are this test plus a few collateral pickups)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(changeset): add fragment for #3678 fix (Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>)

* chore(changeset): set PR number 3679 (Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>)

* fix(3678): preserve skip-aware carve-out in executor completion checklist

The new `final_commit` prose at lines 717-741 teaches the executor to
treat `skipped:true` as success and forbids raw-git fallback, but the
downstream completion checklist still contained an unconditional
"Final metadata commit made" checkbox. An LLM executor reading an
unchecked mandatory box may attempt to satisfy it via raw `git add`,
re-introducing the exact regression this PR is meant to prevent.

Update the checklist line to carve out the intentional-skip case and
add a regression test asserting the carve-out remains present.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-05-18 00:14:03 -04:00
committed by GitHub
parent 09ab16f9e5
commit 7e6ba56985
4 changed files with 284 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/fix-3678-commit-docs-respect.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 3679
---
**`gsd-executor` no longer force-commits `.planning/` artifacts when `commit_docs: false`** — the executor agent prompt now explicitly handles the SDK's `{committed:false, skipped:true, reason:'skipped_commit_docs_false'}` envelope and is forbidden from falling back to raw `git add` / `git add -f` / `git commit`. The SDK's `cmdCommit` now sets `skipped: true` on both skip paths (`commit_docs:false` and `.planning gitignored`) so agents see the skip as a first-class signal. A structural regression test bans `git add -f` / `git add --force` from any agent or workflow body. (#3678)

24
agents/gsd-executor.md
View File

@@ -717,6 +717,28 @@ gsd-sdk query commit "docs({phase}-{plan}): complete [plan-name] plan" --files \
```
Separate from per-task commits — captures execution results only.
**Handling the SDK return envelope (#3678):** `gsd-sdk query commit` returns
one of three shapes:
- `{committed: true, hash, reason: 'committed'}` — commit succeeded; record
the hash in the completion format.
- `{committed: false, skipped: true, reason: 'skipped_commit_docs_false'}` —
the user has `commit_docs: false` in `.planning/config.json`. **This is an
intentional success path.** Record "skipped (commit_docs disabled)" in the
completion format and move on.
- `{committed: false, skipped: true, reason: 'skipped_gitignored'}` —
`.planning/` is gitignored in the user's project. **Also an intentional
success path.** Record "skipped (.planning gitignored)" and move on.
- `{committed: false, reason: 'nothing_to_commit' | 'commit_failed', ...}` —
no-op / genuine failure; surface in the completion notes.
**Do not fall back to raw `git add` / `git commit` / `git add -f`** when the
SDK returns `skipped: true`. The SDK's skip is the user's deliberate choice
to keep `.planning/` files out of git history. Force-staging gitignored
content via `git add -f .planning/...` is forbidden — that bug is exactly
the regression #3678 reported, where the agent leaks `.planning/` artifacts
into the user's project history.
</final_commit>
<completion_format>
@@ -747,6 +769,6 @@ Plan execution complete when:
- [ ] SUMMARY.md created with substantive content
- [ ] STATE.md updated (position, decisions, issues, session)
- [ ] ROADMAP.md updated with plan progress (via `roadmap update-plan-progress`)
- [ ] Final metadata commit made (includes SUMMARY.md, STATE.md, ROADMAP.md)
- [ ] Final metadata commit made (includes SUMMARY.md, STATE.md, ROADMAP.md), or SDK returned an intentional skip (`skipped_commit_docs_false` / `skipped_gitignored`) — record "skipped (<reason>)" in completion notes
- [ ] Completion format returned to orchestrator
</success_criteria>

7
get-shit-done/bin/lib/commands.cjs
View File

@@ -266,15 +266,18 @@ function cmdCommit(cwd, message, files, raw, amend, noVerify) {
const config = loadConfig(cwd);
// Check commit_docs config
// `skipped: true` is explicit so agent prompts can match on a first-class
// success signal rather than inferring "skip" from "committed is missing"
// and improvising raw git fallbacks (#3678).
if (!config.commit_docs) {
const result = { committed: false, hash: null, reason: 'skipped_commit_docs_false' };
const result = { committed: false, skipped: true, hash: null, reason: 'skipped_commit_docs_false' };
output(result, raw, 'skipped');
return;
}
// Check if .planning is gitignored
if (isGitIgnored(cwd, '.planning')) {
const result = { committed: false, hash: null, reason: 'skipped_gitignored' };
const result = { committed: false, skipped: true, hash: null, reason: 'skipped_gitignored' };
output(result, raw, 'skipped');
return;
}

251
tests/bug-3678-executor-commit-docs-respect.test.cjs Normal file
View File

@@ -0,0 +1,251 @@
// allow-test-rule: source-text-is-the-product
// Three of the assertions in this file (A1, A2, C) inspect agent / workflow
// `.md` bodies. Those files ARE the runtime contract that GSD loads into agent
// prompts at run time, so source-text inspection is exactly what the
// `source-text-is-the-product` exception covers.
//
// The remaining assertions (B1, B2, B3) are behavioral — they invoke
// `gsd-tools commit` against a temp project and assert on its structured
// JSON return envelope plus the git index state. No raw-text matching on
// rendered output.
/**
* Regression for #3678 — gsd-executor force-commits .planning/ files when
* commit_docs is false.
*
* Root cause: the executor agent prompt (agents/gsd-executor.md) tells the
* agent to call `gsd-sdk query commit "docs(...)" --files .planning/...`
* in the per-plan final_commit block, but the prompt says nothing about
* what to do when the SDK returns `{committed: false, skipped: true,
* reason: 'skipped_commit_docs_false'}`. With no explicit instruction, the
* agent improvises raw `git add` / `git commit` against `.planning/` paths
* (and uses `-f` to bypass gitignore), which is exactly the leakage the
* reporter observed.
*
* Fix surface:
* 1. Agent prompt: explicit handling text in the final_commit section.
* 2. SDK envelope: add `skipped: true` field so agents see "skipped" as a
* first-class success signal, not "committed is missing, must improvise."
* 3. Structural guard: ban `git add -f` / `git add --force` from agent and
* workflow bodies entirely (no GSD-managed surface should force-stage
* gitignored content).
*/
'use strict';
const { describe, test, beforeEach, afterEach } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const path = require('node:path');
const { execFileSync } = require('node:child_process');
const { createTempGitProject, cleanup, runGsdTools } = require('./helpers.cjs');
// Repo root resolution. This test file lives in `<repo>/tests/`. Use a single
// parent reference (the established repo-wide pattern, e.g. tests/helpers.cjs
// `path.resolve(__dirname, '..', 'get-shit-done', ...)`). A `.git`-anchored
// walker is not portable because the docker test mirror at `/work` strips the
// `.git/` directory before running tests.
const REPO_ROOT = path.resolve(__dirname, '..');
const EXECUTOR_AGENT = path.join(REPO_ROOT, 'agents', 'gsd-executor.md');
// Frozen reason enum mirrors the SDK source — keep in sync with
// `cmdCommit` in get-shit-done/bin/lib/commands.cjs.
const COMMIT_REASON = Object.freeze({
SKIPPED_COMMIT_DOCS_FALSE: 'skipped_commit_docs_false',
SKIPPED_GITIGNORED: 'skipped_gitignored',
});
function git(args, cwd) {
return execFileSync('git', args, { cwd, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] });
}
describe('bug #3678 — executor must respect commit_docs:false', () => {
describe('A — agent prompt teaches the agent how to handle commit_docs:false', () => {
test('A1: agent body explicitly references the SDK skipped envelope', () => {
const body = fs.readFileSync(EXECUTOR_AGENT, 'utf-8');
// The prompt must contain at least one literal mention of the skipped
// reason code OR the `committed: false` envelope so the agent knows
// that skipping is an intentional control flow, not a failure to work
// around.
const mentionsSkipReason = body.includes(COMMIT_REASON.SKIPPED_COMMIT_DOCS_FALSE);
const mentionsCommittedFalse = /committed:\s*false/i.test(body);
const mentionsSkippedTrue = /skipped:\s*true/i.test(body);
assert.ok(
mentionsSkipReason || mentionsCommittedFalse || mentionsSkippedTrue,
'agents/gsd-executor.md must teach the agent how to recognize the '
+ 'skipped envelope from `gsd-sdk query commit` (one of: '
+ `'${COMMIT_REASON.SKIPPED_COMMIT_DOCS_FALSE}', 'committed: false', `
+ "'skipped: true').",
);
});
test('A2: agent body explicitly forbids raw git fallback when SDK skips', () => {
const body = fs.readFileSync(EXECUTOR_AGENT, 'utf-8');
// Look for an explicit instruction tying the SDK-skipped signal to the
// forbidden-fallback rule. Accept any of three shapes the doc writer
// might use: "do not", "must not", or "never" + a verb that names the
// forbidden action.
const forbidsFallbackText = /(do not|must not|never)\s+(fall back|fallback|use .*git add|run .*git commit|force[- ]?add)/i;
assert.ok(
forbidsFallbackText.test(body),
'agents/gsd-executor.md must contain an explicit "do not fall back to '
+ 'raw git" instruction tied to the commit_docs:false / skipped envelope. '
+ 'Without it, the agent improvises raw `git add` / `git add -f` to '
+ 'fulfill its "complete plan" goal.',
);
});
});
describe('B — SDK behavior: commit_docs:false leaves repo state untouched', () => {
let tmpDir;
beforeEach(() => {
tmpDir = createTempGitProject();
// .planning/ already exists from createTempGitProject's setup.
// Set commit_docs to false on the config.
const configPath = path.join(tmpDir, '.planning', 'config.json');
let config = {};
if (fs.existsSync(configPath)) {
config = JSON.parse(fs.readFileSync(configPath, 'utf-8'));
}
config.commit_docs = false;
fs.writeFileSync(configPath, JSON.stringify(config, null, 2));
// Make a token edit to .planning/STATE.md so there IS something the SDK
// could in principle stage (or that an improvising agent could leak).
const statePath = path.join(tmpDir, '.planning', 'STATE.md');
if (!fs.existsSync(statePath)) {
fs.writeFileSync(statePath, '---\nproject: test\n---\n# State\n');
}
fs.appendFileSync(statePath, '\n<!-- token edit for #3678 repro -->\n');
});
afterEach(() => cleanup(tmpDir));
test('B1: commit returns committed:false with skipped envelope', () => {
const result = runGsdTools(
'commit "docs(test): noop" --files .planning/STATE.md',
tmpDir,
);
assert.ok(result.success, `gsd-tools commit should exit 0 even when skipped: ${result.error || ''}`);
const envelope = JSON.parse(result.output);
assert.strictEqual(envelope.committed, false, 'committed must be false when commit_docs is false');
assert.strictEqual(
envelope.skipped,
true,
'envelope must carry skipped:true so agents see skip as a first-class signal (envelope contract for #3678)',
);
assert.strictEqual(
envelope.reason,
COMMIT_REASON.SKIPPED_COMMIT_DOCS_FALSE,
'reason must be the canonical skipped_commit_docs_false code (frozen enum)',
);
});
test('B2: commit_docs:false leaves the git index empty (no .planning/ staged)', () => {
runGsdTools(
'commit "docs(test): noop" --files .planning/STATE.md',
tmpDir,
);
const stagedAll = git(['diff', '--cached', '--name-only'], tmpDir);
const stagedPlanning = stagedAll
.split('\n')
.map(s => s.trim())
.filter(s => s.startsWith('.planning/'));
assert.deepStrictEqual(
stagedPlanning,
[],
'no .planning/ files should be staged when commit_docs is false',
);
});
test('B3: commit_docs:false produces no new commits', () => {
const headBefore = git(['rev-parse', 'HEAD'], tmpDir).trim();
runGsdTools(
'commit "docs(test): noop" --files .planning/STATE.md',
tmpDir,
);
const headAfter = git(['rev-parse', 'HEAD'], tmpDir).trim();
assert.strictEqual(
headAfter,
headBefore,
'HEAD must not advance when commit_docs is false',
);
});
});
test('checklist carve-out preserved for intentional skip', () => {
const body = fs.readFileSync(EXECUTOR_AGENT, 'utf-8');
const checklistLine = body
.split('\n')
.find(line => /Final metadata commit made/.test(line));
assert.ok(
checklistLine,
'agents/gsd-executor.md must contain a "Final metadata commit made" checklist line',
);
assert.ok(
checklistLine.includes('Final metadata commit'),
'checklist line must reference "Final metadata commit"',
);
assert.ok(
checklistLine.includes('skipped_commit_docs_false'),
'checklist line must carve out the intentional-skip case by referencing '
+ '"skipped_commit_docs_false" — prevents executor from treating an '
+ 'unchecked mandatory box as a raw-git TODO (regression guard for #3679)',
);
});
describe('C — structural ban on raw force-add in GSD-managed bodies', () => {
function scanForForceAdd(rootDir) {
const offenders = [];
function walk(dir) {
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
const full = path.join(dir, entry.name);
if (entry.isDirectory()) { walk(full); continue; }
if (!entry.isFile() || !entry.name.endsWith('.md')) continue;
const body = fs.readFileSync(full, 'utf-8');
const lines = body.split('\n');
const danger = lines.filter((line) => {
if (!/git\s+add\s+(-f|--force)\b/.test(line)) return false;
// Allow prohibition / warning sentences and code-fence prose that
// frames `git add -f` AS the bug (so an audit comment doesn't
// create a false positive).
if (/(do not|don'?t|must not|never|forbidden|prohibited)/i.test(line)) return false;
if (/(bug|wrong|incorrect|antipattern|anti-pattern|forces?\s+gitignored|leak)/i.test(line)) return false;
return true;
});
if (danger.length > 0) {
offenders.push({
file: full.replace(REPO_ROOT + '/', ''),
lines: danger.map(l => l.trim().slice(0, 120)),
});
}
}
}
walk(rootDir);
return offenders;
}
test('C1: no agent body contains `git add -f` / `git add --force`', () => {
const offenders = scanForForceAdd(path.join(REPO_ROOT, 'agents'));
assert.deepStrictEqual(
offenders,
[],
'no agent body may use `git add -f` / `git add --force` outside a '
+ 'prohibition sentence — agents must never force-stage gitignored '
+ 'content (regression guard for #3678).',
);
});
test('C2: no workflow body contains `git add -f` / `git add --force`', () => {
const offenders = scanForForceAdd(path.join(REPO_ROOT, 'get-shit-done', 'workflows'));
assert.deepStrictEqual(
offenders,
[],
'no workflow body may use `git add -f` / `git add --force` outside a '
+ 'prohibition sentence (regression guard for #3678).',
);
});
});
});