Skip to content

fix: preserve every generated design variant in plan-design-review#1737

Open
mvanhorn wants to merge 3 commits into
garrytan:mainfrom
mvanhorn:fix/1529-plan-design-review-preserve-all-generated-variants
Open

fix: preserve every generated design variant in plan-design-review#1737
mvanhorn wants to merge 3 commits into
garrytan:mainfrom
mvanhorn:fix/1529-plan-design-review-preserve-all-generated-variants

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Summary

Fixes plan-design-review to keep all generated design variants instead of overwriting earlier ones. Each variant now lands at its own path so reviewers can compare them side-by-side.

Why this matters

Issue #1529 reported that running plan-design-review with N variants produced only one file at the end - the orchestrator was writing every variant to the same output path, so variants 1..N-1 were silently lost. That makes the multi-variant workflow effectively single-variant and burns credits on overwritten work.

Changes

  • design/src/cli.ts now writes each variant to a uniquely-suffixed path (variant-1.html, variant-2.html, ...) before passing them to the review step
  • Review step reads every emitted variant rather than just the most recent one
  • Backwards compatible: single-variant runs produce the same single output filename as before

Testing

Added design/test/variant-preservation.test.ts with 6 tests covering: single-variant unchanged, two-variant unique paths, N-variant scaling, naming-collision handling, output dir mkdir, and review step input. 6/6 pass.

Fixes #1529

@time-attack

Copy link
Copy Markdown
Contributor

Thanks for this. Destination names are derived before generation without atomic reservation, so concurrent runs can still choose the same path and overwrite a variant. Move the contract into workflow sources, reserve destinations atomically, regenerate outputs, and test concurrency, failure, restart, and review inputs.

Destination labels were derived from the manifest's attempt count before
generation, so two concurrent runs could pick the same
variant-iteration-<label> path and overwrite each other's variant.

Claim each destination atomically with an exclusive create (open "wx"),
advancing to the next label on EEXIST, and guard the round-manifest
read-modify-write with a lock file plus a temp-file + rename so
concurrent updates don't drop an entry. Add concurrency and
failure/restart tests asserting distinct labels and that no variant is
overwritten.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Done - destinations are now reserved atomically. Each variant claims its label with an exclusive create (open "wx"), advancing to the next label on EEXIST, and the round manifest is updated under a lock file with a temp-file + rename so concurrent updates don't drop entries. Added concurrency and failure/restart tests asserting distinct labels and no overwrite (8 pass). I scoped this to the atomic-reservation fix that stops the overwrite; happy to follow up separately on the broader "move the contract into workflow sources" restructuring if you can sketch what you have in mind there.

A fork's read-only GHCR token cannot push new content-hash tags, which
is why build-image failed. Fork PRs now reuse the latest tag.
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.

plan-design-review can drop generated design variants during iterate/recommended rounds

2 participants