Skip to content

feat(runtime): return structural rollout artifact#617

Merged
drewstone merged 3 commits into
mainfrom
fix/structural-rollout-artifact
Jul 25, 2026
Merged

feat(runtime): return structural rollout artifact#617
drewstone merged 3 commits into
mainfrom
fix/structural-rollout-artifact

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Closes #524.

structuralRollout() now returns the exact selected candidate text as artifact. Candidate extraction happens once, so the returned value is byte-for-byte the same text passed to visible checks. Runs with no candidate return null.

Checks:

  • pnpm exec vitest run src/runtime/structural-rollout.test.ts (19/19)
  • pnpm typecheck
  • pnpm exec biome check src/runtime/structural-rollout.ts src/runtime/structural-rollout.test.ts
  • git diff --check

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 546bb14b

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-25T00:20:49Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 116.5s (2 bridge agents)
Total 116.5s

💰 Value — sound

Surfaces the selected candidate text on the rollout result by capturing extraction once per shot (so the returned string is byte-for-byte what the checks scored); minimal, correct, and in the codebase's grain.

  • What it does: Adds an artifact: string | null field to StructuralRolloutResult (structural-rollout.ts:491). Previously each shot called extract(out.messages) inline and passed it straight to checkRunner.run without retaining it (structural-rollout.ts:578, 632), so the winning candidate's text was never surfaced on the result. The change hoists that call into a local artifact const, passes the SAME con
  • Goals it achieves: Lets a caller of runAgentic read back the exact text the strategy selected and checked — the deployable artifact for verifier-environment / code-completion domains where the artifact IS the worker's answer (verifier-environment.ts:4-6). Without it, a consumer had to re-run defaultExtractCandidate over messages they don't have (AgenticRunResult carries no messages), with no guarantee the re-ext
  • Assessment: Good change, built correctly. The key correctness property — returned artifact === text passed to checkRunner.run — is enforced structurally by binding one local const artifact and passing it to both the runner and the Candidate store, rather than re-extracting at the end (which could diverge if extraction were non-idempotent or the winner's messages mutated). best.artifact always tracks the
  • Better / existing approach: none — this is the right approach. Searched for an existing equivalent: AgenticRunResult (strategy.ts:608-622) has no artifact field; ShotResult (strategy.ts:438-445) carries messages but no extracted string; the verifier environment keeps submissions internally (verifier-environment.ts:93) but never surfaces them on a result; no other built-in strategy (sample, refine, adaptiveRefine,
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

A minimal, correct hoist that surfaces the exact scored candidate text as artifact — the only channel for the produced code out of runAgentic, and the field the bench rigs (the documented migration target) already consume.

  • Integration: structuralRollout() is reached via runAgentic({ strategy: structuralRollout(...) }) (strategy.ts:1089; consumed by run-benchmark.ts:149 and supervise-surface.ts:133). The artifact field rides through defineStrategy's deliverable spread ({ mode, ...r, ... }, strategy.ts:954-963) and runAgentic's return (strategy.ts:1116-1122), so it is present on the runtime object. The bench rigs that
  • Fit with existing patterns: Follows the established pattern exactly. StructuralRolloutResult already carries strategy-specific fields (selection, repairStop, officialChecks, authoredChecks) that ride the deliverable spread but are intentionally not declared on the shared AgenticRunResult type; the test reads them all via one cast (result as unknown as StructuralRolloutResult & { mode }, structural-rollout.test.
  • Real-world viability: Edge paths are handled: the no-candidates branch returns artifact: null (structural-rollout.ts:596), the success path returns best.artifact (675), and the repair-displacement path carries the challenger's artifact onto best (651). Extraction is synchronous and deterministic per shot; no new concurrency, ordering, or error surface is introduced. An empty extraction flows through identically
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Caller must cast to read artifact — pre-existing type erosion, not introduced here [ergonomics] ``

runAgentic is typed to return AgenticRunResult (strategy.ts:1089,1117), which declares no artifact (nor selection/repairStop). The field is present at runtime via the spread but invisible to a typed caller, who must cast as StructuralRolloutResult (as the test does at structural-rollout.test.ts:366). This is the same friction already affecting the four sibling fields, so it is consistent rather than new; but since the whole point of #524 is to let consumers (the migrated bench rigs)


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260725T002437Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 546bb14b

Review health 100/100 · Reviewer score 92/100 · Confidence 65/100 · 1 finding (1 low)

glm: Correctness 92 · Security 92 · Testing 92 · Architecture 92

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 1/1 planned shots over 2 changed files. Global verifier still owns final merge decision.

🟡 LOW artifact: null (no-candidates) branch has no dedicated test — src/runtime/structural-rollout.test.ts

The new field is asserted only on the happy repaired-pass path (expect(rollout.artifact).toBe('def f():\n return "D"')). The no-candidates return at structural-rollout.ts:596 sets artifact: null, but no test pins that contract — a future refactor could silently drop the null (e.g. return best.artifact after candidates.length === 0 throws differently) and the suite would stay green. Fix: add a one-line assertion in a pool-starved / zero-candidates test (expect(rollout.artifact).toBeNull() and expect(rollout.repairStop).toBe('no-candidates')). Low impact because the path is unreachable under any policy.k >= 1 with a non-starved budget, but the field's null-vs-string contract is now part of the public StructuralRolloutResult surface and worth pinning.


tangletools · 2026-07-25T00:25:13Z · trace

tangletools
tangletools previously approved these changes Jul 25, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 1 non-blocking finding — 546bb14b

Full multi-shot audit completed 1/1 planned shots over 2 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-25T00:25:13Z · immutable trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 4bd73539

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-25T00:48:06Z

@drewstone
drewstone merged commit a8f9eda into main Jul 25, 2026
3 checks passed
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.

feat(runtime): structuralRollout result should carry the winning artifact, not only SelectionReceipts

2 participants