diff --git a/CLAUDE.md b/CLAUDE.md index 52299e1..4b738aa 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, 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/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..de531f7 100644 --- a/apps/console/src/views/Sources.test.tsx +++ b/apps/console/src/views/Sources.test.tsx @@ -163,6 +163,95 @@ 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 () => { + // 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('layer 4') + + 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') { + 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..a5ac84b 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,15 @@ 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) + // 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 setSelectedName(ordered[0]?.name ?? null) @@ -230,11 +239,17 @@ export function Sources({ onAddSource }: { onAddSource?: () => void }) { } } + // 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.name, ...alsoInvalid(s).map((source) => source.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 +328,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 +395,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 +432,46 @@ 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 && (() => { + // 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 ( +
+

+ {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 also removes{' '} + {others.length === 1 ? 'it' : 'them'}:{' '} + {others.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..cccac22 100644 --- a/apps/site/src/content/docs/docs/reference/manifest.md +++ b/apps/site/src/content/docs/docs/reference/manifest.md @@ -252,6 +252,16 @@ 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, 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 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..5d4048f 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,76 @@ 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" + +# 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, @@ -585,4 +659,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; }