From 89793d278ee5b0a9ed1c1d6548f191e6372ce1f2 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Sun, 30 Aug 2026 18:31:03 +0200 Subject: [PATCH 01/13] [US-219] fix: harden review convergence - Sweep bounded contract surfaces before re-review\n- Preserve every accepted finding\n\nRefs: #220, #441 --- .claude/workflows/pair-implement-batch.js | 7 +++- .../workflows/pair-implement-batch.test.mjs | 41 +++++++++++++++++++ .../.workflows/pair-implement-batch.js | 7 +++- .../.workflows/pair-implement-batch.test.mjs | 41 +++++++++++++++++++ 4 files changed, 92 insertions(+), 4 deletions(-) diff --git a/.claude/workflows/pair-implement-batch.js b/.claude/workflows/pair-implement-batch.js index f95817702..c830a31e2 100644 --- a/.claude/workflows/pair-implement-batch.js +++ b/.claude/workflows/pair-implement-batch.js @@ -1230,7 +1230,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) @@ -1361,7 +1364,7 @@ async function driveStory(story) { // 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. 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..4944437d6 100644 --- a/.claude/workflows/pair-implement-batch.test.mjs +++ b/.claude/workflows/pair-implement-batch.test.mjs @@ -1002,6 +1002,47 @@ 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, /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('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/packages/knowledge-hub/dataset/.workflows/pair-implement-batch.js b/packages/knowledge-hub/dataset/.workflows/pair-implement-batch.js index f95817702..c830a31e2 100644 --- a/packages/knowledge-hub/dataset/.workflows/pair-implement-batch.js +++ b/packages/knowledge-hub/dataset/.workflows/pair-implement-batch.js @@ -1230,7 +1230,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) @@ -1361,7 +1364,7 @@ async function driveStory(story) { // 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. 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/packages/knowledge-hub/dataset/.workflows/pair-implement-batch.test.mjs b/packages/knowledge-hub/dataset/.workflows/pair-implement-batch.test.mjs index b3360499b..4944437d6 100644 --- a/packages/knowledge-hub/dataset/.workflows/pair-implement-batch.test.mjs +++ b/packages/knowledge-hub/dataset/.workflows/pair-implement-batch.test.mjs @@ -1002,6 +1002,47 @@ 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, /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('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, From 32713920177e3ebe9923fa68e9e675105181b4fc Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Sun, 30 Aug 2026 20:07:02 +0200 Subject: [PATCH 02/13] =?UTF-8?q?[#217]=20feat:=20`##=20Workflows`=20?= =?UTF-8?q?=E2=80=94=20the=20tag=E2=86=92workflow=20mapping=20schema=20+?= =?UTF-8?q?=20reader?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - KB schema (dataset + mirror): seventh section of tech/automation.md — ` ⇒ ` entries + optional `Precedence:`, tag as opaque routing key (D18), untagged ⇒ never, absent section ⇒ no mapping (opt-in, never an error), eligibility before routing, read-time + routing-time HALTs - workflow-mapping.ts: parses/validates the section; unknown-workflow and multi-tag rules deliberately left to routing (they need board + skill set) - policy-sections.ts: section/HALT/label primitives extracted from automation-policy.ts so both readers of the file share one answer - conformance guard extended over dataset + mirror - Task: T1 — Mapping schema in adoption + validation Refs: #217 --- .../automation/automation-policy.md | 60 ++++++- .../src/commands/run/automation-policy.ts | 135 +++++----------- .../src/commands/run/policy-sections.ts | 95 +++++++++++ .../src/commands/run/workflow-mapping.test.ts | 144 +++++++++++++++++ .../src/commands/run/workflow-mapping.ts | 151 ++++++++++++++++++ .../automation/automation-policy.md | 60 ++++++- .../automation-eligibility.test.ts | 124 +++++++++++++- 7 files changed, 674 insertions(+), 95 deletions(-) create mode 100644 apps/pair-cli/src/commands/run/policy-sections.ts create mode 100644 apps/pair-cli/src/commands/run/workflow-mapping.test.ts create mode 100644 apps/pair-cli/src/commands/run/workflow-mapping.ts diff --git a/.pair/knowledge/guidelines/collaboration/automation/automation-policy.md b/.pair/knowledge/guidelines/collaboration/automation/automation-policy.md index 4da2ad68c..bfe9fbadc 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,63 @@ 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-refine ⇒ pair-process-refine-story +Precedence: auto-dev, auto-refine +``` + +- **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. It is resolved against the **installed** skill set, so this file carries no workflow catalog to drift from reality. +- **`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; +- **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 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 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. 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. + ## 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/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: