diff --git a/CLAIM_INVENTORY.md b/CLAIM_INVENTORY.md index e0ed7d86d..c5099707e 100644 --- a/CLAIM_INVENTORY.md +++ b/CLAIM_INVENTORY.md @@ -97,6 +97,10 @@ must not be inferred from a row here. See | Only intervals a five-field cron can name are declarable; the rest are refused by name | accounts/AGENTS.md § A datasource source is refreshed by three things | `TestTriggerScheduleSpellsOnlyTheIntervalsCronCanName` (`pkg/business`) | implemented | accounts | | An occurrence under the reference arm is minted from current facts, and the next one after a revocation is refused | MODULE.md § Unattended work; WORK_CONTEXTS.md § Triggers | `TestTriggerOccurrenceMintsFromTheReferenceAndIsRefusedAfterRevocation` (`pkg/adapters`) | implemented | accounts | | The installation-principal arm mints for a binding whose audience is the executing module's own name | WORK_CONTEXTS.md § Triggers | `TestTriggerOccurrenceMintsFromTheInstallationBinding`, `TestTriggerIdentityWithoutADeclarationRefusesBothArms` (`pkg/adapters`) | implemented | accounts | +| "Sync now" admits `ReconcileSource` under a kit-minted effect identity, and a double press yields one receipt and one job | MODULE.md § Unattended work; saas-ui README § Datasources | `reconcile-pass.test.tsx` (`packages/saas-ui`) drives the panel: one identity per pass, the repeat press recovers through `LookupReconcileSource` without re-admitting, the identity survives a failure | implemented | frontend | +| No surface in this repository reaches a source sync without an effect identity | MODULE.md § Unattended work; saas-ui README § Datasources | `reconcile-is-the-only-manual-pass.test.ts` walks the TypeScript AST of `src` and the published packages and fails on any `syncSource` on a datasource client | implemented | frontend | +| A person's pass is listed beside the declared schedule's occurrences, with its effect identity | MODULE.md § Unattended work | `source-trigger-schedules.container.test.tsx` asserts the manual row is in the SAME `admissions` table as the two scheduled rows, and that a recovered receipt replaces its row rather than adding one | implemented | frontend | +| Every pass a source ever took, listed | — | none, and none is possible here: a receipt is recovered by identity and never enumerated, so the panel shows only the passes this browser admitted and says so | not claimed | accounts | | A schedule actually coming due, in a composed environment, under either arm | MODULE.md § Unattended work ("what is not proven") | none — nothing in this repository composes a module that executes declared work | planned | #1081 | ## Handbook and interface surfaces diff --git a/MODULE.md b/MODULE.md index ca417f000..a9cdec503 100644 --- a/MODULE.md +++ b/MODULE.md @@ -192,6 +192,22 @@ it, and a test walks the service's AST to keep that true. One occurrence admits `DatasourceService/ReconcileSource`, the same host operation a person's manual pass takes. +**"Sync now" is that admission, not a request beside it** (issue #1094). The +published kit points the button at `ReconcileSource` under an **effect identity +the kit mints**, so a person's pass and a scheduled occurrence are one admission +of one shape and the receipts cannot tell them apart. A press that repeats the +pass still being reported reads the original receipt back through +`LookupReconcileSource` and surfaces *that* — one receipt and one job for two +presses, rather than two full re-sends of the source — and the press after that +is a new pass. `syncSource`, which reached the same sync engines carrying no +effect identity, is **deleted** from the kit rather than deprecated, and a +TypeScript AST walk over this repository fails on any remaining caller +(`src/features/datasources/__tests__/reconcile-is-the-only-manual-pass.test.ts`). +The reconcile-schedules panel lists a person's pass in the **same table** as the +declared schedule's last and next occurrence, with its effect identity beside +them; the passes it can list are the ones this browser admitted, because a +receipt is recovered by identity and never enumerated. + **What is not proven:** no live occurrence exists. Nothing in this repository composes a module that executes declared work, so the declaration is built and held to the contract's field names by test, and the walk that has not been diff --git a/module/services/accounts/AGENTS.md b/module/services/accounts/AGENTS.md index 17cd97beb..312427b57 100644 --- a/module/services/accounts/AGENTS.md +++ b/module/services/accounts/AGENTS.md @@ -947,6 +947,10 @@ person pressing "Sync now", and by the **periodic reconcile**. Only the last of the three is guaranteed to exist, so it is what makes "we sync on push" honest: without it, a missed webhook is permanent and nothing ever notices. +Two of those three are now the **same admission**: a person's "Sync now" and one +occurrence of the declared schedule both admit `ReconcileSource` under an effect +identity their caller minted (issue #1094). + - **The schedule is DECLARED here and FIRED somewhere else** (issue #1081). `business.DeclareSourceTrigger` turns a source's interval into a trigger an declared release carries — a five-field cron in UTC, `"skip"` for both missed @@ -959,16 +963,24 @@ without it, a missed webhook is permanent and nothing ever notices. ticker in `work.go`, `enqueueScheduledReconcile`, `enqueueScheduledPullSync`, `scheduledReconcileKey` and the due query they read (`ListDatasourceSourcesDueForReconcile`). -- **One occurrence admits one host operation.** `DatasourceService/ReconcileSource` - (marked `codefly.runnable.v0.operation`, idempotency required, paired with - `LookupReconcileSource`) is what the declared trigger names, and it is the same - work a person asks for by hand. It routes each provider to the engine that +- **One occurrence admits one host operation, and so does a person's press.** + `DatasourceService/ReconcileSource` (marked `codefly.runnable.v0.operation`, + idempotency required, paired with `LookupReconcileSource`) is what the declared + trigger names, and since #1094 it is literally what "Sync now" calls as well — + the published kit's `syncSource`, which reached these engines carrying no + effect identity, is deleted. It routes each provider to the engine that provider syncs on: GitHub to a conditional reconcile on the delivery queue (the job resolves the head and snapshots only if it moved), everything else to the - same sync request "Sync now" produces. The enqueue is keyed by the occurrence's + pull its connector has. The enqueue is keyed by the occurrence's effect identity, so two attempts of one occurrence are one job — which is what the schedule-derived key had to stand in for while two replicas could both - sweep one due row. + sweep one due row, and which is now also what makes a person's double press + one pass. **The host has no notion of an identity being spent**: it returns the + same receipt for the same identity forever, so "when is my intent new" is the + CLIENT's question, and the kit answers it from presses rather than from a + clock. There is no enumerable list of past passes either — a receipt is read + back by identity, never listed — so a surface claiming to show every pass a + source ever took would be inventing one. - **The interval is per provider, and it is the source's own configuration.** 30 minutes for GitHub; a day for the pull providers, whose connectors re-send their whole content on every sync (the gap registered against each in diff --git a/module/services/frontend/code/package-lock.json b/module/services/frontend/code/package-lock.json index cc2ee7cc5..4b72d51f5 100644 --- a/module/services/frontend/code/package-lock.json +++ b/module/services/frontend/code/package-lock.json @@ -15233,7 +15233,7 @@ }, "packages/codefly-ui": { "name": "@codefly-dev/ui", - "version": "0.12.7", + "version": "0.12.8", "license": "Apache-2.0", "dependencies": { "@base-ui/react": "^1.3.0", @@ -15381,7 +15381,7 @@ }, "packages/saas-ui": { "name": "@codefly-dev/saas-ui", - "version": "0.12.7", + "version": "0.12.8", "license": "Apache-2.0", "devDependencies": { "@bufbuild/protobuf": "^2.11.0", diff --git a/module/services/frontend/code/packages/codefly-ui/package.json b/module/services/frontend/code/packages/codefly-ui/package.json index 056f89478..c1df6ed7d 100644 --- a/module/services/frontend/code/packages/codefly-ui/package.json +++ b/module/services/frontend/code/packages/codefly-ui/package.json @@ -1,6 +1,6 @@ { "name": "@codefly-dev/ui", - "version": "0.12.7", + "version": "0.12.8", "description": "The shared, versioned Codefly SaaS frontend kit: the plugin host and the tokens-as-data skin mechanism, standalone so the host and Module-Federation remotes import and dedupe one copy.", "repository": { "type": "git", diff --git a/module/services/frontend/code/packages/saas-ui/README.md b/module/services/frontend/code/packages/saas-ui/README.md index 414c305fe..4fe8cfbfc 100644 --- a/module/services/frontend/code/packages/saas-ui/README.md +++ b/module/services/frontend/code/packages/saas-ui/README.md @@ -170,9 +170,10 @@ const directory = usePrincipalDirectory(); // loading | ready | refused | failed a **Connect GitHub** action. The last-sync cell shows whichever clock applies: `last_synced_at` for a provider that pulls, or `last ingest · ` for a github source, whose change sets the compiler enqueues - (webhook delivery, periodic reconcile, or a tenant's "Sync now" forced - reconcile) — it never sets `last_synced_at`, so "Never" is dropped rather than - shown above live provenance. Loading/error/empty are first-class. Connect, + (a webhook delivery, or a reconcile pass — a declared schedule's occurrence or + a tenant's "Sync now", which are the same admission) — it never sets + `last_synced_at`, so "Never" is dropped rather than shown above live + provenance. Loading/error/empty are first-class. Connect, Sync, Reconnect, Use GitHub App and Delete are offered only to a viewer who may use them (`canManage`; with a `gateway`, read from the viewer's credential): anyone else keeps a row's History and is told who connects @@ -258,7 +259,11 @@ const directory = usePrincipalDirectory(); // loading | ready | refused | failed offers so the consumer can show the ingesting module's progress. - **Error** — `DEGRADED` or `PAUSED`: the host's `status_reason` verbatim, and Reconnect, which asks for the credential again (a replacement PAT, or - nothing on the App and public paths) and enqueues a sync. + nothing on the App and public paths) and enqueues a sync. Reconnect needs + the client's `reconnectSource`: a reconnect rebinds whose authority the + source's syncs run under, which is not a reconcile and has no idempotent + form, so a client without it is told who reconnects rather than offered a + control that would do something else. There is no fourth state. A source whose status an **older host does not report** (the field decodes to its proto default, which the gateway maps to `unknown` rather than guessing) reads as **Connected** and says its state is @@ -291,6 +296,26 @@ const directory = usePrincipalDirectory(); // loading | ready | refused | failed Do not mount this and `` on the same page: both redeem the GitHub App's single-use return state, and the loser reports a rejection for an installation that in fact succeeded. +- **"Sync now" is an admission, not a request.** It runs + `DatasourceService/ReconcileSource` — the host operation a source's *declared* + reconcile schedule admits one occurrence of — under an **effect identity the + kit mints**, so a person's pass and a scheduled pass are one admission of one + shape and are indistinguishable in the host's receipts. `useReconcileSource` + owns the protocol: it presents an identity from a `ReconcilePassLedger` it + holds for the mounted surface (`createReconcilePassLedger`), and a + press that **repeats** the pass still being reported reads the original + receipt back through `LookupReconcileSource` and surfaces *that* — one receipt + and one job for two presses, rather than two full re-sends of the source. The + press after a repeat is a new pass (the ledger retires the identity); a press + after a *failure* re-presents the same identity, which makes a retry a + recovery. A lookup answering `"unknown"` is **inconclusive**, never "it did + not happen". The panel and `` render the outcome the + operation returned — the identity, the status and the job. + `onSourceReconcileAdmitted((pass) => …)` hears each one, which is how a host + surface shows a person's pass beside the declared schedule's occurrences. + There is **no `syncSource`**: it reached the same engines carrying no effect + identity, so two presses were two passes, and it is deleted rather than + deprecated. - `createDatasourceClient({ apiBase, getAccessToken, refreshAccessToken })` — builds the gateway-bound `DatasourceClient` (with 401 refresh-and-retry) directly, for driving the hooks outside the panel. `datasourceClientOverTransport(transport)` does the same over a @@ -298,7 +323,7 @@ const directory = usePrincipalDirectory(); // loading | ready | refused | failed sync as typed phases (queued, fetching, compiled, handed off, done, failed) with a timestamp for each, the compiled change set, and a typed failure. - `onSourceSyncRequested((sourceId) => …)` — hears every sync the kit's client enqueues - (Sync now, a reconnect, the first sync a connect starts), so a view rendering a source's + (a reconcile pass, a reconnect, the first sync a connect starts), so a view rendering a source's progress in the panel's per-source slot can watch closely at once. It returns the unsubscribe. The registry lives on `globalThis`, so a remote's own copy of the kit hears the host panel. diff --git a/module/services/frontend/code/packages/saas-ui/package.json b/module/services/frontend/code/packages/saas-ui/package.json index d62870fe0..a38f62d47 100644 --- a/module/services/frontend/code/packages/saas-ui/package.json +++ b/module/services/frontend/code/packages/saas-ui/package.json @@ -1,6 +1,6 @@ { "name": "@codefly-dev/saas-ui", - "version": "0.12.7", + "version": "0.12.8", "description": "Reusable SaaS-domain frontend components (datasources, …) the portal and solutions both import, built on @codefly-dev/ui primitives and driven by an injected typed client.", "repository": { "type": "git", diff --git a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/collection-access.test.tsx b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/collection-access.test.tsx index d34c8eb7c..e79fab8ef 100644 --- a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/collection-access.test.tsx +++ b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/collection-access.test.tsx @@ -29,7 +29,12 @@ const collection: CollectionAccessView = { const client = (overrides: Partial): DatasourceClient => ({ listSources: async () => [], addGitHubSource: async () => {}, - syncSource: async () => "job", + reconcileSource: async (_orgId, _id, effectId) => ({ + effectId, + status: "committed" as const, + jobId: "job", + }), + lookupReconcileSource: async () => undefined, deleteSource: async () => {}, ...overrides, }); diff --git a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/datasources-panel.test.tsx b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/datasources-panel.test.tsx index a3f0a295d..9d7190749 100644 --- a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/datasources-panel.test.tsx +++ b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/datasources-panel.test.tsx @@ -16,6 +16,7 @@ import type { AccessibleScopeView, DatasourceClient, DatasourceView, + SourceReconcileReceipt, } from "../types.js"; afterEach(cleanup); @@ -35,7 +36,13 @@ function fakeClient( return { listSources: vi.fn(async () => [] as DatasourceView[]), addGitHubSource: vi.fn(async () => {}), - syncSource: vi.fn(async () => "job-1"), + reconcileSource: vi.fn(async (_orgId, _id, effectId: string) => ({ + effectId, + status: "committed" as const, + jobId: "job-1", + })), + lookupReconcileSource: vi.fn(async () => undefined), + reconnectSource: vi.fn(async () => "job-reconnect"), deleteSource: vi.fn(async () => {}), ...overrides, }; @@ -484,7 +491,7 @@ describe("DatasourcesPanel", () => { it("surfaces a sync failure in the panel instead of swallowing it", async () => { const client = fakeClient({ listSources: vi.fn(async () => [sampleSource]), - syncSource: vi.fn(async () => { + reconcileSource: vi.fn(async () => { throw new Error("gateway timeout"); }), }); @@ -504,11 +511,11 @@ describe("DatasourcesPanel", () => { it("tracks sync state per row and blocks a double-enqueue", async () => { // A sync that never settles keeps its row pending, exposing whether a // second row's state leaks onto the first (the shared-mutation bug). - const pending = new Promise(() => {}); - const syncSource = vi.fn(() => pending); + const pending = new Promise(() => {}); + const reconcileSource = vi.fn(() => pending); const client = fakeClient({ listSources: vi.fn(async () => [sampleSource, secondSource]), - syncSource, + reconcileSource, }); renderWithClient(); @@ -528,7 +535,7 @@ describe("DatasourcesPanel", () => { // Clicking row 1 again is a no-op: its button is disabled, so no second job. fireEvent.click(screen.getByRole("button", { name: /^Syncing…$/ })); - expect(syncSource).toHaveBeenCalledTimes(1); + expect(reconcileSource).toHaveBeenCalledTimes(1); fireEvent.click(stillIdle); await waitFor(() => @@ -536,7 +543,7 @@ describe("DatasourcesPanel", () => { screen.getAllByRole("button", { name: /^Syncing…$/ }), ).toHaveLength(2), ); - expect(syncSource).toHaveBeenCalledTimes(2); + expect(reconcileSource).toHaveBeenCalledTimes(2); }); }); @@ -819,7 +826,7 @@ it("acknowledges queued sync without claiming ingestion completed", async () => it("shows a rejected credential as an actionable message", async () => { const client = fakeClient({ listSources: vi.fn(async () => [sampleSource]), - syncSource: vi.fn(async () => { + reconcileSource: vi.fn(async () => { throw new ConnectError( "rpc error: code = FailedPrecondition desc = GitHub rejected the access token (401). Reconnect the source.", Code.FailedPrecondition, @@ -838,17 +845,19 @@ it.each(["first", "second"])( "settles concurrent syncs when %s finishes first", async (finishFirst) => { let rejectFirst!: (error: Error) => void; - let resolveSecond!: (id: string) => void; - const first = new Promise((_, reject) => { + let resolveSecond!: (receipt: SourceReconcileReceipt) => void; + const first = new Promise((_, reject) => { rejectFirst = reject; }); - const second = new Promise((resolve) => { + const second = new Promise((resolve) => { resolveSecond = resolve; }); const onSyncEnqueued = vi.fn(); const client = fakeClient({ listSources: vi.fn(async () => [sampleSource, secondSource]), - syncSource: vi.fn((_, id) => (id === sampleSource.id ? first : second)), + reconcileSource: vi.fn((_org: string, id: string) => + id === sampleSource.id ? first : second, + ), }); renderWithClient( resolveSecond("job-2"); + const resolve = () => + resolveSecond({ + effectId: "effect-2", + status: "committed", + jobId: "job-2", + }); await act(async () => { (finishFirst === "first" ? reject : resolve)(); }); @@ -915,12 +929,12 @@ it("reconnects the same source without creating or deleting a source", async () renderWithClient(); await openRowActions(); fireEvent.click(await screen.findByRole("menuitem", { name: "Reconnect" })); - const token = screen.getByLabelText("New GitHub PAT"); + const token = screen.getByLabelText("New GitHub PAT (optional)"); expect(token.getAttribute("type")).toBe("password"); fireEvent.change(token, { target: { value: "replacement-test-token" } }); fireEvent.click(screen.getByRole("button", { name: "Reconnect and sync" })); await waitFor(() => - expect(client.syncSource).toHaveBeenCalledWith( + expect(client.reconnectSource).toHaveBeenCalledWith( "org-1", "ds-1", "replacement-test-token", @@ -934,11 +948,7 @@ it("reconnects the same source without creating or deleting a source", async () }); it("reconnects a public source without a credential when the client can", async () => { - const reconnectSource = vi.fn(async () => "job-reconnect"); - const client = fakeClient({ - listSources: vi.fn(async () => [sampleSource]), - reconnectSource, - }); + const client = fakeClient({ listSources: vi.fn(async () => [sampleSource]) }); renderWithClient(); await openRowActions(); fireEvent.click(await screen.findByRole("menuitem", { name: "Reconnect" })); @@ -946,20 +956,35 @@ it("reconnects a public source without a credential when the client can", async expect(token.hasAttribute("required")).toBe(false); fireEvent.click(screen.getByRole("button", { name: "Reconnect and sync" })); await waitFor(() => - expect(reconnectSource).toHaveBeenCalledWith("org-1", "ds-1", undefined), + expect(client.reconnectSource).toHaveBeenCalledWith( + "org-1", + "ds-1", + undefined, + ), ); - expect(client.syncSource).not.toHaveBeenCalled(); + // A reconnect is not a reconcile: it rebinds whose authority the source's + // syncs run under, so it must not travel as a pass under an effect identity. + expect(client.reconcileSource).not.toHaveBeenCalled(); expect((await screen.findByRole("status")).textContent).toContain( "Reconnected. Sync queued", ); }); +// The fallback that went with `syncSource`: a plain sync carrying a replacement +// credential, which the host reads as a reconnect. A client that cannot +// reconnect is now offered no Reconnect at all, rather than a control that +// quietly does a different operation. +it("offers no Reconnect to a client that cannot reconnect", async () => { + const client = fakeClient({ listSources: vi.fn(async () => [sampleSource]) }); + delete client.reconnectSource; + renderWithClient(); + await openRowActions(); + await screen.findByRole("menuitem", { name: "Delete" }); + expect(screen.queryByRole("menuitem", { name: "Reconnect" })).toBeNull(); +}); + it("reconnects with a replacement PAT through the reconnect call when the client has one", async () => { - const reconnectSource = vi.fn(async () => "job-reconnect"); - const client = fakeClient({ - listSources: vi.fn(async () => [sampleSource]), - reconnectSource, - }); + const client = fakeClient({ listSources: vi.fn(async () => [sampleSource]) }); renderWithClient(); await openRowActions(); fireEvent.click(await screen.findByRole("menuitem", { name: "Reconnect" })); @@ -968,7 +993,7 @@ it("reconnects with a replacement PAT through the reconnect call when the client }); fireEvent.click(screen.getByRole("button", { name: "Reconnect and sync" })); await waitFor(() => - expect(reconnectSource).toHaveBeenCalledWith( + expect(client.reconnectSource).toHaveBeenCalledWith( "org-1", "ds-1", "replacement-test-token", diff --git a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/declared-source-card.test.tsx b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/declared-source-card.test.tsx index f33c80f8f..d5c18de6f 100644 --- a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/declared-source-card.test.tsx +++ b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/declared-source-card.test.tsx @@ -33,7 +33,16 @@ function fakeClient( return { listSources: vi.fn(async () => [] as DatasourceView[]), addGitHubSource: vi.fn(async () => {}), - syncSource: vi.fn(async () => "job-1"), + reconcileSource: vi.fn(async (_orgId, _id, effectId: string) => ({ + effectId, + status: "committed" as const, + jobId: "job-1", + })), + lookupReconcileSource: vi.fn(async () => undefined), + // The only reconnect path left: a plain sync carrying a replacement + // credential went with `syncSource`, so a fake without this exercises + // "no Reconnect offered" rather than a fallback. + reconnectSource: vi.fn(async () => "job-reconnect"), deleteSource: vi.fn(async () => {}), ...overrides, }; @@ -205,9 +214,16 @@ describe("DeclaredSourceCard", () => { // Connected is not a state that asks for a credential again. expect(screen.queryByLabelText("Authentication")).toBeNull(); + // "Sync now" admits the host's reconcile OPERATION under an effect + // identity the card minted, so the card asks for one pass rather than + // sending a request that carries no identity at all. fireEvent.click(screen.getByRole("button", { name: "Sync now" })); await waitFor(() => - expect(client.syncSource).toHaveBeenCalledWith("org-1", "ds-1"), + expect(client.reconcileSource).toHaveBeenCalledWith( + "org-1", + "ds-1", + expect.any(String), + ), ); expect(onSyncEnqueued).toHaveBeenCalledWith("job-1"); }); diff --git a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/directory.test.tsx b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/directory.test.tsx index 1315b4e00..20d66ed8c 100644 --- a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/directory.test.tsx +++ b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/directory.test.tsx @@ -40,7 +40,12 @@ function baseClient(overrides: Partial): DatasourceClient { return { listSources: vi.fn(async () => []), addGitHubSource: vi.fn(async () => {}), - syncSource: vi.fn(async () => "job-1"), + reconcileSource: vi.fn(async (_orgId, _id, effectId: string) => ({ + effectId, + status: "committed" as const, + jobId: "job-1", + })), + lookupReconcileSource: vi.fn(async () => undefined), deleteSource: vi.fn(async () => {}), ...overrides, }; diff --git a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/gateway.test.tsx b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/gateway.test.tsx index 2883c9c07..8d0f4ac62 100644 --- a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/gateway.test.tsx +++ b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/gateway.test.tsx @@ -154,7 +154,8 @@ describe("createDatasourceClient", () => { client.completeGitHubAppSetup!("org-1", "state-1", "42", "oauth-1"), migrateGitHubSourceToApp: () => client.migrateGitHubSourceToApp!("org-1", "ds-1"), - syncSource: () => client.syncSource("org-1", "ds-1"), + reconcileSource: () => client.reconcileSource("org-1", "ds-1", "effect-1"), + lookupReconcileSource: () => client.lookupReconcileSource("effect-1"), reconnectSource: () => client.reconnectSource!("org-1", "ds-1"), getSourceSync: () => client.getSourceSync!("org-1", "ds-1"), deleteSource: () => client.deleteSource("org-1", "ds-1"), @@ -194,7 +195,7 @@ describe("createDatasourceClient", () => { const failing = onSourceSyncRequested(() => { throw new Error("a listener's failure is its own"); }); - await client.syncSource("org-1", "ds-1"); + await client.reconcileSource("org-1", "ds-1", "effect-1"); await client.addGitHubSource({ orgId: "org-1", repo: "acme/example", @@ -205,7 +206,7 @@ describe("createDatasourceClient", () => { }); stop(); failing(); - await client.syncSource("org-1", "ds-1").catch(() => {}); + await client.reconcileSource("org-1", "ds-1", "effect-2").catch(() => {}); expect(heard).toEqual(["ds-1", "ds-new"]); }); @@ -632,6 +633,10 @@ it("reads source-specific typed audit history", async () => { expect(events[0].fields).toEqual({ processed: 45, job_id: "j1" }); }); +// A replacement credential now reaches `SyncSource` on ONE path, the reconnect, +// and that is the point of the method surviving the removal of "Sync now"'s use +// of it: a reconnect rebinds whose authority a source's syncs run under, which +// is not a reconcile and nothing makes idempotent. it("serializes a replacement credential only for reconnect on the existing source", async () => { const { calls } = stubFetch({ jobId: "job-1" }); const client = createDatasourceClient({ @@ -639,18 +644,17 @@ it("serializes a replacement credential only for reconnect on the existing sourc getAccessToken: () => "viewer", }); await expect( - client.syncSource("org-1", "source-1", "test-only-replacement"), + client.reconnectSource!("org-1", "source-1", "test-only-replacement"), ).resolves.toBe("job-1"); - await client.syncSource("org-1", "source-1"); expect(calls[0].url).toContain( "saas.accounts.v1.DatasourceService/SyncSource", ); expect(calls[0].body).toEqual({ orgId: "org-1", id: "source-1", + reconnect: true, accessToken: "test-only-replacement", }); - expect(calls[1].body).toEqual({ orgId: "org-1", id: "source-1" }); }); it("asks for a reconnect, with or without a replacement credential", async () => { diff --git a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/reconcile-pass.test.tsx b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/reconcile-pass.test.tsx new file mode 100644 index 000000000..79599e136 --- /dev/null +++ b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/reconcile-pass.test.tsx @@ -0,0 +1,241 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { + cleanup, + fireEvent, + render, + screen, + waitFor, +} from "@testing-library/react"; +import { afterEach, expect, it, vi } from "vitest"; +import { DatasourcesPanel } from "../datasources-panel.js"; +import { onSourceReconcileAdmitted } from "../reconcile-passes.js"; +import type { + DatasourceClient, + DatasourceView, + SourceReconcileReceipt, +} from "../types.js"; + +// "SYNC NOW" IS AN ADMISSION OF THE SAME OPERATION A SCHEDULE'S OCCURRENCE IS. +// +// What these cover that the panel's own suite does not: the operation protocol +// underneath the button. A pass carries an effect identity the kit minted, a +// repeat press recovers the FIRST pass's receipt instead of starting a second, +// and the identity survives a failure so that a retry is a recovery rather +// than a second full re-send of the source. + +afterEach(cleanup); + +const source: DatasourceView = { + id: "ds-1", + orgId: "org-1", + provider: "github", + repo: "acme/handbook", + paths: ["docs/"], + branch: "main", + boundaryNodeId: "11111111-1111-1111-1111-111111111111", + webhookConfigured: true, + status: "active", + lastSyncedAt: undefined, + createdAt: undefined, +}; + +function renderPanel(client: DatasourceClient): void { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + render( + + + , + ); +} + +/** A client whose reconcile commits, recording the identity each pass used. */ +function committingClient(overrides: Partial = {}) { + const admitted: string[] = []; + const receipts = new Map(); + const client: DatasourceClient = { + listSources: vi.fn(async () => [source]), + addGitHubSource: vi.fn(async () => {}), + reconcileSource: vi.fn(async (_orgId, _id, effectId: string) => { + admitted.push(effectId); + // The host is idempotent on the identity: a second call under one + // identity returns the receipt the first committed, never a new job. + const held = receipts.get(effectId); + if (held) return held; + const receipt: SourceReconcileReceipt = { + effectId, + status: "committed", + jobId: `job-${receipts.size + 1}`, + }; + receipts.set(effectId, receipt); + return receipt; + }), + lookupReconcileSource: vi.fn(async (effectId: string) => { + return receipts.get(effectId); + }), + deleteSource: vi.fn(async () => {}), + ...overrides, + }; + return { client, admitted, receipts }; +} + +const press = async () => + fireEvent.click(await screen.findByRole("button", { name: /^Sync$/ })); + +const notice = async () => + (await screen.findByRole("status")).textContent ?? ""; + +const settled = () => + waitFor(() => + expect(screen.queryByRole("button", { name: /^Syncing/ })).toBeNull(), + ); + +it("admits one pass under an effect identity the kit minted", async () => { + const { client, admitted } = committingClient(); + renderPanel(client); + await press(); + await settled(); + expect(admitted).toHaveLength(1); + // Minted here, never taken from the host: an identity the server chose + // would make every press a new pass. + expect(admitted[0]).toBeTruthy(); + expect(client.lookupReconcileSource).not.toHaveBeenCalled(); + // The outcome the operation returned, not an acknowledgement that a request + // was sent: the identity and the job are both on screen. + const text = await notice(); + expect(text).toContain(admitted[0]); + expect(text).toContain("job-1"); +}); + +it("answers a double press with the first pass's receipt, not a second pass", async () => { + const { client, admitted, receipts } = committingClient(); + renderPanel(client); + await press(); + await settled(); + await press(); + await settled(); + + // One identity, one receipt, one job — the whole point of the operation. + expect(new Set(admitted).size).toBe(1); + expect(receipts.size).toBe(1); + // The second press RECOVERED rather than re-admitted: the lookup ran, and + // the operation was not called a second time. + expect(client.lookupReconcileSource).toHaveBeenCalledWith(admitted[0]); + expect(client.reconcileSource).toHaveBeenCalledTimes(1); + const text = await notice(); + expect(text).toContain("already being reconciled"); + expect(text).toContain(admitted[0]); + expect(text).toContain("job-1"); +}); + +it("believes the press after a recovered receipt, and it is a new pass", async () => { + const { client, admitted, receipts } = committingClient(); + renderPanel(client); + await press(); + await settled(); + await press(); + await settled(); + await press(); + await settled(); + + // Two passes, two identities, two jobs — and the middle press produced + // neither. There is no clock in that rule: the person was told the pass was + // already recorded, so the next press is a request they mean. + expect(receipts.size).toBe(2); + const identities = [...new Set(admitted)]; + expect(identities).toHaveLength(2); + expect(await notice()).toContain("job-2"); +}); + +it("re-presents the same identity when a lookup is inconclusive", async () => { + const { client, admitted } = committingClient({ + // The host knows the attempt but holds no receipt: "unknown" is + // INCONCLUSIVE, never "it did not happen". + lookupReconcileSource: vi.fn(async (effectId: string) => ({ + effectId, + status: "unknown" as const, + jobId: "", + })), + }); + renderPanel(client); + await press(); + await settled(); + await press(); + await settled(); + + expect(client.reconcileSource).toHaveBeenCalledTimes(2); + // The SAME identity both times: a new one would be a second pass over work + // that may well be running. + expect(new Set(admitted).size).toBe(1); +}); + +it("keeps the identity through a failure, so a retry recovers the pass", async () => { + const receipts = new Map(); + const admitted: string[] = []; + let failNext = true; + const client: DatasourceClient = { + listSources: vi.fn(async () => [source]), + addGitHubSource: vi.fn(async () => {}), + reconcileSource: vi.fn(async (_orgId, _id, effectId: string) => { + admitted.push(effectId); + if (failNext) { + failNext = false; + // The pass may well have committed on the host; the caller cannot + // tell. That is exactly when the identity must survive. + throw new Error("gateway timeout"); + } + const receipt: SourceReconcileReceipt = { + effectId, + status: "committed", + jobId: "job-1", + }; + receipts.set(effectId, receipt); + return receipt; + }), + lookupReconcileSource: vi.fn(async (effectId: string) => + receipts.get(effectId), + ), + deleteSource: vi.fn(async () => {}), + }; + renderPanel(client); + await press(); + await waitFor(() => + expect(screen.getByRole("alert").textContent).toContain("gateway timeout"), + ); + await press(); + await settled(); + + expect(admitted).toHaveLength(2); + expect(admitted[0]).toBe(admitted[1]); + expect(receipts.size).toBe(1); +}); + +it("announces each pass with its effect identity, for a host surface to list", async () => { + const { client, admitted } = committingClient(); + const heard: { effectId: string; sourceId: string; repeat: boolean }[] = []; + const stop = onSourceReconcileAdmitted((pass) => + heard.push({ + effectId: pass.effectId, + sourceId: pass.sourceId, + repeat: pass.repeat, + }), + ); + const failing = onSourceReconcileAdmitted(() => { + throw new Error("a listener's failure is its own"); + }); + renderPanel(client); + await press(); + await settled(); + await press(); + await settled(); + stop(); + failing(); + + expect(heard).toEqual([ + { effectId: admitted[0], sourceId: "ds-1", repeat: false }, + // A repeat is reported under the SAME identity, which is how a listener + // replaces the row rather than showing one admission as two. + { effectId: admitted[0], sourceId: "ds-1", repeat: true }, + ]); +}); diff --git a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/sync-progress.test.tsx b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/sync-progress.test.tsx index 68f58ab55..42679904b 100644 --- a/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/sync-progress.test.tsx +++ b/module/services/frontend/code/packages/saas-ui/src/datasources/__tests__/sync-progress.test.tsx @@ -66,7 +66,12 @@ function fakeClient( return { listSources: vi.fn(async () => [source]), addGitHubSource: vi.fn(async () => {}), - syncSource: vi.fn(async () => "job-1"), + reconcileSource: vi.fn(async (_orgId, _id, effectId: string) => ({ + effectId, + status: "committed" as const, + jobId: "job-1", + })), + lookupReconcileSource: vi.fn(async () => undefined), deleteSource: vi.fn(async () => {}), ...overrides, }; diff --git a/module/services/frontend/code/packages/saas-ui/src/datasources/datasources-panel.tsx b/module/services/frontend/code/packages/saas-ui/src/datasources/datasources-panel.tsx index 457adcf86..9d386f62d 100644 --- a/module/services/frontend/code/packages/saas-ui/src/datasources/datasources-panel.tsx +++ b/module/services/frontend/code/packages/saas-ui/src/datasources/datasources-panel.tsx @@ -47,8 +47,9 @@ import { useAddGitHubSource, useDeleteSource, useListSources, - useSyncSource, + useReconcileSource, } from "./queries.js"; +import type { AdmittedReconcilePass } from "./reconcile-passes.js"; import type { ConnectGitHubValues } from "./schema.js"; import { SourceSyncProgress, useSourceSync } from "./sync-progress.js"; import type { @@ -273,7 +274,7 @@ function DatasourcesPanelView({ ); // Row action errors have no other surface (no toast dependency, no global // mutation handler), so they would vanish silently without this. - const [syncNotice, setSyncNotice] = useState(null); + const [syncNotice, setSyncNotice] = useState(null); const [actionError, setActionError] = useState(null); const list = useListSources(client, orgId); @@ -322,7 +323,7 @@ function DatasourcesPanelView({ ) : undefined; const addMutation = useAddGitHubSource(client); - const syncMutation = useSyncSource(client); + const reconcileMutation = useReconcileSource(client); const deleteMutation = useDeleteSource(client); const handleConnect = (values: ConnectGitHubValues) => { @@ -452,9 +453,10 @@ function DatasourcesPanelView({ setMigratingIds((prev) => new Set(prev).add(source.id)); try { await migrateToApp(orgId, source.id); - setSyncNotice( - `${source.repo} now authenticates through the GitHub App. Its stored token is no longer used.`, - ); + setSyncNotice({ + title: "Sync queued", + body: `${source.repo} now authenticates through the GitHub App. Its stored token is no longer used.`, + }); await list.refetch(); } catch (error) { setActionError( @@ -465,6 +467,11 @@ function DatasourcesPanelView({ } }; + // "Sync now" is an admission, not a sweep: it runs the same host operation a + // declared reconcile schedule's occurrence does, under an effect identity + // the press minted. The notice therefore reports the OUTCOME the operation + // returned — the identity and the job — rather than announcing that a + // request was sent. const handleSync = async (source: DatasourceView) => { setSyncNotice(null); setActionError(null); @@ -472,11 +479,12 @@ function DatasourcesPanelView({ // Per-call callbacks on a shared mutation only observe the latest call. // Await each request so every row reports its result and clears pending. try { - const jobId = await syncMutation.mutateAsync({ orgId, id: source.id }); - setSyncNotice( - `Sync queued for ${source.repo}. Ingestion runs in the background; content will appear in the collection when ready.`, - ); - onSyncEnqueued?.(jobId); + const pass = await reconcileMutation.mutateAsync({ + orgId, + id: source.id, + }); + setSyncNotice(describePass(source.repo, pass)); + if (pass.jobId) onSyncEnqueued?.(pass.jobId); } catch (error) { setActionError(`Couldn't sync ${source.repo}: ${messageOf(error)}`); } finally { @@ -597,11 +605,11 @@ function DatasourcesPanelView({ {syncNotice && ( setSyncNotice(null)} dismissLabel="Dismiss the sync notice" > - {syncNotice} + {syncNotice.body} )} {actionError && ( @@ -684,10 +692,14 @@ function DatasourcesPanelView({ canManage={canManage} onActivity={client.listActivity ? setActivitySource : undefined} onExecution={renderSourceExecution ? setExecutionSource : undefined} - onReconnect={(source) => { - setReconnectError(undefined); - setReconnecting(source); - }} + onReconnect={ + reconnectSource + ? (source) => { + setReconnectError(undefined); + setReconnecting(source); + } + : undefined + } sources={sources} boundaries={boundaries} onMigrateToApp={migrateToApp ? handleMigrateToApp : undefined} @@ -807,10 +819,9 @@ function DatasourcesPanelView({

)} - {canManage && reconnecting && ( + {canManage && reconnecting && reconnectSource && ( { @@ -820,18 +831,17 @@ function DatasourcesPanelView({ setReconnectPending(true); setReconnectError(undefined); try { - const jobId = reconnectSource - ? await reconnectSource( - orgId, - reconnecting.id, - token || undefined, - ) - : await client.syncSource(orgId, reconnecting.id, token); - setSyncNotice( - token + const jobId = await reconnectSource( + orgId, + reconnecting.id, + token || undefined, + ); + setSyncNotice({ + title: "Sync queued", + body: token ? `Credential replaced. Sync queued for ${reconnecting.repo}. Open History for ingestion results.` : `Reconnected. Sync queued for ${reconnecting.repo}. Open History for ingestion results.`, - ); + }); setReconnecting(null); onSyncEnqueued?.(jobId); await list.refetch(); @@ -1065,7 +1075,14 @@ function SourcesTable({ onActivity?: (source: DatasourceView) => void; /** Present only when a consumer handed the panel an execution view. */ onExecution?: (source: DatasourceView) => void; - onReconnect: (source: DatasourceView) => void; + /** + * Present only when the client can reconnect. A reconnect rebinds whose + * authority the source's syncs run under, which is not a reconcile and has + * no idempotent form here; the plain sync that used to stand in for it went + * with `syncSource`, so a client without `reconnectSource` is offered no + * Reconnect rather than a path that does something else. + */ + onReconnect?: (source: DatasourceView) => void; onMigrateToApp?: (source: DatasourceView) => void; renderSourceDetail?: (source: DatasourceView) => ReactNode; }) { @@ -1203,7 +1220,7 @@ function SourcesTable({ align="end" className="w-auto min-w-44" > - {source.provider === "github" && ( + {onReconnect && source.provider === "github" && ( onReconnect(source)} > @@ -1529,17 +1546,57 @@ function SourceHistory({ ); } +/** What the panel says about one action, with the heading it says it under. */ +interface SyncNotice { + title: string; + body: string; +} + +/** + * The outcome one reconcile pass returned, as the panel reports it. + * + * Three readings, and the differences between them are the point of running + * this as an operation at all: + * + * - a pass this press admitted — the job is new work; + * - a pass this press REPEATED — the receipt was read back, no second re-send + * of the source happened, and the job named is the first press's; + * - a pass the host did not report as committed, which is INCONCLUSIVE rather + * than refused, so the person is told to press again to recover it rather + * than left to read silence as failure. + * + * The effect identity is in every one of them. It is what makes the pass the + * same object a declared schedule's occurrence is, and it is the handle a + * person quotes when asking an operator what happened. + */ +function describePass(repo: string, pass: AdmittedReconcilePass): SyncNotice { + const job = pass.jobId + ? `, job ${pass.jobId}` + : "; the host traced no job for it"; + if (pass.repeat) + return { + title: "That pass is already recorded", + body: `${repo} is already being reconciled under pass ${pass.effectId}${job}. Nothing was sent a second time. Press Sync again for a new pass.`, + }; + if (pass.status !== "committed") + return { + title: "This pass is unconfirmed", + body: `The host did not report pass ${pass.effectId} for ${repo} as committed, which is inconclusive rather than refused. Press Sync again to recover the same pass rather than starting another.`, + }; + return { + title: "Sync queued", + body: `Reconcile admitted for ${repo} under pass ${pass.effectId}${job}. Ingestion runs in the background; content will appear in the collection when ready.`, + }; +} + function ReconnectSource({ source, - credentialOptional, pending, error, onSubmit, onCancel, }: { source: DatasourceView; - /** The client reconnects a source that holds no credential without one. */ - credentialOptional: boolean; pending: boolean; error?: string; onSubmit: (token: string) => Promise; @@ -1552,7 +1609,7 @@ function ReconnectSource({ className="space-y-3 rounded-lg border p-4" onSubmit={(event) => { event.preventDefault(); - if (!pending && (credentialOptional || token.trim())) { + if (!pending) { const replacement = token.trim(); setToken(""); void onSubmit(replacement); @@ -1560,17 +1617,21 @@ function ReconnectSource({ }} >

Reconnect {source.repo}

+ {/* The credential is optional in one direction only: a source that + holds one is refused a credential-less reconnect by the host, which + is the only place that knows whether it holds one. Saying so here + would be a second opinion. */}

- {credentialOptional - ? "Its syncs will run on your behalf from now on. Enter a new PAT to replace the saved one, or leave it empty for a public repository. A sync starts right away; your source, collection, content, and history are preserved." - : "Replace the saved PAT and start a sync. Your source, collection, content, and history are preserved."} + Its syncs will run on your behalf from now on. Enter a new PAT to + replace the saved one, or leave it empty for a public repository. A sync + starts right away; your source, collection, content, and history are + preserved.