Repository navigation
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
|
Independent GPT-6 Astra high review of exact head The reviewer independently reran the public CLI dry-run/ZIP reproduction and 73 focused tests. Existing project assets survive, occupied suffixes are skipped, repeated external references remain deduplicated, and source files stay unchanged. The published patch, tree and actual merge-base were verified. Review reused the verified existing build on Linux arm64; no independent full rebuild, native Windows/macOS, authenticated upload or hosted render was performed. All 12 required checks pass, including the replacement Windows run; the final snapshot has no failed or pending checks. The requested failing-test line is present in the PR description. Jrusso approved this exact head and confirmed the description meets Genesis's request. Genesis's earlier body-only change request remains active, so GitHub still reports CHANGES_REQUESTED. No merge performed. Windows CI follow-up: the first engine-cli run failed in five The complete agent-mode file passes locally on Linux arm64, 435 tests with the existing time limits. This does not establish the Windows timeout cause. The replacement Windows run passed on the same head, including engine-cli and the aggregate gate. No source or timeout changes were made for this failure. Root follow-up on 2026-10-30: all 73 focused archive tests passed (two hosted e2e tests skipped). The public CLI dry-run and independent ZIP inspection retained the original red and blue project assets, placed the green external asset at |
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 9b849c06
- blocker: the body has no
fails without the fix:line naming the test. This lane needs one on every fix PR. Something likefails without the fix: publishProject.collision.test.ts, both cases (expected 3 to be 4 with publishProject.ts from main). It's a body-only change; the code needs nothing.
The fix is right: seeding usedArchivePaths with the staged keys sends a colliding external copy to the next free _N suffix, so it can't overwrite a project file.
Scope
Read publishProject.ts addExternalAsset/localizeExternalAssets, the new test and the docs paragraph. Locally, both new cases pass at this head and fail with publishProject.ts from main (expected 3 to be 4). All 73 publishProject* tests pass. CI and Windows render were still running when this was posted.
|
Added the requested The change is to the PR description only; head remains |
…name collision, Windows contact-sheet labels) publishProject seeds usedArchivePaths from fileContents.keys(), so external files no longer overwrite existing _ext/ names; contactSheet uses node:path basename for Windows paths. Both match the upstream PR diffs line for line; DECISIONS records the adoption. Executed from the roadmap's near-term table; the finished row is removed in the same commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 9b849c06709226bb69b2e47ad32c8be8b260a11e.
I checked archive allocation and its callers. Seeding the occupied names from staged keys preserves project assets while reusing the existing suffix allocator. The body now contains the regression evidence requested by Genesis.
Attempted vitest publishProject.collision.test.ts: collection blocked by missing adm-zip. Genesis independently recorded 73 passing tests at this exact SHA. I read the implementation and tests but did not independently execute archive creation. Root adversarial check required.
Need: yes · Size: ok · Reuse: existing implementation seam · Simpler: already minimal · Structure: fine
Risk: medium - archive asset identity and collision handling.
Verdict: APPROVE
Reasoning: I checked archive allocation and its callers. Seeding the occupied names from staged keys preserves project assets while reusing the existing suffix allocator. The body now contains the regression evidence requested by Genesis.
The current description now includes the requested fails without the fix: test evidence. Genesis's earlier current-head body-only change request is still live.
Reasoning: I checked archive allocation and its callers. Seeding the occupied names from staged keys preserves project assets while reusing the existing suffix allocator. The body now contains the regression evidence requested by Genesis.
Backport the collision allocator fix and regression coverage from heygen-com#6722 by AksharP5, upstream head 9b849c0. Seed occupied archive paths from staged project files so external assets use the next free suffix instead of replacing existing source assets. Document the behavior and retain upstream licensing and provenance. Both regression cases fail without the fix. Verified 82 archive/proxy tests, CLI build/typecheck, lint/format/comment gates, and an independent review. Real source and built-module ZIP checks preserve all three synthetic logos and original source hashes, with network/process traps. No upload, cloud render, browser playback, or native OS parity claimed.
What
Publishing or preparing a cloud render can silently replace an existing project asset with a different sibling asset. Keep both files and give the new copy an unused name.
Why
A project unpacked from a HyperFrames archive can already contain
_ext/shared/logo.svg. If it later references../shared/logo.svg, packing replaces the existing asset's bytes while leaving its references in place. Archive preparation succeeds, but the original image becomes the wrong logo.The synthetic reproduction has a red project logo, a blue
logo_2.svgcontrol, and a green external logo referenced twice. On the same inputs:49e031230cloud render --dry-run --jsonfile_count: 3file_count: 4_ext/shared/logo.svg_ext/shared/logo_2.svg_ext/shared/logo.svg_ext/shared/logo_3.svgoriginal red preservedfalsetruenew green separately archivedfalsetrueRelated work
Fixes #6721.
#1160 introduced localization and the suffix loop for collisions between external files. Its occupied-name set starts empty, so it overlooks staged project files. #6265/#6266 concern URL decoding and already document query/fragment omissions; those are outside this fix. #6687 changes OAuth endpoints in the same source file and does not touch localization.
Live open/closed issue and PR searches and related review discussions were refreshed before filing. No matching report or active work was found for overwriting an already staged project key.
How
Initialize the existing occupied-name set from the staged file-map keys. Reuse the existing suffix loop and external-file deduplication. No dependencies or new naming mechanism.
Reproduce
Run this from a repository checkout after
bun install --frozen-lockfileandbun run build. It creates only synthetic assets and uses the real CLI dry-run without uploading a project or starting a cloud job. The ZIP inspection reads the same archive helper used by publish.Test plan
packages/cli/src/utils/publishProject.collision.test.ts, bothpreserves existing archive paths when localizing assetscases, thresholdsInfinityand1. WithpublishProject.tsfrom main49e031230, both fail withexpected 3 to be 4; both pass at9b849c06. They cover in-memory and disk staging, an occupied_2suffix, deduplication, ZIP contents, rewritten references, and unchanged source files.bun run build, then rebuilt CLI.Limitations: Linux arm64 verification; no authenticated publish/cloud request was sent. Mintlify
validateandbroken-linkscould not run becausemintis not installed. The documentation edit is prose only and adds no links or MDX components.