From 07ebf56301f08b214e8de9c74b3cc97559e14ca3 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Thu, 3 Sep 2026 00:49:53 -0700 Subject: [PATCH 1/3] Add tests for duplicate-callback recovery reportError (CL-7255) --- .../test/report-error-routing.test.ts | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/packages/onboarding/test/report-error-routing.test.ts b/packages/onboarding/test/report-error-routing.test.ts index 01f6e5e53..27f74f863 100644 --- a/packages/onboarding/test/report-error-routing.test.ts +++ b/packages/onboarding/test/report-error-routing.test.ts @@ -231,3 +231,65 @@ describe("plant-env-credentials.ts catches report through reportError and never expect(cause).toBeInstanceOf(Error); }); }); + +describe("recentlyConnectedCredential reports through reportError and still finds nothing", () => { + test("a hub failure during duplicate-callback recovery reports operation + userId and still ends as state_expired", async () => { + // A malformed principals page that parseAs will reject, carrying a + // token-shaped string so this test can prove the recovery catch never + // interpolates the raw cause into deps.log (the same redaction bar + // plant-env-credentials.ts holds — CL-7255). + const SECRET = "sk-or-v1-thisisafakesecretvalue"; + const logs: string[] = []; + const hub = new Hono(); + hub.get("/api/me/principals", (c) => + c.json({ data: SECRET, nextCursor: null }), + ); + const server = Bun.serve({ port: 0, fetch: hub.fetch }); + try { + const routes = createOnboardingRoutes({ + hubUrl: `http://localhost:${server.port}`, + pushWorkflow: async () => ({ + outcome: "pushed" as const, + commitSha: "a".repeat(40), + }), + log: (line) => { + logs.push(line); + }, + pendingSeedStore, + }); + const app = mountAuthenticated(routes); + + const response = await app.request( + "/oauth/openrouter/callback?code=auth_code_1", + { + headers: { + cookie: "workbench_openrouter_connect=not-a-real-state", + }, + }, + ); + + expect(response.status).toBe(302); + const redirect = new URL( + response.headers.get("location") ?? "", + "https://x", + ); + expect(redirect.searchParams.get("outcome")).toBe("error"); + expect(redirect.searchParams.get("code")).toBe("state_expired"); + + expect(reportErrorCalls).toHaveLength(1); + const [cause, context] = reportErrorCalls[0] as [ + unknown, + Record, + ]; + expect(context.operation).toBe("onboarding_duplicate_callback_recovery"); + expect(context.tenantId).toBeUndefined(); + expect((context.extra as { userId: string }).userId).toBe("user_1"); + // reportError is the one place the unredacted cause may travel — + // error-sink redacts before anything reaches a log sink. + expect(cause).toBeInstanceOf(Error); + expect(logs.join("\n")).not.toContain(SECRET); + } finally { + server.stop(true); + } + }); +}); From 79266940572b52c1c5960f6da11090b5284f8f79 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Thu, 3 Sep 2026 00:49:57 -0700 Subject: [PATCH 2/3] Report duplicate-callback recovery through reportError (CL-7255) The recentlyConnectedCredential catch still treats a hub failure as found-nothing so the caller ends at state_expired. It now reports the cause with operation and userId, and no longer interpolates the raw message into the operational log. --- packages/onboarding/src/routes.ts | 9 +++++++-- scripts/checks/report-error-baseline.txt | 1 - 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/onboarding/src/routes.ts b/packages/onboarding/src/routes.ts index 0b611e893..0d778e0a8 100644 --- a/packages/onboarding/src/routes.ts +++ b/packages/onboarding/src/routes.ts @@ -287,9 +287,14 @@ async function recentlyConnectedCredential( ); return match ? tenant : undefined; } catch (cause) { - const message = cause instanceof Error ? cause.message : String(cause); + // Never the raw cause detail — this path lists credentials after a + // connect and can see secret-shaped hub/parse errors (CL-7255). + const refId = reportError(cause, { + operation: "onboarding_duplicate_callback_recovery", + extra: { userId: args.userId }, + }); args.log( - `duplicate-callback recovery check failed for user ${args.userId}: ${message}`, + `duplicate-callback recovery check failed for user ${args.userId} [${refId}]`, ); return undefined; } diff --git a/scripts/checks/report-error-baseline.txt b/scripts/checks/report-error-baseline.txt index dcefb0449..a52dd44b5 100644 --- a/scripts/checks/report-error-baseline.txt +++ b/scripts/checks/report-error-baseline.txt @@ -211,7 +211,6 @@ packages/ollama-adapter/src/inline-tool-json.ts 1 return false; packages/ollama-adapter/src/inline-tool-json.ts 1 return { kind: "incomplete" }; packages/onboarding/src/pending-seed.ts 1 return drop(); packages/onboarding/src/routes.ts 1 // Neither `ProvisionError` nor `CliError` messages are safe to show -packages/onboarding/src/routes.ts 1 const message = cause instanceof Error ? cause.message : String(cause); packages/onboarding/src/routes.ts 1 if (cause instanceof ProvisionError) { packages/onboarding/src/routes.ts 1 return c.json( packages/presence/src/artifact-persistence.ts 1 deps.onSnapshotError?.(key, error); From e3d67a7fdcf89caae94573d67e15f62d5fa1fbda Mon Sep 17 00:00:00 2001 From: Sawyer Date: Thu, 3 Sep 2026 14:58:22 -0700 Subject: [PATCH 3/3] Make the recovery SECRET log assertion load-bearing (CL-7255) A type-mismatch parse summary is `was string` and never mentions the value, so asserting logs omit SECRET was a tautology. Put the token in a union field arktype echoes, prove the cause carries it, and reject it in the reportError context and operational logs. --- .../test/report-error-routing.test.ts | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/packages/onboarding/test/report-error-routing.test.ts b/packages/onboarding/test/report-error-routing.test.ts index 27f74f863..f4136ec9a 100644 --- a/packages/onboarding/test/report-error-routing.test.ts +++ b/packages/onboarding/test/report-error-routing.test.ts @@ -234,15 +234,28 @@ describe("plant-env-credentials.ts catches report through reportError and never describe("recentlyConnectedCredential reports through reportError and still finds nothing", () => { test("a hub failure during duplicate-callback recovery reports operation + userId and still ends as state_expired", async () => { - // A malformed principals page that parseAs will reject, carrying a - // token-shaped string so this test can prove the recovery catch never - // interpolates the raw cause into deps.log (the same redaction bar - // plant-env-credentials.ts holds — CL-7255). + // A principals page arktype will reject, with the secret in a union + // field so the parse summary echoes it. `not.toContain` is otherwise a + // tautology — a type-mismatch summary is `was string` and never + // mentions the value (CL-7255). const SECRET = "sk-or-v1-thisisafakesecretvalue"; const logs: string[] = []; const hub = new Hono(); hub.get("/api/me/principals", (c) => - c.json({ data: SECRET, nextCursor: null }), + c.json({ + data: [ + { + principalId: "prn_1", + tenantId: "ten_1", + tenantName: "user_1's workbench", + tenantSlug: "user-1-user1", + kind: SECRET, + status: "active", + roles: [], + }, + ], + nextCursor: null, + }), ); const server = Bun.serve({ port: 0, fetch: hub.fetch }); try { @@ -287,7 +300,10 @@ describe("recentlyConnectedCredential reports through reportError and still find // reportError is the one place the unredacted cause may travel — // error-sink redacts before anything reaches a log sink. expect(cause).toBeInstanceOf(Error); - expect(logs.join("\n")).not.toContain(SECRET); + const causeMessage = + cause instanceof Error ? cause.message : String(cause); + expect(causeMessage).toContain(SECRET); + expect(JSON.stringify({ logs, context })).not.toContain(SECRET); } finally { server.stop(true); }