feat(implement-loop): tier the quality review by independently-measured diff size
qual+repairs are the pipeline's largest cost pool (51M of 164.5M real tokens, 31%, in the 206-run corpus), with opus weighing ~5x on the quota side. The opus pin's own rationale (consequence-of-a-miss, agent-template § model) scales with diff size — so the tier now does too: the spec-reviewer, which runs before quality, reads the diff itself and is independent of the implementer, reports diff_magnitude (git diff HEAD --shortstat) and touches_contract; <=25 changed lines and no contract-referenced path -> sonnet/high, anything larger, contract-touching, or unmeasured -> opus/xhigh (fail conservative). The independent end-verify/mini-verify suite gate is the deterministic backstop that makes the lower tier defensible; the chosen tier is logged per task (qual_tier) in the end-report. Sampling phase per the issue's acceptance: the first ~10 real small-diff iterations get a one-off opus second review; important+ divergences are recorded on the issue before the threshold counts as proven. closes #30
This commit is contained in:
@@ -169,6 +169,16 @@ ground truth, never a self-report** — a self-report must never be able to
|
||||
The E2E phase's `status` is also read now (not just its fixtures): a
|
||||
non-`DONE` E2E status or a zero-fixture run surfaces as a concern.
|
||||
|
||||
**Quality-tier selection** (issue #30). The quality phase runs
|
||||
opus/xhigh by default, tiered to sonnet/high for a small diff: ≤25
|
||||
changed lines AND no contract-referenced path, both measured by the
|
||||
spec-reviewer (which reads `git diff HEAD` itself — never the
|
||||
implementer's self-report) via `diff_magnitude` / `touches_contract`.
|
||||
Unmeasured or contract-touching diffs stay opus. The independent
|
||||
end-verify suite gate is the deterministic backstop; the chosen tier
|
||||
is logged per task in the end-report (`qual_tier`) so the split stays
|
||||
auditable.
|
||||
|
||||
**Cross-task discard guard** (issue #23). In a multi-task run nothing
|
||||
commits between tasks, so one task's file-level `git checkout`/`git
|
||||
restore` can silently destroy an earlier task's DONE work in a shared
|
||||
|
||||
@@ -19,6 +19,14 @@ has reported `compliant`, never before.
|
||||
only after the spec-compliance phase reports `compliant` —
|
||||
a separate `agent()` call with a fresh context.)
|
||||
|
||||
The frontmatter pins opus/xhigh as this role's default tier;
|
||||
the workflow overrides it per dispatch (issue #30): a small
|
||||
(≤25 changed lines), non-contract diff — as measured
|
||||
independently by the spec-reviewer, never by the implementer —
|
||||
runs at sonnet/high, with the loop's independent end-verify
|
||||
suite gate as the deterministic correctness backstop. The
|
||||
review bar and this file's rules are identical at both tiers.
|
||||
|
||||
## What this role is for
|
||||
|
||||
Spec compliance answers "did the implementer build the right
|
||||
|
||||
@@ -81,8 +81,12 @@
|
||||
// • Model policy: every agent() call pins an explicit model — never the
|
||||
// session-model inherit (which could route to an unintended tier, e.g.
|
||||
// fable — banned for all plugin agents and workflows). Mechanical and
|
||||
// in-loop steps run sonnet; the quality gate is the one deliberate opus
|
||||
// call (last correctness check before a task is marked DONE).
|
||||
// in-loop steps run sonnet; the quality gate runs opus by default (last
|
||||
// correctness review before the end-verify), TIERED to sonnet/high for a
|
||||
// small (<= 25 changed lines), non-contract diff as measured
|
||||
// independently by the spec-reviewer (issue #30) — the end-verify /
|
||||
// mini-verify suite gate is the deterministic backstop that makes the
|
||||
// tier defensible. Unmeasured or contract-touching diffs stay opus.
|
||||
// • Effort policy mirrors the model policy: every agent() call pins an
|
||||
// explicit effort — never the session inherit. Code-writing and review
|
||||
// steps run high, the opus quality gate runs xhigh, and schema-bound
|
||||
@@ -353,11 +357,26 @@ const IMPL_SCHEMA = {
|
||||
const SPEC_SCHEMA = {
|
||||
type: 'object',
|
||||
additionalProperties: false,
|
||||
required: ['status'],
|
||||
required: ['status', 'diff_magnitude', 'touches_contract'],
|
||||
properties: {
|
||||
status: { type: 'string', enum: ['compliant', 'non_compliant', 'unclear', 'infra_blocked'] },
|
||||
findings: { type: 'array', items: { type: 'string' }, description: 'missing requirements + unrequested extras' },
|
||||
ambiguity: { type: 'string' },
|
||||
// Quality-tier inputs (issue #30): measured HERE because the spec-reviewer
|
||||
// runs before the quality gate, reads the diff itself, and is independent
|
||||
// of the implementer — never a producer self-report.
|
||||
diff_magnitude: {
|
||||
type: 'integer',
|
||||
description:
|
||||
'total changed lines of the FULL `git diff HEAD` (insertions + deletions, from `git diff HEAD --shortstat`) ' +
|
||||
'— measure it yourself; never take it from the implementer',
|
||||
},
|
||||
touches_contract: {
|
||||
type: 'boolean',
|
||||
description:
|
||||
'true if any diffed path is part of (or referenced by) the design ledger / contracts the project\'s ' +
|
||||
'CLAUDE.md facts declare; false when the project declares none',
|
||||
},
|
||||
},
|
||||
}
|
||||
const QUAL_SCHEMA = {
|
||||
@@ -768,6 +787,7 @@ async function runTask(task) {
|
||||
let wroteAnyFile = impl.applied_changes === true
|
||||
|
||||
// 2.2 — spec-compliance check (gated before quality), with repair re-loop
|
||||
let specMeta = null
|
||||
for (let round = 0; ; round++) {
|
||||
const spec = await agent(
|
||||
`${STANDING}${ANCHOR}\n\nSpec-compliance review of task ${task.id}. Compare \`git diff HEAD\` against the task ` +
|
||||
@@ -776,10 +796,17 @@ async function runTask(task) {
|
||||
'you have verified yourself (the actual behaviour provably differs from what the task text asserts), or ' +
|
||||
'two task requirements contradict each other, report status `unclear` with the contradiction in ' +
|
||||
'`ambiguity` — never `non_compliant` for a reality-forced deviation, which no repair can resolve. ' +
|
||||
'Additionally measure and report, independent of any implementer report: `diff_magnitude` (total changed ' +
|
||||
'lines from `git diff HEAD --shortstat`, insertions + deletions) and `touches_contract` (does any diffed ' +
|
||||
"path belong to or get referenced by the design ledger / contracts the project's CLAUDE.md facts declare; " +
|
||||
'false when the project declares none). ' +
|
||||
`Focus on the files this task claims to touch.\n\nTASK ${task.id} — ${task.title}\n${task.text}`,
|
||||
{ agentType: 'spec-reviewer', model: 'sonnet', effort: 'high', label: `spec:${task.id}`, phase: 'Per-task loop', schema: SPEC_SCHEMA },
|
||||
)
|
||||
if (spec && spec.status === 'compliant') break
|
||||
if (spec && spec.status === 'compliant') {
|
||||
specMeta = spec
|
||||
break
|
||||
}
|
||||
if (spec && spec.status === 'unclear') return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: 'spec-ambiguous', detail: spec.ambiguity }
|
||||
if (round >= 2) return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: 'review-loop-exhausted (spec-compliance)' }
|
||||
// repair: re-dispatch the implementer with the review findings as the repair brief
|
||||
@@ -801,6 +828,28 @@ async function runTask(task) {
|
||||
// whose fingerprint was already reviewed-and-flagged this task means the
|
||||
// intervening repair changed nothing or cycled back, so re-running quality
|
||||
// is futile).
|
||||
// Quality-tier selection (issue #30): the review model scales with the
|
||||
// independently-measured diff. The spec-reviewer — a separate agent that
|
||||
// read `git diff HEAD` itself — supplied diff_magnitude and
|
||||
// touches_contract; a small, non-contract diff gets sonnet/high, because
|
||||
// consequence-of-a-miss (the opus pin's rationale, agent-template § model)
|
||||
// scales with diff size and the independent end-verify / mini-verify suite
|
||||
// gate is the deterministic correctness backstop. Anything larger,
|
||||
// contract-touching, or unmeasured (missing/implausible fields) keeps
|
||||
// opus/xhigh — fail conservative. Measured once, before round 0; a repair
|
||||
// can grow the diff, but bounded (it addresses findings on a small diff),
|
||||
// so the tier is not re-derived per round.
|
||||
const QUAL_TIER_MAX_LINES = 25
|
||||
const smallDiff =
|
||||
specMeta !== null &&
|
||||
Number.isInteger(specMeta.diff_magnitude) &&
|
||||
specMeta.diff_magnitude >= 0 &&
|
||||
specMeta.diff_magnitude <= QUAL_TIER_MAX_LINES &&
|
||||
specMeta.touches_contract === false
|
||||
const qualModel = smallDiff ? 'sonnet' : 'opus'
|
||||
const qualEffort = smallDiff ? 'high' : 'xhigh'
|
||||
const qualTier = `${qualModel}/${qualEffort}`
|
||||
|
||||
let heldConcerns = []
|
||||
const seenFingerprints = new Set()
|
||||
for (let round = 0; ; round++) {
|
||||
@@ -811,9 +860,10 @@ async function runTask(task) {
|
||||
'Also report `diff_fingerprint` (REQUIRED): the sha256 hex of the FULL `git diff HEAD` ' +
|
||||
'(`git diff HEAD | sha256sum` — the whole working-tree diff, NOT narrowed to this task\'s footprint). ' +
|
||||
'The loop compares it across rounds to detect a no-op/cyclic repair.',
|
||||
// opus by design: the loop's last correctness gate (spec-reviewer only checks
|
||||
// task-text correspondence) — real-bug finding is the documented opus strength.
|
||||
{ agentType: 'quality-reviewer', model: 'opus', effort: 'xhigh', label: `qual:${task.id}`, phase: 'Per-task loop', schema: QUAL_SCHEMA },
|
||||
// The loop's last correctness gate before the end-verify (spec-reviewer only
|
||||
// checks task-text correspondence) — opus by default (real-bug finding is the
|
||||
// documented opus strength), tiered per the selection above.
|
||||
{ agentType: 'quality-reviewer', model: qualModel, effort: qualEffort, label: `qual:${task.id}`, phase: 'Per-task loop', schema: QUAL_SCHEMA },
|
||||
)
|
||||
if (qual && qual.status === 'approved') break
|
||||
// No-op / cyclic-repair backstop: a changes_requested verdict over a diff-state already reviewed-and-flagged
|
||||
@@ -835,6 +885,7 @@ async function runTask(task) {
|
||||
outcome: 'BLOCKED',
|
||||
reason: 'unresolved important quality finding — the repair left the diff unchanged',
|
||||
detail: importants.map((i) => i.text).join('; '),
|
||||
qual_tier: qualTier,
|
||||
}
|
||||
}
|
||||
heldConcerns = (qual.issues && qual.issues.length ? qual.issues.map((i) => `${i.severity}: ${i.text}`) : ['(finding not reported)']).map(
|
||||
@@ -863,7 +914,7 @@ async function runTask(task) {
|
||||
)
|
||||
break
|
||||
}
|
||||
if (round >= 2) return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: 'review-loop-exhausted (quality)' }
|
||||
if (round >= 2) return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: 'review-loop-exhausted (quality)', qual_tier: qualTier }
|
||||
if (qual && qual.diff_fingerprint) seenFingerprints.add(qual.diff_fingerprint)
|
||||
const fix = await agent(
|
||||
`${STANDING}${ANCHOR}\n\nThe quality review of task ${task.id} requested changes. Address EACH Important/Minor issue; ` +
|
||||
@@ -892,6 +943,7 @@ async function runTask(task) {
|
||||
title: task.title,
|
||||
outcome: 'BLOCKED',
|
||||
reason: `quality/plan conflict: ${fix.reason || 'honouring a quality finding would break the ratified plan'}`,
|
||||
qual_tier: qualTier,
|
||||
}
|
||||
}
|
||||
if (fix && fix.held && fix.held.length) {
|
||||
@@ -923,6 +975,7 @@ async function runTask(task) {
|
||||
title: task.title,
|
||||
outcome: 'DONE',
|
||||
concerns: [...concerns, ...heldConcerns, ...noopConcern],
|
||||
qual_tier: qualTier,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1131,7 +1184,11 @@ const aggregate = {
|
||||
outcome,
|
||||
tasks_total: tasks.length,
|
||||
tasks_completed: completed.length,
|
||||
task_summaries: results.map((r) => ({ id: r.id, title: r.title, outcome: r.outcome })),
|
||||
// qual_tier (issue #30): which review tier each task's quality gate ran at
|
||||
// (sonnet/high for a small non-contract diff, opus/xhigh otherwise; null
|
||||
// when the task never reached the quality gate) — logged so the tiering
|
||||
// split stays auditable per iteration.
|
||||
task_summaries: results.map((r) => ({ id: r.id, title: r.title, outcome: r.outcome, qual_tier: r.qual_tier || null })),
|
||||
concerns: [...results.flatMap((r) => r.concerns || []), ...e2eConcerns, ...guardConcerns],
|
||||
// synthBlock (a discard or iteration-level no-op verdict) outranks the
|
||||
// per-task block: when the discard guard broke the loop on a task that
|
||||
|
||||
Reference in New Issue
Block a user