Repository navigation
fix(review): resolve findings from omni-code-review battery (release/v3.8.51 vs main, api/v1 + image handlers) - #14365
Merged
Conversation
The /v1/images/upscale call-log sink rendered error: unknown with a bare String(opts.error), which throws on the null-prototype objects sanitizeUpstreamDetails() produces and forwarded credential-bearing Error messages verbatim. Lift the generation sink's total stringifier into open-sse/handlers/imageErrorLog.ts, add credential redaction, and use it from imageUpscale/shared.ts.
…(omni-code-review LEDGER-22 follow-up)
diegosouzapw
added a commit
that referenced
this pull request
Sep 21, 2026
… own file (file-size gate) tests/unit/image-generation-handler.test.ts is frozen at 2235 lines by check:file-size; commit 8a587a7 grew it to 2389. The nine new cases (LEDGER-6/24/28/29 image_size outcomes, LEDGER-12 hasUsableImage) now live in tests/unit/image-generation-size-and-payload-guard.test.ts; the original file is back to 2232 lines with its other content unchanged.
diegosouzapw
force-pushed
the
fix/omni-code-review-e2e-areas
branch
from
September 22, 2026 01:08
5ac500e to
7107cb7
Compare
The LEDGER-3/9 source-text guard in tests/unit/batch-delete-completed-ownership-wvxc.test.ts matched prose as well as code: its `\bvalidateApiKey\s*\(` alternative was one comment reflow (or a `@see validateApiKey(key)`) away from failing the suite with a message that falsely claims the route re-runs the lifecycle check. - Extract the guard into `routeReRunsValidateApiKey(src)` and strip block and line comments (`/\/\*[\s\S]*?\*\/|\/\/.*$/gm`) before matching, so the assertion is about code only. - Add a fixture case that feeds the guard a comment containing `validateApiKey (` and `@see validateApiKey(key)` — RED against the old regex, GREEN after the strip — plus a mutation check proving a real import or call in code still trips it. The route file is untouched.
…/67 (omni-code-review round 3)
LEDGER-47: Antigravity call-log test fixture returns a real inlineData candidate and
asserts result.data.data.length === 1, instead of an empty candidates array.
LEDGER-48: normalizeImageGenerationSize now returns { value, clamped } so an
unrecognised image_size logs a log.warn line naming the raw value and the clamp.
LEDGER-49: stringifyImageErrorForLog's object branch is pinned against a
credential-bearing null-prototype payload, asserting redaction to [REDACTED].
LEDGER-50: added fetchUntrustedRemoteImage() to src/shared/network/remoteImageFetch.ts
and switched imageGeneration.ts and imageUpscale/shared.ts to call it, removing the
per-file duplicated public-only/pinDns option literal and rationale comment.
LEDGER-52: imageErrorLog.ts's relative import of redactSensitiveErrorText now carries
the explicit .ts extension used by every other relative import in open-sse/handlers/.
LEDGER-54: captureAntigravityImageRequest now reads the persisted call-log row's
requestBody (image_size / image_size_applied) via getCallLogById, not just the
upstream envelope.
LEDGER-56: stringifyImageErrorForLog's Error branch renders name/message
defensively inside the same safety net as the other branches, so a non-string
name/message or a throwing message getter cannot escape as an unhandled throw.
LEDGER-57: normalizeImageGenerationSize's docblock hedges the 1K/2K/4K enum and the
clamp rationale as observed behaviour (#11952), not a confirmed upstream contract.
LEDGER-58: logRequestBody records both image_size (raw caller value, null when
absent) and image_size_applied (what went upstream, "default" when omitted), so the
persisted call-log body never silently drops the caller's original value.
LEDGER-59: same call-log fix as LEDGER-58 (two ledger findings, one change).
LEDGER-60: same call-log fix as LEDGER-58 (two ledger findings, one change).
LEDGER-63: imageErrorLog.ts's relative imports use explicit .ts extensions, matching
every other relative import in open-sse/handlers/.
LEDGER-64: imageUpscale/shared.ts imports imageErrorLog.ts and utils/error.ts with
explicit .ts extensions, matching the sibling import in imageGeneration.ts.
LEDGER-66: the non-object-item 502 test case in image-generation-handler.test.ts now
also asserts the "without an image payload" message, pinning isJsonObject(item)
rather than any 502 from the outer catch.
LEDGER-67: the new LEDGER-12/LEDGER-6 test comments are qualified with the
omni-code-review run id, avoiding a collision with other runs' LEDGER numbering.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Resolves the approved FIX / FIX+TEST items of the
/omni-code-reviewbattery run2026-09-21_release-v3.8.51_vs_main_e2e-areas(15 lenses oversrc/app/api/v1/{_helpers,files,batches}+open-sse/handlers/imageGeneration.ts,imageUpscale; 102 raw findings → 45 ledger entries; run record in_tasks/reviews/omni-code-review/2026-09-21_release-v3.8.51_vs_main_e2e-areas/). This is Task 3 of the family's E2E validation plan.11d71e9getApiKeyRequestScopesurfaces an additivekeyState: "none" | "unresolved" | "invalid" | "valid";DELETE /v1/batches/delete-completeddrops its redundantvalidateApiKeyre-check (the helper is the single lifecycle gate since #13881), its comment now tells the truth and the auditreasonis honest (invalidvsunresolved, previously unreachable). Tests: helperkeyStatecases, audit reason for revoked vs unknown key, source guard that the route no longer importsvalidateApiKey.8a587a7HTTP_STATUS.GONE;image_sizeis forwarded only when supplied (no silent1Kdefault), invalid values still clamp to1K, camelCase alias dropped, doc comment; tests for the fourimage_sizeoutcomes and forhasUsableImage(blankurl, non-object item, well-formed, edit path); SSRF option bag hoisted into one constant; split imports merged.25951c1+6167c0aString(opts.error)(crashed on the sanitizer's null-prototype objects): new sharedopen-sse/handlers/imageErrorLog.ts(stringifyImageErrorForLog+redactSensitiveErrorText), used by both image sinks;imageGeneration.tsdrops its private copy. Tests: Authorization value masked, null-prototype payload does not throw, source guard against re-duplication.DISCUSS items of the same run were opened as issues instead of fixed here: #14332 (anonymous/revoked-key writes create null-owner rows), #14333 (candidates route + key×cookie precedence), #14334 (dashboard ignores
hasMore), #14335 (five image-gen follow-ups).Validation
Per commit: focused suites RED → GREEN (
batches-delete-completed-route-scope11→13/13,batch-delete-completed-ownership-wvxc5→6/6,image-generation-handler50→59/59,image-upscale-error-lognew 7/7,image-upscale34/34),npx prettier --checkandnpx eslint --suppressions-location config/quality/eslint-suppressions.jsonclean on every changed file,npm run typecheck:coreandnpm run check:open-sse-typecheckclean. Re-review rounds of the battery over this diff are recorded in the run directory (round 2 = full roster, 671 changed lines).No production behaviour changes beyond the ledger items: revoked/expired/banned keys still get 401 on the sweep route (pinned by the existing tests);
image_sizeomitted now lets the upstream default apply (previously forced to1K— LEDGER-6)..env.example), reproduces on the base tip.