feat(gcp-cloud-run): support plan protocol v2 - #2790
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
d527c47 to
d22ec30
Compare
dfd3881 to
5fc20bd
Compare
ae9fbad to
2ff3705
Compare
5fc20bd to
d5991e5
Compare
167fabf to
7d7308a
Compare
d5991e5 to
33a06aa
Compare
7d7308a to
5e12d68
Compare
33a06aa to
b6664db
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Adversarial R1 review at head b6664db3a.
Scope-shape confirmed: +2069 / −244 across 27 files, adapter delta on top of feat/plan-protocol-v2-aws. Header claim ("~+2031/−243") matches with rounding.
Behavior lens A — HTTP 400 classification. Verified. NON_RETRYABLE_ERROR_NAMES at packages/gcp-cloud-run/src/server.ts:900-922 contains every v2 terminal error name — PlanV2IntegrityError, PLAN_V2_INTEGRITY_UNRECOVERABLE, PlanProtocolUnsupportedError, PLAN_PROTOCOL_UNSUPPORTED, plus the pre-existing GCP-adapter set (GCS_URI_NOT_ALLOWED, PLAN_HASH_MISMATCH, PLAN_ARTIFACT_DIGEST_MISMATCH, FormatNotSupportedInDistributedError, PlanTooLargeError, RenderChunkValidationError, FFMPEG_VERSION_MISMATCH, FORMAT_NOT_SUPPORTED_IN_DISTRIBUTED, PLAN_TOO_LARGE, BROWSER_GPU_NOT_SOFTWARE, FONT_FETCH_FAILED, ChromeBinaryUnavailableError). Line 952 maps 400 iff .name is in the set, else 500. Cloud Workflows' retryable predicate (packages/gcp-cloud-run/terraform/workflow.yaml:304-318) keys off HTTP status — retries on missing code, 429, 403, 5xx — so 400 is genuinely terminal to the caller.
Behavior lens B — class/code aliases. Verified with a wire-string asymmetry. PlanV2IntegrityError sets this.name = "PlanV2IntegrityError" and this.code = "PLAN_V2_INTEGRITY_UNRECOVERABLE" (packages/producer/src/services/distributed/planV2.ts:56-65 on base). normalizeTerminalErrorName at server.ts:178-184 rewrites .name = .code only for codes PLAN_PROTOCOL_UNSUPPORTED and PLAN_TOO_LARGE — not PLAN_V2_INTEGRITY_UNRECOVERABLE. Classification still correct at 400 because both forms are in the name set, but body.error is PlanV2IntegrityError on class throw and PLAN_V2_INTEGRITY_UNRECOVERABLE on bare-code throw — the test at server.test.ts:517-550 locks this in via expect(body.error).toBe(error.name). Two P2 threads follow.
Behavior lens C — hash-valid v2 fixture. Verified. server.test.ts:243-354 runs createPlanV2FromV1 off a real minimal v1 planDir built by makeMinimalV1PlanDir (line 69-91) with recomputePlanHashFromPlanDir producing a real hash; handlePlanV2 (line 372) verifies the manifest's planHash against result.planHash and handleAssembleV2/handleRenderChunkV2 read the actual uploaded manifest via readPlanV2Manifest. No classifier short-circuit. Test verifies audio artifact is NOT downloaded by chunk worker (line 335) but IS downloaded by assembler (line 351-353) — proves target-scoped materialization.
Behavior lens D — v1/v2 branching. Verified. handlePlan/handleRenderChunk/handleAssemble each check event.PlanProtocol === "v2" explicitly (server.ts:287, 429, 557) and fall through to v1 otherwise. validatePlanProtocolShape (line 142-173) rejects any PlanProtocol value other than undefined/"v1"/"v2" and enforces disjoint locators — no path treats v2 as v1. Workflow-side validatePlanResult (workflow.yaml:104-113) enforces the disjoint locator invariant a second time before fan-out.
Behavior lens E — observability. Two soft gaps, both pre-existing in the v1 baseline (context lines in the diff, not +), so not introduced here: handler_error at server.ts:128-134 omits planProtocol and producerVersion (triage must join to handler_start by request ID), and pre-dispatch failures (validatePlanProtocolShape, validateEventGcsUris) throw before the try block on line 108 so no structured log line is emitted for those cases at all. P3 note only.
Miguel F — standards. as casts limited to wire boundaries followed by shape validation (server.ts:143, 195, 197, 200, 204, 942); one values[index]! (line 731) inside a bounds-checked mapConcurrent worker.
Miguel G — spec forward + reverse. Workflow / handler / test suite pin each other: workflow.test.ts:80-108 asserts default v1 + explicit v2 opt-in + disjoint locators; server.test.ts:356-371 locks the mirror invariant at the handler.
Miguel H — precision. Plan max_retries: 6, chunk/assemble max_retries: 4, timeout 1800s uniform — workflow.test.ts:76-77 pins the count. AWS state machine at packages/aws-lambda/src/cdk/HyperframesRenderStack.ts:196-238 on the same base branch tolerates both name and code forms for every producer terminal error — GCP NON_RETRYABLE_ERROR_NAMES mirrors that plus adds FormatNotSupportedInDistributedError and RenderChunkValidationError class names AWS omits. Divergence is defensive, not a regression.
Miguel I — middle-man. dispatch is the single testable core; handlePlan(V2?)/handleRenderChunk(V2?)/handleAssemble(V2?) are thin routers.
Editor-UI (12 axes): N/A.
Findings.
-
(P2, lens B)
normalizeTerminalErrorNameis inconsistent across the code alias set.server.ts:181promotes.code === "PLAN_PROTOCOL_UNSUPPORTED"and.code === "PLAN_TOO_LARGE"to.name, but not.code === "PLAN_V2_INTEGRITY_UNRECOVERABLE". Effect today: a bare-code throw of the v2 integrity error is still classified 400 (the code string is inNON_RETRYABLE_ERROR_NAMESatserver.ts:906), but a class throw emitsbody.error === "PlanV2IntegrityError"while a code throw emitsbody.error === "PLAN_V2_INTEGRITY_UNRECOVERABLE". Downstream Cloud Logging selectors / alerts that key offjsonPayload.nameneed to accept both strings. Extending the normalizer to includePLAN_V2_INTEGRITY_UNRECOVERABLE(and by symmetryFORMAT_NOT_SUPPORTED_IN_DISTRIBUTED) would give the wire discriminator a single canonical form per error class, matching how AWS bundles both aliases in the state-machine non-retryable lists. Non-blocking — HTTP classification is correct today. -
(P3, lens E)
handler_errordoes not carryplanProtocol/producerVersion.server.ts:128-134emitsaction,message,name; the correlatedhandler_start(line 107) viasummarizeEvent(line 236-269) does carryplanProtocol. Pre-existing pattern; consider adding a follow-up to attachplanProtocoltohandler_errorso a single Logs Explorer row is triage-complete without a request-ID join.
Verdict: APPROVE.
— Via
miguel-heygen
left a comment
There was a problem hiding this comment.
Fresh exact-head review at b6664db3af4a7c1de30917fd799554e8fa1328f8.
The GCP rollout preserves the useful invariants from the lower stack:
packages/gcp-cloud-run/src/server.ts:136-181rejects mixed/missing v1/v2 locator shapes before work begins.packages/gcp-cloud-run/src/gcsTransport.ts:124-199uses a generation-zero precondition and verifies a concurrent winner before reuse, so CAS publication is genuinely create-only.packages/gcp-cloud-run/src/server.ts:901-923maps bothPlanV2IntegrityErrorandPLAN_V2_INTEGRITY_UNRECOVERABLEto HTTP 400, which the workflow does not retry.packages/gcp-cloud-run/terraform/workflow.yaml:26-292keeps v1 defaulted, v2 explicit, and plan/chunk/assemble locators disjoint throughout fan-out.
Audited: event unions, server dispatch/materialization/error mapping, GCS transport, SDK propagation, workflow/Terraform routing, and their focused tests.
Trusting: the 780-line live-smoke shell implementation beyond owner isolation/cleanup integration points, documentation, and sample-event prose.
No code blocker found in this slice. The downstack SAM classifier issue on #2789 must be fixed before merging the Graphite stack, but it is not introduced by this GCP diff.
Verification: GCP typecheck passed; server, GCS transport, SDK, workflow, and smoke-safety tests passed 51/51; git diff --check passed. Current GCP-focused preflight, parity, and performance checks are green; Graphite mergeability remains pending on the open downstack.
Verdict: APPROVE
Reasoning: The exact-head GCP adapter implements atomic CAS publication, fail-closed protocol routing, target-scoped materialization, and terminal class/code handling with focused regression coverage.
— Magi
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at b6664db3af4a7c1de30917fd799554e8fa1328f8 via /code-review max on the 27-file diff.
LGTM. The GCP mirror of the #2789 classifier work lands cleanly, and the HTTP-surface test explicitly pins both the class name and code alias — better test coverage than the AWS side (which pins at the CDK snapshot layer, one indirection removed from the runtime).
Verified clean
-
HTTP classifier wire-up.
NON_RETRYABLE_ERROR_NAMESatpackages/gcp-cloud-run/src/server.ts:900-922contains both"PLAN_V2_INTEGRITY_UNRECOVERABLE"(line 906) and"PlanV2IntegrityError"(line 914) with an explicit comment (line 909-911) explaining the class-name-plus-code-alias pattern. The routing atserver.ts:952—const status = name && NON_RETRYABLE_ERROR_NAMES.has(name) ? 400 : 500;— is straightforward. -
Load-bearing HTTP test.
server.test.ts:517-550"returns 400 for plan v2 integrity error names and code aliases"loops over BOTH a realnew PlanV2IntegrityError(...)ANDObject.assign(new Error(...), { name: PLAN_V2_INTEGRITY_UNRECOVERABLE })— drives Hono throughapp.request("/", …)at line 533, assertsres.status === 400at line 546. This is the direct AWS analog CI never had (no test on the AWS side asserts the classifier fires for the v2 error — see my #2789 C5). Nice. -
GCS transport CAS discipline.
gcsTransport.ts:82-99downloadGcsObjectToFileVerifiedcomputes sha256 post-download andrmSyncon mismatch → typedPLAN_ARTIFACT_DIGEST_MISMATCH. Upload path at:135-180usesifGenerationMatch: 0for create-only atomicity, handles the 412 race by re-verifying metadata of the winning object. Tests cover upload-then-reuse (gcsTransport.test.ts:129-135), digest-mismatch refusal (:137-150), CAS race (:152-168), and post-download digest cleanup (:170-184).assertSha256regex atserver.ts:706-712gates digest strings before they hitjoin()— no path traversal. -
Terraform workflow.
terraform/workflow.yaml:101-113validatePlanResultstep throwsPLAN_PROTOCOL_LOCATOR_MISMATCHon cross-protocol locator bleed.retryablepredicate at:304-318maps 400 non-retryable, 5xx/403/429/no-code retryable. Assertions atworkflow.test.ts:67-148pin OIDC audience + v1/v2 disjoint locators.smoke-safety.test.ts:9-70pins owner-hashed prefix, isolatedTF_DATA_DIR,[ "$STACK_NAME" != "hyperframes" ]guard against wiping the shared stack. -
v2 event shape parity with AWS.
events.ts:63-73, 94-114, 137-159uses the same discriminated-union pattern with?: nevermutex on locator fields. Cross-repo grep confirmsPlanV2ManifestGcsUri/PlanV2ArtifactGcsPrefix/PlanHashtriple appears at every consumer site (server dispatch, workflow YAML, sample events, SDK). -
v2 assemble ignores
AudioGcsUri. Consistent with AWShandleAssembleV2— audio comes from the materialized planDir. Sample event pinsAudioGcsUri: null. -
Smoke script hygiene (
examples/gcp-cloud-run/scripts/smoke.sh).set -euo pipefail, everygcloudcarries--project, all vars quoted, owner-hashed prefix, isolatedTF_DATA_DIRper invocation, refuses[ "$STACK_NAME" = "hyperframes" ]. New coverage: v1↔v2 decoded-frame + audio + normalized-metadata parity atsmoke.sh:589-662. Better negative-path coverage than the AWS side.
Concerns
M1 — normalizeTerminalErrorName at server.ts:178-184 doesn't include PLAN_V2_INTEGRITY_UNRECOVERABLE. Same defensive gap I flagged as #2789 C3 on the AWS handler. Currently line 178 only normalizes PLAN_PROTOCOL_UNSUPPORTED and PLAN_TOO_LARGE. Not a live bug — producer's PlanV2IntegrityError constructor sets error.name = "PlanV2IntegrityError" and that name is in the Set — but a future producer refactor that throws new Error(...) with only .code = "PLAN_V2_INTEGRITY_UNRECOVERABLE" would return Error as the name → HTTP 500 → workflow retries. One-line defense: extend the condition to include the v2 code. This is a mirror bug across both adapters — fixing in one place doesn't fix the other, so worth landing on both.
Nits
- N1 — Bare
as CloudRunEventatserver.ts:942:body = (await c.req.json()) as CloudRunEvent;. RuntimeunwrapEvent+validatePlanProtocolShapecatches malformed input, but the compile-time claim is a lie (a JSON literal likenulltype-checks). Convention (CONTRIBUTING.md:47,54) isas unknown as CloudRunEvent+ a one-line justification. Same atserver.ts:873and cursor casts at:143, 197, 200, 204. - N2 —
gcsTransport.ts:215hash.update(chunk as Buffer)bare cast.createReadStreamdefault encoding is Buffer, but the cast doesn't narrow. Preferif (Buffer.isBuffer(chunk)) hash.update(chunk); else hash.update(chunk, "utf8");. - N3 —
server.ts:731values[index]!non-null assertion. Safe under single-threaded JS after thewhile (cursor < values.length)gate, butCONTRIBUTING.md:55still flags this. Two-line rewrite without the!.
CI: required checks green; the single Perf failure is a ${{ matrix.shard }} placeholder that never fired (all real Perf: drift/fps/load/parity/scrub jobs pass). Not a code issue.
| if (candidate.code === "PLAN_PROTOCOL_UNSUPPORTED" || candidate.code === "PLAN_TOO_LARGE") { | ||
| candidate.name = candidate.code; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 DEFENSIVE — normalizeTerminalErrorName should include PLAN_V2_INTEGRITY_UNRECOVERABLE.
Lines 178-184 currently only normalize PLAN_PROTOCOL_UNSUPPORTED and PLAN_TOO_LARGE. Currently harmless because PlanV2IntegrityError's constructor (producer planV2.ts:63) sets error.name = "PlanV2IntegrityError" in addition to .code, and NON_RETRYABLE_ERROR_NAMES at line 914 has the class name.
Failure scenario: a future producer refactor emits new Error(...) with .code = "PLAN_V2_INTEGRITY_UNRECOVERABLE" and default .name = "Error" → HTTP layer at line 952 reads .name = "Error" → misses the Set → HTTP 500 → workflow's retryable predicate at terraform/workflow.yaml:304-318 retries a truly non-recoverable failure through all max_retries: 4 attempts.
Same defensive gap I flagged on #2789 C3 for AWS handler.ts:145-151. Mirror bugs across both adapters — a fix in one place doesn't cover the other. One-line change here:
if (
candidate.code === "PLAN_PROTOCOL_UNSUPPORTED" ||
candidate.code === "PLAN_TOO_LARGE" ||
candidate.code === "PLAN_V2_INTEGRITY_UNRECOVERABLE"
) {
candidate.name = candidate.code;
}b6664db to
4d165d7
Compare
5e12d68 to
5bf61d6
Compare
The base branch was changed.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-stamping at 4d165d794.
Delta since my prior LGTM at b6664db3 addresses the R1 M1 mirror-bug concern:
server.ts:182-186normalizeTerminalErrorNamenow coversPLAN_V2_INTEGRITY_UNRECOVERABLEalongside the two existing codes — symmetric with AWShandler.ts:148-152in merged #2789.server.test.ts:517-550adds theObject.assign(new Error, { code })shape to the throw-shape loop and tightens the assertion frombody.error).toBe(error.name)tobody.error).toBe(PLAN_V2_INTEGRITY_UNRECOVERABLE)— pins the exact HTTP response body regardless of whether producer throws a class, setsname, or setscode.server.ts:132addsinput: summarizeEvent(unwrapped)to thehandler_errorlog, matching AWS's observability surface. Reuses the existingsummarizeEventhelper atserver.ts:241(already used byhandler_start).
Everything from R1 still stands.
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head re-review at 4d165d79474049e85a43da011b753b75b20c3c75.
The remaining GCP adapter feedback is resolved. normalizeTerminalErrorName now maps PLAN_V2_INTEGRITY_UNRECOVERABLE to the stable workflow/HTTP discriminator before logging and rethrow (packages/gcp-cloud-run/src/server.ts:127-136,176-189). The HTTP regression drives the real app through class-based, name-alias, and code-only throw shapes and pins the same HTTP 400 body for all three (packages/gcp-cloud-run/src/server.test.ts:517-552), so deterministic integrity failures no longer vary by producer error construction.
The observability follow-up also lands cleanly: handler_error carries the same compact, non-PII event summary as handler_start, including planProtocol and the relevant v1/v2 locator (packages/gcp-cloud-run/src/server.ts:127-135,234-270).
Audited: the complete GCP normalization/HTTP-test/observability delta at this head and the restack boundary against merged #2789.
Trusting: the previously reviewed v2 GCS transport, Terraform workflow, and large live-smoke implementation beyond the integration points rechecked in this round.
Verification: GCP package tests passed 90/90; GCP typecheck, focused formatting, and git diff --check passed. Required exact-head CI is still running with no required failure, so merge should continue to wait for the required checks to finish green.
Verdict: APPROVE
Reasoning: All supported v2 integrity-error shapes now converge on one terminal 400 discriminator with direct HTTP coverage, and the GCP structured error log now carries the same protocol context as AWS.
— Magi
vanceingalls
left a comment
There was a problem hiding this comment.
Re-verification at head 4d165d794 — restack of the GCP adapter on freshly-merged #2789.
Verification of prior findings
- Via R1 P2 —
normalizeTerminalErrorNameinconsistent code-alias coverage. RESOLVED atpackages/gcp-cloud-run/src/server.ts:182-186:PLAN_V2_INTEGRITY_UNRECOVERABLEnow sits alongsidePLAN_PROTOCOL_UNSUPPORTEDandPLAN_TOO_LARGEin the code→name promotion. Both class-throw (via.code) and code-only-throw paths converge on the same normalized name before the HTTP encoder picks it up. - Via R1 P3 —
handler_errormissingplanProtocol/ triage context. RESOLVED atpackages/gcp-cloud-run/src/server.ts:129-135:input: summarizeEvent(unwrapped)is now attached, so every error row carriesplanProtocol+ routable event fields without a request-ID join. Symmetric with the AWShandler_errorpayload landed in #2789. - Rames R1 M1 — mirror bug: normalizer omits v2 integrity code. RESOLVED (same fix as Via P2 at
server.ts:182-186). - Rames R1 N1 — bare
as CloudRunEventatserver.ts:947. STILL PRESENT at 947, and same bare form at 205, 209 (obj.Payload as CloudRunEvent,obj.Input as CloudRunEvent). Rames's restamp explicitly notes "everything from R1 still stands"; P3 nit, not blocking. - Rames R1 N2 — bare
chunk as BufferatgcsTransport.ts:215. STILL PRESENT. P3 nit. - Rames R1 N3 —
values[index]!atserver.ts:736. STILL PRESENT. Safe under thewhile (cursor < values.length)gate. P3 nit. - Magi R1. No blockers raised; audited slice unchanged in shape.
Claim-by-claim verification
Claim 1: three-shape normalization
Verified. Trace at packages/gcp-cloud-run/src/server.ts:
- Class throw —
new PlanV2IntegrityError(...)sets both.name = "PlanV2IntegrityError"and.code = "PLAN_V2_INTEGRITY_UNRECOVERABLE"(producer contract).normalizeTerminalErrorNameat:182-186sees.codein the promotion set → rewrites.nameto"PLAN_V2_INTEGRITY_UNRECOVERABLE".NON_RETRYABLE_ERROR_NAMES.has(name)at:957→ status 400.c.json({error: name, message}, 400)at:960→body.error === "PLAN_V2_INTEGRITY_UNRECOVERABLE". - Name-alias throw (Error with
.name = "PLAN_V2_INTEGRITY_UNRECOVERABLE", no.code) — normalizer no-op (no.code),NON_RETRYABLE_ERROR_NAMEScontains the string at:911→ 400 +body.error = "PLAN_V2_INTEGRITY_UNRECOVERABLE". - Code-only throw (Error with
.code = "PLAN_V2_INTEGRITY_UNRECOVERABLE", no matching.name) — normalizer promotes.code→.namevia:187, then Set lookup → 400 + same body.
All three shapes converge on body.error === "PLAN_V2_INTEGRITY_UNRECOVERABLE" before the HTTP response is emitted, inside the handler — no caller-side routing on .code vs .name. Normalization runs at :128 (dispatch catch block) and at :955 (HTTP shell reads err.name), so the promoted name is what hits the wire.
Claim 2: HTTP test coverage + pinned body
Verified at packages/gcp-cloud-run/src/server.test.ts:517-553. The for (const error of [...]) loop drives all three shapes through the real Hono surface (app.request("/", ...) at :536) and asserts res.status === 400 + body.error === PLAN_V2_INTEGRITY_UNRECOVERABLE per iteration. The tightening since R1 (toBe(PLAN_V2_INTEGRITY_UNRECOVERABLE) instead of toBe(error.name)) is what locks the wire discriminator to a single canonical string across all three throw paths — a body-shape drift between shapes would now fail.
One small observation, not a block: the assertion pins body.error and res.status but not the whole body via toEqual; body.message is left implicit. All three throws share "corrupt test artifact" so the messages line up, but a stricter toEqual({error, message: "corrupt test artifact"}) would catch a future accidental field addition/rename to the response envelope. P3.
Claim 3: handler_error observability parity with AWS
Verified. Payload at server.ts:129-135 now emits:
event: "handler_error"action: unwrapped.Actioninput: summarizeEvent(unwrapped)— new at headmessage,name
summarizeEvent at :241-273 produces {planProtocol, projectGcsUri | planGcsUri | planV2ManifestGcsUri, chunkIndex, chunkCount, hasAudio, outputGcsUri, format, ...} — routable metadata only, no project payload, no credentials.
Field-by-field vs AWS #2789 handler_error payload (per merged #2789 context: action + summarizeEvent(unwrapped) with planProtocol on every branch): parity holds. producerVersion remains absent from both handler_start and handler_error on both adapters — it lives only on the plan result — consistent asymmetry, not a GCP regression.
Cross-adapter drift check
AWS state-machine NON_RETRYABLE_PLAN / _CHUNK / _ASSEMBLE (packages/aws-lambda/src/cdk/HyperframesRenderStack.ts:196-238) vs GCP NON_RETRYABLE_ERROR_NAMES (packages/gcp-cloud-run/src/server.ts:905-927):
Every AWS entry present on GCP:
FFMPEG_VERSION_MISMATCH,PLAN_HASH_MISMATCH,BROWSER_GPU_NOT_SOFTWARE,FONT_FETCH_FAILED,PLAN_TOO_LARGE,PlanTooLargeError,PLAN_PROTOCOL_UNSUPPORTED,PlanProtocolUnsupportedError,PLAN_V2_INTEGRITY_UNRECOVERABLE,PlanV2IntegrityError,PLAN_ARTIFACT_DIGEST_MISMATCH,FORMAT_NOT_SUPPORTED_IN_DISTRIBUTED,ChromeBinaryUnavailableError— all match.
Adapter-appropriate substitutions:
- AWS
S3_URI_NOT_ALLOWED↔ GCPGCS_URI_NOT_ALLOWED— correct.
Defensive GCP superset (not a regression):
FormatNotSupportedInDistributedErrorclass name (only code on AWS).RenderChunkValidationErrorclass name (AWS omits — GCP-only, so a producer switching from code to class throw would terminate on GCP but retry on AWS; that's an AWS gap to file, not a GCP block).
Handler-side normalizeTerminalErrorName: AWS covers the same three codes at packages/aws-lambda/src/handler.ts:148-152; GCP mirrors exactly at server.ts:182-186. FORMAT_NOT_SUPPORTED_IN_DISTRIBUTED remains an asymmetric alias on both sides (name/code coexistence but no normalization) — mirror symmetric, worth a stack-wide follow-up but out of scope here.
Cloud Workflows retryable predicate at packages/gcp-cloud-run/terraform/workflow.yaml:304-318: retries on missing .code, 429, 403, and 5xx; returns false otherwise → HTTP 400 from the handler is genuinely terminal.
Fresh 4-lens pass at 4d165d794
Standards
Bare as casts at wire boundaries confined to the three known nit sites (server.ts:205, 209, 947, gcsTransport.ts:215); one values[index]! at server.ts:736 inside a bounds-gated worker. Runtime unwrapEvent + validatePlanProtocolShape compensates for the JSON-boundary casts. Convention hits at CONTRIBUTING.md but not blocking per Rames's restamp.
Spec forward-check
PR body claim → diff:
- "opt-in Plan protocol v2" —
events.ts:73PlanEvent = PlanV1Event | PlanV2EventwithPlanV1Event.PlanProtocol?: "v1"andPlanV2Event.PlanProtocol: "v2"(required literal). ✅ - "manifest-last, content-addressed GCS artifact graph" —
gcsTransport.ts:135-180ifGenerationMatch: 0create-only CAS, 412 winner re-verify. ✅ - "materialize only chunk-scoped dependencies; audio only for assembly" —
server.test.ts:335asserts chunk worker does NOT downloadaudio.aac;:351-353asserts assembler does. ✅ - "keep v1 default, fail closed on mixed/malformed locators" —
server.ts:143-174+ workflowvalidatePlanResultatworkflow.yaml:101-113. ✅ - "SDK/workflow propagation, v2 sample events" — new fixtures
sample-events/plan-v2.json,render-chunk-v2.json,assemble-v2.json; SDK atsdk/renderToCloudRun.ts:+7. ✅ - "owner-isolated real-GCP v1/v2 parity smoke path" —
scripts/smoke.sh+606/-174,smoke-safety.test.ts+71 new; owner-hashedSTACK_NAME, isolatedTF_DATA_DIR,verify_absentfor every provisioned resource. ✅
Spec reverse-check
No undisclosed adapter changes in the diff. Terraform additions (main.tf:+5, outputs.tf:+10, variables.tf:+9, workflow.yaml:+149/-10) all in service of v2 wiring. .fallowrc.jsonc:+7 and bun.lock churn are routine. Dockerfile:+1 is the scripts/package-subpaths.mjs copy the smoke script's tracked-artifact check pins at smoke-safety.test.ts:53-54.
Sibling-precision divergence
max_retries: 6 (plan) × 2 sites, max_retries: 4 (chunk/assemble) × 4 sites — pinned by workflow.test.ts:76-77. Uniform 1800s timeout. No numeric drift between AWS Step Functions (maxAttempts at HyperframesRenderStack.ts) and GCP Workflows in the retry contract as re-verified through prior R1.
Middle-man wrap/unwrap
dispatch at server.ts:103-138 is the single core; handlePlan(V2?)/handleRenderChunk(V2?)/handleAssemble(V2?) are thin routers keyed on event.PlanProtocol === "v2" at :292, ~429, ~557 (per R1 lens D). No caller re-wraps normalized error names — normalization sits at the boundary once.
Behavior + editor-UI lens complement
Silent-catch + error-invariant
dispatch catch block at :127-137 normalizes and re-throws — no swallow. PlanV2IntegrityError maps identically whether thrown as class, name-alias, or code-only (see Claim 1). Retryable-vs-terminal partition consistent with Cloud Workflows retryable predicate.
Concurrency/lifecycle
Hono app.request handles a single POST per invocation. mapConcurrent at :727-740 bounds worker concurrency; cursor++ is safe under single-threaded JS event loop. gcsTransport CAS discipline uses ifGenerationMatch: 0 + 412 handling for genuine create-only publication.
PR-body-vs-diff parity
Matches. Minor: PR body says "80 GCP adapter/workflow/smoke-safety tests" while Rames's restamp said 74/74 — a run-count difference, not a code issue.
Perf audit
No O(n²) hotspots. Streaming sha256 in gcsTransport.ts avoids full-buffer allocation. Same Cloud Run cold-start surface as v1 path.
Verdict
APPROVE.
Prior blocker resolved: normalizeTerminalErrorName at server.ts:182-186 covers PLAN_V2_INTEGRITY_UNRECOVERABLE, the HTTP test at server.test.ts:517-553 pins body.error === "PLAN_V2_INTEGRITY_UNRECOVERABLE" across class / name-alias / code-only throw shapes, handler_error at server.ts:129-135 includes input: summarizeEvent(unwrapped) matching AWS observability, and the GCP terminal-name list is a defensive superset of AWS with adapter-appropriate GCS_URI_NOT_ALLOWED substitution. Three R1 nits (bare as CloudRunEvent, chunk as Buffer, values[index]!) still present but explicitly non-blocking per Rames's restamp and my own R1 read. Downstack #2789 has merged, so mergeability blocks only on remaining stack ancestors.
— Via

Summary
Stack
Safety
plan call gets the longer IAM-propagation horizon, while chunk/assemble keep
their original retry count
Validation
Live GCP gate
Environment:
hyperframes-dev,us-east1, owner hash1315a5c6e8.The first live attempt exposed Cloud Run IAM propagation longer than the
original retry horizon. The final run exercised a newly-created private
service and workflow from cold state:
SUCCEEDED, 114,236 ms, 37.40 dB PSNRSUCCEEDED, 12,059 ms, 37.40 dB PSNRfe3bf65710fd0a362901dbecdfc50f22f5c4d974cbf7723365ad133cb65c929dNO_AUDIOfixture)ac64dc35c1cfb59e7c8aaddad900e15d406d9e9ae465a437e45c09a99fe4dedfAutomatic cleanup passed for the successful run. A separate read-only audit
checked Cloud Run, Workflows, both service accounts, render/build buckets,
Artifact Registry repositories, and image packages for the successful and
earlier failed owner hashes (
1315a5c6e8,ac116fa597,4926f2dcc6,590d3dd8f2): 32/32 resources were absent.Evidence is retained locally under
examples/gcp-cloud-run/scripts/gcp-smoke-artifacts/1315a5c6e8/; GCP retainsonly Cloud Build and Cloud Logging/audit history.