* feat(#3970): per-task external-tracker content-resolution seam Implements ADR-3646 (Phase 1, #3970): a `<task tracker-id="...">` attribute plus a new optional `taskContentResolver` capability-manifest field let a capability resolve a task's action/verify/acceptance-criteria/read_first/done content from an external issue tracker instead of PLAN.md's inline body. - src/plan-document.cts: parses the `tracker-id` attribute into `PlanTask.trackerId` - src/task-content-resolution.cts: new leaf module — split/find/build/resolve, with a hard-halt (throw) contract on ambiguous/failed/timeout/malformed resolution, never a silent fallback to possibly-stale inline text - src/task-command-router.cts: new `task resolve-content --plan --task-id --raw` CLI verb wiring the module into a real process exit code - gsd-core/bin/lib/capability-validator.cjs: validates the new `taskContentResolver` manifest field (feature-role only, cross-capability trackerPrefix uniqueness) - gsd-core/workflows/execute-plan.md, gsd-core/references/loop-hook-dispatch.md, docs/reference/capability-manifest.md: wire the seam into the per-task loop and document it as a new `execute:task` point outside the existing contribution/step/gate vocabulary (unconditional in autonomous mode) Closes #3970 * fix(#3970): gate checkpoint tasks out of content resolution, close trackerPrefix grammar parity gap, cover path-traversal guard Standards/Spec code-review pass on the task-content-resolution seam (ADR-3646 Phase 1) found three defects: 1. execute-plan.md's task-content-resolution bullet fired on any tracker-id-bearing task with no check that it wasn't type="checkpoint:*", contradicting ADR-3646 Decision 1 (a checkpoint task must never enter resolve-content). plan-document.cts already parses trackerId: null unconditionally for checkpoint tasks; only the workflow prose needed the fix, so the bullet now explicitly excludes checkpoint tasks. 2. task-content-resolution.cts's parseResolverDeclaration accepted any non-empty trackerPrefix with no grammar check, while capability- validator.cjs's KEBAB_RE enforces kebab-case at install time — a Generative Fix Divergence gap. Added the same grammar (as a literal regex, documented as intentionally not shared across the .cts/.cjs build boundary) plus a parity test asserting the two surfaces agree across a valid/invalid trackerPrefix table. 3. task-command-router.cts's routeResolveContent path-traversal guard on --plan had zero test coverage. Added a test exercising a ../../../etc/passwit-shaped path and asserting the USAGE rejection names the offending path. * fix(#3970): sanitize resolver diagnostics and cap resolver timeoutMs Two findings caught by an isolated security-review pass on the task content resolution seam: - ResolverFailedError/ResolverMalformedOutputError embedded raw, unsanitized subprocess stderr/stdout (attacker/model-influenced via the tracker-id argv token) into .message. A hostile or buggy resolver could smuggle a newline plus a forged "Error: " line, or terminal escape sequences, into a diagnostic io.cjs's error() writes verbatim to stderr. Fixed at the constructor (task-content-resolution.cts) via io.cjs's existing formatDiagnosticToken(), so every caller of resolveTaskContent gets a safe .message by construction. - capability-validator.cjs's validateTaskContentResolverFields had no upper bound on taskContentResolver.invoke.timeoutMs, letting a manifest declare an effectively unbounded value and defeat the "bounded subprocess" design intent. Added a 120000ms ceiling specific to this field, without touching the shared isPositiveIntegerMs() helper (still used unbounded by the reviewer lane's timeoutFloorMs and probe timeoutMs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3970): fix gsd-test failures — stale prose allowlist line and stderr-vs-message assertion gsd-test (remote dockerized matrix) came back red with 5 failures on this PR; all five are real defects, fixed here. - tests/no-bare-gsd-tools-command-position.test.cjs: PROSE_ALLOWLIST's execute-plan.md entry pointed at line 415, which ffc190df4's checkpoint-exclusion caveat (added near line 221) shifted down by one line. The actual "validated downstream by gsd-tools uat classify-coverage" descriptive mention now sits at line 416. Updated the allowlist entry's line number to match. - tests/task-command-router-resolve-content.test.cjs: the path-traversal test asserted the outside-project-scope diagnostic against the thrown ExitError's own .message. io.cts's error() (ADR-3889) writes its human-readable message to fd 2 via writeAllSync and then throws a bare `new ExitError(1)` with no message argument — by design, so the exception carries no duplicate text and the thrown ExitError's message defaults to "process exit 1" (cli-exit.cts's ExitError constructor). Root cause was the test, not the source: task-command-router.cjs's outside-project-scope rejection already calls error() correctly and the diagnostic text is genuinely emitted, just on fd 2, not on the exception. Fixed the test to capture fd-2 writes (mirroring tests/estimate-calibrate.test.cjs's runCalibrateExpectError and this same file's own captureStdout for fd 1) and assert against the captured stderr text instead of err.message. This was masked locally because a manual `node -e` sanity check that only inspects the caught exception's .message cannot see what the real node:test run actually failed on. Emitted-Drift-Ack-Growth: execute-plan.md — adds the ADR-3646 task-content-resolution bullet and checkpoint-exclusion caveat to the per-task execute loop; a real behavioral prose addition, not incidental bloat. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3970): backfill changeset PR number (pr:0 -> pr:4000) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
215 lines
11 KiB
JavaScript
215 lines
11 KiB
JavaScript
// allow-test-rule: source-text-is-the-product (#2751)
|
|
'use strict';
|
|
|
|
// Regression guard for #2751: agents/*.md and gsd-core/workflows/*.md must not
|
|
// instruct an agent to invoke a BARE `gsd-tools <verb> <args>` command. The bare
|
|
// word fails with "command not found" on a shim-only install (no `gsd-tools`
|
|
// binary on PATH) — every such instruction must use the `gsd_run` resolver the
|
|
// same files already define. #725 fixed this only for the Codex install-
|
|
// conversion pipeline; the Claude-facing SOURCE shipped the bare calls verbatim
|
|
// until #2751 normalized them.
|
|
//
|
|
// A pure regex cannot perfectly distinguish an imperative ("Use `gsd-tools query
|
|
// commit` to commit") from a descriptive mention ("`gsd-tools query commit`
|
|
// returns an envelope") — both contain the same command phrase. So this guard
|
|
// scans for `gsd-tools <verb> <arg>` (the operative shape — a verb followed by
|
|
// its arguments) and subtracts a documented ALLOWLIST of known descriptive
|
|
// mentions that NAME the command without instructing literal invocation. Any
|
|
// site NOT in the allowlist is a new operative bare call and fails the gate.
|
|
//
|
|
// Verb coverage is derived LIVE from the gsd-tools host-command router
|
|
// (`gsd-core/bin/gsd-tools.cjs`) by reading the `'verb': routeHandler` / `verb:
|
|
// routeHandler` entries. This is deliberate — the original #2751 guard used a
|
|
// hand-maintained 6-verb list that silently false-passed `verify-summary` (the
|
|
// `verify` branch matched the prefix, then died on the hyphen), and missed
|
|
// `windows`, `worktree`, `smart-entry`, and `quick-tasks-append` entirely.
|
|
// Deriving the set from the router means a new verb registered there is covered
|
|
// here the moment it lands — no second list to keep in sync.
|
|
//
|
|
// Source-text guard: the deployed contract IS the markdown text the runtime
|
|
// loads. Scans FULL file text (the real hits lived in inline prose/table cells,
|
|
// not fenced bash blocks).
|
|
|
|
const { test } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fs = require('node:fs');
|
|
const path = require('node:path');
|
|
|
|
const ROOT = path.resolve(__dirname, '..');
|
|
const ROUTER_PATH = path.join(ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs');
|
|
// #3809 widened this from agents/ + workflows/ to include gsd-core/references/,
|
|
// which was carrying 37 bare calls the guard simply never looked at.
|
|
//
|
|
// commands/ is deliberately NOT here, and that is a finding rather than an
|
|
// oversight. Its files cannot use the shared launcher the way workflows and agents
|
|
// do: tests/graphify-visualization.test.cjs extracts individual Step-3 shell chains
|
|
// and runs them standalone, so each fenced block needs its OWN preamble —
|
|
// graphify.md carries five on purpose, and collapsing them to one produces
|
|
// `gsd_run: command not found` (exit 127). tests/gsd-tools-path-refs.test.cjs
|
|
// (#1766) separately pins commands/gsd/workstreams.md to the literal string
|
|
// `gsd-tools query workstream.list`. Bringing commands/ under this guard therefore
|
|
// needs those two contracts reconciled first; it is not a scan-set widening.
|
|
//
|
|
// skills/ is absent for a different reason: it is generated from commands/ by
|
|
// scripts/gen-plugin-skills.cjs and pinned by lint:generated-sync, so guarding the
|
|
// source guards both, and scanning the generated mirror would double-report every
|
|
// future offender.
|
|
const SCAN_DIRS = [
|
|
'agents',
|
|
path.join('gsd-core', 'workflows'),
|
|
path.join('gsd-core', 'references'),
|
|
];
|
|
|
|
// Derive the verb set the bare-call guard matches against. Most top-level
|
|
// verbs live in the host-command router table as `'verb': routeHandler` entries
|
|
// (~70); this reads those dynamically so new router verbs are covered the moment
|
|
// they land. A handful of verbs are dispatched as FAMILIES (their own
|
|
// `command === 'verb'` arm, not a route-table entry): `query` (line ~2876),
|
|
// `intel`, `verify`, and `graphify`. These are stable, documented families, so
|
|
// they are supplemented explicitly here rather than parsed from the help string
|
|
// (whose prose mixes real verbs with English words like "for"/"output"/"working",
|
|
// producing noise). If a family verb is ever promoted into the route table the
|
|
// union dedupes harmlessly; if a NEW family verb is added it must be added here.
|
|
//
|
|
// Sorted longest-first so a hyphenated verb (`verify-summary`) is preferred over
|
|
// its prefix (`verify`) — the exact ordering bug that let `verify-summary` slip
|
|
// past a fixed 6-verb list during the first #2751 pass.
|
|
const FAMILY_DISPATCHED_VERBS = ['query', 'intel', 'verify', 'graphify'];
|
|
function readRouterVerbs() {
|
|
const src = fs.readFileSync(ROUTER_PATH, 'utf8');
|
|
const re = /(?:'([a-z][a-z-]*)'|([a-z][a-z-]*))\s*:\s*route[A-Z]\w*/g;
|
|
const verbs = new Set(FAMILY_DISPATCHED_VERBS);
|
|
let m;
|
|
while ((m = re.exec(src)) !== null) verbs.add(m[1] || m[2]);
|
|
return [...verbs].sort((a, b) => b.length - a.length);
|
|
}
|
|
|
|
const VERBS = readRouterVerbs();
|
|
|
|
// Operative shape: `gsd-tools <known-verb>` followed by whitespace and a real
|
|
// argument (the verb is NOT the close of a code span — there is a real arg
|
|
// after it). Restricting to KNOWN verbs (vs any `[a-z-]+` token) avoids
|
|
// false-flagging English prose like "gsd-tools through the ...".
|
|
const BARE_COMMAND_RE = new RegExp(
|
|
String.raw`(?:^|[^./A-Za-z0-9_-])gsd-tools\s+(` + VERBS.join('|') + String.raw`)\s+[^\s` + '`' + String.raw`]`
|
|
);
|
|
|
|
// Lines that legitimately embed `gsd-tools <verb> <arg>` but are descriptive, not
|
|
// command-position: they NAME the command (in prose / parenthetical examples /
|
|
// return-envelope descriptions) rather than instructing an agent to run the bare
|
|
// word. Keyed `file:line` so a rewording that moves the mention forces a conscious
|
|
// allowlist update rather than silently passing.
|
|
//
|
|
// Each entry MUST carry a one-line reason; the test prints the allowlist on
|
|
// failure so a reviewer can see exactly what is sanctioned.
|
|
const PROSE_ALLOWLIST = [
|
|
{ file: 'agents/gsd-executor.md', line: 795, reason: 'describes the SDK return envelope of `gsd-tools query commit`; not an instruction to run the bare word' },
|
|
{ file: 'agents/gsd-phase-researcher.md', line: 33, reason: 'package-legitimacy provenance rule names the command as the source of an OK verdict; descriptive' },
|
|
{ file: 'agents/gsd-roadmapper.md', line: 642, reason: 'parenthetical "e.g." naming SDK queries a user *could* run; not an agent instruction' },
|
|
{ file: 'agents/gsd-intel-updater.md', line: 40, reason: 'cross-platform note names the `gsd-tools intel <subcommand>` CLI surface descriptively ("CLI invocations go through..."); not an agent instruction' },
|
|
{ file: 'gsd-core/workflows/execute-plan.md', line: 416, reason: 'describes the downstream SDK validation step (`validated downstream by ...`); names the mechanism, does not instruct the agent to type it' },
|
|
];
|
|
|
|
// Resolver-snippet definition lines / probes that must never be flagged. A line
|
|
// is a resolver/probe definition when it assigns the shim name, probes PATH, or
|
|
// assigns GSD_TOOLS / defines the gsd_run function — NOT merely when it contains
|
|
// the substring `gsd-tools.cjs` (which would blanket-exempt any prose that
|
|
// happens to name the file).
|
|
const EXCLUSION_RE = /_GSD_SHIM_NAME\s*=|command -v gsd-tools|\bGSD_TOOLS\s*=|gsd_run\s*\(\)\s*\{/;
|
|
|
|
function collectMdFiles(dir) {
|
|
const results = [];
|
|
let entries;
|
|
try { entries = fs.readdirSync(dir, { withFileTypes: true }); } catch { return results; }
|
|
for (const entry of entries) {
|
|
const full = path.join(dir, entry.name);
|
|
if (entry.isDirectory()) {
|
|
results.push(...collectMdFiles(full));
|
|
} else if (entry.isFile() && entry.name.endsWith('.md')) {
|
|
results.push(full);
|
|
}
|
|
}
|
|
return results;
|
|
}
|
|
|
|
function findBareCommandPositionCalls() {
|
|
const offenders = [];
|
|
for (const scanDir of SCAN_DIRS) {
|
|
const abs = path.join(ROOT, scanDir);
|
|
for (const file of collectMdFiles(abs)) {
|
|
// Normalize to forward slashes so the file:line PROSE_ALLOWLIST keys match
|
|
// identically on every OS — path.relative() returns backslash separators on
|
|
// Windows, which would defeat the allowlist lookup and false-flag the 6
|
|
// descriptive mentions (#2751 Windows CI failure).
|
|
const rel = path.relative(ROOT, file).split(path.sep).join('/');
|
|
const lines = fs.readFileSync(file, 'utf8').split(/\r?\n/);
|
|
for (let i = 0; i < lines.length; i += 1) {
|
|
const line = lines[i];
|
|
if (EXCLUSION_RE.test(line)) continue;
|
|
const match = line.match(BARE_COMMAND_RE);
|
|
if (!match) continue;
|
|
const loc = `${rel}:${i + 1}`;
|
|
const allowed = PROSE_ALLOWLIST.find((a) => a.file === rel && a.line === i + 1);
|
|
if (allowed) continue;
|
|
offenders.push({ loc, verb: match[1], text: line.trim() });
|
|
}
|
|
}
|
|
}
|
|
return offenders;
|
|
}
|
|
|
|
test('verb set was derived from the router (guards against a silent extraction regression)', () => {
|
|
// If the router is refactored so the `routeHandler` pattern no longer matches,
|
|
// VERBS would be empty and the bare-call guard below would false-pass
|
|
// everything. Sanity-bound it: a healthy router exposes many host verbs.
|
|
assert.ok(
|
|
VERBS.length > 40,
|
|
`Expected to derive >40 verbs from ${path.relative(ROOT, ROUTER_PATH)}, got ${VERBS.length}. ` +
|
|
'If the router table changed shape, update readRouterVerbs() so the guard keeps coverage.'
|
|
);
|
|
// Longest-first ordering is what lets verify-summary win over verify.
|
|
const sample = ['verify-summary', 'verify', 'query', 'intel', 'graphify', 'windows', 'worktree', 'smart-entry', 'commit', 'check'];
|
|
for (const v of sample) {
|
|
assert.ok(VERBS.includes(v), `expected verb '${v}' in derived set (router/family drift?)`);
|
|
}
|
|
});
|
|
|
|
test('no command-position bare gsd-tools <verb> survives in agents/ or workflows/ (#2751)', () => {
|
|
const offenders = findBareCommandPositionCalls();
|
|
assert.strictEqual(
|
|
offenders.length,
|
|
0,
|
|
'Bare `gsd-tools <verb> <args>` command-position calls must use the `gsd_run` ' +
|
|
'resolver (they fail with "command not found" on a shim-only install — #2751). ' +
|
|
`Found ${offenders.length} offender(s):\n` +
|
|
offenders.map((o) => ` ${o.loc} [${o.verb}] ${o.text}`).join('\n') +
|
|
'\n\nIf a hit is a descriptive prose mention (not an instruction to run the bare ' +
|
|
'word), add it to PROSE_ALLOWLIST in this test with a reason.'
|
|
);
|
|
});
|
|
|
|
test('every PROSE_ALLOWLIST entry still matches a real gsd-tools mention (no stale allowlist entries)', () => {
|
|
// An allowlist entry that no longer matches anything is stale — it was either
|
|
// fixed (remove it) or the line moved (update it). Either way it must not linger.
|
|
const stale = [];
|
|
for (const entry of PROSE_ALLOWLIST) {
|
|
const file = path.join(ROOT, entry.file);
|
|
let lines;
|
|
try { lines = fs.readFileSync(file, 'utf8').split(/\r?\n/); } catch (e) {
|
|
stale.push({ ...entry, problem: 'file missing' });
|
|
continue;
|
|
}
|
|
const line = lines[entry.line - 1];
|
|
if (!line || !BARE_COMMAND_RE.test(line) || EXCLUSION_RE.test(line)) {
|
|
stale.push({ ...entry, problem: 'line no longer matches a bare gsd-tools mention', actual: line ? line.trim() : '(line absent)' });
|
|
}
|
|
}
|
|
assert.strictEqual(
|
|
stale.length,
|
|
0,
|
|
'PROSE_ALLOWLIST has stale entries (the mentioned line no longer carries a bare ' +
|
|
'`gsd-tools <verb> <arg>` mention). Remove or update them:\n' +
|
|
stale.map((s) => ` ${s.file}:${s.line} — ${s.problem}${s.actual ? ` (actual: ${s.actual.slice(0, 80)})` : ''}`).join('\n')
|
|
);
|
|
});
|