From c0298471e359ecd9148243f8164d7015fa8b8f3f Mon Sep 17 00:00:00 2001 From: BakerSean168 Date: Sat, 3 Oct 2026 02:13:57 +0000 Subject: [PATCH] feat: route review children through logical model --- README.md | 7 ++++--- docs/architecture.md | 8 ++++---- docs/model-policy.md | 6 +++--- extension/acceptance-workflow.js | 3 ++- extension/review-workflow.js | 3 ++- package-lock.json | 8 ++++---- package.json | 2 +- skills/forgeflow/SKILL.md | 5 +++-- tests/acceptance-workflow.test.js | 3 ++- tests/architecture-boundary.test.js | 2 +- tests/review-workflow.test.js | 3 ++- 11 files changed, 28 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 846a8a4..20284ab 100644 --- a/README.md +++ b/README.md @@ -42,9 +42,10 @@ See [`docs/model-policy.md`](./docs/model-policy.md) for the logical-model polic ### `forgeflow.review` -Runs a fresh Pi reviewer, persists the full report under `.pi/subagents/`, and -uses a Pi typed gate to validate the reviewer's canonical final merge verdict. -Malformed or missing verdicts fail closed. +Runs a fresh Pi reviewer on the stable `forgeflow/reviewer` virtual model, +persists the full report under `.pi/subagents/`, and uses a Pi typed gate to +validate the reviewer's canonical final merge verdict. Malformed or missing +verdicts fail closed. ### `forgeflow.accept` diff --git a/docs/architecture.md b/docs/architecture.md index b885a8e..51230e7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -48,9 +48,9 @@ The preflight is deterministic and adds no model call or durable workflow state. ## Independent review -`forgeflow.review` launches the builtin Pi reviewer with fresh context. The -reviewer is read-only. Its full report is persisted to an absolute path under -`.pi/subagents/`. +`forgeflow.review` launches the builtin Pi reviewer agent with fresh context and +explicitly selects the stable `forgeflow/reviewer` virtual model. The reviewer is +read-only. Its full report is persisted to an absolute path under `.pi/subagents/`. A Pi typed gate executes `scripts/review-verdict.mjs` against that same report. Only Pi's canonical merge-verdict forms are accepted: @@ -65,7 +65,7 @@ Anything else fails closed. `forgeflow.accept` performs four independent gates in order: 1. **HEAD before review** — the worktree is clean and equals `expectedHead`. -2. **Fresh review** — the Pi reviewer must return a clean typed verdict. +2. **Fresh review** — the Pi reviewer running as `forgeflow/reviewer` must return a clean typed verdict. 3. **HEAD after review** — review must not have changed the candidate. 4. **GitHub exact-head** — the PR is open, targets the expected base, still points at `expectedHead`, and every configured required check resolves to diff --git a/docs/model-policy.md b/docs/model-policy.md index 04d281d..dc8e0b1 100644 --- a/docs/model-policy.md +++ b/docs/model-policy.md @@ -30,7 +30,7 @@ ForgeFlow registers these Pi virtual models: The names are stable contracts. The physical model behind each role is operator policy. -Once the `pi-subagents` child-runtime fixes described below are available in the pinned release, builtin roles can point at the logical models: +The pinned `pi-subagents@0.75.0` includes the child-runtime fixes required for Pi virtual models, so builtin roles can point directly at the logical models: ```json { @@ -45,9 +45,9 @@ Once the `pi-subagents` child-runtime fixes described below are available in the } ``` -The parent Pi session can select `forgeflow/planner` when the parent is acting as the planning/orchestration role. +The parent Pi session can select `forgeflow/planner` when the parent is acting as the planning/orchestration role. ForgeFlow's trusted `forgeflow.review` and `forgeflow.accept` workflows explicitly request `forgeflow/reviewer`, so the workflow owns the stable reviewer role while operator policy remains free to change the physical reviewer model without editing workflow code. -The currently pinned `pi-subagents@0.74.0` predates two upstream fixes needed to launch a Pi virtual model as a child: queued virtual-model registration in child runtimes and verification against the logical selection rather than the dispatched physical response model. Until those fixes are available in a released `pi-subagents` package, ForgeFlow's trusted `forgeflow.review` and `forgeflow.accept` workflows keep the existing builtin-reviewer model selection instead of forcing `forgeflow/reviewer`. This avoids a ForgeFlow compatibility shim. After the dependency is upgraded to a release containing upstream #2636 and #2638, the workflows can hard-cut over to `forgeflow/reviewer` and this compatibility note can be removed. +`pi-subagents@0.75.0` contains the upstream child-runtime fixes for queued virtual-model registration and logical-selection verification (#2636 and #2638). ForgeFlow therefore does not carry a compatibility shim for virtual child models. ## Policy file diff --git a/extension/acceptance-workflow.js b/extension/acceptance-workflow.js index 8174162..9bf8896 100644 --- a/extension/acceptance-workflow.js +++ b/extension/acceptance-workflow.js @@ -77,7 +77,7 @@ export function createAcceptanceWorkflowDefinition(repoRoot) { return { name: "forgeflow.accept", - version: 3, + version: 4, resolve(args) { const validated = validateArgs(args); if ("error" in validated) return validated; @@ -150,6 +150,7 @@ export function createAcceptanceWorkflowDefinition(repoRoot) { const review = await runs.run("forgeflow-final-review", { label: "Review exact ForgeFlow head", agent: "reviewer", + model: "forgeflow/reviewer", context: "fresh", agentContract: { version: 1 }, task: ${JSON.stringify(reviewTask)}, diff --git a/extension/review-workflow.js b/extension/review-workflow.js index 483d147..10c9b54 100644 --- a/extension/review-workflow.js +++ b/extension/review-workflow.js @@ -72,7 +72,7 @@ export function createReviewWorkflowDefinition(repoRoot) { return { name: "forgeflow.review", - version: 3, + version: 4, resolve(args) { const validated = validateTask(args); if ("error" in validated) return validated; @@ -100,6 +100,7 @@ export function createReviewWorkflowDefinition(repoRoot) { const review = await runs.run("forgeflow-review", { label: "Review ForgeFlow candidate", agent: "reviewer", + model: "forgeflow/reviewer", context: "fresh", agentContract: { version: 1 }, task: ${JSON.stringify(reviewTask)}, diff --git a/package-lock.json b/package-lock.json index b64591d..0004848 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "3.0.0-alpha.1", "license": "MIT", "dependencies": { - "pi-subagents": "0.74.0" + "pi-subagents": "0.75.0" }, "peerDependencies": { "@earendil-works/pi-agent-core": "*", @@ -58,9 +58,9 @@ } }, "node_modules/pi-subagents": { - "version": "0.74.0", - "resolved": "https://registry.npmjs.org/pi-subagents/-/pi-subagents-0.74.0.tgz", - "integrity": "sha512-7+67TCpQuYoW2kMu4Kmt4j90hiR8uX8ozg3F/eakApUU6PxK7NteO58ylOWPql7fh/uTIEfS00FT2LKVDTSicw==", + "version": "0.75.0", + "resolved": "https://registry.npmjs.org/pi-subagents/-/pi-subagents-0.75.0.tgz", + "integrity": "sha512-RO4DiTJM6pnK8y9PnD7Y6TLeiX2c8Kh6QhmkecFo+ou5qtnz5qiM+vUKru48UmJG1nB9eJ6rALd1M04uBOR4XQ==", "license": "MIT", "dependencies": { "acorn": "8.18.0", diff --git a/package.json b/package.json index bda197a..3dbdd33 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "agents" ], "dependencies": { - "pi-subagents": "0.74.0" + "pi-subagents": "0.75.0" }, "peerDependencies": { "@earendil-works/pi-agent-core": "*", diff --git a/skills/forgeflow/SKILL.md b/skills/forgeflow/SKILL.md index 7a712e0..3210691 100644 --- a/skills/forgeflow/SKILL.md +++ b/skills/forgeflow/SKILL.md @@ -35,8 +35,9 @@ inside ForgeFlow. ## Review Use the trusted `forgeflow.review` workflow when an independent review is -needed before final delivery. Pi's read-only reviewer writes its full evidence-backed -report to a managed artifact and must end with Pi's standard merge-verdict +needed before final delivery. The workflow selects the stable `forgeflow/reviewer` +virtual model; Pi's read-only reviewer writes its full evidence-backed report to +a managed artifact and must end with Pi's standard merge-verdict contract: `Merge verdict: BLOCK|OK|OK with notes`, either as the plain final line or the builtin reviewer's final Markdown list item. ForgeFlow deterministically validates only those canonical forms and returns the artifact path plus a `clean` diff --git a/tests/acceptance-workflow.test.js b/tests/acceptance-workflow.test.js index b3b1cc7..0afdc45 100644 --- a/tests/acceptance-workflow.test.js +++ b/tests/acceptance-workflow.test.js @@ -52,6 +52,7 @@ test("acceptance binds Pi typed reviewer gate and GitHub evidence to one exact c const definition = createAcceptanceWorkflowDefinition(ROOT); const result = definition.resolve(validArgs()); + assert.equal(definition.version, 4); assert.equal("error" in result, false); assert.equal(result.hostCommands.length, 3); assert.deepEqual( @@ -67,7 +68,7 @@ test("acceptance binds Pi typed reviewer gate and GitHub evidence to one exact c assert.match(result.script, /context: "fresh"/); assert.match(result.script, /agent: "reviewer"/); - assert.doesNotMatch(result.script, /model: "forgeflow\/reviewer"/); + assert.match(result.script, /model: "forgeflow\/reviewer"/); assert.match(result.script, /agentContract: \{ version: 1 \}/); assert.match(result.script, /outputMode: "file-only"/); assert.match(result.script, /standard merge verdict contract/); diff --git a/tests/architecture-boundary.test.js b/tests/architecture-boundary.test.js index 03ecb6c..2f07cd7 100644 --- a/tests/architecture-boundary.test.js +++ b/tests/architecture-boundary.test.js @@ -42,7 +42,7 @@ test("Pi package has one explicit runtime entrypoint and one execution dependenc assert.deepEqual(packageJson.pi.extensions, ["./extension/index.js"]); assert.deepEqual(packageJson.pi.skills, ["./skills"]); - assert.deepEqual(packageJson.dependencies, { "pi-subagents": "0.74.0" }); + assert.deepEqual(packageJson.dependencies, { "pi-subagents": "0.75.0" }); assert.equal(packageJson.peerDependencies["@earendil-works/pi-coding-agent"], ">=0.99.0"); assert.deepEqual( new Set(packageJson.files), diff --git a/tests/review-workflow.test.js b/tests/review-workflow.test.js index 839bff7..3125258 100644 --- a/tests/review-workflow.test.js +++ b/tests/review-workflow.test.js @@ -15,10 +15,11 @@ test("review resource persists reviewer prose and uses Pi typed gate for determi const definition = createReviewWorkflowDefinition(ROOT); const result = definition.resolve({ task: "Review the retry implementation" }); + assert.equal(definition.version, 4); assert.equal("error" in result, false); assert.equal(result.hostCommands, undefined); assert.match(result.script, /agent: "reviewer"/); - assert.doesNotMatch(result.script, /model: "forgeflow\/reviewer"/); + assert.match(result.script, /model: "forgeflow\/reviewer"/); assert.match(result.script, /context: "fresh"/); assert.match(result.script, /agentContract: \{ version: 1 \}/); assert.match(result.script, /outputMode: "file-only"/);