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

enhance(#3913): docs, and the guards come down (#3994)

Browse Source 
ADR-3889 terminal phase. Generated docs/reference/exit-codes.md from the exit-code declaration with a --check drift arm; deleted the inert soft-error-exit-zero oracle; promoted untyped-success from SMELL to VIOLATION so it can fail a build; pruned all 5 smell-baseline entries.

Fixed inline: two mis-scoped oracles (routing-validity, value-hygiene), a second source behind the band table, unescaped declaration strings reaching Markdown, and a pre-existing Windows 8.3 short-name path-comparison defect.

Guard ledger corrected from a claimed net -4 to a measured net -1.

Closes #3913
This commit is contained in:
Tom Boucher
2026-08-28 13:10:20 -04:00
committed by GitHub
parent c3e667df34
commit 12f9d1d9a0
16 changed files with 1517 additions and 152 deletions
Download Patch File Download Diff File Expand all files Collapse all files

114
tests/qa/oracles.cjs
View File

@@ -65,6 +65,12 @@
const nodePath = require('node:path');
const { KIND } = require('./result.cjs');
const { resolveForCompare, isUnderProjectDir } = require('./paths.cjs');
const {
LIVE_COMMAND_TOKEN_PREFIXES,
getLiveCommandTokens,
firstToken,
isLiveCommandToken,
} = require('../helpers/live-command-registry.cjs');
/** Severity of a failed oracle outcome. A SMELL is evidence, not a verdict — it never fails a build. */
const SEVERITY = Object.freeze({ VIOLATION: 'violation', SMELL: 'smell' });
@@ -92,6 +98,20 @@ const SENTINEL_STRINGS = new Set(['undefined', 'null', 'NaN', '[object Object]']
*/
const EXTERNAL_PATH_ALLOWED_KEYS = Object.freeze(new Set(['agents_dir']));
/**
* Strip the walk()-root prefix (`"$."`) from a `walk()`-built path, e.g.
* `"$.next.command"` -> `"next.command"`, `"$.actions[0].command"` ->
* `"actions[0].command"` — the field-naming convention `routing-validity`'s
* `subject.key` has always used, kept stable across the switch to a generic
* `walk()`-based scan (#3913 P9 SEC-2).
*
* @param {string} walkPath
* @returns {string}
*/
function stripRootPrefix(walkPath) {
return walkPath.startsWith('$.') ? walkPath.slice(2) : walkPath;
}
/**
* Extract the leaf key name from a `walk()`-built path (e.g. `"$.agents_dir"`
* -> `"agents_dir"`, `"$.foo.bar[3]"` -> `"bar"` for the array element itself,
@@ -313,8 +333,13 @@ const ORACLES = Object.freeze([
'result.json must not contain a NaN number or a string exactly equal to a coercion-artifact sentinel ' +
'(VIOLATION). When ctx.projectDir is supplied, an absolute-path string outside it is reported as a SMELL — ' +
'never a violation — UNLESS its leaf key name is in EXTERNAL_PATH_ALLOWED_KEYS (a field whose contract is to ' +
'point outside the project, e.g. agents_dir), which is skipped entirely: not a smell, not a violation. Without ' +
'ctx.projectDir the path check is skipped rather than guessed.',
'point outside the project, e.g. agents_dir) or the string\'s FIRST WHITESPACE-DELIMITED WORD is an EXACT ' +
'member of getLiveCommandTokens() (e.g. `/gsd:progress`, or `/gsd-plan-phase 2` where the token carries ' +
'trailing arguments — see isLiveCommandToken in tests/helpers/live-command-registry.cjs, the single predicate ' +
'shared with routing-validity), either of which is skipped entirely: not a smell, not a violation. Exact ' +
'membership, never a `startsWith` prefix test with an unconstrained remainder — a string merely SHARING a ' +
'prefix with a real token (e.g. `/gsd-x/../../../etc/passwd`) is not exempted and still surfaces as a SMELL. ' +
'Without ctx.projectDir the path check is skipped rather than guessed.',
check(ctx) {
try {
/** @type {{message: string, key: string, value: unknown}[]} */
@@ -327,6 +352,9 @@ const ORACLES = Object.freeze([
// Resolved once per runOracles call (this check runs exactly once per ctx), not once
// per candidate string below — projectDir is the same value for every leaf in the walk.
const resolvedProjectDir = projectDir !== null ? resolveForCompare(projectDir) : null;
// Resolved once per check() call, not once per leaf — getLiveCommandTokens() is itself
// memoized, but there is no reason to re-look-up the memo per candidate string.
const liveTokens = getLiveCommandTokens();
walk(json, (leaf, path) => {
if (typeof leaf === 'number' && Number.isNaN(leaf)) {
violations.push({ message: `NaN at ${path}`, key: path, value: leaf });
@@ -337,7 +365,8 @@ const ORACLES = Object.freeze([
typeof leaf === 'string' &&
nodePath.isAbsolute(leaf) &&
!isUnderProjectDir(resolveForCompare(leaf), resolvedProjectDir) &&
!EXTERNAL_PATH_ALLOWED_KEYS.has(leafKeyOf(path))
!EXTERNAL_PATH_ALLOWED_KEYS.has(leafKeyOf(path)) &&
!isLiveCommandToken(leaf, liveTokens)
) {
smells.push({
message: `absolute path ${JSON.stringify(leaf)} at ${path} is outside ctx.projectDir ${JSON.stringify(projectDir)}`,
@@ -523,25 +552,44 @@ const ORACLES = Object.freeze([
Object.freeze({
id: 'routing-validity',
describe:
'A result.json.recommended / recommended_command token must name a command present in ctx.liveCommands.',
'Every command token actually carried by the payload must name a command present in ctx.liveCommands. ' +
'Rather than enumerating fixed field paths (`actions[].command` / `next.command`), this walks the ENTIRE ' +
'payload for any string whose first whitespace-delimited word starts with one of LIVE_COMMAND_TOKEN_PREFIXES ' +
'(`/gsd-`, `/gsd:`, `$gsd-`) — catching the token wherever it is actually carried: `recommended_command`, a ' +
'bare-string `next`, `next` as an array of `{command}`, `actions` as an object map, `steps[].command`, ' +
'`actions[].next.command`, etc (#3913 P9 SEC-2 — the fixed-path version missed all of these). A candidate ' +
'passes only when its first word is an EXACT member of ctx.liveCommands (via the shared ' +
'`isLiveCommandToken` predicate, tests/helpers/live-command-registry.cjs — the SAME predicate ' +
'value-hygiene\'s command-token exemption uses, so the two can never drift on what a live token is); a ' +
'pleasing consequence is that value-hygiene therefore exempts exactly the strings routing-validity vouches ' +
'for. A bare `recommended` id (no `/gsd-`-style prefix — e.g. `discuss-phase`, by design ' +
'`src/smart-entry.cts` `actions.find(a => a.recommended)?.id`, an action id, not a command token) is never a ' +
'candidate. Engages only when at least one prefix-shaped string is present; a payload with no such string is ' +
'not engaged (ok).',
check(ctx) {
try {
const json = ctx && ctx.result ? ctx.result.json : undefined;
if (json === null || typeof json !== 'object' || Array.isArray(json)) return { ok: true };
const field = Object.prototype.hasOwnProperty.call(json, 'recommended')
? 'recommended'
: Object.prototype.hasOwnProperty.call(json, 'recommended_command')
? 'recommended_command'
: null;
if (field === null) return { ok: true };
const value = json[field];
if (typeof value !== 'string') return { ok: true };
const liveCommands = ctx && Array.isArray(ctx.liveCommands) ? ctx.liveCommands : [];
if (!liveCommands.includes(value)) {
const liveTokens = new Set(liveCommands);
const seen = new WeakSet();
/** @type {{field: string, value: string} | null} */
let violation = null;
walk(json, (leaf, path) => {
if (violation !== null || typeof leaf !== 'string') return;
const word = firstToken(leaf);
const looksLikeCommandToken = LIVE_COMMAND_TOKEN_PREFIXES.some((prefix) => word.startsWith(prefix));
if (!looksLikeCommandToken) return;
if (!isLiveCommandToken(leaf, liveTokens)) {
violation = { field: stripRootPrefix(path), value: leaf };
}
}, seen, '$');
if (violation !== null) {
return {
ok: false,
severity: SEVERITY.VIOLATION,
detail: `${field}=${JSON.stringify(value)} is not in ctx.liveCommands`,
subject: { key: violation.field, value: violation.value },
detail: `${violation.field}=${JSON.stringify(violation.value)} is not in ctx.liveCommands`,
};
}
return { ok: true };
@@ -571,38 +619,14 @@ const ORACLES = Object.freeze([
},
}),
Object.freeze({
id: 'soft-error-exit-zero',
describe:
'SMELL, not a violation: result.kind === KIND.SOFT_ERROR means the operation reported failure through a ' +
'payload key ({error:...}) while exiting 0. Every shell caller\'s `if ! cmd; then` is blind to that failure. ' +
'This is legal today (42 call sites use output({error:...})) and is deliberately NOT changed here — the ' +
'blast radius of changing it is CRITICAL — but it is reported so the trade stays visible instead of invisible.',
check(ctx) {
try {
const result = ctx && ctx.result;
if (!result || result.kind !== KIND.SOFT_ERROR) return { ok: true };
const argv = Array.isArray(result.argv) ? result.argv : [];
const argvDisplay = argv.length ? argv.join(' ') : '(no argv)';
const errorValue = result.json && typeof result.json === 'object' ? result.json.error : undefined;
return {
ok: false,
severity: SEVERITY.SMELL,
subject: { argv },
detail: `command "${argvDisplay}" exited 0 but carries result.json.error = ${JSON.stringify(errorValue)}`,
};
} catch (err) {
return { ok: false, severity: SEVERITY.SMELL, detail: `soft-error-exit-zero threw: ${err && err.message}` };
}
},
}),
Object.freeze({
id: 'untyped-success',
describe:
'SMELL, not a violation: result.kind === KIND.PROSE means the command only emits rendered text with no ' +
'typed surface to assert on. CONTRIBUTING.md forbids asserting on rendered text, so a prose-only command is ' +
'permanently unassertable by this harness — the gap is in the product, not the test.',
'VIOLATION: result.kind === KIND.PROSE means the command only emits rendered text with no typed surface to ' +
'assert on. CONTRIBUTING.md forbids asserting on rendered text, so a prose-only command is permanently ' +
'unassertable by this harness. This is enforced, not merely tracked: the corpus PROSE count is 0 (#3913) — ' +
'every executed step across every scenario emits a typed surface, so this oracle can now fire without ever ' +
'having fired against the shipped corpus. A future prose-only step reddens the build immediately.',
check(ctx) {
try {
const result = ctx && ctx.result;
@@ -611,12 +635,12 @@ const ORACLES = Object.freeze([
const argvDisplay = argv.length ? argv.join(' ') : '(no argv)';
return {
ok: false,
severity: SEVERITY.SMELL,
severity: SEVERITY.VIOLATION,
subject: { argv },
detail: `command "${argvDisplay}" emits KIND.PROSE only; no typed surface exists to assert on`,
};
} catch (err) {
return { ok: false, severity: SEVERITY.SMELL, detail: `untyped-success threw: ${err && err.message}` };
return { ok: false, severity: SEVERITY.VIOLATION, detail: `untyped-success threw: ${err && err.message}` };
}
},
}),

38
tests/qa/paths.cjs
View File

@@ -26,6 +26,24 @@
const fs = require('node:fs');
const path = require('node:path');
/**
* Realpath implementation used by `realpathNearestAncestor` / `resolveForCompare` by default.
* Prefers `fs.realpathSync.native`, which — unlike plain `fs.realpathSync` — expands Windows
* DOS 8.3 short-name aliases (e.g. `RUNNER~1` -> `runneradmin`). Plain `fs.realpathSync` does
* NOT perform that expansion, so the two forms of the same directory compare as different paths
* (verified from real `windows-latest` CI output: `git_worktree_root` resolved to
* `.../runneradmin/...` while `ctx.projectDir` resolved to `.../RUNNER~1/...` — same directory,
* spelled two ways). Falls back to `fs.realpathSync` when `.native` is unavailable (older Node)
* so this stays a pure enhancement, never a hard dependency.
*
* @param {string} candidate
* @returns {string}
*/
function defaultRealpath(candidate) {
const native = fs.realpathSync && fs.realpathSync.native;
return typeof native === 'function' ? native(candidate) : fs.realpathSync(candidate);
}
/**
* Realpath-resolves `candidate` by walking up to its nearest EXISTING ancestor and rejoining
* the non-existent suffix, rather than requiring the whole path to exist. Plain
@@ -38,18 +56,23 @@ const path = require('node:path');
* (verified: `.planning/NOT-YET.md` under an mkdtemp'd macOS `/var/...` dir was flagged as
* outside a `/private/var/...`-resolved project root). Resolving the nearest existing ancestor
* and rejoining the missing suffix keeps the comparison correct without requiring the leaf to
* exist.
* exist. `.native` throws ENOENT for a missing path exactly like plain `realpathSync`, so this
* walk-up applies identically regardless of which realpath implementation is injected.
*
* @param {string} candidate absolute path (may or may not exist on disk)
* @param {(p: string) => string} [realpathFn] injectable realpath implementation — defaults to
* `defaultRealpath` (`.native`-preferring). Tests use this seam to simulate the Windows 8.3
* short-name case on any platform, since `.native`'s short-name expansion only actually
* occurs on a real Windows host.
* @returns {string} realpath-resolved path, with any non-existent suffix rejoined
*/
function realpathNearestAncestor(candidate) {
function realpathNearestAncestor(candidate, realpathFn = defaultRealpath) {
try {
return fs.realpathSync(candidate);
return realpathFn(candidate);
} catch {
const parent = path.dirname(candidate);
if (parent === candidate) return candidate; // reached a root that itself doesn't exist
return path.join(realpathNearestAncestor(parent), path.basename(candidate));
return path.join(realpathNearestAncestor(parent, realpathFn), path.basename(candidate));
}
}
@@ -69,13 +92,15 @@ function realpathNearestAncestor(candidate) {
* will regress it.
*
* @param {string} candidate absolute path (may or may not exist on disk)
* @param {(p: string) => string} [realpathFn] injectable realpath implementation, forwarded to
* `realpathNearestAncestor` — see that function's doc for why this seam exists.
* @returns {string} realpath-resolved (nearest-ancestor fallback) path, backslash-normalized
*/
function resolveForCompare(candidate) {
function resolveForCompare(candidate, realpathFn = defaultRealpath) {
// Unconditional: backslash-separated path strings can arrive as *data* (e.g. a
// Windows-style path embedded in JSON) even when running on Linux/macOS, not only when
// `fs.realpathSync` itself returns a drive-letter path on native Windows.
return realpathNearestAncestor(candidate).replace(/\\/g, '/');
return realpathNearestAncestor(candidate, realpathFn).replace(/\\/g, '/');
}
/**
@@ -224,4 +249,5 @@ module.exports = {
isUnderProjectDir,
isAbsoluteLike,
hasTraversalSegment,
defaultRealpath,
};

2
tests/qa/scenarios/greenfield-happy-path.json
View File

@@ -31,7 +31,7 @@
},
{
"at": "plan:post",
"run": [["smart-entry"]]
"run": [["smart-entry", "--json"]]
},
{
"at": "execute:pre",

38
tests/qa/smell-baseline.json
View File

@@ -1,40 +1,4 @@
{
"version": 1,
"smells": [
{
"key": "0cad2954807d::soft-error-exit-zero|greenfield-happy-path|state-snapshot|(subject-with-no-stable-discriminator)",
"id": "soft-error-exit-zero",
"scenario": "greenfield-happy-path",
"issue": 2980,
"reason": "state-snapshot uses the documented soft-failure idiom (output({error:...}) while exiting 0) when STATE.md does not exist yet on a greenfield project. 42 call sites across the engine use this idiom; oracles.cjs rates changing it CRITICAL blast radius — tracked as a known trade-off in #2980."
},
{
"key": "373270350622::soft-error-exit-zero|perturbation-delete-artifact|roadmap get-phase 1|(subject-with-no-stable-discriminator)",
"id": "soft-error-exit-zero",
"scenario": "perturbation-delete-artifact",
"issue": 2980,
"reason": "`roadmap get-phase 1` reports ROADMAP.md not found via the same output({error:...})-exit-0 soft-failure idiom after the perturbation deletes the artifact — same known trade-off as the state-snapshot occurrences above, tracked in #2980."
},
{
"key": "383aca0b3b13::soft-error-exit-zero|perturbation-unicode-headings|roadmap get-phase 1|(subject-with-no-stable-discriminator)",
"id": "soft-error-exit-zero",
"scenario": "perturbation-unicode-headings",
"issue": 2980,
"reason": "Same roadmap get-phase soft-failure idiom as perturbation-delete-artifact, fired here because the unicode-heading perturbation leaves the phase unresolvable — tracked in #2980."
},
{
"key": "8ff1a5363a3f::untyped-success|greenfield-happy-path|smart-entry|(subject-with-no-stable-discriminator)",
"id": "untyped-success",
"scenario": "greenfield-happy-path",
"issue": 2979,
"reason": "smart-entry emits KIND.PROSE unconditionally, with no typed JSON surface for the harness to assert on — a real product gap tracked in #2979, not a QA-harness defect."
},
{
"key": "92b6b4718571::soft-error-exit-zero|out-of-order|state-snapshot|(subject-with-no-stable-discriminator)",
"id": "soft-error-exit-zero",
"scenario": "out-of-order",
"issue": 2980,
"reason": "Same state-snapshot soft-failure idiom as greenfield-happy-path, fired here because the out-of-order walk reaches state-snapshot before STATE.md exists — tracked in #2980."
}
]
"smells": []
}