Skip to content

Role-binding layer: runner + prompts -> Actor/Critic (Task 13)#4

Merged
inhaq merged 1 commit into
mainfrom
task-13-role-binding
Jun 13, 2026
Merged

Role-binding layer: runner + prompts -> Actor/Critic (Task 13)#4
inhaq merged 1 commit into
mainfrom
task-13-role-binding

Conversation

@inhaq

@inhaq inhaq commented Jun 13, 2026

Copy link
Copy Markdown
Owner

This pull request was created by @kiro-agent on behalf of @inhaq 👻

Comment with /kiro fix to address specific feedback or /kiro all to address everything.
Learn about Kiro Web


Summary

Implements Milestone 2, Task 13 from .kiro/specs/loop-engine — the role-binding layer that turns a vendor-neutral AgentRunner plus role prompt templates into working Actor/Critic roles, wired through configuration. This is the seam that lets any backend (the existing CliRunner, the upcoming HttpRunner) drive the Milestone 1 loop with zero engine changes.

What's included

  • 13.1 Actor promptsdraftPlan / build (with prior-failure context fed forward) / degraded selfReview, plus the JSON wire contracts for plan and build output.
  • 13.2 Critic prompts — plan-review and task-review prompts that emit the rubric JSON the engine already validates; the engine's repair hint is forwarded into the prompt.
  • 13.3 Config wiring — runner profiles via LOOPWRIGHT_RUNNERS (JSON) and role bindings via LOOPWRIGHT_ACTOR_RUNNER / LOOPWRIGHT_CRITIC_RUNNER; createRoles() resolves them and fails fast on misconfiguration (naming available profile ids), with an optional per-role model override.

Design notes

  • RunnerActor parses structured plan/build output and retries once with a corrective nudge before throwing RunnerRoleError. The critic review and the actor's self-review return raw text so the engine keeps sole ownership of parsing + rubric enforcement.
  • Extracted a shared last-valid-JSON scanner (engine/jsonExtract.ts) and refactored criticParser onto it, so actor and critic outputs parse through one code path.
  • runnerFactory dispatches profile kind → runner; http throws a clear "not implemented yet (Task 14)" error rather than degrading silently. Added a deterministic MockRunner for tests.

Testing

  • npm run typecheck clean.
  • npm test: 78 passing (24 new across test/runnerRoles.test.ts and test/roleBindings.test.ts), covering parse/retry/quota handling, feedback-in-prompt, raw passthrough, factory dispatch, config parsing, binding errors, and two end-to-end runs that drive the real loop (plan review + per-task fix cycle) to a verified GREEN.

Not in scope

  • HttpRunner (Task 14) and the real end-to-end run against live backends (Task 15).

Summary by CodeRabbit

  • New Features

    • Added support for configurable runner profiles via environment variables, enabling multiple runner backends (CLI and mock types).
    • Implemented the role-binding layer to connect configured runners to actor and critic roles.
    • Added mock runner support for testing and dry runs.
  • Documentation

    • Updated task progress in Milestone 2: role-binding layer marked "in review" with next focus on HTTP runner implementation.
  • Tests

    • Added comprehensive test coverage for runner creation, role binding, and actor/critic integration.

Implements Milestone 2, Task 13 from the loop-engine spec: a vendor-neutral
seam that pairs any AgentRunner with role prompt templates to produce working
Actor/Critic roles, wired through configuration.

- prompts.ts: actor (draftPlan/build/selfReview) and critic (plan/task review)
  prompt templates with the rubric encoded, plus actor JSON wire contracts.
- runnerRoles.ts: RunnerActor parses structured plan/build output (retry-once
  then RunnerRoleError); RunnerCritic and actor selfReview pass raw text through
  so the engine owns parsing + rubric enforcement.
- jsonExtract.ts: shared last-valid-JSON scanner; criticParser refactored onto
  it so actor and critic outputs parse identically.
- runnerFactory.ts + mockRunner.ts: profile->runner dispatch (http deferred to
  Task 14 with a clear error) and a deterministic runner for tests.
- config.ts: runner profiles (LOOPWRIGHT_RUNNERS) + role bindings
  (LOOPWRIGHT_ACTOR_RUNNER/_CRITIC_RUNNER); roleBindings.createRoles resolves
  them and fails fast on misconfiguration, with per-role model override.

Adds 24 tests (78 total); typecheck clean.
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • '.*'

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7e6f4fda-8d18-421b-92e3-ef2320101f98

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch task-13-role-binding

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/adapters/prompts.ts`:
- Around line 169-202: The plan contract and schema disagree:
ACTOR_PLAN_CONTRACT requires each task to have at least one machine-checkable
verifyCommand, but TaskSpecSchema.verifyCommands currently defaults to [] (no
.min(1)), which lets runMechanicalGate([]) vacuously pass; fix by enforcing the
contract in the schema or aligning the contract to behavior. Update
TaskSpecSchema.verifyCommands to require at least one entry (e.g., use
z.array(...).min(1) or add an equivalent refinement/validation step that throws
when verifyCommands is empty) so TaskSpecSchema, ACTOR_PLAN_CONTRACT, and
runMechanicalGate behavior are consistent; alternatively if you prefer changing
the prompt, edit ACTOR_PLAN_CONTRACT to allow empty verifyCommands and document
that mechanical gate treats empty lists as skipped. Reference symbols to change:
TaskSpecSchema.verifyCommands, ACTOR_PLAN_CONTRACT, and consider
runMechanicalGate behavior if you opt to keep empty arrays.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7e15be86-eca6-4172-8079-843dc1608098

📥 Commits

Reviewing files that changed from the base of the PR and between 23814de and 08ca80e.

📒 Files selected for processing (12)
  • .kiro/specs/loop-engine/tasks.md
  • src/adapters/prompts.ts
  • src/adapters/roleBindings.ts
  • src/adapters/runnerRoles.ts
  • src/config.ts
  • src/engine/criticParser.ts
  • src/engine/jsonExtract.ts
  • src/runners/agentRunner.ts
  • src/runners/mockRunner.ts
  • src/runners/runnerFactory.ts
  • test/roleBindings.test.ts
  • test/runnerRoles.test.ts

Comment thread src/adapters/prompts.ts
Comment on lines +169 to +202
const ACTOR_PLAN_CONTRACT =
'Reply with ONLY a JSON object (no prose, no markdown fences) matching:\n' +
'{"tasks":[{"id":string,"title":string,"description":string,' +
'"acceptanceCriteria":[string,...],"verifyCommands":[string,...],' +
'"dependencies":[string,...]}],"notes":string}\n' +
"Rules: ids are unique and short; every task has at least one acceptance " +
"criterion AND at least one machine-checkable verifyCommand (build/test/lint) " +
"so its done-state is verifiable; dependencies reference other task ids only.";

const ACTOR_BUILD_CONTRACT =
'Reply with ONLY a JSON object (no prose, no markdown fences) matching:\n' +
'{"diff":string,"touchedFiles":[string,...],"summary":string}\n' +
"`diff` is a unified diff of your changes; `touchedFiles` lists the paths it " +
"changes; `summary` is one or two sentences. Do not include secrets.";

export const DEFAULT_ACTOR_PROMPTS: ActorPromptTemplates = {
system:
"You are the Actor in an actor-critic engineering loop: the high-volume " +
"worker that decomposes goals into verifiable tasks, builds each task, and " +
"fixes issues from feedback. You are vendor-neutral and reply with strict " +
"JSON when asked. Favor small, verifiable units of work.",

draftPlan(goal, blockers) {
const revision =
blockers && blockers.length > 0
? `\n\nThis is a REVISION. The critic blocked the previous plan; ` +
`resolve every blocker below:\n${renderFindings(blockers)}`
: "";
return (
`Decompose this goal into a plan of dependent tasks.\n\nGOAL: ${goal}` +
revision +
`\n\n${ACTOR_PLAN_CONTRACT}`
);
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify TaskSpecSchema validation requirements for verifyCommands

rg -A 10 'export const TaskSpecSchema' src/schemas/plan.ts

Repository: inhaq/loopwright

Length of output: 418


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find ActorPlanOutputSchema definition and how it validates TaskSpecSchema
rg -n --hidden --glob '!**/node_modules/**' "ActorPlanOutputSchema|TaskSpecSchema" src | head -n 200

# Show the ActorPlanOutputSchema block (up to ~80 lines around matches)
rg -n --hidden --glob '!**/node_modules/**' -C 5 "ActorPlanOutputSchema" src/schemas/plan.ts src/schemas/**/*.ts 2>/dev/null || true

# Show where the plan output is parsed/validated (zod .parse / .safeParse)
rg -n --hidden --glob '!**/node_modules/**' "\.parse\(|safeParse\(" src | rg -n "ActorPlanOutputSchema|plan|draftPlan|ActorPlan" | head -n 200

Repository: inhaq/loopwright

Length of output: 993


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Print ActorPlanOutputSchema definition
sed -n '1,140p' src/adapters/prompts.ts

# Print PlanSchema/related schemas in src/schemas/plan.ts
sed -n '1,120p' src/schemas/plan.ts

# Print relevant part of src/schemas/artifact.ts (imports TaskSpecSchema)
sed -n '1,120p' src/schemas/artifact.ts

# Print where ActorPlanOutputSchema and PlanSchema.parse are called
sed -n '1,160p' src/adapters/runnerRoles.ts

Repository: inhaq/loopwright

Length of output: 13510


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find where mechanicalGate is produced and where verifyCommands is used
rg -n --hidden --glob '!**/node_modules/**' "verifyCommands|mechanicalGate|testCommands" src | head -n 200

# Specifically look for iteration/execution of verifyCommands
rg -n --hidden --glob '!**/node_modules/**' "verifyCommands.*(map|forEach|for \\(|reduce|push|command)" src

# Show the most relevant implementation files around mechanical gate execution
rg -n --hidden --glob '!**/node_modules/**' "mechanicalGate" src | head -n 50

Repository: inhaq/loopwright

Length of output: 1895


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1,260p' src/engine/loop.ts

# Print mechanical gate implementation (file name guessed from import path)
ls -la src/engine | rg -n "mechanicalGate" || true
fd mechanicalGate src/engine -t f -x echo

# Show the mechanical gate runner file
MECH_FILE=$(fd mechanicalGate src/engine -t f | head -n 1)
echo "Using: $MECH_FILE"
sed -n '1,260p' "$MECH_FILE"

Repository: inhaq/loopwright

Length of output: 13303


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search for verifyCommands in critic-related files
rg -n --hidden --glob '!**/node_modules/**' "verifyCommands" src/adapters src/engine src/schemas | head -n 200

# Show critic prompt section for plan review (likely DEFAULT_CRITIC_PROMPTS draftPlan/review)
rg -n --hidden --glob '!**/node_modules/**' "critic.*plan|PlanSchema|plan draft|draft" src/adapters/prompts.ts src/adapters/runnerRoles.ts src/adapters | head -n 200

# If prompts.ts includes critic contract mention verifyCommands, print around it
rg -n --hidden --glob '!**/node_modules/**' "acceptanceCriteria|dependencies|verifyCommands" src/adapters/prompts.ts | head -n 200

Repository: inhaq/loopwright

Length of output: 3914


🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n --hidden --glob '!**/node_modules/**' "verifyCommands" src/adapters src/engine src/schemas | head -n 200

rg -n --hidden --glob '!**/node_modules/**' "critic.*plan|PlanSchema|plan draft|draftPlan|review.*plan" src/adapters/prompts.ts src/adapters/runnerRoles.ts src/adapters | head -n 200

rg -n --hidden --glob '!**/node_modules/**' "acceptanceCriteria|dependencies|verifyCommands" src/adapters/prompts.ts | head -n 200

Repository: inhaq/loopwright

Length of output: 3521


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect critic prompt template(s) in prompts.ts around where verifyCommands appears
sed -n '120,320p' src/adapters/prompts.ts

# Inspect critic schema (normalization + blockers/rubric enforcement)
sed -n '1,260p' src/schemas/critic.ts

# Inspect critic response parsing
rg -n "parseCriticResponse|REPAIR_HINT|criticParser" -S src/engine
sed -n '1,260p' src/engine/criticParser.ts

# Search for logic that checks missing verifyCommands explicitly
rg -n --hidden --glob '!**/node_modules/**' "verifyCommands.*(block|fail|reject)|verifyCommands.*min\\(|machine-checkable|DoD" src | head -n 200

Repository: inhaq/loopwright

Length of output: 12003


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the rest of runTask switch handling of review.kind, especially "unavailable"/"malformed"/"paused"
rg -n "switch \\(review\\.kind\\)" -n src/engine/loop.ts
sed -n '240,460p' src/engine/loop.ts

# Inspect plan review stage too (where critic.review kind "plan")
sed -n '300,420p' src/engine/loop.ts

Repository: inhaq/loopwright

Length of output: 9040


🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "UNVERIFIED|unverified|UNVERIFIED_BY_CRITIC" src/engine src/domain src | head -n 200
sed -n '1,260p' src/domain/stateMachine.ts

Repository: inhaq/loopwright

Length of output: 4963


🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "UNVERIFIED|unverified|UNVERIFIED_BY_CRITIC" src/engine src/domain src | head -n 200

sed -n '1,260p' src/domain/stateMachine.ts

Repository: inhaq/loopwright

Length of output: 4963


Fix contract/schema mismatch for verifyCommands in plan tasks.
ACTOR_PLAN_CONTRACT (src/adapters/prompts.ts) says every task must have at least one machine-checkable verifyCommand, but TaskSpecSchema.verifyCommands (src/schemas/plan.ts) allows [] (z.array(...).default([]) with no .min(1)). Since runMechanicalGate([]) returns { passed: true } (src/engine/mechanicalGate.ts), an empty verifyCommands makes the mechanical “done-state” vacuously pass and relies entirely on the critic’s semantic checks rather than validation.

Consider enforcing verifyCommands length in the schema (or adding a refinement where the plan is validated), or updating the prompt contract to match the actual behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/adapters/prompts.ts` around lines 169 - 202, The plan contract and schema
disagree: ACTOR_PLAN_CONTRACT requires each task to have at least one
machine-checkable verifyCommand, but TaskSpecSchema.verifyCommands currently
defaults to [] (no .min(1)), which lets runMechanicalGate([]) vacuously pass;
fix by enforcing the contract in the schema or aligning the contract to
behavior. Update TaskSpecSchema.verifyCommands to require at least one entry
(e.g., use z.array(...).min(1) or add an equivalent refinement/validation step
that throws when verifyCommands is empty) so TaskSpecSchema,
ACTOR_PLAN_CONTRACT, and runMechanicalGate behavior are consistent;
alternatively if you prefer changing the prompt, edit ACTOR_PLAN_CONTRACT to
allow empty verifyCommands and document that mechanical gate treats empty lists
as skipped. Reference symbols to change: TaskSpecSchema.verifyCommands,
ACTOR_PLAN_CONTRACT, and consider runMechanicalGate behavior if you opt to keep
empty arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant