Skip to content
Merged
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
2 changes: 1 addition & 1 deletion .claude/.claude-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "orchestrai",
"displayName": "OrchestrAI",
"version": "2.8.1",
"version": "2.9.0",
"description": "Orchestrator team: the role agents, the tm- operational skills, and the review workflows.",
"author": {
"name": "Thomas Mueller"
Expand Down
46 changes: 43 additions & 3 deletions .claude/adapters/codex-renderer.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,38 @@ function getFixedListItems(stage, ctx, args) {
return args[stage.items_key] || [{ key: 'stub', name: 'stub-item' }]
}

function getDynamicListItems(stage, ctx, args, name, log) {
// Closed set of item reducers a dynamic-list stage can name with
// items_transform. A reducer only flattens, filters and dedups; the cap stays
// the one generic step in getDynamicListItems. Keep in sync with
// hermes-renderer.mjs and the "Spec format" section of adapter-interface.md.
const ITEM_TRANSFORMS = {
// Worker reports ({ findings: [...] }) -> unique must-fix findings.
must_fix_deduped(reports) {
const seen = new Set()
const out = []
for (const report of reports) {
if (!report || !Array.isArray(report.findings)) continue
for (const f of report.findings) {
if (!f || f.severity !== 'must-fix') continue
const key = JSON.stringify([f.file, f.line, f.problem])
if (seen.has(key)) continue
seen.add(key)
out.push(f)
}
}
return out
},
}

// Exported for testing: Codex's spawn shells out, so the resolver is the
// testable seam.
export function getDynamicListItems(stage, ctx, args, name, log) {
// An unknown transform must fail loudly: a host without the reducer would
// otherwise fan out over unreduced items.
const transform = stage.items_transform
if (transform !== undefined && !Object.hasOwn(ITEM_TRANSFORMS, transform)) {
throw new Error(`stage ${name}: unknown items_transform "${transform}"`)
}
// items_source is a dotted path like "scout_result.areas".
const parts = stage.items_source.split('.')
let val = ctx
Expand All @@ -117,8 +148,16 @@ function getDynamicListItems(stage, ctx, args, name, log) {
)
return [{ name: 'stub-area', paths: ['.'], why: 'stub' }]
}
const cap = args[stage.items_cap] || stage.items_default_cap || Infinity
return val.slice(0, cap)
const items = transform ? ITEM_TRANSFORMS[transform](val) : val
// items_cap names an args field as "args.<field>"; same coercion as the JS
// workflows' MAX_AREAS (positive integer, else the default).
const capField = typeof stage.items_cap === 'string' ? stage.items_cap.replace(/^args\./, '') : undefined
const passed = capField ? args?.[capField] : undefined
const cap = Number.isInteger(passed) && passed > 0 ? passed : stage.items_default_cap || Infinity
if (items.length > cap) {
log(`stage ${name}: ${items.length - cap} item(s) past the cap of ${cap} were not dispatched`)
}
return items.slice(0, cap)
}

function inferRole(stageName) {
Expand All @@ -128,6 +167,7 @@ function inferRole(stageName) {
area_review: 'developer',
area_map: 'developer',
architecture_review: 'developer',
verify: 'fact-checker',
consolidate: 'reviewer',
synthesize: 'architect',
}
Expand Down
56 changes: 51 additions & 5 deletions .claude/adapters/hermes-renderer.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ export function renderTemplate(template, vals) {
const STUB_SLOTS = {
coverageNote: '',
rawFindings: '[]',
confirmedFindings: '[]',
refutedFindings: '[]',
unverifiedFindings: '[]',
reviewedAreas: '[]',
mappedAreas: '[]',
workersFailed: '[]',
Expand Down Expand Up @@ -205,6 +208,9 @@ function collectSlotVals(name, args, root, base) {
} else if (name === 'review') {
// brief and diffHint are per-item; supplied by buildItemTaskPrompt.
vals.diffHint = buildDiffHint(base)
} else if (name === 'verify') {
// finding is per-item; supplied by buildItemTaskPrompt.
vals.diffHint = buildDiffHint(base)
} else if (name === 'consolidate' || name === 'synthesize') {
if (name === 'consolidate') vals.diffHint = buildDiffHint(base)
}
Expand Down Expand Up @@ -234,6 +240,8 @@ function buildItemTaskPrompt(stage, name, item, ctx, args, prompts, root, base)
vals.areaPaths = Array.isArray(item.paths) ? item.paths.join(', ') : ''
vals.repoMap = '' // stub: derived from the scout result in the JS
vals.brief = item.brief || ''
// verify items are findings; the template takes the whole item as JSON.
vals.finding = JSON.stringify(item, null, 2)
}
return renderTemplate(template, vals)
}
Expand All @@ -247,8 +255,37 @@ function getFixedListItems(stage, ctx, args) {
return args[stage.items_key] || [{ key: 'stub', name: 'stub-item' }]
}

// Resolve the item list for a dynamic-list stage.
function getDynamicListItems(stage, ctx, args, name, log) {
// Closed set of item reducers a dynamic-list stage can name with
// items_transform. A reducer only flattens, filters and dedups; the cap stays
// the one generic step in getDynamicListItems. Keep in sync with
// codex-renderer.mjs and the "Spec format" section of adapter-interface.md.
const ITEM_TRANSFORMS = {
// Worker reports ({ findings: [...] }) -> unique must-fix findings.
must_fix_deduped(reports) {
const seen = new Set()
const out = []
for (const report of reports) {
if (!report || !Array.isArray(report.findings)) continue
for (const f of report.findings) {
if (!f || f.severity !== 'must-fix') continue
const key = JSON.stringify([f.file, f.line, f.problem])
if (seen.has(key)) continue
seen.add(key)
out.push(f)
}
}
return out
},
}

// Resolve the item list for a dynamic-list stage. Exported for testing.
export function getDynamicListItems(stage, ctx, args, name, log) {
// An unknown transform must fail loudly: a host without the reducer would
// otherwise fan out over unreduced items.
const transform = stage.items_transform
if (transform !== undefined && !Object.hasOwn(ITEM_TRANSFORMS, transform)) {
throw new Error(`stage ${name}: unknown items_transform "${transform}"`)
}
// Dynamic-list stages read items from a previous stage's output.
// items_source is a dotted path like "scout_result.areas".
const parts = stage.items_source.split('.')
Expand All @@ -265,9 +302,16 @@ function getDynamicListItems(stage, ctx, args, name, log) {
)
return [{ name: 'stub-area', paths: ['.'], why: 'stub' }]
}
// Cap the item count.
const cap = args[stage.items_cap] || stage.items_default_cap || Infinity
return val.slice(0, cap)
const items = transform ? ITEM_TRANSFORMS[transform](val) : val
// items_cap names an args field as "args.<field>"; same coercion as the JS
// workflows' MAX_AREAS (positive integer, else the default).
const capField = typeof stage.items_cap === 'string' ? stage.items_cap.replace(/^args\./, '') : undefined
const passed = capField ? args?.[capField] : undefined
const cap = Number.isInteger(passed) && passed > 0 ? passed : stage.items_default_cap || Infinity
if (items.length > cap) {
log(`stage ${name}: ${items.length - cap} item(s) past the cap of ${cap} were not dispatched`)
}
return items.slice(0, cap)
}

// Infer the role agent for a stage from the stage name.
Expand All @@ -282,6 +326,8 @@ function inferRole(stageName) {
area_review: 'developer',
area_map: 'developer',
architecture_review: 'developer',
// A read-only claim audit on the worker tier; the default would be developer.
verify: 'fact-checker',
consolidate: 'reviewer',
synthesize: 'architect',
}
Expand Down
2 changes: 1 addition & 1 deletion .claude/skills/tm-review-changes/SKILL.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
name: tm-review-changes
description: Token-bounded code review of the current diff, run as a workflow (fixed Sonnet reviewers, one per dimension including docs drift and performance, plus one Opus critic). Plugin-only wrapper; the committed-repo root invokes the tm-review-changes workflow directly by name. User-invocable only.
description: Token-bounded code review of the current diff, run as a workflow (fixed Sonnet reviewers, one per dimension including docs drift and performance, then a capped Sonnet verify pass on must-fix findings, plus one Opus critic). Plugin-only wrapper; the committed-repo root invokes the tm-review-changes workflow directly by name. User-invocable only.
disable-model-invocation: true
argument-hint: "[base ref, default origin/main]"
---
Expand Down
98 changes: 98 additions & 0 deletions .claude/workflows/__tests__/helpers.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -497,3 +497,101 @@ describe('criticWithFallback', () => {
}
})
})

// ===========================================================================
// 6. mustFixDeduped and finalizeReport (issue #406)
//
// Both are plain function declarations so loadFn can slice them out. Neither
// has free variables, so no sandbox is needed. mustFixDeduped mirrors the
// ITEM_TRANSFORMS.must_fix_deduped reducer in the two renderers; the
// renderers' copy is covered in item-transforms.test.mjs.
// ===========================================================================
describe('mustFixDeduped', () => {
const mustFixDeduped = loadFn('tm-review-changes.js', 'mustFixDeduped')
const f = (file, line, problem, severity = 'must-fix') => ({ file, line, problem, severity, fix: 'x' })

test('keeps only must-fix findings, flattened across reports', () => {
const out = mustFixDeduped([
{ findings: [f('a.js', '1', 'p1'), f('a.js', '2', 'p2', 'nit')] },
{ findings: [f('b.js', '3', 'p3'), f('c.js', '4', 'p4', 'should-fix')] },
])
assert.deepEqual(Array.from(out, (x) => x.problem), ['p1', 'p3'])
})

test('dedups on file + line + problem, keeping the first', () => {
const out = mustFixDeduped([
{ findings: [f('a.js', '1', 'p1')] },
{ findings: [f('a.js', '1', 'p1'), f('a.js', '2', 'p1'), f('a.js', '1', 'other')] },
])
assert.equal(out.length, 3)
})

test('skips null reports and reports with no findings array', () => {
const out = mustFixDeduped([null, undefined, {}, { findings: 'x' }, { findings: [f('a.js', '1', 'p1')] }])
assert.deepEqual(Array.from(out, (x) => x.problem), ['p1'])
})

test('returns an empty array for no reports', () => {
assert.deepEqual(Array.from(mustFixDeduped([])), [])
})
})

describe('finalizeReport', () => {
const finalizeReport = loadFn('tm-review-changes.js', 'finalizeReport')
const f = (file, line, problem) => ({ file, line, severity: 'must-fix', problem, fix: 'x' })

test('drops a refuted finding from mustFix and lists it under refuted', () => {
const kept = f('a.js', '1', 'real')
const gone = f('b.js', '2', 'stale')
const refuted = [{ ...gone, note: 'already fixed' }]
const out = finalizeReport({ verdict: 'changes-requested', summary: 's', mustFix: [kept, gone] }, refuted, [])
assert.deepEqual(out.mustFix.map((x) => x.problem), ['real'])
assert.equal(out.refuted.length, 1)
assert.equal(out.refuted[0].problem, 'stale')
assert.equal(out.refuted[0].note, 'already fixed')
})

test('recomputes the verdict to approve when only refuted findings were removed', () => {
const gone = f('b.js', '2', 'stale')
const out = finalizeReport({ verdict: 'changes-requested', mustFix: [gone] }, [{ ...gone, note: 'n' }], [])
assert.equal(out.verdict, 'approve')
assert.deepEqual(Array.from(out.mustFix), [])
})

test('keeps changes-requested when a non-refuted finding remains', () => {
const kept = f('a.js', '1', 'real')
const gone = f('b.js', '2', 'stale')
const out = finalizeReport({ verdict: 'changes-requested', mustFix: [gone, kept] }, [{ ...gone, note: 'n' }], [])
assert.equal(out.verdict, 'changes-requested')
assert.deepEqual(out.mustFix.map((x) => x.problem), ['real'])
})

test('matches on file + line + problem, not on problem alone', () => {
const a = f('a.js', '1', 'same text')
const b = f('b.js', '1', 'same text')
const out = finalizeReport({ mustFix: [a, b] }, [{ ...a, note: 'n' }], [])
assert.deepEqual(out.mustFix.map((x) => x.file), ['b.js'])
})

test('sets unverified from the script data, overriding what the model wrote', () => {
const u = [f('a.js', '1', 'unchecked')]
const out = finalizeReport({ mustFix: [], unverified: [f('z.js', '9', 'model made this up')], refuted: [f('y.js', '1', 'model made this up too')] }, [], u)
assert.deepEqual(out.unverified, u)
assert.deepEqual(out.refuted, [])
})

test('does not throw when the report has no mustFix array', () => {
const out = finalizeReport({ _stub: true }, [], [])
assert.equal(out._stub, true)
assert.equal('mustFix' in out, false)
assert.deepEqual(out.refuted, [])
assert.deepEqual(out.unverified, [])
})

test('returns a new object and does not mutate a frozen input', () => {
const input = Object.freeze({ verdict: 'approve', mustFix: Object.freeze([]) })
const out = finalizeReport(input, [], [])
assert.notEqual(out, input)
assert.equal('refuted' in input, false)
})
})
33 changes: 33 additions & 0 deletions .claude/workflows/__tests__/hermes-adapter.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,39 @@ describe('hermes workflow renderer', () => {
)
})

test('tm-map-codebase: items_cap "args.areas" resolves to the passed args.areas value', async () => {
const calls = []
const origDryRun = process.env.DRY_RUN
const origDelegate = globalThis.delegate_task
process.env.DRY_RUN = 'false'
globalThis.delegate_task = async ({ goal, output_schema }) => {
calls.push({ goal, output_schema })
if (output_schema === 'MAP_SCHEMA') {
return {
areas: [
{ name: 'alpha-area', paths: ['alpha/'], why: 'core' },
{ name: 'beta-area', paths: ['beta/'], why: 'support' },
],
dropped: [],
}
}
return { findings: [], summary: 'ok' }
}

try {
await renderWorkflow('tm-map-codebase', { areas: 1 })
} finally {
globalThis.delegate_task = origDelegate
process.env.DRY_RUN = origDryRun
}

const areaMapGoals = calls
.filter((c) => c.output_schema === 'AREA_MAP_SCHEMA')
.map((c) => c.goal)
assert.equal(areaMapGoals.length, 1, 'args.areas = 1 must cap the dispatch at one area')
assert.ok(areaMapGoals[0].includes('alpha-area'), 'the cap keeps the first area')
})

test('tm-review-codebase: area_review stage receives the real scout areas, not the stub', async () => {
const calls = []
const origDryRun = process.env.DRY_RUN
Expand Down
63 changes: 63 additions & 0 deletions .claude/workflows/__tests__/hermes-verify-stage.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* Hermes renderer: tm-review-changes verify stage (issue #406).
*
* Drives the renderer in live mode against a stubbed delegate_task global and
* inspects the prompts it sends. The verify stage's items come from the review
* stage's reports through the must_fix_deduped transform.
*/

import { test, describe } from 'node:test'
import assert from 'node:assert/strict'

const RENDERER_PATH = '../../adapters/hermes-renderer.mjs'

const MUST_FIX = { file: 'src/a.js', line: '12', severity: 'must-fix', problem: 'unique-problem-text', fix: 'f' }

async function runLive() {
const calls = []
const origDryRun = process.env.DRY_RUN
const origDelegate = globalThis.delegate_task
process.env.DRY_RUN = 'false'
globalThis.delegate_task = async ({ goal, output_schema }) => {
calls.push({ goal, output_schema })
if (output_schema === 'FINDINGS_SCHEMA') return { findings: [MUST_FIX] }
if (output_schema === 'VERIFY_SCHEMA') return { confirmed: true, note: 'reproduced' }
return { verdict: 'approve', summary: 's', mustFix: [], shouldFix: [], nits: [] }
}
const origWarn = console.warn
console.warn = () => {}
try {
const { renderWorkflow } = await import(RENDERER_PATH)
await renderWorkflow('tm-review-changes', { base: 'origin/main' })
} finally {
console.warn = origWarn
globalThis.delegate_task = origDelegate
process.env.DRY_RUN = origDryRun
}
return calls
}

describe('hermes renderer: tm-review-changes verify stage', () => {
test('dispatches one verifier for the deduped must-fix finding, with the finding and diffHint filled in', async () => {
const calls = await runLive()
const verifyCalls = calls.filter((c) => c.output_schema === 'VERIFY_SCHEMA')
assert.equal(verifyCalls.length, 1, 'every reviewer reports the same finding; dedup leaves one verifier')
const goal = verifyCalls[0].goal
assert.ok(goal.includes('unique-problem-text'), 'verify prompt must carry the finding JSON')
assert.ok(goal.includes('origin/main...HEAD'), 'verify prompt must carry the diffHint')
assert.ok(goal.includes('adversarial verifier'), 'verify prompt must carry the adversarial stance')
assert.ok(!goal.includes('{{'), 'no unfilled slot markers')
})

test('verify runs on the fact-checker role prompt, not the developer default', async () => {
const calls = await runLive()
const goal = calls.find((c) => c.output_schema === 'VERIFY_SCHEMA').goal
assert.ok(goal.startsWith('# Fact-checker role prompt'), `got: ${goal.slice(0, 60)}`)
})

test('the consolidate prompt renders with no unfilled slot markers', async () => {
const calls = await runLive()
const goal = calls.find((c) => c.output_schema === 'REPORT_SCHEMA').goal
assert.ok(!goal.includes('{{'), 'no unfilled slot markers')
})
})
Loading
Loading