From 8effdc6c89c69860c1595b62ab6135636282fa33 Mon Sep 17 00:00:00 2001 From: mptyl Date: Tue, 4 Aug 2026 07:05:51 +0200 Subject: [PATCH] fix: report nested workspace conflicts --- backend/src/workspaces/registry.ts | 4 +- backend/test/workspace-registry.test.ts | 117 ++++++++++++++++++ frontend/src/api/workspaces.test.ts | 35 ++++++ frontend/src/api/workspaces.ts | 1 + .../src/shell/WorkspacePublishDialog.test.tsx | 32 +++++ task-10-report.md | 38 +++++- 6 files changed, 225 insertions(+), 2 deletions(-) diff --git a/backend/src/workspaces/registry.ts b/backend/src/workspaces/registry.ts index 2fb3d7ff..91fb8a39 100644 --- a/backend/src/workspaces/registry.ts +++ b/backend/src/workspaces/registry.ts @@ -254,7 +254,9 @@ export class WorkspaceRegistry { remote: unknown, prefix = "", ): string[] { - if (!base || !remote) return ["workspace.id"]; + if (base === undefined || remote === undefined) { + return base === remote ? [] : [prefix || "workspace.id"]; + } if (Array.isArray(base) || Array.isArray(remote) || typeof base !== "object" || typeof remote !== "object") { return JSON.stringify(base) === JSON.stringify(remote) ? [] : [prefix]; } diff --git a/backend/test/workspace-registry.test.ts b/backend/test/workspace-registry.test.ts index 6d5fd73a..459a6ce6 100644 --- a/backend/test/workspace-registry.test.ts +++ b/backend/test/workspace-registry.test.ts @@ -39,6 +39,97 @@ llm_policy: allowed: [zai/glm-5.2] `; +function withDwhRestTransport(source: string): string { + return source.replace( + "supported_transports: [postgres_direct]", + "supported_transports: [postgres_direct, rest_api]", + ); +} + +function withDwhRestDiagnostic(source: string): string { + return withDwhRestTransport(source).concat(`diagnostics: + dwh_rest: + method: GET + path: /health + auth: none + response: + database: database + schema: schema +`); +} + +function withEmbeddingDiagnostic(source: string): string { + return source.concat(`diagnostics: + embedding: + method: GET + path: /models + auth: none + response: + model: model + dimensions: dimensions +`); +} + +function withDwhRestAndEmbeddingDiagnostics(source: string): string { + return withDwhRestTransport(source).concat(`diagnostics: + dwh_rest: + method: GET + path: /health + auth: none + response: + database: database + schema: schema + embedding: + method: GET + path: /models + auth: none + response: + model: model + dimensions: dimensions +`); +} + +function withVectorRestTransport(source: string): string { + return source.replace( + "supported_transports: [pgvector_direct]", + "supported_transports: [pgvector_direct, rest_api]", + ); +} + +function withVectorMetadataDiagnostic(source: string): string { + return withVectorRestTransport(source).concat(`diagnostics: + vector_rest: + metadata: + method: GET + path: /metadata + auth: none + response: + collection: collection + dimensions: dimensions + distance: distance +`); +} + +function withReversibleVectorProbe(source: string): string { + return withVectorRestTransport(source).concat(`diagnostics: + vector_rest: + metadata: + method: GET + path: /metadata + auth: none + response: + collection: collection + dimensions: dimensions + distance: distance + reversible_probe: + method: POST + path: /probe + auth: bearer + response: + operation: operation +`); +} + const runFile = promisify(execFile); const temporaryRoots: string[] = []; @@ -237,6 +328,32 @@ test("reports stale publish conflicts with expected and actual revisions", async }); }); +test.each([ + ["adds", withEmbeddingDiagnostic(withDwhRestTransport(validYaml)), withDwhRestAndEmbeddingDiagnostics(validYaml), "diagnostics.dwh_rest"], + ["removes", withDwhRestAndEmbeddingDiagnostics(validYaml), withEmbeddingDiagnostic(withDwhRestTransport(validYaml)), "diagnostics.dwh_rest"], + ["adds", withVectorMetadataDiagnostic(validYaml), withReversibleVectorProbe(validYaml), "diagnostics.vector_rest.reversible_probe"], + ["removes", withReversibleVectorProbe(validYaml), withVectorMetadataDiagnostic(validYaml), "diagnostics.vector_rest.reversible_probe"], +])("reports an optional diagnostics branch when the registry %s it", async (_operation, baseSource, remoteSource, field) => { + const remote = await fixture(baseSource); + const registry = new WorkspaceRegistry(config(join(remote.root, "registry"), remote.remote)); + await registry.bootstrap(); + const initial = await registry.read("psd-clinical"); + writeFileSync(join(remote.source, "workspaces", "psd-clinical.yaml"), remoteSource); + await git(remote.source, ["add", "workspaces/psd-clinical.yaml"]); + await git(remote.source, ["commit", "-m", `Registry ${_operation} diagnostic branch`]); + await git(remote.source, ["push", "origin", "main"]); + + await expect(registry.publish({ + action: "update", + workspace: workspaceWith("psd-clinical", { description: "Local stale change" }), + baseCommit: initial.revision.commit, + baseBlob: initial.revision.blob, + })).rejects.toMatchObject({ + code: "workspace_conflict", + fields: [field], + }); +}); + test("restores a clean checkout after a failed commit and retries publication", async () => { const remote = await fixture(); const root = join(remote.root, "registry"); diff --git a/frontend/src/api/workspaces.test.ts b/frontend/src/api/workspaces.test.ts index 482dd8f8..756d1502 100644 --- a/frontend/src/api/workspaces.test.ts +++ b/frontend/src/api/workspaces.test.ts @@ -50,6 +50,41 @@ const diagnosticConflictFields = [ "diagnostics.embedding.auth", "diagnostics.embedding.response.model", "diagnostics.embedding.response.dimensions", ] as const; +const optionalDiagnosticsConflictFields = [ + "diagnostics", + "diagnostics.dwh_rest", + "diagnostics.vector_rest", + "diagnostics.vector_rest.reversible_probe", + "diagnostics.embedding", +] as const; + +const diagnosticsWorkspace: CanonicalWorkspace = { + ...workspace, + dwh: { ...workspace.dwh, supported_transports: ["postgres_direct", "rest_api"] }, + semantic_index: { ...workspace.semantic_index, vector_store: { ...workspace.semantic_index.vector_store, supported_transports: ["pgvector_direct", "rest_api"] } }, + diagnostics: { + dwh_rest: { method: "POST", path: "/rpc/ping", auth: "bearer", response: { database: "database", schema: "schema" } }, + vector_rest: { + metadata: { method: "GET", path: "/vector/metadata", auth: "bearer", response: { collection: "collection", dimensions: "dimensions", distance: "distance" } }, + reversible_probe: { method: "POST", path: "/vector/probe", auth: "x-api-key", response: { operation: "operation" } }, + }, + embedding: { method: "GET", path: "/models", auth: "none", response: { model: "model", dimensions: "dimensions" } }, + }, +}; + +test.each(optionalDiagnosticsConflictFields)("accepts optional diagnostics conflict branch %s", async (field) => { + server.use(http.post("http://localhost:8787/workspaces/publish", () => HttpResponse.json({ + code: "workspace_conflict", message: "Workspace changed in the registry.", fields: [field], + expected: { commit: "a".repeat(40), blob: "b".repeat(40) }, + actual: { commit: "c".repeat(40), blob: "d".repeat(40) }, + base: workspace, local: diagnosticsWorkspace, remote: diagnosticsWorkspace, + }, { status: 409 }))); + + const error = await publishWorkspace({ action: "update", workspace: diagnosticsWorkspace, baseCommit: "a".repeat(40), baseBlob: "b".repeat(40) }).catch((cause: unknown) => cause); + + expect(asWorkspaceConflict(error)).toMatchObject({ fields: [field] }); +}); + test.each(diagnosticConflictFields)("accepts canonical diagnostic conflict leaf %s with its remote revision", async (field) => { const diagnosticsWorkspace: CanonicalWorkspace = { ...workspace, diff --git a/frontend/src/api/workspaces.ts b/frontend/src/api/workspaces.ts index c9072c8b..27ace594 100644 --- a/frontend/src/api/workspaces.ts +++ b/frontend/src/api/workspaces.ts @@ -142,6 +142,7 @@ const conflictFields = new Set([ "semantic_index.vector_store.timeout_ms", "semantic_index.vector_store.supported_transports", "semantic_index.embedding.provider", "semantic_index.embedding.model", "semantic_index.embedding.dimensions", "semantic_index.embedding.timeout_ms", "semantic_index.vector_writer", "llm_policy.default", "llm_policy.allowed", + "diagnostics", "diagnostics.dwh_rest", "diagnostics.vector_rest", "diagnostics.vector_rest.reversible_probe", "diagnostics.embedding", "diagnostics.dwh_rest.method", "diagnostics.dwh_rest.path", "diagnostics.dwh_rest.auth", "diagnostics.dwh_rest.response.database", "diagnostics.dwh_rest.response.schema", "diagnostics.vector_rest.metadata.method", "diagnostics.vector_rest.metadata.path", "diagnostics.vector_rest.metadata.auth", diff --git a/frontend/src/shell/WorkspacePublishDialog.test.tsx b/frontend/src/shell/WorkspacePublishDialog.test.tsx index 95a56a95..07db7e0f 100644 --- a/frontend/src/shell/WorkspacePublishDialog.test.tsx +++ b/frontend/src/shell/WorkspacePublishDialog.test.tsx @@ -30,6 +30,18 @@ const conflict: WorkspaceConflict = { remote: { ...workspace, semantic_index: { ...workspace.semantic_index, embedding: { ...workspace.semantic_index.embedding, model: "remote-model" } } }, }; +const diagnosticsBranchConflict: WorkspaceConflict = { + ...conflict, + fields: ["diagnostics.dwh_rest"], + base: workspace, + remote: workspace, + local: { + ...workspace, + dwh: { ...workspace.dwh, supported_transports: ["postgres_direct", "rest_api"] }, + diagnostics: { dwh_rest: { method: "GET", path: "/health", auth: "none", response: { database: "database", schema: "schema" } } }, + }, +}; + beforeEach(() => { server.use(http.post("http://localhost:8787/workspaces/validate", () => HttpResponse.json({ workspace, contract: {} }))); }); @@ -101,6 +113,26 @@ test("saves explicit local choices as a rebased draft and does not republish it" expect(publishCalls).toBe(1); }); +test("rebases a selected optional diagnostics branch into the revised draft", async () => { + const user = userEvent.setup(); + const saved = vi.fn(); + server.use(http.post("http://localhost:8787/workspaces/publish", () => HttpResponse.json({ + ...diagnosticsBranchConflict, message: "Workspace changed in the registry.", + }, { status: 409 }))); + render(); + + await user.click(screen.getByRole("button", { name: "Validate draft" })); + await user.click(await screen.findByRole("button", { name: "Publish" })); + await user.click(screen.getByRole("button", { name: "Confirm publish" })); + await user.click(await screen.findByRole("radio", { name: "Use your draft for diagnostics.dwh_rest" })); + await user.click(screen.getByRole("button", { name: "Save revised draft" })); + + expect(saved).toHaveBeenCalledWith(expect.objectContaining({ + baseCommit: "c".repeat(40), baseBlob: "d".repeat(40), + workspace: expect.objectContaining({ diagnostics: diagnosticsBranchConflict.local.diagnostics }), + })); +}); + test("keeps a conflict open and redacts a failed registry pull", async () => { const user = userEvent.setup(); server.use(http.post("http://localhost:8787/workspaces/publish", () => HttpResponse.json({ diff --git a/task-10-report.md b/task-10-report.md index ab64e702..20edf8d9 100644 --- a/task-10-report.md +++ b/task-10-report.md @@ -39,6 +39,20 @@ without a second publish, active-commit rendering, and every canonical diagnostics conflict path; these initially failed against the pull/reload-only UI and narrow path parser. +## Review round 2 + +- Fixed recursive registry diffs so add/remove changes to optional nested diagnostics branches + report their actual canonical paths instead of the fallback `workspace.id`. The regression cases + cover both add and remove for `diagnostics.dwh_rest` and + `diagnostics.vector_rest.reversible_probe`. +- Extended the conflict-path allowlist to accept the optional `diagnostics` root and every + optional diagnostics branch. The existing structural rebase now saves an explicitly selected + branch (including an added or removed branch) in the revised browser draft, still pinned to the + registry's actual revision and requiring normal validation and confirmation before publishing. +- Wrote the backend/frontend cases first and observed the expected RED failures: backend conflict + fields were `workspace.id`, while the frontend rejected the safe conflict payload before the + resolution UI could render. + ## Verification Run in `frontend/` after the final changes: @@ -53,5 +67,27 @@ npx tsc -b ```text npx vitest run -# 51 files passed, 392 tests passed +# 51 files passed, 398 tests passed ``` + +Round-2 focused verification: + +```text +backend: npx vitest run test/workspace-registry.test.ts +# 1 file passed, 23 tests passed + +backend: npx tsc --noEmit -p . +# exit 0 + +frontend: npx vitest run +# 51 files passed, 398 tests passed + +frontend: npx tsc -b +# exit 0 +``` + +The full backend `npx vitest run` was also attempted after allowing its local SSE test socket. +The Task 10 registry tests passed, but seven unchanged SSE/session tests fail because their +default, unbootstrapped registry makes session authorization return the intentional +`session storage is unavailable` response. This failure is outside the Task 10 diff; it persists +without any changed Task 10 route or test-harness code.