diff --git a/.claude/workflows/pair-implement-batch.js b/.claude/workflows/pair-implement-batch.js index f95817702..4fb327339 100644 --- a/.claude/workflows/pair-implement-batch.js +++ b/.claude/workflows/pair-implement-batch.js @@ -795,7 +795,7 @@ const MAX_FIX_ROUNDS = PIPELINE.maxFixRounds // truncated structured output) — and the contentless shape is the one this repo // actually measured on #432 (the machine slept mid-response), i.e. the retry // missed the exact incident it was written for while covering its rarer sibling. -// The review step therefore passes `hasVerdict`, the SAME predicate its +// The review step therefore passes `hasReviewEvidence`, the SAME predicate its // convergence guard uses, so "did not review" means one thing at both sites: the // transient gets its second chance, and a step that comes back contentless twice // still fails closed. @@ -812,6 +812,11 @@ async function agentRetry(prompt, opts, isUsable = r => !!r) { // ONE predicate, asked by the retry and by the convergence guard, so the two // cannot drift into disagreeing about what a dead reviewer is. const hasVerdict = r => !!r && !!String(r.verdict ?? '').trim() +const REVIEWED_HEAD_PATTERN = /^[0-9a-f]{40}$/ +// A review also has to identify the immutable PR revision it actually inspected. +// Without that baseline a later reviewer cannot distinguish the fix delta from the +// already-audited PR surface, which turns each re-review into another full scan. +const hasReviewEvidence = r => hasVerdict(r) && REVIEWED_HEAD_PATTERN.test(String(r.reviewedHead ?? '')) // ── Schemas (orchestration return-value contracts) ───────────────────────── // These are the compact values agents RETURN for control-flow — NOT the artifact @@ -854,6 +859,9 @@ const LOOSE_REVIEW_SCHEMA = { // Control flow keys on `nonActionable` + actionable count, never on specific // verdict strings. verdict: { type: 'string' }, + // Immutable full SHA of the PR head reviewed. This is workflow evidence, not + // part of the human-facing review template vocabulary. + reviewedHead: { type: 'string', pattern: '^[0-9a-f]{40}$' }, needsHumanDecision: { type: 'boolean' }, findings: { type: 'array', @@ -879,7 +887,7 @@ const LOOSE_REVIEW_SCHEMA = { }, }, }, - required: ['verdict'], + required: ['verdict', 'reviewedHead'], } const FIX_SCHEMA = { type: 'object', @@ -977,7 +985,17 @@ const contracts = STORIES.length ? await parallel(CONTRACT_SPECS.map((s) => () = const crContract = contracts.find((c) => c.name === 'code-review') // Schema the reviewer returns: template-derived when the contract is usable, // the loose skeleton otherwise. Control flow stays value-agnostic either way. -const REVIEW_SCHEMA = crContract?.schema ?? LOOSE_REVIEW_SCHEMA +const REVIEW_SCHEMA_BASE = crContract?.schema ?? LOOSE_REVIEW_SCHEMA +// Template contracts own human verdict/finding vocabulary. The orchestration-only +// baseline is layered on top so a template refresh cannot accidentally remove it. +const REVIEW_SCHEMA = { + ...REVIEW_SCHEMA_BASE, + properties: { + ...REVIEW_SCHEMA_BASE.properties, + reviewedHead: { type: 'string', pattern: '^[0-9a-f]{40}$' }, + }, + required: [...new Set([...(REVIEW_SCHEMA_BASE.required ?? []), 'verdict', 'reviewedHead'])], +} // Reviewer prompt vocabulary: `verdictOptions` and `severities` are CANONICAL, // required contract keys (ensure-contract.mjs's validateContract rejects any // contract missing either) — so whenever a contract IS present, both are @@ -1216,6 +1234,7 @@ async function driveStory(story) { // before honouring it, so the escalation is deferred by a round rather than dropped. let humanDecisionPending = false let prevFindings = [] + let prevReviewedHead = null // ACCUMULATES across rounds — never reassigned. A finding accepted in round 0 (by-design, or // below the floor) is not re-raised by the round-1 reviewer, because round 1 only sees the // fixed code and has no memory of what the human was already told would be carried. So a @@ -1230,7 +1249,10 @@ async function driveStory(story) { const acceptedKeys = new Set() const accept = (findings) => { for (const f of findings) { - const key = `${f.location ?? ''}${f.description ?? ''}` + // Keep a collision-free delimiter without embedding an invisible raw NUL in the shipped + // JavaScript source. A readable space collapses `(location, description)` pairs such as + // (`"a b"`, `"c"`) and (`"a"`, `"b c"`), silently dropping one accepted finding. + const key = `${f.location ?? ''}\u0000${f.description ?? ''}` if (acceptedKeys.has(key)) continue acceptedKeys.add(key) accepted.push(f) @@ -1249,10 +1271,14 @@ async function driveStory(story) { // in-flight log AND no first-review comment already on the PR. Either signal makes // round-0 a SILENT re-review, so a PR never accrues a second first-review. const first = round === 0 && !isContinuation && !firstReviewPosted + // An initial/resumed-without-history review establishes the whole-PR baseline. + // Once a fix is in flight, even the file inventory must start at that baseline; + // otherwise the pacing loop invites a second full audit before its delta rule. + const reviewBase = prevFindings.length ? prevReviewedHead : baseOf(story) const review = await agentRetry( - `Independently review PR #${pr.prNumber} for story ${tag}, following ${SK.review}. ${revWtClause(story)} PACING (mandatory — this is what killed the previous four attempts at this review, measured): a supervisor kills any agent that goes 180 seconds without emitting a TEXT MESSAGE. Tool calls do NOT count as progress: the last stalled reviewer was calling \`sed\`/\`cat\` every ~5 seconds and was still killed, because it had not written a sentence in 200 seconds. So: after EVERY file you inspect, write ONE SHORT LINE of prose saying what you found or that it is clean — before moving to the next file. Never read two files in a row without speaking in between, and never go into a long silent analysis pass. Start by listing the changed files (\`git diff ${baseOf(story)}...origin/${story.branch} --name-only\`), say aloud the order you will take them, then go file by file, narrating as you go. Brevity is fine — one line is enough — but silence is fatal. Review ONLY from the story's acceptance criteria, the PR diff+description, and the code. Do NOT read ${BLIND_PATHS}, nor any checkpoint, handoff or working log under them — they are the author's private context and this review is independent and blind to it. Report EVERY finding regardless of severity (including minor/nit), using the ${REVIEW_TEMPLATE_LABEL} vocabulary: each finding = \`location\` (File:Line), \`severity\` ∈ {${SEVERITIES}}, \`description\` (the CONCRETE FAILURE CASE — inputs/state -> wrong output — not a retelling of the diff), \`recommendation\` (the change, in one or two lines); verdict ∈ {${VERDICTS}}. ${TEXT_SHAPE} DO NOT FILE NEW ISSUES. This is a hard rule, and it overrides any habit of deferring work to a follow-up card: a debt you find in this diff is resolved IN PLACE, in this same PR, within this story's scope. Never invoke ${SK.writeIssue}, never write \`Deferred to #\`, and never recommend "track this separately" — a finding parked in a fresh card is a finding nobody fixes, and it converts a reviewed PR into an unreviewed backlog. Set \`nonActionable: true\` ONLY if fixing it would be genuinely WRONG — byte-consistent with a source of truth, matching an existing convention, an ALREADY-EXISTING tracked story (cite its number; do not create one), or something that can only resolve after merge. Being outside this story's originally stated scope is NOT a reason: fix it here. Whenever you set \`nonActionable: true\`, ALSO set \`disposition\` with a concrete reason replacing the bare label (\`By convention …\` / \`Historical record\` / \`Already tracked in #\` / \`Resolves after merge\`); never leave "non-actionable" as the only explanation. If a finding is SO large that fixing it here would genuinely swamp the story, say so explicitly in \`description\` and leave it ACTIONABLE — the human decides at the merge gate whether to accept the bigger PR or carve it out; that decision is not yours to pre-empt by filing a card. ${first ? `This is the FIRST review: POST your full review report as a PR comment on #${pr.prNumber} (${REVIEW_TEMPLATE_LABEL} structure), and include the marker line \`${firstReviewMarker}\` VERBATIM as the first line of the comment body — it is an HTML comment (invisible in the rendered markdown, so no visible noise) that lets a later resume detect this first review by an EXACT substring match rather than a semantic reading (finding 1). Then return findings + verdict.` : prevFindings.length - ? `This is a RE-REVIEW: do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only. Verify these prior findings were genuinely resolved: ${JSON.stringify(prevFindings)}.` - : `This is a RE-REVIEW on a resumed in-flight cycle (round-0 of this run carries no prior findings): do a FRESH, independent full review pass. do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only.`} Return findings and a verdict.`, + `Independently review PR #${pr.prNumber} for story ${tag}, following ${SK.review}. ${revWtClause(story)} PACING (mandatory — this is what killed the previous four attempts at this review, measured): a supervisor kills any agent that goes 180 seconds without emitting a TEXT MESSAGE. Tool calls do NOT count as progress: the last stalled reviewer was calling \`sed\`/\`cat\` every ~5 seconds and was still killed, because it had not written a sentence in 200 seconds. So: after EVERY file you inspect, write ONE SHORT LINE of prose saying what you found or that it is clean — before moving to the next file. Never read two files in a row without speaking in between, and never go into a long silent analysis pass. Start by listing the changed files (\`git diff ${reviewBase}...origin/${story.branch} --name-only\`), say aloud the order you will take them, then go file by file, narrating as you go. Brevity is fine — one line is enough — but silence is fatal. Review ONLY from the story's acceptance criteria, the PR diff+description, and the code. Do NOT read ${BLIND_PATHS}, nor any checkpoint, handoff or working log under them — they are the author's private context and this review is independent and blind to it. Report EVERY finding regardless of severity (including minor/nit), using the ${REVIEW_TEMPLATE_LABEL} vocabulary: each finding = \`location\` (File:Line), \`severity\` ∈ {${SEVERITIES}}, \`description\` (the CONCRETE FAILURE CASE — inputs/state -> wrong output — not a retelling of the diff), \`recommendation\` (the change, in one or two lines); verdict ∈ {${VERDICTS}}. ${TEXT_SHAPE} DO NOT FILE NEW ISSUES. This is a hard rule, and it overrides any habit of deferring work to a follow-up card: a debt you find in this diff is resolved IN PLACE, in this same PR, within this story's scope. Never invoke ${SK.writeIssue}, never write \`Deferred to #\`, and never recommend "track this separately" — a finding parked in a fresh card is a finding nobody fixes, and it converts a reviewed PR into an unreviewed backlog. Set \`nonActionable: true\` ONLY if fixing it would be genuinely WRONG — byte-consistent with a source of truth, matching an existing convention, an ALREADY-EXISTING tracked story (cite its number; do not create one), or something that can only resolve after merge. Being outside this story's originally stated scope is NOT a reason: fix it here. Whenever you set \`nonActionable: true\`, ALSO set \`disposition\` with a concrete reason replacing the bare label (\`By convention …\` / \`Historical record\` / \`Already tracked in #\` / \`Resolves after merge\`); never leave "non-actionable" as the only explanation. If a finding is SO large that fixing it here would genuinely swamp the story, say so explicitly in \`description\` and leave it ACTIONABLE — the human decides at the merge gate whether to accept the bigger PR or carve it out; that decision is not yours to pre-empt by filing a card. ${first ? `This is the FIRST review: POST your full review report as a PR comment on #${pr.prNumber} (${REVIEW_TEMPLATE_LABEL} structure), and include the marker line \`${firstReviewMarker}\` VERBATIM as the first line of the comment body — it is an HTML comment (invisible in the rendered markdown, so no visible noise) that lets a later resume detect this first review by an EXACT substring match rather than a semantic reading (finding 1). Then return findings + verdict.` : prevFindings.length + ? `This is a RE-REVIEW: do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Verify these prior findings were genuinely resolved: ${JSON.stringify(prevFindings)}. The last complete review covered immutable head ${prevReviewedHead}. First inspect ONLY the fix delta with \`git diff ${prevReviewedHead}...origin/${story.branch} --name-status\`, then its directly changed producer/consumer contract boundaries. Do NOT re-audit the unchanged PR surface. A new finding is actionable only if it is in this delta or a contract boundary changed by this delta; otherwise report it as a Question for the human, not a new fix round.` + : `This is a RE-REVIEW on a resumed in-flight cycle (round-0 of this run carries no prior findings): do a FRESH, independent full review pass. do NOT post any PR comment (the orchestrator synthesizes the cycle at the end).`} Return findings, verdict, and \`reviewedHead\`: the lower-case 40-character SHA printed by \`git rev-parse origin/${story.branch}\` after your inspection.`, // effort was 'xhigh'. The measured cause of the repeated kills was NOT effort and NOT a // stuck command: transcript timing showed the reviewer issuing a tool call every ~5s // (97 events, mean gap 4.9s, max 49s — zero gaps over 180s) yet still killed, because @@ -1263,10 +1289,9 @@ async function driveStory(story) { // narration reliable, restoring 'xhigh' is legitimate: it costs review depth, which is // the whole point of this gate. Do not read this line as "xhigh causes stalls". withModel({ agentType: 'pair-reviewer', phase: 'Review', label: `rev:${tag} r${round}`, effort: 'high', schema: REVIEW_SCHEMA }), - // A review is USABLE only if it carries a verdict — the same predicate the guard below - // converges on. Without it the retry covered the dead reviewer (`null`) and skipped the - // contentless one (`{}`), which is the shape actually measured on #432. - hasVerdict, + // A review is USABLE only with a verdict and its immutable reviewed head. Without the + // latter, the next pass cannot be an evidence-bounded re-review. + hasReviewEvidence, ) // A DEAD reviewer is not a clean review. `agent()` returns null when the subagent // dies, and `review?.findings ?? []` then yields zero findings — which the @@ -1286,15 +1311,16 @@ async function driveStory(story) { // So the test is inverted: a VERDICT must be present. Absence of findings is not evidence // that a review happened; presence of a verdict is. Every real review emits one — it is a // required field of the contract schema — so this costs a genuine clean review nothing. - // `hasVerdict` is the SAME function `agentRetry` was given above: the contentless return is - // retried once like any other dead step, and only then does it land here. - if (!hasVerdict(review)) + // `hasReviewEvidence` is the SAME function `agentRetry` was given above: a contentless or + // unanchored return is retried once like any other dead step, then lands here. + if (!hasReviewEvidence(review)) // `acceptedFindings` travels on EVERY terminal arm, this one included. A card whose // reviewer dies mid-cycle otherwise reports the by-design and below-floor findings of // every earlier round as if none had been raised — and those are precisely the findings // the fixer never receives, so they are recoverable from nowhere else. AC4 says an // accepted finding always reaches the human; a failure is not an exception to that. return { story, prNumber: pr.prNumber, status: 'failed-review', round, acceptedFindings: accepted, reviewLog: cycleHasRemediation ? reviewLog : undefined } + const reviewedHead = String(review.reviewedHead).toLowerCase() const findings = review.findings ?? [] const allActionable = findings.filter((f) => !f.nonActionable) // Below the floor: still reported, still shown to the human, just not blocking. Marked @@ -1357,11 +1383,12 @@ async function driveStory(story) { round++ prevFindings = actionable + prevReviewedHead = reviewedHead cycleHasRemediation = true // FIX — implementer resumes checkpoint (if present) + resolves actionable findings. // Logs the round to the working review log INSTEAD of posting a per-round PR comment. const fix = await agentRetry( - `Resume story ${tag}. ${wtClause(story)} Read the checkpoint if present (${SK.checkpoint} $mode=resume); otherwise work from the PR diff + code. Resolve EVERY one of these actionable review findings on PR #${pr.prNumber} — including minor/nit, do not defer any: ${JSON.stringify(prevFindings)}. Fix them IN PLACE, in this PR: do NOT file a follow-up issue for any of them, do NOT invoke ${SK.writeIssue}, and do NOT leave a "tracked separately" note in lieu of the fix. If a finding turns out to be genuinely larger than this story, still fix what belongs here and say plainly in the working log what remains — the human decides at the merge gate, not a new card. Follow ${SK.implement} for the change itself (test-first where a finding describes a defect), verify with ${SK.verifyQuality} (tier-resolved — do not improvise a gate command), and record any decision a finding forces with ${SK.recordDecision}. Commit and push. Then re-invoke **${SK.publishPr}**: it is create-or-update and idempotent, and re-running it is what keeps the PR body, the classification tags and the \`pr-state:*\` label in sync with the NEW head commit instead of describing the pre-fix state. As in the open-PR step it will emit \`Review: review-dispatch-required\` rather than nesting — expected: this orchestrator drives the re-review. ${TEXT_SHAPE} Re-running it REWRITES the PR body, and this is the only step that does so once a cycle is under way: rewrite it to describe the CURRENT head, do not append a round-by-round history — a body that grows by one section per fix round is re-read in full by every later reviewer of this same cycle. Do NOT post a remediation PR comment; INSTEAD append this round to the working log \`${reviewLog}\` (create it if absent) as a COMPACT TABLE under a \`## Round N\` heading — one row per finding, columns \`severity | location | what changed | commit\`. One row, one line: no paragraph per finding, and do not restate the finding's description (its location identifies it). Add prose ONLY where a fix diverged from the recommendation, and then only the reason. Only for a genuine design disagreement set needsHumanDecision instead of forcing a fix. Do NOT merge.`, + `Resume story ${tag}. ${wtClause(story)} Read the checkpoint if present (${SK.checkpoint} $mode=resume); otherwise work from the PR diff + code. Resolve EVERY one of these actionable review findings on PR #${pr.prNumber} — including minor/nit, do not defer any: ${JSON.stringify(prevFindings)}. Fix them IN PLACE, in this PR: do NOT file a follow-up issue for any of them, do NOT invoke ${SK.writeIssue}, and do NOT leave a "tracked separately" note in lieu of the fix. If a finding turns out to be genuinely larger than this story, still fix what belongs here and say plainly in the working log what remains — the human decides at the merge gate, not a new card. CONVERGENCE SWEEP (mandatory): the finding location is the starting point, not the contract boundary. Before changing code, make a finite map of the same observable contract: the reported case and its paired success/failure path; any state transition or resume path the contract owns; and the canonical source plus every distributed representation of that behavior (generated asset, dataset, installed copy, or documented command). Change every map cell required for that one contract, then stop — do not use the sweep for unrelated cleanup, new behavior, or speculative hardening. For a generated/distributed artifact, resolve the canonical source from the asset registry, edit only that source, then run the declared generator/installer and inspect its output; never hand-edit a derived copy. PROVISIONED ARTIFACT CONTRACT (mandatory when a change installs, builds, publishes, names, or invokes an executable/package): map \`producer -> published identity -> consumer\` — for example installer/release step -> package manifest/bin/file/export -> workflow or user command. Prove the exact path in a clean temporary environment using the real built or installed artifact. Never stub, alias, or fake the exact producer, published identity, or consumer boundary; external effects may be isolated only after that boundary is crossed. For each logic defect, write a test that executes the real function/script against a real or realistic fixture and asserts output/side effects, never a source-string regex. Re-run the finding's evidence command and the mapped boundary cases before commit. Follow ${SK.implement} for the change itself: its TDD discipline and adoption-compliance phase are mandatory. Verify with ${SK.verifyQuality} (tier-resolved — do not improvise a gate command), and record any decision a finding forces with ${SK.recordDecision}. Commit and push. Then re-invoke **${SK.publishPr}**: it is create-or-update and idempotent, and re-running it is what keeps the PR body, the classification tags and the \`pr-state:*\` label in sync with the NEW head commit instead of describing the pre-fix state. As in the open-PR step it will emit \`Review: review-dispatch-required\` rather than nesting — expected: this orchestrator drives the re-review. ${TEXT_SHAPE} Re-running it REWRITES the PR body, and this is the only step that does so once a cycle is under way: rewrite it to describe the CURRENT head, do not append a round-by-round history — a body that grows by one section per fix round is re-read in full by every later reviewer of this same cycle. Do NOT post a remediation PR comment; INSTEAD append this round to the working log \`${reviewLog}\` (create it if absent) as a COMPACT TABLE under a \`## Round N\` heading — one row per finding, columns \`severity | location | what changed | commit\`. One row, one line: no paragraph per finding, and do not restate the finding's description (its location identifies it). Add prose ONLY where a fix diverged from the recommendation, and then only the reason. Only for a genuine design disagreement set needsHumanDecision instead of forcing a fix. Do NOT merge.`, withModel({ agentType: 'pair-implementer', phase: 'Review', label: `fix:${tag} r${round}`, effort: 'high', schema: FIX_SCHEMA }), ) // failed-fix: the fixer died mid-round; a partial working log may exist. Surface diff --git a/.claude/workflows/pair-implement-batch.test.mjs b/.claude/workflows/pair-implement-batch.test.mjs index b3360499b..0a2747bb7 100644 --- a/.claude/workflows/pair-implement-batch.test.mjs +++ b/.claude/workflows/pair-implement-batch.test.mjs @@ -25,12 +25,25 @@ const SRC = readFileSync(new URL('./pair-implement-batch.js', import.meta.url), '', ) const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor +const REVIEWED_HEAD = 'a'.repeat(40) async function runWorkflow({ args, dispatch }) { const calls = [] const agent = async (prompt, opts) => { calls.push({ prompt, opts }) - return dispatch(prompt, opts) + const result = await dispatch(prompt, opts) + // A real reviewer now returns the immutable revision it reviewed. Keep legacy + // fixtures concise while allowing focused tests to provide an invalid/missing + // value explicitly. + if ( + opts.agentType === 'pair-reviewer' && + result && + typeof result === 'object' && + String(result.verdict ?? '').trim() && + result.reviewedHead === undefined + ) + return { ...result, reviewedHead: REVIEWED_HEAD } + return result } // Mirrors the real primitive's contract: "a thunk that throws (or whose agent errors) // resolves to null in the result array — the call itself never rejects". The earlier @@ -105,7 +118,14 @@ test('valid contract: reviewer schema derives from contract.json (AC1) and cache dispatch: stdDispatch({ contractResult: { status: 'cache-hit', contract } }), }) const rev = calls.find(c => c.opts.agentType === 'pair-reviewer') - assert.deepEqual(rev.opts.schema, contract.schema) + assert.deepEqual(rev.opts.schema, { + ...contract.schema, + properties: { + ...contract.schema.properties, + reviewedHead: { type: 'string', pattern: '^[0-9a-f]{40}$' }, + }, + required: ['verdict', 'reviewedHead'], + }) assert.ok(rev.prompt.includes('Blocker'), 'severity vocabulary threaded from the contract') assert.ok(rev.prompt.includes('Rework'), 'verdict vocabulary threaded from the contract') assert.deepEqual(result.contracts, [{ name: 'code-review', status: 'cache-hit' }]) @@ -212,8 +232,16 @@ test('contract with usable schema but missing canonical vocabulary keys: prompt dispatch: stdDispatch({ contractResult: { status: 'cache-hit', contract } }), }) const rev = calls.find(c => c.opts.agentType === 'pair-reviewer') - // Schema is still enum-locked from the (structurally usable) contract... - assert.deepEqual(rev.opts.schema, contract.schema) + // Schema is still enum-locked from the (structurally usable) contract, with + // the orchestration-owned reviewed revision layered on top. + assert.deepEqual(rev.opts.schema, { + ...contract.schema, + properties: { + ...contract.schema.properties, + reviewedHead: { type: 'string', pattern: '^[0-9a-f]{40}$' }, + }, + required: ['verdict', 'reviewedHead'], + }) // ...but the prompt vocabulary text falls back to the documented defaults, // since verdictOptions/severities (the canonical keys it's threaded from) // are absent. In practice ensure-contract.mjs's validateContract now rejects @@ -1002,6 +1030,106 @@ test('the fix step is likewise barred from deferring a finding into a new issue' ) }) +test('the fix step sweeps the bounded contract surface before re-review', async () => { + const finding = { location: 'x.ts:1', severity: 'Major', description: 'd', recommendation: 'r' } + let round = 0 + const { calls } = await runWorkflow({ + args: { stories: [STORY] }, + dispatch: (prompt, opts) => { + if (opts.agentType === 'pair-contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.agentType === 'pair-reviewer') return round++ === 0 ? { verdict: 'Rework', findings: [finding] } : { verdict: 'Approved', findings: [] } + if (opts.phase === 'Implement') return { gatesPassed: true, branch: 'b' } + if (opts.phase === 'PR') return { prNumber: 7 } + return { fixed: true } + }, + }) + + const fix = calls.find(c => c.opts.label?.startsWith('fix:')).prompt + assert.match(fix, /CONVERGENCE SWEEP/, 'the fixer must make the bounded contract explicit') + assert.match(fix, /location is the starting point/i, 'a finding location is not the contract boundary') + assert.match(fix, /success\/failure/i, 'paired execution paths are checked together') + assert.match(fix, /every distributed representation/i, 'source and shipped representations are checked together') + assert.match(fix, /PROVISIONED ARTIFACT CONTRACT/, 'a provisioned command has an explicit end-to-end check') + assert.match(fix, /producer.*published identity.*consumer/i, 'the provisioner, artifact metadata and invocation are mapped together') + assert.match(fix, /clean temporary environment/i, 'the actual installed or built artifact is exercised') + assert.match(fix, /never stub.*boundary/i, 'a stub cannot stand in for the published command boundary') + assert.match(fix, /unrelated cleanup/i, 'the sweep stays bounded and is not scope creep') + assert.doesNotMatch(fix, /touch ONLY what each finding's location names/, 'line-only scope discipline would recreate the gap') +}) + +test('re-review is anchored to the reviewed revision and checks only the fix delta plus prior findings', async () => { + const finding = { location: 'workflow.yml:4', severity: 'Major', description: 'd', recommendation: 'r' } + let round = 0 + const { calls } = await runWorkflow({ + args: { stories: [STORY] }, + dispatch: (prompt, opts) => { + if (opts.agentType === 'pair-contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.agentType === 'pair-reviewer') + return round++ === 0 + ? { verdict: 'Rework', findings: [finding] } + : { verdict: 'Approved', findings: [] } + if (opts.phase === 'Implement') return { gatesPassed: true, branch: 'b' } + if (opts.phase === 'PR') return { prNumber: 7 } + return { fixed: true } + }, + }) + + const reviews = calls.filter(c => c.opts.agentType === 'pair-reviewer') + assert.match(reviews[0].prompt, /reviewedHead/i, 'every review returns the immutable head it covered') + assert.match(reviews[1].prompt, new RegExp(`git diff ${REVIEWED_HEAD}\\.\\.\\.origin/feat/#292-x --name-only`), 're-review inventories the fix delta, not the entire PR') + assert.match(reviews[1].prompt, new RegExp(`git diff ${REVIEWED_HEAD}\\.\\.\\.origin/feat/#292-x`), 're-review starts from the previous review baseline') + assert.match(reviews[1].prompt, /only if it is in this delta or a contract boundary changed by this delta/i, 'unchanged PR surface is not repeatedly re-audited') +}) + +test('a review without an immutable baseline cannot converge', async () => { + const { result, calls } = await runWorkflow({ + args: { stories: [STORY] }, + dispatch: (prompt, opts) => { + if (opts.agentType === 'pair-contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.agentType === 'pair-reviewer') return { verdict: 'Approved', findings: [], reviewedHead: 'not-a-sha' } + if (opts.phase === 'Implement') return { gatesPassed: true, branch: 'b' } + if (opts.phase === 'PR') return { prNumber: 7 } + return { fixed: true } + }, + }) + + assert.equal(result.batch[0].status, 'failed-review') + assert.equal(calls.filter(c => c.opts.agentType === 'pair-reviewer').length, 2, 'missing review evidence is retried once') +}) + +test('a review baseline must be lower-case like the review contract declares', async () => { + const { result } = await runWorkflow({ + args: { stories: [STORY] }, + dispatch: (prompt, opts) => { + if (opts.agentType === 'pair-contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.agentType === 'pair-reviewer') return { verdict: 'Approved', findings: [], reviewedHead: 'A'.repeat(40) } + if (opts.phase === 'Implement') return { gatesPassed: true, branch: 'b' } + if (opts.phase === 'PR') return { prNumber: 7 } + return { fixed: true } + }, + }) + + assert.equal(result.batch[0].status, 'failed-review') +}) + +test('accepted-findings key is collision-free for location and description pairs', async () => { + const { result } = await runWorkflow({ + args: { stories: [STORY] }, + dispatch: stdDispatch({ + contractResult: { status: 'cache-hit', contract: validContract() }, + review: { + verdict: 'Approved', + findings: [ + { location: 'a b', severity: 'Minor', description: 'c', nonActionable: true }, + { location: 'a', severity: 'Minor', description: 'b c', nonActionable: true }, + ], + }, + }), + }) + + assert.equal(result.batch[0].acceptedFindings.length, 2) +}) + // ── A run that drove nothing must not report success ─────────────────────── // Observed: two workflows were launched concurrently on a saturated machine, every // implementer stalled past the supervisor's window, `parallel` returned six nulls, diff --git a/.pair/adoption/decision-log/2026-08-30-atomicity-primitives-use-node-fs-directly.md b/.pair/adoption/decision-log/2026-08-30-atomicity-primitives-use-node-fs-directly.md new file mode 100644 index 000000000..bbbeddeae --- /dev/null +++ b/.pair/adoption/decision-log/2026-08-30-atomicity-primitives-use-node-fs-directly.md @@ -0,0 +1,51 @@ +# Decision: the two atomicity primitives (exclusive create, append) use `node:fs` directly, in leaf modules tested against a real temporary directory + +## Date + +2026-08-30 + +## Status + +Active + +## Category + +Convention Adoption + +## Context + +Story #217's dispatch needs two filesystem operations whose whole value is **atomicity**: + +- an **exclusive create** — the per-card lock that guarantees a trigger burst never starts two runs on one card; +- an **append** — the audit trail, whose lines must survive two dispatches writing the same file concurrently. + +The project's convention is dependency injection through `FileSystemService`, with an `InMemoryFileSystemService` double instead of mocks. That service exposes neither primitive: `mkdirSync` is modelled in the double as "add the path to a set" (it cannot fail a second create at all), and the only write is `writeFile`, a full overwrite — so an append would have to be read-concat-write, which reintroduces exactly the lost update `O_APPEND` exists to prevent. + +Widening `FileSystemService` was the obvious alternative, and it is the one worth stating why we did not take. + +## Decision + +`card-lock.ts` and `dispatch-audit.ts` call `node:fs` **directly** (`mkdirSync` without `recursive`, `appendFileSync`), and are: + +- **leaf modules** — nothing else in the dispatch path touches the filesystem, so the untestable surface is two small files rather than a layer; +- **injected at the call site** — the handler takes a `LockAcquirer` and an `AuditAppender`, so every other test in the run pipeline stays hermetic and none of them touches a real working area; +- **tested against a real temporary directory** (`mkdtempSync`), because the properties under test — a second create fails, two appends both survive — are properties of the real filesystem and of nothing else. There is precedent in this repo: `path-containment.test.ts` tests symlink containment the same way, for the same reason. + +The rule generalises: **when the behaviour under test IS an atomicity or containment guarantee of the operating system, test it against the operating system.** A double that cannot fail the way production fails proves nothing, and asserting against it is worse than not asserting — it reads like coverage. + +## Alternatives Considered + +- **Add `mkdirExclusive`/`appendFile` to `FileSystemService`**: correct in principle, but it widens a package shared by every other story in flight for two callers, and the in-memory double would still have to *simulate* the failure mode — so the double's fidelity, not the filesystem's behaviour, is what the tests would end up asserting. Reconsider when a third caller appears. +- **Read-concat-write the audit through `writeFile`**: loses records when two dispatches on different cards write the same audit file; the per-card lock does not protect a shared file. +- **Lock with `existsSync` + `mkdirSync`**: a check-then-act window, which is the exact race the lock exists to close. + +## Consequences + +- Two modules in `apps/pair-cli/src/commands/run/` bypass `FileSystemService`, each carrying a comment saying why and pointing here. +- Their tests are slower than the rest of the suite (real I/O in `os.tmpdir()`), and clean up after themselves. +- Handler-level tests inject fakes for both, so the dispatch pipeline remains testable in memory. +- A future third caller for either primitive is the trigger to revisit and put it on `FileSystemService` properly. + +## Adoption Impact + +- `adoption/tech/way-of-working.md` — Quality Gates section: records the exception to the "avoid mocks, use the in-memory double" convention for OS atomicity/containment guarantees. diff --git a/.pair/adoption/decision-log/2026-08-30-empty-card-tags-is-an-observation-not-a-malformed-flag.md b/.pair/adoption/decision-log/2026-08-30-empty-card-tags-is-an-observation-not-a-malformed-flag.md new file mode 100644 index 000000000..8b02795b7 --- /dev/null +++ b/.pair/adoption/decision-log/2026-08-30-empty-card-tags-is-an-observation-not-a-malformed-flag.md @@ -0,0 +1,48 @@ +# Decision: an empty `--card-tags` means "this card carries no labels", not a malformed flag + +## Date + +2026-08-30 + +## Status + +Active + +## Category + +Convention Adoption + +## Context + +`pair run` refuses every flag passed with an empty value — `--root ""`, `--filter ""`, `--skill ""` all fail at parse time, deliberately: a flag named with nothing behind it is a caller bug, and accepting it silently is how an unattended run ends up doing something nobody asked for. + +Story #217's `--card-tags` inherited that rule, and the end-to-end test on a populated board (T5) showed it was the wrong rule for this one flag. The dispatch entry point is called by a **host trigger**, and the reference GitHub adapter renders the labels it observed as `join(github.event.issue.labels.*.name, ',')`. On an issue with **no labels** that expression renders `""`. So the very state AC2 is about — "an issue with no mapped tag runs nothing" — arrived at the parser as an empty value and was rejected with `--card-tags was passed with an empty value`, exit 1. + +Two consequences, both bad, and neither visible from inside the module suites (they pass tag lists, not the empty string a host renders): + +- the opt-in boundary of the whole feature — untagged ⇒ skipped, reported, exit 0 — became **unreachable through the entry point**; +- the commonest card on any board turned every trigger firing on it into a **failed CI job**, which is the noise that gets a trigger disabled. + +## Decision + +For `--card-tags`, and only for it, an **empty or whitespace-only value is data**: it is read as the observation "the trigger saw no labels on this card", producing an empty tag list. The dispatcher then does what it does for any card with no mapped tag — skips it, reports the reason, appends the skip to the audit trail, exits `0`. + +A **hole inside a list** stays an error: `auto-dev,,risk:green` still HALTs. The two cases are genuinely different. An empty value is a complete observation of an empty set; a hole is an incomplete rendering of a non-empty one — the caller built a list and lost an item, which is exactly the string-interpolation bug worth failing on. + +The general rule this instantiates: **a flag that carries an observation from an external system is empty-valid when the empty case is a real state of that system; a flag that carries an operator's intent is not.** `--root`, `--filter` and `--skill` are intent — nobody means "" by them. `--card-tags` is an observation, and "no labels" is a state of every board. + +## Alternatives Considered + +- **Keep the refusal, make the adapter skip the call when the label list is empty**: pushes an authorization-relevant decision — "should this card run?" — into every per-host adapter, where it is untested, duplicated per host, and free to drift. ADR-024 puts that decision in the routing core precisely so no adapter can widen or narrow it. +- **Keep the refusal, have the adapter pass a sentinel** (`--card-tags "(none)"`): invents a label that could collide with a real one and makes the trail lie about what the trigger saw. +- **Accept empty values on every flag**: loses the guard where it earns its keep — an empty `--root` or `--skill` is a caller bug with no legitimate reading. + +## Consequences + +- `apps/pair-cli/src/commands/run/parser.ts` reads an empty/whitespace `--card-tags` as an empty tag list; the empty-entry HALT for a hole inside a list is unchanged. +- An unlabelled card now produces the documented skip and exit `0` end-to-end, so a host adapter needs no pre-filter and no conditional call. +- The asymmetry between this flag and its neighbours is deliberate and must stay documented where a reader meets it: the parser module, the CLI reference, and the reference adapter in the KB. + +## Adoption Impact + +- `adoption/tech/way-of-working.md` — CLI conventions: records that flags carrying an external observation are empty-valid when the empty case is a real state of the observed system, while flags carrying operator intent are not. diff --git a/.pair/adoption/decision-log/2026-08-31-review-baseline-and-provisioned-artifact-contract.md b/.pair/adoption/decision-log/2026-08-31-review-baseline-and-provisioned-artifact-contract.md new file mode 100644 index 000000000..79afd4533 --- /dev/null +++ b/.pair/adoption/decision-log/2026-08-31-review-baseline-and-provisioned-artifact-contract.md @@ -0,0 +1,58 @@ +# Decision: Review re-checks use an immutable baseline and prove provisioned artifacts + +## Date + +2026-08-31 + +## Status + +Active + +## Category + +Process Decision + +## Context + +PR #474 / story #217 ran on `89793d27`, which already required a convergence sweep. In round 4, +the fixer added a CLI installation step and retained a runner invocation, but tested a stub named +`pair` rather than the installed package's declared `pair-cli` bin. Round 5 therefore found the +new functional defect. The generic sweep named distributed representations but did not require an +end-to-end proof across installation, published identity, and invocation. Re-review also rescanned +the whole accumulated PR, so each fix expanded the next review surface. + +## Decision + +1. Every reviewer return includes the lower-case 40-character SHA it inspected (`reviewedHead`). + Missing or invalid evidence is retried once then fails closed; it never converges a PR. +2. The initial review remains complete. Each later re-review verifies prior findings plus + `git diff ...origin/` and directly changed producer/consumer boundaries. + A new blocking finding must come from that delta or boundary; an unchanged surface is not + re-audited as a new fix round. +3. A fix touching an installed, built, published, named, or invoked artifact maps + `producer -> published identity -> consumer` and proves it in a clean temporary environment + using the real artifact. The exact boundary cannot be stubbed, aliased, or faked. + +## Alternatives Considered + +- **Keep full-PR scans on each round**: rejected — a growing diff makes a re-review another + independent first review and creates unbounded new scope. +- **Accept findings after a fixed round cap**: rejected — it hides genuine defects rather than + bounding their cause. +- **Only strengthen source-string tests**: rejected — #217 passed such a test while the published + CLI contract was broken. + +## Consequences + +- Re-review is bounded without downgrading Major or Minor findings. +- A caller resuming an older cycle has one fresh full review to establish a new baseline. +- Workflow authors must keep dataset source and root mirror byte-identical; tests cover both. + +## Adoption Impact + +- `.pair/adoption/tech/way-of-working.md`: records the review baseline and provisioned-artifact + proof convention. +- `packages/knowledge-hub/dataset/.workflows/pair-implement-batch.js` and its installed mirror: + enforce the convention. +- Both dry-run copies of `pair-implement-batch.test.mjs`: pin the schema, bounded re-review, and + real-artifact prompt requirements. diff --git a/.pair/adoption/tech/adr/adr-024-tag-driven-dispatch-agnostic-core-host-adapter.md b/.pair/adoption/tech/adr/adr-024-tag-driven-dispatch-agnostic-core-host-adapter.md new file mode 100644 index 000000000..3d8ec5304 --- /dev/null +++ b/.pair/adoption/tech/adr/adr-024-tag-driven-dispatch-agnostic-core-host-adapter.md @@ -0,0 +1,86 @@ +# ADR-024: Tag-driven dispatch — the mapping is adoption, the routing core is host-agnostic, the on-issue record belongs to the host adapter + +## Status + +Accepted + +## Date + +2026-08-30 + +## Context + +- Story #217 (R4.4, epic #212) asks for **tag-driven workflows**: different tags trigger different workflows, exclusively on tagged issues, with the tag→workflow mapping declared in adoption. Automation must start **only** where a team explicitly enabled it, and each tag must route to the right behavior. +- The pieces around it already exist and must not be re-decided: `## Eligibility` (#216) selects *which cards* an unattended run may pick up; `pair-next` stays the frozen selector atom (ADR-017 §1); `pair run` is the portable execution adapter and the ADR-021 tier-2 entry point (#451); `pair-loop` and the other process skills are the workflows themselves. +- What was **not** decided anywhere: where the tag→workflow mapping lives, who evaluates it, how the "run start is recorded on the issue" (AC3) happens given that the driver deliberately holds **no tracker credentials**, and how a trigger burst is prevented from starting two runs on one card. +- **Hard to reverse**: the mapping's grammar becomes adoption content in every adopting project, and the entry point's flags become a public CLI contract that host triggers are written against. +- **Surprising without context**: that the driver is *told* the card's labels instead of reading them from the tracker; that an unmapped card is skipped by *absence of a route* rather than by a guard; and that the on-issue comment is emitted as a line on stdout for someone else to post. +- **Real trade-off**: the alternatives (a dispatcher inside the loop skill, a tracker client inside the CLI) were both available and were rejected for reasons recorded below. + +## Options Considered + +### Option 1: routing inside the loop skill (`pair-loop` reads the mapping and decides) + +- **Description**: the mapping stays in `tech/automation.md`, but the agent skill reads it, matches tags and picks the workflow. +- **Pros**: no new CLI surface; the skill already reads the policy file. +- **Cons**: the routing decision — an **authorization** decision, since "untagged ⇒ never" is the opt-in boundary — would live in prose executed by an LLM, with no test able to pin it. Every safety property of the story (untagged never runs, no default workflow, no silent multi-tag choice) would be unverifiable by anything but another prompt. + +### Option 2: the CLI grows a tracker client and reads the card's labels itself + +- **Description**: `pair run --card 217` fetches the issue from the code host, reads its labels, then routes. +- **Pros**: one argument instead of two; the operator cannot pass stale labels. +- **Cons**: puts host credentials and a per-host API client into the driver, which is the one component that is deliberately host-agnostic — and multiplies by every tracker pair supports. It also duplicates what the trigger already knows: a host workflow firing on a label event *has* the labels in hand. + +### Option 3 (chosen): adoption mapping + a pure routing core in the entry point, fed by a thin per-host adapter + +See Decision. + +## Decision + +1. **The mapping is adoption data**: a seventh section of the optional `.pair/adoption/tech/automation.md`, `## Workflows`, with entries ` ⇒ ` and an optional `Precedence:` line. Its schema is owned by the KB guideline `collaboration/automation/automation-policy.md` (D21: adoption is the delta, the KB is the schema). The **tag is an opaque routing key** and the **workflow is a skill name** — so no classification criterion ever lives in code (D18). The *set of nameable workflows* is not open, and item 7 is why: it is the KB catalog, held in the driver as data and asserted equal to the guideline's table by test. + +2. **The routing core is a pure function in the entry point** (`pair run`, ADR-021 tier 2): `dispatch.ts` takes the card, the labels a trigger observed, the policy and an installed-skill probe, and returns *route* or *skip*, or HALTs. It performs no I/O, holds no credentials, and knows nothing about the tracker. The order is normative — **mapping → eligibility → routing** — so an ineligible card is skipped before its tags are read at all. + +3. **The card's labels are an input, not a lookup**: `pair run --card --card-tags `. The trigger's own **thin per-host adapter** (a GitHub Actions job, a webhook runner) supplies both, under the credentials it already runs with. Adding a code host is a new adapter, never a change to the core. Both values are untrusted host data and are content-checked at parse time, exactly as `--root`/`--filter` already are. + +4. **The on-issue audit is split, and the split is the point**: every decision (start/skip/end) is appended to the run's `## Audit Location` file, and the `start` record — **and only that one** — is *also* printed as a single `DISPATCH-RECORD:` line for the host adapter to post as a comment on the card. The driver writes files and prints lines; it never posts to a tracker. Skips and ends stay in the file deliberately: a card that gets a comment for every unmapped label edit is unreadable within a day, and the `end` duplicates on the card what the trail already holds. AC3 asks for the run *start* on the issue, and that is exactly what ships. + +5. **A trigger burst never starts two runs on one card**: the dispatch takes an **exclusive per-card lock** (an atomic `mkdir` under `working_path`) before spawning and releases it unconditionally afterwards — including when the run throws, which also writes the `end` record (`outcome=crashed`) rather than leaving the trail stopped at `start`. A locked card is **skipped and logged**, never queued — it is still tagged, so the next trigger picks it up — and the skip reports the holder's directory and how long it has been held, because nothing reaps a lock (see the limitation below). + +6. **Fail-safe everywhere, in one direction**: no `## Workflows` section ⇒ "no mapping declared", clean exit; no mapped tag ⇒ skip; ineligible ⇒ skip; a workflow that is not installed, a workflow whose scoping argument the driver cannot spell (item 7), or a multi-tag card with no covering `Precedence:` ⇒ **HALT** with an adoption-fix message. Nothing ever falls back to a default workflow. + +7. **A dispatched card IS the run's scope — under the routed workflow's own name for it, and nothing displaces it.** The card travels as an argument the workflow declares (`--root` for `pair-loop`, `--story` for `pair-process-plan-tasks`), borrowed from its `## Arguments` table and never invented (D18); the mapping from the driver's scope slot to each workflow's spelling is DATA in `invocation.ts`, pinned against the dataset corpus by a test. Three refusals hold that property up, and all three are needed: + - **`--root` (and `--skill`/`--prompt`) alongside `--card` is refused at parse time.** A dispatched card is the whole answer to "what is this run about", and an operator or wrapper flag answering it a second time is not a narrowing: `--card 217 --root 300` would drive the agent over subtree 300 while the audit trail, the `DISPATCH-RECORD:` comment and the exclusive lock all named 217 — 300 unguarded, 217 credited with work nothing did on it. The handler additionally reads the dispatched card *before* `config.scope.root`, so the outcome stays unreachable for a caller that skips the parser. + - **A mapped workflow outside the KB catalog is refused**, even when installed and even when the driver knows how it spells its scope. The mappable set (`DISPATCHABLE_WORKFLOWS`) is its own declaration, deliberately NOT derived from the argument table: `pair-next` has a row there because `--skill pair-next --root 212` is a legitimate hand-driven run, and routing a card to it would take the card's lock and post a `DISPATCH-RECORD:` comment for a run that prints a recommendation and changes nothing. The set is asserted EQUAL to the guideline's catalog table, in both directions. + - **A catalogued workflow the driver holds no scoping row for is refused.** An argument a skill does not declare is *ignored*, not rejected, and the `pair-process-*` workflows then select the highest-priority story on the board themselves — the run works a card nobody tagged while the trail names the card that was. + + Refusing is in every case the only outcome that keeps item 4's record true. + +8. **The mappable set admits only workflows that can finish with nobody watching.** A dispatch spawns its workflow under the operator's one-time `--autonomous` opt-in, holds the card's exclusive lock for the run, and has already posted a public `DISPATCH-RECORD:` comment saying a run started. A workflow whose own SKILL.md requires an explicit human decision has exactly two outcomes there, and both are worse than not running: it **stalls** on a question no one answers until the per-iteration timeout, or the agent — having no interlocutor — **supplies its own approval** and drives the card past the gate, satisfying the authorization control with the party it exists to constrain. So `pair-process-refine-story` is NOT mappable, even though the driver knows exactly how it spells its scope (`--story`): it is the single Draft→Ready path, its phase 0 is "the R3.11 AI↔human alignment gate — a prerequisite, not optional", it adds three per-step `Human-judgment gate`s, and it states that "what is never skipped is explicit human alignment before the story reaches `Ready`" (R3.11, D24). It keeps its `SKILL_PARAMETERS` row, because `--skill pair-process-refine-story --root ` is a legitimate HAND-DRIVEN run — someone is there to answer. The rule is enforced against the skills' own SKILL.md by a KB conformance guard, so putting a row back into the catalog table fails a test rather than shipping. + + Deliberately NOT enforced via `$approval`: none of the mappable workflows declares that argument either (`pair-loop` composes the family that does, `pair-process-plan-tasks` has no approval round at all), so a "must declare `$approval`" gate would refuse the whole catalog. What distinguishes the excluded case is a human-judgment gate in its own steps, not an argument on its interface. + +## Consequences + +### Benefits + +- Every safety property of the feature is a **tested production module**, not prose: untagged-never, eligibility-before-routing, no-silent-choice and one-run-per-card each have unit tests, and the KB's normative claims have a conformance guard. +- The driver stays credential-free and tracker-agnostic; pair can gain a host by gaining an adapter. +- The mapping composes existing skills, so a "workflow" costs a line of adoption rather than an engine. +- `pair-next` and the eligibility filter are untouched — dispatch narrows what runs, it never widens what is selected. + +### Trade-offs and Limitations + +- **Labels are as fresh as the trigger that passed them.** A card whose tag changed between the trigger firing and the dispatch starting is routed on the observed value. Accepted: re-reading them would require the tracker client this ADR exists to avoid, and the eligibility label is re-checked by the invoked skill on every iteration anyway. +- **`--card-tags` is comma-separated**, so a label containing a comma is not routable. Same over-inclusive direction `## Eligibility` already takes: the fix is to rename or re-project the label, never to widen the separator. +- **The on-issue comment is the adapter's job**, so a project whose adapter does not post it gets the file trail only. Documented per host in the KB rather than silently degraded. +- **Tag-driven automation does not cover refinement** (item 8). A team wanting Draft→Ready unattended gets nothing from this feature: the only Draft→Ready path requires a human, so refinement stays hand-driven (or batch-driven with a human present). Accepted rather than worked around — an unattended path past that gate would be a change to D24, not to this ADR. +- **The lock is filesystem-local**: two runners on different machines sharing no working area can still collide. Bounded by the same working area every other run artifact already assumes; a distributed lock is out of scope and out of the story's stated isolation model. **Consequence for the reference adapter**, stated because it inverts what a reader assumes: on GitHub-hosted runners every job checks out a fresh workspace, so the lock can never observe a holder from another job and the host's `concurrency` group is the cross-job guard there. Every path that dispatches a card must sit in that group; the per-card lock is the guard on the *persistent-daemon* deployments (the tutorial's Options A–C), where the host offers none. +- **Nothing reaps a lock.** A run killed by SIGKILL, an OOM kill or a job timeout leaves the directory behind, and every later trigger on that card then skips, exits `0` and looks exactly like a healthy burst — automation silently off for one card. Mitigated, not solved: the skip prints the holder's path and age (`holder.json`'s `acquiredAt`), and the KB's pre-flight documents clearing it. A TTL was rejected as the wrong default — a lock that expires while its run is alive re-creates the race the lock exists for, and no timeout is right for every workflow a mapping can name. +- **A mapping naming an uninstalled workflow — or one outside the KB catalog — HALTs the whole board**, not just the cards carrying that tag: both checks run before eligibility and routing. The second bounds what a mapping may name to the catalog's two: a project mapping a tag to any other skill is refused rather than dispatched blind, and widening that set is a deliberate change to the guideline's table plus the one-line data edit the equality assertion then demands. Deliberate — a broken mapping is broken configuration, and surfacing it only on whichever card happens to carry the tag would make the failure depend on which trigger fired first — but the blast radius is a property adopters must be told about, so it is stated in both the schema and the adapter's pre-flight. + +## Adoption Impact + +- `adoption/tech/architecture.md` — records tag-driven dispatch as the entry point's routing layer, and the agnostic-core / host-adapter boundary. +- `adoption/tech/automation.md` — **unchanged on purpose**: this project declares no `## Workflows` section, so tag-driven dispatch stays off here. The absent-section path is the shipped default and the one this repo exercises. +- KB (`packages/knowledge-hub/dataset/.pair/knowledge/...` + the root mirror) — `automation-policy.md` gains the `## Workflows` schema; `github-automation.md` gains the reference host adapter. diff --git a/.pair/adoption/tech/architecture.md b/.pair/adoption/tech/architecture.md index 12d006c28..4ce2a623f 100644 --- a/.pair/adoption/tech/architecture.md +++ b/.pair/adoption/tech/architecture.md @@ -38,6 +38,15 @@ - Canonical target (`.claude/skills/`) receives physical copies; secondary targets receive symlinks. - Windows environments fall back to copy mode (symlinks rejected at validation time). See [ADR-005](adr/adr-005-skills-infrastructure.md). +## Unattended Dispatch + +- **Tag-driven dispatch is opt-in, per card, and declared in adoption.** `## Workflows` in `tech/automation.md` maps a tag to the workflow (a skill name) that runs on a card carrying it; the tag is an opaque routing key and no classification criterion ever lives in the routing code (D18). A card with no mapped tag runs nothing — there is no default workflow. See [ADR-024](adr/adr-024-tag-driven-dispatch-agnostic-core-host-adapter.md). +- **The routing core is host-agnostic and credential-free.** It lives in the `pair run` entry point (ADR-021 tier 2) as a pure function over the card, the labels a trigger observed, and the policy; the labels are an INPUT (`--card`/`--card-tags`) supplied by a thin per-host trigger adapter, never fetched by the driver. Adding a code host is a new adapter, never a change to the core. +- **A dispatched card reaches its workflow under that workflow's own argument name** (`--root` for `pair-loop`, `--story` for `pair-process-refine-story`/`pair-process-plan-tasks`), borrowed from its `## Arguments` table and never invented (D18). A mapping naming a workflow outside the KB catalog — or one the driver holds no such row for — **HALTs** with the uninstalled-workflow check, before eligibility and routing: an undeclared argument is ignored rather than rejected, and a workflow that picks its own subject when unscoped would then work a card nobody tagged while the trail below named the card that was. The mappable set is a declaration of its own (`DISPATCHABLE_WORKFLOWS`), asserted equal to the guideline's catalog table — knowing how a skill spells its scope is not what makes a tag allowed to route a card to it, and neither does it license routing to a workflow that needs a human: `pair-process-refine-story` is scopable, hand-drivable and deliberately NOT mappable, because its alignment gate ends only on an explicit human approval (ADR-024 item 8). +- **Nothing displaces the dispatched card as the run's scope.** `--root`, like `--skill`/`--prompt`, is refused alongside `--card` at parse time, and the handler reads the dispatched card before `config.scope.root`: an operator flag that silently outranked the mapping would drive the agent over one subtree while the audit trail, the on-issue record and the exclusive lock all named another (ADR-024 item 7). +- **The audit trail is split accordingly**: every decision is appended to the `## Audit Location` file, and the run-start record — only that one — is printed as a `DISPATCH-RECORD:` line for the host adapter to post on the card. Skips and endings stay in the file. +- **Never two runs on one card, within one working area**: a dispatch takes an exclusive per-card lock under `working_path` before spawning and releases it unconditionally (a crash writes `outcome=crashed` on the way out); a trigger burst is skipped and logged, never queued. The lock is filesystem-local, so a host that gives every job a fresh checkout needs its own concurrency group as the cross-job guard — see ADR-024's limitations. + --- All architectural implementations must follow these adopted standards. For process and rationale, see [way-of-working.md](../../way-of-working.md). diff --git a/.pair/adoption/tech/way-of-working.md b/.pair/adoption/tech/way-of-working.md index 3bbc3b8ed..d18b95c89 100644 --- a/.pair/adoption/tech/way-of-working.md +++ b/.pair/adoption/tech/way-of-working.md @@ -54,6 +54,17 @@ Resolution order, the split-tool routing and why the fallback is never the authe - When a bug fix or feature changes behavior covered by an existing CP, the corresponding test case MUST be updated. - **CP5's docs page list is machine-asserted against the filesystem** — `packages/knowledge-hub/src/conformance/docs-page-coverage.test.ts` compares it to `apps/website/content/docs/**/*.mdx`, so adding a docs page without listing it in CP5 fails CI ([ADL](../decision-log/2026-08-20-cp5-page-list-is-asserted-against-the-filesystem.md)). +## Review Convergence + +- **Baseline then delta:** the first review is complete and returns the immutable 40-character + head it inspected. A re-review verifies prior findings plus only the diff from that head and + directly changed producer/consumer boundaries; an unchanged PR surface does not create another + fix round. Missing or invalid review-head evidence fails closed, never converges a PR. +- **Provisioned artifact proof:** a fix that installs, builds, publishes, names, or invokes an + artifact maps `producer -> published identity -> consumer` and proves the real path in a clean + temporary environment. The exact boundary is never stubbed, aliased, or faked. See ADL + [2026-08-31-review-baseline-and-provisioned-artifact-contract.md](../decision-log/2026-08-31-review-baseline-and-provisioned-artifact-contract.md). + ## Quality Gates - `pnpm quality-gate` is the adopted project-level quality gate command. @@ -66,6 +77,8 @@ Resolution order, the split-tool routing and why the fallback is never the authe - **Coverage guardrail**: `enabled` — pair dogfoods its own capability: the [`Coverage guardrail` step](../../../.github/workflows/ci.yml) in CI sources [`coverage-gate.sh`](../../knowledge/assets/coverage-gate.sh), extracts the line-coverage % from each package's istanbul `coverage-summary.json`, and blocks a PR whose coverage drops below the human-committed baseline in [`tech/coverage-baseline.md`](./coverage-baseline.md) (maintaining/improving passes — not an absolute wall). The framework **default remains `disabled`** (the dataset template ships off); this line is pair's project-level opt-in only. See [coverage guardrail](../../knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md#coverage-guardrail-opt-in-regression-gate-consumed-by-this-pipeline) + [config format](../../knowledge/assets/coverage-config-example.md); `/pair-capability-setup-gates` reads this flag before generating the pipeline. **Coverage baseline commit-back**: `disabled` — the separate, nested opt-in ratchet (#372, framework default also `disabled`): when `enabled`, a **push to the base branch** (never a PR run, never a fork) proposes a raised `baseline.` as a **bot pull request** from `chore/coverage-baseline-ratchet`, never a push to `main`, and requires a repo-scoped `COVERAGE_RATCHET_TOKEN` (`contents: write` + `pull requests: write`, no protection bypass) — without it the step warns and the gate's verdict is unchanged. It stays `disabled` here until story #234's branch protection is applied and that secret is provisioned (ADR-018 lands with that story, so it is not linked from here yet); see ADL [2026-07-30-coverage-ratchet-pr-not-push.md](../decision-log/2026-07-30-coverage-ratchet-pr-not-push.md). The step that runs it is the **shipped** KB asset `node .pair/knowledge/assets/coverage-ratchet.cjs` (ADR-023) — the same one an adopter's generated pipeline invokes, so this flag being `enabled` means the same thing here as anywhere else. - **Pair review required checks**: `pair-review` + `pair-explicit-approval` are the required status checks that make the judgment review unskippable (R5.7) and enforce the 🔴 explicit-human-approval rule (D10) — see [pr-states.md](../../knowledge/guidelines/collaboration/project-management-tool/pr-states.md) and [ADR-018](adr/adr-018-pr-state-flow-required-checks.md). Status on this repo: **not yet applied** — writing branch protection needs admin scope, so it is a deliberate human step; until applied, enforcement here is advisory (the documented degraded mode). **Ordering constraint** (applies in this order, or every merge stops): 1. provision the `pr-state:*` labels + add the `pair-explicit-approval` workflow (neither needs admin scope — this repo has not added the workflow yet, so the context does not report today); 2. confirm on a real PR that `pair-review` and `pair-explicit-approval` both report on the head commit, **and** that the approval context re-reports on that same head SHA after a review submission; 3. only then `PUT` the branch protection, keeping `enforce_admins` off until one PR has merged through it. The whole sequence (including the merge-block outcomes per tier) was executed on a throwaway repository — see `github-implementation.md` § "Verified on a throwaway repository" — so what remains here is applying it, not discovering whether it works. **This repo is single-maintainer**, so a 🔴 PR cannot satisfy `pair-explicit-approval` (GitHub rejects a self-approval): a second human reviewer account is a prerequisite for making that context required here — otherwise leave it out of the required list and keep the 🔴 rule advisory. The solo-maintainer alternative (a verified human approval token instead of a second account) is tracked as [#398](https://github.com/foomakers/pair/issues/398). **When the protection is written here, use the `checks` form with `app_id` pinned** for `pair-explicit-approval` (an unpinned status context is satisfiable by any push-access token, including the agent's); `pair-review` stays unpinned and is an anti-accident control, not an authorization control — see `github-implementation.md` § "What each context proves". - **Gate & tooling code:** a gate's logic lives in a tested module in its owning package (white-box unit tests); scripts/CLIs are thin entrypoints and a root gate delegates (`pnpm --filter `). Scripts are never unit-tested — CLI-level checks go to smoke tests. See ADL [2026-07-13-gate-tooling-code-in-tested-modules.md](../decision-log/2026-07-13-gate-tooling-code-in-tested-modules.md). Gate/tooling packages are organized by bounded context, not one package per tool family — a new tool family sharing an existing package's bounded context is a new folder there, not a new package. See [ADR-014](adr/adr-014-tool-package-boundary-by-bounded-context.md). +- **OS guarantees are tested against the OS.** The in-memory `FileSystemService` double is the default, but when the behaviour under test IS an atomicity or containment guarantee of the operating system (an exclusive create, an append, symlink containment), the module calls `node:fs` directly, stays a **leaf** with the primitive injected at its call site, and is tested against a real temporary directory — a double that cannot fail the way production fails proves nothing. See ADL [2026-08-30-atomicity-primitives-use-node-fs-directly.md](../decision-log/2026-08-30-atomicity-primitives-use-node-fs-directly.md). +- **A CLI flag carrying an OBSERVATION is empty-valid; a flag carrying INTENT is not.** Every `pair run` flag refuses an empty value, with one documented exception: `--card-tags`, which reports the labels a host trigger observed on a card. "No labels" is a real state of every board (and what `join(labels.*.name, ',')` renders for an unlabelled issue), so an empty value there is data — read as an empty tag list and skipped cleanly — while an empty item INSIDE the list still HALTs. See ADL [2026-08-30-empty-card-tags-is-an-observation-not-a-malformed-flag.md](../decision-log/2026-08-30-empty-card-tags-is-an-observation-not-a-malformed-flag.md). - **Conformance tests** (`packages/knowledge-hub/src/conformance/`): one test file per target KB artifact (a `SKILL.md`, guideline, or template), not per introducing story — a new story extends the matching file's `describe` block instead of adding a new story-named file. See ADL [2026-07-18-conformance-test-per-file-not-per-story.md](../decision-log/2026-07-18-conformance-test-per-file-not-per-story.md). - **Monorepo tooling gotchas** (e.g. `pnpm --filter` bypassing turbo's `dependsOn` graph on a fresh checkout): documented once, centrally, in `DEVELOPMENT.md`'s `Turbo Caching` section — affected packages' READMEs carry only a short pointer, not a full copy. See ADL [2026-07-18-workspace-gotcha-doc-placement.md](../decision-log/2026-07-18-workspace-gotcha-doc-placement.md). diff --git a/.pair/knowledge/guidelines/collaboration/automation/README.md b/.pair/knowledge/guidelines/collaboration/automation/README.md index 0f4d6515e..338e9f780 100644 --- a/.pair/knowledge/guidelines/collaboration/automation/README.md +++ b/.pair/knowledge/guidelines/collaboration/automation/README.md @@ -28,9 +28,9 @@ This framework does not cover: ## Directory Contents -**[automation-policy.md](automation-policy.md)** - `tech/automation.md` schema: the `## Eligibility` declaration that selects which cards may run unattended +**[automation-policy.md](automation-policy.md)** - `tech/automation.md` schema: the `## Eligibility` declaration that selects which cards may run unattended, and the `## Workflows` mapping that routes a tagged card to the workflow that runs on it -**[github-automation.md](github-automation.md)** - GitHub Actions and workflow automation strategies +**[github-automation.md](github-automation.md)** - GitHub Actions and workflow automation strategies, including the reference tag-driven dispatch trigger adapter **[azure-devops-automation.md](azure-devops-automation.md)** - Azure DevOps board rules, branch policies, and service hooks diff --git a/.pair/knowledge/guidelines/collaboration/automation/automation-policy.md b/.pair/knowledge/guidelines/collaboration/automation/automation-policy.md index 4da2ad68c..883cb79c2 100644 --- a/.pair/knowledge/guidelines/collaboration/automation/automation-policy.md +++ b/.pair/knowledge/guidelines/collaboration/automation/automation-policy.md @@ -2,7 +2,7 @@ How much of the delivery flow a project lets run **unattended** is a project decision, so it lives in an adoption file: the optional `.pair/adoption/tech/automation.md`. This guideline defines that file's schema. -It specifies six sections, landed across two stories and owned one at a time so two stories never claim the same lines of the same file: `## Eligibility` (#216) selects **which cards** an unattended run may pick up at all; `## Harness`/`## Model Policy` (#450) declare supported agent harnesses and per-tier model class; and `## Auto-Advance`, `## Stop Predicate`, `## Max Parallelism`, `## Audit Location` (#250) are the remaining ADR-017 §6 knobs — which tier may auto-merge, when a run stops, the parallel-batch ceiling, and where the audit trail is written. +It specifies seven sections, landed across several stories and owned one at a time so two stories never claim the same lines of the same file: `## Eligibility` (#216) selects **which cards** an unattended run may pick up at all; `## Workflows` (#217) maps **a tag to the workflow that runs on a card carrying it**, which is what makes automation opt-in per card; `## Harness`/`## Model Policy` (#450) declare supported agent harnesses and per-tier model class; and `## Auto-Advance`, `## Stop Predicate`, `## Max Parallelism`, `## Audit Location` (#250) are the remaining ADR-017 §6 knobs — which tier may auto-merge, when a run stops, the parallel-batch ceiling, and where the audit trail is written. **Eligibility selects `which cards`, never which gates.** The per-tier gate/approval policy already exists in [`quality-model.md`](../../quality-assurance/quality-model.md) §4 and is not restated here — auto-advance *enacts* that policy, it does not redefine it. Two sources of truth for the same rule is the failure mode this split exists to prevent. @@ -100,6 +100,7 @@ An **untagged** card — one carrying no `risk:*` label at all — never matches | Is the card Ready / in the right state? | The consumer's own selection rules (`pair-next`). Eligibility is a **label** predicate only; state and readiness gating stay where they already live | | Auto-advance switch, stop predicate, step defaults, `max_parallelism`, audit location | ADR-017 §6 — the four sections below (`## Auto-Advance`, `## Stop Predicate`, `## Max Parallelism`, `## Audit Location`), landed by the automation loop story (#250) | | Which label a card carries | `classify`, via the Tag Projection declaration in `tech/risk-matrix.md` | +| Which workflow runs on an eligible card | `## Workflows` below — the tag→workflow mapping (#217). Eligibility selects, the mapping routes; neither answers the other's question | ## Auto-Advance — which tiers may push/merge unattended @@ -198,6 +199,106 @@ automation/loop-audit.md If the resolved path cannot be created or written, `pair-loop` **MUST HALT the run** rather than proceed unaudited: an unattended run with no audit trail is not an acceptable degraded mode (ADR-017 §6). +## Workflows — which workflow each tag routes to + +The **tag→workflow mapping** (#217, R4.4): the declaration that makes automation opt-in **per card** instead of per run. `## Eligibility` above answers *which cards an unattended run may pick up at all*; this section answers *what runs on a card once a trigger fires on it*, keyed by a tag the card carries. + +```markdown +## Workflows + +auto-dev ⇒ pair-loop +auto-plan ⇒ pair-process-plan-tasks +Precedence: auto-dev, auto-plan +``` + +- **One entry per line, ` ⇒ `** — the same `⇒` (U+21D2) `## Stop Predicate` uses, and only that one. An ASCII `=>` is a **HALT** naming the documented spelling, so the same file cannot mean different things to two consumers. +- **The tag is an OPAQUE routing key** (D18). It is matched against the card's labels with the plain **string equality** `## Eligibility` already uses — no tier arithmetic, no family knowledge, and **no classification criteria anywhere in the routing code**: tags are produced by `classify`, and a workflow only ever *reads* them. That property is grep-verifiable, and it is meant to be. +- **The workflow is a skill name** — the entry point of a composition of existing skills, never a bespoke engine and never a merit rule. Two conditions, both required: it must be **installed**, and it must be one the dispatcher can hand the dispatched card to — the set named in *"The workflows a mapping can name"* below. A skill that is installed but outside that catalog is **refused**, and the refusal stops dispatch for the whole board (see the routing-time HALTs), so this bullet is not the whole rule: read it together with the catalog section. +- **`Precedence: , , …`** — optional, at most one line, first listed wins. It resolves a card carrying **more than one** mapped tag, and nothing else. + +### Untagged ⇒ never. That is the whole opt-in boundary. + +A card carrying **no mapped tag never runs**. There is no default workflow, no "fall back to the develop workflow", no implicit route for an unmapped card — a consumer **MUST** skip it and log the skip. The absence of a route is the authorization decision, so widening it is not a convenience: it is the difference between automation on the cards a team named and automation on the backlog. + +### Absent section ⇒ no workflow is available + +`## Workflows` absent (or the whole optional file absent) ⇒ **no mapping is declared**: nothing can be routed. A dispatch **MUST** report `no mapping declared`, naming the file, and **exit cleanly** — automation is opt-in (D21), so a project that never wrote this section has simply not opted in, and that is never an error and never a default workflow. + +**Absent section ≠ empty section**, exactly as under `## Eligibility`: a heading with no entry is a **half-written declaration** ⇒ HALT. + +### Eligibility is applied BEFORE routing + +The order is normative. A card that does not match `## Eligibility` is **skipped before its tags are looked at at all**, and the skip is **logged**. Routing an ineligible card and relying on a later gate to stop it would put the eligibility filter — the one declaration that keeps business-critical work out of an unattended pipeline — after the decision it exists to bound. + +### Not a routable mapping ⇒ HALT + +At **read** time, a consumer **MUST HALT** with an adoption-fix message naming the file and the offending value when: + +1. the section is present with **no entry line** (a half-written declaration); +2. a line matches **neither** ` ⇒ ` **nor** `Precedence: , …`; +3. an entry uses `=>` instead of `⇒`; +4. the **same tag** is declared twice — one card would route to two workflows, and picking one silently is what this HALT prevents; +5. a **tag** is not usable as a label: longer than the host's label-name cap (**50 characters** on GitHub; another tracker applies its own), carrying a comma or a standalone `AND`/`OR`/`NOT`, opening with a markdown block marker, or containing a character that could turn it into a command fragment once inlined in an agent prompt. These are `## Eligibility`'s own triggers 3–5 plus the content MUST, applied to the same kind of value for the same reasons — one rule set, not a second one; +6. a **workflow name** is not a plain identifier (it is spliced into an agent invocation *and* used as a path segment when probing whether the skill is installed); +7. there is **more than one `Precedence:` line**, the line is empty, it repeats a tag, or it names a tag no entry declares — a precedence naming an undeclared tag is dead configuration that reads as a working tie-break; +8. the file carries **more than one `## Workflows` heading** — counted as rendered markdown at level 2, so an occurrence inside a fenced code block is not one. + +At **routing** time — the two rules that need a board and an installed skill set, so they cannot be answered from the file alone: + +- **a mapped tag whose workflow is not installed ⇒ HALT** with an adoption-fix message naming the tag, the workflow and the file. Never a silent fall back to another workflow: running a *different* workflow than the one declared is the outcome no operator can debug. The check runs **before** eligibility and routing, so this HALT stops dispatch for **every** card — including cards that are ineligible or carry no mapped tag at all — not only the cards carrying the offending tag. One broken line is broken configuration for the whole board, and that is the point: making the failure surface only on whichever card happens to carry that tag would make it depend on which trigger fired first; +- **a card carrying two or more mapped tags with no `Precedence:` line — or with none of those tags listed in it — ⇒ HALT**. A silent choice between two declared workflows is precisely what the precedence line exists to prevent, so its absence is a question for a maintainer, not a tie for a consumer to break. + +### One run per card — the concurrency guard + +A trigger fires on card metadata, and metadata changes in **bursts** (a label added, removed, re-added; a re-run of the same host job). A consumer **MUST** take an **exclusive per-card lock** before it dispatches and release it when the run ends; a second dispatch for a card whose lock is held is **skipped and logged**, never queued behind the first. Two agent runs on one card is the failure mode this guard exists for: they would race on the same branch, the same PR and the same board state. + +**The lock is scoped to ONE working area** (ADR-024): it stops two dispatches that share `working_path` — a persistent daemon, a long-lived runner — and it cannot see a holder on another machine or in another fresh checkout. A host whose jobs get an ephemeral workspace **MUST** put every path that dispatches a card into one host-side concurrency group, because there the group is the only cross-job guard there is. + +**A lock has no timeout and nothing reaps it.** A run killed by a signal, an OOM kill or a job timeout leaves the lock behind, and automation is then silently off for that card: every later trigger skips and exits cleanly. A consumer **MUST** therefore report, in the skip, *where* the lock is and *how long* it has been held — the two facts that separate a healthy burst from a stale lock — and the operator surface **MUST** document clearing it. + +### The audit trail — and where host credentials are not + +Every dispatch decision — **start**, **skip**, **end** — is appended to the run's `## Audit Location` file. The **start** record, and **only** the start record, is *also* emitted on stdout as a single `DISPATCH-RECORD:` line, so the **trigger's host adapter** — the thin, per-host piece that already holds the credentials the trigger runs under — can post it as a comment on the card. A skip and an end stay in the file: a card that gets a comment for every unmapped label edit is unreadable within a day, and an end comment doubles the noise for a fact the trail already holds. The dispatcher core stays **host-agnostic**: it reads tags it was handed, resolves a workflow and writes a file, and never holds a tracker token. Adding a host is a new adapter, never a change to the routing core. + +### The workflows a mapping can name + +A workflow is a **skill that already exists** — the entry point of a composition, so there is no bespoke engine to write and mapping a tag costs one line of adoption. **This table is the mappable set**, not an illustrative sample of it: a mapping may name one of these and nothing else, and a skill outside it is refused even when installed. They are the compositions pair ships, and they are the reason the mapping needs no vocabulary of its own. + +| Workflow | What a card routed to it gets | A tag teams usually map to it | How the dispatched card reaches it | +| --- | --- | --- | --- | +| `pair-loop` | the delivery loop — selects the card, implements it, opens the PR, drives the review/fix rounds, and stops at a review-approved PR (it never merges outside `## Auto-Advance`) | `auto-dev` | `--root ` | +| `pair-process-plan-tasks` | a refined story broken into implementation tasks, with the dependency graph and the AC-coverage table written back onto the card | `auto-plan` | `--story ` | + +**A mapping may only name a workflow in that table, and the last column is why.** The dispatched card is the whole subject of the run, and it arrives as an **argument** — under the name that workflow's own `## Arguments` table declares, borrowed and never invented (D18). The two above spell it differently, and the difference is not cosmetic: `pair-process-plan-tasks` states that **when its `$story` is absent it selects the highest-priority story on the board itself**. So a card handed to it under a name it does not declare is a card it never sees — the workflow runs, on a *different* card, while the audit trail and the on-issue `DISPATCH-RECORD:` both name the card that was tagged. A consumer therefore **MUST HALT** on a mapped workflow this table does not list, with an adoption-fix message, rather than dispatch a run it cannot scope — the same fail-fast, whole-board check an uninstalled workflow already gets, and for the same reason: a run nobody is watching must never pick its own subject. + +**Being scopable is not enough to be mappable.** A consumer may well know how some other skill spells its scope — `pair-next` takes `--root`, and driving it by hand with one is perfectly legitimate — and that is still not a licence to route a card to it. A dispatch takes the card's exclusive lock, writes an `event=start` audit line and emits the `DISPATCH-RECORD:` line the host adapter posts as a comment; a skill that only *reports* changes nothing, so all a team gets is a card claiming work that never happened and a lock nobody needed. Widening the set is a change to this table, made deliberately, with the "what a card routed to it gets" and "how the dispatched card reaches it" columns filled in — never a side effect of a consumer happening to know an argument name. + +#### A workflow that needs a human in the room is not mappable + +A dispatch runs with **nobody watching**, under an autonomy posture the operator opted into once, for as long as the per-iteration timeout allows. So the table above admits only workflows that can **reach a terminal outcome without an interlocutor**. A workflow whose steps require an explicit human decision — a `Human-judgment gate`, an alignment sync that ends only on an explicit "yes" — has exactly two outcomes when it is dispatched, and both are worse than not running: + +- it **stalls** on a question no one answers, until the per-iteration timeout kills it, holding the card's exclusive lock for the whole window while the card carries a public comment saying a run started; or +- the agent, having no one to ask, **supplies its own approval** and drives the card past the gate — which is the authorization control the gate exists to be, satisfied by the party it exists to constrain. + +`pair-process-refine-story` is the concrete exclusion and the reason this rule is written down: it is the single Draft→Ready path, and its own SKILL.md calls phase 0 "the R3.11 AI↔human alignment gate — a prerequisite, not optional", adds three per-step `Human-judgment gate`s, and closes with "what is never skipped is explicit human alignment before the story reaches `Ready`". It is a **hand-driven** workflow (`/pair-process-refine-story --story `, or the refine batch), not a tag-driven one. Refinement is where a human belongs; the mapping is for the work that follows it. + +```markdown +## Workflows + +auto-plan ⇒ pair-process-plan-tasks +auto-dev ⇒ pair-loop +Precedence: auto-plan, auto-dev +``` + +Two properties of that example are worth stating, because both are load-bearing rather than stylistic: + +- **The precedence line is what makes the pair safe.** A card that has just been broken into tasks often still carries `auto-plan` when `auto-dev` is added; without the line, that card is a HALT the moment a trigger fires on it. Declaring `auto-plan` first is not a preference — it is the answer to a question the dispatcher refuses to answer for you. +- **A workflow is never mapped to two tags to mean two intensities of it.** Tags carry no merit (D18), so `auto-dev-fast ⇒ pair-loop` and `auto-dev ⇒ pair-loop` route identically; what varies a run's behaviour is the policy above (`## Eligibility`, `## Stop Predicate`, `## Max Parallelism`), never the tag that routed it. + +### What fires the dispatch — the per-host adapter + +Nothing in this file starts a run. A **trigger** does: a thin, per-host piece that observes a card's labels changing and calls the entry point with what it already holds, `pair-cli run --card --card-tags `. It is the component that carries the tracker credentials, and the one that posts the `DISPATCH-RECORD:` line back onto the card. The reference implementation — a GitHub Actions job firing on `issues: [labeled]` — is in [github-automation.md](github-automation.md); a host with webhooks and a job runner (Azure DevOps service hooks, a Jira automation rule) is the same shape against a different API, and adding one never touches the routing core. + ## Harness and Model Policy A second, independent section of the same file — disjoint from `## Eligibility` above (which cards run unattended) and from `## Auto-Advance` / `## Stop Predicate` / `## Max Parallelism` / `## Audit Location` (the rest-of-file schema ADR-017 §6/#250 lands). This section answers two different questions: **which agent harnesses this project supports**, and **which model class each risk tier gets**. `/pair-capability-setup-harness` reads exactly these two declarations; the [agent-harness framework](../../technical-standards/ai-development/agent-harness/README.md) documents what each harness value means. diff --git a/.pair/knowledge/guidelines/collaboration/automation/github-automation.md b/.pair/knowledge/guidelines/collaboration/automation/github-automation.md index e377f500a..27d43d27b 100644 --- a/.pair/knowledge/guidelines/collaboration/automation/github-automation.md +++ b/.pair/knowledge/guidelines/collaboration/automation/github-automation.md @@ -195,6 +195,120 @@ jobs: - Integration with external tools and notification systems - Advanced reporting and analytics automation +## Tag-Driven Dispatch — the reference trigger adapter + +The **trigger** for tag-driven workflows: the thin, host-specific piece that turns "a label was added to an issue" into one call to pair's entry point. Everything it decides is decided here; everything it *routes* is decided by `## Workflows` in `tech/automation.md` (schema: [automation-policy.md](automation-policy.md)). The adapter is deliberately small — five steps, three of them setup, and no logic of its own — because that is what keeps every host on the same routing core. + +**The runner does not ship `pair-cli`.** `ubuntu-latest` has never heard of it, so the job installs the CLI itself: without that step `pair-cli run` is `command not found`, the step exits 127, the job goes red and nothing is ever routed or audited. The **engine** the run spawns (`claude`, `pi`, `opencode`) and its credentials are the adopter's own step — the block below installs the driver, not the agent. + +### The workflow + +```yaml +name: pair dispatch +on: + issues: + types: [labeled] + +# One in-flight job per issue. On EPHEMERAL runners this group IS the cross-job guard, and +# `cancel-in-progress: false` is what makes it one: every job checks out a fresh workspace, +# so the per-card lock `pair-cli run` takes lives in a working area no other job can see, and it +# can never observe a holder from another runner (ADR-024: the lock is filesystem-local). +# A second trigger declared OUTSIDE this group — a `workflow_dispatch` button, an +# `issue_comment` job — therefore starts a second agent on the same card, the same branch +# and the same PR. Put every path that dispatches a card into THIS group. +# The per-card lock is the guard within ONE working area: a persistent daemon box, where the +# bursts it stops are real and the host has no concurrency group at all. +concurrency: + group: pair-dispatch-${{ github.event.issue.number }} + cancel-in-progress: false + +jobs: + dispatch: + runs-on: ubuntu-latest + # The credentials live HERE, at the adapter — the narrowest set that lets it read the + # issue it was handed and post one comment on it. `pair-cli run` itself is given none. + permissions: + issues: write + contents: read + steps: + - uses: actions/checkout@v4 + + # `pair-cli` is NOT on a hosted runner. Without these two steps the next one is + # `bash: pair-cli: command not found` (exit 127) on every labeled event — a red job, + # nothing routed, nothing audited. Pin the version you adopted rather than + # `@latest` if you want the trigger to be reproducible. + - uses: actions/setup-node@v4 + with: + node-version: 22 + + - name: Install the pair CLI + shell: bash + run: npm install -g @foomakers/pair-cli + + - name: Dispatch the card + id: dispatch + # DECLARED, not defaulted. GitHub's implicit shell is `bash -e {0}` — WITHOUT + # `pipefail` — so the pipeline below would report `tee`'s status and a HALT inside + # `pair-cli run` (an uninstalled workflow, an undecidable multi-tag card) would land as a + # green tick. `shell: bash` is what makes GitHub run the step with `-eo pipefail`. + shell: bash + env: + # The labels the trigger ALREADY observed — passed as data, never re-fetched. An + # adapter that queries the API for them is the tracker client the driver exists + # without. Through the environment, not string-interpolated into the command line. + CARD_TAGS: ${{ join(github.event.issue.labels.*.name, ',') }} + CARD: ${{ github.event.issue.number }} + run: | + pair-cli run --card "$CARD" --card-tags "$CARD_TAGS" --autonomous \ + | tee dispatch.log + + - name: Record the run on the card + if: always() + shell: bash + env: + GH_TOKEN: ${{ github.token }} + CARD: ${{ github.event.issue.number }} + run: | + # The driver PRINTS this line and never posts it: posting is the adapter's job, + # because the adapter is what holds a tracker token. No line ⇒ nothing was + # dispatched (untagged, unmapped or ineligible card) ⇒ nothing to comment. + record="$(grep '^DISPATCH-RECORD:' dispatch.log || true)" + # `if`, not `[ -n "$record" ] && …`: under `-e` a trailing compound whose test is + # false exits 1, so the silent case — the untagged issue this feature promises + # costs nothing — would fail the step and notify a human on every unmapped label + # edit on the board. + if [ -n "$record" ]; then + gh issue comment "$CARD" --body "$record" + fi +``` + +### What the adapter does and does not decide + +| Question | Answered by | +| --- | --- | +| Did anything happen on a card? | the host trigger (`types: [labeled]`) | +| Which workflow runs on it? | `## Workflows` in `tech/automation.md` — never the adapter, never a job condition | +| May this card run at all? | `## Eligibility`, applied by the dispatcher *before* routing | +| Is another run already on this card? | the per-card lock inside `pair-cli run` | +| Who tells the humans? | the adapter, by posting the `DISPATCH-RECORD:` line | + +**Only a run START is ever posted on the card.** `DISPATCH-RECORD:` is emitted for the `start` event and for nothing else — a skip and an end are appended to the audit file only. That is deliberate and it is the whole reason the comment step needs no filter of its own: a board where every unmapped label edit posted a "nothing happened" comment would be unreadable within a day, and an `end` comment would double every run's noise for a fact the audit trail already holds. An adapter that wants the end on the card reads it from `## Audit Location`; it must not re-derive it from the exit status. + +A job `if:` that pre-filters on a label is the one thing worth resisting: it duplicates the mapping in a second place, in a language the dispatcher cannot read, and the two drift on the day someone renames a tag in adoption. Let every labeled event through and let the routing core skip what it should skip — a skip is cheap, reported, and appended to the audit trail. + +### Untagged is not a case the adapter has to handle + +An issue carrying no mapped tag **runs nothing**: the dispatcher reports the skip and exits `0`, and with no `DISPATCH-RECORD:` line the comment step posts nothing. That includes the **unlabelled** issue, where `join(github.event.issue.labels.*.name, ',')` renders an empty string: `--card-tags` reads an empty value as the observation "this card carries no labels", so the adapter needs no guard clause and no conditional call — passing what the trigger saw is always correct. That is the opt-in boundary of the whole feature, and it lives in the routing core precisely so that no adapter can widen it by accident. The same holds when `tech/automation.md` declares no `## Workflows` section at all — the run reports `no mapping declared` and exits cleanly. + +### Before wiring the trigger + +- **Run it once by hand**, on a card you tagged deliberately: `pair-cli run --card --card-tags "" --dry-run` prints the route, the perimeter and the policy, and spawns nothing. +- **Provision `pair-cli` and the engine on the runner.** The job above installs the CLI; the **engine** it spawns (`claude`, `pi`, `opencode`) and that engine's credentials are yours to add. Neither is present on a hosted runner by default, and a missing binary is `command not found` — a red job, with nothing routed and nothing written to the audit file. +- **Scope the token to the repository the cards live in.** The adapter can only ever post where its token reaches; the engine credentials the run itself needs are a separate, and usually much broader, concern. +- **Watch the audit file** (`## Audit Location`) for the first few cycles: every start, skip and end is there, including the ones the card never shows. +- **Check the mapping resolves before the first trigger fires.** A `## Workflows` entry naming a workflow nobody installed — or one the dispatcher cannot hand the card to, i.e. anything outside the [catalog](automation-policy.md#the-workflows-a-mapping-can-name) — HALTs the dispatch *before* eligibility and routing, so it stops **every** card on the board, not only cards carrying that tag. That is the intended blast radius — a broken mapping is broken for everyone, and finding out only on the one card that happens to carry the tag would make the failure depend on which trigger fired first — but it means the dry run above is a check on the whole board, not on one card. +- **Know how to clear a stale lock.** The per-card lock is a directory (`/automation/locks//`) with no timeout and nothing to reap it: a run killed by SIGKILL, an OOM kill or a job timeout leaves it behind, and every later trigger on that card then skips with `run-in-progress` and exits `0` — automation silently off for that one card. The skip line prints the directory and how long it has been held; when no run is alive, `rm -rf` that directory to clear it. On ephemeral runners the workspace is discarded with the job, so this is a **persistent daemon** concern. + ## Implementation Guidelines ### Setup Process @@ -286,12 +400,4 @@ jobs: - Approval workflows and sign-off procedures - Compliance verification and audit trail management -This GitHub automation framework provides comprehensive automation capabilities that integrate seamlessly with development workflows while maintaining visibility, control, and reliability for team collaboration and project management.Automation - -## Overview - -This document outlines automation strategies for GitHub-based collaboration workflows. - -## TODO - -This document needs to be completed with GitHub automation guidelines. +This GitHub automation framework provides comprehensive automation capabilities that integrate seamlessly with development workflows while maintaining visibility, control, and reliability for team collaboration and project management. diff --git a/.pair/llms.txt b/.pair/llms.txt index 9ca1efe76..b6de24dbe 100644 --- a/.pair/llms.txt +++ b/.pair/llms.txt @@ -41,6 +41,7 @@ - [ADR-021: Fan-out is one capability with three realizations — in-harness, external driver, degraded](.pair/adoption/tech/adr/adr-021-fan-out-three-realizations.md) - [ADR-022: The coverage-baseline ratchet is EXPOSED through the published CLI, not ported to a shipped shell asset](.pair/adoption/tech/adr/adr-022-coverage-ratchet-exposed-through-the-cli.md) - [ADR-023: The coverage-baseline ratchet ships as a GENERATED KB asset, not as a CLI command](.pair/adoption/tech/adr/adr-023-coverage-ratchet-ships-as-a-generated-kb-asset.md) +- [ADR-024: Tag-driven dispatch — the mapping is adoption, the routing core is host-agnostic, the on-issue record belongs to the host adapter](.pair/adoption/tech/adr/adr-024-tag-driven-dispatch-agnostic-core-host-adapter.md) - [Architecture](.pair/adoption/tech/architecture.md) - [Automation Policy — this project's delta](.pair/adoption/tech/automation.md) - [Development Collaboration Context](.pair/adoption/tech/boundedcontext/development-collaboration.md) @@ -125,6 +126,9 @@ - [Decision: the CLI invocation name is `pair-cli`, not `pair`](.pair/adoption/decision-log/2026-08-25-cli-invocation-canonical-name-is-pair-cli.md) - [Decision: Post-merge cleanup covers local branches and worktrees; PR analyses retire at merge](.pair/adoption/decision-log/2026-08-25-post-merge-cleanup-covers-local-branches-pr-analyses-retire-at-merge.md) - [Decision: tier 1's `$approval` posture is unconditional, and tier 1 has no declaring composition site yet](.pair/adoption/decision-log/2026-08-28-tier1-approval-posture-is-unconditional-and-has-no-declaring-composition-site-yet.md) +- [Decision: the two atomicity primitives (exclusive create, append) use `node:fs` directly, in leaf modules tested against a real temporary directory](.pair/adoption/decision-log/2026-08-30-atomicity-primitives-use-node-fs-directly.md) +- [Decision: an empty `--card-tags` means "this card carries no labels", not a malformed flag](.pair/adoption/decision-log/2026-08-30-empty-card-tags-is-an-observation-not-a-malformed-flag.md) +- [Decision: Review re-checks use an immutable baseline and prove provisioned artifacts](.pair/adoption/decision-log/2026-08-31-review-baseline-and-provisioned-artifact-contract.md) ## How-To Guides diff --git a/apps/pair-cli/src/cli.e2e.test.ts b/apps/pair-cli/src/cli.e2e.test.ts index 6b959a128..359d3d81b 100644 --- a/apps/pair-cli/src/cli.e2e.test.ts +++ b/apps/pair-cli/src/cli.e2e.test.ts @@ -1,5 +1,9 @@ -import { describe, it, expect, vi } from 'vitest' +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' import { InMemoryFileSystemService } from '@pair/content-ops/test-utils/in-memory-fs' +import { fileSystemService } from '@pair/content-ops' import { installCommand, handleInstallCommand, @@ -8,7 +12,9 @@ import { handlePackageCommand, handleScaffoldKbCommand, handleKbInfoCommand, + commandRegistry, } from './commands' +import type { IterationResult } from './commands/run/stream-reader' /** * pair-cli e2e suite. @@ -259,4 +265,264 @@ describe('pair-cli e2e', () => { expect(clean.report.migrationUrl).toBeUndefined() }) }) + + /** + * US-217 T-5 — tag-driven dispatch against a POPULATED BOARD, end to end. + * + * Genuinely e2e by this suite's own bar (see the header): the run is driven through the command + * registry the CLI dispatches on, against a REAL project directory, and each dispatch hands state + * to the next one through artifacts on disk — the per-card lock and the appended audit file. The + * module suites prove each decision in isolation with the lock and the audit writer injected; + * what only this level can show is that five triggers fired at one project leave exactly the runs, + * the trail and the locks they should, with nothing shared between them but the filesystem. + * + * The board is the fixture: one card per case the story names — routed, untagged, eligible but + * unmapped, multi-tagged, mapped but ineligible. The ONLY injected dependency is the engine spawn, + * because a test that starts a real agent is not a test. + */ + describe('tag-driven dispatch on a populated board (US-217)', () => { + const POLICY = `## Eligibility + +risk:green + +## Workflows + +auto-plan ⇒ pair-process-plan-tasks +auto-dev ⇒ pair-loop +Precedence: auto-plan, auto-dev +` + + /** + * The cards a trigger fires on, with the labels it observed at that moment — and everything + * each one must produce. + * + * The fixture IS the assertion set: `routes` is the prompt the engine must be given (absent ⇒ + * nothing may spawn, nothing may be recorded on the card) and `trail` is what the audit file + * must say about that card. Every check below iterates these rows, so a row added here is a + * row checked, and a row whose workflow, scoping argument or skip reason changes fails on its + * own row instead of shifting a positional index under an assertion about a different card. + */ + const BOARD = [ + // pair-loop declares `--iteration` too, and plan-tasks does not: each invocation carries + // exactly the arguments its own `## Arguments` table declares, and nothing else. + { + card: '301', + tags: ['auto-dev', 'risk:green'], + routes: '/pair-loop --root 301 --iteration 1', + trail: [ + /event=start card=301 tag=auto-dev workflow=pair-loop/, + /event=end card=301 .*outcome=completed/, + ], + }, + // 302 is the UNLABELLED card — the state a host adapter renders as an empty `--card-tags`. + // It stops at the ELIGIBILITY gate, before its (absent) tags are ever routed: an untagged + // card matches no eligibility label either, so the earliest guard is the one that catches it. + { + card: '302', + tags: [], + routes: undefined, + trail: [/event=skip card=302 reason=ineligible/], + }, + // 303 IS eligible and still runs nothing: eligibility selects, the mapping routes, and there + // is no default workflow for a card the mapping does not name. + { + card: '303', + tags: ['risk:green'], + routes: undefined, + trail: [/event=skip card=303 reason=unmapped/], + }, + { + card: '304', + tags: ['auto-plan', 'auto-dev', 'risk:green'], + // The DECLARED precedence wins over the first mapped tag the card carries — and the card + // reaches plan-tasks as `--story`, the argument that skill declares: `--root 304` is a + // scope it never sees, and its Step 0 would then pick the top story on the board. + routes: '/pair-process-plan-tasks --story 304', + trail: [/event=start card=304 tag=auto-plan/], + }, + { + card: '305', + tags: ['auto-dev'], + routes: undefined, + trail: [/event=skip card=305 reason=ineligible/], + }, + ] as const + + type BoardRow = (typeof BOARD)[number] + /** The cards the board expects to run, in trigger order — the fixture, read as data. */ + const routed = BOARD.filter( + (row): row is BoardRow & { routes: string } => row.routes !== undefined, + ) + const unrouted = BOARD.filter(row => row.routes === undefined) + + const AUDIT = '.pair/working/automation/loop-audit.md' + const LOCKS = '.pair/working/automation/locks' + + let project: string + let spawned: string[] + let printed: string[] + let log: ReturnType + + const write = (relative: string, content: string): void => { + const target = join(project, relative) + mkdirSync(join(target, '..'), { recursive: true }) + writeFileSync(target, content) + } + + beforeEach(() => { + project = mkdtempSync(join(tmpdir(), 'pair-dispatch-e2e-')) + spawned = [] + printed = [] + + write( + 'config.json', + JSON.stringify({ + asset_registries: { + skills: { + source: '.skills', + behavior: 'overwrite', + description: 'skills', + prefix: 'pair', + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + }, + }), + ) + // The installed skill set the mapping is resolved against — both declared workflows. + write('.claude/skills/pair-loop/SKILL.md', '') + write('.claude/skills/pair-process-plan-tasks/SKILL.md', '') + write('.pair/adoption/tech/automation.md', POLICY) + // A `claude` on PATH: engine resolution probes the filesystem, and the default cascade + // resolves the schema default when nothing declares one. + write('bin/claude', '') + vi.stubEnv('PATH', join(project, 'bin')) + + log = vi.spyOn(console, 'log').mockImplementation(line => { + printed.push(String(line)) + }) + }) + + afterEach(() => { + log.mockRestore() + vi.unstubAllEnvs() + rmSync(project, { recursive: true, force: true }) + }) + + /** One trigger event, through the registry the CLI dispatches on. */ + const trigger = async ( + card: string, + tags: readonly string[], + runIteration?: () => Promise, + ): Promise => + commandRegistry.run.handle( + commandRegistry.run.parse({ + card, + cardTags: tags.join(','), + cwd: project, + maxIterations: 1, + }), + fileSystemService, + { + runIteration: async input => { + spawned.push(input.promptText) + return runIteration ? await runIteration() : { outcome: 'success', detail: 'done' } + }, + }, + ) + + const auditTrail = (): string => readFileSync(join(project, AUDIT), 'utf-8') + + it('runs exactly the two cards the mapping routes, and leaves the trail to prove the other three', async () => { + for (const { card, tags } of BOARD) expect(await trigger(card, tags)).toBe(0) + + // AC1 — routed cards ran the MAPPED workflow, scoped to their own card, and NOTHING else ran. + // Driven off the fixture: every row that declares a route is checked against the prompt the + // engine was actually given, in the order the triggers fired. + expect(spawned).toEqual(routed.map(row => row.routes)) + + // AC2 — every card left the trail its own row declares, and the ones that ran nothing say + // WHY. Read off the fixture, so a row added above is a row this checks. + const trail = auditTrail() + for (const row of BOARD) for (const line of row.trail) expect(trail).toMatch(line) + // No card was ever routed to a workflow its tags do not name, and none of them started. + for (const { card } of unrouted) { + expect(trail).not.toMatch(new RegExp(`card=${card} (tag|workflow)=`)) + expect(trail).not.toMatch(new RegExp(`event=start card=${card}`)) + } + + // AC3 — the line the host adapter posts on the card exists for the runs that started, and + // ONLY for those: a card that never ran must not get a comment claiming it did. + const records = printed.filter(line => line.startsWith('DISPATCH-RECORD:')) + expect(records).toEqual(routed.map(row => expect.stringContaining(`card=${row.card}`))) + + // Every lock was released: the board is left dispatchable, not parked. + for (const { card } of BOARD) expect(existsSync(join(project, LOCKS, card))).toBe(false) + }) + + /** + * The dispatched card is the ONLY subject a routed run can have — `--root` cannot displace it. + * + * Before the refusal, `--card 301 --root 300` parsed and drove `/pair-loop --root 300` while the + * audit file recorded `card=301` start AND end, the `DISPATCH-RECORD:` line named 301, and the + * exclusive lock was taken on 301 — so the agent worked an unguarded subtree (a second trigger + * on 300 would have acquired its own free lock and started a second agent on the same branch) + * and the trail credited a card nothing ran on. Checked through the registry, because the + * refusal has to hold at the entry point a trigger actually calls. + */ + it('refuses --root on a dispatched card, and spawns nothing when it does', async () => { + expect(() => + commandRegistry.run.parse({ + card: '301', + cardTags: 'auto-dev,risk:green', + root: '300', + cwd: project, + maxIterations: 1, + }), + ).toThrow(/--card cannot be combined with --root/) + + expect(spawned).toHaveLength(0) + expect(existsSync(join(project, AUDIT))).toBe(false) + expect(existsSync(join(project, LOCKS, '301'))).toBe(false) + }) + + it('never starts a second run on a card a run already holds (trigger burst)', async () => { + // The burst, exactly as a host produces it: the second trigger arrives WHILE the first run is + // in flight. Re-entering from inside the iteration is what makes the lock the thing under + // test rather than a sequence of two finished runs. + let reentrant: number | undefined + await trigger('301', ['auto-dev', 'risk:green'], async () => { + reentrant = await trigger('301', ['auto-dev', 'risk:green']) + return { outcome: 'success', detail: 'done' } + }) + + expect(reentrant).toBe(0) + // One spawn, not two: the second dispatch was skipped, never queued behind the first. + expect(spawned).toHaveLength(1) + expect(auditTrail()).toMatch(/event=skip card=301 reason=run-in-progress/) + // ...and the burst did not leave the card locked for the next trigger. + expect(existsSync(join(project, LOCKS, '301'))).toBe(false) + // The skip names the REAL holder — the directory the run probed, and how long it has held it. + // Nothing reaps a lock, so a killed run leaves one behind and every later trigger on the card + // skips forever; the age is what tells an operator this skip is not a healthy burst. + const skip = printed.find(line => line.includes('run-in-progress')) + expect(skip).toContain(join(project, LOCKS, '301')) + expect(skip).toContain('held under a minute') + expect(skip).toMatch(/stale/) + }) + + it('routes nothing at all when the project declares no mapping — the shipped default', async () => { + write('.pair/adoption/tech/automation.md', '## Eligibility\n\nrisk:green\n') + + for (const { card, tags } of BOARD) expect(await trigger(card, tags)).toBe(0) + + expect(spawned).toHaveLength(0) + expect(printed.some(line => line.includes('no mapping declared'))).toBe(true) + // EVERY card on the board, not just the one that would otherwise have routed: with no + // `## Workflows` section nothing is routable, and each card says so in the trail. + const trail = auditTrail() + for (const { card } of BOARD) { + expect(trail).toMatch(new RegExp(`event=skip card=${card} reason=no-mapping-declared`)) + } + }) + }) }) diff --git a/apps/pair-cli/src/commands/run/automation-policy.ts b/apps/pair-cli/src/commands/run/automation-policy.ts index fca21235f..8da2b9da6 100644 --- a/apps/pair-cli/src/commands/run/automation-policy.ts +++ b/apps/pair-cli/src/commands/run/automation-policy.ts @@ -1,6 +1,8 @@ import { join } from 'path' import type { FileSystemService } from '@pair/content-ops' import { isLabelShape, isSafePromptText, promptSafetyFailure } from './prompt-safety' +import { assertLabelValue, policyHalt, POLICY_PATH, sectionLines } from './policy-sections' +import { readWorkflowMapping, type WorkflowMapping } from './workflow-mapping' /** * The automation-policy reader (US-451 T-8) — READ-ONLY, and it BORROWS every parameter. @@ -18,7 +20,7 @@ import { isLabelShape, isSafePromptText, promptSafetyFailure } from './prompt-sa * not been read. */ -export const POLICY_PATH = '.pair/adoption/tech/automation.md' +export { POLICY_PATH } from './policy-sections' export const DEFAULT_AUDIT_LOCATION = 'automation/loop-audit.md' /** Absent stop-predicate section ⇒ exactly one iteration, never an unbounded run. */ export const FAIL_SAFE_MAX_ITERATIONS = 1 @@ -44,15 +46,18 @@ export interface AutomationPolicy { readonly maxIterations: number readonly maxParallelism: number readonly auditLocation: string + /** + * `## Workflows`'s tag→workflow mapping (US-217), absent when the project declares none. + * + * Absent is the SHIPPED state and never an error: with no mapping there is no workflow to route a + * card to, so a tag-driven dispatch reports "no mapping declared" and exits cleanly. Automation is + * opt-in per card, and this is the declaration that opts in. + */ + readonly workflows?: WorkflowMapping readonly source: typeof POLICY_PATH | 'fail-safe defaults (policy file absent)' readonly warnings: readonly string[] } -/** A HALT on the policy read: the message names the file and the offending value. */ -function halt(detail: string): never { - throw new Error(`${POLICY_PATH} — ${detail}. Fix the adoption file, then re-run.`) -} - /** * The one message every unsafe value gets, wherever it was declared — the shared rule set lives in * `prompt-safety.ts` so the CLI flags and the policy fields cannot drift apart (round 6, Major). @@ -68,7 +73,7 @@ function halt(detail: string): never { */ function assertSafePromptText(section: string, value: string): void { if (isSafePromptText(value)) return - halt(promptSafetyFailure(`\`## ${section}\``, value)) + policyHalt(promptSafetyFailure(`\`## ${section}\``, value)) } export function readAutomationPolicy(fs: FileSystemService, projectRoot: string): AutomationPolicy { @@ -91,6 +96,7 @@ export function readAutomationPolicy(fs: FileSystemService, projectRoot: string) const warnings: string[] = [] const eligibility = readEligibility(markdown, warnings) const stop = readStopPredicate(markdown) + const workflows = readWorkflowMapping(markdown) return { ...(eligibility !== undefined && { eligibility }), @@ -99,6 +105,7 @@ export function readAutomationPolicy(fs: FileSystemService, projectRoot: string) maxIterations: stop.maxIterations, maxParallelism: readMaxParallelism(markdown), auditLocation: readAuditLocation(markdown), + ...(workflows !== undefined && { workflows }), source: POLICY_PATH, warnings, } @@ -121,53 +128,8 @@ export function describeParallelism(policy: AutomationPolicy): string { ) } -/* ------------------------------------------------------------------ sections */ - -/** - * The body of a level-2 section, as RENDERED markdown: an occurrence inside a fenced code block - * is not a heading (the schema documents its own declarations inside fences, so a line scan that - * ignored fences would read a documentation example as a declaration). - */ -function sectionBodies(markdown: string, heading: string): string[][] { - const bodies: string[][] = [] - let current: string[] | undefined - let fenced = false - - for (const raw of markdown.split(/\r?\n/)) { - const line = raw.trim() - if (line.startsWith('```')) { - fenced = !fenced - if (current) current.push(raw) - continue - } - if (!fenced && /^##\s+/.test(line)) { - if (current) bodies.push(current) - current = line.replace(/^##\s+/, '') === heading ? [] : undefined - continue - } - if (current) current.push(raw) - } - if (current) bodies.push(current) - return bodies -} - -/** The section's non-empty lines, trimmed — the unit every schema rule is stated over. */ -function sectionLines(markdown: string, heading: string): string[] | undefined { - const bodies = sectionBodies(markdown, heading) - if (bodies.length === 0) return undefined - if (bodies.length > 1) { - halt(`carries ${bodies.length} \`## ${heading}\` headings, but exactly one declaration is read`) - } - return bodies[0]!.map(line => line.trim()).filter(line => line.length > 0) -} - /* -------------------------------------------------------------- eligibility */ -// The schema's list, plus a leading SINGLE backtick: an inline-code paste is the same copied-wrapper -// mistake as a fence, and tier 1 already rejected it (round 7, minor 1). -const MARKDOWN_BLOCK_MARKERS = ['`', '-', '*', '+', '>', '#'] -const GITHUB_LABEL_CAP = 50 - /** * `## Eligibility` — exactly one label, validated by the guideline's seven HALT triggers and * then passed to the skill VERBATIM. Validating is not transforming. @@ -180,33 +142,18 @@ function readEligibility(markdown: string, warnings: string[]): string | undefin ) return undefined } - if (lines.length === 0) halt('`## Eligibility` is present but empty (a half-written declaration)') + if (lines.length === 0) + policyHalt('`## Eligibility` is present but empty (a half-written declaration)') if (lines.length > 1) { - halt(`\`## Eligibility\` carries ${lines.length} non-empty lines, but takes exactly one label`) + policyHalt( + `\`## Eligibility\` carries ${lines.length} non-empty lines, but takes exactly one label`, + ) } const value = lines[0]! - // A STANDALONE token, as the schema says and tier 1 matches — `\b` made `area:OR-tools` a HALT, - // rejecting a legitimate label (round 7, minor 1). - if (value.includes(',') || /(^|\s)(AND|OR|NOT)(\s|$)/.test(value)) { - halt(`\`## Eligibility\` declares \`${value}\`, but the declaration takes exactly one label`) - } - if (MARKDOWN_BLOCK_MARKERS.some(marker => value.startsWith(marker))) { - halt( - `\`## Eligibility\` declares \`${value}\`, which is a copied markdown wrapper, not a bare label`, - ) - } - if (value.length > GITHUB_LABEL_CAP) { - halt( - `\`## Eligibility\` declares a ${value.length}-character value, longer than the host's label cap (${GITHUB_LABEL_CAP})`, - ) - } - if (value.split(/\s+/).filter(token => token.includes(':')).length > 1) { - halt(`\`## Eligibility\` declares \`${value}\`, which juxtaposes several labels on one line`) - } - // The guideline's SEPARATE content MUST, layered on top of the seven triggers rather than - // widening them: this value reaches an agent prompt, so it may never be a command fragment. - assertSafePromptText('Eligibility', value) + // The shape triggers plus the content MUST, in `policy-sections.ts` — the SAME rules each + // `## Workflows` routing key gets, because the schema states them once for every label slot. + assertLabelValue('`## Eligibility`', value) return value } @@ -216,7 +163,7 @@ function readEligibility(markdown: string, warnings: string[]): string | undefin function assertTierShapes(tiers: readonly string[]): void { for (const tier of tiers) { if (!isLabelShape(tier)) { - halt( + policyHalt( `\`## Auto-Advance\` names \`${tier}\`, which is not a well-formed \`family:tier\` label`, ) } @@ -238,19 +185,21 @@ function readAutoAdvance(markdown: string, eligibility: string | undefined): str const value = lines[0]! if (lines.length > 1) { - halt( + policyHalt( `\`## Auto-Advance\` carries ${lines.length} non-empty lines, but takes exactly one switch`, ) } if (value === AUTO_ADVANCE_OFF) return value if (/\b(AND|OR|NOT)\b/.test(value)) { - halt(`\`## Auto-Advance\` declares \`${value}\`, but the switch is a tier, not an expression`) + policyHalt( + `\`## Auto-Advance\` declares \`${value}\`, but the switch is a tier, not an expression`, + ) } const tiers = value.split(',').map(tier => tier.trim()) assertTierShapes(tiers) const foreign = tiers.filter(tier => tier !== eligibility) if (foreign.length > 0 || new Set(tiers).size !== tiers.length) { - halt( + policyHalt( `\`## Auto-Advance\` declares \`${value}\`, which is not this project's \`## Eligibility\` ` + `tier (${eligibility ?? 'none declared'}) — a tier outside eligibility is never selected, ` + `so it could never advance`, @@ -321,13 +270,13 @@ function readStopPredicate(markdown: string): { predicate?: string; maxIteration // Named separately from "matches neither grammar": an ASCII arrow is a spelling mistake with // an obvious fix, and reporting it as an unrecognised line sends the maintainer hunting. if (ASCII_ARROW.test(line)) { - halt( + policyHalt( `\`## Stop Predicate\` line \`${line}\` uses \`=>\`, but the documented arrow is \`⇒\` ` + `(U+21D2) — the same form the fan-out workflow requires, so the two realizations of the ` + `loop read this file identically`, ) } - halt( + policyHalt( `\`## Stop Predicate\` line \`${line}\` matches neither \` ⇒ \` nor \`max-iterations: \``, ) } @@ -360,7 +309,7 @@ function assertSelector(selector: string, line: string): void { if (selector === 'root') return const payload = /^(?:tag|type):(.*)$/.exec(selector)?.[1] ?? '' if (payload.length === 0) { - halt( + policyHalt( `\`## Stop Predicate\` line \`${line}\` has an empty selector payload — \`tag:\`/\`type:\` needs a label`, ) } @@ -377,7 +326,7 @@ function assertCondition(condition: string, line: string): void { const parts = condition.split(/\s+and\s+/i).map(part => part.trim()) const valid = parts.every(part => CONDITIONS.includes(part) || /^has-tag:\S+$/.test(part)) if (!valid) { - halt( + policyHalt( `\`## Stop Predicate\` line \`${line}\` names \`${condition}\`, which is not a canonical macrostate (${CONDITIONS.join(', ')}) or \`has-tag: