From 120db3dbfa438ce169ebc98999a5a798a4e9c192 Mon Sep 17 00:00:00 2001 From: stack72 Date: Mon, 4 May 2026 19:36:59 +0100 Subject: [PATCH] chore(s3-datastore, gcs-datastore): address PR #120 review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five small follow-ups from the post-merge review of #120: 1. gcs_client.ts:tokenRefreshError — collapse the duplicate JSDoc block left over from the test-export commit (parser only attached the second one; the first was dead documentation). 2. gcs_client_test.ts — rewrite the stale "Wrapper integration tests" section comment that described an OAuth-mocking design that was never implemented (we export tokenRefreshError and unit-test it directly instead). 3. classifyGcpCredentialError — add a JSDoc note that the causeName parameter is for callers re-classifying a caught error. send() never reaches the cause-name branch because tokenRefreshError short- circuits the session-expired path at construction time. 4. s3_client.ts:formatAwsCredentialHint — wrap the profile name in double quotes inside the single-quoted command, so the copy-pasted `aws sso login --profile "..."` stays valid for profiles that contain spaces (uncommon but legal in AWS config). Updated unit and integration tests to assert the new shape; added a multi-word- profile unit test. 5. tokenFromMetadataServer — add a comment explaining why this path intentionally throws plain Error rather than going through tokenRefreshError. The metadata-server failure remediation differs from the user-credential / service-account paths (check instance SA and IAM scopes, not run gcloud auth), so stamping `name = "CredentialsProviderError"` here would mislead the classifier into prepending the wrong hint. Manifest bumps: 2026.05.04.2 → 2026.05.04.3 for both extensions. Refs: swamp-club#226, #120 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../extensions/datastores/_lib/gcs_client.ts | 31 +++++++++++++------ .../datastores/_lib/gcs_client_test.ts | 11 ++++--- datastore/gcs/manifest.yaml | 2 +- .../extensions/datastores/_lib/s3_client.ts | 5 ++- .../datastores/_lib/s3_client_test.ts | 25 ++++++++++++--- datastore/s3/manifest.yaml | 2 +- 6 files changed, 53 insertions(+), 23 deletions(-) diff --git a/datastore/gcs/extensions/datastores/_lib/gcs_client.ts b/datastore/gcs/extensions/datastores/_lib/gcs_client.ts index 6ea4b8a5..7aad2bdd 100644 --- a/datastore/gcs/extensions/datastores/_lib/gcs_client.ts +++ b/datastore/gcs/extensions/datastores/_lib/gcs_client.ts @@ -159,6 +159,12 @@ export type GcpCredentialErrorKind = * Detection is HTTP-status-only for the credentials-rejected branch — body * shape varies (JSON vs HTML vs plain text on edge cases) and is not * reliable for classification. + * + * The `causeName` parameter is intended for callers that need to + * re-classify a caught `GcsOperationError` (e.g. by passing in `err.name`). + * `send()` itself never reaches the cause-name branch from this function — + * the session-expired path for token-refresh failures is short-circuited + * inside `tokenRefreshError`, which embeds the hint at construction time. */ export function classifyGcpCredentialError( causeName: string | undefined, @@ -278,16 +284,15 @@ async function createSignedJwt( /** * Wrap a token-endpoint failure as a `GcsOperationError`. When the body * indicates `invalid_grant` (refresh token revoked, expired, or never valid), - * stamp `name = "CredentialsProviderError"` so the downstream classifier in - * `send()` recognises the SSO-equivalent path. Other failures keep a - * generic name; the message preserves status + body for debugging. - */ -/** - * Wrap a token-endpoint failure as a GcsOperationError. Exported for tests - * because mocking `oauth2.googleapis.com/token` from outside the module - * isn't feasible (the URL is hardcoded in `tokenFromUserCredentials`). - * Exporting this lets us prove end-to-end that an `invalid_grant` body - * produces the swamp-flavoured "session expired" message. + * stamp `name = "CredentialsProviderError"` and front-load the + * swamp-flavoured "session expired" hint so callers see the cause and + * remediation before the raw token-endpoint response. Other failures keep + * a generic `TokenRefreshError` name; the message preserves status + body + * for debugging. + * + * Exported so tests can drive it directly — mocking the hardcoded + * `oauth2.googleapis.com/token` URL inside `tokenFromUserCredentials` from + * outside the module isn't feasible. */ export function tokenRefreshError( context: string, @@ -373,6 +378,12 @@ async function tokenFromMetadataServer(): Promise { { headers: { "Metadata-Flavor": "Google" } }, ); if (!resp.ok) { + // Intentionally a plain Error rather than the typed `tokenRefreshError` + // path used by the user-credential and service-account paths. The + // remediation differs (check the instance's attached service account + // and IAM scopes, not `gcloud auth application-default login`), so + // stamping `name = "CredentialsProviderError"` here would mislead + // `classifyGcpCredentialError` into prepending the wrong hint. throw new Error( `Metadata server token request failed: ${resp.status} ${await resp .text()}`, diff --git a/datastore/gcs/extensions/datastores/_lib/gcs_client_test.ts b/datastore/gcs/extensions/datastores/_lib/gcs_client_test.ts index df551e68..8f6b1739 100644 --- a/datastore/gcs/extensions/datastores/_lib/gcs_client_test.ts +++ b/datastore/gcs/extensions/datastores/_lib/gcs_client_test.ts @@ -462,11 +462,12 @@ Deno.test("formatGcpCredentialHint: other → undefined", () => { // --- Wrapper integration tests ------------------------------------------- // -// The token-refresh path is exercised by writing a temporary -// `authorized_user` credentials file, pointing the OAuth token endpoint at -// our mock server, and intercepting the refresh request. `GOOGLE_APPLICATION_CREDENTIALS` -// is restored in `finally`. `clearTokenCache()` runs in setup AND teardown -// because the module-level cache leaks across tests otherwise. +// `send()` is exercised against a mock server returning the relevant +// status codes. `clearTokenCache()` runs in setup AND teardown because +// the module-level token cache leaks across tests otherwise. The +// token-refresh `invalid_grant` path is covered separately below by +// driving `tokenRefreshError` directly — see the comment on its export +// for why we don't mock the OAuth endpoint. Deno.test({ sanitizeResources: false, diff --git a/datastore/gcs/manifest.yaml b/datastore/gcs/manifest.yaml index 37cd35de..fc651409 100644 --- a/datastore/gcs/manifest.yaml +++ b/datastore/gcs/manifest.yaml @@ -1,6 +1,6 @@ manifestVersion: 1 name: "@swamp/gcs-datastore" -version: "2026.05.04.2" +version: "2026.05.04.3" description: | Store data in a Google Cloud Storage bucket with local cache synchronization. Provides distributed locking via GCS generation-based preconditions and diff --git a/datastore/s3/extensions/datastores/_lib/s3_client.ts b/datastore/s3/extensions/datastores/_lib/s3_client.ts index 31ed3929..276a89ff 100644 --- a/datastore/s3/extensions/datastores/_lib/s3_client.ts +++ b/datastore/s3/extensions/datastores/_lib/s3_client.ts @@ -188,8 +188,11 @@ export function formatAwsCredentialHint( awsProfile: string | undefined, ): string | undefined { if (kind === "session-expired") { + // Wrap the profile name in double quotes inside the single-quoted + // command so the copy-pasted shell command stays valid for profiles + // that contain spaces (uncommon but legal in AWS config). const cmd = awsProfile - ? `aws sso login --profile ${awsProfile}` + ? `aws sso login --profile "${awsProfile}"` : `aws sso login`; return `Datastore session expired: your AWS profile's SSO session is no longer valid. Run '${cmd}' to refresh, then retry.`; } diff --git a/datastore/s3/extensions/datastores/_lib/s3_client_test.ts b/datastore/s3/extensions/datastores/_lib/s3_client_test.ts index 3bc1703f..c349b223 100644 --- a/datastore/s3/extensions/datastores/_lib/s3_client_test.ts +++ b/datastore/s3/extensions/datastores/_lib/s3_client_test.ts @@ -400,16 +400,29 @@ Deno.test("classifyAwsCredentialError: undefined code + 403 → other", () => { ); }); -Deno.test("formatAwsCredentialHint: session-expired with profile renders --profile flag", () => { +Deno.test("formatAwsCredentialHint: session-expired with profile renders quoted --profile flag", () => { const hint = formatAwsCredentialHint("session-expired", "demo"); assert(hint !== undefined); + // Profile is double-quoted so spaces in profile names don't break the + // copy-pasted command. Outer single quotes wrap the whole command in prose. assert( - hint.includes("aws sso login --profile demo"), - `expected --profile in hint, got: ${hint}`, + hint.includes(`aws sso login --profile "demo"`), + `expected double-quoted --profile in hint, got: ${hint}`, ); assert(hint.startsWith("Datastore session expired")); }); +Deno.test("formatAwsCredentialHint: session-expired with multi-word profile stays valid shell", () => { + const hint = formatAwsCredentialHint("session-expired", "my dev profile"); + assert(hint !== undefined); + // The full command appears as `Run 'aws sso login --profile "my dev profile"' to refresh` + // — single-quoted outer, double-quoted profile, valid POSIX shell. + assert( + hint.includes(`aws sso login --profile "my dev profile"`), + `expected multi-word profile to be double-quoted, got: ${hint}`, + ); +}); + Deno.test("formatAwsCredentialHint: session-expired without profile renders generic command", () => { const hint = formatAwsCredentialHint("session-expired", undefined); assert(hint !== undefined); @@ -743,8 +756,10 @@ Deno.test({ `expected aws sso login remediation, got: ${err.message}`, ); assert( - err.message.includes("--profile swamp-issue-226-nonexistent-profile"), - `expected --profile flag with the configured profile, got: ${err.message}`, + err.message.includes( + `--profile "swamp-issue-226-nonexistent-profile"`, + ), + `expected double-quoted --profile flag with the configured profile, got: ${err.message}`, ); // .name preserved so existing `error.name === "CredentialsProviderError"` // checks at call sites keep working. diff --git a/datastore/s3/manifest.yaml b/datastore/s3/manifest.yaml index 0fe5a67d..f84c3e61 100644 --- a/datastore/s3/manifest.yaml +++ b/datastore/s3/manifest.yaml @@ -1,6 +1,6 @@ manifestVersion: 1 name: "@swamp/s3-datastore" -version: "2026.05.04.2" +version: "2026.05.04.3" description: | Store data in an Amazon S3 bucket with local cache synchronization. Provides distributed locking via S3 conditional writes and bidirectional