feat(auth): Clerk sign-in with dev-only backend session verification, plus streaming reliability and scroll-indicator fixes - #142
Merged
Merged
Conversation
Java Chat needs user accounts. Clerk ships no Svelte SDK, so the vanilla @clerk/clerk-js client is wrapped in a runes composable; the npm ESM build ships without prebuilt components, so the ui module from @clerk/ui is passed to Clerk.load() explicitly. - own the Clerk lifecycle in clerkAuthentication.svelte.ts (module-level runes state, $state.raw user resource, attachment-based user button) - lazy-load @clerk/clerk-js and @clerk/ui off the first-paint chunk (index chunk stays at 229 kB; Clerk loads as its own async chunk) - surface sign-in/sign-up buttons and the signed-in user button in Header - type VITE_CLERK_PUBLISHABLE_KEY on ImportMetaEnv; ignore .env.local variants so clerk env pull output stays untracked
The SPA now signs users in with Clerk, so the API needs to verify those sessions. Verification is deliberately scoped to dev deployments: only the dev profile (selected by the existing SPRING_PROFILE env var, default prod) carries the resource-server properties, so production javachat.ai has no Clerk configuration and /api/me deterministically denies every request until launch there. - add spring-boot-starter-oauth2-resource-server to the web bundle - create the Clerk JwtDecoder only when the JWKS property exists (dev profile); built from the JWKS URI so startup never makes a network call - validate the azp claim against app.clerk.authorized-parties per Clerk's manual JWT verification guide (ClerkAuthorizedPartyValidator) - expose GET /api/me returning the verified Clerk user id; all existing routes stay anonymous - integration tests cover both postures: dev-shaped (401 anonymous, 200 verified) and production-shaped (403 always); live-verified against the running app in both profiles
…switch Production builds intentionally omit VITE_CLERK_PUBLISHABLE_KEY until Clerk launches on javachat.ai. A missing key now means auth is disabled for that deployment: controls stay hidden with a console note instead of raising an error toast at every production page load.
Clerk work here spans the Svelte frontend, the Spring resource server, and the Clerk dashboard, so every agent session was re-deriving the same API surface from scratch. Vendoring the upstream skills gives each session and machine one pinned reference instead. skills-lock.json records the upstream source and content hash of each skill, so the bundle is reproducible and drift is detectable. - Add .agents/skills covering the Clerk core, backend API, CLI, custom UI, organizations, billing, webhooks, testing, and per-framework guides - Add skills-lock.json pinning every skill by source and sha256
The Clerk sign-in modal and user button rendered in the SDK stock dark theme, clashing with the app palette and typography wherever they appeared. - Load @clerk/ui/themes alongside clerk-js and pass an appearance built on the Clerk dark base theme - Map every Clerk color, font, and radius variable to a global.css design token so the stylesheet stays the sole palette owner - Override colorModalBackdrop to match the MobileChatDrawer ::backdrop; the cream neutral would otherwise wash out the page
[CS1g] permits 0, 1, and -1, but the comparison rule matched literal source text against ^(0|1)$. A long comparison carries the Java width suffix, so remainingPreparationNanos <= 0L in SseSupport was reported as a magic literal even though it compares against zero. Making the suffix optional keeps the allowlist at the same three values while covering their long forms; 5L is still rejected. - Allow an optional l/L suffix on the 0/1 allowlist - Add rule tests covering 0L, 1L, 0l, and a rejected 5L
lint-ast guards ast-grep with require_cmd but calls ruby bare, so a machine without ruby fails the chat-model SSOT step with a bare command not found and make Error 127, giving no hint that ruby is a lint dependency or how to get it. The pre-push hook runs make lint, so that dead end blocks every push on a fresh checkout. - Guard ruby with require_cmd, matching the ast-grep and lefthook entries, so the failure names the tool and the install command
HTTP and transport failures reached views as bare Errors without the retryable flag or details that server-sent error events already carry, so the UI could not decide whether to offer a retry. - wrap non-OK responses, missing bodies, and fetch/read failures in StreamFailureError with retryable derived from the HTTP status (429 and 5xx retry, other 4xx do not) - log the original transport error and attach it as the error cause so network diagnostics survive
The chat-model SSOT check only matched KEY=value and KEY: value forms, so a stale model passed as environment("OPENAI_MODEL", "gpt-5") escaped the scan.
- extend OPENAI_MODEL_ASSIGNMENT to also match the comma-separated environment() call form
- cover both stale and canonical comma-form strings in the self-testThe staging directory was created by mktemp before the INT/TERM traps existed, so a signal arriving during staging leaked the temporary directory and skipped the PID-file cleanup. - install the signal traps before mktemp so cleanup always owns the staging directory - guard the cleanup function against an unset staging directory for the pre-staging failure path
A closing fence written as ```Java was split into a fence plus trailing prose because the info-string check keyed on letter case, but per CommonMark an info-string fence only opens a block and can never close one, regardless of case. - drop the uppercase heuristic so a lone word after the fence marker is always an info string - cover capitalized and lowercase single-word suffixes in the normalizer tests
…timeout A waiter with remaining stage budget retried whenever the owner failure chain held any TimeoutException, including a genuine provider transport timeout preserved by preserveProviderTimeout, issuing a second billable rerank call the provider had already charged time for. - walk the TimeoutException cause chain for OkHttp transport-timeout markers and exclude those failures from retry - split provider-timeout detection into containsProviderTransportTimeout shared by both call sites - cover the stage-deadline retry and the provider-timeout no-retry paths with dedicated tests
…hdog The visible-output watchdog owned a hardcoded 20-second deadline while the SDK request timeout follows the configured budget, so a healthy response whose first visible token lands late inside the request budget (for example with non-none reasoning effort) was killed early. - derive the watchdog deadline from the same streamingRequestTimeout() the SDK request timeout uses - cover the configured-timeout derivation with a virtual-time test
Reconcile-after-abandon used a checkpoint snapshot read before the run lock was acquired, so an owner that persisted a terminal checkpoint in between had its COMPLETE state regressed back to PARTIAL by the reconciliation write. - re-read the durable checkpoint under the lock and return the fresher state when it changed - cover the race with an ObjectMapper hook that completes the run between the two reads
…al info strings Treating every single-word fence suffix as an info string broke the compact-prose repair: a closing fence written as ```Done was left attached instead of being split into fence plus prose. The uppercase heuristic is restored for closing-fence candidates, while a real opening fence keeps its info string because a later structural closing fence preserves the line as literal code. - restore the capitalized-word check limited to the attached-closing-fence repair case - cover capitalized info strings on attached opening fences and title-case prose on closing fences in both test classes
…reams The dev-only repair warning fired on every streamed chunk whenever an enrichment marker was present, because appending the synthetic closing fence for an unfinished stream counted as a repair, drowning out genuine diagnostics. - skip the warning when the normalization difference is append-only (the in-progress stream case); mid-text repairs still warn - cover the silent in-progress case with a regression test
A very long pasted message (stack trace, log) rendered as an unbounded bubble many viewports tall, pushing the rest of the conversation out of reach. - limit the user bubble to 40vh with internal scrolling so long pastes stay readable in context
Contributor
Important Review skippedToo many files! This PR contains 180 files, which is 80 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (181)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:f143a0292a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+45
to
+50
| const publishableKey = import.meta.env.VITE_CLERK_PUBLISHABLE_KEY; | ||
| if (!publishableKey) { | ||
| // Deliberate per-deployment switch, not an error: production builds omit | ||
| // VITE_CLERK_PUBLISHABLE_KEY until Clerk launches there, so auth controls | ||
| // stay hidden. Dev deployments and local .env.local provide the key. | ||
| console.info("Clerk authentication disabled: no VITE_CLERK_PUBLISHABLE_KEY in this build."); |
There was a problem hiding this comment.
In the Docker/Coolify deployment path, Vite resolves this value during npm run build, but the frontend stage in Dockerfile:14-29 neither declares/passes VITE_CLERK_PUBLISHABLE_KEY nor mounts it into the build step. Supplying the variable only to the runtime container therefore leaves it undefined in the generated bundle, takes this early return, and hides authentication on dev—the only deployed profile that enables the backend verifier. Obtain the required approval for this new environment-driven setting and make the approved value available to the frontend build.
AGENTS.md reference: AGENTS.md:L103-L103
Useful? React with 👍 / 👎.
Comment on lines
+71
to
+76
| clerkClient = loadingClient; | ||
| clerkClient.addListener((clerkResources) => { | ||
| clerkAuthentication.signedInUser = clerkResources.user ?? null; | ||
| }); | ||
| clerkAuthentication.signedInUser = clerkClient.user ?? null; | ||
| clerkAuthentication.isLoaded = true; |
There was a problem hiding this comment.
Even when Clerk loads and sign-in succeeds, this listener only mirrors the SDK user into UI state. A repo-wide frontend search finds no call to /api/me, no getToken() call, and no Authorization: Bearer attachment, so the SPA never traverses the new JWT decoder, issuer, or azp validation path; backend misconfiguration can coexist with a header that appears successfully signed in. Send a session token to the authenticated endpoint as part of the sign-in slice so the advertised backend verification is actually exercised.
AGENTS.md reference: AGENTS.md:L72-L72
Useful? React with 👍 / 👎.
…115) Co-authored-by: detail-app[bot] <180357370+detail-app[bot]@users.noreply.github.com>
#130) Co-authored-by: detail-app[bot] <180357370+detail-app[bot]@users.noreply.github.com>
#143) Co-authored-by: detail-app[bot] <180357370+detail-app[bot]@users.noreply.github.com>
…#144) Co-authored-by: detail-app[bot] <180357370+detail-app[bot]@users.noreply.github.com>
Co-authored-by: detail-app[bot] <180357370+detail-app[bot]@users.noreply.github.com>
Co-authored-by: detail-app[bot] <180357370+detail-app[bot]@users.noreply.github.com> Co-authored-by: William Callahan <william@williamcallahan.com>
The Docker frontend stage COPY frontend/ . would otherwise bake a developer's frontend/.env.local into the image, so image contents silently depended on the builder's machine. The Clerk publishable key also had no declared ARG, leaving auth-enabled images unbuildable. - Add **/.env and **/.env.* to .dockerignore so local env files never enter the build context - Declare VITE_CLERK_PUBLISHABLE_KEY as a build-time ARG so Vite inlines the public publishable key during the frontend build
Production had no Clerk resource server, so /api/me denied every request and a deploy of javachat.ai could not verify session tokens. A prod profile now binds the clerk.javachat.ai instance; the runtime image already activates it via SPRING_PROFILE=prod. - add application-prod.properties with issuer, JWKS, and azp origin - update dev-only Clerk comments in application.properties and SecurityConfig - reword the Clerk-disabled test javadoc to the unconfigured-environment posture
Safari renders Fraunces' small-optical-size J design as a broken stub hook at text sizes, so every serif surface follows the brand title onto Times New Roman. A local @font-face with size-adjust 107% keeps the rendered size matched to Fraunces (cap-height 105.7%, x-height 107.8% of its OS/2 metrics). - repoint --font-serif at metric-adjusted local Times New Roman - drop the Fraunces-only opsz variation pins from lesson, markdown, and guided-header styles - delete the unused vendored Fraunces variable font and its license
Startup hard-failed unless OPENAI_MODEL was exactly gpt-5.4, and request construction inspected model names to gate reasoning and temperature. The shared gateway owns upstream capability translation, so the client only needs a non-blank alias and forwards the universal reasoning-effort vocabulary unconditionally. - replace the fixed gpt-5.4 startup check with a non-blank alias check - drop ModelConfiguration.isGpt5Family and model-name inspection - leave app.llm.reasoning-effort unset by default to keep gateway defaults - align configuration and getting-started docs with the alias contract
OpenAiRequestFactory gated reasoning and temperature wiring on GPT-5 family name matching, which contradicts the gateway owning capability translation. Apply the configured temperature and optional reasoning effort uniformly for any gateway alias and rename the input token budget off the gpt-5.4 name.
The app served no CSP, and enabling Clerk in production widens the script surface. SecurityConfig now applies a mandatory policy bound via app.content-security-policy; each profile allows its own Clerk frontend-API origin alongside analytics and Google Fonts sources. - add baseline, dev, and prod policy strings to the property files - emit the Simple Analytics guard as a built asset so script-src stays 'self' without inline hash pinning - fail fast on a blank policy in AppProperties validation - assert the header on every response in the security integration test
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 30, 2026
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 freeto 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
Adds Clerk authentication end-to-end — header sign-in/sign-up controls and backend session verification enabled on dev deployments — and ships nine correctness fixes from post-release review and dogfooding: stream failures are now classified by retryability with the original error preserved, the "new content" pill only appears after a genuine scroll-away, and the streaming watchdog can no longer kill healthy slow responses.
Changes
Features
clerkAuthentication.svelte.ts,Header.svelte,ClerkConfig,ClerkAuthorizedPartyValidator,AuthenticatedUserController,application-dev.properties)App.svelte,Header.svelte)Bug Fixes
createScrollAnchor.svelte.ts:495,546)StreamFailureErrors (429/5xx retryable, other 4xx not) with the original fetch/read error attached ascauseand logged — previously network diagnostics were discarded and server errors rendered without the Retry button (sse.ts)OpenAIStreamingService)TimeoutException, issuing a duplicate billable LLM rerank after genuine provider timeouts; it now fires only for stage-deadline timeouts (RerankerService)```Java/```Cwere misclassified as trailing prose (lowercase wasn't), breaking fence structure; any single-word suffix is now an info string regardless of case, while compact title-case prose repair is preserved for genuine cases (MarkdownNormalizer)markdown.ts)LocalIngestionRunStore)process_all_to_qdrant.sh)MessageBubble.svelte)environment("OPENAI_MODEL", "...")drift inbuild.gradle.ktsis now detected, not just=/:forms (check-chat-model-ssot.rb)Breaking Changes
None
Test Plan
make build,make test(1003 tests),make lintall green on devdogfood-output/2026-07-29-pr141-postdeploy/); the pill fix was verified behaviorally on dev.javachat.ai