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

enhance(#3587): add a per-phase commit_docs override (#3601)

Browse Source 
* feat(#3587): add a per-phase commit_docs override

Delivers epic #2292's second user story: commit an architecture phase's
artifacts while execution phases stay local. commit_docs was project-wide and
binary, so the only choices were all phases or none.

Shape is a config dynamic key phase_commit_docs.<phase-id>, following the 14
existing dynamicKeyPatterns precedents rather than inventing a PLAN.md
frontmatter spec -- which #2292 itself flags as becoming its own maintenance
surface.

Tier 1 resolves in cmdCommit, NOT in loadConfig: loadConfig has no phase
context and is called by nearly every command, so threading one through it to
serve a single caller would be a far larger blast radius for no gain. The phase
comes from detectPhaseNumberFromFiles, which cmdCommit already computes for
branch naming and which is already hardened against the #2539 project-code bug.

Suppression by the per-phase tier returns its own reason rather than reusing
skipped_commit_docs_false -- telling a user their project setting is false when
it is true would be actively misleading. Additive; the two existing reason
strings that agents/gsd-executor.md matches on are unchanged.

The manifest's phase-id pattern is a hand-copy of PHASE_NUMBER_TOKEN_SOURCE
because the manifest is hand-maintained JSON, so a behavioral parity test
asserts both surfaces accept and reject the same token shapes.

* fix(#3587): fold tests, close review findings, update reference docs

Fold: the new tests were added as their own file, which required loosening a
grandfathered lint-test-file-count bucket 5-to-6. A ratchet exists to go down
only. commit-docs-bypass.test.cjs is the established commit_docs test home and
already hosts two folded suites, so the tests fold there as a third block and
the allowlist is reverted untouched.

Standards review: CONTEXT.md and the test header both cited a
phase-commit-docs-manifest-parity.test.cjs that never existed; a repo-wide
sweep found a fourth stale cite in the schema manifest description. All four
now name the real location.

Spec review: the issue's Scope of changes named planning-config.md and
git-planning-commit.md and neither was touched. Both now document the four-tier
precedence and the new skip reason.

Security review, minor and unproven: detectPhaseNumberFromFiles returns the
FIRST matching path's phase, so a --files list spanning two phases resolves the
override against whichever comes first. That helper is hardened and widely used,
so it is not changed; the behavior is pinned by a named test and disclosed in
the design and user docs. A pinned behavior is not a bug; an unpinned surprise
is.

* chore(#3587): backfill changeset pr number to 3601

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-17 21:59:40 -04:00
committed by GitHub
parent f56ffa86ab
commit debeabd524
11 changed files with 718 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

102
src/commands.cts
View File

@@ -1063,6 +1063,79 @@ function detectPhaseNumberFromFiles(files: string[] | undefined): string | null
return null;
}
type CommitDocsSource = 'phase' | 'config' | 'gitignore' | 'default';
interface CommitDocsResolution {
resolved: boolean;
source: CommitDocsSource;
}
/**
* #3587: resolve the `phase_commit_docs.<phase-id>` override for `phaseNum`
* against `config['phase_commit_docs']` (a `{ "<phase-id>": boolean }` map, the
* same shape `agent_skills`/`features` use for their dynamic key families).
* Returns `undefined` — "no override applies" — when: no phase is known (B7),
* the map carries no entry for THIS phase (B5: no cross-phase leak), or the
* entry exists but is not a boolean (B6: never silently coerced). Both sides of
* the comparison route through `normalizePhaseName` so `3`, `03`, and `PROJ-03`
* all resolve to the same entry (B4/B9), reusing the single-owner phase-id
* normalizer rather than a second, looser string-equality rule.
*/
function resolvePhaseCommitDocsOverride(config: Record<string, unknown>, phaseNum: string | null): boolean | undefined {
if (!phaseNum) return undefined;
const overrides = config['phase_commit_docs'];
if (!overrides || typeof overrides !== 'object' || Array.isArray(overrides)) return undefined;
const target = normalizePhaseName(phaseNum);
for (const [key, value] of Object.entries(overrides as Record<string, unknown>)) {
if (normalizePhaseName(key) === target) {
return typeof value === 'boolean' ? value : undefined;
}
}
return undefined;
}
/**
* #3587: the four-tier `commit_docs` precedence chain for a single commit —
* `phase_commit_docs.<phase-id>` (tier 1, resolved HERE because this call site
* is the one place that knows the phase — see 40-design.md "Rejected" §1: NOT
* inside `loadConfig`, which has no phase context and is called by nearly every
* command), then the pre-existing explicit `commit_docs` (tier 2), `.gitignore`
* auto-detect (tier 3), and manifest default (tier 4). Tiers 2-4 are byte-for-
* behaviour identical to the pre-#3587 inline checks (epic #2292 AC4): when no
* phase override applies, `resolved` matches exactly what those checks computed
* and `source` merely labels which of the three decided it.
*
* `isPlanningGitIgnored` is a thunk, not a plain boolean, so the pre-existing
* short-circuit is preserved byte-for-behaviour: the original inline checks
* only ever ran `isGitIgnored` (a real `git check-ignore` subprocess) when
* `commit_docs` was truthy, and a phase override or an explicit `commit_docs:
* false` must keep skipping that call entirely, not just its result. Passing
* a thunk also keeps this function pure and directly property-testable
* (test matrix F1) without spawning git.
*/
function resolveCommitDocsPolicy(
config: Record<string, unknown>,
phaseNum: string | null,
isPlanningGitIgnored: () => boolean,
): CommitDocsResolution {
const phaseOverride = resolvePhaseCommitDocsOverride(config, phaseNum);
if (phaseOverride !== undefined) return { resolved: phaseOverride, source: 'phase' };
if (!config['commit_docs']) return { resolved: false, source: 'config' };
if (isPlanningGitIgnored()) return { resolved: false, source: 'gitignore' };
return { resolved: true, source: 'default' };
}
// Reason string per commit_docs-resolution source, for the tier-1/tier-2 skip
// envelope below. `phase` gets its OWN reason (`skipped_commit_docs_phase_false`)
// rather than reusing `skipped_commit_docs_false` — telling a user "commit_docs
// is false" when their project setting is actually `true` would be actively
// misleading (design "Rejected" §3). `config` keeps the pre-existing string
// unchanged: `agents/gsd-executor.md` pattern-matches on it (D2).
const COMMIT_DOCS_SKIP_REASON: Record<Exclude<CommitDocsSource, 'default'>, string> = {
phase: 'skipped_commit_docs_phase_false',
config: 'skipped_commit_docs_false',
gitignore: 'skipped_gitignored',
};
function cmdCommit(cwd: string, message: string | undefined, files: string[] | undefined, raw: boolean, amend: boolean, noVerify: boolean): void {
if (!message && !amend) {
error('commit message required');
@@ -1079,19 +1152,24 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
const config = loadConfig(cwd);
// Check commit_docs config
// Check commit_docs config — #3587: resolved through the tier 1
// (phase_commit_docs.<phase-id>) → tier 2 (commit_docs) → tier 3 (.gitignore)
// → tier 4 (default) precedence chain; see resolveCommitDocsPolicy above.
// `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, 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, skipped: true, hash: null, reason: 'skipped_gitignored' };
const commitDocsPolicy = resolveCommitDocsPolicy(
config,
detectPhaseNumberFromFiles(files),
() => isGitIgnored(cwd, '.planning'),
);
if (!commitDocsPolicy.resolved) {
const result = {
committed: false,
skipped: true,
hash: null,
reason: COMMIT_DOCS_SKIP_REASON[commitDocsPolicy.source as Exclude<CommitDocsSource, 'default'>],
};
output(result, raw, 'skipped');
return;
}
@@ -2395,6 +2473,10 @@ export = {
cmdResolveGranularity,
cmdResolveExecution,
cmdEffortSync,
detectPhaseNumberFromFiles,
resolvePhaseCommitDocsOverride,
resolveCommitDocsPolicy,
COMMIT_DOCS_SKIP_REASON,
cmdCommit,
cmdCommitToSubrepo,
cmdPrSubrepo,

7
src/config-loader.cts
View File

@@ -862,6 +862,13 @@ function loadConfigResolved(cwd: string, options: Record<string, unknown> = {}):
fast_mode: (parsed['fast_mode']) || null,
agent_skills: (parsed['agent_skills']) || {},
agent_skills_security: (parsed['agent_skills_security']) || null,
// #3587: phase_commit_docs.<phase-id> — a dynamic-key family shaped like
// agent_skills above (`{ "<phase-id>": boolean }`). Must be threaded here
// explicitly: `_baseConfig` is a hand-maintained allowlist, so a key that
// is only in config-schema.manifest.json's dynamicKeyPatterns (and not
// projected here) is silently dropped on read — the exact `features`-key
// failure mode this module's own A3 test guards against.
phase_commit_docs: (parsed['phase_commit_docs']) || {},
manager: (parsed['manager']) || {},
response_language: get('response_language') || null,
claude_md_path: get('claude_md_path') || null,

2
src/config.cts
View File

@@ -727,7 +727,7 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string |
validateKnownConfigKeyPath(kp);
if (!isValidConfigKey(kp, cwd)) {
error(`Unknown config key: "${kp}". Valid keys: ${[...VALID_CONFIG_KEYS].sort().join(', ')}, agent_skills.<agent-type>, features.<feature_name>`, ERROR_REASON.CONFIG_INVALID_KEY);
error(`Unknown config key: "${kp}". Valid keys: ${[...VALID_CONFIG_KEYS].sort().join(', ')}, agent_skills.<agent-type>, features.<feature_name>, phase_commit_docs.<phase-id>`, ERROR_REASON.CONFIG_INVALID_KEY);
}
// Parse value (handle booleans, numbers, and JSON arrays/objects)