From 1a8683ed468815ce715221330168030b191b1e14 Mon Sep 17 00:00:00 2001 From: John Siracusa Date: Fri, 7 Aug 2026 10:04:48 -0400 Subject: [PATCH 1/2] fix(engine): let a user remove the bad layer quarantine only showed them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quarantine made a hand-edited layer visible without making it fixable. Every mutation route reads through mutateContextManifest, which reads STRICTLY, so with an invalid layer present POST/PATCH/DELETE /api/sources and PATCH /api/settings all threw before doing anything — including the one removal that would have made the manifest valid again. The app could show you the problem and nothing else. repairContextManifest is the door that opens: the single mutation allowed to READ a manifest holding an invalid layer, because it is the one that takes one out. Nothing about what may be PERSISTED moves. The write is still writeContextManifest, so the result must pass validateContextManifest in full; a repair that does not leave the manifest valid is refused and the file on disk is untouched. Both tolerant readers now share one private reader, so the door can never accept a manifest the read path would reject — a duplicate layer name, a second live layer, a broken profiles block all stay fatal. And a repair may only remove: no layers array may come out of the callback longer than it went in. The callback gets the RAW manifest, not the quarantined one. Handing it the cleaned manifest would delete every OTHER broken layer from the user's file as a side effect of removing the one they asked about. For the same reason quarantine records now carry `index` and removeSourceApi removes by index rather than by name: a broken layer that also reuses a healthy layer's name is shown as "seed (2)", and a name filter would have taken the healthy `seed` with it. ?name= repeats, and that is not a convenience. Only a valid manifest may be written, so with two invalid entries, removing either alone is refused — the other still fails validation. Removing them in one transaction is the only shape that both repairs the file and keeps the write strict; measured against three bad layers, the single-name form is a dead end from which the app can never recover. A request that asks for too little answers 409 naming what blocked it, never a 500. Console: rows carry `quarantined`, distinct from a source that failed to READ (createErrorSource now takes the flag explicitly — the two were indistinguishable from status alone, and only one of them can be acted on). On an invalid row Rename and Sync are gone, since both could only fail; the panel says the entry is not a working source, and Remove names every other invalid entry before the click rather than quietly sweeping rows nobody selected. PATCH /api/settings stays a strict write — quietly rewriting a manifest read around a bad layer is how a hand-edited layer gets dropped without being asked about — but it answered 500 with a layer validation error on the Settings screen, which tells the user nothing about where to go. It now answers 409 pointing at Sources, where the repair lives. Co-Authored-By: Claude Opus 5 Signed-off-by: John Siracusa --- CLAUDE.md | 2 + apps/console/src/api.ts | 1 + apps/console/src/data.ts | 6 + apps/console/src/types.ts | 8 ++ apps/console/src/views/Sources.test.tsx | 66 +++++++++ apps/console/src/views/Sources.tsx | 89 ++++++++++--- .../content/docs/docs/reference/manifest.md | 8 ++ packages/core/src/manifest.mjs | 98 +++++++++++++- packages/core/src/service.mjs | 125 ++++++++++++++---- packages/core/src/sources/index.mjs | 10 +- packages/core/tests/manifest.test.mjs | 91 +++++++++++++ packages/core/tests/service-test.sh | 60 ++++++++- 12 files changed, 508 insertions(+), 56 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 52299e1..9032f04 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -131,6 +131,8 @@ Key files: - Capture tools (`log_capture`/`confirm_capture`) exist only behind `mcp-server.mjs --capture`. The default server exposes 6 read-only tools (the original `search`/`read_file`/`list_concepts`/`get_links` stay byte-identical to the committed `fixtures/mcp-tools-baseline.json`, plus always-on read-only `find_captures`/`whats_new`); `--capture` adds the two write tools for 8. Telemetry (`--telemetry`) records concept ids and enums only — never content. Top-level tool `description` strings are NOT frozen by the baseline (only `name`/`inputSchema`/`annotations` are) — `search`/`read_file`/`get_links` descriptions teach conflict candor (sections may carry `conflicts[]`; weigh `fresherDissent`; `search` hits may carry `contested`/`conflictSections`). - **The Mac app adds a `github-rest` source kind** (`POST /api/sources`) alongside the existing `github` clone kind — public repos read over the REST adapter with no clone (`repo`/`ref`/`paths`, default `cache:{ttlSeconds:900}`); private repos still use the clone kind ("uses your existing git credentials or SSH"). The app deliberately does not accept `auth`/`apiBase` from its own UI for `github-rest` in this release — the keychain `auth` alias resolution is real in the engine (`buildSources(manifest, dir, {tokens})`) but no caller in the app injects a `tokens` map yet, so an alias would silently read anonymously; headless users can still hand-write `{"auth":{"tokenEnv":"NAME"}}`. - **Manifest reads are profile-view-unified.** `service.mjs` builds one `{...manifest, layers: getManifestProfileLayers(manifest)}` view in `openSources()` and threads it through every read site (index keys, `buildSources`, the manifest watcher, `layerMeta`, sync lookups) plus `layer-files.mjs`'s `layerRootMap` — a manifest migrated to v2 (e.g. by `contextcake profile create`) no longer empties the app's source list. +- **A bad layer is read around, never written around.** `readContextManifestQuarantined` (the read path, via `readManifestForRead()` in `service.mjs`) lifts a layer that fails validation out of the manifest it returns, so one hand-edited layer no longer 500s every route — it becomes an error row with `quarantined: true` in `/api/graph`. Writes stay strict: `mutateContextManifest` reads through the strict reader, and every write goes through `writeContextManifest`, which validates the whole manifest. The single exception is `repairContextManifest`, used only by `removeSourceApi` — it tolerates an invalid layer on the way IN (same rule as the read path), hands the callback the RAW manifest so a removal drops exactly the entry asked for, refuses any callback that lengthens a layers array, and still validates in full before writing. Do not make `mutateContextManifest` tolerant to make some other route work; the repair door is the one that removes, which is why it is allowed to see the mess. +- **Removing invalid entries is all-or-nothing.** Because only a valid manifest may be persisted, removing one of two invalid layers is refused — the other still fails validation. `DELETE /api/sources?name=` therefore repeats (`searchParams.getAll`), and the console's Remove on an invalid row names every other invalid row before it sends them together. A single removal that leaves invalid layers behind answers 409 listing what blocked it, never a 500. - All git mutations against a live root go through `git-core.mjs` (advisory `.contextcake.lock`, per-repo serialization) — never call git directly against a live layer from engine code. - The engine (`packages/core/src/`) is dependency-free — plain Node.js built-ins only. Do not add npm dependencies without discussion. The exceptions are `apps/console/`, `apps/site/`, and `apps/desktop/` — self-contained npm packages. Console and site never import from the engine; the desktop app imports engine modules by path (one-way: app → engine, never the reverse) and must never cause a dependency to leak into `packages/core`. - `apps/console/` and `apps/site/` each have their own `package.json`, build, and tests; run their commands from that subdirectory, not the repo root. Web Demo previews are path-filtered to `apps/console/**`; production deploys with the matching Mac app from the single `app-v*` release workflow. diff --git a/apps/console/src/api.ts b/apps/console/src/api.ts index a01d2a7..28c6a88 100644 --- a/apps/console/src/api.ts +++ b/apps/console/src/api.ts @@ -528,6 +528,7 @@ export function adaptSources(g: GraphSummary): Source[] { conceptCount: s.conceptCount, origin: s.origin ?? null, error: s.error ?? null, + ...(s.quarantined === true ? { quarantined: true } : {}), // The true count, which the capped message list is not: a source with 40 // unreadable files sends 40 here and 10 messages. warnings: s.warnings ?? (s.warningMessages?.length ?? 0), diff --git a/apps/console/src/data.ts b/apps/console/src/data.ts index 070bec8..684a7e2 100644 --- a/apps/console/src/data.ts +++ b/apps/console/src/data.ts @@ -32,6 +32,12 @@ export interface Source { /** Git remote a clone-backed layer came from; enables Sync alongside kind 'github'. */ origin?: string | null error?: string | null + /** + * Not a source at all: a manifest entry the engine could not validate, so + * nothing was built for it. Rename and Sync have nothing to act on; removing + * the entry is the repair. + */ + quarantined?: boolean /** Progress while the background index is reading this source. */ indexing?: SourceProgress /** Content this source indexed around: too big to read, or not readable. */ diff --git a/apps/console/src/types.ts b/apps/console/src/types.ts index 5a57b36..473a8ff 100644 --- a/apps/console/src/types.ts +++ b/apps/console/src/types.ts @@ -78,6 +78,8 @@ export interface SourceStatus { /** Ready and re-reading behind a good snapshot. Distinct from `status: indexing`. */ refreshing: boolean error: string | null + /** An invalid manifest entry rather than a source — same meaning as GraphSource's. */ + quarantined?: boolean } /** @@ -115,6 +117,12 @@ export interface GraphSource { */ status: string error: string | null + /** + * This row is a manifest entry that failed validation, not a source that + * failed to read: nothing was built for it, so there is nothing to retry, + * rename or sync. Removing the entry is the only action that helps. + */ + quarantined?: boolean /** * Things this source could not read even though it indexed successfully — a * document over the per-file size cap, a subfolder it lacks permission to diff --git a/apps/console/src/views/Sources.test.tsx b/apps/console/src/views/Sources.test.tsx index 5149af2..ba4ea88 100644 --- a/apps/console/src/views/Sources.test.tsx +++ b/apps/console/src/views/Sources.test.tsx @@ -163,6 +163,72 @@ describe('Sources remove', () => { }) }) +describe('Sources with an invalid manifest entry', () => { + const broken = (over: Partial = {}) => src({ + name: 'bad-kind', status: 'error', quarantined: true, conceptCount: 0, coverage: 0, + error: 'Layer bad-kind has unsupported source kind: notarealkind', ...over, + }) + + it('offers only Remove, and says the entry is not a working source', async () => { + await mount([broken()]) + + expect(container.textContent).toContain('This entry is not a working source') + // Rename writes through the strict manifest path and Sync has nothing to + // talk to, so offering either would only produce an error. + expect(container.querySelector('button[aria-label^="Rename"]')).toBeNull() + expect(container.querySelector('button[aria-label^="Sync"]')).toBeNull() + expect(container.textContent).not.toContain('The path, repository, or command is fixed') + expect(container.textContent).toContain('unsupported source kind: notarealkind') + buttonByAria('Remove bad-kind') // throws if absent + }) + + it('removes a lone invalid entry on its own', async () => { + await mount([src({}), broken()]) + + await act(async () => sourceButton('bad-kind').click()) + await act(async () => buttonByAria('Remove bad-kind').click()) + expect(container.textContent).toContain('nothing was being read from this entry') + await act(async () => button('Remove entry').click()) + + expect(mocks.apiFetch).toHaveBeenCalledWith('/api/sources?name=bad-kind', expect.objectContaining({ method: 'DELETE' })) + }) + + it('names every other invalid entry and removes them in one request', async () => { + // The engine only persists a manifest that validates, so removing one of + // three would be refused. The panel has to say that before the click + // rather than removing rows the user never selected. + await mount([src({}), broken(), broken({ name: 'layer 4', error: 'Layer in legacy default must have a non-empty name.' })]) + + await act(async () => sourceButton('bad-kind').click()) + await act(async () => buttonByAria('Remove bad-kind').click()) + expect(container.textContent).toContain('One other entry is also invalid') + expect(container.textContent).toContain('bad-kind, layer 4') + + await act(async () => button('Remove all 2 invalid entries').click()) + expect(mocks.apiFetch).toHaveBeenCalledWith('/api/sources?name=bad-kind&name=layer%204', expect.objectContaining({ method: 'DELETE' })) + expect(mocks.reload).toHaveBeenCalled() + }) + + it('renders the engine refusal verbatim when the manifest cannot be repaired', async () => { + mocks.apiFetch.mockImplementation(async (_url: string, init?: RequestInit) => { + if (init?.method === 'DELETE') { + return new Response( + JSON.stringify({ error: 'Nothing was removed: the manifest is invalid in a way this app cannot repair. Edit /kb/manifest.json by hand — legacy default contains duplicate layer name: seed' }), + { status: 409, headers: { 'content-type': 'application/json' } }, + ) + } + return ok() + }) + await mount([broken()]) + + await act(async () => buttonByAria('Remove bad-kind').click()) + await act(async () => button('Remove entry').click()) + + expect(container.textContent).toContain('Edit /kb/manifest.json by hand') + expect(mocks.reload).not.toHaveBeenCalled() + }) +}) + describe('Sources rename + re-level', () => { it('PATCHes only name and level — and says a wrong path means remove + re-add', async () => { await mount([src({ name: 'notes', level: 3 })]) diff --git a/apps/console/src/views/Sources.tsx b/apps/console/src/views/Sources.tsx index 353be71..6cec049 100644 --- a/apps/console/src/views/Sources.tsx +++ b/apps/console/src/views/Sources.tsx @@ -91,7 +91,7 @@ function fmtTime(iso: string): string { /** Sync applies to REST github layers and to clone-backed layers (origin set). */ function canSync(s: Source): boolean { - return s.sourceKind === 'github' || Boolean(s.origin) + return !s.quarantined && (s.sourceKind === 'github' || Boolean(s.origin)) } function btnSmallGhost(): React.CSSProperties { @@ -176,6 +176,10 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { ].some((value) => String(value ?? '').toLowerCase().includes(normalizedQuery))) .sort((a, b) => b.level - a.level || a.name.localeCompare(b.name)) + // Every invalid manifest entry, off the unfiltered list: what a repair has to + // clear is a property of the manifest, not of what the search box is showing. + const invalid = sources.filter((source) => source.quarantined) + useEffect(() => { if (selectedName && sources.some((source) => source.name === selectedName)) return setSelectedName(ordered[0]?.name ?? null) @@ -230,11 +234,17 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { } } + // Removing an invalid entry removes every invalid entry, and the panel says + // so before the click. The engine will only persist a manifest that + // validates, so with two bad entries present, removing either alone is + // refused — one request naming all of them is the only thing that repairs + // the file. `name` repeats; the engine reads them all. const confirmRemove = async (s: Source) => { + const names = s.quarantined ? invalid.map((source) => source.name) : [s.name] setBusy(true) setErr(null) try { - await callApi(`/api/sources?name=${encodeURIComponent(s.name)}`, { method: 'DELETE' }) + await callApi(`/api/sources?${names.map((n) => `name=${encodeURIComponent(n)}`).join('&')}`, { method: 'DELETE' }) closePanel() reload() } catch (e) { @@ -313,10 +323,22 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) {
Last success
{s.lastSuccessAt ? fmtTime(s.lastSuccessAt) : 'Not yet'}
Last error
{s.lastErrorAt ? fmtTime(s.lastErrorAt) : 'None'}
-
Sync
{canSync(s) ? 'Available' : 'Not supported for this source kind'}
+ {/* "Not supported for this source kind" would be answering the + wrong question on an entry that is not a source at all. */} + {!s.quarantined &&
Sync
{canSync(s) ? 'Available' : 'Not supported for this source kind'}
} {s.origin &&
Repository
{s.origin}
}
-

The path, repository, or command is fixed for this source. To change it, remove the source and add it again.

+ {/* An invalid entry has no path, repo or command to speak of — the + sentence below is about a working source, and printing it here + would describe a source that was never built. */} + {!s.quarantined &&

The path, repository, or command is fixed for this source. To change it, remove the source and add it again.

} + {s.quarantined && ( +
+ This entry is not a working source + ContextCake could not read it as a valid source, so nothing was built for it and nothing from it reaches the cascade. + Renaming and syncing have nothing to act on — removing the entry is the fix, and your files are never touched. +
+ )} {s.error && (
{s.error} @@ -368,8 +390,11 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { onClick={() => void syncNow(s)} >{syncing === s.name ? 'Syncing…' : 'Sync now'} )} - - + {/* Rename/level writes through the strict manifest path, so on + an invalid entry it could only fail. Remove is the one + action that goes anywhere from here. */} + {!s.quarantined && } +
)} @@ -402,21 +427,45 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { )} - {removing && ( -
-

- Remove {s.name} from the cascade? Your files stay where they are — only the cascade entry is removed. -

- {s.live && } - {err &&

{err}

} -
- - + {removing && (() => { + // An invalid entry can only be removed alongside every other + // invalid entry: the engine writes a manifest only when the whole + // thing validates, so leaving one behind refuses the write. The + // panel names them before the click rather than removing rows the + // user did not choose. + const sweep = s.quarantined ? invalid : [] + const others = sweep.filter((source) => source.name !== s.name) + return ( +
+

+ {s.quarantined + ? <>Remove the invalid entry {s.name} from your manifest? Your files stay where they are — nothing was being read from this entry. + : <>Remove {s.name} from the cascade? Your files stay where they are — only the cascade entry is removed.} +

+ {others.length > 0 && ( +
+ + {others.length === 1 ? 'One other entry is also invalid' : `${others.length} other entries are also invalid`} + + Your manifest can only be saved once every invalid entry is gone, so this removes {others.length === 1 ? 'both' : `all ${sweep.length}`}:{' '} + {sweep.map((source) => source.name).join(', ')} +
+ )} + {s.live && } + {err &&

{err}

} +
+ + +
-
- )} + ) + })()} })()}
diff --git a/apps/site/src/content/docs/docs/reference/manifest.md b/apps/site/src/content/docs/docs/reference/manifest.md index fe49e1b..aa59d1c 100644 --- a/apps/site/src/content/docs/docs/reference/manifest.md +++ b/apps/site/src/content/docs/docs/reference/manifest.md @@ -252,6 +252,14 @@ rather than broken: a duplicated layer name, or a second `live` layer, stops the read outright rather than guessing which one you meant. Writes always validate strictly, so an invalid layer can never be saved through a tolerated read. +You can remove a broken row from the app — the Remove action on that row edits +the manifest for you, and it is the only action offered there, since renaming or +syncing something that was never built could only fail. Because a manifest is +only ever saved once the whole file validates, removing one broken row while +another remains is refused: the app asks to remove them together, and names the +others before you confirm. Anything else — settings included — stays blocked +until the manifest is valid again, and says so. + GitHub may truncate very large recursive tree responses. ContextCake refuses to index that partial response as if it were complete; it serves the last complete cached index when available, or resolves without that layer until a complete tree diff --git a/packages/core/src/manifest.mjs b/packages/core/src/manifest.mjs index 68d489d..500158f 100644 --- a/packages/core/src/manifest.mjs +++ b/packages/core/src/manifest.mjs @@ -120,12 +120,29 @@ export function readContextManifest(manifestPath, { allowMissing = true, validat * * Returns { manifest, quarantined }, where each quarantined record carries the * `profileId` of the layers array it came from — "default" is always the array - * getManifestProfileLayers(manifest) itself would return. + * getManifestProfileLayers(manifest) itself would return — and the `index` it + * sat at in that array, which is the only handle a repair can remove it by. */ export function readContextManifestQuarantined(manifestPath, { allowMissing = true, validatePacks = true } = {}) { + const { cleaned, quarantined } = readTolerantManifest(manifestPath, { allowMissing, validatePacks }); + return { manifest: cleaned, quarantined }; +} + +// The one tolerance rule, written once. Both tolerant entry points read through +// this, so the door a repair comes in by can never accept a manifest the read +// path would have rejected. +// +// Returns { raw, cleaned, quarantined }: `cleaned` is the manifest with the +// invalid layers deleted (what may be BUILT), `raw` is the parsed file with +// them still in place (what may be REPAIRED). They are the same object when +// nothing was quarantined. +function readTolerantManifest(manifestPath, { allowMissing, validatePacks }) { const resolved = path.resolve(manifestPath); if (!fs.existsSync(resolved)) { - if (allowMissing) return { manifest: { layers: [] }, quarantined: [] }; + if (allowMissing) { + const empty = { layers: [] }; + return { raw: empty, cleaned: empty, quarantined: [] }; + } throw new Error(`ContextCake manifest does not exist: ${resolved}`); } let manifest; @@ -140,7 +157,7 @@ export function readContextManifestQuarantined(manifestPath, { allowMissing = tr let firstError; try { validateContextManifest(manifest, { validatePacks }); - return { manifest, quarantined: [] }; + return { raw: manifest, cleaned: manifest, quarantined: [] }; } catch (error) { firstError = error; } @@ -159,7 +176,75 @@ export function readContextManifestQuarantined(manifestPath, { allowMissing = tr // never per-layer (a broken profiles block, a dangling pack). Fail closed // exactly as before rather than serving a manifest nobody has validated. validateContextManifest(cleaned, { validatePacks }); - return { manifest: cleaned, quarantined }; + return { raw: manifest, cleaned, quarantined }; +} + +/** + * mutateContextManifest's repair door: the one mutation allowed to READ a + * manifest that still holds an invalid layer, because it is the mutation that + * takes one out. Without it, quarantine left a user able to see the broken + * layer and unable to do anything about it — every write route reads strictly + * first, so the removal that would make the manifest valid again was itself + * blocked by the manifest being invalid. + * + * What is tolerated is the way IN, and only as far as the read path already + * tolerates it (same readTolerantManifest rule: per-layer breakage only, a + * whole-manifest failure still throws). Two things keep the door narrow: + * + * 1. The manifest that gets WRITTEN goes through the unchanged + * writeContextManifest, so it must pass validateContextManifest in full. + * A repair that does not leave the manifest valid is refused and the file + * on disk is untouched. + * 2. A repair may only REMOVE. No layers array may come out of the callback + * longer than it went in, so this cannot become a back way to add or + * re-point a source from an unvalidated read. + * + * The callback receives the RAW manifest — invalid layers included — because + * removing exactly the layer that was asked for is the point: handing it the + * cleaned manifest would silently drop every OTHER broken layer from the user's + * file as a side effect of removing one. + * + * mutate({ manifest, layers, quarantined }) where `layers` is the live default + * layers array (the one getManifestProfileLayers would return) and each + * quarantined record's `index` addresses that array directly. + */ +export function repairContextManifest(manifestPath, mutate, { allowLegacy = true, allowTransitional = false } = {}) { + const resolved = path.resolve(manifestPath); + return withManifestLock(resolved, () => { + // allowMissing is deliberately not an option: there is nothing to repair in + // a manifest that does not exist. + const { raw, quarantined } = readTolerantManifest(resolved, { allowMissing: false, validatePacks: true }); + const before = layerCountsByContainer(raw); + const result = mutate({ manifest: raw, layers: defaultLayersInPlace(raw), quarantined }); + for (const [container, count] of layerCountsByContainer(raw)) { + if (count > (before.get(container) ?? 0)) throw new Error("A manifest repair may only remove layers."); + } + writeContextManifest(resolved, raw, { allowLegacy, allowTransitional }); + return result; + }); +} + +// The default container's layers array, in place and without validating — the +// array a repair removes from. Mirrors getManifestProfileLayers(manifest) for +// profile=null, including its habit of creating the array when it is absent so +// the caller mutates the manifest rather than a copy. +function defaultLayersInPlace(manifest) { + if (classifyManifest(manifest) === "v2") { + assertObject(manifest.profiles.default, "Profile default"); + manifest.profiles.default.layers ??= []; + return manifest.profiles.default.layers; + } + manifest.layers ??= []; + return manifest.layers; +} + +function layerCountsByContainer(manifest) { + const counts = new Map(); + if (Array.isArray(manifest.layers)) counts.set("", manifest.layers.length); + for (const [id, profile] of Object.entries(manifest.profiles ?? {})) { + if (Array.isArray(profile?.layers)) counts.set(id, profile.layers.length); + } + return counts; } // Splits every layers array in the manifest into the layers that validate and @@ -230,6 +315,11 @@ function partitionLayers(layers, owner, transitional) { taken.add(name); return { name, + // Where this layer sits in its own layers array. A repair removes by + // index, never by name: the name above may have been synthesized, and + // a broken layer that also reuses a valid layer's name would otherwise + // take that healthy layer down with it. + index, level: Number.isInteger(Number(layer?.level)) ? Number(layer.level) : 0, // The declared kind, as a scalar. The layer OBJECT deliberately never // leaves this module — that is what makes it structurally impossible to diff --git a/packages/core/src/service.mjs b/packages/core/src/service.mjs index 68243a1..001baf8 100644 --- a/packages/core/src/service.mjs +++ b/packages/core/src/service.mjs @@ -46,6 +46,7 @@ import { mutateContextManifest, readContextManifest, readContextManifestQuarantined, + repairContextManifest, stableJson, withManifestLockAsync, } from "./manifest.mjs"; @@ -448,7 +449,7 @@ export function createEngineService({ // the source list purely so the index gives each one an error row. sources: [ ...buildSourcesQuarantined(manifest, MANIFEST_DIR, { tokens: tokenState.tokens }), - ...broken.map((entry) => createErrorSource(entry)), + ...broken.map((entry) => createErrorSource({ ...entry, quarantined: true })), ], keys, }; @@ -961,7 +962,7 @@ export function createEngineService({ if (p === "/api/sources" && (req.method === "POST" || req.method === "DELETE" || req.method === "PATCH")) { if (!allowMutations) { json(res, 405, { error: "Mutations are disabled on this service" }); return true; } if (req.method === "POST") { json(res, 200, await addSourceApi(await readBody(req))); return true; } - if (req.method === "DELETE") { json(res, 200, removeSourceApi(url.searchParams.get("name"))); return true; } + if (req.method === "DELETE") { json(res, 200, removeSourceApi(url.searchParams.getAll("name"))); return true; } json(res, 200, patchSourceApi(await readBody(req))); return true; } @@ -1058,6 +1059,11 @@ export function createEngineService({ // contributed nothing. status: degraded ? "degraded" : status, error: degraded ? health.lastError : error ?? null, + // This row is a manifest entry that failed validation, not a source + // that failed to read: there is nothing behind it to retry, and the + // only action that helps is removing it. Says so out loud because the + // two are indistinguishable from `status: "error"` alone. + quarantined: s.quarantined === true, // Orthogonal to status, on purpose: this source read successfully and // is serving what it read — it just isn't serving everything it was // pointed at. The count drives a badge; the messages are capped so one @@ -1243,6 +1249,7 @@ export function createEngineService({ name: source.name, level: source.level, kind: source.quarantinedKind ?? layerMeta.get(source.name)?.source ?? "okf-local", + quarantined: source.quarantined === true, // same meaning as /api/graph's status: degraded ? "degraded" : status, phase: entry.phase, loaded: entry.loaded, @@ -1406,15 +1413,26 @@ export function createEngineService({ } catch (err) { throw httpError(400, err.message); } - mutateContextManifest(MANIFEST, (manifest) => { - const next = { ...(manifest.settings ?? {}) }; - for (const key of Object.keys(body.settings ?? body)) { - if (clean[key] === undefined) delete next[key]; // null = reset to default - else next[key] = clean[key]; - } - if (Object.keys(next).length === 0) delete manifest.settings; - else manifest.settings = next; - }, { allowMissing: false, allowTransitional: true }); + try { + mutateContextManifest(MANIFEST, (manifest) => { + const next = { ...(manifest.settings ?? {}) }; + for (const key of Object.keys(body.settings ?? body)) { + if (clean[key] === undefined) delete next[key]; // null = reset to default + else next[key] = clean[key]; + } + if (Object.keys(next).length === 0) delete manifest.settings; + else manifest.settings = next; + }, { allowMissing: false, allowTransitional: true }); + } catch (err) { + // Settings deliberately stay a STRICT write — an invalid layer is not + // this route's to tolerate, and quietly rewriting a manifest read around + // one is how a hand-edited layer would get dropped without being asked + // about. But answering 500 with a layer's validation error, on the + // Settings screen, tells the user nothing about where to go. Removing + // the bad source is the repair, and it has a screen of its own. + if (err.status) throw err; + throw httpError(409, `Settings were not saved: a source in your manifest is invalid, and saving would rewrite the file around it. Remove it in Sources first — ${err.message}`); + } // New limits change how sources are read, so their indexes are stale: the // settings are part of the index key, so reload() rebuilds them. reload(); @@ -1606,27 +1624,76 @@ export function createEngineService({ } } - function removeSourceApi(name) { - if (!name) throw httpError(400, "Provide ?name="); - let removed = null; + /** + * The one repair route. It reads through repairContextManifest rather than + * mutateContextManifest, so a quarantined layer — the row /api/graph shows as + * an error — can be taken out from the app. Everything about what may be + * WRITTEN is unchanged: repairContextManifest validates the whole manifest + * before the file is touched. + * + * `?name=` may repeat, and that is not a convenience. What may be persisted + * is a VALID manifest, so with two invalid entries present, removing either + * one on its own is refused — the remaining one still fails validation. A + * manifest with several bad layers would be unrepairable from the app, which + * is the situation this whole path exists to end. Removing them in one + * transaction is the only shape that both fixes the file and keeps the write + * strict. The 409 below says so when a client asked for too little. + */ + function removeSourceApi(names) { + const wanted = [...new Set(names.filter((name) => typeof name === "string" && name))]; + if (wanted.length === 0) throw httpError(400, "Provide ?name="); + const removed = []; let survivors = []; - mutateContextManifest(MANIFEST, (manifest) => { - const layers = getManifestProfileLayers(manifest); - const before = layers.length; - const container = defaultProfileContainer(manifest); - const pendingBefore = container.pendingSources?.length ?? 0; - removed = layers.find((layer) => layer.name === name) ?? null; - const retained = layers.filter((layer) => layer.name !== name); - layers.splice(0, layers.length, ...retained); - removePendingSource(container, name); - if (layers.length === before && (container.pendingSources?.length ?? 0) === pendingBefore) { - throw httpError(404, `No source named "${name}"`); + let blocking = []; + try { + repairContextManifest(MANIFEST, ({ manifest, layers, quarantined }) => { + const container = defaultProfileContainer(manifest); + // Quarantined rows for the profile this service reads. A layer + // quarantined out of some OTHER profile has no row here to have been + // clicked, and removing it is not this route's business. + const broken = quarantined.filter((entry) => entry.profileId === "default"); + // A set, because these become splices: two names resolving to one index + // would take a second, innocent layer with them. + const doomed = new Set(); + for (const name of wanted) { + const pendingBefore = container.pendingSources?.length ?? 0; + removePendingSource(container, name); + const droppedPending = (container.pendingSources?.length ?? 0) !== pendingBefore; + const index = layers.findIndex((layer) => layer.name === name); + if (index >= 0) { doomed.add(index); continue; } + // A quarantined row is matched on the name the graph gave it, which + // may be synthesized, and removed at the index that name was minted + // for — see the record's `index`. Valid layers win the name first, so + // this can never shadow a healthy row. + const entry = broken.find((candidate) => candidate.name === name); + if (entry) { doomed.add(entry.index); continue; } + if (!droppedPending) throw httpError(404, `No source named "${name}"`); + } + // Descending, so each splice leaves the indices below it alone. + for (const index of [...doomed].sort((a, b) => b - a)) { + removed.push(layers[index]); + layers.splice(index, 1); + } + // What the write is about to reject on, if it rejects: every invalid + // entry the caller did NOT ask to remove. + blocking = broken.filter((entry) => !doomed.has(entry.index)); + survivors = allManifestLayers(manifest); // every profile — a shared clone must survive + }, { allowTransitional: true }); + } catch (err) { + if (err.status) throw err; + if (blocking.length > 0) { + const listed = blocking.map((entry) => `"${entry.name}" (${entry.error})`).join("; "); + throw httpError(409, `Nothing was removed: ${blocking.length} other source${blocking.length === 1 ? " is" : "s are"} also invalid, and the manifest cannot be saved while ${blocking.length === 1 ? "it remains" : "they remain"}. Remove ${blocking.length === 1 ? "it" : "them"} in the same request — ${listed}`); } - survivors = allManifestLayers(manifest); // every profile — a shared clone must survive - }, { allowMissing: false, allowTransitional: true }); - cleanupCloneDir(removed, survivors); + // A manifest broken in a way no single layer explains (two layers sharing + // a name, a malformed profiles block) is not repairable from here, and a + // 500 would read as "the app is broken" rather than "your file is". Say + // which, and keep the engine's own message — it names the actual defect. + throw httpError(409, `Nothing was removed: the manifest is invalid in a way this app cannot repair. Edit ${MANIFEST} by hand — ${err.message}`); + } + for (const layer of removed) cleanupCloneDir(layer, survivors); reload(); - return { ok: true, removed: name }; + return { ok: true, removed: wanted[0], removedNames: wanted }; } // Every layer the manifest still declares, across the legacy array and every diff --git a/packages/core/src/sources/index.mjs b/packages/core/src/sources/index.mjs index 5ae61b6..2cb129e 100644 --- a/packages/core/src/sources/index.mjs +++ b/packages/core/src/sources/index.mjs @@ -56,8 +56,14 @@ export function buildSourcesQuarantined(manifest, manifestDir, { tokens = {}, pr * background index is what reads it — that throw is how the layer reaches the * user as an error row — while loadConcept answers null so one broken layer * cannot fail a resolve the healthy layers can still answer. + * + * `quarantined` separates the two cases, because the app can only act on one of + * them: a layer lifted out by the manifest reader is a config defect the user + * fixes by removing the entry, while a layer that validated and then failed to + * construct is a real source having a bad day — renaming, syncing and removing + * all still work on it as usual. */ -export function createErrorSource({ name, level, kind = null, error }) { +export function createErrorSource({ name, level, kind = null, error, quarantined = false }) { return { name: typeof name === "string" && name.trim() ? name : "unnamed source", level: Number.isFinite(Number(level)) ? Number(level) : 0, @@ -65,7 +71,7 @@ export function createErrorSource({ name, level, kind = null, error }) { // entry in. Null for a layer that validated and then failed to construct — // there the manifest still knows what it was. quarantinedKind: kind, - quarantined: true, + quarantined, async loadConcept() { return null; }, async listConceptIds() { throw new Error(error); }, close() {}, diff --git a/packages/core/tests/manifest.test.mjs b/packages/core/tests/manifest.test.mjs index 644d244..1defdb4 100644 --- a/packages/core/tests/manifest.test.mjs +++ b/packages/core/tests/manifest.test.mjs @@ -18,6 +18,7 @@ import { normalizeProfileLabel, readContextManifest, readContextManifestQuarantined, + repairContextManifest, selectManifestProfile, sourceConfigFingerprint, validateContextManifest, @@ -237,6 +238,96 @@ test("quarantined reads drop only the malformed layer, and only when one layer e assert.deepEqual(getManifestProfileLayers(v2.manifest), [good]); }); +test("a repair can remove a quarantined layer, and can persist only a manifest that validates", (t) => { + const directory = temporaryDirectory(t); + const write = (manifest) => { + const file = path.join(directory, `${crypto.randomUUID()}.json`); + fs.writeFileSync(file, JSON.stringify(manifest)); + return file; + }; + const read = (file) => JSON.parse(fs.readFileSync(file, "utf8")); + const good = { name: "seed", level: 1, source: "files", path: "seed" }; + const badKind = { name: "bad-kind", level: 2, source: "notarealkind" }; + // Removes the layer the record points at. Mirrors what removeSourceApi does. + const removeByRow = (file, ...names) => repairContextManifest(file, ({ layers, quarantined }) => { + const doomed = names.map((name) => { + const index = layers.findIndex((layer) => layer.name === name); + if (index >= 0) return index; + const entry = quarantined.find((candidate) => candidate.profileId === "default" && candidate.name === name); + assert.ok(entry, `no row named ${name}`); + return entry.index; + }); + for (const index of [...doomed].sort((a, b) => b - a)) layers.splice(index, 1); + }); + + // The point of the whole exercise: the strict readers cannot get here at all. + const single = write({ layers: [good, badKind] }); + assert.throws(() => mutateContextManifest(single, () => {}, { allowMissing: false }), /unsupported source kind/); + removeByRow(single, "bad-kind"); + assert.deepEqual(read(single), { layers: [good] }); + + // The callback sees the RAW manifest, invalid layers and all. Handing it the + // cleaned one would delete every other broken layer as a side effect of + // removing this one — data loss the user never asked for. + const many = write({ layers: [good, badKind, { name: "bad-shape", level: 0 }] }); + assert.throws(() => repairContextManifest(many, ({ manifest, quarantined }) => { + assert.equal(manifest.layers.length, 3, "the repair reads the file as written"); + assert.deepEqual(quarantined.map((entry) => [entry.name, entry.index]), [["bad-kind", 1], ["bad-shape", 2]]); + throw new Error("abort"); + }), /abort/); + assert.equal(read(many).layers.length, 3, "a thrown repair writes nothing"); + + // Removing one of two invalid layers leaves a manifest that does not + // validate, so the write is refused and the file is untouched. + assert.throws(() => removeByRow(many, "bad-kind"), /must have a non-empty name|requires a path|integer level/); + assert.equal(read(many).layers.length, 3, "a refused repair leaves the file exactly as it was"); + removeByRow(many, "bad-kind", "bad-shape"); + assert.deepEqual(read(many), { layers: [good] }); + + // A broken layer that also reuses a healthy layer's name is quarantined under + // a distinct row name and removed by INDEX, so the healthy layer survives. + const shadowed = write({ layers: [good, { name: "seed", level: 9, source: "notarealkind" }] }); + assert.deepEqual( + readContextManifestQuarantined(shadowed).quarantined.map((entry) => [entry.name, entry.index]), + [["seed (2)", 1]], + ); + removeByRow(shadowed, "seed (2)"); + assert.deepEqual(read(shadowed), { layers: [good] }); + + // The repair door only removes. Anything else and the write never happens. + const guarded = write({ layers: [good, badKind] }); + assert.throws( + () => repairContextManifest(guarded, ({ layers }) => { layers.push({ name: "sneak", level: 5, source: "files", path: "x" }); }), + /may only remove layers/, + ); + assert.throws( + () => repairContextManifest(guarded, ({ manifest, layers }) => { layers.splice(1, 1); manifest.profiles = { default: profile("Default", [good]) }; }), + /may only remove layers/, + ); + // Nor is it a way to write a layer that does not validate. + assert.throws( + () => repairContextManifest(guarded, ({ layers }) => { layers.splice(1, 1, { name: "worse", level: 1, source: "alsonotreal" }); }), + /unsupported source kind: alsonotreal/, + ); + assert.equal(read(guarded).layers.length, 2, "none of the refused repairs touched the file"); + + // Whole-manifest breakage is no more repairable here than it is readable. + assert.throws(() => repairContextManifest(write({ layers: [good, { ...good, level: 2 }] }), () => {}), /duplicate layer name/); + assert.throws(() => repairContextManifest(path.join(directory, "absent.json"), () => {}), /does not exist/); + + // v2 repairs the profile the service reads, and leaves the others alone. + const v2 = write({ + profiles: { + default: profile("Default", [good, { name: "bad", level: 1, source: "mcp" }]), + other: profile("Other", [good]), + }, + projects: {}, + }); + removeByRow(v2, "bad"); + assert.deepEqual(read(v2).profiles.default.layers, [good]); + assert.deepEqual(read(v2).profiles.other.layers, [good]); +}); + test("inactive Pack drift warns while selecting the affected profile fails closed", () => { const manifest = { profiles: { diff --git a/packages/core/tests/service-test.sh b/packages/core/tests/service-test.sh index 21129db..a126628 100755 --- a/packages/core/tests/service-test.sh +++ b/packages/core/tests/service-test.sh @@ -13,10 +13,13 @@ ROOT="$(cd "$(dirname "$0")/../../.." && pwd)" TMP="$(mktemp -d)" PORT4=$((PORT + 3)) # graph-cache host: freshness of the memoized /api/graph BASE4="http://127.0.0.1:$PORT4" +PORT5=$((PORT + 4)) # quarantine-repair host: a manifest with invalid layers +BASE5="http://127.0.0.1:$PORT5" PID1="" PID2="" PID3="" PID4="" +PID5="" FAILED=0 cleanup() { @@ -24,6 +27,7 @@ cleanup() { [ -n "$PID2" ] && kill "$PID2" 2>/dev/null [ -n "$PID3" ] && kill "$PID3" 2>/dev/null [ -n "$PID4" ] && kill "$PID4" 2>/dev/null + [ -n "$PID5" ] && kill "$PID5" 2>/dev/null rm -rf "$TMP" } trap cleanup EXIT @@ -502,6 +506,60 @@ code 200 "$(C -X PATCH "${AUTH[@]}" -H 'content-type: application/json' -d '{"ma for _ in $(seq 1 60); do [ "$(G4 | JQ 'String(d.sources.find((s) => s.name === "d").status)')" = "ok" ] && break; sleep 0.25; done [ "$(GRAPHTOK4)" = "$(EXPECT4)" ] && pass "counts match a fresh encode once the limit is restored" || fail "stale counts after settings were restored" +echo "repairing a manifest from the app: an invalid layer can be removed" +# Quarantine made a bad layer visible. This is the other half: it has to be +# fixable from the same app that shows it. The write stays strict — what these +# assertions pin is that the way IN tolerates a bad layer while the way OUT +# still refuses to persist one. +mkdir -p "$TMP/seedq" +printf '# Seed\n\n## Body\n\nquarantine fixture.\n' > "$TMP/seedq/seed.md" +cat > "$TMP/manifest-bad.json" </dev/null 2>&1 & +PID5=$! +for _ in $(seq 1 60); do [ "$(C "${AUTH[@]}" "$BASE5/api/graph")" != "000" ] && break; sleep 0.1; done +BADG="$(curl -s "${AUTH[@]}" "$BASE5/api/graph?wait=15000")" +[ "$(JQ 'JSON.stringify(d.sources.map((s) => [s.name, s.status, s.quarantined === true]))' <<<"$BADG")" \ + = '[["seed","ok",false],["bad-kind","error",true],["seed (2)","error",true],["layer 4","error",true]]' ] \ + && pass "invalid entries are rows of their own, flagged quarantined, and never shadow the healthy layer" \ + || fail "quarantined rows wrong ($(JQ 'JSON.stringify(d.sources.map((s) => [s.name, s.status, s.quarantined]))' <<<"$BADG"))" + +# One of three cannot be removed: the remaining two still fail validation, and +# a manifest that does not validate is never written. The refusal has to name +# what is blocking it, or the user is stuck with no way forward. +ONE="$(curl -s -o "$TMP/one.json" -w '%{http_code}' -X DELETE "${AUTH[@]}" "$BASE5/api/sources?name=bad-kind")" +code 409 "$ONE" "removing one of three invalid entries is refused" +grep -q '2 other sources are also invalid' "$TMP/one.json" && pass "the refusal names how many others block it" || fail "refusal message unhelpful ($(cat "$TMP/one.json"))" +grep -q 'notarealkind' "$TMP/manifest-bad.json" && pass "the refused removal left the manifest untouched" || fail "a refused removal edited the manifest" + +# Settings are a strict write and stay one — but the answer has to point at the +# repair rather than dropping a layer validation error on the Settings screen. +SET="$(curl -s -o "$TMP/set.json" -w '%{http_code}' -X PATCH "${AUTH[@]}" -H 'content-type: application/json' -d '{"maxDocFiles":222}' "$BASE5/api/settings")" +code 409 "$SET" "settings cannot be saved while a source is invalid" +grep -q 'Remove it in Sources first' "$TMP/set.json" && pass "the settings refusal points at the screen that fixes it" || fail "settings refusal unhelpful ($(cat "$TMP/set.json"))" + +# All three together is a repair, so it lands. +code 200 "$(C -X DELETE "${AUTH[@]}" "$BASE5/api/sources?name=bad-kind&name=seed%20(2)&name=layer%204")" "removing every invalid entry at once succeeds" +code 200 "$(C -X PATCH "${AUTH[@]}" -H 'content-type: application/json' -d '{"maxDocFiles":222}' "$BASE5/api/settings")" "settings save once the manifest is valid" +node -e ' + const m = JSON.parse(require("node:fs").readFileSync(process.argv[1], "utf8")); + const names = m.layers.map((l) => l.name); + process.stdout.write(JSON.stringify([names, m.layers.length, m.settings?.maxDocFiles])); +' "$TMP/manifest-bad.json" > "$TMP/after.json" +[ "$(cat "$TMP/after.json")" = '[["seed"],1,222]' ] \ + && pass "only the invalid entries were removed — the healthy same-named layer survived" \ + || fail "manifest after repair is wrong ($(cat "$TMP/after.json"))" +[ "$(curl -s "${AUTH[@]}" "$BASE5/api/graph?wait=15000" | JQ 'String(d.sources.find((s) => s.name === "seed")?.conceptCount)')" = "1" ] \ + && pass "the surviving layer still resolves after the repair" || fail "the repair damaged the healthy layer" +code 404 "$(C -X DELETE "${AUTH[@]}" "$BASE5/api/sources?name=ghost")" "a name that is neither a layer nor a quarantined row is still 404" +code 400 "$(C -X DELETE "${AUTH[@]}" "$BASE5/api/sources")" "DELETE with no name is still 400" + echo "injected credentials: reported, never echoed" # The engine receives secrets by value from whoever owns the keychain. Two # things have to hold at once: the credential must actually reach the adapter, @@ -585,4 +643,4 @@ grep -q '"boundAlias":"github.com/octo"' <<<"$CRED" && pass "the alias (a name, grep -q '"boundIndexed":true' <<<"$CRED" && pass "the credentialed layer actually indexed" || fail "credentialed layer did not index ($CRED)" grep -q '"unboundState":"missing-token"' <<<"$CRED" && pass "setTokens re-indexes and reports the now-missing credential" || fail "setTokens did not invalidate ($CRED)" -[ "$FAILED" = 0 ] && echo "service test passed (bearer gate + allowMutations + fall-through + CRUD reload + console mount + github-rest + v2 reads + mcp health + clone cleanup + section guard + credential injection + graph-cache freshness + /api/status)" || { echo "service test FAILED"; exit 1; } +[ "$FAILED" = 0 ] && echo "service test passed (bearer gate + allowMutations + fall-through + CRUD reload + console mount + github-rest + v2 reads + mcp health + clone cleanup + section guard + credential injection + graph-cache freshness + /api/status + quarantine repair)" || { echo "service test FAILED"; exit 1; } From 3d58308f5333cd799abcc5493f4dc40d413fbe13 Mon Sep 17 00:00:00 2001 From: John Siracusa Date: Fri, 7 Aug 2026 10:16:37 -0400 Subject: [PATCH 2/2] fix(console): an invalid entry blocks removing a healthy source too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the parent commit found the repair half-built. The write rewrites the whole manifest and only a valid one is ever saved, so an invalid entry refuses the removal of a perfectly healthy source exactly as it refuses the removal of another invalid one. The engine already answered that 409 with the remedy — name them in the same request — but the console only offered the sweep on an invalid row, so a user with one bad layer could not remove anything at all and was left staring at a message about a row that has nothing wrong with it. Every removal now carries the invalid entries with it, named in the confirm panel before the click. Same informed-consent shape as before, one row wider. The service test gains the mixed case directly: a healthy source refused on its own, then coming out together with the invalid entry beside it. Co-Authored-By: Claude Opus 5 Signed-off-by: John Siracusa --- CLAUDE.md | 2 +- apps/console/src/views/Sources.test.tsx | 33 +++++++++++++--- apps/console/src/views/Sources.tsx | 38 +++++++++++-------- .../content/docs/docs/reference/manifest.md | 10 +++-- packages/core/tests/service-test.sh | 16 ++++++++ 5 files changed, 73 insertions(+), 26 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9032f04..4b738aa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -132,7 +132,7 @@ Key files: - **The Mac app adds a `github-rest` source kind** (`POST /api/sources`) alongside the existing `github` clone kind — public repos read over the REST adapter with no clone (`repo`/`ref`/`paths`, default `cache:{ttlSeconds:900}`); private repos still use the clone kind ("uses your existing git credentials or SSH"). The app deliberately does not accept `auth`/`apiBase` from its own UI for `github-rest` in this release — the keychain `auth` alias resolution is real in the engine (`buildSources(manifest, dir, {tokens})`) but no caller in the app injects a `tokens` map yet, so an alias would silently read anonymously; headless users can still hand-write `{"auth":{"tokenEnv":"NAME"}}`. - **Manifest reads are profile-view-unified.** `service.mjs` builds one `{...manifest, layers: getManifestProfileLayers(manifest)}` view in `openSources()` and threads it through every read site (index keys, `buildSources`, the manifest watcher, `layerMeta`, sync lookups) plus `layer-files.mjs`'s `layerRootMap` — a manifest migrated to v2 (e.g. by `contextcake profile create`) no longer empties the app's source list. - **A bad layer is read around, never written around.** `readContextManifestQuarantined` (the read path, via `readManifestForRead()` in `service.mjs`) lifts a layer that fails validation out of the manifest it returns, so one hand-edited layer no longer 500s every route — it becomes an error row with `quarantined: true` in `/api/graph`. Writes stay strict: `mutateContextManifest` reads through the strict reader, and every write goes through `writeContextManifest`, which validates the whole manifest. The single exception is `repairContextManifest`, used only by `removeSourceApi` — it tolerates an invalid layer on the way IN (same rule as the read path), hands the callback the RAW manifest so a removal drops exactly the entry asked for, refuses any callback that lengthens a layers array, and still validates in full before writing. Do not make `mutateContextManifest` tolerant to make some other route work; the repair door is the one that removes, which is why it is allowed to see the mess. -- **Removing invalid entries is all-or-nothing.** Because only a valid manifest may be persisted, removing one of two invalid layers is refused — the other still fails validation. `DELETE /api/sources?name=` therefore repeats (`searchParams.getAll`), and the console's Remove on an invalid row names every other invalid row before it sends them together. A single removal that leaves invalid layers behind answers 409 listing what blocked it, never a 500. +- **Removing invalid entries is all-or-nothing.** Because only a valid manifest may be persisted, any removal that leaves an invalid layer behind is refused — including one that only meant to drop a healthy source, since the write rewrites the whole file. `DELETE /api/sources?name=` therefore repeats (`searchParams.getAll`), and the console's Remove on ANY row names every invalid row before it sends them together. A removal that would leave invalid layers behind answers 409 listing what blocked it, never a 500. - All git mutations against a live root go through `git-core.mjs` (advisory `.contextcake.lock`, per-repo serialization) — never call git directly against a live layer from engine code. - The engine (`packages/core/src/`) is dependency-free — plain Node.js built-ins only. Do not add npm dependencies without discussion. The exceptions are `apps/console/`, `apps/site/`, and `apps/desktop/` — self-contained npm packages. Console and site never import from the engine; the desktop app imports engine modules by path (one-way: app → engine, never the reverse) and must never cause a dependency to leak into `packages/core`. - `apps/console/` and `apps/site/` each have their own `package.json`, build, and tests; run their commands from that subdirectory, not the repo root. Web Demo previews are path-filtered to `apps/console/**`; production deploys with the matching Mac app from the single `app-v*` release workflow. diff --git a/apps/console/src/views/Sources.test.tsx b/apps/console/src/views/Sources.test.tsx index ba4ea88..de531f7 100644 --- a/apps/console/src/views/Sources.test.tsx +++ b/apps/console/src/views/Sources.test.tsx @@ -194,21 +194,44 @@ describe('Sources with an invalid manifest entry', () => { }) it('names every other invalid entry and removes them in one request', async () => { - // The engine only persists a manifest that validates, so removing one of - // three would be refused. The panel has to say that before the click - // rather than removing rows the user never selected. + // Two invalid entries: the engine only persists a manifest that validates, + // so removing either alone would be refused. The panel has to say that + // before the click rather than removing rows the user never selected. await mount([src({}), broken(), broken({ name: 'layer 4', error: 'Layer in legacy default must have a non-empty name.' })]) await act(async () => sourceButton('bad-kind').click()) await act(async () => buttonByAria('Remove bad-kind').click()) expect(container.textContent).toContain('One other entry is also invalid') - expect(container.textContent).toContain('bad-kind, layer 4') + expect(container.textContent).toContain('layer 4') - await act(async () => button('Remove all 2 invalid entries').click()) + await act(async () => button('Remove 2 entries').click()) expect(mocks.apiFetch).toHaveBeenCalledWith('/api/sources?name=bad-kind&name=layer%204', expect.objectContaining({ method: 'DELETE' })) expect(mocks.reload).toHaveBeenCalled() }) + it('carries the invalid entries along when a healthy source is removed', async () => { + // The write rewrites the whole manifest, so an invalid entry blocks + // removing a working source too. Refusing with an explanation the user + // cannot act on would leave them stuck on a row that has nothing wrong. + await mount([src({ name: 'notes' }), broken()]) + + await act(async () => sourceButton('notes').click()) + await act(async () => buttonByAria('Remove notes').click()) + expect(container.textContent).toContain('One other entry is also invalid') + expect(container.textContent).toContain('only the cascade entry is removed') + + await act(async () => button('Remove 2 entries').click()) + expect(mocks.apiFetch).toHaveBeenCalledWith('/api/sources?name=notes&name=bad-kind', expect.objectContaining({ method: 'DELETE' })) + }) + + it('leaves an ordinary removal alone when nothing is invalid', async () => { + await mount([src({ name: 'notes' })]) + + await act(async () => buttonByAria('Remove notes').click()) + await act(async () => button('Remove source').click()) + expect(mocks.apiFetch).toHaveBeenCalledWith('/api/sources?name=notes', expect.objectContaining({ method: 'DELETE' })) + }) + it('renders the engine refusal verbatim when the manifest cannot be repaired', async () => { mocks.apiFetch.mockImplementation(async (_url: string, init?: RequestInit) => { if (init?.method === 'DELETE') { diff --git a/apps/console/src/views/Sources.tsx b/apps/console/src/views/Sources.tsx index 6cec049..a5ac84b 100644 --- a/apps/console/src/views/Sources.tsx +++ b/apps/console/src/views/Sources.tsx @@ -179,6 +179,11 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { // Every invalid manifest entry, off the unfiltered list: what a repair has to // clear is a property of the manifest, not of what the search box is showing. const invalid = sources.filter((source) => source.quarantined) + // The invalid entries a removal of THIS row has to take with it. Any write + // rewrites the whole manifest and the engine only saves one that validates, + // so an invalid entry blocks removing a perfectly healthy source just as + // surely as it blocks removing another invalid one. + const alsoInvalid = (s: Source) => invalid.filter((source) => source.name !== s.name) useEffect(() => { if (selectedName && sources.some((source) => source.name === selectedName)) return @@ -234,13 +239,13 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { } } - // Removing an invalid entry removes every invalid entry, and the panel says - // so before the click. The engine will only persist a manifest that - // validates, so with two bad entries present, removing either alone is - // refused — one request naming all of them is the only thing that repairs - // the file. `name` repeats; the engine reads them all. + // A removal carries the invalid entries with it, and the panel names them + // before the click. The engine will only persist a manifest that validates, + // so while anything is invalid, removing this row alone is refused — one + // request naming all of them is the only thing that repairs the file. + // `name` repeats; the engine reads them all. const confirmRemove = async (s: Source) => { - const names = s.quarantined ? invalid.map((source) => source.name) : [s.name] + const names = [s.name, ...alsoInvalid(s).map((source) => source.name)] setBusy(true) setErr(null) try { @@ -428,13 +433,13 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { )} {removing && (() => { - // An invalid entry can only be removed alongside every other - // invalid entry: the engine writes a manifest only when the whole - // thing validates, so leaving one behind refuses the write. The - // panel names them before the click rather than removing rows the - // user did not choose. - const sweep = s.quarantined ? invalid : [] - const others = sweep.filter((source) => source.name !== s.name) + // Any removal takes the invalid entries with it: the engine + // writes a manifest only when the whole thing validates, so + // leaving one behind refuses the write — including a write that + // was only meant to remove a healthy source. The panel names them + // before the click rather than removing rows the user did not + // choose. + const others = alsoInvalid(s) return (

@@ -447,8 +452,9 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { {others.length === 1 ? 'One other entry is also invalid' : `${others.length} other entries are also invalid`} - Your manifest can only be saved once every invalid entry is gone, so this removes {others.length === 1 ? 'both' : `all ${sweep.length}`}:{' '} - {sweep.map((source) => source.name).join(', ')} + Your manifest can only be saved once every invalid entry is gone, so this also removes{' '} + {others.length === 1 ? 'it' : 'them'}:{' '} + {others.map((source) => source.name).join(', ')}

)} {s.live && } @@ -459,7 +465,7 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { {busy ? 'Removing…' : others.length > 0 - ? `Remove all ${sweep.length} invalid entries` + ? `Remove ${others.length + 1} entries` : s.quarantined ? 'Remove entry' : 'Remove source'} diff --git a/apps/site/src/content/docs/docs/reference/manifest.md b/apps/site/src/content/docs/docs/reference/manifest.md index aa59d1c..cccac22 100644 --- a/apps/site/src/content/docs/docs/reference/manifest.md +++ b/apps/site/src/content/docs/docs/reference/manifest.md @@ -255,10 +255,12 @@ strictly, so an invalid layer can never be saved through a tolerated read. You can remove a broken row from the app — the Remove action on that row edits the manifest for you, and it is the only action offered there, since renaming or syncing something that was never built could only fail. Because a manifest is -only ever saved once the whole file validates, removing one broken row while -another remains is refused: the app asks to remove them together, and names the -others before you confirm. Anything else — settings included — stays blocked -until the manifest is valid again, and says so. +only ever saved once the whole file validates, any removal that would leave a +broken row behind is refused — including removing a source that is perfectly +healthy, since the same write rewrites the whole file. The app therefore removes +the broken rows alongside whatever you asked to remove, and names them before +you confirm. Settings stay blocked until the manifest is valid again, and say +so. GitHub may truncate very large recursive tree responses. ContextCake refuses to index that partial response as if it were complete; it serves the last complete diff --git a/packages/core/tests/service-test.sh b/packages/core/tests/service-test.sh index a126628..5d4048f 100755 --- a/packages/core/tests/service-test.sh +++ b/packages/core/tests/service-test.sh @@ -560,6 +560,22 @@ node -e ' code 404 "$(C -X DELETE "${AUTH[@]}" "$BASE5/api/sources?name=ghost")" "a name that is neither a layer nor a quarantined row is still 404" code 400 "$(C -X DELETE "${AUTH[@]}" "$BASE5/api/sources")" "DELETE with no name is still 400" +# An invalid entry blocks removing a HEALTHY source too — the write rewrites the +# whole manifest either way. So the same one-request repair has to accept a mix, +# or a user with a bad layer cannot remove anything at all. +cat > "$TMP/manifest-bad.json" < l.name)))' "$TMP/manifest-bad.json")" = '["seed"]' ] \ + && pass "the mixed removal left exactly the untouched layer" || fail "mixed removal wrong ($(cat "$TMP/manifest-bad.json"))" + echo "injected credentials: reported, never echoed" # The engine receives secrets by value from whoever owns the keychain. Two # things have to hold at once: the credential must actually reach the adapter,