diff --git a/README.md b/README.md index 050445c..a76975f 100644 --- a/README.md +++ b/README.md @@ -80,3 +80,5 @@ no-credit acceptance tests. The [implementation record](docs/ogc-1560-review-har separates tested capabilities from remaining integrations and quality evaluation. The [specification](docs/ogc-1560-review-harness/specification.md) defines the target contract; the checklist tracks implementation. Automatic publication, Claude integration and the model-quality pilot are later work. + +See the [execution guide](docs/ogc-1560-review-harness/execution-guide.md) for isolated mechanical commands, accepted decisions and re-review history. diff --git a/docs/ogc-1560-review-harness/execution-guide.md b/docs/ogc-1560-review-harness/execution-guide.md new file mode 100644 index 0000000..89c96ce --- /dev/null +++ b/docs/ogc-1560-review-harness/execution-guide.md @@ -0,0 +1,28 @@ +# Running a review + +Create a plan before execution. Plans pin source revisions and retain effective settings. The base revision's `.code-qa.json` or an explicit operator `--config` supplies execution commands; PR prose cannot authorize commands. + +## Mechanical checks without OpenELIS + +Pull a trusted execution image once (`docker pull node:22-bookworm`), then use `--level L1 --environment container --checks checks.json`. The image must have Python 3 for the trusted result collector and the tools needed by the checks. `--image` selects it independently of review level. The run records the resolved image identity. + +```json +[ + {"id":"syntax","kind":"command","argv":["node","--check","src/index.mjs"],"reports":[],"timeoutSeconds":30}, + {"id":"behavior","kind":"test","argv":["node","--test","--test-reporter=junit","--test-reporter-destination=results.xml","test.mjs"],"reports":["results.xml"],"timeoutSeconds":60} +] +``` + +The container has no network, host mounts, credentials, Docker socket or root privileges. Only temporary container filesystems are writable. Install required dependencies in the trusted image beforehand. A missing image or dependency is a failed execution, never a clean review. Containers are deleted in the cleanup path, including failed checks. OpenELIS remains a separate `--environment openelis` adapter with an isolated virtual machine and its own startup/readiness logic. + +## Accepted decisions and discussion + +PR collection retrieves review threads, general comments, reviews and linked issue references. Missing or truncated discussion is recorded as a coverage gap. Discussion is context, not approval: GitHub OWNER/MEMBER association does not make a comment authoritative. + +Use `--decisions decisions.json` to designate accepted intent explicitly. Each entry includes `id`, `source`, `author`, `authority`, `retrievedAt`, full `revision`, `status`, `statement`, `previousExpectation` and `replacementExpectation`. Status is `accepted`, `proposed`, `conflicting` or `unavailable`. Preserve both expectations when approving a removal. Conflicting/unavailable decisions prevent a complete semantic review. Source references should point to the actual decision, not a paraphrased PR description. + +## Re-review + +Pass `--history /previous/run/history.json`. The reviewer reassesses each prior finding as `still-present`, `resolved`, `superseded`, `declined` or `unverified`, retaining original and current evidence. Missing reassessment remains unverified. Evidence is checked against the current source; old report text is not fresh verification. `history.json` is private history, including resolved items; it is not a public comment payload. Nothing is published automatically. + +Reviewers can record a consequential approach objection with its problem, reachable path, supported simpler alternative and counterevidence. Suggestions stay separate from actionable defects. These are reviewer judgments; a valid source quotation alone does not prove the conclusion. diff --git a/docs/ogc-1560-review-harness/implementation-status.md b/docs/ogc-1560-review-harness/implementation-status.md index 118ebe2..bed987c 100644 --- a/docs/ogc-1560-review-harness/implementation-status.md +++ b/docs/ogc-1560-review-harness/implementation-status.md @@ -123,3 +123,7 @@ Live testing found and fixed three harness issues: Docker group activation, a ne The common runner owns source capture, evidence validation and outcomes. The OpenELIS adapter owns its test setup and probes; the existing OpenELIS launcher owns application startup. The model supplies semantic judgment, without repair or publication authority. Obsolete-test removal remains valid when it follows accepted requirements. This version uses a fixed authentication smoke, not complete OpenELIS/browser suites or dynamically selected probes. General repository configuration, human/managed-result import, Claude and other adapters, automated GitHub review, cross-repository collection, hard token/currency caps and the held-out quality evaluation remain later work. Linux-host virtualization and hostile-code penetration testing are unverified. See the [runner guide](../review-runner.md) for usage and limits. + +## Context and isolated commands follow-up + +Added explicit accepted-decision provenance, paginated review-thread collection, private re-review history and generic container execution. Regression: 55 passing tests plus skill validation. Real L1 fixture: two passing behavior tests, zero AI calls, image identity recorded, cleanup removed the container. This is implementation and executor evidence; it does not establish higher-level review quality or completion of Stages 2–5. See [execution guide](execution-guide.md). diff --git a/docs/ogc-1560-review-harness/roadmap.md b/docs/ogc-1560-review-harness/roadmap.md index e741673..6a451b5 100644 --- a/docs/ogc-1560-review-harness/roadmap.md +++ b/docs/ogc-1560-review-harness/roadmap.md @@ -41,12 +41,12 @@ Scope is independent of level. L1 may execute existing deterministic probes; L2 ### G02 — Recover intent, decisions and changed expectations -- [ ] Collect accepted intent, linked decisions and changed expectations with source and authority. +- [x] Collect accepted intent, linked decisions and changed expectations with source and authority. - [ ] **Acceptance:** Record relevant intent and decisions with source, author/authority, retrieval time and revision. Keep old/new expectations when a refactor intentionally changes them. A fixture with an approved removal must not recommend rebuilding the removed machinery just to satisfy an old test. Unavailable or conflicting decisions remain explicit questions. ### G03 — Track findings across review rounds -- [ ] Add stable finding identities, original revisions and re-review dispositions. +- [x] Add stable finding identities, original revisions and re-review dispositions. - [ ] **Acceptance:** Re-review classifies prior items as still present, resolved, superseded, declined with a recorded reason, or unverified. Cite current and original evidence. Resolved items stay in private history without duplicate public comments. Reuse evidence only when its source/configuration remains applicable. ### G04 — Make approach and counterevidence review concrete @@ -66,7 +66,7 @@ Scope is independent of level. L1 may execute existing deterministic probes; L2 ### G16 — Make mechanical checks and environments repository-owned -- [ ] Separate repository-owned mechanical checks and environments from the generic plan/runner. +- [x] Separate repository-owned mechanical checks and environments from the generic plan/runner. - [ ] **Acceptance:** A small repository adapter supplies applicable deterministic commands, setup/readiness/cleanup and evidence collectors. L1 can run selected lint/build/test/probe checks with zero model calls, without booting OpenELIS unnecessarily. Validate a minimal second repository fixture and keep OpenELIS-specific boot logic in its adapter. ### G20 — Correct specifications, examples and tests together @@ -76,7 +76,9 @@ Scope is independent of level. L1 may execute existing deterministic probes; L2 **First slice evidence:** v0.4 executes L1/L2, rejects unsupported higher-level claims before spend/startup, separates model settings from levels, and reports coverage/questions/dispositions. One low-effort Sol fixture review honored the approved obsolete-test deletion; its report explicitly did not claim tests ran. The fixture's two behavior tests were run separately. The test-quality skill is now loaded, but the structured behavior-to-test map and probe executor remain Stage 2 work. -**Still needed in Stage 1:** automatic decision/conversation collection (G02), stable re-review identity/history (G03), fuller approach/alternative verification (G04), adapter conformance completion (G15), generic repository execution (G16), and complete example/skill migration (G20). +**Second slice:** decision provenance, GitHub thread/discussion collection, private finding history and a generic isolated container executor are implemented. All 55 deterministic tests pass. A real L1 run executed both boundary tests, made zero model calls and removed its container (run `a93b3b57-6ba7-4eff-9e6d-8a27c308754e`, source `36d2eb43ed7b3de1a47c52cbebd9476c6a5dbf06`). See the [execution guide](execution-guide.md). Full stage acceptance still requires the combined semantic demonstration. + +**Remaining acceptance work in Stage 1:** automatic decision/conversation collection (G02), stable re-review identity/history (G03), fuller approach/alternative verification (G04), adapter conformance completion (G15), generic repository execution (G16), and complete example/skill migration (G20). ### Stage 1 exit diff --git a/review/checks.mjs b/review/checks.mjs index 9a2d1d0..db0af35 100644 --- a/review/checks.mjs +++ b/review/checks.mjs @@ -5,10 +5,10 @@ export function validateChecks(checks) { for (const check of checks) { if (!/^[a-z][a-z0-9-]{0,63}$/.test(check.id) || ids.has(check.id)) throw new Error('Check IDs must be unique safe names'); ids.add(check.id); - if (!['test', 'scenario'].includes(check.kind)) throw new Error('Check kind must be test or scenario'); + if (!['test', 'scenario', 'command'].includes(check.kind)) throw new Error('Check kind must be test, scenario or command'); if (!Array.isArray(check.argv) || !check.argv.length || check.argv.some(x => typeof x !== 'string' || !x || x.includes('\0'))) throw new Error('Check argv must contain nonempty strings'); if (!Number.isInteger(check.timeoutSeconds) || check.timeoutSeconds < 1 || check.timeoutSeconds > 3600) throw new Error('Check timeout must be 1..3600 seconds'); - if (!Array.isArray(check.reports) || !check.reports.length) throw new Error('Check needs JUnit XML report patterns'); + if (!Array.isArray(check.reports) || (check.kind !== 'command' && !check.reports.length)) throw new Error('Check needs JUnit XML report patterns'); for (const path of check.reports) sourcePath(path); if (check.cwd) sourcePath(check.cwd); } diff --git a/review/cli.mjs b/review/cli.mjs index 0663c16..17c55e2 100755 --- a/review/cli.mjs +++ b/review/cli.mjs @@ -24,7 +24,8 @@ const help = `code-qa v0.4 Options: --application pr|targeted --question TEXT --lens TEXT --path PATH (repeatable) --context FILE (repeatable) - --requirements FILE --config FILE --environment none|openelis + --requirements FILE --config FILE --environment none|openelis|container --image IMAGE + --decisions FILE --history FILE --profile development --adapter codex --model MODEL_ID --billing-mode subscription|api|unknown --review-seconds N --reasoning-effort low|medium|high --startup-seconds N --checks FILE @@ -47,7 +48,7 @@ try { out:{type:'string'}, plan:{type:'string'}, run:{type:'string'}, level:{type:'string'}, lens:{type:'string'}, application:{type:'string'}, question:{type:'string'}, purpose:{type:'string'}, requirements:{type:'string'}, path:{type:'string',multiple:true}, context:{type:'string',multiple:true}, - config:{type:'string'}, checks:{type:'string'}, environment:{type:'string'}, rigor:{type:'string'}, + image:{type:'string'},decisions:{type:'string'},history:{type:'string'},config:{type:'string'}, checks:{type:'string'}, environment:{type:'string'}, rigor:{type:'string'}, profile:{type:'string'}, adapter:{type:'string'}, model:{type:'string'}, 'billing-mode':{type:'string'}, 'reasoning-effort':{type:'string'}, 'review-seconds':{type:'string'},'startup-seconds':{type:'string'}, }}); @@ -68,6 +69,7 @@ try { const baseRequirements = await configurationRequirements(snap,config); const requirements = [baseRequirements, v.requirements ? await readFile(v.requirements,'utf8') : ''].filter(Boolean).join('\n\n'); plan = await createPlan({...source,...neighbors,application:v.application,question:v.question,purpose:v.purpose, + decisions:v.decisions ? JSON.parse(await readFile(v.decisions,'utf8')) : config.decisions ?? [], history:v.history ? JSON.parse(await readFile(v.history,'utf8')) : null,image:v.image ?? config.image, requirements,paths:v.path,environment:v.environment ?? config.environment,level:v.level,rigor:v.rigor,lens:v.lens, profile:v.profile ?? config.profile,adapter:v.adapter ?? (resolveLevel(v).level === 'L1' ? undefined : config.adapter),model:v.model ?? (resolveLevel(v).level === 'L1' ? undefined : config.model),billingMode:v['billing-mode'],reasoningEffort:v['reasoning-effort'], checks, ...(v['review-seconds'] ? {reviewSeconds:Number(v['review-seconds'])}:{}), diff --git a/review/decisions.mjs b/review/decisions.mjs new file mode 100644 index 0000000..a93d89e --- /dev/null +++ b/review/decisions.mjs @@ -0,0 +1,20 @@ +import { SHA } from './snapshot.mjs'; + +// Only operator inputs can designate accepted intent. Contributor prose is context, +// even when GitHub labels its author OWNER/MEMBER; association is not approval. +export function validateDecisions(decisions) { + if (!Array.isArray(decisions)) throw new Error('decisions must be an array'); + const ids = new Set(); + for (const d of decisions) { + if (!d.id || ids.has(d.id)) throw new Error('Decision IDs must be unique'); + ids.add(d.id); + for (const key of ['source', 'author', 'authority', 'retrievedAt', 'statement', 'previousExpectation', 'replacementExpectation']) + if (typeof d[key] !== 'string') throw new Error('Missing decision provenance: ' + key); + if (!d.source.trim() || !d.author.trim() || !d.statement.trim() || !SHA.test(d.revision) || !Number.isFinite(Date.parse(d.retrievedAt))) throw new Error('Invalid decision provenance'); + if (!['accepted', 'proposed', 'conflicting', 'unavailable'].includes(d.status)) throw new Error('Invalid decision status'); + if (d.status === 'accepted' && !d.authority.trim()) throw new Error('Accepted decision needs explicit authority'); + } + return decisions; +} +export const decisionGaps = decisions => decisions.filter(d => ['conflicting', 'unavailable'].includes(d.status)) + .map(d => 'Decision ' + d.id + ' is ' + d.status + ': ' + d.statement); diff --git a/review/environments/container.mjs b/review/environments/container.mjs new file mode 100644 index 0000000..d7fec88 --- /dev/null +++ b/review/environments/container.mjs @@ -0,0 +1,53 @@ +import { readFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { command, requireSuccess } from '../process.mjs'; +import { workerName } from './openelis.mjs'; + +// Repository code runs without host mounts, network, credentials, Docker socket, +// root privileges or writable root filesystem. Only this run's tmpfs is writable. +export class ContainerEnvironment { + constructor(plan, directory, execute = command) { + this.plan = plan; this.directory = directory; this.execute = execute; + this.name = workerName(plan.id); this.owned = false; this.sequence = 0; + } + async call(label, argv, options = {}) { + return requireSuccess(await this.execute(argv, { log: join(this.directory, (++this.sequence) + '-' + label + '.log'), ...options }), label); + } + async start(signal) { + const image = await this.call('image', ['docker','image','inspect',this.plan.image,'--format','{{.Id}}'], {signal}); + this.imageId = image.stdout.trim(); + if (!/^sha256:[0-9a-f]{64}$/.test(this.imageId)) throw new Error('Invalid image identity'); + await this.call('create', ['docker','create','--name',this.name,'--label','code-qa.run=' + this.plan.id, + '--network','none','--read-only','--cap-drop=ALL','--security-opt','no-new-privileges', + '--pids-limit','128','--memory','2g','--cpus','2','--user','1000:1000', + '--tmpfs','/workspace:rw,exec,uid=1000,gid=1000,mode=0700', + '--tmpfs','/tmp:rw,exec,uid=1000,gid=1000,mode=0700','--workdir','/workspace',this.imageId,'sleep','infinity'], {signal}); + this.owned = true; + await this.call('start', ['docker','start',this.name], {signal}); + const archive = join(this.directory,'source.tar'); + await this.call('archive', ['git','-C',this.plan.snapshot.repo,'archive','--format=tar','--output='+archive,this.plan.snapshot.head], {signal}); + await this.call('extract', ['docker','exec','-i',this.name,'tar','--no-same-owner','-xf','-','-C','/workspace'], {signal,input:await readFile(archive)}); + } + async verify(signal) { + const result = await this.call('identity', ['docker','inspect',this.name,'--format','{{.Image}} {{.State.Running}}'], {signal}); + const passed = result.stdout.trim() === this.imageId + ' true'; + return {id:'isolated-executor',head:this.plan.snapshot.head,status:passed?'pass':'fail',executed:1,measure:'assertions',assertions:[{name:'selected execution image is running',passed}],image:this.imageId}; + } + async runCheck(check, signal) { + const script = await readFile(new URL('./openelis-check.py', import.meta.url),'utf8'); + const payload = Buffer.from(JSON.stringify({check,head:this.plan.snapshot.head,root:'/workspace',verifyGit:false,devStack:false})).toString('base64'); + const result = await this.call('check-'+check.id,['docker','exec','-i',this.name,'python3','-',payload], + {input:script,signal,timeoutMs:(check.timeoutSeconds+30)*1000,maxBytes:16*1024*1024}); + return JSON.parse(result.stdout.trim()); + } + async collect() { + if (this.owned) await this.call('identity-evidence',['docker','inspect',this.name,'--format','{{json .State}} {{json .HostConfig}}']); + } + async cleanup() { + if (!this.owned) return 'not-created'; + const label = await this.call('ownership',['docker','inspect',this.name,'--format','{{index .Config.Labels "code-qa.run"}}']); + if (label.stdout.trim() !== this.plan.id) throw new Error('Container ownership changed'); + await this.call('delete',['docker','rm','-f',this.name]); + return 'removed'; + } +} diff --git a/review/environments/openelis-check.py b/review/environments/openelis-check.py index 07e61da..bb77c25 100644 --- a/review/environments/openelis-check.py +++ b/review/environments/openelis-check.py @@ -10,7 +10,7 @@ import sys import xml.etree.ElementTree as ET -def execute_check(spec, root, head): +def execute_check(spec, root, head, dev_stack=True): root = root.resolve() def owned(path): resolved = Path(path).resolve() @@ -26,7 +26,7 @@ def reports(): path.unlink() env = os.environ.copy() launcher = root / "scripts/dev-stack" - if launcher.exists(): + if dev_stack and launcher.exists(): result = subprocess.run([str(launcher), "env"], cwd=root, capture_output=True, text=True, check=True) for line in result.stdout.splitlines(): # dev-stack emits shell-quoted `export NAME=value`; parse data, never eval it. @@ -37,7 +37,7 @@ def reports(): key, sep, value = words[0].partition("=") if sep and key.replace("_", "").isalnum(): env[key] = value - log = root.parent / ("check-" + spec["id"] + ".log") + log = Path("/tmp") / ("check-" + spec["id"] + ".log") timed_out = False with log.open("w") as output: process = subprocess.Popen(spec["argv"], cwd=cwd, env=env, @@ -55,6 +55,11 @@ def reports(): code = process.returncode if timed_out: raise ValueError("Check timed out; behavior unverified") + if spec["kind"] == "command": + return {"id": spec["id"], "kind": "command", "head": head, "executed": 1, + "measure": "commands", "exitCode": code, "status": "pass" if code == 0 else "fail", + "assertions": [{"name": "command exit status", "passed": code == 0}], + "logTail": log.read_text(errors="replace")[-4000:], "argv": spec["argv"]} assertions, skipped = [], 0 found = reports() for report in found: @@ -79,14 +84,14 @@ def reports(): if code != 0 and all(a["passed"] for a in assertions): raise ValueError("Command failed outside reported assertions; exit=" + str(code) + "\n" + tail) return {"id": spec["id"], "kind": spec["kind"], "head": head, "executed": len(assertions), - "skipped": skipped, "exitCode": code, "assertions": assertions, + "measure": "test cases", "skipped": skipped, "exitCode": code, "assertions": assertions, "status": "pass" if code == 0 and all(a["passed"] for a in assertions) else "fail", "logTail": tail, "argv": spec["argv"]} if __name__ == "__main__": config = json.loads(base64.b64decode(sys.argv[1])) - root = Path.home() / "review/source" - head = subprocess.check_output(["git", "rev-parse", "HEAD"], cwd=root, text=True).strip() + root = Path(config.get("root", str(Path.home() / "review/source"))) + head = subprocess.check_output(["git", "rev-parse", "HEAD"], cwd=root, text=True).strip() if config.get("verifyGit", True) else config["head"] if head != config["head"]: raise ValueError("Worker revision changed") - print(json.dumps(execute_check(config["check"], root, head))) + print(json.dumps(execute_check(config["check"], root, head, config.get("devStack", True)))) diff --git a/review/github.mjs b/review/github.mjs index b29b7ad..a1ca203 100644 --- a/review/github.mjs +++ b/review/github.mjs @@ -15,6 +15,34 @@ export async function readPR(identity, execute = command) { 'number,url,title,body,baseRefName,baseRefOid,headRefName,headRefOid,state'], execute)); return { ...identity, ...info }; } +export async function collectConversation(identity, execute = command) { + const [owner, name] = identity.repository.split('/'); + const query = `query($owner:String!,$name:String!,$number:Int!,$cursor:String) { + repository(owner:$owner,name:$name) { pullRequest(number:$number) { + reviewThreads(first:100,after:$cursor) { pageInfo { hasNextPage endCursor } + nodes { id isResolved isOutdated path comments(first:100) { + pageInfo { hasNextPage } nodes { id url body createdAt author { login } authorAssociation originalCommit { oid } commit { oid } } + } } + } + } } + }`; + const threads = [], gaps = []; + let cursor; + do { + const args = ['api', 'graphql', '-f', 'query=' + query, '-f', 'owner=' + owner, '-f', 'name=' + name, '-F', 'number=' + identity.number]; + if (cursor) args.push('-f', 'cursor=' + cursor); + const response = JSON.parse(await gh(args, execute)); + if (response.errors) throw new Error('GitHub conversation query failed'); + const page = response.data.repository.pullRequest.reviewThreads; + for (const thread of page.nodes) { + if (thread.comments.pageInfo.hasNextPage) gaps.push('Thread ' + thread.id + ' has additional unavailable comments'); + threads.push(thread); + } + cursor = page.pageInfo.hasNextPage ? page.pageInfo.endCursor : null; + } while (cursor); + const discussion = JSON.parse(await gh(['pr', 'view', String(identity.number), '--repo', identity.repository, '--json', 'comments,reviews,closingIssuesReferences'], execute)); + return { retrievedAt: new Date().toISOString(), authority: 'context-only; explicit operator decisions determine accepted intent', threads, ...discussion, gaps }; +} export async function collectPR(url, repoPath, execute = command) { const identity = parsePR(url), info = await readPR(identity, execute); if (info.state !== 'OPEN') throw new Error('PR is not open; use explicit base/head for a historical review'); @@ -33,7 +61,10 @@ export async function collectPR(url, repoPath, execute = command) { if (![0,1,8].includes(result.code)) requireSuccess(result, 'Collect PR checks'); checks = JSON.parse(result.stdout); } catch (error) { checksWarning = error.message; } - return { repo, base: info.baseRefOid, head: info.headRefOid, pullRequest: { ...info, observedAt: new Date().toISOString(), checks, checksWarning } }; + let conversation; + try { conversation = await collectConversation(identity, execute); } + catch (error) { conversation = { threads: [], gaps: ['Conversation unavailable: ' + error.message], retrievedAt: new Date().toISOString() }; } + return { repo, base: info.baseRefOid, head: info.headRefOid, pullRequest: { ...info, conversation, observedAt: new Date().toISOString(), checks, checksWarning } }; } export async function isPRFresh(plan, execute = command) { if (!plan.pullRequest) return true; diff --git a/review/history.mjs b/review/history.mjs new file mode 100644 index 0000000..3827b5e --- /dev/null +++ b/review/history.mjs @@ -0,0 +1,41 @@ +import { createHash } from 'node:crypto'; +import { SHA } from './snapshot.mjs'; + +// IDs are private report identities, never application acceptance gates. +export function identifyFindings(report, repository, head) { + return report.findings.map(f => ({ ...f, id: createHash('sha256').update(JSON.stringify([ + repository, f.owner, f.title, f.evidence.map(e => [e.kind, e.path, e.checkId]), + ])).digest('hex').slice(0, 24), originalHead: head })); +} +export function validateHistory(history) { + if (history === null) return null; + if (history.schemaVersion !== 1 || !history.repository || !SHA.test(history.head) || !Array.isArray(history.findings)) throw new Error('Invalid prior review history'); + const ids = new Set(); + for (const f of history.findings) { + if (!/^[a-f0-9]{24}$/.test(f.id) || ids.has(f.id) || !SHA.test(f.originalHead) || !f.title || !f.evidence?.length) throw new Error('Invalid prior finding'); + ids.add(f.id); + } + return history; +} +export function reconcileHistory(prior, report, current, repository, head) { + const dispositions = report.priorFindings ?? []; + if (!prior) { + if (dispositions.length) throw new Error('Disposition refers to absent history'); + return { schemaVersion: 1, repository, head, findings: current, rounds: [] }; + } + if (prior.repository !== repository) throw new Error('History repository mismatch'); + const seen = new Set(); + const rounds = dispositions.map(d => { + const original = prior.findings.find(f => f.id === d.id); + if (!original || seen.has(d.id) || !d.reason.trim()) throw new Error('Invalid prior finding disposition'); + seen.add(d.id); + if (!['still-present', 'resolved', 'superseded', 'declined', 'unverified'].includes(d.status)) throw new Error('Invalid prior finding state'); + if (d.status !== 'unverified' && !d.evidence.length) throw new Error('Re-review needs current evidence'); + if (d.status === 'still-present' && !current.some(f => f.id === d.currentFindingId)) throw new Error('Still-present item must link a current finding'); + return { ...d, head, originalHead: original.originalHead, originalEvidence: original.evidence }; + }); + for (const old of prior.findings) if (!seen.has(old.id)) rounds.push({ id: old.id, status: 'unverified', reason: 'No current assessment', head, originalHead: old.originalHead, originalEvidence: old.evidence, evidence: [] }); + // Preserve private history including resolved findings. Publication consumes only + // current actionable findings and can consult round status to avoid repetition. + return { schemaVersion: 1, repository, head, findings: [...prior.findings, ...current.filter(f => !prior.findings.some(p => p.id === f.id))], rounds: [...(prior.rounds ?? []), ...rounds] }; +} diff --git a/review/plan.mjs b/review/plan.mjs index 49fb285..dee4e38 100644 --- a/review/plan.mjs +++ b/review/plan.mjs @@ -1,3 +1,5 @@ +import { validateDecisions } from './decisions.mjs'; +import { validateHistory } from './history.mjs'; import { randomUUID } from 'node:crypto'; import { resolve } from 'node:path'; import { snapshot, sourcePath, SHA } from './snapshot.mjs'; @@ -16,7 +18,7 @@ export function validatePlan(plan) { if (!['pr', 'targeted'].includes(plan.application) || (plan.application === 'targeted' && !plan.question?.trim())) throw new Error('Targeted review needs a question'); if (!['mechanical', 'standard'].includes(plan.rigor)) throw new Error('Unknown rigor; use --level L1..L5'); if (plan.rigor !== (plan.level === 'L1' ? 'mechanical' : 'standard')) throw new Error('Mechanical and AI level mismatch'); - if (!['none', 'openelis'].includes(plan.environment)) throw new Error('Unknown environment'); + if (!['none', 'openelis', 'container'].includes(plan.environment)) throw new Error('Unknown environment'); if (plan.level === 'L1' ? plan.adapter !== 'none' : plan.adapter === 'none') throw new Error('L1 must use no AI; other levels require a reviewer'); if (plan.adapter !== 'none') reviewerAdapter(plan.adapter).validate(plan); if (!['subscription', 'api', 'unknown'].includes(plan.billingMode)) throw new Error('Unknown billing mode'); @@ -31,8 +33,11 @@ export function validatePlan(plan) { for (const path of [...plan.paths, ...plan.contextPaths]) sourcePath(path); if (typeof plan.snapshot.repo !== 'string' || !plan.snapshot.repo.startsWith('/')) throw new Error('Repository must be absolute'); if (typeof plan.purpose !== 'string' || typeof plan.requirements !== 'string') throw new Error('Invalid purpose/requirements'); + validateDecisions(plan.decisions ?? []); + validateHistory(plan.history ?? null); + if (plan.environment === 'container' && (typeof plan.image !== 'string' || !/^[a-zA-Z0-9][a-zA-Z0-9._/@:-]+$/.test(plan.image))) throw new Error('Choose a trusted container image'); validateChecks(plan.checks); - if (plan.checks.length && plan.environment !== 'openelis') throw new Error('Executable checks require the isolated OpenELIS environment'); + if (plan.checks.length && plan.environment === 'none') throw new Error('Executable checks require an isolated environment'); if (plan.environment === 'openelis' && plan.pullRequest && plan.pullRequest.repository !== 'DIGI-UW/OpenELIS-Global-2') throw new Error('OpenELIS environment only supports DIGI-UW/OpenELIS-Global-2'); return plan; } @@ -52,7 +57,8 @@ export async function createPlan(options) { rigor: ai ? 'standard' : 'mechanical', adapter: options.adapter ?? (ai ? 'codex' : 'none'), environment: options.environment ?? 'none', model: options.model ?? (ai ? defaults.model : ''), billingMode: options.billingMode ?? 'unknown', maxSessions: ai ? 1 : 0, reviewSeconds: options.reviewSeconds ?? (ai ? defaults.reviewSeconds : 0), startupSeconds: options.startupSeconds ?? 3600, - checks: options.checks ?? [], + checks: options.checks ?? [], image: options.image ?? 'node:22-bookworm', + decisions: options.decisions ?? [], history: options.history ?? null, contextNotes: options.contextNotes ?? [], pullRequest: options.pullRequest ?? null, overrides: Object.fromEntries(['model','reviewSeconds','startupSeconds','reasoningEffort'] .filter(key => options[key] !== undefined).map(key => [key, options[key]])), diff --git a/review/report.mjs b/review/report.mjs index c6d3e85..3715684 100644 --- a/review/report.mjs +++ b/review/report.mjs @@ -37,6 +37,18 @@ export const reviewerSchema = { }, required: ['schemaVersion', 'head', 'summary', 'coverage', 'omissions', 'assessments', 'questions', 'findings'], }; +const evidenceSchema = reviewerSchema.properties.findings.items.properties.evidence; +reviewerSchema.properties.priorFindings = { type: 'array', items: { + type: 'object', additionalProperties: false, + properties: { id: {type:'string'}, currentFindingId: {type:'string'}, status: {type:'string',enum:['still-present','resolved','superseded','declined','unverified']}, reason: {type:'string'}, evidence: evidenceSchema }, + required: ['id','currentFindingId','status','reason','evidence'], +} }; +reviewerSchema.properties.findings.items.properties.approach = { + type:'object', additionalProperties:false, + properties: Object.fromEntries(['problem','reachablePath','simplerAlternative','alternativeEvidence'].map(k => [k,{type:'string'}])), + required:['problem','reachablePath','simplerAlternative','alternativeEvidence'], +}; + // The CLI's schema request is not validation. Reject malformed or incomplete reports locally. export function validateSchema(value, schema, label = 'report') { if (schema.type === 'object') { @@ -65,6 +77,7 @@ export function validateReviewer(report, packet, checks) { } for (const question of report.questions) if (!question.question.trim() || !question.neededEvidence.trim()) throw new Error('Empty unresolved question/evidence request'); for (const finding of report.findings) { + if (finding.approach && Object.values(finding.approach).some(v => !v.trim())) throw new Error('Approach objection needs problem, reachable path and supported alternative'); for (const name of ['title', 'consequence', 'owner', 'counterevidence']) if (!finding[name].trim()) throw new Error(`Empty finding ${name}`); for (const evidence of finding.evidence) { if (!evidence.quote.trim()) throw new Error('Empty evidence quotation'); @@ -99,7 +112,7 @@ export function renderReport(run) { `Level: **${run.plan.level}**. Evidence contract: ${run.plan.presetVersion}. Execution profile: ${run.plan.profile}. Model: ${run.plan.model || 'none'}.`, `Revision: ${run.plan.snapshot.head}`, `Application: ${run.plan.application}${run.plan.question ? ' — ' + run.plan.question : ''}`, `Environment: ${run.plan.environment}. Reviewer: ${run.plan.adapter}.`, '', '## Required evidence', '', ...run.plan.requiredEvidence.map(x => '- ' + x), '', '## Checks', '']; - for (const check of run.checks) lines.push(`- ${check.id}: ${check.status}${check.executed !== undefined ? ` (${check.executed} assertions)` : ''}`); + for (const check of run.checks) lines.push(`- ${check.id}: ${check.status}${check.executed !== undefined ? ` (${check.executed} ${check.measure ?? 'assertions'})` : ''}`); if (run.reviewer) { lines.push('', '## Reviewer assessment', '', run.reviewer.summary); lines.push('', '## Coverage', '', ...run.reviewer.assessments.map(a => '- ' + a.area + ': ' + a.status + ' — ' + a.reason)); diff --git a/review/reviewers/codex.mjs b/review/reviewers/codex.mjs index 62a54bf..073beee 100644 --- a/review/reviewers/codex.mjs +++ b/review/reviewers/codex.mjs @@ -28,7 +28,7 @@ export async function reviewWithCodex(plan, packet, checks, directory, signal, e const compact = { ...packet, snapshot: { head: packet.snapshot.head, base: packet.snapshot.base, mergeBase: packet.snapshot.mergeBase }, level: plan.level, lens: plan.lens, verification: plan.checks.map(c => ({ id: c.id, kind: c.kind })) }; - const instruction = `Answer the selected question only. Start with the selected diff, then inspect relevant unchanged callers, consumers, tests and configuration as needed. You have ${plan.reviewSeconds} seconds in one session; stay focused and do not exhaustively dump the repository. There is no source-byte allowance. Explicitly excluded workflows or unavailable runtime tests do not make a source-only scope partial. Missing evidence necessary for the question does. Assess intent, ownership, consumers and tests explicitly. Name the changed behavior, suitable test layer and existing test or gap in the tests assessment. Missing necessary evidence belongs in questions with the experiment needed. Record counterevidence for candidate findings and an applicable simpler alternative for approach objections; suggestions and refuted candidates are not actionable defects. Accepted removals can delete obsolete tests. Challenge findings against supplied executed verification. Do not infer runtime verification from test source.`; + const instruction = `Answer the selected question only. Start with the selected diff, then inspect relevant unchanged callers, consumers, tests and configuration as needed. You have ${plan.reviewSeconds} seconds in one session; stay focused and do not exhaustively dump the repository. There is no source-byte allowance. Explicitly excluded workflows or unavailable runtime tests do not make a source-only scope partial. Missing evidence necessary for the question does. Assess intent, ownership, consumers and tests explicitly. Name the changed behavior, suitable test layer and existing test or gap in the tests assessment. Missing necessary evidence belongs in questions with the experiment needed. Record counterevidence for candidate findings and an applicable simpler alternative for approach objections; suggestions and refuted candidates are not actionable defects. Accepted removals can delete obsolete tests. The decisions array contains operator-designated intent; conversation association labels alone do not establish approval. Preserve previous/replacement expectations. Reassess each prior history finding against current source, returning priorFindings with current evidence and a disposition. Never infer resolved from a previous report. Consequential approach objections must populate approach with the intended problem, reachable path and simpler alternative with supporting evidence. Challenge findings against supplied executed verification. Do not infer runtime verification from test source.`; const access = `The full base/head source is available in a local bare Git repository at ./repository.git. Use the shell tool for read-only inspection. For example: git --git-dir=repository.git diff --no-ext-diff --no-textconv review-base review-head --stat git --git-dir=repository.git diff --no-ext-diff --no-textconv review-base review-head -- diff --git a/review/reviewers/index.mjs b/review/reviewers/index.mjs index fb63fc3..2cf7171 100644 --- a/review/reviewers/index.mjs +++ b/review/reviewers/index.mjs @@ -5,6 +5,7 @@ import { reviewWithCodex } from './codex.mjs'; const adapters = { codex: { evidence: ['source-review', 'behavior-assessment'], + capabilities: { source:true, images:false, checkRequests:false, structuredOutput:true, cancellation:true, usage:true, hardCurrencyLimit:false }, review: reviewWithCodex, validate(plan) { if (typeof plan.model !== 'string' || !plan.model.trim() || /[\x00-\x1f]/.test(plan.model)) throw new Error('Choose a nonempty model ID'); diff --git a/review/run.mjs b/review/run.mjs index c0f06b7..82e1047 100644 --- a/review/run.mjs +++ b/review/run.mjs @@ -1,3 +1,6 @@ +import { identifyFindings, reconcileHistory } from './history.mjs'; +import { decisionGaps } from './decisions.mjs'; +import { ContainerEnvironment } from './environments/container.mjs'; import { mkdir, writeFile } from 'node:fs/promises'; import { join } from 'node:path'; import { command, requireSuccess } from './process.mjs'; @@ -10,7 +13,7 @@ import { validateCheckResult } from './checks.mjs'; import { OpenELISEnvironment } from './environments/openelis.mjs'; export async function runReview(plan, outputRoot, { signal, reviewer, - environmentFactory = (p, d) => new OpenELISEnvironment(p, d), onProgress = () => {}, freshness = isPRFresh } = {}) { + environmentFactory = (p, d) => p.environment === 'container' ? new ContainerEnvironment(p, d) : new OpenELISEnvironment(p, d), onProgress = () => {}, freshness = isPRFresh } = {}) { validatePlan(plan); const directory = runDirectory(outputRoot, plan); await mkdir(outputRoot, { recursive: true, mode: 0o700 }); @@ -39,7 +42,7 @@ export async function runReview(plan, outputRoot, { signal, reviewer, plan.snapshot.mergeBase, plan.snapshot.head, '--', ...packet.selectedPaths], { signal, log: join(directory, 'whitespace.log') }); if (![0, 2].includes(diff.code) || diff.timedOut || diff.cancelled || diff.overflow || diff.spawnError) requireSuccess(diff, 'Whitespace check'); run.checks.push({ id: 'whitespace', status: diff.code === 0 ? 'pass' : 'fail', evidence: diff.stdout.trim(), paths: packet.selectedPaths }); - if (plan.environment === 'openelis') { + if (plan.environment !== 'none') { const environment = environmentFactory(plan, directory); try { await phase('environment-start'); @@ -69,7 +72,18 @@ export async function runReview(plan, outputRoot, { signal, reviewer, validateSchema(reviewed.report, reviewerSchema); await captureSourceEvidence(reviewed.report, packet); await writeFile(join(directory, 'packet.json'), JSON.stringify(packet, null, 2) + '\n', { mode: 0o600 }); + if (reviewed.report.priorFindings?.length) { + const evidenceReport = { ...reviewed.report, findings: reviewed.report.priorFindings.filter(d => d.evidence.length).map(d => ({ title: d.id, consequence: d.reason, owner: 'history', severity: 'low', disposition: 'suggestion', counterevidence: d.reason, alternative: '', evidence: d.evidence })) }; + await captureSourceEvidence(evidenceReport, packet); + validateReviewer(evidenceReport, packet, run.checks); + } run.reviewer = validateReviewer(reviewed.report, packet, run.checks); run.usage.reported = reviewed.usage; + const repository = plan.pullRequest?.repository ?? plan.snapshot.repo; + run.findings = identifyFindings(run.reviewer, repository, plan.snapshot.head); + run.history = reconcileHistory(plan.history, run.reviewer, run.findings, repository, plan.snapshot.head); + await writeFile(join(directory, 'history.json'), JSON.stringify(run.history, null, 2) + '\n', {mode:0o600}); + run.reasons.push(...run.history.rounds.filter(r => r.head === plan.snapshot.head && r.status === 'unverified').map(r => 'Prior finding unverified: ' + r.id)); + run.reasons.push(...decisionGaps(plan.decisions ?? []), ...(plan.pullRequest?.conversation?.gaps ?? [])); run.inspection = reviewed.inspection ?? null; run.adapterDiagnostics = reviewed.diagnostics ?? []; run.reasons.push(...coverageGaps(run.reviewer)); diff --git a/review/snapshot.mjs b/review/snapshot.mjs index 6e4ccfc..8189935 100644 --- a/review/snapshot.mjs +++ b/review/snapshot.mjs @@ -32,8 +32,8 @@ export async function packetFor(plan) { for (const path of [...changed, ...plan.contextPaths]) sourcePath(path); return { schemaVersion: 2, sourceAccess: 'repository', snapshot: snap, application: plan.application, question: plan.question, - purpose: plan.purpose, requirements: plan.requirements, selectedPaths: changed, - contextNotes: plan.contextNotes ?? [], pullRequest: plan.pullRequest ? { url: plan.pullRequest.url, title: plan.pullRequest.title, body: plan.pullRequest.body, checks: plan.pullRequest.checks, checksWarning: plan.pullRequest.checksWarning } : null, + decisions: plan.decisions ?? [], history: plan.history ?? null, purpose: plan.purpose, requirements: plan.requirements, selectedPaths: changed, + contextNotes: plan.contextNotes ?? [], pullRequest: plan.pullRequest ? { url: plan.pullRequest.url, title: plan.pullRequest.title, body: plan.pullRequest.body, checks: plan.pullRequest.checks, checksWarning: plan.pullRequest.checksWarning, conversation: plan.pullRequest.conversation } : null, contextPaths: plan.contextPaths, files: [], omissions: [] }; } diff --git a/tests/review/context-history.test.mjs b/tests/review/context-history.test.mjs new file mode 100644 index 0000000..47c15df --- /dev/null +++ b/tests/review/context-history.test.mjs @@ -0,0 +1,71 @@ +import test from 'node:test'; +import {mkdtemp,writeFile,rm} from 'node:fs/promises'; +import {tmpdir} from 'node:os'; +import {join} from 'node:path'; +import assert from 'node:assert/strict'; +import {validateDecisions,decisionGaps} from '../../review/decisions.mjs'; +import {identifyFindings,validateHistory,reconcileHistory} from '../../review/history.mjs'; +import {collectConversation} from '../../review/github.mjs'; +import {ContainerEnvironment} from '../../review/environments/container.mjs'; +import {reviewerAdapter} from '../../review/reviewers/index.mjs'; +import {cleanReport} from './fixtures.mjs'; +const head='a'.repeat(40), next='b'.repeat(40); +const finding={title:'Consumer reinterprets decimals',owner:'consumer',disposition:'actionable',evidence:[{kind:'source',path:'consumer.js',side:'head',line:1,quote:'parseFloat(value)',checkId:''}]}; +test('approved removal preserves old and replacement expectations; conflicts remain gaps',()=>{ + const d={id:'remove-duplicate',source:'operator:review',author:'maintainer',authority:'explicit accepted boundary',retrievedAt:new Date().toISOString(),revision:head,status:'accepted',statement:'Producer decides normalization once',previousExpectation:'Consumer parses commas',replacementExpectation:'Consumer accepts canonical decimals'}; + assert.equal(validateDecisions([d])[0].previousExpectation,'Consumer parses commas'); + assert.deepEqual(decisionGaps([d]),[]); + assert.equal(decisionGaps([{...d,status:'conflicting'}]).length,1); + assert.throws(()=>validateDecisions([{...d,authority:''}]),/authority/); +}); +test('re-review preserves original evidence and reports missing dispositions as unverified',()=>{ + const original=identifyFindings({findings:[finding]},'example/repo',head); + const history=validateHistory({schemaVersion:1,repository:'example/repo',head,findings:original,rounds:[]}); + const report=cleanReport(next); + assert.equal(reconcileHistory(history,report,[],'example/repo',next).rounds[0].status,'unverified'); + report.priorFindings=[{id:original[0].id,currentFindingId:'',status:'resolved',reason:'Removed interpretation at consumer boundary',evidence:[{...finding.evidence[0],quote:'save(value)'}]}]; + const reviewed=reconcileHistory(history,report,[],'example/repo',next); + assert.equal(reviewed.findings[0].originalHead,head); + assert.deepEqual(reviewed.rounds[0].originalEvidence,finding.evidence); + assert.equal(reviewed.rounds[0].head,next); + assert.throws(()=>reconcileHistory(history,report,[],'different/repo',next),/mismatch/); + report.priorFindings[0].evidence=[]; + assert.throws(()=>reconcileHistory(history,report,[],'example/repo',next),/current evidence/); +}); +test('finding identity survives line movement but never carries old evidence forward as current',()=>{ + const before=identifyFindings({findings:[finding]},'example/repo',head)[0]; + const after=identifyFindings({findings:[{...finding,evidence:[{...finding.evidence[0],line:80}]}]},'example/repo',next)[0]; + assert.equal(before.id,after.id);assert.equal(after.evidence[0].line,80); +}); +test('GitHub collection paginates and exposes truncated conversations without treating association as authority',async()=>{ + let pages=0; + const execute=async argv=>{ + if(argv[1]!=='api') return {code:0,stdout:JSON.stringify({comments:[],reviews:[],closingIssuesReferences:[]})}; + pages++; + const reviewThreads={nodes:[{id:'thread'+pages,comments:{pageInfo:{hasNextPage:pages===1},nodes:[]}}],pageInfo:{hasNextPage:pages===1,endCursor:'next'}}; + return {code:0,stdout:JSON.stringify({data:{repository:{pullRequest:{reviewThreads}}}})}; + }; + const c=await collectConversation({repository:'test/repo',number:1},execute); + assert.equal(pages,2);assert.equal(c.threads.length,2);assert.equal(c.gaps.length,1);assert.match(c.authority,/context-only/); +}); +test('generic executor command policy has no mounts, socket, network or root execution',async t=>{ + const directory=await mkdtemp(join(tmpdir(),'executor-policy-'));t.after(()=>rm(directory,{recursive:true,force:true})); + const calls=[];const image='sha256:'+'f'.repeat(64); + const plan={id:'12345678-1234-1234-1234-123456789012',image:'node:22-bookworm',snapshot:{repo:'/fixture',head}}; + const env=new ContainerEnvironment(plan,directory,async argv=>{calls.push(argv);if(argv.includes('archive'))await writeFile(join(directory,'source.tar'),'fixture');return {code:0,stdout:argv[1]==='image'?image:''};}); + await env.start(); + const create=calls.find(a=>a[1]==='create'); + assert.equal(create[create.indexOf('--network')+1],'none'); + assert.equal(create[create.indexOf('--user')+1],'1000:1000'); + assert.ok(create.includes('--read-only'));assert.ok(!create.includes('-v'));assert.ok(!create.includes('--mount')); + assert.ok(!calls.some(a=>a.join(' ').includes('docker.sock'))); +}); +test('reviewer conformance advertises actual capabilities independently of model choice',()=>{ + const adapter=reviewerAdapter('codex'); + assert.equal(typeof adapter.review,'function'); + adapter.validate({model:'operator-selected-model',reasoningEffort:'low'}); + assert.equal(adapter.capabilities.source,true); + assert.equal(adapter.capabilities.images,false); + assert.equal(adapter.capabilities.cancellation,true); + assert.equal(adapter.capabilities.hardCurrencyLimit,false); +});