[#220] feat: breakdown-to-task feedback loop (close tasks + comment)

.claude/skills/pair-loop/SKILL.md

@@ -1,7 +1,7 @@
 ---
 name: pair-loop
 description: "Unattended delivery loop: per iteration, selects eligible cards via pair-next, runs a dependency + mutex analysis, composes implement-batch for a mutex-safe parallel batch (or drives one card sequentially), enacts the automation policy (auto-advance) and evaluates the stop predicate. In an environment with a fan-out runner it delegates the whole unattended run to the pair-loop workflow; elsewhere it drives exactly one eligible card and reports a continue-token."
-version: 0.1.0
+version: 0.1.1
 author: Foomakers
 ---
 
@@ -50,6 +50,7 @@ Fan-out is ONE capability with THREE realizations, in preference order: **(1) in
 
 - **Never re-implements the per-card pipeline.** Implement→PR→review is `/implement-batch`'s (#219); this skill composes it and consumes its outcomes only (PR opened/updated, review-approved, escalated, failed).
 - **Never merges outside the tier the policy permits.** Merge authority belongs to `## Auto-Advance`, never to this skill inventing a looser rule, and never to branch protection it cannot count on (a project with `Review enforcement: disabled` gets no safety net there — this skill verifies the 🟢 gate set itself before ever pushing/merging).
+- **Never posts its own per-task progress comments.** The breakdown-to-task feedback loop — checklist ticks plus one batched comment per run iteration — belongs to `/pair-process-implement` ([task-progress-feedback.md](../../../.pair/knowledge/guidelines/collaboration/project-management-tool/task-progress-feedback.md)), which this skill reaches through `/implement-batch`, so a supervised run already carries it. A second writer here would double every line. The one comment this skill does post is its own: the card-level "awaits human action" note in the degraded path, which reports the loop's decision, not a task's outcome.
 - **Never modifies `/pair-next`.** Selection stays the frozen atom (ADR-017 §1) — this skill only ever passes `--root`/`--filter`.
 - **Never widens an override.** A pin-sequential or exclude override may only narrow the parallel set the dependency+mutex analysis computed; it can never add back an excluded card.
 

.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,129 @@ 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('review and fix exhaust finite protocol states before another round', async () => {
+  const finding = { location: 'state.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 review = calls.find(c => c.opts.agentType === 'pair-reviewer').prompt
+  const fix = calls.find(c => c.opts.label?.startsWith('fix:')).prompt
+  assert.ok(review.includes('CONTRACT INVENTORY (mandatory)'), 'the reviewer inventories a contract before reporting its first hole')
+  assert.ok(review.includes('finite decision table of every supported state'), 'a finite protocol/state space is exhausted in the same review')
+  assert.ok(review.includes('AUTHORITATIVE BOUNDARY PROOF (mandatory)'), 'the reviewer must prove externally-defined state semantics at the real boundary')
+  assert.ok(fix.includes('FINITE-STATE COMPLETENESS (mandatory when'), 'the fixer must preserve that complete state model')
+  assert.ok(fix.includes('Do not implement one newly discovered row at a time'), 'the next re-review is not used to discover ordinary variants serially')
+  assert.ok(fix.includes('A unit test of the function being changed cannot establish external semantics'), 'the fixer cannot infer external-tool behavior from its own unit tests')
+})
+
+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,
@@ -1247,6 +1398,68 @@ test('an unknown severity blocks regardless of the floor (fail safe), and a bad
   )
 })
 
+// ── The DEFAULT floor ──────────────────────────────────────────────────────
+// Measured across three cycles on PR #477: the PR reached APPROVED with zero actionable
+// findings, the next round implemented review Questions the reviewer had marked "No change
+// requested", and the re-review found new Minors INSIDE the code that round added (three the
+// first time, two the second). Questions are, by the review template's own definition,
+// questions for the human — putting them in the fix set contradicts what they are. The floor
+// therefore defaults to `Minor`: Major and Minor block and drive fix rounds, everything below
+// is carried to the merge gate. A caller can still pass a floor explicitly to override it.
+function contractWithQuestions() {
+  const c = validContract()
+  c.vocabulary.severities = ['Blocker', 'Major', 'Minor', 'Questions']
+  c.severityRanks = { Blocker: 4, Major: 3, Minor: 2, Questions: 1 }
+  return c
+}
+const QUESTION = { location: 'c.ts:3', severity: 'Questions', description: 'is this intended?', recommendation: 'no change requested' }
+
+test('by default a Questions-only review converges: carried to the gate, never fixed', async () => {
+  const { result, calls } = await runWorkflow({
+    args: { stories: [STORY] },
+    dispatch: stdDispatch({
+      contractResult: { status: 'cache-hit', contract: contractWithQuestions() },
+      review: { verdict: 'Rework', findings: [QUESTION, { ...QUESTION, location: 'c.ts:9' }] },
+    }),
+  })
+  const b = result.batch[0]
+  assert.equal(b.status, 'ready-for-merge', 'Questions alone do not keep the loop open')
+  assert.equal(b.acceptedFindings.length, 2, 'both Questions reach the human')
+  assert.match(b.acceptedFindings[0].disposition, /Below severity floor \(Minor\)/)
+  assert.ok(!calls.some(c => c.opts.label?.startsWith('fix:')), 'no fix round is spent on a Question')
+})
+
+test('by default a Minor still blocks and still drives a fix round', async () => {
+  let round = 0
+  const { result, calls } = await runWorkflow({
+    args: { stories: [STORY] },
+    dispatch: (prompt, opts) => {
+      if (opts.agentType === 'pair-contract-generator') return { status: 'cache-hit', contract: contractWithQuestions() }
+      if (opts.agentType === 'pair-reviewer')
+        return round++ === 0 ? { verdict: 'Rework', findings: [MINOR, QUESTION] } : { verdict: 'Approved', findings: [QUESTION] }
+      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, 'ready-for-merge')
+  assert.ok(calls.some(c => c.opts.label?.startsWith('fix:')), 'the Minor drove a fix round')
+})
+
+test('a vocabulary without Minor falls back to no floor instead of throwing', async () => {
+  const c = validContract()
+  c.vocabulary.severities = ['Severe', 'Trivial']
+  c.severityRanks = { Severe: 2, Trivial: 1 }
+  const { result } = await runWorkflow({
+    args: { stories: [STORY] },
+    dispatch: stdDispatch({
+      contractResult: { status: 'cache-hit', contract: c },
+      review: { verdict: 'Rework', findings: [{ location: 'x:1', severity: 'Trivial', description: 'd', recommendation: 'r' }] },
+    }),
+  })
+  assert.equal(result.batch[0].status, 'escalate', 'without a usable default every actionable finding blocks, as before')
+})
+
 // ── Options must survive a JSON-string `args` ───────────────────────────────
 // Real bug: the runtime can hand this script `args` as a JSON STRING. parseBatchArgs
 // normalized it, but severityFloor was read off the RAW value, where