fix: Integration gaps — mail, auth, images, CDN, bare errors - #34
Merged
Conversation
The SMTP and Resend transports shipped, but nothing constructed them: `x dev` — which is also the production boot, one role per container — hardcoded `createMemoryDriver()`, `createSmtpDriver`/`createResendDriver` had zero callers outside tests, and mail could not be delivered from any deployment. `selectMailDriver(env)` is the one answer to which transport a boot installs, following the same law as the database, event bus and storage bindings: an unset variable means the embedded default. `SMTP_URL` or `RESEND_API_KEY` selects a real transport, both at once is refused rather than resolved, and a transport without `MAIL_FROM` fails at boot instead of on the first send. Selection runs before the queue, so a bad credential never costs a PGlite boot. The credential never reaches a printed string — `x dev` reports the env key that selected the transport (`mail=external(smtp via SMTP_URL)`), because `SMTP_URL` carries a password and that line is logged and scraped. `/_x`'s mail panel degrades instead of lying: only the memory driver retains an outbox, so with a real transport the hook is omitted and the panel shows its wiring note rather than an empty list claiming nobody was mailed. `wiki/Configuration.md` documented `mail.from`/`mail.driver`/`mail.url` as `app.config.ts` fields. Nothing loads that file's contents at runtime, so those settings could never have been read; the section now documents the env keys that are actually honoured. Co-Authored-By: Claude <noreply@anthropic.com>
The authorization-code exchange itself shipped in 9c1e105, but a github or google login still could not finish: `beginOAuth`'s state, nonce and PKCE verifier have to survive a redirect into a separate callback request, and the framework offered nowhere to put them. The README's own example said "store the handshake in a short-lived signed cookie" as a comment over an API that did not exist, so every app would have hand-rolled that store — which is where PKCE quietly stops proving anything. - `oauth-cookie.ts`: `sealHandshake`/`openHandshake` (one codec, cookie or server-side store) plus `handshakeCookie`/`readHandshakeCookie`/ `clearHandshakeCookie`. Signed with `SESSION_SECRET`, `__Host-` + `HttpOnly; Secure; SameSite=Lax`, expired against the server's clock rather than the client's copy of `Max-Age`. - `openHandshake` takes the provider for the reason `decodeCursor` takes a scope: an optional check is one a call site forgets. Every rejection is `X_OAUTH_STATE_INVALID`, matching `assertOAuthCallback` — no new code. - `session.ts`: one `Cookie:` parser (`readCookie`), two callers. 17 tests, including a github login driven across two requests carrying only the cookie. Negative-proved: dropping the signature check, the provider binding or the server-side ttl each reddens its own test. Co-Authored-By: Claude <noreply@anthropic.com>
The pipeline shipped in core and storage/seo/pwa each call it — but none of them had a production call site, so a `srcset` URL and a manifest icon both named bytes nothing could produce. - seo: `IMAGE_QUERY_KEYS` + `parseImageQuery` — the reader for the URL `defaultUrlFor` already minted. An unusable `?w=`/`?q=` is `X_IMAGE_QUERY_INVALID`, never a silent fall back to the original. - cli: `dev-assets.ts` mounts the pipeline's only HTTP surface — `/icons/*` from pwa's `planIcons` + `BuiltinImagePipeline`, `/media/*` through seo's driver, cached by storage's `variantKey`. Core stays the only scaler. - cli: `x new` scaffolds `icon.png`, not `icon.svg` — core decodes PNG/JPEG only, so the old source could never become an icon. `GeneratedFile` carries bytes; the icon is built with core's own encoder, never a checked-in binary. - `ICON_SOURCE` moves beside the code that reads it, so `x doctor` cannot pass while `x dev` serves nothing. - http: `X_IMAGE_QUERY_INVALID` -> 400, `X_IMAGE_UNSUPPORTED` -> 415. Both were unmapped 500s blaming the server for the caller's query. Proven live: `x dev` answers /icons/icon-192.png, maskable-512, apple-touch and the exact srcset URL `responsiveImage()` mints, at the right sizes. Co-Authored-By: Claude <noreply@anthropic.com>
- fastlyPurgeDriver: batch surrogate-key purge (256/call) + purge_all - cloudflarePurgeDriver: cache-tag purge (30/call) + purge_everything; a 200 carrying success:false is a refusal, not a completed purge - purge-http.ts holds the one retryable table, the batching and the key guard — a key with whitespace or a comma is refused before the request, because a CDN splits on both and answers 200 having cleared nothing - selectPurgeDriver reads the environment (FASTLY_* / CLOUDFLARE_*), never app.config.ts; two credentials or half a pair are refused, not resolved - x dev registers the cdn tier when a credential names a real edge, so invalidates: [tag.post] reaches the edge — M7's fifth leg. A noop tier stays unregistered rather than reporting keys no edge accepted - X_CACHE_PURGE_FAILED replaces the X_NOT_IMPLEMENTED stub; cache now borrows no code from core Co-Authored-By: Claude <noreply@anthropic.com>
- pwa: strategies.ts fallback exhaustion, background-sync.ts flush failures (browser SW runtime, code carried as text since it cannot import @ultimat3/core) - testing: evalTest threshold, schema/job matcher misuse, sealed network race - ai: embedOne batch-size invariant - new codes X_PWA_STRATEGY_EXHAUSTED, X_PWA_SYNC_FLUSH_FAILED, X_PWA_SYNC_INCOMPLETE, X_TEST_EVAL_THRESHOLD, X_TEST_SCHEMA_EXPECTED, X_TEST_JOB_EXPECTED, X_TEST_NETWORK_RACE, X_AI_EMBEDDER_INVALID, each with a runnable fix and a wiki/Error-Codes.md row Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (53)
📝 WalkthroughWalkthroughThis change adds typed errors, signed OAuth handshake cookies, environment-selected mail and CDN drivers, surrogate-key purging, responsive image parsing, development asset routes, and deterministic PNG scaffolding. ChangesTyped error contracts
OAuth handshake cookies
CDN purge drivers
Development runtime and assets
Responsive image queries
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant OAuthCallback
participant HandshakeCookie
participant OAuthProvider
participant Session
Browser->>OAuthCallback: Send callback with handshake cookie and code
OAuthCallback->>HandshakeCookie: Read and open sealed handshake
HandshakeCookie-->>OAuthCallback: Return validated state and PKCE verifier
OAuthCallback->>OAuthProvider: Exchange code and verifier
OAuthProvider-->>OAuthCallback: Return OAuth identity
OAuthCallback->>Session: Set session cookie and clear handshake cookie
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/src/errors.ts`:
- Around line 344-353: Replace the prose fix values in AiEmbedderInvalidError at
packages/ai/src/errors.ts:344-353, the X_PWA_STRATEGY_EXHAUSTED error at
packages/pwa/src/errors.ts:121-131, and all four typed errors at
packages/testing/src/errors.ts:150-199 with accepted actionable forms: exact
runnable commands, pasteable call expressions, or named files; preserve each
error’s existing code and cause while making every fix executable.
In `@packages/auth/src/oauth-cookie.ts`:
- Around line 146-153: Update handshakeCookie to derive its default cookie name
from the handshake provider instead of the shared OAUTH_HANDSHAKE_COOKIE, while
preserving options.name as an explicit override. Update clearHandshakeCookie and
readHandshakeCookie to accept and use the provider argument consistently so all
handshake cookie operations target the provider-scoped name.
In `@packages/auth/src/session.ts`:
- Around line 220-231: Update readCookie to catch URIError from
decodeURIComponent and return the original trimmed cookie value instead;
preserve normal decoding for valid values and the existing null behavior for
missing cookies. This lets readHandshakeCookie and readSessionCookie perform
their existing coded validation instead of propagating a bare decoding error.
In `@packages/cache/src/purge-env.ts`:
- Around line 66-71: Update the conflicting-CDN validation around the FASTLY_*
and CLOUDFLARE_* environment checks so ConfigInvalidError.meta.selected contains
the actual non-empty configured key names, including service/zone IDs when
tokens are absent, instead of always reporting the API token keys; preserve the
existing error cause and fix guidance.
In `@packages/cache/src/purge-fastly.ts`:
- Around line 40-50: Update fixFor() in packages/cache/src/purge-fastly.ts
(lines 40-50) so every status branch returns a safe, executable diagnostic or
documented framework command without exposing credentials. Update requirePair()
in packages/cache/src/purge-env.ts (lines 43-50) and the conflicting-CDN branch
(lines 66-71) to return executable remediation commands as well; ensure every
resulting throw retains a stable X_* code, a cause, and an exact fix command.
In `@packages/cache/src/purge-http.test.ts`:
- Line 72: Replace the sentinel bare-Error paths with assertion-based failures
in packages/cache/src/purge-http.test.ts:72-72,
packages/cache/src/purge-cloudflare.test.ts:51-51, and
packages/cache/src/purge-env.test.ts:17-17, preserving each test’s expected
failure assertion. In packages/cache/src/purge-http.test.ts:147-147, make the
mocked transport reject with the established typed error fixture rather than
constructing Error directly; any remaining throw must use a stable X_* code,
cause, and executable fix.
In `@packages/cache/src/purge-http.ts`:
- Around line 30-35: Validate size at the start of chunked, requiring a positive
finite integer before entering the loop; reject 0, -1, NaN, and other invalid
values. Throw the established UltimateError subclass with a stable code, cause,
and runnable fix, preserving normal chunking for valid sizes and adding coverage
for the specified invalid cases.
In `@packages/cli/src/cmd-doctor.ts`:
- Line 109: Update the icon validation error’s fix instruction in the doctor
command to contain one executable command that creates the required 1024x1024
square PNG for the current app, rather than descriptive prose or the incomplete
x new invocation. Preserve the stable X_* error code and include the command as
the exact fix carried by the thrown error.
In `@packages/cli/src/dev-assets.test.ts`:
- Around line 21-30: Update the png fixture helper to avoid assigning literal
RGBA channel values; use the default raster pixel bytes or an existing framework
colour token while preserving valid PNG generation through encodeImage.
In `@packages/cli/src/dev-assets.ts`:
- Around line 1-5: Shorten the file header comment to at most four lines while
retaining its explanation that the module defines the two base paths used by the
development server and does not handle image processing.
- Line 7: Add a nearby comment at packages/cli/src/dev-assets.ts:7-7 explaining
why node:path is required to resolve ICON_SOURCE against the application root.
Also add a nearby comment at packages/cli/src/dev-assets.test.ts:7-9 explaining
why the imported Node APIs are required for isolated temporary test roots and
cleanup; no other changes are needed.
In `@packages/cli/src/dev-dashboard.test.ts`:
- Around line 227-229: Remove the duplicate trailing .catch(...) chain in the
devSources(...).mail() expression within the test, leaving only one
error-capture handler so the file parses correctly.
In `@packages/cli/src/dev-runtime.test.ts`:
- Around line 6-8: Add a concise comment beside the node:fs, node:os, and
node:path imports explaining that the real-boot test requires Node filesystem
temporary-directory and cleanup APIs because no suitable Bun-native test helper
is available. Keep the existing imports and test behavior unchanged.
- Around line 38-45: Replace both fake mail-driver Promise.reject(new
Error('unused')) implementations in packages/cli/src/dev-runtime.test.ts lines
38-45 and the external-driver rejection in
packages/cli/src/dev-dashboard.test.ts lines 77-81 with an existing
UltimateError subclass configured with a stable X_* code, a cause, and an exact
runnable fix command; preserve the drivers’ rejected-Promise behavior and do not
use bare Error or any.
In `@packages/cli/src/dev-runtime.ts`:
- Around line 154-158: Update the async stop() shutdown sequence so
resetTiers(), resetMailDriver(), and queue.stop() always execute even when
transport.close() rejects. Preserve the first shutdown failure and rethrow it
after all cleanup steps have been attempted.
- Around line 52-66: Move the human-facing mail and CDN label strings out of
describeMail and describeCdn into messages.ts, exposing them through the
existing msg() mechanism. Update cmd-dev.ts to render these labels via msg(),
while preserving the underlying stable status values used by --json and the
current embedded/external/none semantics.
In `@packages/cli/src/templates/scaffold-icon.ts`:
- Line 25: Update MARK_RGBA to use the existing lower-tier framework colour
token import instead of literal RGBA components. Reuse the framework token
directly and do not define another colour token in scaffold-icon.ts.
In `@packages/mail/src/driver-env.test.ts`:
- Around line 13-20: Replace the bare Error thrown by the thrown helper with the
test framework’s assertion for the expected-throw path, and update the
impostor.send rejection around the referenced lines to use a coded UltimateError
subclass with stable code, cause, and runnable fix fields. Ensure no bare Error
or TypeError remains in the affected test flows.
In `@packages/mail/src/driver-env.ts`:
- Around line 37-41: Update the fix values in the ConfigInvalidError instances
for the MAIL_FROM validation branches around selectedBy and the related checks
so each is an exact executable command for the supported deployment or
environment-file workflow, rather than prose. Preserve the existing cause and
metadata, and ensure all affected errors retain their stable X_* code while
providing runnable commands for setting or unsetting the relevant variables.
In `@packages/mail/src/driver.ts`:
- Around line 121-122: Update isMemoryDriver to validate every required
MemoryMailDriver member, including sent, lastTo, and clear, before returning
true; preserve the type predicate as MemoryMailDriver only when the complete
contract is present. Alternatively, narrow the predicate’s return type to a
dedicated outbox-only interface if that is the intended contract.
In `@packages/pwa/src/background-sync.ts`:
- Around line 8-13: Replace the bare Error throws in the generated
service-worker code around X_PWA_SYNC_FLUSH_FAILED and X_PWA_SYNC_INCOMPLETE
with a local coded error class or factory. Ensure each thrown error exposes
code, cause, fix, and docs, and provide an actionable fix specific to its
failure while preserving the existing failure conditions and messages.
In `@packages/seo/README.md`:
- Around line 99-104: Update the README route example’s images.transform call to
forward the parsed query.quality value alongside width and format, preserving
the existing fallback behavior for query.width.
In `@packages/seo/src/images.test.ts`:
- Around line 85-115: Replace every literal image-query key in these tests with
the corresponding properties from IMAGE_QUERY_KEYS, including URLSearchParams
inputs and the cause assertion in the fix-line test. Update the width, quality,
and format cases while preserving all existing invalid-value coverage and
expectations.
In `@packages/seo/src/images.ts`:
- Around line 102-104: Update parsePositiveInt to validate the parsed number
with Number.isSafeInteger and reject unsafe values using the existing
imageQueryInvalid error; preserve the positive-integer format check and add
coverage for a 400-digit string such as '9'.repeat(400).
In `@wiki/Configuration.md`:
- Around line 127-131: Unify the environment error-code contract: choose one
stable code for incomplete or conflicting CDN environment configuration, then
update selectPurgeDriver() and both Configuration.md sections to use and
describe that same code. Preserve X_CACHE_PURGE_FAILED for provider refusal and
ensure operators can reliably distinguish configuration failures from runtime
purge failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b8973d6-9bd8-4f27-b23e-3f28fb95a266
📒 Files selected for processing (70)
docs/idea/05-caching.mdframework.manifest.jsonpackages/ai/src/embeddings.tspackages/ai/src/errors.tspackages/auth/CLAUDE.mdpackages/auth/README.mdpackages/auth/src/index.tspackages/auth/src/oauth-cookie.test.tspackages/auth/src/oauth-cookie.tspackages/auth/src/session.tspackages/cache/CLAUDE.mdpackages/cache/README.mdpackages/cache/src/cdn.test.tspackages/cache/src/cdn.tspackages/cache/src/errors.test.tspackages/cache/src/errors.tspackages/cache/src/index.tspackages/cache/src/purge-cloudflare.test.tspackages/cache/src/purge-cloudflare.tspackages/cache/src/purge-env.test.tspackages/cache/src/purge-env.tspackages/cache/src/purge-fastly.test.tspackages/cache/src/purge-fastly.tspackages/cache/src/purge-http.test.tspackages/cache/src/purge-http.tspackages/cli/CLAUDE.mdpackages/cli/package.jsonpackages/cli/src/cmd-dev.tspackages/cli/src/cmd-doctor.test.tspackages/cli/src/cmd-doctor.tspackages/cli/src/cmd-generate.tspackages/cli/src/cmd-new.test.tspackages/cli/src/dev-assets.test.tspackages/cli/src/dev-assets.tspackages/cli/src/dev-dashboard.test.tspackages/cli/src/dev-dashboard.tspackages/cli/src/dev-runtime.test.tspackages/cli/src/dev-runtime.tspackages/cli/src/index.tspackages/cli/src/mcp-host.tspackages/cli/src/templates/naming.tspackages/cli/src/templates/scaffold-app.tspackages/cli/src/templates/scaffold-icon.tspackages/cli/tsconfig.jsonpackages/http/src/error-map.test.tspackages/http/src/error-map.tspackages/mail/CLAUDE.mdpackages/mail/README.mdpackages/mail/src/driver-env.test.tspackages/mail/src/driver-env.tspackages/mail/src/driver.tspackages/mail/src/index.tspackages/pwa/CLAUDE.mdpackages/pwa/src/background-sync.tspackages/pwa/src/errors.tspackages/pwa/src/strategies.tspackages/seo/CLAUDE.mdpackages/seo/README.mdpackages/seo/src/errors.tspackages/seo/src/images.test.tspackages/seo/src/images.tspackages/seo/src/index.tspackages/storage/CLAUDE.mdpackages/testing/src/errors.tspackages/testing/src/matchers.tspackages/testing/src/sealed-network.tspackages/testing/src/test-types.tswiki/Caching-And-Invalidation.mdwiki/Configuration.mdwiki/Error-Codes.md
CI (`live` step of `x verify`): both cases in packages/realtime/src/pg-replication.live.test.ts shared one replication slot, so the resume case inherited whatever the decode case left unconfirmed on it — a change delivered but not yet confirmed when a feed stops is legitimately re-sent to whoever opens that slot next, which made "exactly two" an assertion the contract never promised. Each case now owns its slot. The same file read `pg_replication_slots.active` once, immediately after `stop()`; the flag flips when the server's walsender exits, so it is polled now. Measured, not guessed: 20/20 clean runs of the full live suite, from 1-in-8 red. Review comments, 26 threads: per-provider OAuth handshake cookie; `readCookie` no longer lets a malformed percent-escape throw a bare URIError through the callback leg; `chunked()` refuses a batch size that would spin forever; `selectPurgeDriver` reports the CDN keys actually set instead of a hardcoded pair; `isMemoryDriver` checks every member it narrows to; the generated service worker throws a locally-defined coded error rather than a bare one; image widths past `Number.isSafeInteger` are refused; `stop()` releases every service even when the transport will not close; CLI boot labels render through `messages.ts` while `--json` keeps its stable status values; bare `Error`s swept out of cache/cli/mail; prose `fix:` lines that carried no command token rewritten. `X_CONFIG_INVALID` now has one documented meaning — a configuration that cannot boot, env or `app.config.ts` — across wiki and docs/architecture. Three findings answered rather than applied, each measured: the duplicated `.catch(...)` does not exist (the file parses); `cli` cannot import `@ultimat3/ui` (`checkTier` says same-tier, and declared edges do not transit), so `MARK_RGBA` stays with a sharper comment; mail's `.env.production` fix lines already match the repo's own contract and were not degraded. `bun run verify` 17/17 green with live Postgres/pgvector/NATS. `bun run test` 4174 pass / 0 fail. Reference-app ratchet unchanged at 10/17. Co-Authored-By: Claude <noreply@anthropic.com>
The placeholder mark was `[113, 113, 122, 255]` — Tailwind zinc-500, which is a palette value recreated as literal RGBA components however neutral it looks, and the comment claiming it copied nothing was therefore wrong. `cli` cannot import `@ultimat3/ui`'s roles instead (both are tier 5, and `cli -> admin -> ui` does not transit), so the honest placeholder is no colour: one grey LEVEL written to all three channels. `cmd-new.test.ts` pins that as a property rather than a byte — R == G == B over a transparent canvas — so a palette value pasted in here goes red on the gate, not in a review. Negative-proved: +9 on one channel fails it. PR 34's other eight threads answered rather than applied, each measured against `docs/architecture/04-error-contract.md`'s own accepted forms: the six `fix:` lines in ai/pwa/testing each name a pasteable call or `app.config.ts`; doctor's icon fix and mail's `.env.production` lines are the "one-line edit naming the file" form the contract lists as accepted; `dev-assets.ts`'s header is 4 lines, with the `node:path` rationale a separate comment below it; the duplicated `.catch(...)` does not exist (13/13, one `.catch(` in the file); and the two findings on `purge-env.ts` and `wiki/Configuration.md` were already applied in 171fc91. `bun run verify` 17/17 green, `errors` step included. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
Looks ready — CI green, CodeRabbit approved, no risk signals. Consider applying a 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
Contributor
|
Released v0.2.2: https://github.com/developerz-ai/ultimate/releases/tag/v0.2.2 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
This was referenced Aug 11, 2026
Merged
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
1 participant
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.
Summary
x devError/TypeErrorthrows acrosspwa,testing,aiwith stableX_*codes, runnable fixes, and wiki rowsTest plan
bun run verify— 17/17 greenbun test packages/pwa/src packages/testing/src packages/ai/src— 307 pass / 0 failbun test— no regressions vs baseline🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit