test(ai-chat): crypto/SSRF/assistant-parts coverage + a11y + refactors #10

Ghost · 2026-06-20T05:52:29+03:00

Ghost commented

2026-06-20 05:52:29 +03:00

Implements docs/backlog/ai-chat-review-followups.md (the non-blocking follow-ups from the ai-chat multi-aspect review).

What

Adds test coverage for previously-untested security-critical code, fixes two keyboard-a11y gaps, and lands several small refactors flagged in review.

How

Priority 1 — tests (no prod-code behavior change except exporting pure helpers for testing):

secret-box.spec.ts — AES-256-GCM round-trip; random salt+iv → different blobs both decrypt; tampered ciphertext / flipped auth-tag byte → throws the "APP_SECRET may have changed" message; wrong APP_SECRET → throws.
ssrf-guard.spec.ts — isIpAllowed blocks loopback/link-local+metadata/private/CGNAT/ULA/unspecified/IPv4-mapped, allows public; isUrlAllowed blocks bad scheme, invalid URL, IP literals, and DNS-rebinding (mocked dns.lookup).
ai-chat.service.spec.ts — assistantParts emits a synthetic output-error for an unpaired tool call (the load-bearing guard for the MissingToolResultsError fix — fails on pre-fix logic), output-available when paired, skips malformed calls; plus serializeSteps/rowToUiMessage.
ai-chat-tools.service.spec.ts — JSON-string node/content coercion forwards as object, invalid JSON throws the documented message, updatePageJson title-only vs object.
page-embedding.repo.spec.ts — empty spaceIds early-returns [] with no DB call.

Priority 2 — a11y + docs: Chat-history toggle + conversation rows are now keyboard-operable (role=button/tabIndex/Enter+Space), matching the existing history-item.tsx pattern; stale tool-list comment in tool-parts.tsx refreshed.

Priority 3 — refactors: useChat.onError adopts the server chat id when the first turn errors (AI SDK v6 onFinish doesn't fire on error); isToolPart exported once and shared (was duplicated in two components); buildInitialValues() dedups the MCP-server-form initial values; describeProviderError replaces two inline statusCode: message snippets so provider-error formatting is unified.

Reasoning / decisions

Exported assistantParts/serializeSteps/rowToUiMessage purely for testability (commented as test-only; no runtime change) — the unpaired-tool-call→output-error branch is the regression most worth pinning, since losing it silently reintroduces a next-turn crash.
API keys are protected only by secret-box at rest, and SSRF-guard is the only defense for admin-set external-MCP URLs — both had zero tests, so these are the highest-value additions.

Review findings

Item 2.3 skipped (not applicable): the plan referenced docs/ai-agent-chat-plan.md §5.3/§6.3 to correct the crypto description, but that file does not exist in the repo (grep-confirmed). secret-box.ts already documents the real layout. Flagged rather than inventing a target.
Self-reviewed the diff; no correctness issues. Pre-existing jest src/-rooted module-resolution gap makes ~16 unrelated suites fail to load (untouched files, e.g. PageService graph) — not introduced here.

Verification

pnpm --filter server build + pnpm --filter client build — clean.
pnpm --filter server test -- secret-box ssrf-guard ai-chat.service ai-chat-tools.service page-embedding.repo — 59/59 pass (5 suites).
Browser (headless Chromium, live z.ai): created 2 chats; Enter/Space on the Chat-history toggle flips aria-expanded and shows/hides the list; Enter on a conversation row switches the active chat; assistant messages render with no broken items (isToolPart refactor intact). No app errors. Screenshot captured.

🤖 Generated with Claude Code

Implements `docs/backlog/ai-chat-review-followups.md` (the non-blocking follow-ups from the ai-chat multi-aspect review). ## What Adds test coverage for previously-untested security-critical code, fixes two keyboard-a11y gaps, and lands several small refactors flagged in review. ## How **Priority 1 — tests (no prod-code behavior change except exporting pure helpers for testing):** - `secret-box.spec.ts` — AES-256-GCM round-trip; random salt+iv → different blobs both decrypt; tampered ciphertext / flipped auth-tag byte → throws the "APP_SECRET may have changed" message; wrong APP_SECRET → throws. - `ssrf-guard.spec.ts` — `isIpAllowed` blocks loopback/link-local+metadata/private/CGNAT/ULA/unspecified/IPv4-mapped, allows public; `isUrlAllowed` blocks bad scheme, invalid URL, IP literals, and DNS-rebinding (mocked `dns.lookup`). - `ai-chat.service.spec.ts` — `assistantParts` emits a synthetic `output-error` for an **unpaired** tool call (the load-bearing guard for the `MissingToolResultsError` fix — fails on pre-fix logic), `output-available` when paired, skips malformed calls; plus `serializeSteps`/`rowToUiMessage`. - `ai-chat-tools.service.spec.ts` — JSON-string `node`/`content` coercion forwards as object, invalid JSON throws the documented message, `updatePageJson` title-only vs object. - `page-embedding.repo.spec.ts` — empty `spaceIds` early-returns `[]` with no DB call. **Priority 2 — a11y + docs:** Chat-history toggle + conversation rows are now keyboard-operable (`role=button`/`tabIndex`/Enter+Space), matching the existing `history-item.tsx` pattern; stale tool-list comment in `tool-parts.tsx` refreshed. **Priority 3 — refactors:** `useChat.onError` adopts the server chat id when the first turn errors (AI SDK v6 `onFinish` doesn't fire on error); `isToolPart` exported once and shared (was duplicated in two components); `buildInitialValues()` dedups the MCP-server-form initial values; `describeProviderError` replaces two inline `statusCode: message` snippets so provider-error formatting is unified. ## Reasoning / decisions - Exported `assistantParts`/`serializeSteps`/`rowToUiMessage` purely for testability (commented as test-only; no runtime change) — the unpaired-tool-call→`output-error` branch is the regression most worth pinning, since losing it silently reintroduces a next-turn crash. - API keys are protected only by `secret-box` at rest, and SSRF-guard is the only defense for admin-set external-MCP URLs — both had zero tests, so these are the highest-value additions. ## Review findings - **Item 2.3 skipped (not applicable):** the plan referenced `docs/ai-agent-chat-plan.md` §5.3/§6.3 to correct the crypto description, but that file does not exist in the repo (grep-confirmed). `secret-box.ts` already documents the real layout. Flagged rather than inventing a target. - Self-reviewed the diff; no correctness issues. Pre-existing jest `src/`-rooted module-resolution gap makes ~16 unrelated suites fail to *load* (untouched files, e.g. PageService graph) — not introduced here. ## Verification - `pnpm --filter server build` + `pnpm --filter client build` — clean. - `pnpm --filter server test -- secret-box ssrf-guard ai-chat.service ai-chat-tools.service page-embedding.repo` — **59/59 pass** (5 suites). - Browser (headless Chromium, live z.ai): created 2 chats; **Enter/Space on the Chat-history toggle** flips `aria-expanded` and shows/hides the list; **Enter on a conversation row** switches the active chat; assistant messages render with no broken items (isToolPart refactor intact). No app errors. Screenshot captured. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Ghost added 2 commits 2026-06-20 05:52:30 +03:00

test(ai-chat): cover crypto/SSRF/assistant-parts; a11y + refactors e2d180ab0b

Closes the ai-chat code-review follow-ups.

Tests (security-critical paths previously uncovered):
- secret-box.spec: AES-256-GCM round-trip, random-salt uniqueness, tampered
  blob / wrong APP_SECRET throw the expected message.
- ssrf-guard.spec: isIpAllowed blocks loopback/link-local/private/CGNAT/ULA/
  unspecified/IPv4-mapped, allows public; isUrlAllowed blocks bad scheme,
  invalid URL, IP literals, and DNS-rebinding (mocked dns.lookup).
- ai-chat.service.spec: assistantParts emits output-error for an UNPAIRED
  tool call (guards the MissingToolResultsError fix), output-available when
  paired, skips malformed calls; serializeSteps/rowToUiMessage.
- ai-chat-tools.service.spec: JSON-string node/content coercion + invalid-JSON
  throws; updatePageJson title-only vs object.
- page-embedding.repo.spec: empty spaceIds early-returns [] with no DB call.

a11y:
- Chat history toggle and conversation rows are now keyboard-operable
  (role=button, tabIndex, Enter/Space), matching history-item.tsx.

Refactors:
- onError on useChat adopts the server chat id when the first turn errors
  (AI SDK v6 onFinish doesn't fire on error).
- isToolPart exported once from tool-parts and shared (was duplicated).
- buildInitialValues() dedups the ai-mcp-server-form initial values.
- describeProviderError replaces two inline statusCode/message snippets.
- tool-parts stale tool-list comment refreshed.

Implements docs/backlog/ai-chat-review-followups.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs: remove implemented ai-chat-review-followups backlog plan 3a04e09f13

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

vvzvlad force-pushed feat/ai-chat-review-followups from 3a04e09f13 to ec128d54b4

2026-06-20 18:03:15 +03:00

Compare

vvzvlad added 1 commit 2026-06-20 18:09:46 +03:00

Merge branch 'develop' into feat/ai-chat-review-followups 45cf4140eb

Integrate the already-merged step-limit work from develop. Only conflict was
ai-chat.service.spec.ts: both sides appended a describe block and edited the
import line. Resolved as a union — keep compactToolOutput + the assistantParts/
serializeSteps/rowToUiMessage suites (this branch) AND the prepareAgentStep
suite (develop), importing all symbols from ai-chat.service.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Ghost merged commit 127d26c057 into develop

2026-06-20 18:10:33 +03:00

Ghost referenced this issue from a commit

2026-06-20 18:10:35 +03:00

Merge pull request 'test(ai-chat): crypto/SSRF/assistant-parts coverage + a11y + refactors' (#10) from feat/ai-chat-review-followups into develop

vvzvlad referenced this issue from a commit

2026-06-21 05:21:31 +03:00

feat(html-embed): sandbox the embed block; split trusted trackers into an admin field

Ghost referenced this pull request

2026-06-23 04:15:40 +03:00

test: unit tests for the 10 candidates (#139) #142

~~Ghost referenced this pull request 2026-06-24 00:42:55 +03:00~~