From e55ebfd69a9f6eb4e1d2a7c2fdbf4057a2e2fd99 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 15 Sep 2026 01:29:54 +0200 Subject: [PATCH 1/2] fix(approvals): a readable diff when the proposed body is not JSON, and 6.4.0 The operator's updateLlm call proposed an LLM config with one closing brace too many. It parsed on neither side of the approval, so the stored document was pretty-printed and key-sorted while the proposed one stayed a single raw line: every stored line removed, one line added, nothing to compare. - Re-indent a side that does not parse (whitespace only, never repaired), so a stray bracket shows as its own line. Text that does not open as an object or array is compared as written. - Against a broken side, ignore indentation in the comparison and print the parsed side in the broken side's key order, so neither mis-nesting after a stray closer nor a moved key reads as a change. - Mark the words that changed within paired lines (/, bounded by a per-pair timeout and a render budget), and summarise added, removed and unchanged lines in a sticky legend. - Warn on a whole-document write that is not JSON, and tell the two outcomes apart: broken inside, EDDI rejects it; complete but followed by more text, Jackson stops at the end of the document and the write can succeed with the tail dropped. Hedged when redaction markers are present. - Scan the leading document for capability grants when text trails it. A trailing brace used to skip the scan entirely while EDDI would still store the grant. - Bump the Manager to 6.4.0. --- .design-sync/NOTES.md | 2 +- .design-sync/ds-entry.tsx | 2 +- HANDOFF.md | 8 +- package-lock.json | 4 +- package.json | 2 +- .../agents/resource-diff-viewer.tsx | 230 +++++++++++++++--- .../__tests__/request-preview.test.tsx | 56 +++++ src/components/operator/request-preview.tsx | 44 ++++ .../__tests__/resource-diff-viewer.test.tsx | 105 ++++++++ src/i18n/locales/ar.json | 6 +- src/i18n/locales/de.json | 6 +- src/i18n/locales/en.json | 6 +- src/i18n/locales/es.json | 6 +- src/i18n/locales/fr.json | 6 +- src/i18n/locales/hi.json | 6 +- src/i18n/locales/ja.json | 6 +- src/i18n/locales/ko.json | 6 +- src/i18n/locales/pt.json | 6 +- src/i18n/locales/th.json | 6 +- src/i18n/locales/zh.json | 6 +- src/lib/__tests__/redacted-json.test.ts | 42 +++- src/lib/__tests__/reindent-json.test.ts | 75 ++++++ .../__tests__/escalation-flags.test.ts | 9 + src/lib/operator/escalation-flags.ts | 11 +- src/lib/redacted-json.ts | 39 +++ src/lib/reindent-json.ts | 132 ++++++++++ 26 files changed, 772 insertions(+), 55 deletions(-) create mode 100644 src/lib/__tests__/reindent-json.test.ts create mode 100644 src/lib/reindent-json.ts diff --git a/.design-sync/NOTES.md b/.design-sync/NOTES.md index 650ba3e9..800a97b5 100644 --- a/.design-sync/NOTES.md +++ b/.design-sync/NOTES.md @@ -266,7 +266,7 @@ grep -o -- '--color-warning[a-z-]*:' .design-sync/.cache/compiled.css | sort -u - **`cfg.dtsPropsFor` is hand-maintained and nothing verifies it against the source.** It is the highest-value thing in the config (see the section above) and the easiest to let rot — a prop renamed in `src/` leaves the design agent coding against the old name. -- **`__APP_VERSION__` in `ds-entry.tsx` is a hardcoded `"6.3.0"`.** Cosmetic (it only feeds +- **`__APP_VERSION__` in `ds-entry.tsx` is a hardcoded `"6.4.0"`.** Cosmetic (it only feeds the sidebar footer), but bump it when the app's major/minor moves, or accept the drift. - **`src/assets/logo_eddi.png` must stay importable.** Moving it back under `public/` silently reintroduces a broken image in every design using the expanded `Sidebar`. diff --git a/.design-sync/ds-entry.tsx b/.design-sync/ds-entry.tsx index 2e20f80f..3391f647 100644 --- a/.design-sync/ds-entry.tsx +++ b/.design-sync/ds-entry.tsx @@ -15,7 +15,7 @@ // component untouched. The value is cosmetic chrome; it is a static string and // will drift from package.json (recorded in NOTES.md's re-sync risks). (globalThis as unknown as { __APP_VERSION__?: string }).__APP_VERSION__ ??= - "6.3.0"; + "6.4.0"; // ── ui/ ──────────────────────────────────────────────────────────────────── export { AccessibleDialog } from "@/components/ui/accessible-dialog"; diff --git a/HANDOFF.md b/HANDOFF.md index 5ed163fc..ace7052d 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -1,6 +1,6 @@ # EDDI Manager Project Handoff -## Current Status (v6.3.0, plus the post-6.3.0 EDDI alignment in #197-#203) +## Current Status (v6.4.0, plus the post-6.3.0 EDDI alignment in #197-#203) ### How a linked account is actually consumed (traced end to end, after getting it wrong once) @@ -69,6 +69,11 @@ when the agent makes an outbound call; chat attachments are uploads via enumerate a user's remote content, so a picker would need a new backend surface. ### Completed Phases +- **An approval diff you can read when the proposed body is not JSON** (branch: `fix/approval-diff-invalid-json`), plus the Manager version to **6.4.0**. Reported from a live pause: the operator's `updateLlm` call proposed an LLM config with one closing brace too many (`…60000}}]}`). It parsed on neither side of the approval, so the stored document was pretty-printed and key-sorted while the proposed one stayed a single raw line — every stored line red, one green line, nothing comparable. **6511 passing (411 files)**; each review fix below was mutation-checked (reverting it fails a named test). + - **The broken side is re-indented, never repaired** (`src/lib/reindent-json.ts`). Whitespace outside strings is the only thing it touches, so a stray bracket stays visible as its own line. Text that does not open with `{`/`[` is compared as written. Against a broken side the diff ignores indentation — its nesting is wrong from the first stray closer on, and comparing indentation reported every later line as changed — and prints the parsed side in the broken side's key order (`jsonKeyOrder`), since alphabetical sorting of one side only turned moved keys into changes. + - **What changed, and what did not.** Paired changed lines mark the words that differ (``/``, with a per-pair timeout and a render budget — two long dissimilar lines cost over a second each), and the sticky legend reads "Lines: N added · N removed · N unchanged". + - **Two failure modes, two warnings.** A body broken *inside* the document is rejected by EDDI. A complete document followed by more text is not: Jackson's default reader stops at the end of the first value, and EDDI's `StrictConfigurationParser` only adds `FAIL_ON_UNKNOWN_PROPERTIES`, so the write can succeed with the tail silently dropped. `parseLeadingJson` tells them apart; the approval preview says which, hedges when redaction markers are present, and hides the viewer's own caveat so the warning is not repeated. + - **A trailing brace was a way past the capability scan.** `detectEscalationFlags` skipped any body that failed to parse, so appending `}` to a group create hid `dynamicAgents.allowCreation` while EDDI would still store it. It now scans the leading document. - **Connections review findings** (branch: `fix/connections-review-findings`): a second review of the connections feature against the backend's current contract and the fixes landing in parallel there. Seven items, one commit each; **6188 passing (388 files)**, +101 tests and 3 files against the 6158 recorded below. The full run on this machine shows **60 failures in 8 files that are not this branch's** — `updates.test.ts`, `update-check-card`, `update-banner`, the operator canaries and page, `use-platform-status`, `use-send-message-409-rollback` — every one a raw `fetch` (external host, `credentials: "omit"` + `signal`, or an SSE stream) that MSW did not intercept here, so the request hit a network that is not there. The same 10 `updates.test.ts` failures reproduce at the base commit `184fde17` in the same worktree; CI, where these pass, is the source of truth for them. - **`CALLER_SUPPLIED` is the third binding, and the Manager now knows it.** `binding` was derived from the auth type alone, so a caller-supplied document loaded into the editor was saved back as `SERVICE` on the first unrelated edit — with an empty `valueTemplate` the backend refuses on that binding. `bindingFor(authType, requested)` honours a requested binding where the type allows it (STATIC has two legal values; every other type exactly one), `legalBindings` names the pairs, and a loaded document keeps its stored binding. The STATIC form offers the choice as a two-way radio group with the doc's reasoning — authority, not convenience: an agent holding one shared key can reach everything that key can — and, for caller-supplied, replaces the header-value field with the exact `X-EDDI-Connection-Credential: ` line the integrator must send. The wizard now builds its document through `toStoredConnection` like the editor, so a template typed before switching the binding is not sent. The badge has a third look; the MSW fixture (`conn6`, gnowbe) carries the shape. - **The mirror carries the backend's newer write rules**: the name grammar `^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$`, `timeoutMs` in 1..60000 (null is the default), the literal-segment rule on `valueTemplate` (a literal half over 32 characters or with a run of 12+ from the credential alphabet — `Bearer ${vault:k}` passes, `sk-live-abcdef${vault:x}` does not; checked on the literal halves only, so a long vault key *name* never trips it), and `extraAuthParams` value rules (no `${`, ≤ 512 characters, not one long token-alphabet run) plus reserved OAuth names refused stripped of case and separators. The `code_verifier` note above is corrected: the backend fixed its denylist spelling, so normalising both sides is now the same rule. The backend's own 400 still reaches the toast verbatim, and a test pins it. @@ -455,6 +460,7 @@ enumerate a user's remote content, so a picker would need a new backend surface. - 112 Backend tenancy tests passing (`mvn test`) ### Last Commit Focus +- Frontend: the approval diff against a non-JSON body, and 6.4.0 (above). Worth carrying forward: **"not JSON" is two different outcomes on the backend.** Check what EDDI's reader actually does with a malformed body before writing a warning about it — here the obvious sentence ("EDDI will reject this") was wrong for the case that matters most, the one where the write goes through. - Frontend: the post-6.3.0 EDDI alignment above (#197-#203). Three things worth carrying forward: - **A passing test hid several of these bugs.** The GDPR counter typo survived because its test asserted the label and not the value; the MSW fixture had been written to match the page rather than the server. When a fixture and a client agree, neither is evidence. Fixtures here now describe what EDDI actually sends, including an export that answers 207. - **Dead API surface is a defect, not spare capacity.** The first cut of #197 shipped a `headerCount` helper, a `postWithResponse` method and a header accessor with no callers anywhere — the same fault the same set of PRs was fixing in `transferOwnership`. All three were cut before merge; `ApiResponse` carries the status and nothing else. Add the accessor with its consumer, not before it. diff --git a/package-lock.json b/package-lock.json index d8882e0d..04f13f19 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "eddi-manager", - "version": "6.3.0", + "version": "6.4.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "eddi-manager", - "version": "6.3.0", + "version": "6.4.0", "dependencies": { "@dnd-kit/core": "^6.3.1", "@dnd-kit/sortable": "^10.0.0", diff --git a/package.json b/package.json index 934504d9..7a5af174 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "eddi-manager", "private": true, - "version": "6.3.0", + "version": "6.4.0", "type": "module", "scripts": { "dev": "vite", diff --git a/src/components/agents/resource-diff-viewer.tsx b/src/components/agents/resource-diff-viewer.tsx index 5a264847..905cc278 100644 --- a/src/components/agents/resource-diff-viewer.tsx +++ b/src/components/agents/resource-diff-viewer.tsx @@ -1,8 +1,9 @@ import { useMemo, useState } from "react"; import { useTranslation } from "react-i18next"; -import { diffLines, type ChangeObject } from "diff"; +import { diffLines, diffWordsWithSpace, type ChangeObject } from "diff"; import { Equal, AlertTriangle } from "lucide-react"; import { parseRedactedJson } from "@/lib/redacted-json"; +import { jsonKeyOrder, reindentJsonText } from "@/lib/reindent-json"; interface ResourceDiffViewerProps { sourceContent: string | null; @@ -13,6 +14,12 @@ interface ResourceDiffViewerProps { * "Proposed", which is what an approver is actually looking at. */ labels?: { target: string; source: string }; + /** + * Whether to explain, above the rows, that a side is not valid JSON. Off + * for a caller that already says so more pointedly — the approval preview — + * so the reader does not get the same warning twice in a row. + */ + showRawComparisonNotice?: boolean; } /** Unchanged lines kept either side of a change, git-style. */ @@ -20,12 +27,38 @@ const CONTEXT_LINES = 3; /** A gap this short is not worth a fold — the fold row costs a line itself. */ const MIN_COLLAPSIBLE_GAP = 4; const NO_GAPS: ReadonlySet = new Set(); +/** + * Below this share of characters in common (indentation aside), a removed and + * an added line are a replacement, not an edit — whole-line colour says that + * more plainly than a scatter of highlighted fragments. + */ +const MIN_SHARED_RATIO = 0.4; +/** Word-diffing is not linear; past this a line keeps whole-line colour only. */ +const MAX_WORD_DIFF_LENGTH = 10_000; +/** + * Word-diffing runs on the main thread during render. Two long lines with + * little in common cost over a second each, so every pair gets a ceiling and + * the whole diff a budget; whatever runs out keeps whole-line colour. + */ +const WORD_DIFF_TIMEOUT_MS = 50; +const WORD_DIFF_BUDGET_MS = 250; type LineKind = "added" | "removed" | "context"; +interface Segment { + text: string; + changed: boolean; +} + interface DiffLine { kind: LineKind; text: string; + /** + * For a changed line paired with its counterpart on the other side: which + * parts of it actually differ. On a long line — a system prompt is one JSON + * string — whole-line colour alone leaves the reader hunting for the edit. + */ + segments?: Segment[]; } type DiffRow = { row: "line"; line: DiffLine } | { row: "gap"; id: number; count: number }; @@ -34,6 +67,8 @@ interface ComputedDiff { lines: DiffLine[]; /** True when at least one side could not be parsed and the comparison is raw text. */ rawComparison: boolean; + /** Line counts per kind, for the header — what did NOT change matters too. */ + counts: Record; } /** @@ -48,24 +83,36 @@ export function ResourceDiffViewer({ sourceContent, targetContent, labels, + showRawComparisonNotice = true, }: ResourceDiffViewerProps) { const { t } = useTranslation(); const diff = useMemo(() => { if (!sourceContent && !targetContent) return null; - const source = normalizeJson(sourceContent); - const target = normalizeJson(targetContent); + const source = readSide(sourceContent); + const target = readSide(targetContent); - if (source.text === target.text) return "identical"; + // One unparseable side is enough to drop the comparison to text, and the + // reader has to be told. That side is re-indented as written; its keys + // cannot be sorted, and sorting only the other side would turn every key + // that sits somewhere else into a change. So the parsed side is printed in + // the order the broken one was written in instead. + const brokenText = source.kind === "text" ? source.content : target.kind === "text" ? target.content : null; + const keyOrder = brokenText === null ? "sorted" : jsonKeyOrder(brokenText); + const sourceText = printSide(source, keyOrder); + const targetText = printSide(target, keyOrder); - return { - lines: toDiffLines(diffLines(target.text, source.text)), - // One unparseable side is enough to drop the comparison to raw text, and - // the reader has to be told: the other side is still re-printed, so a - // difference of formatting alone then renders as a whole-document rewrite. - rawComparison: !source.parsed || !target.parsed, - }; + if (sourceText === targetText) return "identical"; + + // A re-indented broken side nests wrongly from its first stray bracket on, + // so comparing indentation would report every later line as changed. There + // it is layout, not content, and is left out of the comparison. + const changes = diffLines(targetText, sourceText, { ignoreWhitespace: brokenText !== null }); + const lines = markChangedWords(toDiffLines(changes)); + const counts: Record = { added: 0, removed: 0, context: 0 }; + for (const line of lines) counts[line.kind]++; + return { lines, rawComparison: brokenText !== null, counts }; }, [sourceContent, targetContent]); // Expanded folds are remembered against the diff they belong to. Gap ids are @@ -103,7 +150,10 @@ export function ResourceDiffViewer({ {/* The header doubles as the legend: which colour is which side is the first thing a reader needs and the thing a bare "Target → Source" never said. */} -
+ {/* Sticky, so which side is which — and how much of the document did not + change — stays in view while scrolling a long diff. Opaque for the + same reason: rows would otherwise show through it. */} +
- {diff.rawComparison && ( + {diff.rawComparison && showRawComparisonNotice && (

)} @@ -179,7 +237,24 @@ export function ResourceDiffViewer({ would collapse; `wrap-anywhere` wraps an over-long line instead of pushing every other line behind a scrollbar. */} - {entry.line.text || " "} + {entry.line.segments + ? entry.line.segments.map((segment, s) => + !segment.changed ? ( + segment.text + ) : entry.line.kind === "added" ? ( + // / rather than a styled span, so the markup + // says which words changed and not only the colour — + // wherever that is exposed, and to the tests. + + {segment.text} + + ) : ( + + {segment.text} + + ), + ) + : entry.line.text || " "}

), @@ -189,37 +264,50 @@ export function ResourceDiffViewer({ ); } -interface NormalizedJson { - text: string; - /** False when the content could not be parsed and is compared as raw text. */ - parsed: boolean; -} +/** One side of the comparison, read but not yet printed. */ +type Side = { kind: "empty" } | { kind: "json"; value: unknown } | { kind: "text"; content: string }; -/** Deep-sort all keys recursively and re-print, for stable diffing */ -function normalizeJson(content: string | null): NormalizedJson { - if (!content) return { text: "", parsed: true }; +function readSide(content: string | null): Side { + if (!content) return { kind: "empty" }; // Tolerant of the one malformed shape EDDI's redaction filter produces — see // `redacted-json.ts`. Without it a single credential field costs the reader - // the whole diff: the body falls back to its raw single line, and every line - // of the other side reads as deleted. + // the key-sorted comparison for the whole document. const parsed = parseRedactedJson(content); - if (parsed.ok) return { text: printJson(parsed.value), parsed: true }; - - return { text: content, parsed: false }; + return parsed.ok ? { kind: "json", value: parsed.value } : { kind: "text", content }; } -function printJson(value: unknown): string { - return JSON.stringify(deepSortKeys(value), null, 2); +/** + * Print a side for diffing: deep key-sorted when both sides parse, otherwise + * in the key order of the side that did not (see `jsonKeyOrder`). + */ +function printSide(side: Side, keyOrder: "sorted" | Map): string { + switch (side.kind) { + case "empty": + return ""; + case "json": { + // A key the broken side never names ranks last, keeping its own order. + const compare = + keyOrder === "sorted" + ? undefined + : (a: string, b: string) => (keyOrder.get(a) ?? keyOrder.size) - (keyOrder.get(b) ?? keyOrder.size); + return JSON.stringify(deepSortKeys(side.value, compare), null, 2); + } + case "text": + // Re-indenting is for text that is trying to be JSON. Anything else is + // compared as it came: its line breaks are its layout. + return /^\s*[[{]/.test(side.content) ? reindentJsonText(side.content) : side.content; + } } -function deepSortKeys(value: unknown): unknown { - if (Array.isArray(value)) return value.map(deepSortKeys); +/** Re-key every object, recursively, in `compare` order (default: alphabetical). */ +function deepSortKeys(value: unknown, compare?: (a: string, b: string) => number): unknown { + if (Array.isArray(value)) return value.map((item) => deepSortKeys(item, compare)); if (value !== null && typeof value === "object") { return Object.keys(value as Record) - .sort() + .sort(compare) .reduce>((acc, key) => { - acc[key] = deepSortKeys((value as Record)[key]); + acc[key] = deepSortKeys((value as Record)[key], compare); return acc; }, {}); } @@ -239,6 +327,80 @@ function toDiffLines(changes: ChangeObject[]): DiffLine[] { return lines; } +/** + * Pair each run of removed lines with the run of added lines right after it, + * line by line, and mark the words that differ within each pair. + * + * jsdiff emits a replaced block as removals then additions, so index pairing + * lines up an edited field with its new value. Where the counts differ the + * extra lines stay unpaired, and a pair with too little in common is left + * alone — pairing is a guess, and a bad guess must degrade to plain whole-line + * colour, never to highlighting that implies an edit which was not made. + */ +function markChangedWords(lines: DiffLine[]): DiffLine[] { + const deadline = performance.now() + WORD_DIFF_BUDGET_MS; + let i = 0; + while (i < lines.length) { + if (lines[i]!.kind !== "removed") { + i++; + continue; + } + const removedStart = i; + while (i < lines.length && lines[i]!.kind === "removed") i++; + const addedStart = i; + while (i < lines.length && lines[i]!.kind === "added") i++; + + const pairs = Math.min(addedStart - removedStart, i - addedStart); + for (let p = 0; p < pairs; p++) { + const remaining = deadline - performance.now(); + if (remaining <= 0) return lines; + const removed = lines[removedStart + p]!; + const added = lines[addedStart + p]!; + const segments = wordSegments(removed.text, added.text, Math.min(WORD_DIFF_TIMEOUT_MS, remaining)); + if (segments) { + removed.segments = segments.removed; + added.segments = segments.added; + } + } + } + return lines; +} + +function wordSegments( + before: string, + after: string, + timeout: number, +): { removed: Segment[]; added: Segment[] } | null { + if (before.length > MAX_WORD_DIFF_LENGTH || after.length > MAX_WORD_DIFF_LENGTH) return null; + + // `undefined` when the timeout ran out first. + const parts = diffWordsWithSpace(before, after, { timeout }); + if (!parts) return null; + + const removed: Segment[] = []; + const added: Segment[] = []; + let shared = 0; + for (const part of parts) { + if (part.added) added.push({ text: part.value, changed: true }); + else if (part.removed) removed.push({ text: part.value, changed: true }); + else { + shared += part.value.length; + removed.push({ text: part.value, changed: false }); + added.push({ text: part.value, changed: false }); + } + } + + // Shared indentation says nothing about whether two lines are the same field. + const indent = Math.min(leadingWhitespace(before), leadingWhitespace(after)); + const longest = Math.max(before.length, after.length) - indent; + if (longest <= 0 || shared - indent < MIN_SHARED_RATIO * longest) return null; + return { removed, added }; +} + +function leadingWhitespace(text: string): number { + return text.length - text.trimStart().length; +} + /** * Fold long runs of unchanged lines away. * diff --git a/src/components/operator/__tests__/request-preview.test.tsx b/src/components/operator/__tests__/request-preview.test.tsx index 99c0ffa9..880f2617 100644 --- a/src/components/operator/__tests__/request-preview.test.tsx +++ b/src/components/operator/__tests__/request-preview.test.tsx @@ -278,6 +278,62 @@ describe("RequestPreview — whole-document PUT diff", () => { expect(diff).toHaveTextContent("Stored v3"); expect(diff).toHaveTextContent("Proposed"); expect(diff).not.toHaveTextContent(/Target/); + expect(screen.queryByTestId("request-preview-body-issue-c1")).not.toBeInTheDocument(); + }); + + describe("a proposed document that isn't JSON — the extra-brace report", () => { + // The operator sent an updateLlm body with one closing brace too many. It + // parsed on neither side of the approval, so the diff showed the compact + // body as a single added line and the capability scan found nothing. + + it("says extra text after a complete document may be dropped on write, and still lines the diff up", async () => { + serveStored(); + renderWithProviders( + , + ); + + const warning = await screen.findByTestId("request-preview-body-issue-c1"); + expect(warning).toHaveAttribute("role", "alert"); + expect(warning).toHaveAttribute("data-issue", "trailingText"); + expect(warning).toHaveTextContent(/drop the rest/); + + const diff = await screen.findByTestId("request-preview-diff-c1"); + expect(diffRows(diff, "added")).toEqual([' "threshold": 9,', "}"]); + expect(diffRows(diff, "removed")).toEqual([' "threshold": 5,']); + expect(diffRows(diff).filter((line) => line.includes("unchanged"))).toHaveLength(1); + // Warned once, above the diff — not again inside it. + expect(within(diff).queryByTestId("diff-raw-comparison")).not.toBeInTheDocument(); + }); + + it("says a document broken inside will be rejected", async () => { + serveStored(); + renderWithProviders( + , + ); + + const warning = await screen.findByTestId("request-preview-body-issue-c1"); + expect(warning).toHaveAttribute("data-issue", "notJson"); + expect(warning).toHaveTextContent(/will reject this write/); + }); + + it("hedges when redacted credentials are in the body, since the preview is not the exact request", async () => { + serveStored(); + renderWithProviders( + ",}') })} + pinned + callId="c1" + />, + ); + + const warning = await screen.findByTestId("request-preview-body-issue-c1"); + expect(warning).toHaveAttribute("data-issue", "notJsonRedacted"); + expect(warning).toHaveTextContent(/most likely reject/); + }); }); it("survives the redaction filter's mangled credential field — the shape from the report", async () => { diff --git a/src/components/operator/request-preview.tsx b/src/components/operator/request-preview.tsx index c60490ca..782e86f5 100644 --- a/src/components/operator/request-preview.tsx +++ b/src/components/operator/request-preview.tsx @@ -4,6 +4,7 @@ import { useQuery } from "@tanstack/react-query"; import { ShieldAlert, Loader2 } from "lucide-react"; import { detectEscalationFlags } from "@/lib/operator/escalation-flags"; import { resolveConfigWriteTarget, bodyHasRedactions } from "@/lib/operator/config-write-target"; +import { parseLeadingJson, parseRedactedJson } from "@/lib/redacted-json"; import { ResourceDiffViewer } from "@/components/agents/resource-diff-viewer"; import { getResource } from "@/lib/api/resources"; import { useAuth } from "@/hooks/use-auth"; @@ -73,6 +74,24 @@ export function RequestPreview({ preview, pinned, callId }: RequestPreviewProps) * most of the config". */ const writeTarget = preview.bodyTruncated ? null : resolveConfigWriteTarget(preview); + // A whole-document config write has to be JSON. When it is not, the capability + // scan can say nothing and the diff can only line text up — both quiet + // failures an approver would read as "looks fine". Observed: the operator sent + // an LLM config with one closing brace too many. A non-JSON body on any other + // call is ordinary (a form post), so this stays scoped to document writes. + // + // Two different outcomes, so two different warnings. Broken inside the + // document, the write fails. Complete but followed by more text, Jackson's + // default reader stops at the end of the document and never reads the rest — + // the write can succeed with the tail silently dropped. + const bodyIssue: "trailingText" | "notJson" | "notJsonRedacted" | null = + !writeTarget || !preview.body || parseRedactedJson(preview.body).ok + ? null + : parseLeadingJson(preview.body).ok + ? "trailingText" + : bodyHasRedactions(preview.body) + ? "notJsonRedacted" + : "notJson"; // Reading the stored document needs eddi-admin/eddi-editor, and this surface // is used by eddi-approver — whose entire job is approving. Gate the fetch // rather than firing a 403 on every pause, and say plainly that the @@ -168,6 +187,30 @@ export function RequestPreview({ preview, pinned, callId }: RequestPreviewProps) )}

)} + {bodyIssue && ( +

+

+ )} {showDiff && (

@@ -182,6 +225,7 @@ export function RequestPreview({ preview, pinned, callId }: RequestPreviewProps) target: t("operator.approval.diffStored", "Stored v{{version}}", { version: writeTarget!.version }), source: t("operator.approval.diffProposed", "Proposed"), }} + showRawComparisonNotice={!bodyIssue} /> {bodyHasRedactions(preview.body) && (

{ expect(screen.getByText("+")).toBeInTheDocument(); }); + it("compares text that isn't trying to be JSON as it came, line breaks and spaces intact", () => { + renderDiff("line one\nline two changed", "line one\nline two"); + expect(rowTexts("context")).toEqual(["line one"]); + expect(rowTexts("removed")).toEqual(["line two"]); + expect(rowTexts("added")).toEqual(["line two changed"]); + }); + it("handles one side being null (entirely new content)", () => { renderDiff('{"new": true}', null); // All lines should be additions @@ -114,6 +121,54 @@ describe("ResourceDiffViewer", () => { expect(screen.getByTestId("diff-raw-comparison")).toBeInTheDocument(); }); + it("lays a body that isn't JSON out line by line, so only what differs is a change", () => { + // The reported case: a compact config body with one closing brace too + // many. Compared as it came, that was one added line against a fully + // removed stored document — no way to see the change, or the stray brace. + renderDiff('{"a":1,"b":{"c":2}}}', '{"a":1,"b":{"c":3}}'); + + expect(screen.getByTestId("diff-raw-comparison")).toBeInTheDocument(); + expect(rowTexts("added")).toEqual([' "c": 2', "}"]); + expect(rowTexts("removed")).toEqual([' "c": 3']); + expect(rowTexts("context")).toContain(' "a": 1,'); + expect(rowTexts().filter((line) => line.includes('"a"'))).toHaveLength(1); + }); + + it("does not report every line after a stray closing bracket as changed", () => { + // One `}` too many EARLY in a document re-indents everything after it one + // level short. That is layout, not content: only the extra bracket counts. + renderDiff('{"a":{"b":1}},"c":2,"d":3}', '{"a":{"b":1},"c":2,"d":3}'); + + expect(rowTexts("removed")).toEqual([]); + expect(rowTexts("added").map((line) => line.trim())).toEqual(["}"]); + expect(rowTexts("context").map((line) => line.trim())).toContain('"d": 3'); + }); + + it("prints the parsed side in the broken side's key order, so a moved key is not a change", () => { + // Sorting the parsed side alphabetically put `a` before `b`, and both then + // read as changed against a body that wrote `b` first. + renderDiff('{"b":1,"a":2,"c":}', '{"a":2,"b":1,"c":3}'); + + expect(rowTexts("removed").map((line) => line.trim())).toEqual(['"c": 3']); + expect(rowTexts("added").map((line) => line.trim())).toEqual(['"c":']); + }); + + it("leaves the caveat out when the caller already warns about the invalid JSON", () => { + render(); + expect(screen.getAllByTestId("diff-line").length).toBeGreaterThan(0); + expect(screen.queryByTestId("diff-raw-comparison")).not.toBeInTheDocument(); + }); + + it("does not sort the parsed side against one that can't be sorted", () => { + // Sorting only the valid side would move "z" below "a" and report both + // as changed; the broken side keeps the order it was written in. + renderDiff('{"z":1,"a":2,"b":}', '{"z":1,"a":2,"b":3}'); + + expect(rowTexts("context")).toContain(' "z": 1,'); + expect(rowTexts().filter((line) => line.includes('"z"'))).toHaveLength(1); + expect(rowTexts("removed")).toEqual([' "b": 3']); + }); + it("shows no caveat when both sides parse", () => { renderDiff('{"a": 1}', '{"a": 2}'); expect(screen.queryByTestId("diff-raw-comparison")).not.toBeInTheDocument(); @@ -195,6 +250,56 @@ describe("ResourceDiffViewer", () => { }); }); + describe("within a changed line", () => { + /** The marked fragments on rows of one kind, in order. */ + function changedWords(kind: "added" | "removed"): string[] { + return screen + .queryAllByTestId("diff-changed-words") + .filter((el) => el.closest('[data-testid="diff-line"]')?.getAttribute("data-diff-kind") === kind) + .map((el) => el.textContent ?? ""); + } + + it("marks only the words that changed, so an edit to a long line can be found", () => { + // A system prompt is one JSON string, so one changed word used to colour + // a paragraph-long line with nothing to say where in it the edit was. + renderDiff( + JSON.stringify({ systemMessage: "You are a friendly support agent who answers briefly." }), + JSON.stringify({ systemMessage: "You are a friendly sales agent who answers briefly." }), + ); + + expect(changedWords("removed")).toEqual(["sales"]); + expect(changedWords("added")).toEqual(["support"]); + // Announced as a change, not only coloured as one. + const [removed, added] = screen.getAllByTestId("diff-changed-words"); + expect(removed!.tagName).toBe("DEL"); + expect(added!.tagName).toBe("INS"); + // The row still reads as the whole line. + expect(rowTexts("added")).toEqual([' "systemMessage": "You are a friendly support agent who answers briefly."']); + }); + + it("colours a replaced line whole instead of highlighting fragments it merely shares", () => { + renderDiff('{"a": "a completely different sentence stands here"}', '{"a": 1}'); + expect(screen.queryAllByTestId("diff-changed-words")).toHaveLength(0); + }); + + it("leaves lines that have no counterpart unmarked — the stray brace in the report", () => { + renderDiff('{"a":1,"b":{"c":2}}}', '{"a":1,"b":{"c":3}}'); + expect(changedWords("added")).toEqual(["2"]); + expect(changedWords("removed")).toEqual(["3"]); + // The extra "}" is an added row with nothing to pair against. + expect(rowTexts("added")).toContain("}"); + }); + }); + + describe("summary", () => { + it("says how much changed AND how much did not", () => { + const many = Object.fromEntries(Array.from({ length: 20 }, (_, i) => [`k${String(i).padStart(2, "0")}`, i])); + renderDiff(JSON.stringify({ ...many, k00: 999 }), JSON.stringify(many)); + // 22 printed lines: the braces and twenty keys, one of them changed. + expect(screen.getByTestId("diff-summary")).toHaveTextContent("Lines: 1 added · 1 removed · 21 unchanged"); + }); + }); + describe("legend", () => { it("names the sides by the caller's vocabulary when given", () => { render( diff --git a/src/i18n/locales/ar.json b/src/i18n/locales/ar.json index e8a41b25..fb326bc3 100644 --- a/src/i18n/locales/ar.json +++ b/src/i18n/locales/ar.json @@ -2977,7 +2977,8 @@ "targetContent": "الهدف", "sourceContent": "المصدر", "diffHiddenLines": "إظهار {{lines}} أسطر دون تغيير", - "diffRawComparison": "ليس JSON صالحًا — تجري المقارنة كنص عادي، لذا يظهر اختلاف التنسيق وحده أيضًا كتغيير.", + "diffSummary": "الأسطر: {{added}} مضافة · {{removed}} محذوفة · {{unchanged}} دون تغيير", + "diffRawComparison": "ليس JSON صالحًا — يُعرض بإعادة المسافات البادئة كما كُتب ويُقارن سطرًا بسطر مع تجاهل المسافات البادئة.", "partialTitle": "لم يتم استيراد بعض الموارد", "partialBody": "تمت كتابة بقية الوكيل. بقيت هذه الموارد كما هي وتحتاج إلى معالجة.", "partialCounts": "{{created}} أُنشئت، {{updated}} حُدِّثت، {{failed}} أخفقت.", @@ -4328,6 +4329,9 @@ "diffFailed": "تعذّر تحميل الإصدار المخزَّن للمقارنة — المستند المقترح كاملًا موجود أدناه.", "diffForbidden": "تتطلب المقارنة مع الإصدار المخزَّن صلاحية محرّر — المستند المقترح كاملًا موجود أدناه.", "diffRedactionNote": "قيم بيانات الاعتماد مخفيّة في الإصدار المقترح، لذا تظهر هنا كتغييرات حتى لو لم تتغير.", + "proposedNotJson": "المستند المقترح ليس JSON صالحًا، لذا سيرفض EDDI عملية الكتابة هذه. كما تعذّر فحصه بحثًا عن منح صلاحيات إضافية — ارفضها واطلب طلبًا مصحّحًا.", + "proposedNotJsonRedacted": "المستند المقترح ليس JSON صالحًا حتى مع مراعاة بيانات الاعتماد المخفيّة في هذه المعاينة، لذا من المرجح جدًا أن يرفض EDDI عملية الكتابة هذه. كما تعذّر فحصه بحثًا عن منح صلاحيات إضافية — ارفضها واطلب طلبًا مصحّحًا.", + "proposedTrailingText": "يحتوي المستند المقترح على نص إضافي بعد نهايته. قد يحفظ EDDI المستند الذي يسبقه فقط ويتجاهل الباقي دون تنبيه، ولم يُفحص بحثًا عن منح صلاحيات إضافية إلا ذلك الجزء — ارفضها واطلب طلبًا مصحّحًا.", "diffStored": "المخزَّن الإصدار {{version}}", "diffProposed": "المقترح", "showFullRequest": "عرض المستند المقترح كاملًا", diff --git a/src/i18n/locales/de.json b/src/i18n/locales/de.json index 939dc1aa..256baf93 100644 --- a/src/i18n/locales/de.json +++ b/src/i18n/locales/de.json @@ -3027,7 +3027,8 @@ "targetContent": "Ziel", "sourceContent": "Quelle", "diffHiddenLines": "{{lines}} unveränderte Zeilen anzeigen", - "diffRawComparison": "Kein gültiges JSON – Vergleich als reiner Text, daher erscheint auch ein reiner Formatierungsunterschied als Änderung.", + "diffSummary": "Zeilen: {{added}} hinzugefügt · {{removed}} entfernt · {{unchanged}} unverändert", + "diffRawComparison": "Kein gültiges JSON – so wie geschrieben neu eingerückt und Zeile für Zeile verglichen, ohne Einrückung zu berücksichtigen.", "partialTitle": "Einige Ressourcen wurden nicht importiert", "partialBody": "Der Rest des Agenten wurde geschrieben. Diese Ressourcen blieben unverändert und erfordern Aufmerksamkeit.", "partialCounts": "{{created}} erstellt, {{updated}} aktualisiert, {{failed}} fehlgeschlagen.", @@ -4180,6 +4181,9 @@ "diffFailed": "Die gespeicherte Version konnte nicht zum Vergleich geladen werden — das vollständige vorgeschlagene Dokument steht unten.", "diffForbidden": "Der Vergleich mit der gespeicherten Version erfordert Bearbeitungsrechte — das vollständige vorgeschlagene Dokument steht unten.", "diffRedactionNote": "Zugangsdaten sind in der vorgeschlagenen Version geschwärzt und erscheinen hier daher als Änderungen, auch wenn sie unverändert sind.", + "proposedNotJson": "Das vorgeschlagene Dokument ist kein gültiges JSON, daher wird EDDI diesen Schreibvorgang ablehnen. Es konnte auch nicht auf Berechtigungserweiterungen geprüft werden – lehnen Sie ab und fordern Sie eine korrigierte Anfrage an.", + "proposedNotJsonRedacted": "Das vorgeschlagene Dokument ist kein gültiges JSON, auch unter Berücksichtigung der in dieser Vorschau geschwärzten Zugangsdaten, daher wird EDDI diesen Schreibvorgang sehr wahrscheinlich ablehnen. Es konnte auch nicht auf Berechtigungserweiterungen geprüft werden – lehnen Sie ab und fordern Sie eine korrigierte Anfrage an.", + "proposedTrailingText": "Das vorgeschlagene Dokument enthält nach seinem Ende weiteren Text. EDDI speichert möglicherweise nur das Dokument davor und verwirft den Rest stillschweigend; nur dieser Teil wurde auf Berechtigungserweiterungen geprüft – lehnen Sie ab und fordern Sie eine korrigierte Anfrage an.", "diffStored": "Gespeichert v{{version}}", "diffProposed": "Vorgeschlagen", "showFullRequest": "Vollständiges vorgeschlagenes Dokument anzeigen", diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 26e9ca18..d9683e02 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -3080,7 +3080,8 @@ "targetContent": "Target", "sourceContent": "Source", "diffHiddenLines": "Show {{lines}} unchanged lines", - "diffRawComparison": "Not valid JSON — compared as plain text, so a difference of formatting alone also shows up as a change.", + "diffSummary": "Lines: {{added}} added · {{removed}} removed · {{unchanged}} unchanged", + "diffRawComparison": "Not valid JSON — shown re-indented as written and compared line by line, ignoring indentation.", "partialTitle": "Some resources were not imported", "partialBody": "The rest of the agent was written. These resources were left as they were and need attention.", "partialCounts": "{{created}} created, {{updated}} updated, {{failed}} failed.", @@ -4180,6 +4181,9 @@ "diffFailed": "Couldn't load the stored version to compare against — the full proposed document is below.", "diffForbidden": "Comparing against the stored version needs editor access — the full proposed document is below.", "diffRedactionNote": "Credential values are redacted in the proposed version, so they appear as changes here even when unchanged.", + "proposedNotJson": "The proposed document is not valid JSON, so EDDI will reject this write. It could not be scanned for capability grants either — reject it and ask for a corrected request.", + "proposedNotJsonRedacted": "The proposed document is not valid JSON, even allowing for the credentials redacted in this preview, so EDDI will most likely reject this write. It could not be scanned for capability grants either — reject it and ask for a corrected request.", + "proposedTrailingText": "The proposed document has extra text after its end. EDDI may store only the document before it and silently drop the rest, and only that part was scanned for capability grants — reject it and ask for a corrected request.", "diffStored": "Stored v{{version}}", "diffProposed": "Proposed", "showFullRequest": "Show the full proposed document", diff --git a/src/i18n/locales/es.json b/src/i18n/locales/es.json index 5c29e3ce..1a031027 100644 --- a/src/i18n/locales/es.json +++ b/src/i18n/locales/es.json @@ -2890,7 +2890,8 @@ "targetContent": "Destino", "sourceContent": "Origen", "diffHiddenLines": "Mostrar {{lines}} líneas sin cambios", - "diffRawComparison": "JSON no válido: se compara como texto plano, así que una diferencia solo de formato también aparece como un cambio.", + "diffSummary": "Líneas: {{added}} añadidas · {{removed}} eliminadas · {{unchanged}} sin cambios", + "diffRawComparison": "JSON no válido: se muestra reindentado tal como está escrito y se compara línea por línea, sin tener en cuenta la sangría.", "partialTitle": "Algunos recursos no se importaron", "partialBody": "El resto del agente se escribió. Estos recursos quedaron como estaban y requieren atención.", "partialCounts": "{{created}} creados, {{updated}} actualizados, {{failed}} fallidos.", @@ -4217,6 +4218,9 @@ "diffFailed": "No se pudo cargar la versión almacenada para comparar: el documento propuesto completo está abajo.", "diffForbidden": "Comparar con la versión almacenada requiere acceso de editor: el documento propuesto completo está abajo.", "diffRedactionNote": "Las credenciales están ocultas en la versión propuesta, por lo que aparecen aquí como cambios aunque no hayan cambiado.", + "proposedNotJson": "El documento propuesto no es JSON válido, así que EDDI rechazará esta escritura. Tampoco se pudo analizar en busca de concesiones de capacidad: recházala y pide una solicitud corregida.", + "proposedNotJsonRedacted": "El documento propuesto no es JSON válido, incluso teniendo en cuenta las credenciales ocultas en esta vista previa, así que lo más probable es que EDDI rechace esta escritura. Tampoco se pudo analizar en busca de concesiones de capacidad: recházala y pide una solicitud corregida.", + "proposedTrailingText": "El documento propuesto tiene texto adicional después de su final. EDDI podría guardar solo el documento anterior y descartar el resto sin avisar, y solo esa parte se analizó en busca de concesiones de capacidad: recházala y pide una solicitud corregida.", "diffStored": "Almacenado v{{version}}", "diffProposed": "Propuesto", "showFullRequest": "Mostrar el documento propuesto completo", diff --git a/src/i18n/locales/fr.json b/src/i18n/locales/fr.json index 3b57010b..2c0b8e12 100644 --- a/src/i18n/locales/fr.json +++ b/src/i18n/locales/fr.json @@ -2890,7 +2890,8 @@ "targetContent": "Cible", "sourceContent": "Source", "diffHiddenLines": "Afficher {{lines}} lignes inchangées", - "diffRawComparison": "JSON non valide — comparaison en texte brut : une simple différence de mise en forme apparaît donc aussi comme une modification.", + "diffSummary": "Lignes : {{added}} ajoutées · {{removed}} supprimées · {{unchanged}} inchangées", + "diffRawComparison": "JSON non valide — affiché réindenté tel qu'écrit et comparé ligne par ligne, sans tenir compte de l'indentation.", "partialTitle": "Certaines ressources n’ont pas été importées", "partialBody": "Le reste de l’agent a été écrit. Ces ressources sont restées telles quelles et demandent votre attention.", "partialCounts": "{{created}} créées, {{updated}} mises à jour, {{failed}} en échec.", @@ -4217,6 +4218,9 @@ "diffFailed": "Impossible de charger la version enregistrée pour comparaison — le document proposé complet figure ci-dessous.", "diffForbidden": "La comparaison avec la version enregistrée nécessite un accès éditeur — le document proposé complet figure ci-dessous.", "diffRedactionNote": "Les identifiants sont masqués dans la version proposée : ils apparaissent donc ici comme des modifications même s'ils sont inchangés.", + "proposedNotJson": "Le document proposé n'est pas du JSON valide : EDDI rejettera donc cette écriture. Il n'a pas non plus pu être analysé à la recherche d'octrois de capacités — rejetez-la et demandez une requête corrigée.", + "proposedNotJsonRedacted": "Le document proposé n'est pas du JSON valide, même en tenant compte des identifiants masqués dans cet aperçu : EDDI rejettera donc très probablement cette écriture. Il n'a pas non plus pu être analysé à la recherche d'octrois de capacités — rejetez-la et demandez une requête corrigée.", + "proposedTrailingText": "Le document proposé contient du texte supplémentaire après sa fin. EDDI peut n'enregistrer que le document qui précède et ignorer le reste sans prévenir, et seule cette partie a été analysée à la recherche d'octrois de capacités — rejetez-la et demandez une requête corrigée.", "diffStored": "Enregistré v{{version}}", "diffProposed": "Proposé", "showFullRequest": "Afficher le document proposé complet", diff --git a/src/i18n/locales/hi.json b/src/i18n/locales/hi.json index 98404ffe..2f329039 100644 --- a/src/i18n/locales/hi.json +++ b/src/i18n/locales/hi.json @@ -2861,7 +2861,8 @@ "targetContent": "लक्ष्य", "sourceContent": "स्रोत", "diffHiddenLines": "{{lines}} अपरिवर्तित पंक्तियाँ दिखाएँ", - "diffRawComparison": "मान्य JSON नहीं — सादे पाठ के रूप में तुलना की गई, इसलिए केवल स्वरूपण का अंतर भी परिवर्तन के रूप में दिखता है।", + "diffSummary": "पंक्तियाँ: {{added}} जोड़ी गईं · {{removed}} हटाई गईं · {{unchanged}} अपरिवर्तित", + "diffRawComparison": "मान्य JSON नहीं — जैसा लिखा गया है वैसा ही पुनः इंडेंट करके दिखाया गया है और इंडेंटेशन को अनदेखा करते हुए पंक्ति-दर-पंक्ति तुलना की गई है।", "partialTitle": "कुछ संसाधन आयात नहीं हुए", "partialBody": "एजेंट का शेष भाग लिख दिया गया। ये संसाधन जैसे थे वैसे ही रह गए और उन पर ध्यान देना आवश्यक है।", "partialCounts": "{{created}} बनाए गए, {{updated}} अद्यतन किए गए, {{failed}} विफल।", @@ -4180,6 +4181,9 @@ "diffFailed": "तुलना के लिए संग्रहीत संस्करण लोड नहीं हो सका — पूरा प्रस्तावित दस्तावेज़ नीचे है।", "diffForbidden": "संग्रहीत संस्करण से तुलना के लिए संपादक पहुँच चाहिए — पूरा प्रस्तावित दस्तावेज़ नीचे है।", "diffRedactionNote": "प्रस्तावित संस्करण में क्रेडेंशियल मान छिपाए गए हैं, इसलिए अपरिवर्तित होने पर भी वे यहाँ बदलाव के रूप में दिखते हैं।", + "proposedNotJson": "प्रस्तावित दस्तावेज़ मान्य JSON नहीं है, इसलिए EDDI इस लेखन को अस्वीकार कर देगा। क्षमता-वृद्धि सेटिंग्स के लिए इसकी जाँच भी नहीं हो सकी — इसे अस्वीकार करें और सुधरा हुआ अनुरोध माँगें।", + "proposedNotJsonRedacted": "इस पूर्वावलोकन में छिपाए गए क्रेडेंशियल को ध्यान में रखने के बाद भी प्रस्तावित दस्तावेज़ मान्य JSON नहीं है, इसलिए EDDI इस लेखन को बहुत संभवतः अस्वीकार कर देगा। क्षमता-वृद्धि सेटिंग्स के लिए इसकी जाँच भी नहीं हो सकी — इसे अस्वीकार करें और सुधरा हुआ अनुरोध माँगें।", + "proposedTrailingText": "प्रस्तावित दस्तावेज़ के अंत के बाद अतिरिक्त पाठ है। EDDI केवल उससे पहले का दस्तावेज़ सहेजकर बाकी को चुपचाप छोड़ सकता है, और क्षमता-वृद्धि सेटिंग्स के लिए केवल उसी भाग की जाँच हुई है — इसे अस्वीकार करें और सुधरा हुआ अनुरोध माँगें।", "diffStored": "संग्रहीत v{{version}}", "diffProposed": "प्रस्तावित", "showFullRequest": "पूरा प्रस्तावित दस्तावेज़ दिखाएँ", diff --git a/src/i18n/locales/ja.json b/src/i18n/locales/ja.json index 2c647f80..ea2a1029 100644 --- a/src/i18n/locales/ja.json +++ b/src/i18n/locales/ja.json @@ -2861,7 +2861,8 @@ "targetContent": "ターゲット", "sourceContent": "ソース", "diffHiddenLines": "変更のない {{lines}} 行を表示", - "diffRawComparison": "有効な JSON ではないため、プレーンテキストとして比較します。書式だけの違いも変更として表示されます。", + "diffSummary": "行: 追加 {{added}} · 削除 {{removed}} · 変更なし {{unchanged}}", + "diffRawComparison": "有効な JSON ではないため、記述どおりにインデントし直して表示し、インデントを無視して行ごとに比較します。", "partialTitle": "一部のリソースがインポートされませんでした", "partialBody": "エージェントの残りの部分は書き込まれました。以下のリソースはそのまま残っており、対応が必要です。", "partialCounts": "{{created}} 件作成、{{updated}} 件更新、{{failed}} 件失敗。", @@ -4180,6 +4181,9 @@ "diffFailed": "比較用の保存済みバージョンを読み込めませんでした — 提案されたドキュメント全体は下に表示されます。", "diffForbidden": "保存済みバージョンとの比較には編集者権限が必要です — 提案されたドキュメント全体は下に表示されます。", "diffRedactionNote": "提案版では認証情報がマスクされているため、変更されていなくてもここでは変更として表示されます。", + "proposedNotJson": "提案されたドキュメントは有効な JSON ではないため、EDDI はこの書き込みを拒否します。権限付与の有無も確認できませんでした。却下して、修正したリクエストを求めてください。", + "proposedNotJsonRedacted": "このプレビューでマスクされた認証情報を考慮しても、提案されたドキュメントは有効な JSON ではないため、EDDI はこの書き込みをほぼ確実に拒否します。権限付与の有無も確認できませんでした。却下して、修正したリクエストを求めてください。", + "proposedTrailingText": "提案されたドキュメントの終わりの後に余分なテキストがあります。EDDI はその前のドキュメントだけを保存し、残りを通知なく破棄する可能性があります。権限付与の有無を確認したのもその部分だけです。却下して、修正したリクエストを求めてください。", "diffStored": "保存済み v{{version}}", "diffProposed": "提案", "showFullRequest": "提案されたドキュメント全体を表示", diff --git a/src/i18n/locales/ko.json b/src/i18n/locales/ko.json index 9602d988..85c319a7 100644 --- a/src/i18n/locales/ko.json +++ b/src/i18n/locales/ko.json @@ -2861,7 +2861,8 @@ "targetContent": "대상 (Target)", "sourceContent": "원본 (Source)", "diffHiddenLines": "변경되지 않은 {{lines}}줄 표시", - "diffRawComparison": "유효한 JSON이 아니므로 일반 텍스트로 비교합니다. 서식만 다른 경우에도 변경으로 표시됩니다.", + "diffSummary": "줄: 추가 {{added}} · 삭제 {{removed}} · 변경 없음 {{unchanged}}", + "diffRawComparison": "유효한 JSON이 아니므로 작성된 그대로 들여쓰기를 다시 적용해 표시하고, 들여쓰기는 무시한 채 줄 단위로 비교합니다.", "partialTitle": "일부 리소스를 가져오지 못했습니다", "partialBody": "에이전트의 나머지 부분은 기록되었습니다. 아래 리소스는 그대로 남아 있으며 조치가 필요합니다.", "partialCounts": "{{created}}개 생성, {{updated}}개 업데이트, {{failed}}개 실패.", @@ -4180,6 +4181,9 @@ "diffFailed": "비교할 저장된 버전을 불러오지 못했습니다 — 제안된 전체 문서는 아래에 있습니다.", "diffForbidden": "저장된 버전과 비교하려면 편집자 권한이 필요합니다 — 제안된 전체 문서는 아래에 있습니다.", "diffRedactionNote": "제안된 버전에서는 자격 증명 값이 가려져 있어, 변경되지 않았더라도 여기서는 변경으로 표시됩니다.", + "proposedNotJson": "제안된 문서가 유효한 JSON이 아니므로 EDDI가 이 쓰기를 거부합니다. 권한 부여 여부도 확인할 수 없었습니다 — 거부하고 수정된 요청을 요청하세요.", + "proposedNotJsonRedacted": "이 미리보기에서 가려진 자격 증명을 감안하더라도 제안된 문서가 유효한 JSON이 아니므로 EDDI가 이 쓰기를 거부할 가능성이 매우 높습니다. 권한 부여 여부도 확인할 수 없었습니다 — 거부하고 수정된 요청을 요청하세요.", + "proposedTrailingText": "제안된 문서의 끝 뒤에 추가 텍스트가 있습니다. EDDI는 그 앞의 문서만 저장하고 나머지는 알리지 않고 버릴 수 있으며, 권한 부여 여부도 그 부분만 확인했습니다 — 거부하고 수정된 요청을 요청하세요.", "diffStored": "저장됨 v{{version}}", "diffProposed": "제안됨", "showFullRequest": "제안된 전체 문서 보기", diff --git a/src/i18n/locales/pt.json b/src/i18n/locales/pt.json index 8bd598c6..ea593039 100644 --- a/src/i18n/locales/pt.json +++ b/src/i18n/locales/pt.json @@ -2890,7 +2890,8 @@ "targetContent": "Alvo", "sourceContent": "Origem", "diffHiddenLines": "Mostrar {{lines}} linhas inalteradas", - "diffRawComparison": "JSON inválido — comparado como texto simples, portanto uma diferença apenas de formatação também aparece como alteração.", + "diffSummary": "Linhas: {{added}} adicionadas · {{removed}} removidas · {{unchanged}} inalteradas", + "diffRawComparison": "JSON inválido — mostrado reindentado tal como foi escrito e comparado linha a linha, ignorando a indentação.", "partialTitle": "Alguns recursos não foram importados", "partialBody": "O resto do agente foi escrito. Estes recursos ficaram como estavam e precisam de atenção.", "partialCounts": "{{created}} criados, {{updated}} atualizados, {{failed}} falharam.", @@ -4217,6 +4218,9 @@ "diffFailed": "Não foi possível carregar a versão armazenada para comparação — o documento proposto completo está abaixo.", "diffForbidden": "Comparar com a versão armazenada requer acesso de editor — o documento proposto completo está abaixo.", "diffRedactionNote": "As credenciais estão ocultas na versão proposta, pelo que aparecem aqui como alterações mesmo quando não mudaram.", + "proposedNotJson": "O documento proposto não é JSON válido, por isso o EDDI rejeitará esta escrita. Também não foi possível analisá-lo quanto a concessões de capacidade — rejeite-a e peça um pedido corrigido.", + "proposedNotJsonRedacted": "O documento proposto não é JSON válido, mesmo tendo em conta as credenciais ocultas nesta pré-visualização, por isso o EDDI muito provavelmente rejeitará esta escrita. Também não foi possível analisá-lo quanto a concessões de capacidade — rejeite-a e peça um pedido corrigido.", + "proposedTrailingText": "O documento proposto tem texto adicional depois do seu fim. O EDDI pode guardar apenas o documento anterior e descartar o resto sem aviso, e só essa parte foi analisada quanto a concessões de capacidade — rejeite-a e peça um pedido corrigido.", "diffStored": "Armazenado v{{version}}", "diffProposed": "Proposto", "showFullRequest": "Mostrar o documento proposto completo", diff --git a/src/i18n/locales/th.json b/src/i18n/locales/th.json index 54131fa4..b85754e5 100644 --- a/src/i18n/locales/th.json +++ b/src/i18n/locales/th.json @@ -2861,7 +2861,8 @@ "targetContent": "เป้าหมาย", "sourceContent": "แหล่งที่มา", "diffHiddenLines": "แสดง {{lines}} บรรทัดที่ไม่เปลี่ยนแปลง", - "diffRawComparison": "ไม่ใช่ JSON ที่ถูกต้อง — เปรียบเทียบเป็นข้อความล้วน ความแตกต่างเพียงรูปแบบการจัดวางจึงแสดงเป็นการเปลี่ยนแปลงด้วย", + "diffSummary": "บรรทัด: เพิ่ม {{added}} · ลบ {{removed}} · ไม่เปลี่ยนแปลง {{unchanged}}", + "diffRawComparison": "ไม่ใช่ JSON ที่ถูกต้อง — แสดงโดยจัดย่อหน้าใหม่ตามที่เขียนไว้ และเปรียบเทียบทีละบรรทัดโดยไม่สนใจการย่อหน้า", "partialTitle": "บางทรัพยากรไม่ได้ถูกนำเข้า", "partialBody": "ส่วนที่เหลือของเอเจนต์ถูกเขียนแล้ว ทรัพยากรเหล่านี้ยังคงเดิมและต้องได้รับการแก้ไข", "partialCounts": "สร้าง {{created}} รายการ อัปเดต {{updated}} รายการ ล้มเหลว {{failed}} รายการ", @@ -4180,6 +4181,9 @@ "diffFailed": "ไม่สามารถโหลดเวอร์ชันที่จัดเก็บไว้เพื่อเปรียบเทียบได้ — เอกสารที่เสนอฉบับเต็มอยู่ด้านล่าง", "diffForbidden": "การเปรียบเทียบกับเวอร์ชันที่จัดเก็บไว้ต้องใช้สิทธิ์ผู้แก้ไข — เอกสารที่เสนอฉบับเต็มอยู่ด้านล่าง", "diffRedactionNote": "ค่าข้อมูลรับรองถูกปกปิดในเวอร์ชันที่เสนอ จึงแสดงเป็นการเปลี่ยนแปลงที่นี่แม้ว่าจะไม่ได้เปลี่ยนก็ตาม", + "proposedNotJson": "เอกสารที่เสนอไม่ใช่ JSON ที่ถูกต้อง EDDI จึงจะปฏิเสธการเขียนนี้ และยังไม่สามารถตรวจสอบการให้สิทธิ์เพิ่มเติมได้ — ให้ปฏิเสธและขอคำขอที่แก้ไขแล้ว", + "proposedNotJsonRedacted": "แม้จะคำนึงถึงข้อมูลรับรองที่ถูกปกปิดในตัวอย่างนี้แล้ว เอกสารที่เสนอก็ยังไม่ใช่ JSON ที่ถูกต้อง EDDI จึงน่าจะปฏิเสธการเขียนนี้ และยังไม่สามารถตรวจสอบการให้สิทธิ์เพิ่มเติมได้ — ให้ปฏิเสธและขอคำขอที่แก้ไขแล้ว", + "proposedTrailingText": "เอกสารที่เสนอมีข้อความเกินมาหลังจุดสิ้นสุด EDDI อาจบันทึกเฉพาะเอกสารก่อนหน้านั้นและทิ้งส่วนที่เหลือโดยไม่แจ้ง และมีเพียงส่วนนั้นที่ได้รับการตรวจสอบการให้สิทธิ์เพิ่มเติม — ให้ปฏิเสธและขอคำขอที่แก้ไขแล้ว", "diffStored": "ที่จัดเก็บไว้ v{{version}}", "diffProposed": "ที่เสนอ", "showFullRequest": "แสดงเอกสารที่เสนอฉบับเต็ม", diff --git a/src/i18n/locales/zh.json b/src/i18n/locales/zh.json index 360c01f6..73e0d207 100644 --- a/src/i18n/locales/zh.json +++ b/src/i18n/locales/zh.json @@ -2861,7 +2861,8 @@ "targetContent": "目标", "sourceContent": "源", "diffHiddenLines": "显示 {{lines}} 行未更改内容", - "diffRawComparison": "不是有效的 JSON——按纯文本比较，因此仅格式上的差异也会显示为更改。", + "diffSummary": "行：新增 {{added}} · 删除 {{removed}} · 未更改 {{unchanged}}", + "diffRawComparison": "不是有效的 JSON——按原样重新缩进显示，并逐行比较，忽略缩进。", "partialTitle": "部分资源未导入", "partialBody": "智能体的其余部分已写入。这些资源保持原样，需要处理。", "partialCounts": "已创建 {{created}} 个，已更新 {{updated}} 个，失败 {{failed}} 个。", @@ -4180,6 +4181,9 @@ "diffFailed": "无法加载用于比较的已存储版本——完整的拟议文档见下方。", "diffForbidden": "与已存储版本比较需要编辑者权限——完整的拟议文档见下方。", "diffRedactionNote": "拟议版本中的凭据值已被隐去，因此即使未更改也会在此显示为变更。", + "proposedNotJson": "拟议文档不是有效的 JSON，因此 EDDI 会拒绝此次写入。也无法扫描是否存在权限授予——请拒绝并要求提交更正后的请求。", + "proposedNotJsonRedacted": "即使考虑到此预览中被隐去的凭据，拟议文档仍不是有效的 JSON，因此 EDDI 很可能会拒绝此次写入。也无法扫描是否存在权限授予——请拒绝并要求提交更正后的请求。", + "proposedTrailingText": "拟议文档在结尾之后还有多余的文本。EDDI 可能只保存其之前的文档并悄悄丢弃其余部分，且只对该部分扫描了权限授予——请拒绝并要求提交更正后的请求。", "diffStored": "已存储 v{{version}}", "diffProposed": "提议", "showFullRequest": "显示完整的拟议文档", diff --git a/src/lib/__tests__/redacted-json.test.ts b/src/lib/__tests__/redacted-json.test.ts index ab61764f..350c2c35 100644 --- a/src/lib/__tests__/redacted-json.test.ts +++ b/src/lib/__tests__/redacted-json.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { parseRedactedJson } from "@/lib/redacted-json"; +import { parseLeadingJson, parseRedactedJson } from "@/lib/redacted-json"; /** * The inputs below are what EDDI's `SecretRedactionFilter` actually emits, not @@ -157,3 +157,43 @@ describe("parseRedactedJson", () => { expect(performance.now() - started).toBeLessThan(2_000); }); }); + +describe("parseLeadingJson", () => { + it("reads the value before a stray closing brace, and hands back what follows", () => { + expect(parseLeadingJson('{"a":1}}')).toEqual({ ok: true, value: { a: 1 }, trailing: "}" }); + }); + + it("stops at the first complete value even when the rest reads like more fields", () => { + // What a reader that stops there would store: `b` is silently gone. + expect(parseLeadingJson('{"a":1},"b":2}')).toEqual({ ok: true, value: { a: 1 }, trailing: ',"b":2}' }); + }); + + it("does not take a bracket inside a string for the end of the value", () => { + expect(parseLeadingJson('{"a":"}]","b":"\\"}"}}')).toEqual({ + ok: true, + value: { a: "}]", b: '"}' }, + trailing: "}", + }); + }); + + it("repairs a redacted credential inside the leading value", () => { + expect(parseLeadingJson('{"apiKey="}}')).toEqual({ + ok: true, + value: { apiKey: "" }, + trailing: "}", + }); + }); + + it("fails for a body broken before its first value closes — the reported shape", () => { + // `{"tasks":[{…}}]}`: the extra brace sits inside the array, so there is no + // complete leading value to fall back to. + expect(parseLeadingJson('{"tasks":[{"id":"a"}}]}').ok).toBe(false); + }); + + it("fails for text that does not open as an object or array, or never closes", () => { + expect(parseLeadingJson("name=x&y=1").ok).toBe(false); + expect(parseLeadingJson('"a"}').ok).toBe(false); + expect(parseLeadingJson('{"a":1').ok).toBe(false); + expect(parseLeadingJson(" ").ok).toBe(false); + }); +}); diff --git a/src/lib/__tests__/reindent-json.test.ts b/src/lib/__tests__/reindent-json.test.ts new file mode 100644 index 00000000..66da1550 --- /dev/null +++ b/src/lib/__tests__/reindent-json.test.ts @@ -0,0 +1,75 @@ +import { describe, it, expect } from "vitest"; +import { jsonKeyOrder, reindentJsonText } from "@/lib/reindent-json"; + +describe("reindentJsonText", () => { + it("prints valid JSON exactly as JSON.stringify does", () => { + const value = { + a: 1, + b: [true, null, { c: "text" }], + empty: {}, + none: [], + nested: { deeper: { deepest: -1.5 } }, + }; + expect(reindentJsonText(JSON.stringify(value))).toBe(JSON.stringify(value, null, 2)); + // Already pretty-printed input comes out the same, too. + expect(reindentJsonText(JSON.stringify(value, null, 4))).toBe(JSON.stringify(value, null, 2)); + }); + + it("leaves string contents alone — structural characters, whitespace and escapes included", () => { + const value = { note: 'a, {b}: [c] "quoted" \\ back', spaced: " two spaces " }; + expect(reindentJsonText(JSON.stringify(value))).toBe(JSON.stringify(value, null, 2)); + }); + + it("lays out a body with a closing brace too many — the reported case — without repairing it", () => { + // The operator's updateLlm body closed an object twice before the array. + // The stray brace must stay visible as its own line, not be dropped. + expect(reindentJsonText('{"tasks":[{"id":"a","n":1}}]}')).toBe( + ['{', ' "tasks": [', " {", ' "id": "a",', ' "n": 1', " }", " }", "]", "}"].join("\n"), + ); + }); + + it("keeps every non-whitespace character of a broken body, in order", () => { + const broken = '{"a": 1,, "b": [2, 3}, "c": "unterminated'; + const strip = (text: string) => text.replace(/\s/g, ""); + expect(strip(reindentJsonText(broken))).toBe(strip(broken)); + }); + + it("keeps two adjacent values apart instead of running them into one token", () => { + expect(reindentJsonText('{"a": true false, "b": "x" "y"}')).toBe('{\n "a": true false,\n "b": "x" "y"\n}'); + }); + + it("indents what follows a stray closer from the margin, not one level short of it", () => { + expect(reindentJsonText('{"a":1}}{"b":2}')).toBe('{\n "a": 1\n}\n}{\n "b": 2\n}'); + expect(reindentJsonText("}")).toBe("}"); + }); + + it("closes a trailing comma's line instead of leaving a blank row", () => { + expect(reindentJsonText('{"a":1,}')).toBe('{\n "a": 1,\n}'); + expect(reindentJsonText("[1,\n ]")).toBe("[\n 1,\n]"); + }); + + it("carries the redaction filter's mangled field through as written", () => { + expect(reindentJsonText('{"modelName":"x","apiKey="}')).toBe( + '{\n "modelName": "x",\n "apiKey="\n}', + ); + }); +}); + +describe("jsonKeyOrder", () => { + it("numbers keys in the order they are first written, nested ones included", () => { + // `"x:y"` is a value, and `b` is only counted where it first appears. + expect([...jsonKeyOrder('{"b":1,"a":{"c":"x:y","b":2}}').entries()]).toEqual([ + ["b", 0], + ["a", 1], + ["c", 2], + ]); + }); + + it("reads a body that does not parse, the redaction filter's mangled field included", () => { + expect([...jsonKeyOrder('{"modelName":"x","apiKey="}},"n" : 1').keys()]).toEqual([ + "modelName", + "apiKey", + "n", + ]); + }); +}); diff --git a/src/lib/operator/__tests__/escalation-flags.test.ts b/src/lib/operator/__tests__/escalation-flags.test.ts index cde29813..cc60e886 100644 --- a/src/lib/operator/__tests__/escalation-flags.test.ts +++ b/src/lib/operator/__tests__/escalation-flags.test.ts @@ -96,6 +96,15 @@ describe("detectEscalationFlags", () => { expect(detectEscalationFlags("name=x&value=y")).toEqual([]); }); + it("still scans a body with text trailing its document — a stray brace is not a way past the check", () => { + // A reader that stops at the end of the first value stores the grant and + // ignores the extra `}`; this scan used to skip the whole body instead. + const body = `${groupBody({ dynamicAgents: { enabled: true, allowCreation: true } })}}`; + expect(detectEscalationFlags(body)).toEqual([ + { id: "dynamicAgentCreation", path: "dynamicAgents.allowCreation" }, + ]); + }); + it("returns nothing for JSON that is not an object", () => { expect(detectEscalationFlags("[1,2,3]")).toEqual([]); expect(detectEscalationFlags('"a string"')).toEqual([]); diff --git a/src/lib/operator/escalation-flags.ts b/src/lib/operator/escalation-flags.ts index f51d0ea1..aaede77c 100644 --- a/src/lib/operator/escalation-flags.ts +++ b/src/lib/operator/escalation-flags.ts @@ -1,5 +1,5 @@ import { gateLooksInstalled } from "@/lib/api/operator"; -import { parseRedactedJson } from "@/lib/redacted-json"; +import { parseLeadingJson, parseRedactedJson } from "@/lib/redacted-json"; import type { Agent } from "@/lib/api/agents"; /** @@ -285,9 +285,14 @@ export function detectEscalationFlags(body: string | null | undefined): Escalati flags.push({ id: "inlineCredential", path: credential }); } + // A body with text after its first complete value can still be WRITTEN — a + // reader that stops at the end of that value never sees the rest — so the part + // that may be stored is scanned rather than none of it. One stray closing + // brace must not be a way past every check below. const result = parseRedactedJson(body); - if (!result.ok) return flags; - const parsed = result.value; + const readable = result.ok ? result : parseLeadingJson(body); + if (!readable.ok) return flags; + const parsed = readable.value; if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) return flags; return [ diff --git a/src/lib/redacted-json.ts b/src/lib/redacted-json.ts index cf604158..15c795af 100644 --- a/src/lib/redacted-json.ts +++ b/src/lib/redacted-json.ts @@ -106,6 +106,45 @@ export function parseRedactedJson(content: string): ParseResult { return { ok: false }; } +export type LeadingParseResult = { ok: true; value: unknown; trailing: string } | { ok: false }; + +/** + * The first complete JSON value at the start of a body, and the text after it. + * + * A JSON reader that stops at the end of the first value — Jackson's default — + * never looks at the rest, so `{"a":1}}` or `{"a":1},"b":2}` can be accepted + * as `{"a":1}` with whatever followed silently dropped. Treating such a body as + * simply "not JSON" would hide exactly the part that may be written. + * + * Meant for a body that already failed {@link parseRedactedJson}: one that + * parses has nothing trailing it. The leading value goes through the same + * redaction repair, and must itself parse — a body broken before its first + * value closes (the stray brace INSIDE a document) is still a failure. + */ +export function parseLeadingJson(content: string): LeadingParseResult { + const start = content.search(/\S/); + if (start < 0 || (content[start] !== "{" && content[start] !== "[")) return { ok: false }; + + let depth = 0; + for (let i = start; i < content.length; i++) { + const c = content[i]; + if (c === '"') { + // Skip the string literal; the loop's own i++ steps past its closing quote. + i++; + while (i < content.length && content[i] !== '"') i += content[i] === "\\" ? 2 : 1; + } else if (c === "{" || c === "[") { + depth++; + } else if (c === "}" || c === "]") { + depth--; + if (depth === 0) { + const head = parseRedactedJson(content.slice(0, i + 1)); + return head.ok ? { ok: true, value: head.value, trailing: content.slice(i + 1) } : { ok: false }; + } + } + } + return { ok: false }; +} + /** * Per-field choices to try, uniform ones first. * diff --git a/src/lib/reindent-json.ts b/src/lib/reindent-json.ts new file mode 100644 index 00000000..8191331d --- /dev/null +++ b/src/lib/reindent-json.ts @@ -0,0 +1,132 @@ +/** + * Re-indent JSON-shaped text without requiring it to be valid JSON. + * + * The diff viewer pretty-prints both sides so that only content differences + * show. A side that does not parse used to be compared as it came — and a + * request body arrives compact, on one line — so a single stray brace turned + * the whole comparison into "every stored line deleted, one line added": the + * approver could not see what changed, nor where the document was broken. + * + * This only ever changes whitespace OUTSIDE string literals; every other + * character is copied through in order. Nothing is repaired or dropped — an + * approver must see the document as written, and a closing bracket too many + * shows up as exactly that: an extra line at the outer indentation. On valid + * JSON the layout matches `JSON.stringify(value, null, 2)` — but literals stay + * as written, so `1.50` or `"é"` are not normalised the way a re-printed + * parse would be. + */ +export function reindentJsonText(text: string, indent = " "): string { + let out = ""; + let depth = 0; + // Whitespace between two values (`true false`, `"a" "b"`) is not layout, it + // is the only thing keeping them apart — dropping it would print one token + // that is in neither document. Such a run collapses to a single space. + let skippedWhitespace = false; + let afterValue = false; + const newline = () => "\n" + indent.repeat(depth); + const value = (token: string) => { + out += (skippedWhitespace && afterValue ? " " : "") + token; + skippedWhitespace = false; + afterValue = true; + }; + const punctuation = (printed: string) => { + out += printed; + skippedWhitespace = false; + afterValue = false; + }; + + let i = 0; + while (i < text.length) { + const c = text[i]!; + + if (c === '"') { + // Copied verbatim up to the unescaped closing quote — or to the end, for + // a string that never closes. + let end = i + 1; + while (end < text.length && text[end] !== '"') end += text[end] === "\\" ? 2 : 1; + value(text.slice(i, end + 1)); + i = end + 1; + continue; + } + + if (/\s/.test(c)) { + skippedWhitespace = true; + i++; + continue; + } + + if (c === "{" || c === "[") { + // An empty container stays on one line, as JSON.stringify prints it. + let next = i + 1; + while (next < text.length && /\s/.test(text[next]!)) next++; + if (text[next] === (c === "{" ? "}" : "]")) { + value(c + text[next]); + i = next + 1; + continue; + } + depth++; + punctuation(c + newline()); + } else if (c === "}" || c === "]") { + // Clamped, not just clamped when printed: a stray closer must not leave a + // debt that the next opener silently pays off, printing whatever follows + // it one level too far left. + depth = Math.max(0, depth - 1); + // A trailing comma (`{"a":1,}`) has already opened an empty indented + // line; the closer takes that line instead of leaving it blank. + out = out.replace(/\n[^\S\n]*$/, ""); + punctuation((out === "" ? "" : newline()) + c); + } else if (c === ",") { + punctuation("," + newline()); + } else if (c === ":") { + punctuation(": "); + } else { + value(c); + } + i++; + } + return out; +} + +/** + * Every object key in JSON-shaped text, numbered in the order each first + * appears — the text need not parse. + * + * For lining a parsed document up against one that could not be parsed: the + * parsed side is then printed in the order the broken side was written in, so a + * key the writer merely put somewhere else does not read as removed and added. + * Keys are matched by name alone, whatever object they sit in; that is enough to + * order by, which is all this is for. + * + * The redaction filter's mangled field (`"apiKey="`, see + * `redacted-json.ts`) counts as the key it was. + */ +export function jsonKeyOrder(text: string): Map { + const order = new Map(); + let i = 0; + while (i < text.length) { + if (text[i] !== '"') { + i++; + continue; + } + let end = i + 1; + while (end < text.length && text[end] !== '"') end += text[end] === "\\" ? 2 : 1; + const literal = text.slice(i, end + 1); + + let next = end + 1; + while (next < text.length && /\s/.test(text[next]!)) next++; + const name = text[next] === ":" ? decodeStringLiteral(literal) : /^"([^"\\]*?)=/.exec(literal)?.[1]; + if (name !== undefined && !order.has(name)) order.set(name, order.size); + + i = end + 1; + } + return order; +} + +function decodeStringLiteral(literal: string): string | undefined { + try { + const value: unknown = JSON.parse(literal); + return typeof value === "string" ? value : undefined; + } catch { + return undefined; + } +} From 7826bbf0b993789fd02fa6fb161ff0e9e157f2ea Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 15 Sep 2026 01:42:21 +0200 Subject: [PATCH 2/2] fix(approvals): keep text after a closed container apart from its bracket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reindentJsonText treated a closing } or ] as punctuation, so trailing text after a complete document ({"a":1} true) printed glued onto the bracket — the very case the approval's trailing-text warning is about. A closed container now counts as a finished value. --- src/lib/__tests__/reindent-json.test.ts | 9 +++++++++ src/lib/reindent-json.ts | 3 +++ 2 files changed, 12 insertions(+) diff --git a/src/lib/__tests__/reindent-json.test.ts b/src/lib/__tests__/reindent-json.test.ts index 66da1550..090da7eb 100644 --- a/src/lib/__tests__/reindent-json.test.ts +++ b/src/lib/__tests__/reindent-json.test.ts @@ -38,6 +38,15 @@ describe("reindentJsonText", () => { expect(reindentJsonText('{"a": true false, "b": "x" "y"}')).toBe('{\n "a": true false,\n "b": "x" "y"\n}'); }); + it("keeps text after a closed container apart from its bracket", () => { + // Trailing text after a complete document is exactly what the approval + // warns about — it must not be printed glued onto the closing brace. + expect(reindentJsonText('{"a":1} true')).toBe('{\n "a": 1\n} true'); + expect(reindentJsonText('[1] "x"')).toBe('[\n 1\n] "x"'); + // No separator was written, so none is invented. + expect(reindentJsonText('{"a":1}true')).toBe('{\n "a": 1\n}true'); + }); + it("indents what follows a stray closer from the margin, not one level short of it", () => { expect(reindentJsonText('{"a":1}}{"b":2}')).toBe('{\n "a": 1\n}\n}{\n "b": 2\n}'); expect(reindentJsonText("}")).toBe("}"); diff --git a/src/lib/reindent-json.ts b/src/lib/reindent-json.ts index 8191331d..fcf6c685 100644 --- a/src/lib/reindent-json.ts +++ b/src/lib/reindent-json.ts @@ -75,6 +75,9 @@ export function reindentJsonText(text: string, indent = " "): string { // line; the closer takes that line instead of leaving it blank. out = out.replace(/\n[^\S\n]*$/, ""); punctuation((out === "" ? "" : newline()) + c); + // A closed container is a finished value: text after it (`{…} true`) + // must stay apart from it, not run into the bracket. + afterValue = true; } else if (c === ",") { punctuation("," + newline()); } else if (c === ":") {