feat(review): collect decisions, retain finding history and isolate repository checks

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.

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.

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).

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
 

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);
   }

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'])}:{}),

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);

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';
+  }
+}