Skip to content

Add autodesign: UI/UX harness auto-research loop - #2

Open
bukacdan wants to merge 28 commits into
mainfrom
autodesign-loop
Open

Add autodesign: UI/UX harness auto-research loop#2
bukacdan wants to merge 28 commits into
mainfrom
autodesign-loop

Conversation

@bukacdan

@bukacdan bukacdan commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

What this is

A script-driven research loop that iteratively improves a Pi harness config for building better-looking landing-page UIs. Each iteration builds one self-contained HTML page per train prompt (via the pi CLI), screenshots it in scrolled viewport segments, scores it with an Opus vision call against a fixed rubric, and mutates the harness config (schema-constrained) toward the best-scoring version. All state persists under runs/.

This wholesale-replaces the previous bhvr/introspection starter contents (kept only .git, prompts.json, and the design docs).

Architecture

Config genome (JSON) → deterministic resolver → pi-subprocess builder → scroll-segment screenshotter (Playwright) → Opus rubric evaluator → per-prompt pipeline (bounded concurrency) → aggregation → schema-constrained mutator (elitist hill-climbing) → run store → orchestrator + CLI.

  • The genome is the entire mutable search space: model + thinking level, tool set, system instructions, skills, and subagents (rendered as mandatory internal review passes — plain pi can't spawn subagents hermetically). The mutator's output is forced through the same zod schema, so it can never do freeform file edits or break version lineage.
  • Train/holdout discipline is structural: the loop optimizes only split=train prompts; holdout scoring never touches history or feeds the mutator.
  • Screenshots are per-viewport scroll segments (desktop + mobile), not full-page — long pages stay legible to the vision model.

Notable decisions

  • Reference comparison is deferred (TBD). The evaluator currently scores rubric-only; references are optional throughout. The reference generator (src/reference/build-reference.ts, bun run reference) is implemented and tested but intentionally unwired.
  • Builder runs pi hermetically (--no-session --no-extensions --no-context-files --no-prompt-templates) in an isolated scratch dir per prompt.

Commands

  • bun run loop --iterations N [--run-id X] [--concurrency 5] — run the research loop
  • bun run holdout --run-id X [--version V] — score a config on holdout prompts (report only)
  • bun run resume --run-id X — resume from the last completed iteration
  • bun run reference — (optional, TBD) build cached reference pages

Testing

31 tests pass (bun test), bunx tsc --noEmit clean. Tests use a stubbed pi (via PI_BIN) and fake LLM clients — no network — with real Playwright for screenshots.

A final whole-branch review (Opus) caught and we fixed two real defects:

  • Critical: referenceSegments threw on a missing runs/reference/ dir → silent all-zero loop on a fresh reference-free checkout. Now returns [].
  • Important: resume wasn't idempotent across a mid-iteration crash (duplicate history row + divergent config). Fixed via config-version pinning + idempotent history append.

Not yet done

  • Live smoke run against real pi + paid Opus API (needs ANTHROPIC_API_KEY) — left for a maintainer to trigger.
  • Cosmetic: a resumed iteration accumulates an orphan config version (best-tracking unaffected).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Introduced an automated UI/UX design workflow that builds landing-page candidates, captures segmented desktop/mobile screenshots, scores them with a fixed rubric, and iteratively improves configurations (including reference generation, train loop with resume, and holdout reporting).
    • Added Bun CLI commands (reference/loop/resume/holdout) with artifacts persisted under runs/, plus schema-validated prompts/configs and structured evaluation outputs.
  • Documentation

    • Updated README with setup, workflow/status, command usage, and testing notes; added auto-research design/spec and planning documentation.
  • Tests

    • Added comprehensive Bun test coverage for schemas, screenshot segmentation, build/evaluate pipeline, orchestration, run persistence, and reference caching.
  • Chores

    • Removed the previous template app scaffold and related tooling/config to streamline the project.

bukacdan and others added 21 commits July 18, 2026 15:52
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd nextConfigVersion zero-state

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reference-page comparison is deferred (TBD); the evaluator now scores
candidates on the rubric alone when no reference screenshots are available.
vs_reference/diff_dimensions become optional in EvalResult, evaluatePage
accepts referenceDesktopPngs as optional, the pipeline no longer fails when
no reference segments exist for a prompt, and the mutator tolerates a
missing vs_reference when building critique summaries.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wires runLoop/runHoldout and the CLI per task 14, but deviates from the
brief: reference-page comparison is deferred, so loop/resume/holdout no
longer call or import assertReferencesExist and run fine with an empty
reference dir. The `reference` subcommand stays wired to
buildReferenceSet so it remains runnable later.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
referenceSegments() threw ENOENT when runs/reference/ was absent, which
was silently caught by runPromptPipeline and turned every prompt into
eval_failed / overall 0 on a fresh checkout. Guard with existsSync and
return [] (already handled downstream as reference-free eval).

runLoop's resume path could duplicate a history row and re-evaluate a
different config than the crashed attempt if the process died between
appendHistory/saveConfig and saveSummary. Pin the eval config to an
existing iteration's config-version.txt when present, and skip
appendHistory when a row for that iteration already exists.

Also drops a dead .gitignore negation for the no-longer-required
runs/reference/ dir, and locks in the all-eval_failed aggregate() edge
case with a test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The project is replaced with a Bun-based autodesign workflow. New modules define validated harness configurations and prompts, deterministic Pi resolution, page building, Playwright screenshots, rubric-based LLM evaluation, reference generation, run persistence, aggregation, mutation, orchestration, and CLI commands. Documentation describes the architecture and operation. Tests cover the main components and end-to-end mocked iteration behavior.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change and is conventional in form.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch autodesign-loop
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch autodesign-loop

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

@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: 17

🤖 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 @.gitignore:
- Line 4: Update the .gitignore environment-file pattern to ignore .env and all
environment-specific variants such as .env.local and .env.development,
preventing files containing secrets like ANTHROPIC_API_KEY from being committed.

In `@docs/superpowers/plans/2026-07-18-ui-harness-autoresearch.md`:
- Line 7: Update the workflow documentation to treat reference artifacts as
optional: in docs/superpowers/plans/2026-07-18-ui-harness-autoresearch.md:7-7,
describe rubric-only scoring as the default and use reference comparison only
when cached artifacts are available; in
docs/superpowers/specs/2026-07-18-ui-harness-autoresearch-design.md:101-119,
make reference segments optional and remove the requirement that the loop refuse
to start without them.

In `@src/cli.ts`:
- Around line 14-26: Validate iterations, concurrency, and version at the CLI
boundary in src/cli.ts lines 14-26, rejecting non-finite, non-integer, or
out-of-range values before parsing results are used; ensure src/cli.ts lines
48-64 pass only validated numeric values to loop and holdout operations. In
src/util/concurrency.ts lines 1-5, update the limiter validation to throw unless
n is a positive safe integer.

In `@src/config/resolver.ts`:
- Around line 16-24: Remove the unsupported per-subagent tools field from the
configuration schema/genome and any related handling, since the pass-building
loop around sub.name and sub.system_instructions does not enforce it. Keep
internal passes governed by the global config.tools behavior.

In `@src/config/schema.ts`:
- Line 29: Update the skills validation in the config schema around SkillSchema
so arrays containing duplicate skill IDs are rejected before the mutated config
is accepted. Preserve the existing maximum of eight skills and validate
uniqueness using each skill’s ID.

In `@src/inner/build.ts`:
- Around line 24-36: Update the subprocess execution flow around Bun.spawn and
the Promise.all in runPromptPipeline so startup, stdout/stderr reads, and
proc.exited rejections are caught and converted into a per-prompt BuildResult
with status build_failed. Ensure the timeout is cleared when failures occur, and
keep successful subprocess results unchanged.
- Around line 24-39: Remove any existing output.html in workspaceDir before
invoking Bun.spawn for Pi, ensuring retries cannot reuse a stale candidate.
Anchor the change in the build flow surrounding proc creation and the existing
htmlPath construction, while preserving the current execution and logging
behavior.

In `@src/inner/pipeline.ts`:
- Around line 11-18: Update referenceSegments to escape promptId before
interpolating it into the filename-matching RegExp, using the escaped value
while preserving the existing segment sorting and path construction behavior.

In `@src/inner/screenshot.ts`:
- Around line 22-29: Update the screenshot segment positioning loop in the page
evaluation flow so capped segments are distributed across the full scrollable
page, including the footer, rather than sampling only the first MAX_SEGMENTS
viewports. Preserve single-segment behavior and keep the final segment
bottom-aligned by deriving each y position from its segment index and the total
segment count.
- Around line 19-21: Update the screenshot flow around browser.newPage and
page.goto to route requests and deny all non-local/external URLs, allowing only
the generated local file resources before navigation. Remove the MAX_SEGMENTS =
8 limitation and continue scrolling or segment capture until the full document
height is covered, including the final sections.

In `@src/llm.ts`:
- Around line 47-59: Wrap the messages.create call in the retry loop with
request-error handling so timeouts, 429 responses, and network failures consume
maxRetries instead of immediately escaping. Record the failure, apply a bounded
backoff between attempts, and throw or propagate the final request error after
retries are exhausted while preserving the existing validation retry behavior.

In `@src/orchestrator.ts`:
- Around line 61-90: The iteration flow around aggregate, appendHistory,
mutateConfig, saveConfig, and saveSummary is only partially idempotent: crashes
can cause reevaluation or a second mutation. Persist explicit iteration commit
state and proposal version, or use an atomic transaction, so resume detects and
reuses an existing iteration’s summary, history entry, and generated
configuration without rerunning evaluation or mutation, while keeping their
scores and versions consistent.

In `@src/outer/mutate.ts`:
- Around line 56-60: The returned proposal in the mutation flow must preserve
the existing model name. Update the object passed to HarnessConfigSchema.parse
in the mutate function to take model.name from opts.bestConfig while retaining
the proposed model.thinking_level and other mutation fields.

In `@src/prompts.ts`:
- Line 5: Constrain the prompt schema’s id field to a filesystem-safe slug
before it is used for workspace and artifact paths. Update the visible id
validation in the prompt schema to reject path separators, traversal segments,
and other unsafe characters while preserving non-empty valid identifiers.

In `@tests/build.test.ts`:
- Around line 17-36: Update the test setup around the PI_BIN assignments in the
success and failure tests to preserve the original process.env.PI_BIN value and
restore it in an afterEach hook. Ensure restoration also handles the variable
being initially undefined, preventing the stub from leaking into later tests.

In `@tests/orchestrator.test.ts`:
- Around line 75-91: Update the resume path in runLoop so an iteration with an
existing saved mutation/config is recognized as already mutated and does not
call mutateConfig or create another config version. Preserve the existing
history-row deduplication, reuse the previously saved proposal/config state, and
extend the test to assert config versions remain [0, 1] after resuming.

In `@tests/resolver.test.ts`:
- Around line 31-38: Strengthen the deterministic test around resolveHarness by
defining at least two skills and running equivalent configs with those skills in
opposite input orders. Assert that both resolutions produce identical piArgs and
materialized skill contents, while retaining the existing system prompt and
skill filename checks.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 9f382722-4e43-4611-a061-20bf7993311d

📥 Commits

Reviewing files that changed from the base of the PR and between a0d02af and ed8ea4b.

⛔ Files ignored due to path filters (4)
  • bun.lock is excluded by !**/*.lock
  • client/public/vite.svg is excluded by !**/*.svg
  • client/src/assets/beaver.svg is excluded by !**/*.svg
  • introspection/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (77)
  • .github/FUNDING.yml
  • .gitignore
  • .introspection/customer-support-agent.yaml
  • BHVR.md
  • CONTRIBUTING.md
  • LICENSE
  • README.md
  • client/.gitignore
  • client/README.md
  • client/eslint.config.js
  • client/index.html
  • client/package.json
  • client/src/App.css
  • client/src/App.tsx
  • client/src/index.css
  • client/src/main.tsx
  • client/src/vite-env.d.ts
  • client/tsconfig.app.json
  • client/tsconfig.json
  • client/tsconfig.node.json
  • client/vite.config.ts
  • docs/superpowers/plans/2026-07-18-ui-harness-autoresearch.md
  • docs/superpowers/specs/2026-07-18-ui-harness-autoresearch-design.md
  • introspection/.env.example
  • introspection/.githooks/pre-commit
  • introspection/.github/workflows/recipe-validation.yml
  • introspection/.gitignore
  • introspection/.pi/mcp.local.example.json
  • introspection/README.md
  • introspection/SYSTEM.md
  • introspection/agents/agent.yaml
  • introspection/agents/escalation.yaml
  • introspection/agents/responder.yaml
  • introspection/agents/triage.yaml
  • introspection/package.json
  • introspection/skills/ticket-triage/SKILL.md
  • package.json
  • server/.gitignore
  • server/README.md
  • server/package.json
  • server/src/index.ts
  • server/tsconfig.json
  • shared/package.json
  • shared/src/index.ts
  • shared/src/types/index.ts
  • shared/tsconfig.json
  • src/cli.ts
  • src/config/resolver.ts
  • src/config/schema.ts
  • src/eval/rubric.md
  • src/inner/build.ts
  • src/inner/evaluate.ts
  • src/inner/pipeline.ts
  • src/inner/screenshot.ts
  • src/llm.ts
  • src/orchestrator.ts
  • src/outer/aggregate.ts
  • src/outer/mutate.ts
  • src/prompts.ts
  • src/reference/build-reference.ts
  • src/store/run-store.ts
  • src/util/concurrency.ts
  • tests/aggregate.test.ts
  • tests/build.test.ts
  • tests/config-schema.test.ts
  • tests/evaluate.test.ts
  • tests/llm.test.ts
  • tests/mutate.test.ts
  • tests/orchestrator.test.ts
  • tests/pipeline.test.ts
  • tests/prompts.test.ts
  • tests/reference.test.ts
  • tests/resolver.test.ts
  • tests/run-store.test.ts
  • tests/screenshot.test.ts
  • tsconfig.json
  • turbo.json
💤 Files with no reviewable changes (42)
  • LICENSE
  • introspection/skills/ticket-triage/SKILL.md
  • .introspection/customer-support-agent.yaml
  • introspection/agents/triage.yaml
  • client/src/main.tsx
  • introspection/SYSTEM.md
  • client/src/index.css
  • client/package.json
  • CONTRIBUTING.md
  • BHVR.md
  • server/tsconfig.json
  • introspection/agents/responder.yaml
  • client/index.html
  • .github/FUNDING.yml
  • client/tsconfig.app.json
  • client/.gitignore
  • server/README.md
  • shared/src/types/index.ts
  • turbo.json
  • introspection/package.json
  • client/tsconfig.node.json
  • introspection/README.md
  • introspection/.pi/mcp.local.example.json
  • introspection/.gitignore
  • client/vite.config.ts
  • client/src/vite-env.d.ts
  • client/src/App.css
  • introspection/.github/workflows/recipe-validation.yml
  • client/README.md
  • shared/package.json
  • client/eslint.config.js
  • introspection/agents/agent.yaml
  • server/.gitignore
  • shared/src/index.ts
  • client/src/App.tsx
  • server/src/index.ts
  • client/tsconfig.json
  • server/package.json
  • introspection/.githooks/pre-commit
  • introspection/.env.example
  • shared/tsconfig.json
  • introspection/agents/escalation.yaml

Comment thread .gitignore
node_modules/
runs/
*.log
.env

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Ignore environment-specific secret files.

Only .env is excluded; .env.local, .env.development, etc. can be committed with ANTHROPIC_API_KEY.

Proposed fix
 .env
+.env.*
+!.env.example
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.env
.env
.env.*
!.env.example
🤖 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 @.gitignore at line 4, Update the .gitignore environment-file pattern to
ignore .env and all environment-specific variants such as .env.local and
.env.development, preventing files containing secrets like ANTHROPIC_API_KEY
from being committed.


**Goal:** Script-driven research loop that iteratively mutates a Pi harness config (system instructions, skills, tools, model, subagent roles) to maximize a vision-model rubric score on generated landing pages.

**Architecture:** An inner loop builds one self-contained `output.html` per train prompt via the `pi` CLI, screenshots it with Playwright, and scores it with one Opus vision call against a fixed rubric plus a cached reference screenshot. An outer loop aggregates the batch and asks Opus (schema-constrained) for the next config version, hill-climbing from the best-scoring version so far. Everything persists as files under `runs/`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document references as optional across the workflow specification. The current workflow supports rubric-only scoring when references are unavailable, but both documents still describe references as required prerequisites.

  • docs/superpowers/plans/2026-07-18-ui-harness-autoresearch.md#L7-L7: describe evaluation as rubric-only by default, with reference comparison used only when cached artifacts exist.
  • docs/superpowers/specs/2026-07-18-ui-harness-autoresearch-design.md#L101-L119: make reference segments optional and remove the claim that the loop refuses to start without them.
📍 Affects 2 files
  • docs/superpowers/plans/2026-07-18-ui-harness-autoresearch.md#L7-L7 (this comment)
  • docs/superpowers/specs/2026-07-18-ui-harness-autoresearch-design.md#L101-L119
🤖 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 `@docs/superpowers/plans/2026-07-18-ui-harness-autoresearch.md` at line 7,
Update the workflow documentation to treat reference artifacts as optional: in
docs/superpowers/plans/2026-07-18-ui-harness-autoresearch.md:7-7, describe
rubric-only scoring as the default and use reference comparison only when cached
artifacts are available; in
docs/superpowers/specs/2026-07-18-ui-harness-autoresearch-design.md:101-119,
make reference segments optional and remove the requirement that the loop refuse
to start without them.

Comment thread src/cli.ts
Comment on lines +14 to +26
const { values } = parseArgs({
args: Bun.argv.slice(3),
options: {
iterations: { type: "string", default: "5" },
"run-id": { type: "string" },
concurrency: { type: "string", default: "5" },
version: { type: "string" },
force: { type: "boolean", default: false },
},
});

const all = loadPrompts();
const concurrency = Number(values.concurrency);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Validate numeric options at the CLI boundary and defensively in the limiter. Unchecked values currently permit silent no-op runs and permanently stalled queues.

  • src/cli.ts#L14-L26: parse and reject non-finite, non-integer, or out-of-range iterations, concurrency, and version values.
  • src/cli.ts#L48-L64: pass only validated numbers into loop and holdout operations.
  • src/util/concurrency.ts#L1-L5: throw unless n is a positive safe integer.
📍 Affects 2 files
  • src/cli.ts#L14-L26 (this comment)
  • src/cli.ts#L48-L64
  • src/util/concurrency.ts#L1-L5
🤖 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/cli.ts` around lines 14 - 26, Validate iterations, concurrency, and
version at the CLI boundary in src/cli.ts lines 14-26, rejecting non-finite,
non-integer, or out-of-range values before parsing results are used; ensure
src/cli.ts lines 48-64 pass only validated numeric values to loop and holdout
operations. In src/util/concurrency.ts lines 1-5, update the limiter validation
to throw unless n is a positive safe integer.

Comment thread src/config/resolver.ts
Comment on lines +16 to +24
for (const sub of config.subagents) {
parts.push(
[
`## Internal pass: ${sub.name}`,
`Before finishing, perform this pass as "${sub.name}" (${sub.description}):`,
sub.system_instructions,
].join("\n"),
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not expose inert subagent tool settings.

sub.tools is never rendered or enforced; all internal passes run under the global config.tools at Lines 44-48. Either remove this unsupported field from the genome or explicitly render it as an advisory constraint and document that plain Pi cannot enforce per-pass permissions.

🤖 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/config/resolver.ts` around lines 16 - 24, Remove the unsupported
per-subagent tools field from the configuration schema/genome and any related
handling, since the pass-building loop around sub.name and
sub.system_instructions does not enforce it. Keep internal passes governed by
the global config.tools behavior.

Comment thread src/config/schema.ts
}),
tools: z.array(z.enum(ALLOWED_TOOLS)).min(1),
system_instructions: z.string().min(1),
skills: z.array(SkillSchema).max(8),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reject duplicate skill IDs.

Duplicate IDs validate but resolve to the same skills/<id>/SKILL.md, silently discarding an earlier skill. Enforce uniqueness before accepting a mutated config.

🤖 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/config/schema.ts` at line 29, Update the skills validation in the config
schema around SkillSchema so arrays containing duplicate skill IDs are rejected
before the mutated config is accepted. Preserve the existing maximum of eight
skills and validate uniqueness using each skill’s ID.

Comment thread src/outer/mutate.ts Outdated
Comment on lines +56 to +60
return HarnessConfigSchema.parse({
...proposal,
version: opts.nextVersion,
parent_version: opts.bestConfig.version,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep model.name outside the mutation search space.

MUTATOR_SYSTEM permits changing only model.thinking_level, but the returned proposal can replace the model name. Normalize it from bestConfig while retaining the proposed thinking level.

Proposed fix
   return HarnessConfigSchema.parse({
     ...proposal,
+    model: {
+      ...proposal.model,
+      name: opts.bestConfig.model.name,
+    },
     version: opts.nextVersion,
     parent_version: opts.bestConfig.version,
   });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return HarnessConfigSchema.parse({
...proposal,
version: opts.nextVersion,
parent_version: opts.bestConfig.version,
});
return HarnessConfigSchema.parse({
...proposal,
model: {
...proposal.model,
name: opts.bestConfig.model.name,
},
version: opts.nextVersion,
parent_version: opts.bestConfig.version,
});
🤖 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/outer/mutate.ts` around lines 56 - 60, The returned proposal in the
mutation flow must preserve the existing model name. Update the object passed to
HarnessConfigSchema.parse in the mutate function to take model.name from
opts.bestConfig while retaining the proposed model.thinking_level and other
mutation fields.

Comment thread src/prompts.ts
import { readFileSync } from "node:fs";

const PromptSpecSchema = z.object({
id: z.string().min(1),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Constrain prompt IDs to filesystem-safe slugs.

id flows into join(..., prompt.id) for prompt workspaces and reference artifacts. Values such as ../../outside escape runs/ and can place the Pi subprocess outside its intended workspace.

Proposed fix
-  id: z.string().min(1),
+  id: z.string().regex(/^[a-z0-9][a-z0-9-]*$/),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
id: z.string().min(1),
id: z.string().regex(/^[a-z0-9][a-z0-9-]*$/),
🤖 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/prompts.ts` at line 5, Constrain the prompt schema’s id field to a
filesystem-safe slug before it is used for workspace and artifact paths. Update
the visible id validation in the prompt schema to reject path separators,
traversal segments, and other unsafe characters while preserving non-empty valid
identifiers.

Comment thread tests/build.test.ts
Comment on lines +17 to +36
test("success when stub writes output.html", async () => {
const base = mkdtempSync(join(tmpdir(), "build-"));
const ws = join(base, "workspace");
mkdirSync(ws, { recursive: true });
const filler = "x".repeat(200);
process.env.PI_BIN = stubPi(base, `echo '<html><body>hi ${filler}</body></html>' > output.html`);
const resolved = resolveHarness(BASELINE_CONFIG, join(base, "resolved"));
const r = await buildPage({ resolved, prompt, workspaceDir: ws });
expect(r.ok).toBe(true);
});

test("failure when no output.html", async () => {
const base = mkdtempSync(join(tmpdir(), "build2-"));
const ws = join(base, "workspace");
mkdirSync(ws, { recursive: true });
process.env.PI_BIN = stubPi(base, `echo did nothing`);
const resolved = resolveHarness(BASELINE_CONFIG, join(base, "resolved"));
const r = await buildPage({ resolved, prompt, workspaceDir: ws });
expect(r.ok).toBe(false);
if (!r.ok) expect(r.error).toContain("output.html");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Restore PI_BIN after each test.

These tests leak a process-global stub into later tests, making suite behavior order-dependent. Preserve the original value and restore it in afterEach.

Proposed fix
-import { expect, test } from "bun:test";
+import { afterEach, expect, test } from "bun:test";
 ...
+const originalPiBin = process.env.PI_BIN;
+afterEach(() => {
+  if (originalPiBin === undefined) delete process.env.PI_BIN;
+  else process.env.PI_BIN = originalPiBin;
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test("success when stub writes output.html", async () => {
const base = mkdtempSync(join(tmpdir(), "build-"));
const ws = join(base, "workspace");
mkdirSync(ws, { recursive: true });
const filler = "x".repeat(200);
process.env.PI_BIN = stubPi(base, `echo '<html><body>hi ${filler}</body></html>' > output.html`);
const resolved = resolveHarness(BASELINE_CONFIG, join(base, "resolved"));
const r = await buildPage({ resolved, prompt, workspaceDir: ws });
expect(r.ok).toBe(true);
});
test("failure when no output.html", async () => {
const base = mkdtempSync(join(tmpdir(), "build2-"));
const ws = join(base, "workspace");
mkdirSync(ws, { recursive: true });
process.env.PI_BIN = stubPi(base, `echo did nothing`);
const resolved = resolveHarness(BASELINE_CONFIG, join(base, "resolved"));
const r = await buildPage({ resolved, prompt, workspaceDir: ws });
expect(r.ok).toBe(false);
if (!r.ok) expect(r.error).toContain("output.html");
import { afterEach, expect, test } from "bun:test";
const originalPiBin = process.env.PI_BIN;
afterEach(() => {
if (originalPiBin === undefined) delete process.env.PI_BIN;
else process.env.PI_BIN = originalPiBin;
});
test("success when stub writes output.html", async () => {
const base = mkdtempSync(join(tmpdir(), "build-"));
const ws = join(base, "workspace");
mkdirSync(ws, { recursive: true });
const filler = "x".repeat(200);
process.env.PI_BIN = stubPi(base, `echo '<html><body>hi ${filler}</body></html>' > output.html`);
const resolved = resolveHarness(BASELINE_CONFIG, join(base, "resolved"));
const r = await buildPage({ resolved, prompt, workspaceDir: ws });
expect(r.ok).toBe(true);
});
test("failure when no output.html", async () => {
const base = mkdtempSync(join(tmpdir(), "build2-"));
const ws = join(base, "workspace");
mkdirSync(ws, { recursive: true });
process.env.PI_BIN = stubPi(base, `echo did nothing`);
const resolved = resolveHarness(BASELINE_CONFIG, join(base, "resolved"));
const r = await buildPage({ resolved, prompt, workspaceDir: ws });
expect(r.ok).toBe(false);
if (!r.ok) expect(r.error).toContain("output.html");
});
🤖 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 `@tests/build.test.ts` around lines 17 - 36, Update the test setup around the
PI_BIN assignments in the success and failure tests to preserve the original
process.env.PI_BIN value and restore it in an afterEach hook. Ensure restoration
also handles the variable being initially undefined, preventing the stub from
leaking into later tests.

Comment thread tests/orchestrator.test.ts
Comment thread tests/resolver.test.ts
Comment on lines +31 to +38
test("deterministic: same config twice → identical bytes", () => {
const a = mkdtempSync(join(tmpdir(), "ra-"));
const b = mkdtempSync(join(tmpdir(), "rb-"));
resolveHarness(cfg, a);
resolveHarness(cfg, b);
expect(readFileSync(join(a, "system-prompt.md"), "utf8")).toBe(readFileSync(join(b, "system-prompt.md"), "utf8"));
expect(readdirSync(join(a, "skills")).sort()).toEqual(readdirSync(join(b, "skills")).sort());
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Test canonical skill ordering with multiple input orders.

A single skill and identical config inputs pass even if skill sorting is removed. Add at least two skills in opposite orders and assert identical piArgs/materialized skill contents, so the deterministic ordering contract is protected.

🤖 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 `@tests/resolver.test.ts` around lines 31 - 38, Strengthen the deterministic
test around resolveHarness by defining at least two skills and running
equivalent configs with those skills in opposite input orders. Assert that both
resolutions produce identical piArgs and materialized skill contents, while
retaining the existing system prompt and skill filename checks.

bukacdan and others added 5 commits July 18, 2026 17:53
The mutator LLM sometimes returns model as a bare string or without
thinking_level, which crashed the whole run after 3 forcedToolCall retries.
Normalize both shapes to the canonical object before validation; the emitted
JSON schema stays strict so the tool still guides the model correctly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s model

A mutator config leaked tool-call markup into model.name, so pi got a garbage
model and every build failed. Constrain model.name to a verified allowlist,
strip embedded markup, and coerce unknown names to the default. Add a --model
CLI flag (and runLoop builderModel) that pins the builder model across seed and
mutations so an experiment stays on the chosen model.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the one-shot Opus mutator with an agentic Pi session (default Fable).
It reads the iteration's candidate + reference screenshots and generated HTML,
then authors the next genome (skills, subagents, system instructions, tools,
thinking level) as next-config.json, which is schema-validated with version
lineage pinned. MUTATOR_MODEL overrides the model. Tests stub pi to serve both
builder and mutator roles.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New prompts-styled.json: brutalist, editorial, luxury-minimal, cinematic,
retro-maximalist, playful, plus swiss + organic holdouts. --prompts <path>
lets a run target any brief set, so experiments can probe headroom the
generic baseline lacks on standard SaaS pages.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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: 3

🤖 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/config/schema.ts`:
- Around line 34-55: Update the ModelField preprocessing to sanitize
thinking_level values before passing them to ModelSchema, including stripping
embedded markup and whitespace and falling back to "medium" when the result is
absent or not in THINKING_LEVELS. Preserve valid thinking levels and the
existing cleanModelName behavior.

In `@src/outer/mutate.ts`:
- Around line 142-146: Update the configuration object passed to
HarnessConfigSchema.safeParse in the mutate flow to overwrite model.name with
the run-fixed model value after spreading raw. Reuse the existing run-pinned
model symbol, ensuring valid but different mutator-provided names cannot replace
it while preserving other parsed configuration fields.

In `@tests/mutate.test.ts`:
- Around line 50-85: Restore the process-global PI_BIN environment override
after each test in the affected mutateConfig tests. Capture its original value
before tests run and add afterEach cleanup that reinstates it when defined or
deletes PI_BIN when it was originally absent; keep the existing test setup and
assertions unchanged.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 55373f6c-112c-4d4a-bb32-9375b46e5979

📥 Commits

Reviewing files that changed from the base of the PR and between ed8ea4b and 7c3f9d8.

📒 Files selected for processing (9)
  • README.md
  • prompts-styled.json
  • src/cli.ts
  • src/config/schema.ts
  • src/orchestrator.ts
  • src/outer/mutate.ts
  • tests/config-schema.test.ts
  • tests/mutate.test.ts
  • tests/orchestrator.test.ts

Comment thread src/config/schema.ts
Comment on lines +34 to +55
// The mutator LLM misbehaves in two observed ways: (1) it flattens `model` to a bare string or omits
// thinking_level; (2) it leaks tool-call markup into the value, e.g. `\n<parameter name="name">anthropic/...`,
// which `pi` then rejects as an unknown model and every build fails. Normalize before validation: strip any
// embedded markup/whitespace, keep the name only if it is allowlisted, otherwise fall back to DEFAULT_MODEL,
// and default a missing thinking_level to medium. This keeps one stray shape from crashing or wedging a run.
const cleanModelName = (n: unknown): (typeof ALLOWED_MODELS)[number] => {
const s = typeof n === "string" ? n.replace(/<[^>]*>/g, "").trim() : "";
return (ALLOWED_MODELS as readonly string[]).includes(s) ? (s as (typeof ALLOWED_MODELS)[number]) : DEFAULT_MODEL;
};

const ModelField = z.preprocess((v) => {
const obj =
typeof v === "string"
? { name: v }
: v && typeof v === "object" && !Array.isArray(v)
? { ...(v as Record<string, unknown>) }
: {};
return {
name: cleanModelName((obj as Record<string, unknown>).name),
thinking_level: "thinking_level" in obj ? (obj as Record<string, unknown>).thinking_level : "medium",
};
}, ModelSchema);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

thinking_level isn't sanitized the same way name is.

The comment above this block explicitly calls out two mutator failure modes (bare-string/missing fields, and leaked tool-call markup) and cleanModelName handles both for name. But for thinking_level, malformed-but-present values (e.g. leaked markup) aren't cleaned — only a fully-absent key gets the "medium" default. A malformed thinking_level will fail ModelSchema's z.enum(THINKING_LEVELS) and propagate as a hard safeParse failure in mutateConfig, burning retries or crashing the run, exactly the scenario this normalization was added to prevent for name.

Proposed fix
+const cleanThinkingLevel = (t: unknown): (typeof THINKING_LEVELS)[number] => {
+  const s = typeof t === "string" ? t.replace(/<[^>]*>/g, "").trim() : "";
+  return (THINKING_LEVELS as readonly string[]).includes(s) ? (s as (typeof THINKING_LEVELS)[number]) : "medium";
+};
+
 const ModelField = z.preprocess((v) => {
   const obj =
     typeof v === "string"
       ? { name: v }
       : v && typeof v === "object" && !Array.isArray(v)
         ? { ...(v as Record<string, unknown>) }
         : {};
   return {
     name: cleanModelName((obj as Record<string, unknown>).name),
-    thinking_level: "thinking_level" in obj ? (obj as Record<string, unknown>).thinking_level : "medium",
+    thinking_level: cleanThinkingLevel((obj as Record<string, unknown>).thinking_level),
   };
 }, ModelSchema);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// The mutator LLM misbehaves in two observed ways: (1) it flattens `model` to a bare string or omits
// thinking_level; (2) it leaks tool-call markup into the value, e.g. `\n<parameter name="name">anthropic/...`,
// which `pi` then rejects as an unknown model and every build fails. Normalize before validation: strip any
// embedded markup/whitespace, keep the name only if it is allowlisted, otherwise fall back to DEFAULT_MODEL,
// and default a missing thinking_level to medium. This keeps one stray shape from crashing or wedging a run.
const cleanModelName = (n: unknown): (typeof ALLOWED_MODELS)[number] => {
const s = typeof n === "string" ? n.replace(/<[^>]*>/g, "").trim() : "";
return (ALLOWED_MODELS as readonly string[]).includes(s) ? (s as (typeof ALLOWED_MODELS)[number]) : DEFAULT_MODEL;
};
const ModelField = z.preprocess((v) => {
const obj =
typeof v === "string"
? { name: v }
: v && typeof v === "object" && !Array.isArray(v)
? { ...(v as Record<string, unknown>) }
: {};
return {
name: cleanModelName((obj as Record<string, unknown>).name),
thinking_level: "thinking_level" in obj ? (obj as Record<string, unknown>).thinking_level : "medium",
};
}, ModelSchema);
// The mutator LLM misbehaves in two observed ways: (1) it flattens `model` to a bare string or omits
// thinking_level; (2) it leaks tool-call markup into the value, e.g. `\n<parameter name="name">anthropic/...`,
// which `pi` then rejects as an unknown model and every build fails. Normalize before validation: strip any
// embedded markup/whitespace, keep the name only if it is allowlisted, otherwise fall back to DEFAULT_MODEL,
// and default a missing thinking_level to medium. This keeps one stray shape from crashing or wedging a run.
const cleanModelName = (n: unknown): (typeof ALLOWED_MODELS)[number] => {
const s = typeof n === "string" ? n.replace(/<[^>]*>/g, "").trim() : "";
return (ALLOWED_MODELS as readonly string[]).includes(s) ? (s as (typeof ALLOWED_MODELS)[number]) : DEFAULT_MODEL;
};
const cleanThinkingLevel = (t: unknown): (typeof THINKING_LEVELS)[number] => {
const s = typeof t === "string" ? t.replace(/<[^>]*>/g, "").trim() : "";
return (THINKING_LEVELS as readonly string[]).includes(s) ? (s as (typeof THINKING_LEVELS)[number]) : "medium";
};
const ModelField = z.preprocess((v) => {
const obj =
typeof v === "string"
? { name: v }
: v && typeof v === "object" && !Array.isArray(v)
? { ...(v as Record<string, unknown>) }
: {};
return {
name: cleanModelName((obj as Record<string, unknown>).name),
thinking_level: cleanThinkingLevel((obj as Record<string, unknown>).thinking_level),
};
}, ModelSchema);
🤖 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/config/schema.ts` around lines 34 - 55, Update the ModelField
preprocessing to sanitize thinking_level values before passing them to
ModelSchema, including stripping embedded markup and whitespace and falling back
to "medium" when the result is absent or not in THINKING_LEVELS. Preserve valid
thinking levels and the existing cleanModelName behavior.

Comment thread src/outer/mutate.ts
Comment on lines +142 to +146
const parsed = HarnessConfigSchema.safeParse({
...(raw as Record<string, unknown>),
version: opts.nextVersion,
parent_version: opts.bestConfig.version,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Still doesn't pin model.name to the run-fixed value.

MUTATOR_SYSTEM tells the agent it "may NOT change model.name (it is fixed for the run)," but nothing in the parse step enforces this — the mutator can still emit any allowlisted model name (schema.ts only falls back to DEFAULT_MODEL for invalid names, not different-but-valid ones) and it will silently become the new best config's model, drifting away from the model pinned for the run. This was flagged on a prior revision and remains unaddressed.

Proposed fix
     const parsed = HarnessConfigSchema.safeParse({
       ...(raw as Record<string, unknown>),
+      model: {
+        ...(raw as Record<string, unknown>).model as object,
+        name: opts.bestConfig.model.name,
+      },
       version: opts.nextVersion,
       parent_version: opts.bestConfig.version,
     });
🤖 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/outer/mutate.ts` around lines 142 - 146, Update the configuration object
passed to HarnessConfigSchema.safeParse in the mutate flow to overwrite
model.name with the run-fixed model value after spreading raw. Reuse the
existing run-pinned model symbol, ensuring valid but different mutator-provided
names cannot replace it while preserving other parsed configuration fields.

Comment thread tests/mutate.test.ts
Comment on lines +50 to +85
test("reads pi-written next-config.json and pins version/parent_version", async () => {
const base = mkdtempSync(join(tmpdir(), "mut-"));
const work = join(base, "work");
mkdirSync(work, { recursive: true });
const cfg = JSON.stringify({ ...BASELINE_CONFIG, version: 999, parent_version: 42, rationale: "Add a typography skill." });
process.env.PI_BIN = stubPi(base, `cat > next-config.json <<'CFGEOF'\n${cfg}\nCFGEOF`);

const next = await mutateConfig(baseOpts(work));
expect(next.version).toBe(5); // pinned from nextVersion
expect(next.parent_version).toBe(0); // pinned from bestConfig.version
expect(next.rationale).toBe("Add a typography skill.");
});

test("retries when the first attempt writes nothing, then succeeds", async () => {
const base = mkdtempSync(join(tmpdir(), "mut2-"));
const work = join(base, "work");
mkdirSync(work, { recursive: true });
const cfg = JSON.stringify({ ...BASELINE_CONFIG, rationale: "second attempt" });
// count invocations in the cwd; only write on the 2nd+ call
process.env.PI_BIN = stubPi(
base,
`n=$(cat .n 2>/dev/null || echo 0); n=$((n+1)); echo $n > .n\nif [ "$n" -ge 2 ]; then cat > next-config.json <<'CFGEOF'\n${cfg}\nCFGEOF\nfi`,
);

const next = await mutateConfig(baseOpts(work));
expect(next.rationale).toBe("second attempt");
expect(next.version).toBe(5);
});

test("throws after retries when pi never writes a valid config", async () => {
const base = mkdtempSync(join(tmpdir(), "mut3-"));
const work = join(base, "work");
mkdirSync(work, { recursive: true });
process.env.PI_BIN = stubPi(base, `echo "did nothing useful"`);

await expect(mutateConfig({ ...baseOpts(work), maxRetries: 1 })).rejects.toThrow(/failed after 2 attempts/);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Restore PI_BIN after each test. These cases mutate a process-global env override without cleanup, so later tests can inherit the stub; capture the original value and restore/delete it in afterEach.

🤖 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 `@tests/mutate.test.ts` around lines 50 - 85, Restore the process-global PI_BIN
environment override after each test in the affected mutateConfig tests. Capture
its original value before tests run and add afterEach cleanup that reinstates it
when defined or deletes PI_BIN when it was originally absent; keep the existing
test setup and assertions unchanged.

The pi timeout killed the child but we kept awaiting its stdout/stderr streams,
which a lingering grandchild held open — so a timed-out mutation hung the whole
loop forever instead of retrying. Add runPiCapped: drains concurrently, races
the timeout, SIGKILLs, and never awaits the streams past a grace period.
Both builder and mutator use it. Also drop mutator thinking to medium
(MUTATOR_THINKING override), shorten the mutation timeout to 6m, and cap the
screenshots the mutator reads (4 desktop candidate + 3 reference) to cut its
latency and token load.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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: 2

♻️ Duplicate comments (1)
src/outer/mutate.ts (1)

138-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

model.name still isn't pinned to the run-fixed value.

MUTATOR_SYSTEM tells the agent it may not change model.name, but the safeParse call only forces version/parent_version; a valid-but-different model name from the mutated JSON will pass validation and silently become the new best config's model. Flagged on prior revisions and still unaddressed here.

Proposed fix
     const parsed = HarnessConfigSchema.safeParse({
       ...(raw as Record<string, unknown>),
+      model: {
+        ...(raw as Record<string, unknown>).model as object,
+        name: opts.bestConfig.model.name,
+      },
       version: opts.nextVersion,
       parent_version: opts.bestConfig.version,
     });
🤖 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/outer/mutate.ts` around lines 138 - 142, Update the
HarnessConfigSchema.safeParse input in the mutation flow to overwrite model.name
with the run-fixed model name from the original configuration or established
runtime value, alongside version and parent_version. Ensure any mutated
model.name is ignored while preserving the rest of the parsed configuration.
🤖 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/util/pi.ts`:
- Around line 8-36: Update runPiCapped to retain handles for both the main
timeout and the 2-second grace timeout, clearing the main timer when completed
wins and clearing the grace timer when the drain promise wins. Preserve the
existing timeout, kill, output, and timedOut result behavior.

In `@tests/pi.test.ts`:
- Around line 32-42: Extend the timeout test for runPiCapped to assert that
r.stdout contains the buffered “started” output produced before the process is
killed, while preserving the existing timedOut and prompt-return assertions.

---

Duplicate comments:
In `@src/outer/mutate.ts`:
- Around line 138-142: Update the HarnessConfigSchema.safeParse input in the
mutation flow to overwrite model.name with the run-fixed model name from the
original configuration or established runtime value, alongside version and
parent_version. Ensure any mutated model.name is ignored while preserving the
rest of the parsed configuration.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 12423526-66f5-48d4-898b-164834949532

📥 Commits

Reviewing files that changed from the base of the PR and between 7c3f9d8 and 5affd54.

📒 Files selected for processing (5)
  • src/inner/build.ts
  • src/orchestrator.ts
  • src/outer/mutate.ts
  • src/util/pi.ts
  • tests/pi.test.ts

Comment thread src/util/pi.ts
Comment on lines +8 to +36
export async function runPiCapped(bin: string, args: string[], opts: { cwd: string; timeoutMs: number }): Promise<PiResult> {
const proc = Bun.spawn([bin, ...args], { cwd: opts.cwd, stdout: "pipe", stderr: "pipe", env: { ...process.env } });
const drain = Promise.all([
new Response(proc.stdout).text().catch(() => ""),
new Response(proc.stderr).text().catch(() => ""),
]);

const completed = Promise.all([drain, proc.exited]).then(
([[stdout, stderr], exitCode]) => ({ stdout, stderr, exitCode, timedOut: false }) as PiResult,
);
const timeout = new Promise<null>((resolve) =>
setTimeout(() => {
try {
proc.kill(9);
} catch {
// already gone
}
resolve(null);
}, opts.timeoutMs),
);

const raced = await Promise.race([completed, timeout]);
if (raced) return raced;

// Timed out and killed. Grab whatever buffered output is available, but never hang on the streams.
const grace = new Promise<[string, string]>((r) => setTimeout(() => r(["", ""]), 2000));
const [stdout, stderr] = await Promise.race([drain, grace]);
return { stdout, stderr, exitCode: proc.exitCode ?? -1, timedOut: true };
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Do JavaScript/Bun setTimeout timers keep the event loop alive if not cleared or unref'd?

💡 Result:

Yes, in both JavaScript runtimes Node.js and Bun, setTimeout and setInterval timers keep the event loop alive by default [1][2][3]. As long as an active timer exists, the process will not exit [1][2]. You can change this behavior by calling.unref on the timer object returned by setTimeout or setInterval [2][3][4]. When a timer is unref'd, it will no longer prevent the process from exiting if there is no other activity keeping the event loop running [2][3]. If other activities (like an open server or pending I/O) keep the loop active, the unref'd timer will still fire as scheduled [5]. This functionality is consistent between Node.js and Bun [1][5]. Both provide the same.ref and.unref API on the Timer objects to allow fine-grained control over process lifecycle management [2][3][4][6].

Citations:


Clear the dangling timers
Neither setTimeout is cleared on the fast path, so a normal runPiCapped completion can still leave a live timer that keeps the process open until opts.timeoutMs elapses. Clear the timeout when completed wins, and do the same for the 2s grace timer after the timeout path.

Proposed fix
-  const timeout = new Promise<null>((resolve) =>
-    setTimeout(() => {
-      try {
-        proc.kill(9);
-      } catch {
-        // already gone
-      }
-      resolve(null);
-    }, opts.timeoutMs),
-  );
+  let timeoutTimer: ReturnType<typeof setTimeout>;
+  const timeout = new Promise<null>((resolve) => {
+    timeoutTimer = setTimeout(() => {
+      try {
+        proc.kill(9);
+      } catch {
+        // already gone
+      }
+      resolve(null);
+    }, opts.timeoutMs);
+  });
 
   const raced = await Promise.race([completed, timeout]);
+  clearTimeout(timeoutTimer);
   if (raced) return raced;
 
   // Timed out and killed. Grab whatever buffered output is available, but never hang on the streams.
-  const grace = new Promise<[string, string]>((r) => setTimeout(() => r(["", ""]), 2000));
-  const [stdout, stderr] = await Promise.race([drain, grace]);
+  let graceTimer: ReturnType<typeof setTimeout>;
+  const grace = new Promise<[string, string]>((r) => {
+    graceTimer = setTimeout(() => r(["", ""]), 2000);
+  });
+  const [stdout, stderr] = await Promise.race([drain, grace]);
+  clearTimeout(graceTimer);
   return { stdout, stderr, exitCode: proc.exitCode ?? -1, timedOut: true };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export async function runPiCapped(bin: string, args: string[], opts: { cwd: string; timeoutMs: number }): Promise<PiResult> {
const proc = Bun.spawn([bin, ...args], { cwd: opts.cwd, stdout: "pipe", stderr: "pipe", env: { ...process.env } });
const drain = Promise.all([
new Response(proc.stdout).text().catch(() => ""),
new Response(proc.stderr).text().catch(() => ""),
]);
const completed = Promise.all([drain, proc.exited]).then(
([[stdout, stderr], exitCode]) => ({ stdout, stderr, exitCode, timedOut: false }) as PiResult,
);
const timeout = new Promise<null>((resolve) =>
setTimeout(() => {
try {
proc.kill(9);
} catch {
// already gone
}
resolve(null);
}, opts.timeoutMs),
);
const raced = await Promise.race([completed, timeout]);
if (raced) return raced;
// Timed out and killed. Grab whatever buffered output is available, but never hang on the streams.
const grace = new Promise<[string, string]>((r) => setTimeout(() => r(["", ""]), 2000));
const [stdout, stderr] = await Promise.race([drain, grace]);
return { stdout, stderr, exitCode: proc.exitCode ?? -1, timedOut: true };
}
export async function runPiCapped(bin: string, args: string[], opts: { cwd: string; timeoutMs: number }): Promise<PiResult> {
const proc = Bun.spawn([bin, ...args], { cwd: opts.cwd, stdout: "pipe", stderr: "pipe", env: { ...process.env } });
const drain = Promise.all([
new Response(proc.stdout).text().catch(() => ""),
new Response(proc.stderr).text().catch(() => ""),
]);
const completed = Promise.all([drain, proc.exited]).then(
([[stdout, stderr], exitCode]) => ({ stdout, stderr, exitCode, timedOut: false }) as PiResult,
);
let timeoutTimer: ReturnType<typeof setTimeout>;
const timeout = new Promise<null>((resolve) => {
timeoutTimer = setTimeout(() => {
try {
proc.kill(9);
} catch {
// already gone
}
resolve(null);
}, opts.timeoutMs);
});
const raced = await Promise.race([completed, timeout]);
clearTimeout(timeoutTimer);
if (raced) return raced;
// Timed out and killed. Grab whatever buffered output is available, but never hang on the streams.
let graceTimer: ReturnType<typeof setTimeout>;
const grace = new Promise<[string, string]>((r) => {
graceTimer = setTimeout(() => r(["", ""]), 2000);
});
const [stdout, stderr] = await Promise.race([drain, grace]);
clearTimeout(graceTimer);
return { stdout, stderr, exitCode: proc.exitCode ?? -1, timedOut: true };
}
🤖 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/util/pi.ts` around lines 8 - 36, Update runPiCapped to retain handles for
both the main timeout and the 2-second grace timeout, clearing the main timer
when completed wins and clearing the grace timer when the drain promise wins.
Preserve the existing timeout, kill, output, and timedOut result behavior.

Comment thread tests/pi.test.ts
Comment on lines +32 to +42
test("times out and does not hang when a grandchild keeps the pipe open", async () => {
const dir = mkdtempSync(join(tmpdir(), "pi3-"));
// spawn a background grandchild that holds stdout open, then the parent 'exits' but the pipe stays
// open — the classic hang. runPiCapped must still return within the timeout.
const bin = stub(dir, `sleep 30 & echo started; wait`);
const start = Date.now();
const r = await runPiCapped(bin, [], { cwd: dir, timeoutMs: 1000 });
const elapsed = Date.now() - start;
expect(r.timedOut).toBe(true);
expect(elapsed).toBeLessThan(8000); // returned promptly, did not hang on the open pipe
}, 15000);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider asserting the grace-drain actually captures buffered output.

This test proves runPiCapped doesn't hang on a timeout, but it doesn't verify the grace-drain path actually returns the output buffered before the kill (the echo started line). Asserting r.stdout would validate the core value of the grace mechanism, not just its non-hanging behavior.

Proposed addition
   expect(r.timedOut).toBe(true);
+  expect(r.stdout).toContain("started");
   expect(elapsed).toBeLessThan(8000); // returned promptly, did not hang on the open pipe
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test("times out and does not hang when a grandchild keeps the pipe open", async () => {
const dir = mkdtempSync(join(tmpdir(), "pi3-"));
// spawn a background grandchild that holds stdout open, then the parent 'exits' but the pipe stays
// open — the classic hang. runPiCapped must still return within the timeout.
const bin = stub(dir, `sleep 30 & echo started; wait`);
const start = Date.now();
const r = await runPiCapped(bin, [], { cwd: dir, timeoutMs: 1000 });
const elapsed = Date.now() - start;
expect(r.timedOut).toBe(true);
expect(elapsed).toBeLessThan(8000); // returned promptly, did not hang on the open pipe
}, 15000);
test("times out and does not hang when a grandchild keeps the pipe open", async () => {
const dir = mkdtempSync(join(tmpdir(), "pi3-"));
// spawn a background grandchild that holds stdout open, then the parent 'exits' but the pipe stays
// open — the classic hang. runPiCapped must still return within the timeout.
const bin = stub(dir, `sleep 30 & echo started; wait`);
const start = Date.now();
const r = await runPiCapped(bin, [], { cwd: dir, timeoutMs: 1000 });
const elapsed = Date.now() - start;
expect(r.timedOut).toBe(true);
expect(r.stdout).toContain("started");
expect(elapsed).toBeLessThan(8000); // returned promptly, did not hang on the open pipe
}, 15000);
🤖 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 `@tests/pi.test.ts` around lines 32 - 42, Extend the timeout test for
runPiCapped to assert that r.stdout contains the buffered “started” output
produced before the process is killed, while preserving the existing timedOut
and prompt-return assertions.

--seed-config seeds the loop from a custom genome; weak-seed.json is a
deliberately crippled baseline (Haiku, thinking off, no design guidance/skills)
so iteration 1 fails. prompts-showcase.json is one design-critical dev-tool
page, scored against strong Mobbin references, so the smart Opus mutator's
design skills produce a steep, legible climb. MUTATOR_TIMEOUT_MS override added.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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: 3

🤖 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 `@prompts-showcase.json`:
- Line 3: Update the description value in the showcase prompt configuration to
state that scoring uses rubric-only evaluation and that reference comparison is
deferred, removing the claim that results are scored against Mobbin references.

In `@src/cli.ts`:
- Line 30: Update the fallback usage text in the CLI argument-parser error path
to include the accepted --seed-config option, matching the existing option
formatting and preserving all current usage entries.

In `@src/outer/mutate.ts`:
- Line 97: Validate the timeout value resolved in the options destructuring
before it reaches runPiCapped: reject non-finite or non-positive
MUTATOR_TIMEOUT_MS values, while preserving the existing default behavior when
the environment variable is absent or invalid according to the chosen contract.
Apply the validation to the timeout option used by the surrounding mutate flow
rather than changing unrelated retry or workDir handling.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: c6f38217-c87f-4966-9d4c-7aec130fd940

📥 Commits

Reviewing files that changed from the base of the PR and between 5affd54 and d72c506.

📒 Files selected for processing (5)
  • prompts-showcase.json
  • src/cli.ts
  • src/orchestrator.ts
  • src/outer/mutate.ts
  • weak-seed.json

Comment thread prompts-showcase.json
@@ -0,0 +1,12 @@
{
"version": 1,
"description": "Single design-critical prompt for the improvement showcase run. Scored against strong Mobbin references (Linear/Neon/Vercel-class dev-tool pages), so a naive baseline page scores low and the outer loop's design skills produce a steep, legible climb.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the scoring description.

The description says this run is scored against Mobbin references, but the current workflow uses rubric-only evaluation because reference comparison is deferred. Update the text to avoid misleading showcase results.

🤖 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 `@prompts-showcase.json` at line 3, Update the description value in the
showcase prompt configuration to state that scoring uses rubric-only evaluation
and that reference comparison is deferred, removing the claim that results are
scored against Mobbin references.

Comment thread src/cli.ts
limit: { type: "string" },
model: { type: "string" },
prompts: { type: "string" },
"seed-config": { type: "string" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Advertise --seed-config in the usage text.

The parser accepts the new flag, but the fallback usage message at Line 90 omits it, making the new entry point undiscoverable from CLI errors.

Proposed fix
- ... [--version V]
+ ... [--version V] [--seed-config PATH]
🤖 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/cli.ts` at line 30, Update the fallback usage text in the CLI
argument-parser error path to include the accepted --seed-config option,
matching the existing option formatting and preserving all current usage
entries.

Comment thread src/outer/mutate.ts
timeoutMs?: number;
maxRetries?: number;
}): Promise<HarnessConfig> {
const { workDir, timeoutMs = Number(process.env.MUTATOR_TIMEOUT_MS) || 6 * 60 * 1000, maxRetries = 2 } = opts;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Validate MUTATOR_TIMEOUT_MS before passing it to runPiCapped.

Line 97 accepts negative or infinite values, while silently treating 0 as the default. Reject non-finite/non-positive values or define their semantics explicitly.

Proposed fix
-  const { workDir, timeoutMs = Number(process.env.MUTATOR_TIMEOUT_MS) || 6 * 60 * 1000, maxRetries = 2 } = opts;
+  const configuredTimeout = process.env.MUTATOR_TIMEOUT_MS === undefined
+    ? undefined
+    : Number(process.env.MUTATOR_TIMEOUT_MS);
+  if (configuredTimeout !== undefined &&
+      (!Number.isFinite(configuredTimeout) || configuredTimeout <= 0)) {
+    throw new Error("MUTATOR_TIMEOUT_MS must be a finite positive number");
+  }
+  const { workDir, timeoutMs = configuredTimeout ?? 6 * 60 * 1000, maxRetries = 2 } = opts;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { workDir, timeoutMs = Number(process.env.MUTATOR_TIMEOUT_MS) || 6 * 60 * 1000, maxRetries = 2 } = opts;
const configuredTimeout = process.env.MUTATOR_TIMEOUT_MS === undefined
? undefined
: Number(process.env.MUTATOR_TIMEOUT_MS);
if (configuredTimeout !== undefined &&
(!Number.isFinite(configuredTimeout) || configuredTimeout <= 0)) {
throw new Error("MUTATOR_TIMEOUT_MS must be a finite positive number");
}
const { workDir, timeoutMs = configuredTimeout ?? 6 * 60 * 1000, maxRetries = 2 } = opts;
🤖 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/outer/mutate.ts` at line 97, Validate the timeout value resolved in the
options destructuring before it reaches runPiCapped: reject non-finite or
non-positive MUTATOR_TIMEOUT_MS values, while preserving the existing default
behavior when the environment variable is absent or invalid according to the
chosen contract. Apply the validation to the timeout option used by the
surrounding mutate flow rather than changing unrelated retry or workDir
handling.

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