53ea8e0664
* fix(#3057): refuse the write when the duplicate scan cannot complete writeManifest documents itself as a fail-closed duplicate guard: if any existing manifest shares plan_id with a different, non-terminal job_id it must refuse, because dispatching again would duplicate the external job. It could not honour that. The scan reads every sibling manifest looking for the duplicate, and an unreadable or unparseable sibling was `continue`d past. If the corrupt file was the one holding the live duplicate, the scan found nothing and a duplicate external job dispatched. The asymmetry is what gives it away: a malformed TARGET refused with malformed_existing because clobbering is unacceptable, while a malformed SIBLING was skipped — yet siblings are the only thing the duplicate check reads. Adds a scan_incomplete verdict that refuses and names the offending file, so an operator can quarantine or repair it. Fail-closed alone would let one stale corrupt manifest wedge every dispatch for that planning dir permanently; naming the file is what makes refusing survivable. malformed_existing is untouched, so the target/sibling distinction stays visible. The docstring is updated — it previously stated a rule the function did not keep. memFs() gains an optional failReads map so these branches are reachable at all; they had zero coverage because the fake could not express a per-file read fault. The signature is additive and every existing caller is unchanged. The regression is proved by a pair, not a single test. A control writes a readable sibling holding a genuine non-terminal duplicate and asserts duplicate_plan_id, establishing the scenario is real; the regression then makes that same path unreadable and asserts scan_incomplete. A first draft of this test used a corrupt-JSON fixture containing no plan_id at all while its comment claimed otherwise — it duplicated the unparseable-sibling case and proved nothing, which is the defect class this phase exists to remove. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): make a guard's failure distinguishable from its benign result Wave 1 of the negative-space backfill: the branches where a guard that could not verify something reported the same value it reports when everything is fine. That indistinguishability is the defect; every fix here makes the two states tellable apart, and every test proves it with a pair — one for the failure, one for the benign case. A single test cannot establish that two states are distinguishable, which is the whole property being fixed. state.cts phaseInventoryProvider returned null for both a real disk-scan failure and a genuinely empty phases dir, so `state rebuild` could report success while phase-table reconciliation never ran. It now returns a discriminated result and the CLI surfaces phase_inventory_scan_failed plus a reason. The reason field turned out never to have been wired into the emitted JSON at all — it existed only as an internal variable — so a test could only assert on the operator-facing note. It is a real field now. state.cts treated an unreadable lock body the same as an empty one, applying the 1-second stealable floor. A lock we cannot read is not a lock we know is stale; an unreadable body is now held to the deadman ceiling like a live holder. verification.cts findStaleVerificationSummary returned null on any fs, scan or clock failure — meaning "not stale". It now returns a discriminated StaleCheckResult and the caller records that the check was indeterminate. git-base-branch resolveBaseBranch returned 'main' both when no candidate branch existed and when every git tier timed out. A diagnostics variant now reports whether the answer was verified, and the CLI writes an unverified-fallback note to stderr. The stdout contract five workflows parse is untouched. worktree-safety snapshotWorktreeInventory left exists:true when statSync threw, so a guard that could not check reported the worktree present; exists is now tri-state and a stat failure surfaces as an 'unverified' finding. planWorktreePrune reported 'no_worktrees' for a parse failure, which is not the same as an empty list — and it drives a prune. It now reports 'parse_failed'. Fixing the inventory change exposed a second fail-open in verify.cts: the validate-health consumer silently dropped findings whose kind it did not recognise, so the new kind would have vanished. That is closed too — worth noting that the survey enumerated producers of degraded verdicts, not consumers that discard them. worktree-base-ref and state-transition gain the distinguishing signal without changing what they do: headAbsenceVerified, and a phase-inventory scan meta. Whether those guards should ACT differently is a product question this change does not answer, and both are flagged rather than quietly settled. rescueSummaryArtifacts is left alone: rescuing on an uncertain cat-file is deliberate per #2556. It now has tests proving it, and a recorded negative finding — git cat-file -e returns 128 for both "absent from HEAD" and a fatal error, so "uncertain" and "certain-and-fine" are not separable at the git level. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): assert typed values, not rendered text Ten assertions in the rebuild CLI suite matched substrings of produced output — STATE.md body fields, a markdown table row, an audit-log heading, and JSON keys read as text. CONTRIBUTING prohibits that: if the code under test produces text, the test asserts on its structured surface instead. No production surface had to be built. Every one already existed and was already compiled into bin/lib: stateExtractField for body fields, parseMarkdownTable for the phase table, collectSection for the audit-log section, and result.data.log — already a typed RebuildLogEntry[]. The tests were matching rendered text sitting next to the structured data. One of those assertions was passing for the wrong reason. `stdout.includes ('rebuilt')` matched the JSON KEY name, not a value: the dry-run path emits `mutated` and the real path emits `rebuilt`, so it would have passed whether the value was true or false. It now asserts the value. external-job's refusal already had to name the offending file — that naming is why the fail-closed variant is survivable rather than a permanent wedge — but the tests proved it by substring of a prose message. The failure result now carries offendingPath as its own field and the tests assert it by value. The human message is unchanged; operators read it. Array membership is left alone. `phaseIds.includes('99')` and `result.updated.includes('Completed Phases')` are membership checks on real arrays, not text matching, and converting them would weaken nothing and clarify nothing. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): execute acquireStateLock instead of grepping its source The non-EEXIST lock test asserted on the TEXT of the built .cjs and never called acquireStateLock. It carried an allow-test-rule: architectural-invariant exemption to permit that. A source grep proves a literal is present in a file, not that the behaviour works — it is weaker than a liveness test, which at least runs the code, and it was the only coverage the fatal-errno path had. Replaced with tests that inject the errno through fs and assert what actually happens: a fatal EACCES propagates out of acquireStateLock with zero backoff sleeps, while EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE/EPERM/EBUSY retry once and succeed. The exemption is removed and its allowlist entry with it. One old assertion is deliberately not carried over: it checked the retryable errnos were expressed as a Set rather than an inline literal. That is a shape check with no runtime signature; the behavioural tests fail if the code reverts to the old inline check, which is the regression it was really guarding. The #3057 lock-body tests move into that same file rather than a new one, which is what lint-test-file-count asks for and puts every acquireStateLock test in one place. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): surface an indeterminate staleness check to its callers An isolated review caught an inconsistency inside this wave. Two of the three "add the distinguishing signal" fixes wire through to something a user sees: git base-branch writes an unverified-fallback diagnostic to stderr, and an unverifiable worktree surfaces as a W020 finding. The third set staleCheckIndeterminate on readVerificationStatus's result and nothing read it. A signal nobody consumes leaves the fail-open exactly as silent as before: the staleness check could fail and the operator saw precisely what they would see if the answer were genuinely "not stale". That is the defect this issue exists to remove, so it is not defensible as scaffolding when its two siblings in the same change already wire through. All five callers now surface it, each through the channel it already had rather than a mechanism imposed uniformly: phase complete adds it to its existing warnings array and, on the blocked path, as an additive note on the error text; init and roadmap carry it as a field on output they already emit; the UAT report carries it without ever gating passed/blockers; workstream inventory takes an injectable writeDiagnostic mirroring the git base-branch idiom, because its return shape had nowhere to hang a per-phase field without rippling the builder's types. The routing decision is unchanged everywhere. What changes is only that a caller and an operator can now tell a failed check from a completed one. That diagnostic carries structured meta rather than being asserted by regex — the default still writes only the human message to stderr, but tests assert phaseDir and reason by value. Two earlier assertions in this branch were converted the same way; this was the last raw-text assertion left. Also records a scope correction: the completePhaseCore guards now compare stateReplaceField's result to the body instead of testing truthiness, so a field whose substitution produced identical text no longer reports as updated. That is a real behaviour fix, not the signal-only change this file was described as carrying, and its tests cover both the changed and unchanged cases. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): bound two heavy subprocesses for a loaded bench, not an idle one The remote matrix surfaced three failures unrelated to this branch's changes. All were bad tests, and a re-run would have hidden every one of them. The reviewer-flags parse block bounded bash -> node -> a full gsd-tools cold start at 5 seconds. On a bench running thirty thousand tests in parallel that is not a hang, it is a busy machine. Raised to 30s, matching the convention sibling suites already use for script invocations, with a comment saying what the budget covers so nobody tightens it back. Two further copies of the same 5-second spawn in the same file had the identical defect and are raised too — they were not in the failure report, but they will be next time. The fragment-propagation test bounded npm run regen:derived — a full build plus eight generators, the heaviest subprocess in the suite — at five minutes, and node22 was killed near the end. The captured output proves it: every generator had written its files and gen:install-tree had emitted all fifteen runtimes before the kill. Raised to fifteen minutes. That failure read as `null !== 0`, which says nothing. status null means killed, not a non-zero exit, and the two want different responses: one is a timeout to size correctly, the other is a real build break. The assertion now distinguishes them and names the signal. Neither test's assertions were weakened and no retry was added. A retry here would suppress exactly the signal the timeout exists to produce. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture fd 1 through the mock tracker, not a raw reassignment The phase suite reported zero test results on both lanes while running for five and a half minutes and exiting 1. No assertion text, no stderr, four events for the whole file: enqueue, start, dequeue, complete. That shape is not a failing assertion — it is the runner being unable to read the child at all, because it parses its event stream from the child's stdout. The cause was the capture helper reassigning fs.writeSync directly. Proven rather than assumed: a standalone probe patched fs.writeSync and called process.stdout.write, and the interception fired only when fd 1 resolved to a FILE, not when it was a pipe. The remote runner captures the event stream to a file, so a helper that was invisible against a pipe swallowed the reporter's own output on the bench. That is also why the two sibling suites wired the same way in this change pass cleanly — they use the mock tracker, the seam io.test.cjs established for this exact function. The helper now uses t.mock.method with an explicit restore after each call, so teardown belongs to node:test rather than a second hand-rolled implementation, and the interception cannot outlive the one synchronous call it wraps even if that call throws. Ten call sites thread the test context through; three test callbacks gained the parameter they lacked. The three B3 tests are untouched — same assertions, same fault injection. Only how the context reaches the helper changed. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture phase-complete output from a subprocess, not fd 1 Two attempts to make in-process fd-1 interception safe both failed on the bench. The suite reported zero test results on either lane while exiting 1 — four events for the whole file — because the runner parses its event stream from the child's stdout, and process.stdout.write routes through fs.writeSync whenever fd 1 resolves to a file, which is how the runner captures. Patching that seam anywhere in a file can therefore destroy the file's own reporting, and tightening the window only moved the runtime from 326s to 125s without recovering a single event. So the interception is gone rather than tuned. The helper now spawns gsd-tools as a real subprocess and reads stdout the way the OS already gives it to us, which is what the rest of the suite does. It asserts the command succeeded before parsing, so a genuine failure can no longer present as a JSON parse error. The two fault-injecting tests could not survive that move as written: a subprocess cannot see a mock installed in the parent. Instead of reinstating the interception they now produce the fault on disk — the summary artifact is created as a dangling symlink, so the staleness check's real statSync throws inside the child. That is a more honest fixture than a mock in any case, since it is a condition a user's tree can actually be in. Skipped on Windows, matching the existing symlink precedent in the write-guard suite. Three further call sites turned out to depend on parent-process writeFileSync mocks the subprocess could not see. Those call the CJS function directly, which is what they always wanted — they never needed stdout at all. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): one name for one signal, one encoding for one distinction Standards review found four things this branch introduced, all of them inconsistencies with itself rather than with the repo. One upstream bit reached its consumers under three names — verification_stale_check_indeterminate in two modules, the same value with "stale" dropped in a third, and stderr only in the fourth. Standardised on the long name wherever it is a field. The workstream inventory keeps its stderr channel, since its return shape has nowhere to hang a per-phase field without rippling the builder's types, but it now says the same word for the same thing. worktree-safety encoded one three-way distinction two ways in a single file: a named union for a finding's kind, and boolean|null for an inventory entry's existence. The second is now a named union too. Two assertions matched human prose because the blocked and non-blocked completion paths carried no typed field for the signal. Both now assert typed values. The first round of this fix added the field but left the regex beside it, which is the banned pattern sitting next to its own replacement; the second removed it and added an assertion on the reason enum so nothing was lost. The remaining two were reasoned away before being fixed, and both reasons were bad. "No typed surface exists" is the condition CONTRIBUTING says to fix by adding one — it took three lines. "The file already does this dozens of times" is not licence to add instance number thirty-one; a convention that violates a documented rule is debt, not precedent. Vocabulary differing across DIFFERENT modules is left alone: CONTEXT.md rejects a single shared result envelope, so per-module shapes are precedented, and a baseline smell does not outrank a documented standard. A census of every line this branch adds to a test file now finds no regex or substring assertion on produced prose: 87 strictEqual, 25 ok (all non-empty or shape guards), 12 equal, 3 throws (all typed err.code predicates), 3 deepStrictEqual, 2 notStrictEqual. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3057): backfill changeset pr number to 3088 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1505 lines
74 KiB
JavaScript
1505 lines
74 KiB
JavaScript
/**
|
|
* Tests for gsd:plan-review-convergence command (#2306)
|
|
*
|
|
* Validates that the command source and workflow contain the key structural
|
|
* elements required for correct cross-AI plan convergence loop behavior:
|
|
* initial planning gate, review agent spawning, CYCLE_SUMMARY contract for
|
|
* unresolved review count extraction, stall detection, escalation gate, and STATE.md update
|
|
* on convergence.
|
|
*
|
|
* v2 additions (#2306-v2):
|
|
* - CYCLE_SUMMARY contract replaces raw grep (prevents false stalls from
|
|
* accumulated REVIEWS.md history across cycles)
|
|
* - workflow.plan_review_convergence config gate (disabled by default)
|
|
* - --ws forwarded to review agent (symmetric with replan agent)
|
|
* - PARTIALLY RESOLVED / FULLY RESOLVED definitions in contract
|
|
* - HIGH_LINES validation warning when HIGH_COUNT > 0 but section absent
|
|
* - Success criteria updated to reflect CYCLE_SUMMARY parsing
|
|
*
|
|
* v3 additions (#724):
|
|
* - CYCLE_SUMMARY includes current_actionable for unresolved actionable MEDIUM/LOW findings
|
|
* - convergence requires HIGH_COUNT == 0 and ACTIONABLE_COUNT == 0
|
|
* - reviews-mode planner/checker prompts require REVIEWS.md feedback to land in PLAN.md
|
|
*/
|
|
|
|
// allow-test-rule: source-text-is-the-product
|
|
// The workflow markdown IS the runtime instruction. Testing its text content
|
|
// tests the deployed contract — if the CYCLE_SUMMARY requirement is absent,
|
|
// the false-stall bug is absent from defenses too.
|
|
|
|
const { test, describe } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fs = require('fs');
|
|
const path = require('path');
|
|
const { execFileSync } = require('node:child_process');
|
|
const { readFileNormalized } = require('./helpers.cjs');
|
|
|
|
const COMMAND_PATH = path.join(__dirname, '..', 'commands', 'gsd', 'plan-review-convergence.md');
|
|
const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-review-convergence.md');
|
|
const CONFIG_DOC_PATH = path.join(__dirname, '..', 'docs', 'CONFIGURATION.md');
|
|
const PLAN_PHASE_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-phase.md');
|
|
const PLANNER_REVIEWS_PATH = path.join(__dirname, '..', 'gsd-core', 'references', 'planner-reviews.md');
|
|
const PLAN_CHECKER_PATH = path.join(__dirname, '..', 'agents', 'gsd-plan-checker.md');
|
|
|
|
// #2315: the workflow's reviewer-resolution block pipes through `jq`, which is
|
|
// a documented production dependency (review.md:244 "install jq if missing")
|
|
// but is NOT present in every test container (gsd-test's linux-node{22,24}
|
|
// images lack it; see tests/opencode-review-reconstruction.property.test.cjs
|
|
// for the same skip pattern). Behavioral tests that exercise the deployed jq
|
|
// pipeline skip when jq is absent; structural tests still run.
|
|
let jqAvailable = false;
|
|
try { execFileSync('jq', ['--version'], { stdio: 'ignore', timeout: 10000, killSignal: 'SIGKILL' }); jqAvailable = true; } catch { /* no jq on PATH */ }
|
|
|
|
// #2800: the hand-written per-flag `grep -q '\-\-<flag>'` whitelist lines were
|
|
// replaced by a loop deriving REVIEWER_FLAGS from `gsd_run review-lane flags`
|
|
// (the declared lane roster). These helpers extract the REAL deployed parse
|
|
// block (`REVIEWER_FLAGS=""` through the closing `done` of the for-loop) and
|
|
// execute it under a real `gsd_run` shim backed by the actual gsd-tools.cjs
|
|
// binary, so behavioral coverage proves pass-through against the real
|
|
// implementation rather than a stale hand-written literal. POSIX-only —
|
|
// callers must skip on win32.
|
|
const GSD_TOOLS_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs');
|
|
|
|
function extractReviewerFlagsParseBlock(workflowText) {
|
|
const startIdx = workflowText.indexOf('REVIEWER_FLAGS=""');
|
|
const doneIdx = workflowText.indexOf('\ndone\n', startIdx);
|
|
if (startIdx === -1 || doneIdx === -1) return null;
|
|
return workflowText.slice(startIdx, doneIdx + '\ndone'.length);
|
|
}
|
|
|
|
function runReviewerFlagsParseBlock(block, args) {
|
|
const stub = 'gsd_run() { node "$GSD_TOOLS_PATH" "$@"; }';
|
|
const script = `${stub}\n${block}\nprintf "%s" "$REVIEWER_FLAGS"`;
|
|
return execFileSync('bash', ['-c', script], {
|
|
env: { ...process.env, ARGUMENTS: args, GSD_TOOLS_PATH },
|
|
encoding: 'utf8',
|
|
// 30s covers the nested bash → node → gsd-tools.cjs cold-start chain this
|
|
// helper spawns. 5s was too tight: on a loaded bench (30k tests running
|
|
// in parallel) that budget was consumed by process-spawn scheduling
|
|
// latency alone, producing a spurious ETIMEDOUT with no genuine hang.
|
|
// 30_000 matches the convention other script-invocation tests in this
|
|
// repo already use (see e.g. adr-index-gate.test.cjs, check-env.test.cjs).
|
|
timeout: 30_000,
|
|
}).trim();
|
|
}
|
|
|
|
// ─── Command source ────────────────────────────────────────────────────────
|
|
|
|
describe('plan-review-convergence command source (#2306)', () => {
|
|
const command = fs.readFileSync(COMMAND_PATH, 'utf8');
|
|
|
|
test('command name uses gsd: prefix (installer converts to gsd- on install)', () => {
|
|
assert.ok(
|
|
command.includes('name: gsd:plan-review-convergence'),
|
|
'command name must use gsd: prefix so installer converts it to gsd-plan-review-convergence'
|
|
);
|
|
});
|
|
|
|
test('command declares all reviewer flags in context', () => {
|
|
assert.ok(command.includes('--codex'), 'must document --codex flag');
|
|
assert.ok(command.includes('--gemini'), 'must document --gemini flag');
|
|
assert.ok(command.includes('--claude'), 'must document --claude flag');
|
|
assert.ok(command.includes('--opencode'), 'must document --opencode flag');
|
|
assert.ok(command.includes('--all'), 'must document --all flag');
|
|
assert.ok(command.includes('--max-cycles'), 'must document --max-cycles flag');
|
|
});
|
|
|
|
test('command documents local model reviewer flags (--ollama, --lm-studio, --llama-cpp)', () => {
|
|
assert.ok(command.includes('--ollama'), 'must document --ollama flag for local Ollama server');
|
|
assert.ok(command.includes('--lm-studio'), 'must document --lm-studio flag for local LM Studio server');
|
|
assert.ok(command.includes('--llama-cpp'), 'must document --llama-cpp flag for local llama.cpp server');
|
|
});
|
|
|
|
// #2293: the 1.7.0 Antigravity adapter (successor to the discontinued Gemini
|
|
// CLI) was unreachable from convergence because its reviewer whitelist dropped
|
|
// --agy/--antigravity. The flag must be documented and in the argument-hint.
|
|
test('command documents the --agy / --antigravity reviewer flag (#2293)', () => {
|
|
assert.ok(command.includes('--agy'), 'must document --agy flag (Antigravity CLI reviewer)');
|
|
assert.ok(command.includes('--antigravity'), 'must document the --antigravity alias');
|
|
});
|
|
|
|
test('argument-hint advertises --agy (#2293)', () => {
|
|
const hint = command.match(/^argument-hint:\s*"(.*)"\s*$/m);
|
|
assert.ok(hint, 'command must declare an argument-hint');
|
|
assert.ok(hint[1].includes('--agy'), `argument-hint must list --agy, got: ${hint[1]}`);
|
|
});
|
|
|
|
test('command references the workflow file via execution_context', () => {
|
|
assert.ok(
|
|
command.includes('@$HOME/.claude/gsd-core/workflows/plan-review-convergence.md'),
|
|
'execution_context must reference the workflow file'
|
|
);
|
|
});
|
|
|
|
test('command references supporting reference files', () => {
|
|
assert.ok(
|
|
command.includes('revision-loop.md'),
|
|
'must reference revision-loop.md for stall detection pattern'
|
|
);
|
|
assert.ok(
|
|
command.includes('gates.md'),
|
|
'must reference gates.md for gate taxonomy'
|
|
);
|
|
assert.ok(
|
|
command.includes('agent-contracts.md'),
|
|
'must reference agent-contracts.md for completion markers'
|
|
);
|
|
});
|
|
|
|
test('command declares Agent in allowed-tools (required for spawning review sub-agents)', () => {
|
|
assert.ok(
|
|
command.includes('- Agent'),
|
|
'Agent must be in allowed-tools — command spawns isolated agents for reviewing'
|
|
);
|
|
});
|
|
|
|
test('command declares Skill in allowed-tools (required for inline plan-phase invocations)', () => {
|
|
assert.ok(
|
|
command.includes('- Skill'),
|
|
'Skill must be in allowed-tools — command invokes gsd-plan-phase inline via Skill() at depth 0 (#936 fix)'
|
|
);
|
|
});
|
|
|
|
test('command has Copilot runtime_note for AskUserQuestion fallback', () => {
|
|
assert.ok(
|
|
command.includes('vscode_askquestions'),
|
|
'must document vscode_askquestions fallback for Copilot compatibility'
|
|
);
|
|
});
|
|
|
|
test('--codex is the default reviewer when no flag is given AND review.default_reviewers is unset (#2315)', () => {
|
|
// #2315: a bare invocation now respects review.default_reviewers per
|
|
// ADR-0011. The command must document that --codex is the default ONLY
|
|
// when review.default_reviewers is unset; otherwise the configured default
|
|
// wins. The pre-fix claim ("default if no reviewer specified") was the
|
|
// user-facing mirror of the #2315 bug.
|
|
assert.ok(
|
|
command.includes('default if no reviewer flag given') &&
|
|
command.includes('review.default_reviewers'),
|
|
'command must document that --codex is the default ONLY when review.default_reviewers is unset (#2315)'
|
|
);
|
|
});
|
|
|
|
test('command documents the workflow.plan_review_convergence config key', () => {
|
|
assert.ok(
|
|
command.includes('workflow.plan_review_convergence') ||
|
|
command.includes('plan_review_convergence'),
|
|
'command must document the config key required to enable the feature (#2306-v2)'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── #2293: Antigravity reviewer flag reachable from convergence ─────────────
|
|
|
|
describe('plan-review-convergence: --agy/--antigravity reviewer whitelist (#2293)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
const SKILL_PATH = path.join(__dirname, '..', 'skills', 'gsd-plan-review-convergence', 'SKILL.md');
|
|
|
|
test('workflow REVIEWER_FLAGS extraction derives its whitelist from review-lane flags, not a hand-written list (#2800)', () => {
|
|
// #2800: the runtime contract used to be a hand-written grep-accumulation
|
|
// block — one `grep -q '\-\-<flag>'` line per recognized flag. Absence meant
|
|
// the flag was silently dropped, and that whitelist drifted three separate
|
|
// times (--coderabbit, then --qwen/--cursor/--kimi-code, then the unanchored
|
|
// --agy pattern matching inside --antigravity). The fix derives the whitelist
|
|
// structurally from the declared lane roster via `gsd_run review-lane flags`,
|
|
// so --agy/--antigravity (and every other lane flag) are guaranteed reachable
|
|
// without being named individually here.
|
|
assert.ok(
|
|
workflow.includes('gsd_run review-lane flags'),
|
|
'workflow must derive REVIEWER_FLAGS by iterating `gsd_run review-lane flags` (the declared lane roster), not a hand-written list'
|
|
);
|
|
// Anti-parity guard: re-introducing a hand-enumerated per-flag whitelist line
|
|
// (the exact shape of the pre-#2800 bug) must fail this test, even though the
|
|
// structural loop above would still make --agy/--antigravity work — a FUTURE
|
|
// flag added only to a resurrected hand list, and never to the descriptor,
|
|
// would otherwise silently regress to the #2293 failure mode.
|
|
assert.ok(
|
|
!/grep -q '\\-\\-[a-zA-Z0-9-]+'\s*&&\s*REVIEWER_FLAGS=/.test(workflow),
|
|
'workflow must NOT contain a hand-written per-flag `grep -q \'--<flag>\' && REVIEWER_FLAGS=...` whitelist line — flags must be derived from the declared lane roster (#2800), never re-enumerated by hand'
|
|
);
|
|
});
|
|
|
|
test('generated SKILL.md mirrors the --agy flag (argument-hint parity)', () => {
|
|
const skill = fs.readFileSync(SKILL_PATH, 'utf8');
|
|
assert.ok(skill.includes('--agy'), 'generated SKILL.md must document --agy (regenerate via gen:plugin-skills)');
|
|
});
|
|
|
|
// Behavioral: execute the ACTUAL deployed REVIEWER_FLAGS parse block and
|
|
// assert --antigravity passes through instead of being dropped. POSIX-only —
|
|
// the block is /bin/sh-style grep pipework; skip on Windows where a bash
|
|
// shim is not guaranteed on PATH.
|
|
//
|
|
// #2315: the parse block no longer applies a --codex default. The endMarker
|
|
// was previously the unconditional `if [ -z "$REVIEWER_FLAGS" ]; then
|
|
// REVIEWER_FLAGS="--codex"; fi` line; that line was the #2315 bug and is now
|
|
// gone. Default resolution moved to step 1.5 after the config gate (see the
|
|
// #2315 describe block below). The bare-invocation assertion now expects an
|
|
// empty REVIEWER_FLAGS from the parse block — the default is applied later,
|
|
// respecting review.default_reviewers.
|
|
test('[behavioral] the deployed parse block passes --antigravity through (parse block no longer applies a --codex default — #2315)', (t) => {
|
|
if (process.platform === 'win32') { t.skip('POSIX shell extraction; not run on Windows'); return; }
|
|
const block = extractReviewerFlagsParseBlock(workflow);
|
|
assert.ok(block, 'the REVIEWER_FLAGS parse block must exist in the workflow');
|
|
const run = (args) => runReviewerFlagsParseBlock(block, args);
|
|
|
|
const agy = run('5 --antigravity');
|
|
assert.ok(agy.split(/\s+/).includes('--antigravity'), `--antigravity must pass through, got: "${agy}"`);
|
|
// The parse block must NOT inject --codex when an explicit flag is present.
|
|
assert.notStrictEqual(agy, '--codex', 'must NOT produce --codex-only when --antigravity is given');
|
|
|
|
assert.ok(run('5 --agy').split(/\s+/).includes('--agy'), '--agy short form must pass through');
|
|
|
|
// #2315 regression: the parse block no longer applies a default. The bare
|
|
// invocation (no flag) MUST yield an empty REVIEWER_FLAGS here; the default
|
|
// is resolved later in step 1.5 against review.default_reviewers.
|
|
assert.strictEqual(run('5'), '', 'no reviewer flag → empty REVIEWER_FLAGS from parse (default applied in step 1.5 per #2315)');
|
|
const mixed = run('5 --codex --gemini');
|
|
assert.ok(mixed.includes('--codex') && mixed.includes('--gemini'), 'existing flags still recognized');
|
|
// --agy must not be spuriously matched by an unrelated flag (independence).
|
|
assert.ok(!run('5 --gemini').includes('--agy'), '--gemini must not trip the --agy whitelist');
|
|
});
|
|
});
|
|
|
|
// ─── #2315: bare invocation respects review.default_reviewers ──────────────
|
|
|
|
describe('plan-review-convergence: #2315 respects review.default_reviewers (no-flag default)', () => {
|
|
// readFileNormalized() strips \r\n -> \n before the two behavioral tests
|
|
// below slice a bash block out of `workflow` and hand it to
|
|
// execFileSync('bash', ...) — an un-normalized read on a Windows checkout
|
|
// would break bash mid-script (DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE,
|
|
// #2650). Those two tests already skip on win32 for an unrelated POSIX-
|
|
// shell-extraction reason, but `workflow` is shared with non-skipped
|
|
// structural assertions in this same describe block, so normalizing here
|
|
// is the single correct fix rather than a per-test patch.
|
|
const workflow = readFileNormalized(WORKFLOW_PATH);
|
|
const command = fs.readFileSync(COMMAND_PATH, 'utf8');
|
|
const SKILL_PATH = path.join(__dirname, '..', 'skills', 'gsd-plan-review-convergence', 'SKILL.md');
|
|
const skill = fs.readFileSync(SKILL_PATH, 'utf8');
|
|
|
|
// Pre-fix #2315: the workflow unconditionally set REVIEWER_FLAGS="--codex" in
|
|
// step 1 (line 37) BEFORE the workflow.plan_review_convergence config gate.
|
|
// gsd-review sees the injected --codex as an explicit flag (precedence rule 1)
|
|
// and never reaches rule 3 (review.default_reviewers), silently overriding
|
|
// any configured default — a violation of ADR-0011 and ADR-0015.
|
|
//
|
|
// The buggy one-liner must not appear ANYWHERE in the workflow — the post-fix
|
|
// resolution lives in step 1.5 as a config-gated if/else/fi block, never as
|
|
// the bare one-liner. (Earlier versions of this test only asserted the line
|
|
// was not BEFORE the gate, which still allowed a re-introduction after the
|
|
// gate; the assertion is now unconditional per review.)
|
|
test('workflow does NOT contain the unconditional REVIEWER_FLAGS=--codex one-liner anywhere', () => {
|
|
const buggyLine = 'if [ -z "$REVIEWER_FLAGS" ]; then REVIEWER_FLAGS="--codex"; fi';
|
|
assert.strictEqual(
|
|
workflow.indexOf(buggyLine),
|
|
-1,
|
|
'the unconditional --codex one-liner must not appear anywhere in the workflow (#2315); ' +
|
|
'default resolution is conditional on review.default_reviewers in step 1.5 (if/else/fi block).'
|
|
);
|
|
});
|
|
|
|
test('workflow resolves REVIEWER_FLAGS against review.default_reviewers AFTER the config gate', () => {
|
|
const resolutionIdx = workflow.indexOf('gsd_run query config-get review.default_reviewers');
|
|
const configGateIdx = workflow.indexOf('CONVERGENCE_ENABLED=$(gsd_run query config-get workflow.plan_review_convergence');
|
|
assert.ok(resolutionIdx !== -1, 'workflow must query review.default_reviewers to resolve the no-flag default (#2315)');
|
|
assert.ok(
|
|
resolutionIdx > configGateIdx,
|
|
'review.default_reviewers resolution must come AFTER the config gate (only runs when convergence is enabled)'
|
|
);
|
|
});
|
|
|
|
test('workflow documents that empty REVIEWER_FLAGS lets gsd-review apply review.default_reviewers', () => {
|
|
// After the fix, an empty REVIEWER_FLAGS is INTENTIONAL — it signals "let
|
|
// gsd-review apply its own precedence (rule 3: review.default_reviewers)".
|
|
// A comment must document this so a future maintainer does not re-add the
|
|
// unconditional --codex fallback and resurrect the #2315 bug.
|
|
assert.ok(
|
|
/gsd-review applies.*review\.default_reviewers|review\.default_reviewers.*gsd-review applies/i.test(workflow),
|
|
'workflow must document that empty REVIEWER_FLAGS lets gsd-review apply review.default_reviewers (prevent #2315 regression)'
|
|
);
|
|
});
|
|
|
|
test('startup banner uses REVIEWER_DISPLAY (not raw REVIEWER_FLAGS) so users see what will actually run', () => {
|
|
// AC4 of #2315: banner must reflect actual reviewers, not a hardcoded value.
|
|
// When REVIEWER_FLAGS is empty (because default_reviewers is configured),
|
|
// the banner must show the resolved default — not an empty string and not
|
|
// a misleading "--codex". Assert the literal banner placeholder so a
|
|
// maintainer cannot satisfy this by defining REVIEWER_DISPLAY in a comment
|
|
// while leaving the banner pointing at REVIEWER_FLAGS.
|
|
assert.ok(
|
|
workflow.includes('Reviewers: {REVIEWER_DISPLAY}'),
|
|
'startup banner must use the {REVIEWER_DISPLAY} placeholder, not {REVIEWER_FLAGS} (#2315 AC4)'
|
|
);
|
|
assert.ok(
|
|
!/\bReviewers:\s*\{REVIEWER_FLAGS\}/.test(workflow),
|
|
'startup banner must NOT reference {REVIEWER_FLAGS} directly (#2315 AC4)'
|
|
);
|
|
});
|
|
|
|
test('command and skill doc both document the review.default_reviewers precedence (content parity)', () => {
|
|
// The #2315 fix updated the --codex flag description in BOTH the command
|
|
// (commands/gsd/plan-review-convergence.md) and the generated skill mirror
|
|
// (skills/gsd-plan-review-convergence/SKILL.md). The two are kept in sync
|
|
// by `npm run gen:plugin-skills`; this assertion catches a manual edit to
|
|
// one that the other doesn't mirror. Both must mention review.default_reviewers
|
|
// alongside the --codex default claim.
|
|
const expected = 'review.default_reviewers';
|
|
assert.ok(
|
|
command.includes(expected),
|
|
`command file must document the review.default_reviewers precedence on the --codex flag (#2315)`
|
|
);
|
|
assert.ok(
|
|
skill.includes(expected),
|
|
`skill file must mirror the command file's review.default_reviewers precedence (#2315)`
|
|
);
|
|
});
|
|
|
|
// Behavioral: extract the parse block + the post-config-gate resolution block
|
|
// and execute them with a stubbed gsd_run. Proves the matrix:
|
|
// - bare + default_reviewers configured → empty REVIEWER_FLAGS (gsd-review applies default)
|
|
// - bare + default_reviewers unset → --codex fallback (pre-fix behavior preserved)
|
|
// - bare + empty-array default → --codex fallback (defensive — schema would reject)
|
|
// - explicit --gemini + default set → --gemini wins (explicit flags unaffected, #2315 AC5)
|
|
test('[behavioral] no-flag invocation resolves to default_reviewers when configured, --codex otherwise', (t) => {
|
|
if (process.platform === 'win32') { t.skip('POSIX shell extraction; not run on Windows'); return; }
|
|
if (!jqAvailable) { t.skip('jq not on PATH — workflow resolution block pipes through jq (production dependency, review.md:244); structural tests above still validate the fix'); return; }
|
|
const { execFileSync } = require('node:child_process');
|
|
|
|
// Parse block: the REAL deployed REVIEWER_FLAGS derivation loop (#2800), from
|
|
// `REVIEWER_FLAGS=""` through the closing `done`. It calls `gsd_run review-lane
|
|
// flags`, so the stub below delegates that specific call to the real gsd-tools.cjs
|
|
// binary rather than a hand-maintained flag list.
|
|
const parseBlock = extractReviewerFlagsParseBlock(workflow);
|
|
assert.ok(parseBlock, 'parse block must exist');
|
|
|
|
// Resolution block: the `if [ -z "$REVIEWER_FLAGS" ]; then` that appears
|
|
// AFTER the config gate (CONVERGENCE_ENABLED=). This is the #2315 fix.
|
|
// Extract to the closing fence of the enclosing ```bash block — the block
|
|
// contains a nested if/else/fi, so a naive "first \nfi\n" match would stop
|
|
// at the inner fi and yield unbalanced bash.
|
|
const configGateIdx = workflow.indexOf('CONVERGENCE_ENABLED=$(gsd_run query config-get workflow.plan_review_convergence');
|
|
const resolutionStart = workflow.indexOf('if [ -z "$REVIEWER_FLAGS" ]; then', configGateIdx);
|
|
assert.ok(resolutionStart !== -1, 'post-config-gate REVIEWER_FLAGS resolution block must exist (#2315)');
|
|
const closingFence = workflow.indexOf('\n```\n', resolutionStart);
|
|
assert.ok(closingFence !== -1, 'could not locate closing fence of resolution block');
|
|
const resolutionBlock = workflow.slice(resolutionStart, closingFence);
|
|
|
|
const run = ({ args, defaultReviewers }) => {
|
|
// Stub gsd_run: `query config-get review.default_reviewers` returns the
|
|
// stubbed value; `review-lane flags` delegates to the REAL gsd-tools.cjs
|
|
// binary (so the parse block's loop is genuinely exercised, not stubbed
|
|
// into a no-op); anything else is a no-op.
|
|
//
|
|
// The default_reviewers value is passed via env var ($GSD_TEST_DEFAULT_REVIEWERS)
|
|
// rather than inline-interpolated into the bash script. This avoids a quoting
|
|
// fragility: a future test input containing a single quote would otherwise
|
|
// break the bash single-quoted string and execute as bash. With env-var
|
|
// handoff, the value never crosses an interpreting shell context.
|
|
const stub = `gsd_run() { case "$*" in *"config-get review.default_reviewers"*) printf '%s' "$GSD_TEST_DEFAULT_REVIEWERS";; *"review-lane flags"*) node "$GSD_TOOLS_PATH" review-lane flags;; *) return 0;; esac; }`;
|
|
const script = `${stub}\n${parseBlock}\n${resolutionBlock}\nprintf 'REVIEWER_FLAGS=[%s] REVIEWER_DISPLAY=[%s]' "$REVIEWER_FLAGS" "$REVIEWER_DISPLAY"`;
|
|
return execFileSync('bash', ['-c', script], {
|
|
env: { ...process.env, ARGUMENTS: args, GSD_TEST_DEFAULT_REVIEWERS: defaultReviewers ?? '', GSD_TOOLS_PATH },
|
|
encoding: 'utf8',
|
|
// 30s covers the same nested bash → node → gsd-tools.cjs cold-start
|
|
// chain as runReviewerFlagsParseBlock above; see that helper's
|
|
// comment for why 5s flaked under bench load.
|
|
timeout: 30_000,
|
|
});
|
|
};
|
|
|
|
// AC1: bare invocation with default_reviewers configured → empty REVIEWER_FLAGS
|
|
// (gsd-review applies configured default per its rule 3) and banner shows the resolved default.
|
|
let r = run({ args: '5', defaultReviewers: '["gemini","claude"]' });
|
|
assert.ok(/REVIEWER_FLAGS=\[\s*\]/.test(r), `configured default → REVIEWER_FLAGS empty, got: "${r}"`);
|
|
assert.ok(/review\.default_reviewers \(gemini, claude\)/.test(r), `banner shows configured default, got: "${r}"`);
|
|
|
|
// AC2: reviewer instances participate via default_reviewers — same path.
|
|
r = run({ args: '5', defaultReviewers: '["opencode-deepseek","opencode-mimo"]' });
|
|
assert.ok(/REVIEWER_FLAGS=\[\s*\]/.test(r), `instance default → REVIEWER_FLAGS empty, got: "${r}"`);
|
|
|
|
// AC3: bare invocation with default_reviewers unset → --codex fallback preserved.
|
|
r = run({ args: '5', defaultReviewers: '' });
|
|
assert.ok(/REVIEWER_FLAGS=\[--codex\]/.test(r), `unset default → --codex fallback, got: "${r}"`);
|
|
|
|
// AC3 defensive: empty-array default → --codex fallback (schema rejects this, but be safe).
|
|
r = run({ args: '5', defaultReviewers: '[]' });
|
|
assert.ok(/REVIEWER_FLAGS=\[--codex\]/.test(r), `empty-array default → --codex fallback, got: "${r}"`);
|
|
|
|
// AC5 (out of scope but must not regress): explicit --gemini overrides configured default.
|
|
r = run({ args: '5 --gemini', defaultReviewers: '["claude"]' });
|
|
assert.ok(/REVIEWER_FLAGS=\[.*--gemini.*\]/.test(r), `explicit flag wins over configured default, got: "${r}"`);
|
|
});
|
|
|
|
// Property test — CLAUDE.md mandates at least one fast-check (fc) property
|
|
// test for parsers and bijective contracts. The resolution block parses the
|
|
// configured review.default_reviewers JSON and classifies it into one of two
|
|
// outcomes: "non-empty array → delegate to gsd-review" (REVIEWER_FLAGS empty)
|
|
// or "anything else → fall back to --codex" (defensive — schema rejects most
|
|
// of these at config-set time, but corruption/edge cases must not crash or
|
|
// misclassify). Locks the contract so a future change can't subtly narrow or
|
|
// widen the accepted shape.
|
|
const fc = require('fast-check');
|
|
test('[property] resolution classifies arbitrary JSON values: non-empty array → empty flags, anything else → --codex fallback', (t) => {
|
|
if (process.platform === 'win32') { t.skip('POSIX shell extraction; not run on Windows'); return; }
|
|
if (!jqAvailable) { t.skip('jq not on PATH — workflow resolution block pipes through jq; structural tests above still validate the fix'); return; }
|
|
|
|
// Re-extract the blocks (the test above proved extraction works; we re-use
|
|
// the same logic rather than promoting to a helper to keep the test scope local).
|
|
const parseBlock = extractReviewerFlagsParseBlock(workflow);
|
|
const configGateIdx = workflow.indexOf('CONVERGENCE_ENABLED=$(gsd_run query config-get workflow.plan_review_convergence');
|
|
const resolutionStart = workflow.indexOf('if [ -z "$REVIEWER_FLAGS" ]; then', configGateIdx);
|
|
const closingFence = workflow.indexOf('\n```\n', resolutionStart);
|
|
const resolutionBlock = workflow.slice(resolutionStart, closingFence);
|
|
const { execFileSync } = require('node:child_process');
|
|
const run = ({ args, defaultReviewers }) => {
|
|
const stub = `gsd_run() { case "$*" in *"config-get review.default_reviewers"*) printf '%s' "$GSD_TEST_DEFAULT_REVIEWERS";; *"review-lane flags"*) node "$GSD_TOOLS_PATH" review-lane flags;; *) return 0;; esac; }`;
|
|
const script = `${stub}\n${parseBlock}\n${resolutionBlock}\nprintf 'REVIEWER_FLAGS=[%s] REVIEWER_DISPLAY=[%s]' "$REVIEWER_FLAGS" "$REVIEWER_DISPLAY"`;
|
|
return execFileSync('bash', ['-c', script], {
|
|
env: { ...process.env, ARGUMENTS: args, GSD_TEST_DEFAULT_REVIEWERS: defaultReviewers ?? '', GSD_TOOLS_PATH },
|
|
encoding: 'utf8',
|
|
// 30s covers the same nested bash → node → gsd-tools.cjs cold-start
|
|
// chain as runReviewerFlagsParseBlock above; see that helper's
|
|
// comment for why 5s flaked under bench load.
|
|
timeout: 30_000,
|
|
});
|
|
};
|
|
|
|
// Slug pattern mirrors the schema (ADR-0011: ^[a-zA-Z0-9_-]+$).
|
|
const slug = fc.stringMatching(/^[a-zA-Z0-9_-]{1,8}$/);
|
|
// Non-empty arrays of slugs → MUST classify as "use default" (REVIEWER_FLAGS empty).
|
|
const nonEmptyArray = fc.array(slug, { minLength: 1, maxLength: 4 }).map((a) => JSON.stringify(a));
|
|
// Defensive-corpus: empty array, scalar JSON, malformed JSON. All MUST fall
|
|
// back to --codex. The schema rejects the first two at config-set time, but
|
|
// a corruption/typo landing in the file directly would still reach this code.
|
|
const emptyArray = fc.constant('[]');
|
|
const scalarJson = fc.oneof(
|
|
fc.string({ maxLength: 8 }).filter((s) => !s.includes('"')).map((s) => JSON.stringify(s)),
|
|
fc.integer({ min: -10, max: 10 }).map((n) => JSON.stringify(n)),
|
|
fc.constant('null'),
|
|
fc.constant('true'),
|
|
fc.constant('false')
|
|
);
|
|
const malformedJson = fc.oneof(
|
|
fc.constant('[unclosed'),
|
|
fc.constant('{bad json'),
|
|
fc.constant('not json at all'),
|
|
fc.constant('{"k":'),
|
|
fc.string({ maxLength: 12 }).filter((s) => {
|
|
try { JSON.parse(s); return false; } catch { return true; }
|
|
})
|
|
);
|
|
|
|
// Property A: any non-empty array of slugs → empty REVIEWER_FLAGS.
|
|
fc.assert(
|
|
fc.property(nonEmptyArray, (dr) => /REVIEWER_FLAGS=\[\s*\]/.test(run({ args: '5', defaultReviewers: dr }))),
|
|
{ numRuns: 25 }
|
|
);
|
|
|
|
// Property B: anything in the defensive corpus → --codex fallback.
|
|
fc.assert(
|
|
fc.property(fc.oneof(emptyArray, scalarJson, malformedJson), (dr) => /REVIEWER_FLAGS=\[--codex\]/.test(run({ args: '5', defaultReviewers: dr }))),
|
|
{ numRuns: 25 }
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: initialization ──────────────────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: initialization (#2306)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow calls gsd-tools.cjs init plan-phase for initialization', () => {
|
|
assert.ok(
|
|
workflow.includes('gsd-tools.cjs') && workflow.includes('init') && workflow.includes('plan-phase'),
|
|
'workflow must initialize via gsd-tools.cjs init plan-phase'
|
|
);
|
|
});
|
|
|
|
test('workflow parses --max-cycles with default of 3', () => {
|
|
assert.ok(
|
|
workflow.includes('MAX_CYCLES') && workflow.includes('3'),
|
|
'workflow must parse --max-cycles with default of 3'
|
|
);
|
|
});
|
|
|
|
test('workflow displays a startup banner with phase number and reviewer flags', () => {
|
|
assert.ok(
|
|
workflow.includes('PLAN CONVERGENCE') || workflow.includes('Plan Convergence'),
|
|
'workflow must display a startup banner'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: config gate (disabled by default) ───────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: config gate (#2306-v2)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow checks workflow.plan_review_convergence config key before running', () => {
|
|
assert.ok(
|
|
workflow.includes('workflow.plan_review_convergence'),
|
|
'workflow must check workflow.plan_review_convergence config key — feature is disabled by default (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('workflow exits with enable instructions when config key is false', () => {
|
|
// Must tell the user how to enable the feature
|
|
assert.ok(
|
|
workflow.includes('gsd config-set workflow.plan_review_convergence true') ||
|
|
workflow.includes('config-set workflow.plan_review_convergence'),
|
|
'workflow must show the user how to enable the feature when disabled (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('workflow defaults config key to false (opt-in, not opt-out)', () => {
|
|
// The config-get call must default to false, not true
|
|
const configGetMatch = workflow.match(/config-get\s+workflow\.plan_review_convergence[^\r\n]*/);
|
|
assert.ok(
|
|
configGetMatch,
|
|
'workflow must read workflow.plan_review_convergence via config-get'
|
|
);
|
|
assert.ok(
|
|
configGetMatch[0].includes('"false"') || configGetMatch[0].includes("'false'") || configGetMatch[0].includes('false'),
|
|
'workflow must default workflow.plan_review_convergence to false (disabled by default) (#2306-v2)'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: initial planning gate ──────────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: initial planning gate (#2306)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow skips initial planning when plans already exist', () => {
|
|
assert.ok(
|
|
workflow.includes('has_plans') || workflow.includes('plan_count'),
|
|
'workflow must check whether plans already exist before running inline planning'
|
|
);
|
|
});
|
|
|
|
test('workflow runs gsd-plan-phase when no plans exist', () => {
|
|
assert.ok(
|
|
workflow.includes('gsd-plan-phase'),
|
|
'workflow must invoke gsd-plan-phase when no plans exist'
|
|
);
|
|
});
|
|
|
|
test('workflow errors if initial planning produces no PLAN.md files', () => {
|
|
assert.ok(
|
|
workflow.includes('PLAN_COUNT') || workflow.includes('plan_count'),
|
|
'workflow must verify PLAN.md files were created after initial planning'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: convergence loop ────────────────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: convergence loop (#2306)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow spawns isolated review agent each cycle', () => {
|
|
assert.ok(
|
|
workflow.includes('gsd-review'),
|
|
'workflow must spawn Agent → gsd-review each cycle'
|
|
);
|
|
});
|
|
|
|
test('workflow extracts HIGH and actionable counts from CYCLE_SUMMARY contract, NOT from grepping REVIEWS.md', () => {
|
|
// Critical regression guard: REVIEWS.md accumulates history across cycles;
|
|
// resolved HIGHs from cycle N remain in the file during cycle N+1 as audit trail,
|
|
// inflating raw grep counts and causing false stalls. Counts must come from
|
|
// the review agent's CYCLE_SUMMARY return message, not from the file.
|
|
assert.ok(
|
|
workflow.includes('CYCLE_SUMMARY'),
|
|
'workflow must use CYCLE_SUMMARY contract from review agent return message, not raw grep (#2306-v2 false-stall fix)'
|
|
);
|
|
assert.ok(
|
|
workflow.includes('current_high'),
|
|
'workflow must parse current_high from CYCLE_SUMMARY line'
|
|
);
|
|
assert.ok(
|
|
workflow.includes('ACTIONABLE_COUNT') && workflow.includes('current_actionable'),
|
|
'workflow must parse current_actionable from CYCLE_SUMMARY line (#724)'
|
|
);
|
|
});
|
|
|
|
test('workflow aborts if review agent omits CYCLE_SUMMARY contract', () => {
|
|
assert.ok(
|
|
workflow.includes('did not honor the CYCLE_SUMMARY contract') ||
|
|
workflow.includes('CYCLE_SUMMARY contract'),
|
|
'workflow must abort with clear error when review agent omits CYCLE_SUMMARY (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('workflow distinguishes malformed CYCLE_SUMMARY from absent CYCLE_SUMMARY', () => {
|
|
// Helps debugging: "present but malformed" vs "completely missing" are different errors
|
|
assert.ok(
|
|
workflow.includes('malformed') ||
|
|
(workflow.includes('CYCLE_SUMMARY') && workflow.includes('present')),
|
|
'workflow must distinguish malformed CYCLE_SUMMARY from absent one for debuggability (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('workflow fails closed when current_actionable is missing or malformed', () => {
|
|
assert.ok(
|
|
workflow.includes('current_actionable is missing or malformed'),
|
|
'missing or malformed current_actionable must abort instead of silently treating actionable findings as zero (#724)'
|
|
);
|
|
});
|
|
|
|
test('review agent spawn forwards --ws via GSD_WS (symmetric with replan agent)', () => {
|
|
// Critical correctness bug: if GSD_WS is not forwarded to the review agent,
|
|
// the review reads from the wrong workspace while replanning reads from the correct one.
|
|
const reviewAgentBlock = workflow.match(/gsd-review['"`,\s][\s\S]{0,300}?GSD_WS/);
|
|
assert.ok(
|
|
reviewAgentBlock ||
|
|
(workflow.includes("'gsd-review'") && workflow.includes('{GSD_WS}') &&
|
|
workflow.indexOf('{GSD_WS}') < workflow.indexOf("'gsd-plan-phase'")),
|
|
'review agent spawn must forward {GSD_WS} — workspace flag must reach the reviewer (#2306-v2 --ws fix)'
|
|
);
|
|
});
|
|
|
|
test('workflow exits loop only when HIGH_COUNT and ACTIONABLE_COUNT are zero', () => {
|
|
assert.ok(
|
|
workflow.includes('HIGH_COUNT == 0 and ACTIONABLE_COUNT == 0'),
|
|
'workflow must require both HIGH_COUNT and ACTIONABLE_COUNT to be zero before convergence (#724)'
|
|
);
|
|
assert.ok(
|
|
workflow.includes('If HIGH_COUNT > 0 or ACTIONABLE_COUNT > 0'),
|
|
'current_high=0 with current_actionable>0 must continue to replan/escalation instead of converging (#724)'
|
|
);
|
|
});
|
|
|
|
test('workflow updates STATE.md on convergence', () => {
|
|
assert.ok(
|
|
workflow.includes('planned-phase') || workflow.includes('state'),
|
|
'workflow must update STATE.md via gsd-tools.cjs when converged'
|
|
);
|
|
});
|
|
|
|
test('workflow invokes inline replan with --reviews flag', () => {
|
|
assert.ok(
|
|
workflow.includes('--reviews'),
|
|
'inline replan must pass --reviews so gsd-plan-phase incorporates review feedback'
|
|
);
|
|
});
|
|
|
|
test('workflow passes --skip-research to inline replan (research already done)', () => {
|
|
assert.ok(
|
|
workflow.includes('--skip-research'),
|
|
'inline replan must skip research — only initial planning needs research'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: CYCLE_SUMMARY contract definition ──────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: CYCLE_SUMMARY contract definition (#2306-v2)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('review agent prompt defines CYCLE_SUMMARY current_high/current_actionable format', () => {
|
|
assert.ok(
|
|
workflow.includes('CYCLE_SUMMARY: current_high=<N> current_actionable=<M>'),
|
|
'review agent spawn prompt must define the CYCLE_SUMMARY current_high/current_actionable output format (#724)'
|
|
);
|
|
});
|
|
|
|
test('CYCLE_SUMMARY contract defines PARTIALLY RESOLVED (acknowledged, mitigation incomplete)', () => {
|
|
assert.ok(
|
|
workflow.includes('PARTIALLY RESOLVED'),
|
|
'CYCLE_SUMMARY INCLUDE list must define PARTIALLY RESOLVED — prevents under-counting of in-progress issues (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('CYCLE_SUMMARY contract defines FULLY RESOLVED (verified/closed)', () => {
|
|
assert.ok(
|
|
workflow.includes('FULLY RESOLVED'),
|
|
'CYCLE_SUMMARY EXCLUDE list must define FULLY RESOLVED — prevents over-counting of closed issues (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('CYCLE_SUMMARY contract requires ## Current HIGH Concerns section in review return', () => {
|
|
assert.ok(
|
|
workflow.includes('## Current HIGH Concerns'),
|
|
'review agent must provide ## Current HIGH Concerns section so escalation gate can show specific issues (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('CYCLE_SUMMARY contract defines ACTIONABLE non-HIGH findings and requires their section', () => {
|
|
assert.ok(
|
|
workflow.includes('ACTIONABLE') && workflow.includes('Current Actionable Non-HIGH Concerns'),
|
|
'review agent must define actionable non-HIGH findings and list current unresolved actionable items (#724)'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: HIGH_LINES validation ──────────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: HIGH_LINES validation (#2306-v2)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow warns when HIGH_COUNT > 0 but ## Current HIGH Concerns section is absent', () => {
|
|
// Prevents silent UX degradation: escalation gate shows blank concern list
|
|
assert.ok(
|
|
workflow.includes('HIGH_LINES') &&
|
|
(workflow.includes('incomplete escalation') || workflow.includes('Current HIGH Concerns')),
|
|
'workflow must warn when HIGH_COUNT > 0 but HIGH_LINES is empty (contract partially violated) (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('workflow warns when ACTIONABLE_COUNT > 0 but actionable section is absent', () => {
|
|
assert.ok(
|
|
workflow.includes('ACTIONABLE_LINES') &&
|
|
workflow.includes('Current Actionable Non-HIGH Concerns'),
|
|
'workflow must warn when ACTIONABLE_COUNT > 0 but actionable details are empty (#724)'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: stall detection ─────────────────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: stall detection (#2306)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow tracks previous unresolved count to detect stalls', () => {
|
|
assert.ok(
|
|
workflow.includes('prev_unresolved_count'),
|
|
'workflow must track the previous total unresolved review count for stall detection (#724)'
|
|
);
|
|
});
|
|
|
|
test('workflow warns when unresolved count is not decreasing', () => {
|
|
assert.ok(
|
|
workflow.includes('stall') || workflow.includes('Stall') || workflow.includes('not decreasing'),
|
|
'workflow must warn user when unresolved review count is not decreasing between cycles'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: escalation gate ────────────────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: escalation gate (#2306)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow escalates to user when max cycles reached with HIGHs remaining', () => {
|
|
assert.ok(
|
|
workflow.includes('MAX_CYCLES') &&
|
|
(workflow.includes('AskUserQuestion') || workflow.includes('vscode_askquestions')),
|
|
'workflow must escalate to user via AskUserQuestion when max cycles reached'
|
|
);
|
|
});
|
|
|
|
test('escalation offers "Proceed anyway" option', () => {
|
|
assert.ok(
|
|
workflow.includes('Proceed anyway'),
|
|
'escalation gate must offer "Proceed anyway" to accept plans with remaining HIGH concerns'
|
|
);
|
|
});
|
|
|
|
test('escalation offers "Manual review" option', () => {
|
|
assert.ok(
|
|
workflow.includes('Manual review') || workflow.includes('manual'),
|
|
'escalation gate must offer a manual review option'
|
|
);
|
|
});
|
|
|
|
test('workflow has text-mode fallback for escalation (plain numbered list)', () => {
|
|
assert.ok(
|
|
workflow.includes('TEXT_MODE') || workflow.includes('text_mode'),
|
|
'workflow must support TEXT_MODE for plain-text escalation prompt'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: stall detection — behavioral ───────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: stall detection behavioral (#2306)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow surfaces stall warning when unresolved count stops decreasing', () => {
|
|
assert.ok(
|
|
workflow.includes('prev_unresolved_count'),
|
|
'workflow must track prev_unresolved_count across cycles (#724)'
|
|
);
|
|
assert.ok(
|
|
workflow.includes('UNRESOLVED_COUNT >= prev_unresolved_count') ||
|
|
workflow.includes('not decreasing'),
|
|
'workflow must compare current unresolved count against previous to detect stall (#724)'
|
|
);
|
|
assert.ok(
|
|
workflow.includes('stall') || workflow.includes('Stall') || workflow.includes('not decreasing'),
|
|
'workflow must emit a stall warning when unresolved review count is not decreasing'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: --max-cycles 1 immediate escalation — behavioral ────────────
|
|
|
|
describe('plan-review-convergence workflow: --max-cycles 1 immediate escalation behavioral (#2306)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow escalates immediately after cycle 1 when --max-cycles 1 and HIGH > 0', () => {
|
|
assert.ok(
|
|
workflow.includes('cycle >= MAX_CYCLES') ||
|
|
workflow.includes('cycle >= max_cycles') ||
|
|
(workflow.includes('MAX_CYCLES') && workflow.includes('AskUserQuestion')),
|
|
'workflow must check cycle >= MAX_CYCLES so --max-cycles 1 triggers escalation after first cycle'
|
|
);
|
|
assert.ok(
|
|
workflow.includes('HIGH_COUNT > 0') ||
|
|
workflow.includes('ACTIONABLE_COUNT > 0') ||
|
|
workflow.includes('HIGH concerns remain') ||
|
|
workflow.includes('Proceed anyway'),
|
|
'escalation gate must be reachable when unresolved findings remain after a single cycle'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: REVIEWS.md verification ────────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: artifact verification (#2306)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow verifies REVIEWS.md exists after each review cycle', () => {
|
|
assert.ok(
|
|
workflow.includes('REVIEWS.md') || workflow.includes('REVIEWS_FILE'),
|
|
'workflow must verify REVIEWS.md was produced by the review agent each cycle'
|
|
);
|
|
});
|
|
|
|
test('workflow errors if review agent does not produce REVIEWS.md', () => {
|
|
assert.ok(
|
|
workflow.includes('REVIEWS_FILE') || workflow.includes('review agent did not produce'),
|
|
'workflow must error if the review agent fails to produce REVIEWS.md'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Workflow: success criteria ────────────────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: success criteria (#2306-v2)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('success criteria references CYCLE_SUMMARY parsing, not grep findings', () => {
|
|
const successBlock = workflow.slice(workflow.lastIndexOf('<success_criteria>'));
|
|
assert.ok(
|
|
(successBlock.includes('CYCLE_SUMMARY') || successBlock.includes('parse')) &&
|
|
successBlock.includes('actionable non-HIGH'),
|
|
'success_criteria must reflect that orchestrator parses HIGH and actionable CYCLE_SUMMARY counts, not greps REVIEWS.md (#724)'
|
|
);
|
|
assert.ok(
|
|
!successBlock.includes('grep HIGHs'),
|
|
'success_criteria must NOT say "grep HIGHs" — that was the false-stall bug (#2306-v2)'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Config schema registration ───────────────────────────────────────────
|
|
|
|
describe('plan-review-convergence config schema registration (#2306-v2)', () => {
|
|
// After Cycle 5 (#3536), config-schema.cjs is a thin adapter sourcing from
|
|
// the manifest. Use the runtime Set instead of text-parsing the source file.
|
|
const { VALID_CONFIG_KEYS } = require('../gsd-core/bin/lib/config-schema.cjs');
|
|
|
|
test('workflow.plan_review_convergence is registered in config-schema.cjs', () => {
|
|
assert.ok(
|
|
VALID_CONFIG_KEYS.has('workflow.plan_review_convergence'),
|
|
"workflow.plan_review_convergence must be registered in VALID_CONFIG_KEYS in config-schema.cjs so gsd config-set accepts it (#2306-v2)"
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── CONFIGURATION.md documentation ──────────────────────────────────────
|
|
|
|
describe('plan-review-convergence CONFIGURATION.md documentation (#2306-v2)', () => {
|
|
const configDoc = fs.readFileSync(CONFIG_DOC_PATH, 'utf8');
|
|
|
|
test('workflow.plan_review_convergence is documented in CONFIGURATION.md', () => {
|
|
assert.ok(
|
|
configDoc.includes('workflow.plan_review_convergence'),
|
|
'workflow.plan_review_convergence must be documented in docs/CONFIGURATION.md — schema/docs parity test enforces this (#2306-v2)'
|
|
);
|
|
});
|
|
|
|
test('CONFIGURATION.md entry documents disabled-by-default behavior', () => {
|
|
const row = configDoc.match(/workflow\.plan_review_convergence[^\r\n]*/);
|
|
assert.ok(row, 'workflow.plan_review_convergence row must exist in CONFIGURATION.md');
|
|
assert.ok(
|
|
row[0].includes('false') || row[0].includes('disabled'),
|
|
'CONFIGURATION.md entry must document that the feature defaults to false (disabled by default) (#2306-v2)'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Reviews-mode incorporation contract (#724) ────────────────────────────
|
|
|
|
describe('plan-review-convergence reviews-mode incorporation contract (#724)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
const planPhase = fs.readFileSync(PLAN_PHASE_PATH, 'utf8');
|
|
const plannerReviews = fs.readFileSync(PLANNER_REVIEWS_PATH, 'utf8');
|
|
const planChecker = fs.readFileSync(PLAN_CHECKER_PATH, 'utf8');
|
|
|
|
test('workflow replans while actionable non-HIGH findings remain', () => {
|
|
assert.ok(
|
|
workflow.includes('Actionable MEDIUM/LOW findings must be incorporated into executable PLAN.md content'),
|
|
'inline replan must route actionable non-HIGH findings back through plan-phase --reviews (#724)'
|
|
);
|
|
});
|
|
|
|
test('plan-phase planner prompt says REVIEWS.md is feedback input, not the execution contract', () => {
|
|
assert.ok(
|
|
planPhase.includes('<review_incorporation_contract>') &&
|
|
planPhase.includes('REVIEWS.md is feedback input') &&
|
|
planPhase.includes('/gsd:execute-phase primarily consumes PLAN.md'),
|
|
'planner prompt must explain that actionable review feedback must land in PLAN.md for execute-phase (#724)'
|
|
);
|
|
});
|
|
|
|
test('plan-phase checker prompt reads REVIEWS.md in reviews mode and fails hidden actionable findings', () => {
|
|
assert.ok(
|
|
planPhase.includes('{reviews_path}') &&
|
|
planPhase.includes('<review_incorporation_verification>') &&
|
|
planPhase.includes('return `## ISSUES FOUND`'),
|
|
'checker prompt must read REVIEWS.md and fail if actionable findings remain only there (#724)'
|
|
);
|
|
});
|
|
|
|
test('planner reviews reference requires actionable findings to appear in PLAN.md or be deferred there', () => {
|
|
assert.ok(
|
|
plannerReviews.includes('/gsd:execute-phase primarily consumes PLAN.md') &&
|
|
plannerReviews.includes('Every current actionable review finding') &&
|
|
plannerReviews.includes('deferral/rejection rationale in that PLAN.md'),
|
|
'planner reviews reference must keep REVIEWS.md from becoming a hidden execution contract (#724)'
|
|
);
|
|
});
|
|
|
|
test('gsd-plan-checker has a Review Incorporation dimension for reviews mode', () => {
|
|
assert.ok(
|
|
planChecker.includes('Review Incorporation') &&
|
|
planChecker.includes('current_actionable=<M>') &&
|
|
planChecker.includes('remains only in REVIEWS.md'),
|
|
'plan checker must validate review incorporation when REVIEWS.md is present (#724)'
|
|
);
|
|
// The current_actionable=<M> reference must appear in a prohibition context,
|
|
// not as an instruction to parse machine-readable fields from REVIEWS.md.
|
|
// The CYCLE_SUMMARY line exists only in the convergence orchestrator's return message.
|
|
assert.ok(
|
|
planChecker.includes('Do NOT look for') || planChecker.includes('do NOT look for'),
|
|
'plan checker must explicitly prohibit looking for CYCLE_SUMMARY/current_actionable=<M> in REVIEWS.md — those machine-readable fields are only on the orchestrator return message, never in the file'
|
|
);
|
|
assert.ok(
|
|
planChecker.includes('CYCLE_SUMMARY') &&
|
|
(planChecker.includes('Do NOT look for') || planChecker.includes('do NOT look for')),
|
|
'CYCLE_SUMMARY must appear in plan-checker only as a prohibited pattern, not as a parsing instruction'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Local model reviewer support ────────────────────────────────────────
|
|
|
|
describe('plan-review-convergence local model reviewer flags (#2306-local)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
// #2800: local-model flags are no longer hand-listed in the workflow — they
|
|
// reach REVIEWER_FLAGS via the derived `gsd_run review-lane flags` loop (see
|
|
// the #2293 describe block above). Prove pass-through behaviorally against
|
|
// the REAL deployed parse block instead of grepping for a literal string
|
|
// that no longer exists in the workflow source.
|
|
test('[behavioral] --ollama, --lm-studio, --llama-cpp pass through the derived REVIEWER_FLAGS loop', (t) => {
|
|
if (process.platform === 'win32') { t.skip('POSIX shell extraction; not run on Windows'); return; }
|
|
const block = extractReviewerFlagsParseBlock(workflow);
|
|
assert.ok(block, 'the REVIEWER_FLAGS parse block must exist in the workflow');
|
|
for (const flag of ['--ollama', '--lm-studio', '--llama-cpp']) {
|
|
const out = runReviewerFlagsParseBlock(block, `5 ${flag}`);
|
|
assert.ok(
|
|
out.split(/\s+/).includes(flag),
|
|
`${flag} must pass through the derived REVIEWER_FLAGS loop, got: "${out}"`
|
|
);
|
|
}
|
|
});
|
|
});
|
|
|
|
describe('plan-review-convergence local model config schema registration (#2306-local)', () => {
|
|
// After Cycle 5 (#3536), config-schema.cjs is a thin adapter sourcing from
|
|
// the manifest. Use the runtime Set instead of text-parsing the source file.
|
|
//
|
|
// #2797 (ADR-2782 D9): these three host keys are no longer in VALID_CONFIG_KEYS
|
|
// — they are federated to their reviewer-lane capabilities (`ollama`,
|
|
// `lm-studio`, `llama-cpp`), and the exclusivity invariant forbids a key living
|
|
// in both places. What #2306-local actually protects is that `gsd config-set`
|
|
// ACCEPTS these keys, so that is what is asserted now, via isValidConfigKey —
|
|
// the predicate config-set itself uses, which spans both the central schema and
|
|
// federated capability slices. Asserting central membership would now be
|
|
// asserting the migration did not happen.
|
|
const { isValidConfigKey, isCapabilityConfigKey } =
|
|
require('../gsd-core/bin/lib/config-schema.cjs');
|
|
|
|
for (const key of ['review.ollama_host', 'review.lm_studio_host', 'review.llama_cpp_host']) {
|
|
test(`${key} is accepted by config-set`, () => {
|
|
assert.ok(
|
|
isValidConfigKey(key),
|
|
`${key} must be a valid config key so gsd config-set accepts it`
|
|
);
|
|
});
|
|
|
|
test(`${key} is owned by its reviewer-lane capability (#2797)`, () => {
|
|
assert.ok(
|
|
isCapabilityConfigKey(key),
|
|
`${key} must be federated to its lane capability, not stranded centrally`
|
|
);
|
|
});
|
|
}
|
|
});
|
|
|
|
// ─── Workflow: source-grounding pass (#22) ───────────────────────────────────
|
|
|
|
describe('plan-review-convergence workflow: source-grounding reviewer pass (#22)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
test('workflow documents plan_review.source_grounding config key (default on)', () => {
|
|
assert.ok(
|
|
workflow.includes('plan_review.source_grounding'),
|
|
'workflow must document the plan_review.source_grounding config key that gates the source-grounding pass (#22)'
|
|
);
|
|
});
|
|
|
|
test('source-grounding section defines all four symbol verdicts and their severity mappings within the section prose', () => {
|
|
// Extract the source-grounding section slice so verdicts buried in dead text,
|
|
// comments, or success-criteria prose outside this section cannot produce a
|
|
// false green. The section runs from the '### Source-grounding pass' heading
|
|
// to the 'After agent returns' paragraph that immediately follows it.
|
|
const SECTION_ANCHOR = '### Source-grounding pass';
|
|
const SECTION_END = 'After agent returns';
|
|
const anchorIdx = workflow.indexOf(SECTION_ANCHOR);
|
|
assert.ok(
|
|
anchorIdx !== -1,
|
|
`workflow must contain a '${SECTION_ANCHOR}' heading as the canonical location for verdict definitions (#22)`
|
|
);
|
|
const endIdx = workflow.indexOf(SECTION_END, anchorIdx);
|
|
assert.ok(
|
|
endIdx !== -1,
|
|
`'${SECTION_END}' paragraph must follow '${SECTION_ANCHOR}' to bound the section (#22)`
|
|
);
|
|
const section = workflow.slice(anchorIdx, endIdx);
|
|
|
|
// ── Four verdicts must appear in the resolve-step of the section ──────────
|
|
assert.ok(
|
|
section.includes('VERIFIED'),
|
|
'source-grounding section must define VERIFIED verdict within its prose (not just in surrounding text)'
|
|
);
|
|
assert.ok(
|
|
section.includes('MISSING'),
|
|
'source-grounding section must define MISSING verdict within its prose'
|
|
);
|
|
assert.ok(
|
|
section.includes('AMBIGUOUS'),
|
|
'source-grounding section must define AMBIGUOUS verdict within its prose'
|
|
);
|
|
assert.ok(
|
|
section.includes('UNCHECKABLE'),
|
|
'source-grounding section must define UNCHECKABLE verdict within its prose'
|
|
);
|
|
|
|
// ── Severity mappings: AMBIGUOUS→MEDIUM and UNCHECKABLE→INFO must appear
|
|
// on the SAME line inside the section, not just anywhere in the file ────
|
|
const severityLine = section.split(/\r?\n/).find((line) =>
|
|
line.includes('AMBIGUOUS') && line.includes('MEDIUM') &&
|
|
line.includes('UNCHECKABLE') && line.includes('INFO')
|
|
);
|
|
assert.ok(
|
|
severityLine !== undefined,
|
|
'source-grounding section must have a single severity-mapping line that states ' +
|
|
'AMBIGUOUS→MEDIUM AND UNCHECKABLE→INFO together (e.g. "**AMBIGUOUS** → MEDIUM. **UNCHECKABLE** → INFO.") (#22)'
|
|
);
|
|
|
|
// ── Guard the exact direction of each mapping ─────────────────────────────
|
|
// The line must pair AMBIGUOUS with MEDIUM (not INFO) and UNCHECKABLE with
|
|
// INFO (not MEDIUM) — a swap would be a contract bug the old tests couldn't catch.
|
|
const ambiguousBeforeMedium = severityLine.indexOf('AMBIGUOUS') < severityLine.indexOf('MEDIUM');
|
|
const uncheckableBeforeInfo = severityLine.indexOf('UNCHECKABLE') < severityLine.indexOf('INFO');
|
|
assert.ok(
|
|
ambiguousBeforeMedium,
|
|
'severity-mapping line must list AMBIGUOUS before MEDIUM (AMBIGUOUS→MEDIUM) (#22)'
|
|
);
|
|
assert.ok(
|
|
uncheckableBeforeInfo,
|
|
'severity-mapping line must list UNCHECKABLE before INFO (UNCHECKABLE→INFO) (#22)'
|
|
);
|
|
});
|
|
|
|
test('workflow specifies needs-acknowledgement gating for MISSING symbols', () => {
|
|
assert.ok(
|
|
workflow.includes('needs-acknowledgement'),
|
|
'workflow must specify needs-acknowledgement (not hard block) for MISSING at grep/intel authority (#22)'
|
|
);
|
|
});
|
|
|
|
test('workflow instructs reviewer to exclude symbols declared under "Artifacts this phase produces"', () => {
|
|
assert.ok(
|
|
workflow.includes('Artifacts this phase produces'),
|
|
'workflow must exclude new artifacts declared by the plan from symbol verification (#22)'
|
|
);
|
|
});
|
|
|
|
test('workflow requires "Verification coverage" section appended to REVIEWS.md', () => {
|
|
assert.ok(
|
|
workflow.includes('Verification coverage'),
|
|
'workflow must require a Verification coverage section in REVIEWS.md listing every UNCHECKABLE/skipped symbol (#22)'
|
|
);
|
|
});
|
|
});
|
|
|
|
describe('plan-review-convergence local model CONFIGURATION.md documentation (#2306-local)', () => {
|
|
const configDoc = fs.readFileSync(CONFIG_DOC_PATH, 'utf8');
|
|
|
|
test('review.ollama_host is documented in CONFIGURATION.md', () => {
|
|
assert.ok(
|
|
configDoc.includes('review.ollama_host'),
|
|
'review.ollama_host must be documented in docs/CONFIGURATION.md'
|
|
);
|
|
});
|
|
|
|
test('review.lm_studio_host is documented in CONFIGURATION.md', () => {
|
|
assert.ok(
|
|
configDoc.includes('review.lm_studio_host'),
|
|
'review.lm_studio_host must be documented in docs/CONFIGURATION.md'
|
|
);
|
|
});
|
|
|
|
test('review.llama_cpp_host is documented in CONFIGURATION.md', () => {
|
|
assert.ok(
|
|
configDoc.includes('review.llama_cpp_host'),
|
|
'review.llama_cpp_host must be documented in docs/CONFIGURATION.md'
|
|
);
|
|
});
|
|
|
|
test('review.models.ollama is documented in CONFIGURATION.md', () => {
|
|
assert.ok(
|
|
configDoc.includes('review.models.ollama'),
|
|
'review.models.ollama must be documented so users know how to configure the local model name'
|
|
);
|
|
});
|
|
|
|
test('review.models.lm_studio is documented in CONFIGURATION.md', () => {
|
|
assert.ok(
|
|
configDoc.includes('review.models.lm_studio'),
|
|
'review.models.lm_studio must be documented so users know how to configure the local model name'
|
|
);
|
|
});
|
|
|
|
test('review.models.llama_cpp is documented in CONFIGURATION.md', () => {
|
|
assert.ok(
|
|
configDoc.includes('review.models.llama_cpp'),
|
|
'review.models.llama_cpp must be documented so users know how to configure the local model name'
|
|
);
|
|
});
|
|
});
|
|
|
|
// ─── Bug #936: plan-phase must run inline, not inside Agent() ─────────────
|
|
//
|
|
// Regression guard: inverted from the pre-#936 behavior that locked in the bug.
|
|
// On Claude Code a depth-1 Agent has no Agent tool, so gsd-plan-phase wrapped in
|
|
// Agent() cannot spawn gsd-planner / gsd-plan-checker → the replan loop breaks.
|
|
// Fix: run plan-phase inline (bare Skill()) from the depth-0 convergence orchestrator.
|
|
//
|
|
// These tests FAIL on pre-fix code and PASS after the fix.
|
|
|
|
describe('plan-review-convergence workflow: inline plan-phase dispatch (#936)', () => {
|
|
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
// Helper: extract Agent() block bodies from workflow text
|
|
function extractAgentBlocks(content) {
|
|
const blocks = [];
|
|
let pos = 0;
|
|
while (pos < content.length) {
|
|
const start = content.indexOf('Agent(', pos);
|
|
if (start === -1) break;
|
|
let depth = 0;
|
|
let i = start + 'Agent('.length - 1;
|
|
for (; i < content.length; i++) {
|
|
if (content[i] === '(') depth++;
|
|
else if (content[i] === ')') { depth--; if (depth === 0) break; }
|
|
}
|
|
blocks.push({ start, end: i + 1, blockText: content.slice(start, i + 1) });
|
|
pos = i + 1;
|
|
}
|
|
return blocks;
|
|
}
|
|
|
|
test('initial planning does NOT wrap gsd-plan-phase inside Agent() (#936 fix)', () => {
|
|
// Pre-fix: Agent( ... Skill('gsd-plan-phase') ... ) in step 4
|
|
// Post-fix: bare Skill(skill="gsd-plan-phase") at orchestrator level
|
|
const blocks = extractAgentBlocks(workflow);
|
|
const wrapping = blocks.filter((b) =>
|
|
/Skill\(\s*skill=['"]gsd-plan-phase['"]/.test(b.blockText)
|
|
);
|
|
assert.deepStrictEqual(
|
|
wrapping.map((b) => b.blockText.slice(0, 80).replace(/\r?\n/g, '\\n')),
|
|
[],
|
|
'Initial planning must NOT wrap gsd-plan-phase inside Agent() — run it inline so ' +
|
|
'it can spawn gsd-planner/gsd-plan-checker at depth 1. See: bug #936'
|
|
);
|
|
});
|
|
|
|
test('replan step does NOT wrap gsd-plan-phase inside Agent() (#936 fix)', () => {
|
|
// Same check as above; explicitly named for the replan site (step 5d)
|
|
const blocks = extractAgentBlocks(workflow);
|
|
const wrapping = blocks.filter((b) =>
|
|
/Skill\(\s*skill=['"]gsd-plan-phase['"]/.test(b.blockText) &&
|
|
/--reviews/.test(b.blockText)
|
|
);
|
|
assert.deepStrictEqual(
|
|
wrapping.map((b) => b.blockText.slice(0, 80).replace(/\r?\n/g, '\\n')),
|
|
[],
|
|
'Replan step must NOT wrap gsd-plan-phase inside Agent() — the replan loop can ' +
|
|
'never produce a plan on Claude Code when plan-phase is at depth 1. See: bug #936'
|
|
);
|
|
});
|
|
|
|
test('workflow calls gsd-plan-phase inline (bare Skill outside Agent block) (#936 fix)', () => {
|
|
// After the fix there must be at least one bare Skill(skill="gsd-plan-phase")
|
|
// OUTSIDE any Agent() block.
|
|
const blocks = extractAgentBlocks(workflow);
|
|
let masked = workflow;
|
|
const sorted = [...blocks].sort((a, b) => b.start - a.start);
|
|
for (const b of sorted) {
|
|
masked = masked.slice(0, b.start) + ' '.repeat(b.end - b.start) + masked.slice(b.end);
|
|
}
|
|
assert.ok(
|
|
/Skill\(\s*skill=["']gsd-plan-phase["']/.test(masked),
|
|
'plan-review-convergence must contain at least one bare Skill(skill="gsd-plan-phase") ' +
|
|
'outside any Agent() block — the inline call that preserves depth-0 Agent availability. See: bug #936'
|
|
);
|
|
});
|
|
|
|
test('success_criteria describes inline plan-phase, not Agent → Skill (#936 fix)', () => {
|
|
const successBlock = workflow.slice(workflow.lastIndexOf('<success_criteria>'));
|
|
// The broken criterion said "Initial planning via Agent → Skill"
|
|
assert.ok(
|
|
!successBlock.includes('via Agent → Skill("gsd-plan-phase")'),
|
|
'success_criteria must NOT describe plan-phase as Agent → Skill — that was the broken pattern. See: bug #936'
|
|
);
|
|
// The broken criterion said "isolated, not inline" for the replan
|
|
assert.ok(
|
|
!successBlock.includes('isolated, not inline'),
|
|
'success_criteria must NOT say "isolated, not inline" for plan-phase — the fix makes it inline. See: bug #936'
|
|
);
|
|
});
|
|
});
|
|
|
|
|
|
// ────────────────────────────────────────────────────────────────────────
|
|
// Folded from tests/bug-936-no-nested-spawner-wrap.test.cjs — consolidation epic #1969 (B4 #1973)
|
|
// ────────────────────────────────────────────────────────────────────────
|
|
{
|
|
const { describe: __foldDescribe } = require('node:test');
|
|
__foldDescribe("folded:bug-936-no-nested-spawner-wrap (consolidation epic #1969 B4 #1973)", () => {
|
|
'use strict';
|
|
/**
|
|
* Structural guard — bug(#936): plan-review-convergence wrapped gsd-plan-phase
|
|
* in Agent() at TWO sites (initial planning + replan). On Claude Code, a depth-1
|
|
* Agent has no Agent tool, so plan-phase cannot spawn gsd-planner / gsd-plan-checker
|
|
* → the replan loop never works when HIGHs are found.
|
|
*
|
|
* Fix: run plan-phase INLINE (bare Skill()) from the convergence orchestrator,
|
|
* which runs at depth 0 and has Agent available — exactly how autonomous.md,
|
|
* manager.md, and discuss-phase-assumptions.md already chain plan-phase.
|
|
*
|
|
* This guard dynamically derives the set of "spawner" workflows (those containing
|
|
* `subagent_type=`) and asserts that NO workflow wraps a spawner inside Agent()
|
|
* UNLESS the wrapping block includes a RUNTIME != claude carve-out (the #853
|
|
* pattern already applied to autonomous.md / manager.md).
|
|
*/
|
|
|
|
// allow-test-rule: source-text-is-the-product (see #936)
|
|
// The workflow markdown IS the runtime instruction — static guards over
|
|
// workflow text are the canonical regression-test mechanism (per CONTRIBUTING
|
|
// exception matrix and tests/bug-853-bg-dispatch-runtime-gating.test.cjs).
|
|
|
|
const { describe, test } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fs = require('node:fs');
|
|
const path = require('node:path');
|
|
|
|
const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows');
|
|
|
|
// ── 1. Derive spawner skill names dynamically ──────────────────────────────
|
|
// A "spawner" workflow is one that contains `subagent_type=` — it NEEDS the
|
|
// Agent tool to run and therefore cannot safely be wrapped in another Agent()
|
|
// on Claude Code (where depth-1 agents have no Agent tool).
|
|
|
|
// Recursively collect all *.md files under WORKFLOWS_DIR (covers nested fragments
|
|
// like discuss-phase/modes/*.md and execute-phase/steps/*.md).
|
|
function collectWorkflowFiles(dir) {
|
|
const entries = fs.readdirSync(dir, { withFileTypes: true });
|
|
const results = [];
|
|
for (const e of entries) {
|
|
const fullPath = path.join(dir, e.name);
|
|
if (e.isDirectory()) {
|
|
results.push(...collectWorkflowFiles(fullPath));
|
|
} else if (e.name.endsWith('.md')) {
|
|
results.push({
|
|
name: path.relative(WORKFLOWS_DIR, fullPath),
|
|
path: fullPath,
|
|
content: fs.readFileSync(fullPath, 'utf8'),
|
|
});
|
|
}
|
|
}
|
|
return results;
|
|
}
|
|
|
|
const allWorkflowFiles = collectWorkflowFiles(WORKFLOWS_DIR);
|
|
|
|
// Map: base-slug → workflow filename (e.g. "plan-phase" → "plan-phase.md")
|
|
// Skill() calls use the "gsd-<slug>" convention in all workflow files.
|
|
// We build BOTH the bare slug set and the gsd-prefixed skill-name set.
|
|
const SPAWNER_BASE_SLUGS = new Set(
|
|
allWorkflowFiles
|
|
.filter((w) => w.content.includes('subagent_type='))
|
|
.map((w) => w.name.replace(/\.md$/, ''))
|
|
);
|
|
|
|
// Skill invocations use "gsd-<slug>" (e.g. gsd-plan-phase, gsd-execute-phase).
|
|
// Build the regex from the prefixed names so it actually matches what workflows write.
|
|
const SPAWNER_GSD_NAMES = new Set([...SPAWNER_BASE_SLUGS].map((s) => `gsd-${s}`));
|
|
|
|
// Build a regex that matches Skill(skill='gsd-<spawner>') or Skill(skill="gsd-<spawner>")
|
|
const spawnerPattern = new RegExp(
|
|
`Skill\\(\\s*skill=['"](?:${[...SPAWNER_GSD_NAMES].join('|')})['"]`,
|
|
's'
|
|
);
|
|
|
|
// ── 2. Helper: extract Agent() blocks from a workflow ─────────────────────
|
|
// Each block starts at "Agent(" and ends at the balancing ")". We collect
|
|
// the text of each such block together with the surrounding context (a 400
|
|
// char window before the block) so we can check for RUNTIME carve-outs.
|
|
|
|
function extractAgentBlocks(content) {
|
|
const blocks = [];
|
|
let pos = 0;
|
|
while (pos < content.length) {
|
|
const start = content.indexOf('Agent(', pos);
|
|
if (start === -1) break;
|
|
// Walk forward to find the balancing closing paren
|
|
let depth = 0;
|
|
let i = start + 'Agent('.length - 1; // at the '('
|
|
for (; i < content.length; i++) {
|
|
if (content[i] === '(') depth++;
|
|
else if (content[i] === ')') {
|
|
depth--;
|
|
if (depth === 0) break;
|
|
}
|
|
}
|
|
const end = i + 1;
|
|
const blockText = content.slice(start, end);
|
|
// Capture context: 400 chars before the block (for RUNTIME gate detection)
|
|
const contextBefore = content.slice(Math.max(0, start - 400), start);
|
|
blocks.push({ start, end, blockText, contextBefore });
|
|
pos = end;
|
|
}
|
|
return blocks;
|
|
}
|
|
|
|
// ── 3. Helper: does a block have a RUNTIME != claude carve-out nearby? ────
|
|
// The #853 pattern looks like: "RUNTIME is `claude`" in a preceding condition
|
|
// that switches to inline Skill() instead of the Agent() block. A block is
|
|
// considered guarded when the 400-char context window before it (or the block
|
|
// body itself for block-internal guards) contains any of these markers.
|
|
|
|
function hasRuntimeCarveout(block) {
|
|
const haystack = block.contextBefore + block.blockText;
|
|
return (
|
|
/RUNTIME[^`\n]{0,30}(?:!=|≠|is not|!==)\s*[`'"]?claude/i.test(haystack) ||
|
|
/RUNTIME[^`\n]{0,30}claude[^`\n]{0,30}(?:inline|not.*Agent|do NOT)/i.test(haystack) ||
|
|
/If `RUNTIME` is `claude`/i.test(haystack) ||
|
|
/On Claude Code.*inline/is.test(haystack)
|
|
);
|
|
}
|
|
|
|
// ── 4. The guard: scan every workflow for unguarded Agent→spawner wraps ───
|
|
|
|
describe('bug-936 — no workflow wraps a spawner skill inside Agent() without a RUNTIME carve-out', () => {
|
|
test('spawner set is non-empty (self-check: subagent_type= grep must find files)', () => {
|
|
assert.ok(SPAWNER_BASE_SLUGS.size > 0, `No spawner workflows found in ${WORKFLOWS_DIR} — SPAWNER_BASE_SLUGS derivation is broken`);
|
|
// plan-phase must be a spawner (base slug)
|
|
assert.ok(SPAWNER_BASE_SLUGS.has('plan-phase'), 'plan-phase.md must be in the spawner set (contains subagent_type=)');
|
|
// gsd-plan-phase must be in the prefixed set used by the regex
|
|
assert.ok(SPAWNER_GSD_NAMES.has('gsd-plan-phase'), 'gsd-plan-phase must be in SPAWNER_GSD_NAMES — the prefixed form used in Skill() calls');
|
|
});
|
|
|
|
for (const wf of allWorkflowFiles) {
|
|
// Only scan files that have at least one Agent( call
|
|
if (!wf.content.includes('Agent(')) continue;
|
|
|
|
test(`${wf.name}: no Agent() block wraps a spawner Skill without a RUNTIME carve-out`, () => {
|
|
const blocks = extractAgentBlocks(wf.content);
|
|
const violations = blocks.filter((b) => {
|
|
const wrapsSpawner = spawnerPattern.test(b.blockText);
|
|
if (!wrapsSpawner) return false;
|
|
return !hasRuntimeCarveout(b);
|
|
});
|
|
|
|
assert.deepStrictEqual(
|
|
violations.map((v) => v.blockText.slice(0, 120).replace(/\n/g, '\\n')),
|
|
[],
|
|
`${wf.name} wraps a spawner Skill inside Agent() without a RUNTIME != claude carve-out.\n` +
|
|
`Fix: run the spawner Skill inline (bare Skill() call at depth 0) OR add a RUNTIME gate.\n` +
|
|
`See: bug #936, tests/bug-853-bg-dispatch-runtime-gating.test.cjs for the guarded pattern.`
|
|
);
|
|
});
|
|
}
|
|
});
|
|
|
|
// ── 5. Focused regression: plan-review-convergence never wraps plan-phase ─
|
|
|
|
describe('bug-936 — plan-review-convergence runs plan-phase inline, not inside Agent()', () => {
|
|
const CONVERGENCE = fs.readFileSync(
|
|
path.join(WORKFLOWS_DIR, 'plan-review-convergence.md'),
|
|
'utf8'
|
|
);
|
|
|
|
test('plan-review-convergence does NOT wrap gsd-plan-phase inside Agent()', () => {
|
|
// The anti-pattern: Agent( block whose body contains Skill(skill='gsd-plan-phase')
|
|
const blocks = extractAgentBlocks(CONVERGENCE);
|
|
const wrapping = blocks.filter((b) =>
|
|
/Skill\(\s*skill=['"]gsd-plan-phase['"]/.test(b.blockText) &&
|
|
!hasRuntimeCarveout(b)
|
|
);
|
|
assert.deepStrictEqual(
|
|
wrapping.map((v) => v.blockText.slice(0, 120).replace(/\n/g, '\\n')),
|
|
[],
|
|
'plan-review-convergence must NOT wrap gsd-plan-phase inside Agent(). ' +
|
|
'Run it inline (bare Skill() at depth 0) so it can spawn gsd-planner/gsd-plan-checker. ' +
|
|
'See: bug #936'
|
|
);
|
|
});
|
|
|
|
test('plan-review-convergence calls gsd-plan-phase inline (bare Skill call outside Agent block)', () => {
|
|
// After the fix: at least one bare Skill(skill="gsd-plan-phase") must appear
|
|
// outside any Agent( block — that is the inline call from the depth-0 orchestrator.
|
|
const blocks = extractAgentBlocks(CONVERGENCE);
|
|
// Remove all Agent block ranges from the text
|
|
let masked = CONVERGENCE;
|
|
// Work from end to start so offsets stay valid
|
|
const sorted = [...blocks].sort((a, b) => b.start - a.start);
|
|
for (const b of sorted) {
|
|
masked = masked.slice(0, b.start) + ' '.repeat(b.end - b.start) + masked.slice(b.end);
|
|
}
|
|
const hasInlineCall = /Skill\(\s*skill=["']gsd-plan-phase["']/.test(masked);
|
|
assert.ok(
|
|
hasInlineCall,
|
|
'plan-review-convergence must contain at least one bare Skill(skill="gsd-plan-phase") ' +
|
|
'outside any Agent() block — this is the inline call that lets plan-phase spawn its sub-agents. ' +
|
|
'See: bug #936'
|
|
);
|
|
});
|
|
|
|
test('plan-review-convergence still wraps gsd-review inside Agent() (leaf — isolation is correct)', () => {
|
|
// gsd-review is a leaf (shells out via Bash, no subagent_type) so the Agent wrap is fine and intentional.
|
|
const blocks = extractAgentBlocks(CONVERGENCE);
|
|
const reviewWrap = blocks.some((b) => /Skill\(\s*skill=['"]gsd-review['"]/.test(b.blockText));
|
|
assert.ok(reviewWrap, 'gsd-review must still be wrapped in Agent() — it is a Bash leaf and isolation is intentional');
|
|
});
|
|
});
|
|
});
|
|
}
|