feat(ci,app): ci-success aggregator + typecheck boundary gaps + Grok/Claude plan (WS-3+WS-6) - #297
Merged
Merged
Conversation
Branch protection currently enumerates 4 individual contexts (Security Audit, Build, Quality Gate Node 22, Quality Gate Node 24) -- every time a required job is renamed or a new one needs to gate merges, someone has to remember to update branch-protection settings too, a step that's easy to forget and invisible until a PR merges without a check that should have blocked it. Adds `ci-success`, a job that needs: [security, quality, build] with if: always(), and fails loudly if any of those three didn't resolve to success (a matrix job like `quality` only reports success once every Node 22/24 leg passes). Branch protection can then require just this one context; a future required job joins ci-success's needs list instead of touching branch-protection settings. Placed at job-group "3" in ci.yml's numbered section comments, filling a gap between "2. BUILD" and "4. DEPLOY" that was already reserved for this. Does NOT change actual branch-protection settings -- that's a manual maintainer step (documented in docs/CI.md), out of scope for an automated PR per this repo's own governance rule.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe CI workflow adds a required ChangesCI status aggregation
Project data selection
Provider integration planning
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
app/listenerMiddleware.ts had a local ProjectStateWithHistory type
guessing at state.project's shape (`present?: { data?: ProjectData };
data?: ProjectData`) instead of trusting RootState, which is already
correctly inferred via `ReturnType<typeof _tempStore.getState>`. That
local type was actually copied from PersistedRootState (types.ts) --
the on-disk persistence format, which legitimately has schema-version
ambiguity -- and misapplied to the live in-memory Redux store, which
has none.
The real live shape is state.project.present: ProjectSliceState (`{
data: ProjectData }`), exactly matching the codebase's existing
canonical selector, selectProjectData (features/project/
projectSelectors.ts), used everywhere else project data is read from
state. Both listenerMiddleware call sites now use that selector instead
of re-deriving the access inline with a wrong/dead fallback branch
(`?? projectState.data`, which could never trigger against the real
shape).
No behavior change on the reachable path: all 24 existing
listenerMiddleware tests pass unchanged, confirming this was a
type-correctness fix, not a runtime one. Verified via the exact CI
typecheck command (tsgo --project tsconfig.tsgo.json --noEmit
--checkers 4) -- clean. Removes the now-resolved CLAUDE.md Known
Technical Debt line for this item.
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/ci.yml:
- Around line 261-263: Remove the rationale comments near the QNBS-v3 workflow
section in the CI YAML, leaving the workflow configuration unchanged. Preserve
or move the explanation in docs/CI.md, which is the designated location for
documenting this branch-protection contract.
In `@app/listenerMiddleware.ts`:
- Line 4: Add adjacent one-line QNBS-v3 why-comments in the required “[Grund /
Impact / Kreativer Mehrwert]” format for the selectProjectData import and both
selector-based state updates in the listener middleware. Place each comment
directly beside its corresponding substantive TypeScript change, including the
areas around the referenced later ranges, and do not rely on existing file-level
comments.
In `@docs/CI.md`:
- Around line 100-106: Remove the blank line between the consecutive blockquote
paragraphs in the maintainer follow-up section of docs/CI.md, keeping the
“Desktop” line and the “Maintainer follow-up” text contiguous within the same
blockquote to satisfy markdownlint MD028.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 900d8d09-56de-4e73-86b2-cb0490303d86
📒 Files selected for processing (4)
.github/workflows/ci.ymlCLAUDE.mdapp/listenerMiddleware.tsdocs/CI.md
…eferred) Plan-only commit, per explicit instruction -- no code changes. Covers a request to make Grok (xAI) a first-class native provider (README documents it as Cloud 4, but it's only reachable via the OpenRouter :free catalog today, not as a direct BYOK selection), later expanded to include Claude (Anthropic, README's Cloud 3) once investigation showed it has the identical "documented but not in the primary provider dropdown" symptom. Verification against the actual code surfaced that the two providers have fundamentally different root causes, corrected from the original draft's assumption that both were simply unimplemented: - Grok: services/aiProviderService.ts already has a real, working fetch-based implementation (streamGrok, key storage, a live /v1/models connection test). The gap is UI-only -- it's absent from AiProviderCard.tsx's primary provider dropdown, reachable only as a hybrid-fallback-chain option. - Claude: services/aiProviderService.ts's streamAnthropic() unconditionally throws "Direct browser requests are blocked by Anthropic (CORS)". This is not a wiring gap -- Anthropic's API doesn't permit direct browser fetch, and this app has zero backend infrastructure today (no api/ directory, no serverless functions, purely static across all 4 deployment targets). Claude requires a purpose-built serverless CORS-relay -- the app's first backend dependency -- not just a UI fix. Execution is explicitly deferred until every other workstream in the current post-recovery audit sprint (WS-3 through WS-10, including the WS-8 close-out) has merged into main, so this architectural change lands on a clean, fully-reconciled main rather than mid-sprint.
Maintainer decision (2026-07-30): execute this plan immediately after PR #297 merges, before WS-4/WS-5/WS-9 and the WS-8 close-out, rather than waiting for every other workstream first. WS-8 still runs last regardless, since it audits the final merged state.
qnbs
changed the title
Three CodeRabbit findings on PR #297: - .github/workflows/ci.yml: removed QNBS-v3 rationale comments from the YAML file -- this repo's own convention prohibits inline comments in JSON/YAML ("explain in the commit message" instead). The rationale already lives in docs/CI.md's new ci-success table row. - app/listenerMiddleware.ts: added the required QNBS-v3 why-comments next to the selectProjectData import and both call sites. - docs/CI.md: removed a blank line inside a blockquote (markdownlint MD028) between the "Desktop" and "Maintainer follow-up" notes. Codecov flagged 3 missing/partial patch-coverage lines in listenerMiddleware.ts -- the `if (!presentData) { ... }` guards kept from the original code in the prior commit. Investigating properly (rather than writing a test to force the branch) showed the guards are now provably unreachable: once RootState is correctly inferred (this PR's whole point), TypeScript narrows selectProjectData's return type in this exact calling context down to plain ProjectData, not `ProjectData | null` -- confirmed by tsgo rejecting a test's attempt to mock a null return with "Argument of type 'null' is not assignable to parameter of type 'ProjectData'". This is the same class of provably-dead branch as this session's earlier localNlpService.ts fix: removed the guards instead of writing a test for an impossible case, closing the coverage gap at its root rather than papering over it.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@GROK-PROVIDER-INTEGRATION-PLAN.md`:
- Around line 92-96: Update the i18n plan to require translation keys for all
user-facing provider labels, including Grok and Claude, as well as model
descriptions and connection-status/error text. Remove the option to reuse
hardcoded literals, require coverage across all 19 locales using the documented
workflow, and include translation-key usage in the definition of done.
- Around line 150-165: Update the proxy contract and test plan to require abuse
controls before implementation: validate the request schema, enforce a maximum
body size, restrict requests according to an explicit origin policy, apply rate
limiting, and enforce upstream/request timeouts. Document the expected rejection
and timeout behavior and add tests covering each control, alongside the existing
secret-handling requirements.
- Around line 176-192: Use the existing provider identifier `anthropic`
consistently across the Claude settings entry, provider types, persisted
selection, service dispatch, and `providerFactory` routing. Update the
`AiProviderCard.tsx` entry from `claude` to `anthropic` and ensure all related
comparisons and switch cases use the same identifier rather than introducing a
parallel `claude` value.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc71f2fd-e616-4c41-926d-b243ab7f865a
📒 Files selected for processing (4)
.github/workflows/ci.ymlGROK-PROVIDER-INTEGRATION-PLAN.mdapp/listenerMiddleware.tsdocs/CI.md
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/CI.md
… plan CodeRabbit findings on the plan doc (PR #297): - Made translation-key usage mandatory (no hardcoded literals) for both Grok and Claude UI strings, added explicitly to both Definition of Done checklists rather than left as an open question. - Made the Claude proxy's abuse controls (schema validation, body-size limit, origin policy, rate limiting, timeouts) mandatory requirements with a test-coverage expectation, not an undecided "consider this" note -- an unauthenticated public relay is a real resource-exhaustion surface (CWE-400). - Fixed a genuine identifier bug in the plan itself: used 'claude' as the proposed Settings UI id while the actual codebase type is 'anthropic' everywhere else (AIProvider union, aiProviderService.ts's case 'anthropic', etc.) -- would have left selection, key storage, service dispatch, and factory routing mismatched. Now consistently 'anthropic'. New §7 addendum, unrelated to Grok/Claude but bundled into this same plan file at the maintainer's request: an opt-in feature flag letting the PWA attempt direct browser-to-Ollama connections, following up on Issue #266. Investigated whether the Claude-style serverless proxy pattern could apply here -- it cannot: a hosted proxy can only reach itself, never a user's own localhost, so no proxy design bridges that gap. The actual answer already exists in the issue's own comment thread: a "technically: yes" opt-in using direct browser fetch (same model NovelCrafter uses), requiring the user to configure their own Ollama server's OLLAMA_ORIGINS to the exact PWA origin. Wrote up the full mechanism, UI, error-classification caveats, and explicit non-goals, cross-referencing ADR-0012 (the existing desktop-only decision this narrowly widens, not reverses).
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…patch gate Codecov flagged 1 remaining missing patch-coverage line in listenerMiddleware.ts: the selectProjectData(state) call inside runLocalFirstShadowSync. Unlike the auto-save listener (24 existing tests), this function had zero test coverage before this commit -- it's gated behind enableLocalFirstSync, which defaults off, and no existing test ever flips it on. Added one test that flips the flag on and lets the OFF->ON listener run for real, including its real (unmocked) dynamic Yjs doc/binding imports -- verified via debug assertions first that a freshly created shadow doc synced from the same present data produces a clean run with neither a drift-warning nor an error logged, then asserted exactly that as the real, meaningful check. This exercises the flagged line honestly rather than mocking around it.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@GROK-PROVIDER-INTEGRATION-PLAN.md`:
- Around line 288-292: Update the addendum heading and all related subsection
and cross-reference numbering so the section remains 7: change subsections
8.1–8.4 to 7.1–7.4, replace references to §8 with §7, and change “§§1–7 above”
to exclude the addendum itself. Apply the same normalization to the additional
referenced lines.
In `@tests/unit/listenerMiddleware.test.ts`:
- Around line 457-471: Add a single-line QNBS-v3 rationale comment immediately
before the new “local-first shadow sync (B1.1)” describe block. Replace or leave
the existing internal test comments unchanged; only ensure the required
rationale format is present at the describe-block boundary.
- Around line 457-471: The test for “local-first shadow sync (B1.1)” should
directly verify that selectProjectData(state) supplies the expected project data
to the local-first sync boundary. Spy on the selector or stub
getLocalFirstHandle so syncFromProject/verify receives an inspectable payload,
assert that payload, and remove the environment-dependent
mockLoggerWarn/mockLoggerError assertions.
- Around line 6-8: Add the required adjacent single-line QNBS-v3 rationale
comment next to the featureFlagsReducer/featureFlagsActions import, using the
exact format “// QNBS-v3: [Grund / Impact / Kreativer Mehrwert]” and documenting
the change’s rationale, impact, and creative value.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 296a6271-53a6-4b48-a706-d17bf6d914c8
📒 Files selected for processing (2)
GROK-PROVIDER-INTEGRATION-PLAN.mdtests/unit/listenerMiddleware.test.ts
Comment on lines
+288
to
+292
| ## 7. Addendum — opt-in browser-fetch Ollama for the PWA (Issue #266 follow-up) | ||
|
|
||
| **This is a separate initiative from §§1-7 above** — different issue, different mechanism, no | ||
| backend involved — bundled into this same plan file at the maintainer's request rather than as a | ||
| second document. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the addendum’s section numbering and cross-references.
The document labels the addendum as section 7, but its subsections are numbered 8.1–8.4; line 290 also refers to “§§1–7 above,” which includes the current section. Normalize these references (for example, use 7.1–7.4 and update §8 references to §7) to keep implementation links unambiguous.
Also applies to: 363-365
🤖 Prompt for 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.
In `@GROK-PROVIDER-INTEGRATION-PLAN.md` around lines 288 - 292, Update the
addendum heading and all related subsection and cross-reference numbering so the
section remains 7: change subsections 8.1–8.4 to 7.1–7.4, replace references to
§8 with §7, and change “§§1–7 above” to exclude the addendum itself. Apply the
same normalization to the additional referenced lines.
Owner
Author
There was a problem hiding this comment.
Noted — valid finding (the addendum header says §7 but its own subsections say 8.1-8.4, and the '§§1-7' self-reference includes itself). Deferring the fix to the next branch/PR per explicit instruction, to get this PR merged now rather than run another correction wave.
There was a problem hiding this comment.
@qnbs, acknowledged. The finding remains valid and unresolved for this PR; deferring it to avoid delaying the merge is reasonable.
Would you like me to open a GitHub follow-up issue for the next branch/PR, covering the §7 subsection and cross-reference normalization?
You are interacting with an AI system.
Comment on lines
+6
to
+8
| import featureFlagsReducer, { | ||
| featureFlagsActions, | ||
| } from '../../features/featureFlags/featureFlagsSlice'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required QNBS-v3 rationale comment.
This non-trivial TypeScript import change needs an adjacent single-line // QNBS-v3: [Grund / Impact / Kreativer Mehrwert] comment.
Proposed fix
import featureFlagsReducer, {
featureFlagsActions,
} from '../../features/featureFlags/featureFlagsSlice';
+// QNBS-v3: [Grund: dispatch the local-first flag / Impact: exercise the listener path / Kreativer Mehrwert: keep test setup explicit]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import featureFlagsReducer, { | |
| featureFlagsActions, | |
| } from '../../features/featureFlags/featureFlagsSlice'; | |
| import featureFlagsReducer, { | |
| featureFlagsActions, | |
| } from '../../features/featureFlags/featureFlagsSlice'; | |
| // QNBS-v3: [Grund: dispatch the local-first flag / Impact: exercise the listener path / Kreativer Mehrwert: keep test setup explicit] |
🤖 Prompt for 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.
In `@tests/unit/listenerMiddleware.test.ts` around lines 6 - 8, Add the required
adjacent single-line QNBS-v3 rationale comment next to the
featureFlagsReducer/featureFlagsActions import, using the exact format “//
QNBS-v3: [Grund / Impact / Kreativer Mehrwert]” and documenting the change’s
rationale, impact, and creative value.
Source: Coding guidelines
Owner
Author
There was a problem hiding this comment.
Noted — valid finding, missing QNBS-v3 comment on the new import. Deferring the fix to the next branch/PR per explicit instruction.
There was a problem hiding this comment.
@qnbs, acknowledged. I’ll leave this finding open for the follow-up branch/PR. Would you like me to create a GitHub issue to track the deferred QNBS-v3 comment addition?
You are interacting with an AI system.
Comment on lines
+457
to
+471
| describe('local-first shadow sync (B1.1)', () => { | ||
| it('reads project data via the canonical selector when the flag flips on', async () => { | ||
| const store = makeFullStore(); | ||
| store.dispatch(featureFlagsActions.setEnableLocalFirstSync(true)); | ||
| await vi.advanceTimersByTimeAsync(100); | ||
| // getLocalFirstHandle's dynamic imports (Yjs doc/binding/persistence) aren't mocked in this | ||
| // suite; whatever happens next is caught internally and logged as non-critical (see | ||
| // listenerMiddleware.ts's runLocalFirstShadowSync catch block) — this test only needs to prove | ||
| // the selector line runs and the listener doesn't throw out to the store. | ||
| // Real dynamic-imported Yjs doc/binding modules run for real here (not mocked) — a freshly | ||
| // created shadow doc synced from the same present data has nothing to drift against, so a | ||
| // clean run logs neither a warning (self-heal) nor an error. | ||
| expect(mockLoggerWarn).not.toHaveBeenCalled(); | ||
| expect(mockLoggerError).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required QNBS-v3 rationale comment for the new test.
The existing explanatory comments do not use the required format. Add one single-line rationale immediately before the new describe block.
Proposed fix
+// QNBS-v3: [Grund: cover canonical selector use during local-first sync / Impact: prevent project-data selection regressions / Kreativer Mehrwert: preserve the shared data boundary]
describe('local-first shadow sync (B1.1)', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe('local-first shadow sync (B1.1)', () => { | |
| it('reads project data via the canonical selector when the flag flips on', async () => { | |
| const store = makeFullStore(); | |
| store.dispatch(featureFlagsActions.setEnableLocalFirstSync(true)); | |
| await vi.advanceTimersByTimeAsync(100); | |
| // getLocalFirstHandle's dynamic imports (Yjs doc/binding/persistence) aren't mocked in this | |
| // suite; whatever happens next is caught internally and logged as non-critical (see | |
| // listenerMiddleware.ts's runLocalFirstShadowSync catch block) — this test only needs to prove | |
| // the selector line runs and the listener doesn't throw out to the store. | |
| // Real dynamic-imported Yjs doc/binding modules run for real here (not mocked) — a freshly | |
| // created shadow doc synced from the same present data has nothing to drift against, so a | |
| // clean run logs neither a warning (self-heal) nor an error. | |
| expect(mockLoggerWarn).not.toHaveBeenCalled(); | |
| expect(mockLoggerError).not.toHaveBeenCalled(); | |
| }); | |
| // QNBS-v3: [Grund: cover canonical selector use during local-first sync / Impact: prevent project-data selection regressions / Kreativer Mehrwert: preserve the shared data boundary] | |
| describe('local-first shadow sync (B1.1)', () => { |
🤖 Prompt for 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.
In `@tests/unit/listenerMiddleware.test.ts` around lines 457 - 471, Add a
single-line QNBS-v3 rationale comment immediately before the new “local-first
shadow sync (B1.1)” describe block. Replace or leave the existing internal test
comments unchanged; only ensure the required rationale format is present at the
describe-block boundary.
Source: Coding guidelines
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'selectProjectData|runLocalFirstShadowSync|getLocalFirstHandle|useFakeTimers|advanceTimersByTimeAsync|setEnableLocalFirstSync|mockLogger(Error|Warn)|clearAllMocks' \
tests/unit/listenerMiddleware.test.ts \
app/listenerMiddleware.tsRepository: qnbs/WorldScript-Studio
Length of output: 31266
🏁 Script executed:
sed -n '1,220p' tests/unit/listenerMiddleware.test.ts
printf '\n--- local-first block ---\n'
sed -n '440,490p' tests/unit/listenerMiddleware.test.ts
printf '\n--- local-first implementation ---\n'
sed -n '688,715p' app/listenerMiddleware.tsRepository: qnbs/WorldScript-Studio
Length of output: 11253
Assert the selector/sync boundary directly. The logger checks don’t prove selectProjectData(state) still feeds local-first sync, and the unmocked dynamic imports make the no-warning assertion environment-dependent. Spy on the selector or stub the sync handle and assert the payload passed into syncFromProject/verify instead.
🤖 Prompt for 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.
In `@tests/unit/listenerMiddleware.test.ts` around lines 457 - 471, The test for
“local-first shadow sync (B1.1)” should directly verify that
selectProjectData(state) supplies the expected project data to the local-first
sync boundary. Spy on the selector or stub getLocalFirstHandle so
syncFromProject/verify receives an inspectable payload, assert that payload, and
remove the environment-dependent mockLoggerWarn/mockLoggerError assertions.
Owner
Author
There was a problem hiding this comment.
Noted — both findings valid (missing QNBS-v3 comment on the new describe block, and the test should assert the selectProjectData payload actually reaches syncFromProject/verify directly instead of relying on the absence of logger calls in an unmocked-dependency environment). Deferring the fix to the next branch/PR per explicit instruction.
There was a problem hiding this comment.
@qnbs, acknowledged. Both findings remain valid and are deferred from this PR as requested.
Would you like me to open a follow-up GitHub issue for the next branch/PR, covering the required QNBS-v3 comment and a deterministic selector-to-sync boundary test?
You are interacting with an AI system.
This was referenced Jul 30, 2026
qnbs
added a commit
that referenced
this pull request
Jul 30, 2026
* fix: address deferred CodeRabbit findings from PR #297 Four findings deferred from PR #297 to keep that PR's merge unblocked, addressed here first per explicit instruction before starting the Grok/Claude/Ollama plan execution: 1. GROK-PROVIDER-INTEGRATION-PLAN.md: the addendum was numbered "## 7" but its own subsections said "8.1"-"8.4", and a "§§1-7 above" self-reference included the addendum itself. Renumbered to 7.1-7.4, fixed both self-references, and fixed a second pre-existing ambiguous "§5" cross-reference (meant "item 5 within Phase 1's own list", not top-level section 5) while in the area. 2. tests/unit/listenerMiddleware.test.ts: added the required QNBS-v3 comment next to the new featureFlagsActions import. 3. Added the required QNBS-v3 comment before the new "local-first shadow sync" describe block. 4. Rewrote the local-first sync test to assert the actual selector-to-binding boundary directly instead of inferring success from an absence of logger calls in an unmocked-dependency, environment-dependent setup. Added real mocks for services/localFirst/{projectDoc,docBinding,docPersistence} and services/storage/storageEncryptionService so the test can assert the exact object selectProjectData(state) returns is what reaches ProjectDocBinding's constructor and syncFromProject/verify. Hit two real bugs while building this: (a) an arrow-function mock implementation can never be used as a constructor -- switched to a real class, which Biome's useArrowFunction rule doesn't flag either (a function-expression fix would have been immediately reverted by the same linter that was satisfied by removing the arrow function in the first place); (b) vi.fn().mockImplementation() can't type a class constructor against its generic (...args) => any signature -- mocked the class directly instead of wrapping it in vi.fn(). * docs: Phase 0 re-verification finds Claude split -- two tracks, not one Re-checked the plan's own findings against current main before starting implementation (Phase 0's explicit purpose), and found the original draft's Claude analysis was incomplete in a way that changes the whole approach: - AiProviderCard.tsx's primary provider dropdown already has an 'anthropic' entry with a dedicated warning block -- missed originally because the investigation searched for the literal string "claude", never "anthropic" (the actual identifier), when checking that file. Its existing i18n copy already half-promises desktop support ("...or use the Tauri desktop app for Anthropic calls"). - streamAnthropic() throws unconditionally on EVERY platform, including desktop -- but CORS is a browser-only restriction. It never even checks isTauriRuntime() before throwing. Tauri's native HTTP plugin (@tauri-apps/plugin-http, services/localServerHttp.ts) already solves exactly this class of problem for Ollama/LM Studio/vLLM (ADR-0012); the same escape hatch works for any HTTPS endpoint, including api.anthropic.com, since native networking isn't subject to browser CORS at all. Restructured Phase 2 into two independent tracks: Track A (desktop) is a narrow bug fix reusing ADR-0012's established pattern, no new infrastructure, ships first. Track B (web/PWA) is the actual new architecture -- the serverless proxy -- and is now correctly scoped to only the three web deploy targets (desktop no longer needs it at all). Updated the Executive Summary, both Definition of Done checklists, and all internal cross-references accordingly. * docs(adr): add ADR-0016 for native Grok provider + split Claude fix Formalizes GROK-PROVIDER-INTEGRATION-PLAN.md's Phase 0 decision record: Grok is a pure UI-wiring fix (backend already works); Claude splits into Track A (desktop, native-HTTP via the ADR-0012 pattern, ships first, no new infrastructure) and Track B (web/PWA, this app's first serverless backend dependency, ships second). Also documents why the Ollama-in-PWA follow-up (Issue #266) is a separate decision, not a Track-B variant -- a hosted proxy can't reach a user's own localhost. * feat(ai): Phase 1 -- native Grok provider wiring Grok (xAI, Cloud 4) had a real, working backend (aiProviderService.ts's streamGrok(), key storage, a live /v1/models connection test) but was never selectable as a primary provider -- reachable only as a hybrid- fallback-chain option. This closes that UI/wiring gap; no new backend work needed. - AiProviderCard.tsx: added Grok to the primary provider dropdown with its own API-key input + model selector (grok-3 / grok-3-mini), reusing the existing storageService key-storage plumbing and the already-working testAIConnection() grok case for Test Connection. Also fixed the whole provider array's i18n while touching it -- gemini/openai/ollama/anthropic labels were hardcoded literals too, not just Grok's gap. - providerFactory.ts: providerToKind() gets a distinct 'grok' kind (not folded into 'openaiCompatible') -- Grok needs a fixed baseURL (https://api.x.ai/v1) and a real stored key, unlike the Ollama default that also maps to 'openaiCompatible'; folding it in would have silently sent Grok requests through Ollama's localhost default. - worldScriptCompletionFetch.ts: resolveModelConfig() branches on the new 'grok' kind, bringing Grok into the newer Writer-streaming path (useWorldScriptAI), not just legacy thunks. - i18n: added settings.ai.grokKey + settings.ai.provider.{gemini, openai,ollama,anthropic,grok} across all 19 locales -- hand- translated for the 5 production locales (de/en/es/fr/it), EN fallback seeded for the other 14 pending the established bulk- translate workflow. Bundles rebuilt; README's key-count badge/table updated 2849 -> 2855. - Tests: providerFactory (grok maps to 'grok', not 'unsupported'), worldScriptCompletionFetch (grok success path + missing-key 401), AiProviderCard (renders key input + model selector, saves via storageService.saveApiKey('grok', ...)). No CSP change needed: https://api.x.ai is already in src-tauri/tauri.conf.json, and the web CSP's https: scheme-source (ADR-0004) already covers it. No new feature flag -- confirmed via `pnpm exec tsx scripts/audit-feature-parity.ts` (22 flags, 0 drifts). The many locales/*/{characters,common,dashboard,desktop,export, outline,worlds,writer}.json changes in this commit are incidental -- running the i18n tooling's --fix pass re-sorted keys in files it touched along the way. No content was added, removed, or changed in any of those files, only key order. * docs: address CodeRabbit quick-win findings on PR #299 Add missing single-line QNBS-v3 rationale comments (Grok key state, key-persistence handler, the openaiCompatible-reuse decision in worldScriptCompletionFetch.ts, two test fixtures/blocks) and collapse four multi-line QNBS-v3 comments to the required single line. Not addressed here (documented, not fixed): the ADR-0016 wording describing Claude desktop as immediately usable is accurate once the full stacked sequence merges (Track A is PR #300, already stacked next) and will read correctly at that point without further edits; the locale-translation findings for settings.ai.grokKey reflect this repo's established i18n tiering (5 production locales get hand translations, 14 beta locales intentionally carry an EN fallback per CLAUDE.md's own i18n section), not a defect; the vi.mock hoisting concern in listenerMiddleware.test.ts is a false positive verified against the actual passing suite -- ProjectDocBinding is consumed via a dynamic await import() inside test bodies, not a static top-level import, so by the time any mock factory runs the module has already evaluated top-to-bottom once.
qnbs
added a commit
that referenced
this pull request
Jul 30, 2026
* fix: address deferred CodeRabbit findings from PR #297 Four findings deferred from PR #297 to keep that PR's merge unblocked, addressed here first per explicit instruction before starting the Grok/Claude/Ollama plan execution: 1. GROK-PROVIDER-INTEGRATION-PLAN.md: the addendum was numbered "## 7" but its own subsections said "8.1"-"8.4", and a "§§1-7 above" self-reference included the addendum itself. Renumbered to 7.1-7.4, fixed both self-references, and fixed a second pre-existing ambiguous "§5" cross-reference (meant "item 5 within Phase 1's own list", not top-level section 5) while in the area. 2. tests/unit/listenerMiddleware.test.ts: added the required QNBS-v3 comment next to the new featureFlagsActions import. 3. Added the required QNBS-v3 comment before the new "local-first shadow sync" describe block. 4. Rewrote the local-first sync test to assert the actual selector-to-binding boundary directly instead of inferring success from an absence of logger calls in an unmocked-dependency, environment-dependent setup. Added real mocks for services/localFirst/{projectDoc,docBinding,docPersistence} and services/storage/storageEncryptionService so the test can assert the exact object selectProjectData(state) returns is what reaches ProjectDocBinding's constructor and syncFromProject/verify. Hit two real bugs while building this: (a) an arrow-function mock implementation can never be used as a constructor -- switched to a real class, which Biome's useArrowFunction rule doesn't flag either (a function-expression fix would have been immediately reverted by the same linter that was satisfied by removing the arrow function in the first place); (b) vi.fn().mockImplementation() can't type a class constructor against its generic (...args) => any signature -- mocked the class directly instead of wrapping it in vi.fn(). * docs: Phase 0 re-verification finds Claude split -- two tracks, not one Re-checked the plan's own findings against current main before starting implementation (Phase 0's explicit purpose), and found the original draft's Claude analysis was incomplete in a way that changes the whole approach: - AiProviderCard.tsx's primary provider dropdown already has an 'anthropic' entry with a dedicated warning block -- missed originally because the investigation searched for the literal string "claude", never "anthropic" (the actual identifier), when checking that file. Its existing i18n copy already half-promises desktop support ("...or use the Tauri desktop app for Anthropic calls"). - streamAnthropic() throws unconditionally on EVERY platform, including desktop -- but CORS is a browser-only restriction. It never even checks isTauriRuntime() before throwing. Tauri's native HTTP plugin (@tauri-apps/plugin-http, services/localServerHttp.ts) already solves exactly this class of problem for Ollama/LM Studio/vLLM (ADR-0012); the same escape hatch works for any HTTPS endpoint, including api.anthropic.com, since native networking isn't subject to browser CORS at all. Restructured Phase 2 into two independent tracks: Track A (desktop) is a narrow bug fix reusing ADR-0012's established pattern, no new infrastructure, ships first. Track B (web/PWA) is the actual new architecture -- the serverless proxy -- and is now correctly scoped to only the three web deploy targets (desktop no longer needs it at all). Updated the Executive Summary, both Definition of Done checklists, and all internal cross-references accordingly. * docs(adr): add ADR-0016 for native Grok provider + split Claude fix Formalizes GROK-PROVIDER-INTEGRATION-PLAN.md's Phase 0 decision record: Grok is a pure UI-wiring fix (backend already works); Claude splits into Track A (desktop, native-HTTP via the ADR-0012 pattern, ships first, no new infrastructure) and Track B (web/PWA, this app's first serverless backend dependency, ships second). Also documents why the Ollama-in-PWA follow-up (Issue #266) is a separate decision, not a Track-B variant -- a hosted proxy can't reach a user's own localhost. * feat(ai): Phase 1 -- native Grok provider wiring Grok (xAI, Cloud 4) had a real, working backend (aiProviderService.ts's streamGrok(), key storage, a live /v1/models connection test) but was never selectable as a primary provider -- reachable only as a hybrid- fallback-chain option. This closes that UI/wiring gap; no new backend work needed. - AiProviderCard.tsx: added Grok to the primary provider dropdown with its own API-key input + model selector (grok-3 / grok-3-mini), reusing the existing storageService key-storage plumbing and the already-working testAIConnection() grok case for Test Connection. Also fixed the whole provider array's i18n while touching it -- gemini/openai/ollama/anthropic labels were hardcoded literals too, not just Grok's gap. - providerFactory.ts: providerToKind() gets a distinct 'grok' kind (not folded into 'openaiCompatible') -- Grok needs a fixed baseURL (https://api.x.ai/v1) and a real stored key, unlike the Ollama default that also maps to 'openaiCompatible'; folding it in would have silently sent Grok requests through Ollama's localhost default. - worldScriptCompletionFetch.ts: resolveModelConfig() branches on the new 'grok' kind, bringing Grok into the newer Writer-streaming path (useWorldScriptAI), not just legacy thunks. - i18n: added settings.ai.grokKey + settings.ai.provider.{gemini, openai,ollama,anthropic,grok} across all 19 locales -- hand- translated for the 5 production locales (de/en/es/fr/it), EN fallback seeded for the other 14 pending the established bulk- translate workflow. Bundles rebuilt; README's key-count badge/table updated 2849 -> 2855. - Tests: providerFactory (grok maps to 'grok', not 'unsupported'), worldScriptCompletionFetch (grok success path + missing-key 401), AiProviderCard (renders key input + model selector, saves via storageService.saveApiKey('grok', ...)). No CSP change needed: https://api.x.ai is already in src-tauri/tauri.conf.json, and the web CSP's https: scheme-source (ADR-0004) already covers it. No new feature flag -- confirmed via `pnpm exec tsx scripts/audit-feature-parity.ts` (22 flags, 0 drifts). The many locales/*/{characters,common,dashboard,desktop,export, outline,worlds,writer}.json changes in this commit are incidental -- running the i18n tooling's --fix pass re-sorted keys in files it touched along the way. No content was added, removed, or changed in any of those files, only key order. * feat(ai): Phase 2 Track A -- native Claude support on desktop Claude/Anthropic's streamAnthropic() threw unconditionally on every platform, including desktop -- but CORS is a browser-only restriction. Tauri's native HTTP plugin (localServerFetch, ADR-0012) already exists in this codebase to bypass exactly this class of problem for Ollama; the same escape hatch works for any HTTPS endpoint, since native networking isn't subject to browser CORS at all. - aiProviderService.ts: streamAnthropic() branches on isTauriRuntime() before throwing -- desktop calls https://api.anthropic.com/v1/messages directly via localServerFetch (Messages API format: x-api-key + anthropic-version headers). generateTextSingleProvider()'s anthropic case now delegates to the fixed streamAnthropic instead of its own separate unconditional throw. testAIConnection()'s anthropic case gets the same branch, using a minimal (max_tokens: 1) real request as the connectivity check since Anthropic has no public /v1/models endpoint to probe. Image generation is deliberately left throwing -- Anthropic's API doesn't offer that endpoint at all, so this isn't a CORS bug to fix. - src-tauri/tauri.conf.json: added https://api.anthropic.com to the CSP connect-src (mirrors the existing Grok entry). - AiProviderCard.tsx: the existing 'anthropic' warning-only block is now desktop-conditional -- desktop renders a real API-key input + model selector (Opus/Sonnet/Haiku 4.x) exactly like every other cloud provider; web/PWA keeps the warning, with copy updated to say desktop now genuinely works rather than "might" (Track B's proxy note stays "coming soon", since that part isn't built yet). - i18n: added settings.ai.anthropicKey; revised anthropicCorsNote/ anthropicHint wording across the 5 production locales to reflect desktop actually working now; EN fallback seeded for the other 14. 2855 -> 2856 keys; README badge/table updated. - Tests: aiProviderService (testAIConnection + streamText desktop success/failure/no-key paths, web-path unchanged), AiProviderCard (desktop shows key input, web shows warning, key save wired to storageService.saveApiKey('anthropic', ...)). Track B (the web/PWA serverless proxy) is unaffected by this commit -- still not built, still the only path for browser-tab Claude. * docs: address CodeRabbit quick-win findings on PR #299 Add missing single-line QNBS-v3 rationale comments (Grok key state, key-persistence handler, the openaiCompatible-reuse decision in worldScriptCompletionFetch.ts, two test fixtures/blocks) and collapse four multi-line QNBS-v3 comments to the required single line. Not addressed here (documented, not fixed): the ADR-0016 wording describing Claude desktop as immediately usable is accurate once the full stacked sequence merges (Track A is PR #300, already stacked next) and will read correctly at that point without further edits; the locale-translation findings for settings.ai.grokKey reflect this repo's established i18n tiering (5 production locales get hand translations, 14 beta locales intentionally carry an EN fallback per CLAUDE.md's own i18n section), not a defect; the vi.mock hoisting concern in listenerMiddleware.test.ts is a false positive verified against the actual passing suite -- ProjectDocBinding is consumed via a dynamic await import() inside test bodies, not a static top-level import, so by the time any mock factory runs the module has already evaluated top-to-bottom once. * fix(ai): concatenate all Claude text blocks, bound the connectivity ping Two Major CodeRabbit findings on PR #300, both real: - streamAnthropic() used content?.find(c => c.type === 'text') -- Claude can return multiple content blocks (e.g. a 'thinking' block plus several 'text' blocks on extended-thinking models), and find() silently dropped everything after the first match. Now filters and concatenates every text block in order. - testAIConnection's anthropic ping had no timeout, unlike every sibling connectivity check (testOllamaConnection: timeoutMs 5000; openai/grok/ gemini: AbortSignal.timeout(8000)) -- a stalled native HTTP call could hang the Settings test spinner indefinitely. Added timeoutMs: 8000. Deliberately NOT applied to streamAnthropic's own outbound call (the second location CodeRabbit's finding named): real generation calls across this file intentionally stay unbounded by a fixed timeout, using only the caller's own AbortSignal for cancellation -- Ollama's real generation call (streamOllama) follows the same pattern, only its connectivity check gets timeoutMs. Special-casing Anthropic's generation call would be the inconsistent choice, not the fix.
qnbs
added a commit
that referenced
this pull request
Jul 30, 2026
* fix: address deferred CodeRabbit findings from PR #297 Four findings deferred from PR #297 to keep that PR's merge unblocked, addressed here first per explicit instruction before starting the Grok/Claude/Ollama plan execution: 1. GROK-PROVIDER-INTEGRATION-PLAN.md: the addendum was numbered "## 7" but its own subsections said "8.1"-"8.4", and a "§§1-7 above" self-reference included the addendum itself. Renumbered to 7.1-7.4, fixed both self-references, and fixed a second pre-existing ambiguous "§5" cross-reference (meant "item 5 within Phase 1's own list", not top-level section 5) while in the area. 2. tests/unit/listenerMiddleware.test.ts: added the required QNBS-v3 comment next to the new featureFlagsActions import. 3. Added the required QNBS-v3 comment before the new "local-first shadow sync" describe block. 4. Rewrote the local-first sync test to assert the actual selector-to-binding boundary directly instead of inferring success from an absence of logger calls in an unmocked-dependency, environment-dependent setup. Added real mocks for services/localFirst/{projectDoc,docBinding,docPersistence} and services/storage/storageEncryptionService so the test can assert the exact object selectProjectData(state) returns is what reaches ProjectDocBinding's constructor and syncFromProject/verify. Hit two real bugs while building this: (a) an arrow-function mock implementation can never be used as a constructor -- switched to a real class, which Biome's useArrowFunction rule doesn't flag either (a function-expression fix would have been immediately reverted by the same linter that was satisfied by removing the arrow function in the first place); (b) vi.fn().mockImplementation() can't type a class constructor against its generic (...args) => any signature -- mocked the class directly instead of wrapping it in vi.fn(). * docs: Phase 0 re-verification finds Claude split -- two tracks, not one Re-checked the plan's own findings against current main before starting implementation (Phase 0's explicit purpose), and found the original draft's Claude analysis was incomplete in a way that changes the whole approach: - AiProviderCard.tsx's primary provider dropdown already has an 'anthropic' entry with a dedicated warning block -- missed originally because the investigation searched for the literal string "claude", never "anthropic" (the actual identifier), when checking that file. Its existing i18n copy already half-promises desktop support ("...or use the Tauri desktop app for Anthropic calls"). - streamAnthropic() throws unconditionally on EVERY platform, including desktop -- but CORS is a browser-only restriction. It never even checks isTauriRuntime() before throwing. Tauri's native HTTP plugin (@tauri-apps/plugin-http, services/localServerHttp.ts) already solves exactly this class of problem for Ollama/LM Studio/vLLM (ADR-0012); the same escape hatch works for any HTTPS endpoint, including api.anthropic.com, since native networking isn't subject to browser CORS at all. Restructured Phase 2 into two independent tracks: Track A (desktop) is a narrow bug fix reusing ADR-0012's established pattern, no new infrastructure, ships first. Track B (web/PWA) is the actual new architecture -- the serverless proxy -- and is now correctly scoped to only the three web deploy targets (desktop no longer needs it at all). Updated the Executive Summary, both Definition of Done checklists, and all internal cross-references accordingly. * docs(adr): add ADR-0016 for native Grok provider + split Claude fix Formalizes GROK-PROVIDER-INTEGRATION-PLAN.md's Phase 0 decision record: Grok is a pure UI-wiring fix (backend already works); Claude splits into Track A (desktop, native-HTTP via the ADR-0012 pattern, ships first, no new infrastructure) and Track B (web/PWA, this app's first serverless backend dependency, ships second). Also documents why the Ollama-in-PWA follow-up (Issue #266) is a separate decision, not a Track-B variant -- a hosted proxy can't reach a user's own localhost. * feat(ai): Phase 1 -- native Grok provider wiring Grok (xAI, Cloud 4) had a real, working backend (aiProviderService.ts's streamGrok(), key storage, a live /v1/models connection test) but was never selectable as a primary provider -- reachable only as a hybrid- fallback-chain option. This closes that UI/wiring gap; no new backend work needed. - AiProviderCard.tsx: added Grok to the primary provider dropdown with its own API-key input + model selector (grok-3 / grok-3-mini), reusing the existing storageService key-storage plumbing and the already-working testAIConnection() grok case for Test Connection. Also fixed the whole provider array's i18n while touching it -- gemini/openai/ollama/anthropic labels were hardcoded literals too, not just Grok's gap. - providerFactory.ts: providerToKind() gets a distinct 'grok' kind (not folded into 'openaiCompatible') -- Grok needs a fixed baseURL (https://api.x.ai/v1) and a real stored key, unlike the Ollama default that also maps to 'openaiCompatible'; folding it in would have silently sent Grok requests through Ollama's localhost default. - worldScriptCompletionFetch.ts: resolveModelConfig() branches on the new 'grok' kind, bringing Grok into the newer Writer-streaming path (useWorldScriptAI), not just legacy thunks. - i18n: added settings.ai.grokKey + settings.ai.provider.{gemini, openai,ollama,anthropic,grok} across all 19 locales -- hand- translated for the 5 production locales (de/en/es/fr/it), EN fallback seeded for the other 14 pending the established bulk- translate workflow. Bundles rebuilt; README's key-count badge/table updated 2849 -> 2855. - Tests: providerFactory (grok maps to 'grok', not 'unsupported'), worldScriptCompletionFetch (grok success path + missing-key 401), AiProviderCard (renders key input + model selector, saves via storageService.saveApiKey('grok', ...)). No CSP change needed: https://api.x.ai is already in src-tauri/tauri.conf.json, and the web CSP's https: scheme-source (ADR-0004) already covers it. No new feature flag -- confirmed via `pnpm exec tsx scripts/audit-feature-parity.ts` (22 flags, 0 drifts). The many locales/*/{characters,common,dashboard,desktop,export, outline,worlds,writer}.json changes in this commit are incidental -- running the i18n tooling's --fix pass re-sorted keys in files it touched along the way. No content was added, removed, or changed in any of those files, only key order. * feat(ai): Phase 2 Track A -- native Claude support on desktop Claude/Anthropic's streamAnthropic() threw unconditionally on every platform, including desktop -- but CORS is a browser-only restriction. Tauri's native HTTP plugin (localServerFetch, ADR-0012) already exists in this codebase to bypass exactly this class of problem for Ollama; the same escape hatch works for any HTTPS endpoint, since native networking isn't subject to browser CORS at all. - aiProviderService.ts: streamAnthropic() branches on isTauriRuntime() before throwing -- desktop calls https://api.anthropic.com/v1/messages directly via localServerFetch (Messages API format: x-api-key + anthropic-version headers). generateTextSingleProvider()'s anthropic case now delegates to the fixed streamAnthropic instead of its own separate unconditional throw. testAIConnection()'s anthropic case gets the same branch, using a minimal (max_tokens: 1) real request as the connectivity check since Anthropic has no public /v1/models endpoint to probe. Image generation is deliberately left throwing -- Anthropic's API doesn't offer that endpoint at all, so this isn't a CORS bug to fix. - src-tauri/tauri.conf.json: added https://api.anthropic.com to the CSP connect-src (mirrors the existing Grok entry). - AiProviderCard.tsx: the existing 'anthropic' warning-only block is now desktop-conditional -- desktop renders a real API-key input + model selector (Opus/Sonnet/Haiku 4.x) exactly like every other cloud provider; web/PWA keeps the warning, with copy updated to say desktop now genuinely works rather than "might" (Track B's proxy note stays "coming soon", since that part isn't built yet). - i18n: added settings.ai.anthropicKey; revised anthropicCorsNote/ anthropicHint wording across the 5 production locales to reflect desktop actually working now; EN fallback seeded for the other 14. 2855 -> 2856 keys; README badge/table updated. - Tests: aiProviderService (testAIConnection + streamText desktop success/failure/no-key paths, web-path unchanged), AiProviderCard (desktop shows key input, web shows warning, key save wired to storageService.saveApiKey('anthropic', ...)). Track B (the web/PWA serverless proxy) is unaffected by this commit -- still not built, still the only path for browser-tab Claude. * feat(ai): Phase 2 Track B -- Claude serverless proxy for web/PWA Track A (previous commit) fixed Claude on desktop. This closes the other half: browser tabs have no native-HTTP escape hatch from CORS, so Anthropic requests from Vercel/Cloudflare Pages deployments now relay through this app's own stateless serverless proxy -- its first backend dependency ever (previously a purely static SPA + a desktop bundle with no server of its own). - api/_shared/claudeProxyCore.ts: platform-agnostic relay core (Web Request/Response only, no @vercel/node or @cloudflare/workers-types dependency). Forwards { apiKey, model, messages, maxTokens? } to Anthropic's Messages API and returns the response unmodified. Mandatory abuse controls per ADR-0016 (the endpoint is public and unauthenticated by construction -- CWE-400 surface): Zod schema validation, a 256 KiB body-size cap checked against both the Content-Length header and the actual body (defeats a spoofed header), a same-origin check (Origin must match the deployment's own host), a best-effort per-client-IP rate limit (20 req/60s, in-memory -- explicitly not distributed, see code comment), and a 20s outbound timeout to Anthropic. Never logs the key, prompt, or response on any path -- asserted directly in tests via console spies across every branch. - api/claude-proxy.ts: Vercel Edge Function entry point (thin wrapper). - functions/api/claude-proxy.ts: Cloudflare Pages Function equivalent, at the matching /api/claude-proxy route (Cloudflare has no automatic "/api" prefix the way some platforms do -- the path has to earn it). - services/deployTarget.ts: isServerlessProxyCapable() -- reuses import.meta.env.BASE_URL (already this codebase's build-time GitHub-Pages-vs-edge marker, see config/resolveViteBase.ts) to detect whether the current web build can host the proxy at all. GitHub Pages is static-only and never can. - services/aiProviderService.ts: streamAnthropic() now has three branches -- desktop (Track A, unchanged), proxy-capable web (fetches /api/claude-proxy), and GitHub Pages (throws immediately, before ever checking for a key, since no key would help). testAIConnection's anthropic case gets the same three-way split. Retired the 'backendProxyRequired' TestConnectionErrorKind (nothing can produce it anymore) in favor of 'proxyUnavailableStaticHost'. - AiProviderCard.tsx: extracted the whole anthropic UI block into components/settings/AnthropicProviderFields.tsx -- both to keep AiProviderCard under the Biome cognitive-complexity gate (52 > 50 before the split) and under CLAUDE.md's 700-line file-size target. Desktop and proxy-capable web now render the same real key + model UI every other cloud provider gets; only GitHub Pages keeps the warning block, with copy rewritten from a generic CORS note to the actual "no proxy host" reason. - i18n: settings.ai.anthropicProxyNote (new); anthropicCorsNote/ anthropicHint/corsRestriction copy rewritten (desktop and Vercel/CF no longer say "coming soon" -- they work); testError. backendProxyRequired removed, testError.proxyUnavailableStaticHost added. Hand-translated for the 5 production locales; EN fallback seeded for the other 14. 2856 -> 2857 keys; README badge/table/count updated in all 4 places. - docs/SECURITY-THREAT-MODEL.md: new "Claude serverless proxy trust-model change" section, an Information Disclosure row, a Denial of Service row (the abuse-control list), a Mitigation Mapping row, and a checklist item -- this is the first provider whose BYOK key transits infrastructure WorldScript runs, and that's stated plainly rather than folded into the general BYOK framing. - README.md: privacy bullet and the encryption section both carry the same caveat; the Cloud 3 provider-stack row lists the actual model ids (Opus 4.7/Sonnet 4.6/Haiku 4.5, not the stale "Claude 3.5 Sonnet") and where Claude does/doesn't work. - docs/adr/0016: documents why providerFactory.ts's Vercel-AI-SDK layer still returns 'unsupported' for anthropic (would need the @ai-sdk/anthropic package -- Anthropic's Messages API isn't OpenAI-Chat-Completions-shaped like Grok/Ollama are, so that's a new runtime dependency out of scope here) and why no dedicated E2E spec was added (no other cloud provider -- including Grok -- has one in this repo; the RTL/unit level is where this class of Settings-form coverage already lives). Tests: 14 new (claudeProxyCore: schema/size/origin/rate-limit/timeout/ relay-fidelity/no-logging), 3 new (Vercel + Cloudflare entry-point delegation), 3 new (deployTarget branch coverage), aiProviderService's Anthropic describe block rewritten for the three-way split, AiProviderCard's anthropic describe block rewritten with a new proxy-capable-web case. 126 tests green across the full Track B surface; lint, typecheck, i18n:check, docs:check, and parity:check (22 flags, 0 drifts -- no new flag needed) all clean. * docs: address CodeRabbit quick-win findings on PR #299 Add missing single-line QNBS-v3 rationale comments (Grok key state, key-persistence handler, the openaiCompatible-reuse decision in worldScriptCompletionFetch.ts, two test fixtures/blocks) and collapse four multi-line QNBS-v3 comments to the required single line. Not addressed here (documented, not fixed): the ADR-0016 wording describing Claude desktop as immediately usable is accurate once the full stacked sequence merges (Track A is PR #300, already stacked next) and will read correctly at that point without further edits; the locale-translation findings for settings.ai.grokKey reflect this repo's established i18n tiering (5 production locales get hand translations, 14 beta locales intentionally carry an EN fallback per CLAUDE.md's own i18n section), not a defect; the vi.mock hoisting concern in listenerMiddleware.test.ts is a false positive verified against the actual passing suite -- ProjectDocBinding is consumed via a dynamic await import() inside test bodies, not a static top-level import, so by the time any mock factory runs the module has already evaluated top-to-bottom once. * fix(docs): restore the '+' suffix scripts/sync-readme-metrics.mjs expects PR #301's Build, E2E Tests, and E2E Deep Coverage jobs all failed at the same root cause: pnpm run build's predev/prebuild sync step invokes sync-readme-metrics.mjs, which hard-fails via its own drift guard when a metric occurrence in README.md doesn't match the real, computed value. Four README occurrences read "6477 tests / 529 files" (no "+"), while every regex the script uses -- both its targeted replacements and the drift guard's generic check -- expects the "6477+ tests" convention the rest of the file already follows. Because the file count only changed NOW (529 -> 532, from adding claudeProxyCore.test.ts, claudeProxyEntrypoints.test.ts, and deployTarget.test.ts across Track B and the Ollama addendum), the guard fired for the first time -- the "+"- less phrasing had been silently tolerated as long as the numeric value happened to still match. Restored the "+" suffix (this repo's actual, already-established convention for a fast-changing count, not a new one) rather than adding a parallel no-"+" regex path to the script -- the script's convention was correct; my own earlier edit was the drift. Running the script now auto-updates 529 -> 532 and is idempotent on a second run. * fix(api): bound the Claude proxy rate-limiter's eviction instead of clearing it (CWE-770) A spoofed x-forwarded-for burst that pushed the tracked-client map over its 5000-entry cap triggered a wholesale rateLimitLog.clear(), resetting every real client's rate-limit window — a DoS vector against the limiter itself. Now evicts stale-window entries first, then oldest-by-insertion-order, never the current caller's own entry. Also fixes two doc-drift nitpicks CodeRabbit caught on the same PR: a stale functions/claude-proxy.ts path in two header comments (actual path is functions/api/claude-proxy.ts), and a multi-line JSDoc block in AnthropicProviderFields.tsx that should have been the repo's single-line QNBS-v3 comment format. Co-Authored-By: Claude Sonnet 5 <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]>
6 tasks
qnbs
added a commit
that referenced
this pull request
Jul 30, 2026
#266) (#302) * fix: address deferred CodeRabbit findings from PR #297 Four findings deferred from PR #297 to keep that PR's merge unblocked, addressed here first per explicit instruction before starting the Grok/Claude/Ollama plan execution: 1. GROK-PROVIDER-INTEGRATION-PLAN.md: the addendum was numbered "## 7" but its own subsections said "8.1"-"8.4", and a "§§1-7 above" self-reference included the addendum itself. Renumbered to 7.1-7.4, fixed both self-references, and fixed a second pre-existing ambiguous "§5" cross-reference (meant "item 5 within Phase 1's own list", not top-level section 5) while in the area. 2. tests/unit/listenerMiddleware.test.ts: added the required QNBS-v3 comment next to the new featureFlagsActions import. 3. Added the required QNBS-v3 comment before the new "local-first shadow sync" describe block. 4. Rewrote the local-first sync test to assert the actual selector-to-binding boundary directly instead of inferring success from an absence of logger calls in an unmocked-dependency, environment-dependent setup. Added real mocks for services/localFirst/{projectDoc,docBinding,docPersistence} and services/storage/storageEncryptionService so the test can assert the exact object selectProjectData(state) returns is what reaches ProjectDocBinding's constructor and syncFromProject/verify. Hit two real bugs while building this: (a) an arrow-function mock implementation can never be used as a constructor -- switched to a real class, which Biome's useArrowFunction rule doesn't flag either (a function-expression fix would have been immediately reverted by the same linter that was satisfied by removing the arrow function in the first place); (b) vi.fn().mockImplementation() can't type a class constructor against its generic (...args) => any signature -- mocked the class directly instead of wrapping it in vi.fn(). * docs: Phase 0 re-verification finds Claude split -- two tracks, not one Re-checked the plan's own findings against current main before starting implementation (Phase 0's explicit purpose), and found the original draft's Claude analysis was incomplete in a way that changes the whole approach: - AiProviderCard.tsx's primary provider dropdown already has an 'anthropic' entry with a dedicated warning block -- missed originally because the investigation searched for the literal string "claude", never "anthropic" (the actual identifier), when checking that file. Its existing i18n copy already half-promises desktop support ("...or use the Tauri desktop app for Anthropic calls"). - streamAnthropic() throws unconditionally on EVERY platform, including desktop -- but CORS is a browser-only restriction. It never even checks isTauriRuntime() before throwing. Tauri's native HTTP plugin (@tauri-apps/plugin-http, services/localServerHttp.ts) already solves exactly this class of problem for Ollama/LM Studio/vLLM (ADR-0012); the same escape hatch works for any HTTPS endpoint, including api.anthropic.com, since native networking isn't subject to browser CORS at all. Restructured Phase 2 into two independent tracks: Track A (desktop) is a narrow bug fix reusing ADR-0012's established pattern, no new infrastructure, ships first. Track B (web/PWA) is the actual new architecture -- the serverless proxy -- and is now correctly scoped to only the three web deploy targets (desktop no longer needs it at all). Updated the Executive Summary, both Definition of Done checklists, and all internal cross-references accordingly. * docs(adr): add ADR-0016 for native Grok provider + split Claude fix Formalizes GROK-PROVIDER-INTEGRATION-PLAN.md's Phase 0 decision record: Grok is a pure UI-wiring fix (backend already works); Claude splits into Track A (desktop, native-HTTP via the ADR-0012 pattern, ships first, no new infrastructure) and Track B (web/PWA, this app's first serverless backend dependency, ships second). Also documents why the Ollama-in-PWA follow-up (Issue #266) is a separate decision, not a Track-B variant -- a hosted proxy can't reach a user's own localhost. * feat(ai): Phase 1 -- native Grok provider wiring Grok (xAI, Cloud 4) had a real, working backend (aiProviderService.ts's streamGrok(), key storage, a live /v1/models connection test) but was never selectable as a primary provider -- reachable only as a hybrid- fallback-chain option. This closes that UI/wiring gap; no new backend work needed. - AiProviderCard.tsx: added Grok to the primary provider dropdown with its own API-key input + model selector (grok-3 / grok-3-mini), reusing the existing storageService key-storage plumbing and the already-working testAIConnection() grok case for Test Connection. Also fixed the whole provider array's i18n while touching it -- gemini/openai/ollama/anthropic labels were hardcoded literals too, not just Grok's gap. - providerFactory.ts: providerToKind() gets a distinct 'grok' kind (not folded into 'openaiCompatible') -- Grok needs a fixed baseURL (https://api.x.ai/v1) and a real stored key, unlike the Ollama default that also maps to 'openaiCompatible'; folding it in would have silently sent Grok requests through Ollama's localhost default. - worldScriptCompletionFetch.ts: resolveModelConfig() branches on the new 'grok' kind, bringing Grok into the newer Writer-streaming path (useWorldScriptAI), not just legacy thunks. - i18n: added settings.ai.grokKey + settings.ai.provider.{gemini, openai,ollama,anthropic,grok} across all 19 locales -- hand- translated for the 5 production locales (de/en/es/fr/it), EN fallback seeded for the other 14 pending the established bulk- translate workflow. Bundles rebuilt; README's key-count badge/table updated 2849 -> 2855. - Tests: providerFactory (grok maps to 'grok', not 'unsupported'), worldScriptCompletionFetch (grok success path + missing-key 401), AiProviderCard (renders key input + model selector, saves via storageService.saveApiKey('grok', ...)). No CSP change needed: https://api.x.ai is already in src-tauri/tauri.conf.json, and the web CSP's https: scheme-source (ADR-0004) already covers it. No new feature flag -- confirmed via `pnpm exec tsx scripts/audit-feature-parity.ts` (22 flags, 0 drifts). The many locales/*/{characters,common,dashboard,desktop,export, outline,worlds,writer}.json changes in this commit are incidental -- running the i18n tooling's --fix pass re-sorted keys in files it touched along the way. No content was added, removed, or changed in any of those files, only key order. * feat(ai): Phase 2 Track A -- native Claude support on desktop Claude/Anthropic's streamAnthropic() threw unconditionally on every platform, including desktop -- but CORS is a browser-only restriction. Tauri's native HTTP plugin (localServerFetch, ADR-0012) already exists in this codebase to bypass exactly this class of problem for Ollama; the same escape hatch works for any HTTPS endpoint, since native networking isn't subject to browser CORS at all. - aiProviderService.ts: streamAnthropic() branches on isTauriRuntime() before throwing -- desktop calls https://api.anthropic.com/v1/messages directly via localServerFetch (Messages API format: x-api-key + anthropic-version headers). generateTextSingleProvider()'s anthropic case now delegates to the fixed streamAnthropic instead of its own separate unconditional throw. testAIConnection()'s anthropic case gets the same branch, using a minimal (max_tokens: 1) real request as the connectivity check since Anthropic has no public /v1/models endpoint to probe. Image generation is deliberately left throwing -- Anthropic's API doesn't offer that endpoint at all, so this isn't a CORS bug to fix. - src-tauri/tauri.conf.json: added https://api.anthropic.com to the CSP connect-src (mirrors the existing Grok entry). - AiProviderCard.tsx: the existing 'anthropic' warning-only block is now desktop-conditional -- desktop renders a real API-key input + model selector (Opus/Sonnet/Haiku 4.x) exactly like every other cloud provider; web/PWA keeps the warning, with copy updated to say desktop now genuinely works rather than "might" (Track B's proxy note stays "coming soon", since that part isn't built yet). - i18n: added settings.ai.anthropicKey; revised anthropicCorsNote/ anthropicHint wording across the 5 production locales to reflect desktop actually working now; EN fallback seeded for the other 14. 2855 -> 2856 keys; README badge/table updated. - Tests: aiProviderService (testAIConnection + streamText desktop success/failure/no-key paths, web-path unchanged), AiProviderCard (desktop shows key input, web shows warning, key save wired to storageService.saveApiKey('anthropic', ...)). Track B (the web/PWA serverless proxy) is unaffected by this commit -- still not built, still the only path for browser-tab Claude. * feat(ai): Phase 2 Track B -- Claude serverless proxy for web/PWA Track A (previous commit) fixed Claude on desktop. This closes the other half: browser tabs have no native-HTTP escape hatch from CORS, so Anthropic requests from Vercel/Cloudflare Pages deployments now relay through this app's own stateless serverless proxy -- its first backend dependency ever (previously a purely static SPA + a desktop bundle with no server of its own). - api/_shared/claudeProxyCore.ts: platform-agnostic relay core (Web Request/Response only, no @vercel/node or @cloudflare/workers-types dependency). Forwards { apiKey, model, messages, maxTokens? } to Anthropic's Messages API and returns the response unmodified. Mandatory abuse controls per ADR-0016 (the endpoint is public and unauthenticated by construction -- CWE-400 surface): Zod schema validation, a 256 KiB body-size cap checked against both the Content-Length header and the actual body (defeats a spoofed header), a same-origin check (Origin must match the deployment's own host), a best-effort per-client-IP rate limit (20 req/60s, in-memory -- explicitly not distributed, see code comment), and a 20s outbound timeout to Anthropic. Never logs the key, prompt, or response on any path -- asserted directly in tests via console spies across every branch. - api/claude-proxy.ts: Vercel Edge Function entry point (thin wrapper). - functions/api/claude-proxy.ts: Cloudflare Pages Function equivalent, at the matching /api/claude-proxy route (Cloudflare has no automatic "/api" prefix the way some platforms do -- the path has to earn it). - services/deployTarget.ts: isServerlessProxyCapable() -- reuses import.meta.env.BASE_URL (already this codebase's build-time GitHub-Pages-vs-edge marker, see config/resolveViteBase.ts) to detect whether the current web build can host the proxy at all. GitHub Pages is static-only and never can. - services/aiProviderService.ts: streamAnthropic() now has three branches -- desktop (Track A, unchanged), proxy-capable web (fetches /api/claude-proxy), and GitHub Pages (throws immediately, before ever checking for a key, since no key would help). testAIConnection's anthropic case gets the same three-way split. Retired the 'backendProxyRequired' TestConnectionErrorKind (nothing can produce it anymore) in favor of 'proxyUnavailableStaticHost'. - AiProviderCard.tsx: extracted the whole anthropic UI block into components/settings/AnthropicProviderFields.tsx -- both to keep AiProviderCard under the Biome cognitive-complexity gate (52 > 50 before the split) and under CLAUDE.md's 700-line file-size target. Desktop and proxy-capable web now render the same real key + model UI every other cloud provider gets; only GitHub Pages keeps the warning block, with copy rewritten from a generic CORS note to the actual "no proxy host" reason. - i18n: settings.ai.anthropicProxyNote (new); anthropicCorsNote/ anthropicHint/corsRestriction copy rewritten (desktop and Vercel/CF no longer say "coming soon" -- they work); testError. backendProxyRequired removed, testError.proxyUnavailableStaticHost added. Hand-translated for the 5 production locales; EN fallback seeded for the other 14. 2856 -> 2857 keys; README badge/table/count updated in all 4 places. - docs/SECURITY-THREAT-MODEL.md: new "Claude serverless proxy trust-model change" section, an Information Disclosure row, a Denial of Service row (the abuse-control list), a Mitigation Mapping row, and a checklist item -- this is the first provider whose BYOK key transits infrastructure WorldScript runs, and that's stated plainly rather than folded into the general BYOK framing. - README.md: privacy bullet and the encryption section both carry the same caveat; the Cloud 3 provider-stack row lists the actual model ids (Opus 4.7/Sonnet 4.6/Haiku 4.5, not the stale "Claude 3.5 Sonnet") and where Claude does/doesn't work. - docs/adr/0016: documents why providerFactory.ts's Vercel-AI-SDK layer still returns 'unsupported' for anthropic (would need the @ai-sdk/anthropic package -- Anthropic's Messages API isn't OpenAI-Chat-Completions-shaped like Grok/Ollama are, so that's a new runtime dependency out of scope here) and why no dedicated E2E spec was added (no other cloud provider -- including Grok -- has one in this repo; the RTL/unit level is where this class of Settings-form coverage already lives). Tests: 14 new (claudeProxyCore: schema/size/origin/rate-limit/timeout/ relay-fidelity/no-logging), 3 new (Vercel + Cloudflare entry-point delegation), 3 new (deployTarget branch coverage), aiProviderService's Anthropic describe block rewritten for the three-way split, AiProviderCard's anthropic describe block rewritten with a new proxy-capable-web case. 126 tests green across the full Track B surface; lint, typecheck, i18n:check, docs:check, and parity:check (22 flags, 0 drifts -- no new flag needed) all clean. * feat(ai): Addendum -- opt-in browser-Ollama connection for the PWA (ADR-0017, Issue #266) Issue #266's own comment thread already worked out the correct answer: CORS is the server's own configuration, not an immutable browser wall. If a user starts their own Ollama server with OLLAMA_ORIGINS covering the PWA's exact origin, a direct browser fetch to localhost succeeds -- real, standards-compliant CORS, the same non-magic model NovelCrafter's own browser-Ollama support uses. This wires that up as an explicit, default-off opt-in, without touching the "PWA stays desktop-only by default" guarantee ADR-0012 established. - features/featureFlags/featureFlagsSlice.ts: new enableBrowserOllama flag, default off (23rd flag, 7th user opt-in). - features/featureCatalog.ts: catalog entry (tier: ai, risk: medium, matches enableVoiceSupport's classification for a similarly-scoped opt-in) with real gateLocations. - hooks/useSettingsView.ts: dispatch case for the new flag (caught by scripts/audit-feature-parity.ts's "toggle fires, Redux doesn't update" check -- exactly the class of drift that gate exists for). - services/aiProviderService.ts: testAIConnection's ollama case now accepts opts.browserOllamaEnabled instead of hard-requiring isTauriRuntime(); a generic 'unreachable' result remaps to a new 'corsSuspected' kind when running the opt-in browser path -- CORS rejection and "server genuinely down" are identical TypeErrors at the JS level, so this is an honestly-hedged heuristic, not a diagnosis. - services/localServerHttp.ts: NO functional change -- verified during implementation that resolveFetch() already returns globalThis.fetch unconditionally on the web; the "PWA never probes localhost" policy lived entirely in the UI/testAIConnection call sites, not the transport layer. Added a comment explaining why, so a future reader doesn't go looking for a change that was never needed here. - components/settings/AiProviderCard.tsx (+ AiSections.tsx passing the flag down): canAttemptOllama = isDesktop || browserOllamaEnabled replaces every bare isDesktop check gating the ollama auto-probe effect, Load Models, and Test Connection. When on, an info block renders the exact `OLLAMA_ORIGINS=<origin> ollama serve` command for window.location.origin -- always correct for the current deployment, never a guessed/generic value, since WorldScript ships from multiple possible origins (unlike NovelCrafter's single fixed SaaS URL). - docs/adr/0017-pwa-browser-ollama-opt-in.md: new ADR: why this differs from the Claude proxy pattern (a hosted function can reach the public internet, never a user's own localhost -- hard networking fact, not policy), why WorldScript never defaulted this on unlike NovelCrafter, explicit non-goals (no LAN-IP, no PNA, not the default). ADR-0012 cross-referenced both directions. - docs/FEATURE-PARITY.md, CLAUDE.md (x2), tests/CLAUDE.md, AGENTS.md (x2), .github/copilot-instructions.md: flag-count references updated 22->23 total / 6->7 opt-in-off across every doc that enumerates them. - i18n: settings.featureFlags.enableBrowserOllama, ai.ollamaBrowserOptInTitle/ Body, ai.testError.corsSuspected -- hand-translated for the 5 production locales, EN fallback seeded for the other 14. 2857 -> 2861 keys; README updated in all 4 places. - Help articles (help.aiStudio.providers.content, help.faq.api.content, 5 production locales): fixed pre-existing stale content discovered while updating for this work -- wrong model names (Claude 3.5 Sonnet/Grok-2, both outdated), a genuine CSP-vs-CORS factual error in the Ollama description (CORS blocks localhost, not CSP -- the exact misconception ADR-0012 already corrected elsewhere), and a missing OpenRouter entry (shipped as Cloud 5, never documented in either article). Not scope creep: same articles, same edit, left broken would misinform users reading about the very feature this PR ships. - README.md: Ollama provider-stack row now documents both the desktop native path and the new opt-in browser path with the ADR-0017 link. Tests: 5 new (testAIConnection's browserOllamaEnabled on/off, corsSuspected remapping, non-remap of timeout/success), 5 new (AiProviderCard's flag-off/on UI states, auto-probe, button enablement, desktop-unaffected), 1 new (useSettingsView's dispatch case), plus the parametrized featureFlagsSlice case and updated 21->22 toggle count in FeatureFlagsSection. 336 tests green across the full affected surface; lint, typecheck, i18n:check, docs:check, and parity:check (23 flags, 0 drifts) all clean. * fix(tokens): avoid token-audit false-positive from #266 issue references audit-tokens.mjs's raw-hex rule (/#[0-9a-fA-F]{3,8}\b/) can't distinguish a CSS color literal from a GitHub issue reference -- "#266" matches just as well as "#f0f". The previous commit's two brand-new comments referencing Issue #266 (an info-block comment in AiProviderCard.tsx, a roadmapTarget string in featureCatalog.ts) pushed the count to 161 against a baseline of 160. Reworded both to drop the "#" immediately before the digits rather than raising the baseline -- same rule this repo already applies to biome-ignore suppressions. * docs: address CodeRabbit quick-win findings on PR #299 Add missing single-line QNBS-v3 rationale comments (Grok key state, key-persistence handler, the openaiCompatible-reuse decision in worldScriptCompletionFetch.ts, two test fixtures/blocks) and collapse four multi-line QNBS-v3 comments to the required single line. Not addressed here (documented, not fixed): the ADR-0016 wording describing Claude desktop as immediately usable is accurate once the full stacked sequence merges (Track A is PR #300, already stacked next) and will read correctly at that point without further edits; the locale-translation findings for settings.ai.grokKey reflect this repo's established i18n tiering (5 production locales get hand translations, 14 beta locales intentionally carry an EN fallback per CLAUDE.md's own i18n section), not a defect; the vi.mock hoisting concern in listenerMiddleware.test.ts is a false positive verified against the actual passing suite -- ProjectDocBinding is consumed via a dynamic await import() inside test bodies, not a static top-level import, so by the time any mock factory runs the module has already evaluated top-to-bottom once. * fix(docs): restore the '+' suffix scripts/sync-readme-metrics.mjs expects PR #301's Build, E2E Tests, and E2E Deep Coverage jobs all failed at the same root cause: pnpm run build's predev/prebuild sync step invokes sync-readme-metrics.mjs, which hard-fails via its own drift guard when a metric occurrence in README.md doesn't match the real, computed value. Four README occurrences read "6477 tests / 529 files" (no "+"), while every regex the script uses -- both its targeted replacements and the drift guard's generic check -- expects the "6477+ tests" convention the rest of the file already follows. Because the file count only changed NOW (529 -> 532, from adding claudeProxyCore.test.ts, claudeProxyEntrypoints.test.ts, and deployTarget.test.ts across Track B and the Ollama addendum), the guard fired for the first time -- the "+"- less phrasing had been silently tolerated as long as the numeric value happened to still match. Restored the "+" suffix (this repo's actual, already-established convention for a fast-changing count, not a new one) rather than adding a parallel no-"+" regex path to the script -- the script's convention was correct; my own earlier edit was the drift. Running the script now auto-updates 529 -> 532 and is idempotent on a second run. * fix(api): bound the Claude proxy rate-limiter's eviction instead of clearing it (CWE-770) A spoofed x-forwarded-for burst that pushed the tracked-client map over its 5000-entry cap triggered a wholesale rateLimitLog.clear(), resetting every real client's rate-limit window — a DoS vector against the limiter itself. Now evicts stale-window entries first, then oldest-by-insertion-order, never the current caller's own entry. Also fixes two doc-drift nitpicks CodeRabbit caught on the same PR: a stale functions/claude-proxy.ts path in two header comments (actual path is functions/api/claude-proxy.ts), and a multi-line JSDoc block in AnthropicProviderFields.tsx that should have been the repo's single-line QNBS-v3 comment format. Co-Authored-By: Claude Sonnet 5 <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]>
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
Consolidated PR B of the post-recovery audit's remaining workstreams (WS-3 + WS-6), plus a
planning-only doc requested mid-review.
WS-3 — required CI aggregator job:
ci-success, a job withneeds: [security, quality, build]andif: always()that fails loudly if any of those threedidn't resolve to
success— branch protection can then require just this one context.ci.yml— a numbering gap that already existed between "2. BUILD" and"4. DEPLOY".
docs/CI.md). Note: this job only becomes selectable in GitHub's branch-protection UI once ithas reported at least one successful run — that happens on this PR's CI.
WS-6 — close typecheck boundary gaps:
app/listenerMiddleware.tshad a localProjectStateWithHistorytype guessing atstate.project's shape — actually copied fromPersistedRootState(the on-disk persistenceformat, which legitimately has schema-version ambiguity) and misapplied to the live in-memory
Redux store, which has none (
RootStateis directly and correctly inferred from the actualreducer graph). Both call sites now use the codebase's existing canonical selector,
selectProjectData, removing the wrong type + a dead fallback branch that could never trigger.listenerMiddlewaretests passunchanged, confirming this was purely a type-correctness fix. Verified via the exact CI typecheck
command (
tsgo --project tsconfig.tsgo.json --noEmit --checkers 4).Bonus:
GROK-PROVIDER-INTEGRATION-PLAN.md(plan only, no code) — investigated a report thatGrok is documented in README as a working Cloud provider but unreachable from the Settings UI.
Verification found two structurally different root causes: Grok has a real, working backend
(
aiProviderService.ts'sstreamGrok()) and is purely a UI-wiring gap; Claude (also documented,also missing from the UI) is architecturally blocked —
streamAnthropic()unconditionally throwsbecause Anthropic's API doesn't permit direct browser CORS, and this app has zero backend
infrastructure today. Fixing Claude needs a purpose-built serverless CORS proxy — the app's first
backend dependency. Full plan (phases, deployment-target tradeoffs, security implications) is in
the file. Execution is scheduled to start immediately after this PR merges, ahead of the remaining
WS-4/WS-5/WS-9/WS-8 workstreams (re-prioritized per explicit instruction).
Test plan
pnpm run typecheck— clean (exact CI command)pnpm run lint— cleanpnpm run docs:check— OKpnpm exec vitest run tests/unit/listenerMiddleware.test.ts— 24/24 passci-successreports on this PR's CI runSummary by CodeRabbit
ci-successstatus that aggregates security, quality, and build checks for easier branch protection.