From 332b89e44d0f18016ea0d9d520f5b3f31324c72d Mon Sep 17 00:00:00 2001 From: otomatty Date: Mon, 18 May 2026 23:19:49 +0900 Subject: [PATCH 1/3] fix: address PR #905 review comments Implement CodeRabbit/Codex feedback for admin errors UX, Sentry scrubbing, GitHub Actions analyze-error hardening, Claude JSON parsing, and related tests. Co-authored-by: Cursor --- .../claude-analyze/__tests__/schema.test.mjs | 15 ++- .github/actions/claude-analyze/action.yml | 6 +- .../claude-analyze/autoIssueRunner.mjs | 5 +- .../claude-analyze/escapeGithubAnnotation.mjs | 21 +++ .github/actions/claude-analyze/schema.mjs | 121 ++++++++++++++---- .github/workflows/analyze-error.yml | 10 +- admin/src/api/admin.test.ts | 14 +- admin/src/i18n/locales/en/nav.json | 3 +- admin/src/i18n/locales/ja/nav.json | 3 +- admin/src/pages/errors/ErrorDetailDialog.tsx | 10 +- admin/src/pages/errors/ErrorsContent.tsx | 4 +- admin/src/pages/errors/useApiErrors.ts | 27 +++- .../src/__tests__/routes/admin/errors.test.ts | 23 +++- .../__tests__/routes/pageSnapshots.test.ts | 7 +- server/api/src/__tests__/routes/pages.test.ts | 10 +- .../src/__tests__/routes/syncPages.test.ts | 5 +- .../__tests__/routes/webhooks/sentry.test.ts | 59 +++++---- .../services/pageAccessService.test.ts | 4 +- server/api/src/lib/githubAppAuth.ts | 14 +- server/api/src/lib/sentry.test.ts | 13 ++ server/api/src/lib/sentry.ts | 30 +++++ 21 files changed, 318 insertions(+), 86 deletions(-) create mode 100644 .github/actions/claude-analyze/escapeGithubAnnotation.mjs diff --git a/.github/actions/claude-analyze/__tests__/schema.test.mjs b/.github/actions/claude-analyze/__tests__/schema.test.mjs index 3df6cc99..06df8d2e 100644 --- a/.github/actions/claude-analyze/__tests__/schema.test.mjs +++ b/.github/actions/claude-analyze/__tests__/schema.test.mjs @@ -96,7 +96,20 @@ test("parseAndValidate strips Claude's ```json``` fence and prose preamble", () }); test("parseAndValidate throws when no JSON object is present", () => { - assert.throws(() => parseAndValidate("nope, no braces here"), /JSON object/); + assert.throws(() => parseAndValidate("nope, no braces here"), /valid analysis JSON/i); +}); + +test("parseAndValidate finds schema-valid JSON when prose contains stray braces", () => { + const payload = { + severity: "low", + ai_summary: "brace noise tolerated", + ai_root_cause: null, + ai_suggested_fix: null, + ai_suspected_files: null, + }; + const raw = `Here is { "not": "the payload" } the real payload:\n${JSON.stringify(payload)}`; + const validated = parseAndValidate(raw); + assert.equal(validated.severity, "low"); }); test("parseAndValidate throws on empty input", () => { diff --git a/.github/actions/claude-analyze/action.yml b/.github/actions/claude-analyze/action.yml index ae46f8e7..e5c013ef 100644 --- a/.github/actions/claude-analyze/action.yml +++ b/.github/actions/claude-analyze/action.yml @@ -156,11 +156,13 @@ runs: break fi if [ "${http_status}" -ge 400 ] && [ "${http_status}" -lt 500 ]; then - echo "::error title=Callback rejected (${http_status})::$(cat /tmp/callback-response.txt)" + CALLBACK_ERR_BODY="$(node "${{ github.action_path }}/escapeGithubAnnotation.mjs" /tmp/callback-response.txt)" + echo "::error title=Callback rejected (${http_status})::${CALLBACK_ERR_BODY}" exit 1 fi if [ "${attempt}" -ge "${max_attempts}" ]; then - echo "::error title=Callback failed after ${max_attempts} attempts (${http_status})::$(cat /tmp/callback-response.txt)" + CALLBACK_ERR_BODY="$(node "${{ github.action_path }}/escapeGithubAnnotation.mjs" /tmp/callback-response.txt)" + echo "::error title=Callback failed after ${max_attempts} attempts (${http_status})::${CALLBACK_ERR_BODY}" exit 1 fi attempt=$((attempt + 1)) diff --git a/.github/actions/claude-analyze/autoIssueRunner.mjs b/.github/actions/claude-analyze/autoIssueRunner.mjs index 6db207e5..6afea7b3 100644 --- a/.github/actions/claude-analyze/autoIssueRunner.mjs +++ b/.github/actions/claude-analyze/autoIssueRunner.mjs @@ -32,8 +32,9 @@ import { runAutoIssue } from "./autoIssue.mjs"; * @returns {string} */ function requireEnv(name) { - const value = process.env[name]; - if (typeof value !== "string" || value.length === 0) { + const raw = process.env[name]; + const value = typeof raw === "string" ? raw.trim() : ""; + if (value.length === 0) { throw new Error(`Missing required env var: ${name}`); } return value; diff --git a/.github/actions/claude-analyze/escapeGithubAnnotation.mjs b/.github/actions/claude-analyze/escapeGithubAnnotation.mjs new file mode 100644 index 00000000..6bf9ba3e --- /dev/null +++ b/.github/actions/claude-analyze/escapeGithubAnnotation.mjs @@ -0,0 +1,21 @@ +#!/usr/bin/env node +/** + * GitHub Actions のワークフローコマンド(例: ::error)へ埋め込む本文をエスケープする。 + * `%` / CR / LF / `:` がそのままだと注釈が壊れたり追加コマンドとして解釚される。 + * + * Escape text embedded in GitHub Actions workflow commands (e.g. ::error). + * Raw `%`, CR/LF, and `:` can corrupt annotations or inject extra commands. + * + * @see https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands + */ +import fs from "node:fs"; + +const path = process.argv[2]; +if (!path) { + console.error("usage: escapeGithubAnnotation.mjs "); + process.exit(1); +} +const s = fs.readFileSync(path, "utf8"); +process.stdout.write( + s.replace(/%/g, "%25").replace(/\r/g, "%0D").replace(/\n/g, "%0A").replace(/:/g, "%3A"), +); diff --git a/.github/actions/claude-analyze/schema.mjs b/.github/actions/claude-analyze/schema.mjs index 014319e6..5212a72e 100644 --- a/.github/actions/claude-analyze/schema.mjs +++ b/.github/actions/claude-analyze/schema.mjs @@ -74,16 +74,82 @@ export const analysisOutputSchema = z * @typedef {z.infer} AnalysisOutput */ +/** + * @param {string} str + * @param {number} startIdx - index of `{` + * @returns {string | null} balanced JSON object substring or null + */ +function extractBalancedJsonObject(str, startIdx) { + if (str[startIdx] !== "{") return null; + let depth = 0; + let inString = false; + let stringEscape = false; + for (let i = startIdx; i < str.length; i++) { + const c = str[i]; + if (stringEscape) { + stringEscape = false; + continue; + } + if (inString) { + if (c === "\\") { + stringEscape = true; + continue; + } + if (c === '"') { + inString = false; + } + continue; + } + if (c === '"') { + inString = true; + continue; + } + if (c === "{") depth++; + else if (c === "}") { + depth--; + if (depth === 0) return str.slice(startIdx, i + 1); + } + } + return null; +} + +/** + * severity + ai_summary を持つオブジェクトか(ネストした suspected_files エントリと区別)。 + * + * Whether `value` looks like the top-level analysis payload rather than a nested + * suspected-file entry (which lacks `ai_summary`). + * + * @param {unknown} value + * @returns {boolean} + */ +function looksLikeAnalysisPayload(value) { + if (!value || typeof value !== "object") return false; + const o = /** @type {Record} */ (value); + return typeof o.severity === "string" && typeof o.ai_summary === "string"; +} + +/** + * ```json ... ``` フェンスがあれば内側だけを返す(なければそのまま)。 + * + * If a ```json ... ``` fence exists, return the inner body; otherwise raw. + * + * @param {string} raw + * @returns {string} + */ +function stripMarkdownJsonFence(raw) { + const m = raw.match(/```(?:json)?\s*([\s\S]*?)```/im); + return m ? m[1].trim() : raw; +} + /** * Claude の生応答 (テキスト) から JSON を抽出して `analysisOutputSchema` で - * 検証する。Claude は時々 ```json ... ``` のコードフェンスで包んだり前置きを - * 付けたりするので、最初の `{` から最後の `}` までを切り出してパースする。 + * 検証する。フェンスや前置きに加え、本文中の `{}` がバランスしない場合でも、 + * 各 `{` 起点で括弧バランスを取った候補を順に試して最初にスキーマ合格したものを採用する。 * - * Extract a JSON object from Claude's raw text response and validate it - * against `analysisOutputSchema`. Claude occasionally wraps JSON in - * ```json ... ``` fences or adds prose preambles, so we slice from the first - * `{` to the last `}` rather than relying on `JSON.parse(raw)`. Throws - * `Error` with a descriptive message on malformed JSON or schema violation. + * Extract and validate analysis JSON from Claude's raw text. Besides fences and + * prose, stray `{}` pairs in preambles are handled by scanning each `{` start, + * taking the balanced span, and accepting the first candidate that passes the + * Zod schema (instead of slicing first `{` to last `}`). * * @param {string} raw - The raw text returned by Claude. * @returns {AnalysisOutput} @@ -92,23 +158,28 @@ export function parseAndValidate(raw) { if (typeof raw !== "string" || raw.length === 0) { throw new Error("Claude response was empty"); } - const start = raw.indexOf("{"); - const end = raw.lastIndexOf("}"); - if (start === -1 || end === -1 || end <= start) { - throw new Error("Could not locate a JSON object in Claude response"); - } - const slice = raw.slice(start, end + 1); - /** @type {unknown} */ - let parsed; - try { - parsed = JSON.parse(slice); - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - throw new Error(`Claude response was not valid JSON: ${msg}`); - } - const result = analysisOutputSchema.safeParse(parsed); - if (!result.success) { - throw new Error(`Claude response failed schema validation: ${result.error.message}`); + const normalized = stripMarkdownJsonFence(raw); + /** @type {string | null} */ + let lastJsonError = null; + for (let i = 0; i < normalized.length; i++) { + if (normalized[i] !== "{") continue; + const slice = extractBalancedJsonObject(normalized, i); + if (!slice) continue; + /** @type {unknown} */ + let parsed; + try { + parsed = JSON.parse(slice); + } catch (err) { + lastJsonError = err instanceof Error ? err.message : String(err); + continue; + } + const result = analysisOutputSchema.safeParse(parsed); + if (result.success) return result.data; + lastJsonError = result.error.message; + if (looksLikeAnalysisPayload(parsed)) { + throw new Error(`Claude response failed schema validation: ${lastJsonError}`); + } } - return result.data; + const suffix = lastJsonError ? `: ${lastJsonError}` : ""; + throw new Error(`Could not locate valid analysis JSON in Claude response${suffix}`); } diff --git a/.github/workflows/analyze-error.yml b/.github/workflows/analyze-error.yml index f23b2612..806dc7da 100644 --- a/.github/workflows/analyze-error.yml +++ b/.github/workflows/analyze-error.yml @@ -69,16 +69,18 @@ permissions: contents: read issues: write -# 同一 sentry_issue_id への並行起動を防ぐ。再来時にも余計に Claude を呼ばないため。 -# Prevent concurrent runs for the same Sentry issue. Avoids spending Anthropic -# credits twice when the webhook briefly double-fires on the same issue id. +# 同一 sentry_issue_id で複数 run が積まれたとき、古い run をキャンセルして +# Anthropic 呼び出しと repository_dispatch の二重課金を避ける。 +# +# When multiple runs queue for the same `sentry_issue_id`, cancel older runs so +# we do not pay for duplicate Claude analyses while a newer dispatch waits. concurrency: group: >- analyze-error-${{ github.event.client_payload.sentry_issue_id || github.event.inputs.sentry_issue_id }} - cancel-in-progress: false + cancel-in-progress: true jobs: analyze: diff --git a/admin/src/api/admin.test.ts b/admin/src/api/admin.test.ts index e7f7c61e..55d01f5b 100644 --- a/admin/src/api/admin.test.ts +++ b/admin/src/api/admin.test.ts @@ -286,12 +286,18 @@ describe("getApiErrorById", () => { }); it("200 なら row を返し、id を URL エンコードする", async () => { + const rowNeedsEncoding = { + ...sampleErrorRow, + id: "550e8400-e29b-41d4-a716-446655440000/with+reserved", + }; vi.mocked(adminFetch).mockResolvedValueOnce( - new Response(JSON.stringify({ error: sampleErrorRow }), { status: 200 }), + new Response(JSON.stringify({ error: rowNeedsEncoding }), { status: 200 }), + ); + const out = await getApiErrorById(rowNeedsEncoding.id); + expect(out).toEqual(rowNeedsEncoding); + expect(adminFetch).toHaveBeenCalledWith( + `/api/admin/errors/${encodeURIComponent(rowNeedsEncoding.id)}`, ); - const out = await getApiErrorById(sampleErrorRow.id); - expect(out).toEqual(sampleErrorRow); - expect(adminFetch).toHaveBeenCalledWith(`/api/admin/errors/${sampleErrorRow.id}`); }); }); diff --git a/admin/src/i18n/locales/en/nav.json b/admin/src/i18n/locales/en/nav.json index 2fe330a9..3d34c54c 100644 --- a/admin/src/i18n/locales/en/nav.json +++ b/admin/src/i18n/locales/en/nav.json @@ -2,7 +2,8 @@ "adminPanelTitle": "Zedi Admin", "adminShortTitle": "Admin", "menu": "Menu", - "unreadBadgeAriaLabel": "{{count}} unresolved API errors", + "unreadBadgeAriaLabel_one": "{{count}} unresolved API error", + "unreadBadgeAriaLabel_other": "{{count}} unresolved API errors", "items": { "aiModels": "AI Models", "users": "Users", diff --git a/admin/src/i18n/locales/ja/nav.json b/admin/src/i18n/locales/ja/nav.json index 03bc0290..7faa737d 100644 --- a/admin/src/i18n/locales/ja/nav.json +++ b/admin/src/i18n/locales/ja/nav.json @@ -2,7 +2,8 @@ "adminPanelTitle": "Zedi 管理画面", "adminShortTitle": "管理画面", "menu": "メニュー", - "unreadBadgeAriaLabel": "未対応の API エラー {{count}} 件", + "unreadBadgeAriaLabel_one": "未対応の API エラー {{count}} 件", + "unreadBadgeAriaLabel_other": "未対応の API エラー {{count}} 件", "items": { "aiModels": "AI モデル", "users": "ユーザー管理", diff --git a/admin/src/pages/errors/ErrorDetailDialog.tsx b/admin/src/pages/errors/ErrorDetailDialog.tsx index d2660091..4cae22eb 100644 --- a/admin/src/pages/errors/ErrorDetailDialog.tsx +++ b/admin/src/pages/errors/ErrorDetailDialog.tsx @@ -70,7 +70,15 @@ export function ErrorDetailDialog({ }; return ( - !open && onClose()}> + { + if (!open) { + setPendingFor(null); + onClose(); + } + }} + > {row.title} diff --git a/admin/src/pages/errors/ErrorsContent.tsx b/admin/src/pages/errors/ErrorsContent.tsx index 32cb5c6d..7ea9b0be 100644 --- a/admin/src/pages/errors/ErrorsContent.tsx +++ b/admin/src/pages/errors/ErrorsContent.tsx @@ -126,7 +126,7 @@ export function ErrorsContent({ {t("errors.filters.status")} onSeverityFilterChange(v === ANY ? "all" : (v as ApiErrorSeverity)) } diff --git a/admin/src/pages/errors/useApiErrors.ts b/admin/src/pages/errors/useApiErrors.ts index 7aecdac7..d552cdcb 100644 --- a/admin/src/pages/errors/useApiErrors.ts +++ b/admin/src/pages/errors/useApiErrors.ts @@ -101,6 +101,26 @@ function mergeRow(prev: GetApiErrorsResponse | null, row: ApiErrorRow): GetApiEr }; } +/** + * SSE でフィルタに一致しない更新が来たとき、一覧から該当 ID を落とす。 + * + * Drop a row id from the cached list when an SSE update no longer matches the + * active filter (so filtered views do not keep stale rows after status churn). + */ +function dropRowById( + prev: GetApiErrorsResponse | null, + rowId: string, +): GetApiErrorsResponse | null { + if (!prev) return prev; + const idx = prev.errors.findIndex((r) => r.id === rowId); + if (idx === -1) return prev; + return { + ...prev, + errors: prev.errors.filter((r) => r.id !== rowId), + total: Math.max(0, prev.total - 1), + }; +} + /** * フィルタ条件と SSE で push された行が合致するかを判定する。 * 合致しないものは UI に出さない(一覧の意味的な整合を保つ)。 @@ -223,7 +243,12 @@ export function useApiErrors(params: UseApiErrorsParams = {}): UseApiErrorsResul return; } const { status: curStatus, severity: curSeverity } = filterRef.current; - if (!matchesFilter(row, curStatus, curSeverity)) return; + if (!matchesFilter(row, curStatus, curSeverity)) { + if (isMountedRef.current) { + setData((prev) => dropRowById(prev, row.id)); + } + return; + } if (isMountedRef.current) { setData((prev) => mergeRow(prev, row)); } diff --git a/server/api/src/__tests__/routes/admin/errors.test.ts b/server/api/src/__tests__/routes/admin/errors.test.ts index 7742384b..2d690cc3 100644 --- a/server/api/src/__tests__/routes/admin/errors.test.ts +++ b/server/api/src/__tests__/routes/admin/errors.test.ts @@ -344,14 +344,23 @@ async function readUntil( ): Promise { const decoder = new TextDecoder(); let acc = ""; - const start = Date.now(); - while (Date.now() - start < timeoutMs) { - const { done, value } = await reader.read(); - if (done) break; - if (value) acc += decoder.decode(value, { stream: true }); - if (predicate(acc)) return acc; + let timedOut = false; + const timer = globalThis.setTimeout(() => { + timedOut = true; + void reader.cancel(); + }, timeoutMs); + try { + for (;;) { + if (timedOut) break; + const { done, value } = await reader.read(); + if (done) break; + if (value) acc += decoder.decode(value, { stream: true }); + if (predicate(acc)) return acc; + } + return acc; + } finally { + globalThis.clearTimeout(timer); } - return acc; } describe("GET /api/admin/errors/stream", () => { diff --git a/server/api/src/__tests__/routes/pageSnapshots.test.ts b/server/api/src/__tests__/routes/pageSnapshots.test.ts index 7be47cd1..2db3b462 100644 --- a/server/api/src/__tests__/routes/pageSnapshots.test.ts +++ b/server/api/src/__tests__/routes/pageSnapshots.test.ts @@ -54,7 +54,12 @@ function mockNote(noteOwnerId: string) { }; } -/** 1: pages row 2: caller email 3: findActiveNoteById */ +/** + * 1: pages row — 2: caller email — 3: findActiveNoteById + * SELECT の並び順(モック DB)。 + * + * Chain order for mocks: pages row, caller email lookup, findActiveNoteById. + */ function viewAccessPrefix( asUserEmail: string, noteOwnerId: string, diff --git a/server/api/src/__tests__/routes/pages.test.ts b/server/api/src/__tests__/routes/pages.test.ts index f9f8df28..177e184f 100644 --- a/server/api/src/__tests__/routes/pages.test.ts +++ b/server/api/src/__tests__/routes/pages.test.ts @@ -67,7 +67,10 @@ import { createMockDb } from "../createMockDb.js"; const TEST_USER_ID = "user-test-123"; const PAGE_ID = "page-content-test-001"; -/** pages.note_id と findActiveNoteById が参照するノート ID を一致させる。 */ +/** + * pages.note_id と findActiveNoteById が参照するノート ID を一致させる。 + * Align NOTE_ID with pages.note_id and the findActiveNoteById mock chain. + */ const NOTE_ID = "note-access-test-001"; function authHeaders() { @@ -93,7 +96,10 @@ function mockNoteRow() { }; } -/** PR 1b 以降の assertPage*Access が要求する SELECT 3 連を先頭に付ける。 */ +/** + * PR 1b 以降の assertPage*Access が要求する SELECT 3 連を先頭に付ける。 + * Prefix chains with the three SELECT results required by assertPage*Access (PR 1b+). + */ function pageAccessPrefix(extraPageFields: Record = {}) { return [ [{ id: PAGE_ID, ownerId: TEST_USER_ID, noteId: NOTE_ID, ...extraPageFields }], diff --git a/server/api/src/__tests__/routes/syncPages.test.ts b/server/api/src/__tests__/routes/syncPages.test.ts index f06163df..b1a8492d 100644 --- a/server/api/src/__tests__/routes/syncPages.test.ts +++ b/server/api/src/__tests__/routes/syncPages.test.ts @@ -17,7 +17,10 @@ vi.mock("../../middleware/auth.js", () => ({ }, })); -/** PR 1b: GET/POST sync は ensureDefaultNote を先に叩く。モックしてチェーンをページ同期クエリに寄せる。 */ +/** + * PR 1b: GET/POST sync は ensureDefaultNote を先に叩く。モックしてチェーンをページ同期クエリに寄せる。 + * Mock ensureDefaultNote so SELECT chains align with page sync queries (PR 1b). + */ vi.mock("../../services/defaultNoteService.js", () => ({ ensureDefaultNote: vi.fn(async () => ({ id: "sync-default-note-id", diff --git a/server/api/src/__tests__/routes/webhooks/sentry.test.ts b/server/api/src/__tests__/routes/webhooks/sentry.test.ts index ced31680..5820635d 100644 --- a/server/api/src/__tests__/routes/webhooks/sentry.test.ts +++ b/server/api/src/__tests__/routes/webhooks/sentry.test.ts @@ -283,34 +283,37 @@ describe("POST /api/webhooks/sentry", () => { const fetchMock = vi.fn(async () => new Response("{}", { status: 200 })); vi.stubGlobal("fetch", fetchMock); - const existingRow = { - id: "00000000-0000-0000-0000-0000000000cc", - sentryIssueId: "recur-1", - title: "recurrence", - route: null, - occurrences: 5, - }; - const upserted = { ...existingRow, occurrences: 6 }; - // First SELECT returns the existing row → isNew=false → no dispatch. - const { app } = createApp([[existingRow], [upserted]]); - const body = JSON.stringify({ - data: { issue: { id: "recur-1", title: "recurrence" } }, - }); - - const res = await app.request("/api/webhooks/sentry", { - method: "POST", - headers: { - "Content-Type": "application/json", - "sentry-hook-signature": sign(body), - }, - body, - }); - - expect(res.status).toBe(200); - await new Promise((r) => setTimeout(r, 0)); - // No GitHub fetch should have happened: the dispatch path was skipped. - expect(fetchMock).not.toHaveBeenCalled(); - vi.unstubAllGlobals(); + try { + const existingRow = { + id: "00000000-0000-0000-0000-0000000000cc", + sentryIssueId: "recur-1", + title: "recurrence", + route: null, + occurrences: 5, + }; + const upserted = { ...existingRow, occurrences: 6 }; + // First SELECT returns the existing row → isNew=false → no dispatch. + const { app } = createApp([[existingRow], [upserted]]); + const body = JSON.stringify({ + data: { issue: { id: "recur-1", title: "recurrence" } }, + }); + + const res = await app.request("/api/webhooks/sentry", { + method: "POST", + headers: { + "Content-Type": "application/json", + "sentry-hook-signature": sign(body), + }, + body, + }); + + expect(res.status).toBe(200); + await new Promise((r) => setTimeout(r, 0)); + // No GitHub fetch should have happened: the dispatch path was skipped. + expect(fetchMock).not.toHaveBeenCalled(); + } finally { + vi.unstubAllGlobals(); + } }); it("returns 403 when signature is missing", async () => { diff --git a/server/api/src/__tests__/services/pageAccessService.test.ts b/server/api/src/__tests__/services/pageAccessService.test.ts index 7f8f8360..b3d01a15 100644 --- a/server/api/src/__tests__/services/pageAccessService.test.ts +++ b/server/api/src/__tests__/services/pageAccessService.test.ts @@ -143,8 +143,8 @@ describe("assertPageEditAccess (issue #823)", () => { await expect(assertPageEditAccess(asDb(db), PAGE_ID, USER_ID)).resolves.toBeUndefined(); }); - it("denies non-owner of underlying row when no note role resolves", async () => { - const { db } = createMockDb(noRoleChains(USER_ID)); + it("denies caller without note role when the page row is owned by someone else", async () => { + const { db } = createMockDb(noRoleChains(OTHER_USER_ID)); await expect(assertPageEditAccess(asDb(db), PAGE_ID, USER_ID)).rejects.toMatchObject({ status: 403, }); diff --git a/server/api/src/lib/githubAppAuth.ts b/server/api/src/lib/githubAppAuth.ts index 31ce97d3..b9069955 100644 --- a/server/api/src/lib/githubAppAuth.ts +++ b/server/api/src/lib/githubAppAuth.ts @@ -15,7 +15,8 @@ * it for an installation access token via * `POST /app/installations/{id}/access_tokens`. * - Caches the resulting installation token in memory and refreshes 60 s - * before expiry to avoid stampede on every dispatch. + * before expiry to avoid stampede on every dispatch; concurrent callers share + * a single in-flight refresh promise while the cache is cold or near expiry. * - `triggerRepositoryDispatch` fires a `repository_dispatch` event without * awaiting the response on the user-visible path (errors are logged). * - `verifyInstallationToken` validates inbound installation tokens by calling @@ -95,6 +96,9 @@ interface CachedInstallationToken { let cachedInstallationToken: CachedInstallationToken | null = null; +/** Single in-flight refresh so webhook bursts coalesce to one GitHub token mint. */ +let installationTokenRefreshInFlight: Promise | null = null; + /** * 環境変数から GitHub App の設定を読み出す。各値が欠けていれば throw する。 * Read GitHub App configuration from environment. Throws when any value is @@ -159,6 +163,13 @@ export async function getInstallationToken(): Promise { ) { return cachedInstallationToken.token; } + installationTokenRefreshInFlight ??= fetchInstallationTokenFromGitHub().finally(() => { + installationTokenRefreshInFlight = null; + }); + return installationTokenRefreshInFlight; +} + +async function fetchInstallationTokenFromGitHub(): Promise { const { installationId } = readAppConfig(); const jwt = await createAppJWT(); const res = await fetchGitHubWithTimeout( @@ -197,6 +208,7 @@ export async function getInstallationToken(): Promise { */ export function __resetInstallationTokenCacheForTests(): void { cachedInstallationToken = null; + installationTokenRefreshInFlight = null; } /** diff --git a/server/api/src/lib/sentry.test.ts b/server/api/src/lib/sentry.test.ts index 5e7bd240..75cd111c 100644 --- a/server/api/src/lib/sentry.test.ts +++ b/server/api/src/lib/sentry.test.ts @@ -108,6 +108,19 @@ describe("scrubSentryEvent", () => { expect(scrubbed.request?.query_string).toBe("page=1&token=[Filtered]&safe=ok"); }); + it("scrubs request.url query strings and redacts UUID-shaped path segments", () => { + const event = makeEvent({ + request: { + url: "https://api.example.com/api/pages/550e8400-e29b-41d4-a716-446655440000?token=secret&safe=1", + headers: {}, + }, + }); + + const scrubbed = scrubSentryEvent(event); + + expect(scrubbed.request?.url).toBe("https://api.example.com/api/pages/[uuid]"); + }); + it("scrubs user, extra, contexts, tags, and breadcrumb data", () => { const event = makeEvent({ user: { id: "user-1", email: "user@example.com" }, diff --git a/server/api/src/lib/sentry.ts b/server/api/src/lib/sentry.ts index c1b9772a..ed05f988 100644 --- a/server/api/src/lib/sentry.ts +++ b/server/api/src/lib/sentry.ts @@ -100,6 +100,7 @@ export function scrubSentryEvent(event: ErrorEvent): ErrorEvent { if (event.request) { event.request = { ...event.request, + url: scrubRequestUrl(event.request.url), headers: scrubShallowRecord(event.request.headers), data: scrubDeep(event.request.data, new WeakSet()), query_string: scrubQueryStringField(event.request.query_string), @@ -129,6 +130,35 @@ export function scrubSentryEvent(event: ErrorEvent): ErrorEvent { return event; } +/** + * SDK が記録するリクエスト URL からクエリと不透明パス成分を落とす。 + * matched route は `routePath` など別チャネルに載せる前提。 + * + * Strip query strings and redact opaque path segments from the raw URL SDKs + * record; structured routing context belongs in `routePath` / extras instead. + */ +function scrubRequestUrl(url: unknown): unknown { + if (typeof url !== "string" || url.length === 0) return url; + try { + const u = new URL(url); + u.search = ""; + const segments = u.pathname.split("/").map((seg) => { + if (seg.length === 0) return seg; + if (/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(seg)) { + return "[uuid]"; + } + if (/^[A-Za-z0-9._~-]{24,}$/.test(seg)) { + return FILTERED; + } + return seg; + }); + u.pathname = segments.join("/") || "/"; + return u.toString(); + } catch { + return FILTERED; + } +} + function scrubShallowRecord | undefined>(input: T): T; function scrubShallowRecord( input: Record | undefined, From 0b83c79a96722fa187a15d8a3f182c3e917770ff Mon Sep 17 00:00:00 2001 From: otomatty Date: Mon, 18 May 2026 23:31:10 +0900 Subject: [PATCH 2/3] fix: address PR #908 review comments Fix API typecheck for scrubRequestUrl, scrub relative request URLs with a synthetic base, and only unwrap json-labeled markdown fences in Claude analyze parsing. Co-authored-by: Cursor --- .../claude-analyze/__tests__/schema.test.mjs | 21 ++++++++ .github/actions/claude-analyze/schema.mjs | 6 ++- server/api/src/lib/sentry.test.ts | 13 +++++ server/api/src/lib/sentry.ts | 53 +++++++++++++------ 4 files changed, 74 insertions(+), 19 deletions(-) diff --git a/.github/actions/claude-analyze/__tests__/schema.test.mjs b/.github/actions/claude-analyze/__tests__/schema.test.mjs index 06df8d2e..681a19f9 100644 --- a/.github/actions/claude-analyze/__tests__/schema.test.mjs +++ b/.github/actions/claude-analyze/__tests__/schema.test.mjs @@ -95,6 +95,27 @@ test("parseAndValidate strips Claude's ```json``` fence and prose preamble", () assert.equal(validated.ai_summary, "wrapped in fence"); }); +test("parseAndValidate does not strip a plain ``` fenced block without the json label", () => { + const payload = { + severity: "low", + ai_summary: "plain fence skipped", + ai_root_cause: null, + ai_suggested_fix: null, + ai_suspected_files: null, + }; + const wrapped = [ + "Here is a code sample:", + "```", + "not json", + "```", + "", + JSON.stringify(payload), + ].join("\n"); + const validated = parseAndValidate(wrapped); + assert.equal(validated.severity, "low"); + assert.equal(validated.ai_summary, "plain fence skipped"); +}); + test("parseAndValidate throws when no JSON object is present", () => { assert.throws(() => parseAndValidate("nope, no braces here"), /valid analysis JSON/i); }); diff --git a/.github/actions/claude-analyze/schema.mjs b/.github/actions/claude-analyze/schema.mjs index 5212a72e..df163d11 100644 --- a/.github/actions/claude-analyze/schema.mjs +++ b/.github/actions/claude-analyze/schema.mjs @@ -130,14 +130,16 @@ function looksLikeAnalysisPayload(value) { /** * ```json ... ``` フェンスがあれば内側だけを返す(なければそのまま)。 + * 言語ラベル無しの ``` ... ``` は JSON とみなさずそのままにする(誤ってコードブロックを剥がさない)。 * - * If a ```json ... ``` fence exists, return the inner body; otherwise raw. + * If an explicit ```json ... ``` fence exists, return the inner body; otherwise raw. + * Plain ``` ... ``` fences are left intact so non-JSON fenced blocks are not stripped. * * @param {string} raw * @returns {string} */ function stripMarkdownJsonFence(raw) { - const m = raw.match(/```(?:json)?\s*([\s\S]*?)```/im); + const m = raw.match(/```json\s*([\s\S]*?)```/im); return m ? m[1].trim() : raw; } diff --git a/server/api/src/lib/sentry.test.ts b/server/api/src/lib/sentry.test.ts index 75cd111c..a3bd3f01 100644 --- a/server/api/src/lib/sentry.test.ts +++ b/server/api/src/lib/sentry.test.ts @@ -121,6 +121,19 @@ describe("scrubSentryEvent", () => { expect(scrubbed.request?.url).toBe("https://api.example.com/api/pages/[uuid]"); }); + it("scrubs relative request.url paths using a synthetic base URL", () => { + const event = makeEvent({ + request: { + url: "/api/pages/550e8400-e29b-41d4-a716-446655440000?token=secret&safe=1", + headers: {}, + }, + }); + + const scrubbed = scrubSentryEvent(event); + + expect(scrubbed.request?.url).toBe("https://sentry-request-url-scrub.invalid/api/pages/[uuid]"); + }); + it("scrubs user, extra, contexts, tags, and breadcrumb data", () => { const event = makeEvent({ user: { id: "user-1", email: "user@example.com" }, diff --git a/server/api/src/lib/sentry.ts b/server/api/src/lib/sentry.ts index ed05f988..fdb29f0d 100644 --- a/server/api/src/lib/sentry.ts +++ b/server/api/src/lib/sentry.ts @@ -130,33 +130,52 @@ export function scrubSentryEvent(event: ErrorEvent): ErrorEvent { return event; } +/** + * `new URL(relative)` が失敗する場合のみ使う合成オリジン。 + * Synthetic origin used only when `new URL(absolute)` fails for SDK-recorded paths. + */ +const RELATIVE_REQUEST_URL_BASE = "https://sentry-request-url-scrub.invalid"; + /** * SDK が記録するリクエスト URL からクエリと不透明パス成分を落とす。 * matched route は `routePath` など別チャネルに載せる前提。 * * Strip query strings and redact opaque path segments from the raw URL SDKs * record; structured routing context belongs in `routePath` / extras instead. + * + * 相対パス(例: `/api/pages/...`)もパースできるよう合成ベースで解決する。 + * Relative paths (e.g. `/api/pages/...`) are resolved against a synthetic base so + * scrubbing still applies instead of collapsing everything to `[Filtered]`. */ -function scrubRequestUrl(url: unknown): unknown { - if (typeof url !== "string" || url.length === 0) return url; +function scrubRequestUrl(url: string | undefined): string | undefined { + if (url === undefined) return undefined; + if (typeof url !== "string") return undefined; + if (url.length === 0) return ""; + + let u: URL; try { - const u = new URL(url); - u.search = ""; - const segments = u.pathname.split("/").map((seg) => { - if (seg.length === 0) return seg; - if (/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(seg)) { - return "[uuid]"; - } - if (/^[A-Za-z0-9._~-]{24,}$/.test(seg)) { - return FILTERED; - } - return seg; - }); - u.pathname = segments.join("/") || "/"; - return u.toString(); + u = new URL(url); } catch { - return FILTERED; + try { + u = new URL(url, `${RELATIVE_REQUEST_URL_BASE}/`); + } catch { + return FILTERED; + } } + + u.search = ""; + const segments = u.pathname.split("/").map((seg) => { + if (seg.length === 0) return seg; + if (/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(seg)) { + return "[uuid]"; + } + if (/^[A-Za-z0-9._~-]{24,}$/.test(seg)) { + return FILTERED; + } + return seg; + }); + u.pathname = segments.join("/") || "/"; + return u.toString(); } function scrubShallowRecord | undefined>(input: T): T; From d0b0d6987bff376acd5b4d4fb3181e3f3e65afd2 Mon Sep 17 00:00:00 2001 From: otomatty Date: Mon, 18 May 2026 23:39:16 +0900 Subject: [PATCH 3/3] fix: address PR #908 CodeRabbit review comments Detect analysis root payload by own keys for fast schema failures; reset pending status on Cancel in ErrorDetailDialog. Co-authored-by: Cursor --- .../claude-analyze/__tests__/schema.test.mjs | 19 +++ .github/actions/claude-analyze/schema.mjs | 13 +- .../pages/errors/ErrorDetailDialog.test.tsx | 152 ++++++++++++++++++ admin/src/pages/errors/ErrorDetailDialog.tsx | 11 +- 4 files changed, 187 insertions(+), 8 deletions(-) create mode 100644 admin/src/pages/errors/ErrorDetailDialog.test.tsx diff --git a/.github/actions/claude-analyze/__tests__/schema.test.mjs b/.github/actions/claude-analyze/__tests__/schema.test.mjs index 681a19f9..d533814d 100644 --- a/.github/actions/claude-analyze/__tests__/schema.test.mjs +++ b/.github/actions/claude-analyze/__tests__/schema.test.mjs @@ -133,6 +133,25 @@ test("parseAndValidate finds schema-valid JSON when prose contains stray braces" assert.equal(validated.severity, "low"); }); +test("parseAndValidate fails fast when root-shaped JSON has invalid types (no later-object fallback)", () => { + const badRoot = { + severity: 999, + ai_summary: "severity has wrong type", + ai_root_cause: null, + ai_suggested_fix: null, + ai_suspected_files: null, + }; + const laterObject = { + severity: "low", + ai_summary: "would be wrongly picked without root-intent detection", + ai_root_cause: null, + ai_suggested_fix: null, + ai_suspected_files: null, + }; + const raw = `${JSON.stringify(badRoot)}\nTrailing prose ${JSON.stringify(laterObject)}`; + assert.throws(() => parseAndValidate(raw), /schema validation/i); +}); + test("parseAndValidate throws on empty input", () => { assert.throws(() => parseAndValidate(""), /empty/); }); diff --git a/.github/actions/claude-analyze/schema.mjs b/.github/actions/claude-analyze/schema.mjs index df163d11..17ad8c89 100644 --- a/.github/actions/claude-analyze/schema.mjs +++ b/.github/actions/claude-analyze/schema.mjs @@ -114,18 +114,21 @@ function extractBalancedJsonObject(str, startIdx) { } /** - * severity + ai_summary を持つオブジェクトか(ネストした suspected_files エントリと区別)。 + * トップレベル解析ペイロードの「形」か(severity / ai_summary のキーが両方あるか)。 + * 型までは見ない。誤った型のオブジェクトでもキーが揃っていればルート意図とみなし、 + * 後続の別オブジェクトへフォールスルーしない(誤採択防止)。 * - * Whether `value` looks like the top-level analysis payload rather than a nested - * suspected-file entry (which lacks `ai_summary`). + * Whether `value` looks like the intended root analysis object (both `severity` and + * `ai_summary` own keys). Types are not checked: malformed values still fail fast via + * schema throw instead of scanning for a later unrelated JSON object. * * @param {unknown} value * @returns {boolean} */ function looksLikeAnalysisPayload(value) { - if (!value || typeof value !== "object") return false; + if (!value || typeof value !== "object" || Array.isArray(value)) return false; const o = /** @type {Record} */ (value); - return typeof o.severity === "string" && typeof o.ai_summary === "string"; + return Object.hasOwn(o, "severity") && Object.hasOwn(o, "ai_summary"); } /** diff --git a/admin/src/pages/errors/ErrorDetailDialog.test.tsx b/admin/src/pages/errors/ErrorDetailDialog.test.tsx new file mode 100644 index 00000000..ecb6ce18 --- /dev/null +++ b/admin/src/pages/errors/ErrorDetailDialog.test.tsx @@ -0,0 +1,152 @@ +import React from "react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { ErrorDetailDialog } from "./ErrorDetailDialog"; +import type { ApiErrorRow } from "@/api/admin"; + +vi.mock("@zedi/ui", () => ({ + Badge: ({ children }: { children: React.ReactNode }) => {children}, + Button: ({ + children, + onClick, + disabled, + variant, + }: { + children: React.ReactNode; + onClick?: () => void; + disabled?: boolean; + variant?: string; + }) => ( + + ), + Dialog: ({ + open, + onOpenChange, + children, + }: { + open: boolean; + onOpenChange: (open: boolean) => void; + children: React.ReactNode; + }) => + open ? ( +
+ + {children} +
+ ) : null, + DialogContent: ({ children, className }: { children: React.ReactNode; className?: string }) => ( +
{children}
+ ), + DialogHeader: ({ children }: { children: React.ReactNode }) =>
{children}
, + DialogTitle: ({ children }: { children: React.ReactNode }) =>

{children}

, + DialogDescription: ({ children }: { children: React.ReactNode }) =>

{children}

, + DialogFooter: ({ children }: { children: React.ReactNode }) =>
{children}
, + Select: ({ + value, + onValueChange, + disabled, + }: { + value: string; + onValueChange: (v: string) => void; + disabled?: boolean; + children?: React.ReactNode; + }) => ( + + ), + SelectTrigger: ({ children }: { children: React.ReactNode }) => {children}, + SelectContent: ({ children }: { children: React.ReactNode }) => <>{children}, + SelectItem: ({ children }: { children: React.ReactNode }) => <>{children}, + SelectValue: () => null, +})); + +const baseRow: ApiErrorRow = { + id: "00000000-0000-0000-0000-000000000001", + sentryIssueId: "sentry-1", + fingerprint: null, + title: "TypeError", + route: "GET /api/x", + statusCode: 500, + occurrences: 1, + firstSeenAt: "2026-05-01T00:00:00Z", + lastSeenAt: "2026-05-04T00:00:00Z", + severity: "high", + status: "open", + aiSummary: null, + aiSuspectedFiles: null, + aiRootCause: null, + aiSuggestedFix: null, + githubIssueNumber: null, + createdAt: "2026-05-01T00:00:00Z", + updatedAt: "2026-05-04T00:00:00Z", +}; + +describe("ErrorDetailDialog", () => { + const onClose = vi.fn(); + const onUpdateStatus = vi.fn(); + + beforeEach(() => { + onClose.mockClear(); + onUpdateStatus.mockClear(); + }); + + /** + * キャンセルで閉じたあと同じ行を再度開いたとき、未保存のステータス選択が残らないこと。 + * Cancel clears pending status so reopening the same row does not show a stale draft. + */ + it("discards pending status when Cancel is clicked so reopen shows server status", async () => { + const row = { ...baseRow, status: "open" as const }; + const { rerender } = render( + , + ); + + await userEvent.selectOptions(screen.getByTestId("status-select"), "resolved"); + expect(screen.getByTestId("status-select")).toHaveValue("resolved"); + + await userEvent.click(screen.getByRole("button", { name: "キャンセル" })); + expect(onClose).toHaveBeenCalledTimes(1); + + rerender( + , + ); + + rerender( + , + ); + + expect(screen.getByTestId("status-select")).toHaveValue("open"); + }); +}); diff --git a/admin/src/pages/errors/ErrorDetailDialog.tsx b/admin/src/pages/errors/ErrorDetailDialog.tsx index 4cae22eb..0164158b 100644 --- a/admin/src/pages/errors/ErrorDetailDialog.tsx +++ b/admin/src/pages/errors/ErrorDetailDialog.tsx @@ -59,6 +59,12 @@ export function ErrorDetailDialog({ setPendingFor({ id: row.id, status: next }); }; + /** オーバーレイ・ESC と同様に、キャンセルボタンでも未保存選択を破棄する。 */ + const handleClose = () => { + setPendingFor(null); + onClose(); + }; + if (!row) return null; const effectiveStatus: ApiErrorStatus = pendingStatus ?? row.status; @@ -74,8 +80,7 @@ export function ErrorDetailDialog({ open={row !== null} onOpenChange={(open) => { if (!open) { - setPendingFor(null); - onClose(); + handleClose(); } }} > @@ -194,7 +199,7 @@ export function ErrorDetailDialog({ -