Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions .claude/skills/pair-capability-publish-pr/SKILL.md

Large diffs are not rendered by default.

79 changes: 62 additions & 17 deletions .claude/workflows/pair-implement-batch.js

Large diffs are not rendered by default.

159 changes: 155 additions & 4 deletions .claude/workflows/pair-implement-batch.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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' }])
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ and the package script runs the module through a TS runner (`ts-node`/`tsx`) beh

**Scripts are never unit-tested.** No importing a script's functions into a test, and no black-box `spawnSync`/`exec` of a script inside a vitest unit test. Unit tests target the module's exported logic. When script/CLI-level (end-to-end) verification is wanted, it uses the **smoke-test suite** (`scripts/smoke-tests/`, `pnpm smoke-tests`), not vitest.

> **Bounded exception, added 2026-09-01 (#419)** — a *thin script whose behaviour IS the deliverable* (no logic to extract; `run-format.sh`, `regenerate-mirrors.sh`) may be black-box executed from vitest against a **throwaway fixture**, asserting observable behaviour only. Conditions, rationale and why the smoke suite is not the right home for those cases: [2026-09-01-publish-pr-realigns-mirrors-before-the-gate.md](./2026-09-01-publish-pr-realigns-mirrors-before-the-gate.md), Decision. Everything above stands for every script that does hold logic — the fix there is still "extract to a module + white-box test".

Rationale: a gate is testable logic, not an opaque script; keeping the logic in an importable module removes duplication and orphan tests that reach into root `scripts/`; unit tests then cover module logic while smoke tests cover CLI wiring end-to-end. The module's public functions are the single tested surface; the CLI wrapper is a trivial, unit-test-exempt shell.

## Alternatives Considered
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ A second failure mode surfaced while implementing this: generated artifacts. In

## Resolved Decision (2026-08-05) — neither A nor B; a dedicated command instead

**Closed 2026-09-01 by story #419** — the Open Decision below is no longer open, and nothing here is pending. All three parts shipped: the command is the root script `pnpm mirrors:regenerate` (`scripts/regenerate-mirrors.sh`, wrapping `pair update --source <local dataset> --offline`, no check mode); `PRE_PUSH_REMEDY`, `DEVELOPMENT.md`, its docs-site twin **and both mirror guards' own failure messages** name it instead of `pair update`; and `/pair-capability-publish-pr` runs it in Phase 1, before its gate, committing the output separately when it drifted — see ADL [2026-09-01-publish-pr-realigns-mirrors-before-the-gate.md](./2026-09-01-publish-pr-realigns-mirrors-before-the-gate.md) for why that phase and why the command is read from the adoption. `pnpm format` and the check-only gate are byte-for-byte unchanged.

**Decided by the maintainer on 2026-08-05. Tracked as story #419.** Both shapes below were declined as written, for reasons that only became visible when the actual remedy command was inspected.

**The remedy was naming the wrong command.** `PRE_PUSH_REMEDY`, `DEVELOPMENT.md` and its docs-site twin all say `pair update` — which `DEVELOPMENT.md` itself documents as *"Update knowledge base to latest version"*. That resolves and installs the **published** KB; what a mirror divergence needs is regeneration **from the local dataset** (`pair update --source <local dataset>`, the form `CP3` and the `source-resolution` smoke scenario already exercise). So the documented fix for a reformatted table was a knowledge-base update — disproportionate and non-deterministic, and the most plausible explanation for why three of the seven incidents were hand-ports: a contributor faced with that command reasonably chose to edit the mirror instead.
Expand All @@ -68,7 +70,7 @@ A second failure mode surfaced while implementing this: generated artifacts. In

This also becomes load-bearing once **#414** lands: with the mirrors inside `format:check` scope, a contributor without this command would be pushed toward hand-formatting a generated file — which the mirror guards forbid.

### Original framing (kept for the record)
### Original framing (kept for the record — closed, see above)

**Should the gate apply the fix as well as failing, and should `pnpm format` realign the generated mirrors?** Raised by the maintainer 2026-08-04 while reviewing this story; deliberately not implemented at the time, and not to be implemented without their call.

Expand Down
Original file line number Diff line number Diff line change
@@ -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 <reviewedHead>...origin/<branch>` 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.
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# Decision: External boundary proof prevents false equivalence

## Date

2026-09-01

## Status

Active

## Category

Process Decision

## Context

The finite-state inventory rule exposed #416's CRLF problem, but the fixer extended the repair to
bare CR by treating all carriage-return forms as equivalent. Its unit tests proved only the drift
checker’s classification. A minimal real Git probe showed the promised remedy was false: Git
normalizes CRLF to LF, but preserves a lone CR blob through checkout. The next review therefore
found a major regression: its advice forbade the only repair that actually worked.

## Decision

Whenever a decision-table row, equivalence, normalization or user-facing repair depends on an
external command, service, file format or runtime, identify its authoritative producer/consumer
and prove the claim with a minimal isolated end-to-end probe. Keep variants distinct until that
boundary demonstrates equivalence. Apply any proposed repair in the probe and verify its stated
postcondition. Unit tests of the changed function remain required but cannot substitute for this
boundary evidence.

## Alternatives Considered

- **Trust the implementation unit tests**: rejected — they cannot establish Git’s checkout
semantics or prove user guidance outside the function’s process.
- **Treat all syntactically similar inputs as one state**: rejected — the external producer can
distinguish them, as CRLF and lone CR demonstrate.
- **Defer external proof to re-review**: rejected — that makes the reviewer discover a repair
regression after the fixer has already committed it.

## Consequences

- A finite-state table may contain an external-boundary probe per row or equivalence class.
- Fix reports include command, observed result and postcondition for external repair claims.
- Re-review may still stop on increased findings; it remains the signal that the required proof
was absent or incorrect.

## Adoption Impact

- `.pair/adoption/tech/way-of-working.md`: extends Review Convergence with the boundary-proof
requirement.
- `packages/knowledge-hub/dataset/.workflows/pair-implement-batch.js` and installed mirror:
require reviewers and fixers to prove externally-defined semantics and repair advice.
- Both workflow test copies: pin the prompt requirement so it cannot silently disappear.
Loading
Loading