diff --git a/frontend/src/api/workspaces.test.ts b/frontend/src/api/workspaces.test.ts index 89e5ec1d..482dd8f8 100644 --- a/frontend/src/api/workspaces.test.ts +++ b/frontend/src/api/workspaces.test.ts @@ -38,3 +38,41 @@ test("rejects a conflict payload that attempts to surface a secret field", async expect(asWorkspaceConflict(error)).toBeUndefined(); }); + +const diagnosticConflictFields = [ + "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", + "diagnostics.vector_rest.metadata.response.collection", "diagnostics.vector_rest.metadata.response.dimensions", + "diagnostics.vector_rest.metadata.response.distance", "diagnostics.vector_rest.reversible_probe.method", + "diagnostics.vector_rest.reversible_probe.path", "diagnostics.vector_rest.reversible_probe.auth", + "diagnostics.vector_rest.reversible_probe.response.operation", "diagnostics.embedding.method", "diagnostics.embedding.path", + "diagnostics.embedding.auth", "diagnostics.embedding.response.model", "diagnostics.embedding.response.dimensions", +] as const; + +test.each(diagnosticConflictFields)("accepts canonical diagnostic conflict leaf %s with its remote revision", async (field) => { + 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" } }, + }, + }; + 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: diagnosticsWorkspace, 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({ actual: { commit: "c".repeat(40), blob: "d".repeat(40) }, fields: [field] }); +}); diff --git a/frontend/src/api/workspaces.ts b/frontend/src/api/workspaces.ts index 735d586d..c9072c8b 100644 --- a/frontend/src/api/workspaces.ts +++ b/frontend/src/api/workspaces.ts @@ -115,11 +115,18 @@ export type PublishWorkspaceRequest = export interface WorkspaceConflict { code: "workspace_conflict"; fields: string[]; + expected: WorkspaceConflictRevision; + actual: WorkspaceConflictRevision; base: CanonicalWorkspace; local: CanonicalWorkspace; remote: CanonicalWorkspace; } +export interface WorkspaceConflictRevision { + commit: string; + blob?: string; +} + export interface WorkspaceApiError { status: number; code: WorkspaceErrorCode; @@ -128,13 +135,21 @@ export interface WorkspaceApiError { } const conflictFields = new Set([ - "workspace.name", "workspace.description", "workspace.language", - "dwh.database", "dwh.schema", "dwh.port", "dwh.timeout_ms", "dwh.supported_transports", - "semantic_index.vector_store.database", "semantic_index.vector_store.schema", "semantic_index.vector_store.collection", + "workspace.schema_version", "workspace.id", "workspace.name", "workspace.description", "workspace.language", + "dwh.engine", "dwh.database", "dwh.schema", "dwh.port", "dwh.timeout_ms", "dwh.supported_transports", + "semantic_index.vector_store.engine", "semantic_index.vector_store.database", "semantic_index.vector_store.schema", "semantic_index.vector_store.collection", "semantic_index.vector_store.dimensions", "semantic_index.vector_store.distance", "semantic_index.vector_store.port", "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.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", + "diagnostics.vector_rest.metadata.response.collection", "diagnostics.vector_rest.metadata.response.dimensions", + "diagnostics.vector_rest.metadata.response.distance", "diagnostics.vector_rest.reversible_probe.method", + "diagnostics.vector_rest.reversible_probe.path", "diagnostics.vector_rest.reversible_probe.auth", + "diagnostics.vector_rest.reversible_probe.response.operation", "diagnostics.embedding.method", "diagnostics.embedding.path", + "diagnostics.embedding.auth", "diagnostics.embedding.response.model", "diagnostics.embedding.response.dimensions", ]); const workspaceErrorCodes = new Set([ @@ -149,6 +164,15 @@ function object(value: unknown): Record | undefined { : undefined; } +function conflictRevision(value: unknown): WorkspaceConflictRevision | undefined { + const source = object(value); + const commit = source?.commit; + const blob = source?.blob; + if (typeof commit !== "string" || !/^[0-9a-f]{40}$/.test(commit)) return undefined; + if (blob !== undefined && (typeof blob !== "string" || !/^[0-9a-f]{40}$/.test(blob))) return undefined; + return { commit, ...(typeof blob === "string" ? { blob } : {}) }; +} + /** Sanitized registry error data; it intentionally excludes the raw response body. */ export function asWorkspaceApiError(error: unknown): WorkspaceApiError | undefined { if (!(error instanceof ApiError)) return undefined; @@ -173,10 +197,14 @@ export function asWorkspaceConflict(error: unknown): WorkspaceConflict | undefin const base = payload && sanitizeCanonicalWorkspace(payload.base); const local = payload && sanitizeCanonicalWorkspace(payload.local); const remote = payload && sanitizeCanonicalWorkspace(payload.remote); - if (!payload || !fields || !fields.every((field) => conflictFields.has(field)) || !base || !local || !remote) return undefined; + const expected = payload && conflictRevision(payload.expected); + const actual = payload && conflictRevision(payload.actual); + if (!payload || !fields || !fields.every((field) => conflictFields.has(field)) || !expected || !actual || !base || !local || !remote) return undefined; return { code: "workspace_conflict", fields, + expected, + actual, base, local, remote, diff --git a/frontend/src/shell/WorkspaceManager.test.tsx b/frontend/src/shell/WorkspaceManager.test.tsx index c3c7d8fe..43252f6c 100644 --- a/frontend/src/shell/WorkspaceManager.test.tsx +++ b/frontend/src/shell/WorkspaceManager.test.tsx @@ -46,6 +46,7 @@ test("lists registry workspaces and saves a new workspace only as a browser draf expect(await screen.findByRole("heading", { name: "Workspace management" })).toBeVisible(); expect(await screen.findByRole("button", { name: "PSD Clinical" })).toBeVisible(); + expect(screen.getByText(`Commit ${"a".repeat(12)}`)).toBeVisible(); await user.click(screen.getByRole("button", { name: "New workspace" })); await user.clear(screen.getByLabelText("Workspace ID")); await user.type(screen.getByLabelText("Workspace ID"), "trial-registry"); @@ -124,6 +125,38 @@ test("stages duplicate and delete operations without publishing", async () => { expect(published).toBe(false); }); +test("saves resolved conflict choices as a rebased browser draft without publishing again", async () => { + const user = userEvent.setup(); + let publishCalls = 0; + const local = { ...workspace, semantic_index: { ...workspace.semantic_index, embedding: { ...workspace.semantic_index.embedding, model: "local-model" } } }; + const remote = { ...workspace, semantic_index: { ...workspace.semantic_index, embedding: { ...workspace.semantic_index.embedding, model: "remote-model" } } }; + server.use( + http.post("http://localhost:8787/workspaces/validate", () => HttpResponse.json({ workspace: local, contract: {} })), + http.post("http://localhost:8787/workspaces/publish", () => { + publishCalls += 1; + return HttpResponse.json({ + code: "workspace_conflict", message: "Workspace changed in the registry.", fields: ["semantic_index.embedding.model"], + expected: { commit: "a".repeat(40), blob: "b".repeat(40) }, actual: { commit: "c".repeat(40), blob: "d".repeat(40) }, + base: workspace, local, remote, + }, { status: 409 }); + }), + ); + renderManager(); + + await user.click(await screen.findByRole("button", { name: "PSD Clinical" })); + await user.click(screen.getByRole("button", { name: "Publish draft" })); + 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 semantic_index.embedding.model" })); + await user.click(screen.getByRole("button", { name: "Save revised draft" })); + + expect(await screen.findByText("Revised draft saved with registry revision cccccccccccc. Validate it before publishing.")).toBeVisible(); + expect(localStorage.getItem("thothii.workspace-registry.v1.draft.psd-clinical")).toContain('"baseCommit":"cccccccccccccccccccccccccccccccccccccccc"'); + expect(localStorage.getItem("thothii.workspace-registry.v1.draft.psd-clinical")).toContain('"baseBlob":"dddddddddddddddddddddddddddddddddddddddd"'); + expect(publishCalls).toBe(1); +}); + test("proposes a different valid ID when duplicating a 63-character workspace ID", async () => { const user = userEvent.setup(); const maxId = `w${"a".repeat(62)}`; diff --git a/frontend/src/shell/WorkspaceManager.tsx b/frontend/src/shell/WorkspaceManager.tsx index 0218b7b6..4d49a0dd 100644 --- a/frontend/src/shell/WorkspaceManager.tsx +++ b/frontend/src/shell/WorkspaceManager.tsx @@ -234,6 +234,16 @@ export function WorkspaceManager({ open, onClose }: { open: boolean; onClose: () void Promise.all([statusQuery.refetch(), workspacesQuery.refetch(), detailQuery.refetch()]); } + function saveResolvedDraft(draft: WorkspaceDraft) { + workspaceDrafts.save(draft); + setSelectedId(draft.workspaceId); + setLocalDraft(draft); + setDeletionDraft(undefined); + setPublishRequest(undefined); + setDiagnostics([]); + setNotice(`Revised draft saved with registry revision ${draft.baseCommit.slice(0, 12)}. Validate it before publishing.`); + } + async function validateCurrent() { if (!currentDraft) return; setNotice(undefined); @@ -295,7 +305,7 @@ export function WorkspaceManager({ open, onClose }: { open: boolean; onClose: ()

Git status

- {statusQuery.isError ? { void statusQuery.refetch(); }} /> : statusQuery.isLoading ?

Loading…

: status && <>

{status.branch}

{status.degraded ? "Degraded" : "Current"} · ↑{status.ahead} ↓{status.behind}

} + {statusQuery.isError ? { void statusQuery.refetch(); }} /> : statusQuery.isLoading ?

Loading…

: status && <>

{status.branch}

{status.head &&

Commit {status.head.slice(0, 12)}

}

{status.degraded ? "Degraded" : "Current"} · ↑{status.ahead} ↓{status.behind}

}
@@ -333,7 +343,7 @@ export function WorkspaceManager({ open, onClose }: { open: boolean; onClose: () - {publishRequest && { if (!nextOpen) setPublishRequest(undefined); }} onPublished={published} onPull={pullLatest} onReload={reloadWorkspace} />} + {publishRequest && { if (!nextOpen) setPublishRequest(undefined); }} onPublished={published} onResolved={saveResolvedDraft} onPull={pullLatest} onReload={reloadWorkspace} />} ); } diff --git a/frontend/src/shell/WorkspacePublishDialog.test.tsx b/frontend/src/shell/WorkspacePublishDialog.test.tsx index 03883cba..95a56a95 100644 --- a/frontend/src/shell/WorkspacePublishDialog.test.tsx +++ b/frontend/src/shell/WorkspacePublishDialog.test.tsx @@ -23,6 +23,8 @@ const request: PublishWorkspaceRequest = { const conflict: WorkspaceConflict = { code: "workspace_conflict", fields: ["semantic_index.embedding.model"], + expected: { commit: "a".repeat(40), blob: "b".repeat(40) }, + actual: { commit: "c".repeat(40), blob: "d".repeat(40) }, base: workspace, local: { ...workspace, semantic_index: { ...workspace.semantic_index, embedding: { ...workspace.semantic_index.embedding, model: "local-model" } } }, remote: { ...workspace, semantic_index: { ...workspace.semantic_index, embedding: { ...workspace.semantic_index.embedding, model: "remote-model" } } }, @@ -40,7 +42,7 @@ test("validates a draft and requires a separate confirmation before publishing", publishCalls += 1; return HttpResponse.json({ revision: { id: "psd-clinical", commit: "c".repeat(40), blob: "d".repeat(40), snapshotPath: "/safe", state: "operational" } }); })); - render(); + render(); expect(screen.getByRole("button", { name: "Publish" })).toBeDisabled(); await user.click(screen.getByRole("button", { name: "Validate draft" })); @@ -61,7 +63,7 @@ test("shows a field-level conflict and never overwrites the remote workspace", a published = true; return HttpResponse.json({ ...conflict, message: "Workspace changed in the registry." }, { status: 409 }); })); - render(); + render(); await user.click(screen.getByRole("button", { name: "Validate draft" })); await user.click(await screen.findByRole("button", { name: "Publish" })); @@ -70,18 +72,41 @@ test("shows a field-level conflict and never overwrites the remote workspace", a expect(await screen.findByText("semantic_index.embedding.model")).toBeVisible(); expect(screen.getByText("local-model")).toBeVisible(); expect(screen.getByText("remote-model")).toBeVisible(); - expect(screen.getByRole("button", { name: "Pull latest registry" })).toBeVisible(); - expect(screen.getByRole("button", { name: "Reload workspace" })).toBeVisible(); - expect(screen.queryByRole("button", { name: /use local|use remote|confirm publish/i })).not.toBeInTheDocument(); + expect(screen.getByRole("radio", { name: "Use your draft for semantic_index.embedding.model" })).toBeVisible(); + expect(screen.getByRole("radio", { name: "Use registry value for semantic_index.embedding.model" })).toBeVisible(); + expect(screen.getByRole("button", { name: "Save revised draft" })).toBeDisabled(); expect(published).toBe(true); }); +test("saves explicit local choices as a rebased draft and does not republish it", async () => { + const user = userEvent.setup(); + const saved = vi.fn(); + let publishCalls = 0; + server.use(http.post("http://localhost:8787/workspaces/publish", () => { + publishCalls += 1; + return HttpResponse.json({ ...conflict, 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 semantic_index.embedding.model" })); + 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({ semantic_index: expect.objectContaining({ embedding: expect.objectContaining({ model: "local-model" }) }) }), + })); + expect(publishCalls).toBe(1); +}); + 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({ ...conflict, message: "Workspace changed in the registry.", }, { status: 409 }))); - render(); + render(); await user.click(screen.getByRole("button", { name: "Validate draft" })); await user.click(await screen.findByRole("button", { name: "Publish" })); diff --git a/frontend/src/shell/WorkspacePublishDialog.tsx b/frontend/src/shell/WorkspacePublishDialog.tsx index c0418764..ac4e628c 100644 --- a/frontend/src/shell/WorkspacePublishDialog.tsx +++ b/frontend/src/shell/WorkspacePublishDialog.tsx @@ -9,6 +9,7 @@ import { type WorkspaceConflict, type WorkspaceRevision, } from "../api/workspaces"; +import type { WorkspaceDraft } from "../workspaces/drafts"; import { Button } from "../components/ui/button"; import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from "../components/ui/dialog"; @@ -17,6 +18,7 @@ export interface WorkspacePublishDialogProps { request: PublishWorkspaceRequest; onOpenChange: (open: boolean) => void; onPublished: (revision: WorkspaceRevision | undefined) => void; + onResolved: (draft: WorkspaceDraft) => void; onPull: () => Promise | void; onReload: () => void; } @@ -30,6 +32,25 @@ function valueAt(workspace: CanonicalWorkspace, path: string): string { : JSON.stringify(value); } +function valueAtPath(workspace: CanonicalWorkspace, path: string): unknown { + return path.split(".").reduce((current, key) => ( + current && typeof current === "object" ? (current as Record)[key] : undefined + ), workspace); +} + +function replaceAtPath(value: Record, path: string[], replacement: unknown): Record { + const [key, ...remaining] = path; + const copy = { ...value }; + if (remaining.length === 0) { + if (replacement === undefined) delete copy[key]; + else copy[key] = replacement; + return copy; + } + const child = copy[key]; + copy[key] = replaceAtPath(child && typeof child === "object" && !Array.isArray(child) ? child as Record : {}, remaining, replacement); + return copy; +} + function requestWithWorkspace(request: PublishWorkspaceRequest, workspace: CanonicalWorkspace): PublishWorkspaceRequest { return request.action === "delete" ? request : { ...request, workspace }; } @@ -39,10 +60,17 @@ function RevisionSummary({ request }: { request: PublishWorkspaceRequest }) { return

{label} · base {request.baseCommit.slice(0, 12)}

; } -function ConflictReview({ conflict, onPull, onReload }: { conflict: WorkspaceConflict; onPull: () => Promise | void; onReload: () => void }) { +function ConflictReview({ conflict, onPull, onReload, onResolved }: { + conflict: WorkspaceConflict; + onPull: () => Promise | void; + onReload: () => void; + onResolved: (draft: WorkspaceDraft) => void; +}) { const [pulling, setPulling] = useState(false); const [pulled, setPulled] = useState(false); const [pullError, setPullError] = useState(false); + const [choices, setChoices] = useState>({}); + const canSave = Boolean(conflict.actual.blob) && conflict.fields.every((field) => choices[field]); async function pull() { setPulling(true); @@ -57,10 +85,26 @@ function ConflictReview({ conflict, onPull, onReload }: { conflict: WorkspaceCon } } + function saveRevisedDraft() { + if (!conflict.actual.blob || !canSave) return; + const workspace = conflict.fields.reduce((current, field) => ( + choices[field] === "local" + ? replaceAtPath(current as unknown as Record, field.split("."), valueAtPath(conflict.local, field)) as unknown as CanonicalWorkspace + : current + ), conflict.remote); + onResolved({ + workspaceId: workspace.workspace.id, + baseCommit: conflict.actual.commit, + baseBlob: conflict.actual.blob, + workspace, + updatedAt: new Date().toISOString(), + }); + } + return

The registry changed before publication.

-

Your browser draft is unchanged. Pull or reload before creating a revised draft; this screen never merges or overwrites remote values.

+

Choose a value for every changed field to save a revised browser draft against registry revision {conflict.actual.commit.slice(0, 12)}. Saving never publishes it.

{conflict.fields.map((field) =>
@@ -70,18 +114,24 @@ function ConflictReview({ conflict, onPull, onReload }: { conflict: WorkspaceCon
Your draft
{valueAt(conflict.local, field)}
Registry
{valueAt(conflict.remote, field)}
+
+ Resolve {field} + + +
)}
{pulled &&

Latest registry state pulled. Reload the workspace before editing or publishing again.

} {pullError &&

Registry pull could not be completed. Try again or reload the workspace.

}
+
; } -export function WorkspacePublishDialog({ open, request, onOpenChange, onPublished, onPull, onReload }: WorkspacePublishDialogProps) { +export function WorkspacePublishDialog({ open, request, onOpenChange, onPublished, onResolved, onPull, onReload }: WorkspacePublishDialogProps) { const [validatedRequest, setValidatedRequest] = useState(); const [confirmationOpen, setConfirmationOpen] = useState(false); const [conflict, setConflict] = useState(); @@ -141,10 +191,10 @@ export function WorkspacePublishDialog({ open, request, onOpenChange, onPublishe {conflict ? "Publication conflict" : "Publish workspace"} - {conflict ? "Compare the changed fields, then pull and reload before revising your draft." : "Validation and an explicit confirmation are required before this shared definition is published."} + {conflict ? "Choose each local or registry value, then save a revised draft for normal validation and confirmation." : "Validation and an explicit confirmation are required before this shared definition is published."}
- {conflict ? : <> + {conflict ? : <> {message &&

{message}

}
diff --git a/task-10-report.md b/task-10-report.md index 4b5688b5..ab64e702 100644 --- a/task-10-report.md +++ b/task-10-report.md @@ -16,6 +16,17 @@ Conflict payloads now pass through the canonical draft sanitizer and reject unknown/secret fields before rendering. +## Review round 1 + +- Replaced the pull/reload-only conflict recovery with an explicit choice of the local draft or + registry value for every changed field. A revised draft can be saved only after every field has + a choice; it is rebased to the conflict's `actual.commit` and `actual.blob` and is never + published automatically. +- Kept normal validation and the explicit publish confirmation as mandatory steps after saving a + resolution. Nothing silently discards the local draft or merges it into the registry. +- Added typed expected/actual conflict revisions, displayed the active registry `status.head` + commit, and whitelisted every canonical `diagnostics.*` leaf path structurally. + ## TDD evidence - Wrote the publish-dialog and manager import/export tests before the implementation and observed @@ -24,14 +35,17 @@ before wiring the conflict parser through the canonical sanitizer. - Added a regression test for a failed pull during conflict recovery and observed the original unhandled rejection before adding the redacted in-dialog error state. +- Added review-round tests first for per-field local/registry selection, rebased draft saving + 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. ## Verification Run in `frontend/` after the final changes: ```text -npx vitest run src/shell/WorkspacePublishDialog.test.tsx src/api/workspaces.test.ts src/shell/WorkspaceManager.test.tsx -# 3 files passed, 15 tests passed +npx vitest run src/shell/WorkspacePublishDialog.test.tsx src/api/workspaces.test.ts src/shell/WorkspaceManager.test.tsx src/workspaces/drafts.test.ts +# 4 files passed, 47 tests passed npx tsc -b # exit 0 @@ -39,5 +53,5 @@ npx tsc -b ```text npx vitest run -# 51 files passed, 370 tests passed +# 51 files passed, 392 tests passed ```