chore(s3-datastore, gcs-datastore): address PR #120 review comments#121
chore(s3-datastore, gcs-datastore): address PR #120 review comments#121
Conversation
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) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Overview
Five targeted follow-ups to PR #120: duplicate JSDoc collapse, stale comment correction, two JSDoc clarifications, a shell-quoting correctness fix for AWS profiles with spaces, and matching test updates. No behavior change except the profile quoting.
Blocking Issues
None.
Suggestions
-
Profile name double-quote injection (cosmetic): In
formatAwsCredentialHint, the profile name is embedded directly into the hint string with double quotes:--profile "${awsProfile}". If a profile name ever contains a double-quote character (theoretically possible in an INI file), the copy-pasted command would have unbalanced quotes. This is a display-only message (never executed), and AWS config profiles with embedded double quotes are vanishingly rare in practice, so this does not block merge — just worth noting if the hint is ever made executable. -
Quoting inconsistency between error paths: The
credentials-rejectedbranch at line 200 wraps the profile name in single quotes ('${awsProfile}'), while thesession-expiredbranch now uses double quotes. Both are user-facing prose, so this is a minor style inconsistency rather than a correctness issue.
Everything else looks solid:
- Duplicate JSDoc collapse is correct — the second block was the live one.
- The stale
gcs_client_test.tssection comment now accurately describes what's actually there (mock server for status codes, directtokenRefreshErrordriving for theinvalid_grantpath). - The
classifyGcpCredentialErrorJSDoc addition correctly explains the short-circuit intokenRefreshErrorand whencauseNameis relevant. - The
tokenFromMetadataServerplain-Error comment accurately captures the design intent (different remediation path, not an oversight). - Test updates are correct: existing assertion updated, new multi-word-profile test added, integration test updated to assert the new quoted shape.
- No
model/files touched, no live cloud service dependencies, env vars are not at risk, no new imports.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None found.
Medium
datastore/s3/extensions/datastores/_lib/s3_client.ts:195— profile names containing double quotes produce broken copy-paste commands. The fix wraps the profile name in double quotes ("${awsProfile}"), which handles spaces. But if a profile name contains a literal double quote (e.g.,my"profile), the resulting commandaws sso login --profile "my"profile"is invalid POSIX shell. Thecredentials-rejectedpath on line 200 has the same class of issue with its single-quote wrapping. Mitigated by: AWS profile names come from the user's own~/.aws/configand double quotes in profile names are vanishingly rare (and arguably invalid AWS config). The output is a display string, not executed code, so this is a cosmetic issue, not a security one.
Low
-
datastore/gcs/manifest.yaml:4— version bump for comment-only changes. The GCS manifest was bumped from2026.05.04.2to2026.05.04.3, but all GCS changes are JSDoc/comment rewrites with zero behavioral change. Per the project's CI rules, this will trigger a publish of a functionally identical package. Not harmful, but unnecessary churn in the registry. -
datastore/s3/extensions/datastores/_lib/s3_client.ts:197— nested quoting in hint message relies on user extracting the right substring. The full hint readsRun 'aws sso login --profile "my dev profile"' to refresh— the user must extract only the inner command, excluding the prose single quotes. This works because double quotes inside single quotes are literal in POSIX shell, but could confuse a user who copies the outer single quotes as part of the command. This is pre-existing behavior, not introduced by this PR.
Verdict
PASS — The substantive code change (profile quoting) is correct for its stated goal and the edge case (double quotes in profile names) is theoretical enough to not block. The GCS changes are purely documentation. Tests cover the new behavior adequately. Ship it.
…ion hint (#228) (#126) Follow-up to swamp-club#226 (PR #120/#121 covered s3-datastore and gcs-datastore). The aws-sm vault wraps the AWS SDK and surfaced the same CredentialsProviderError discoverability issue when SSO sessions expire mid-secret-fetch. Adds vault/aws-sm/extensions/vaults/aws_sm_errors.ts containing: - AwsSmOperationError: error wrapper that preserves the SDK error's `name` (so error.name === "ResourceNotFoundException" still works) and exposes httpStatusCode, code, and requestId. - classifyAwsCredentialError, deriveAwsErrorCode, formatAwsCredentialHint: pure helpers mirroring the s3 pattern. - wrapAwsSmError: front-loads "Vault session expired:" or "Vault credentials rejected by AWS:" with `aws sso login --profile <P>` pointing at the next action. aws_sm.ts wraps every .send() call and routes errors through wrapAwsSmError. The put() create-on-missing fallback migrates from `error instanceof ResourceNotFoundException` to `error.name === "ResourceNotFoundException"`, which is functionally equivalent and removes the SDK class import. Empirical grounding (probe at @aws-sdk/client-secrets-manager@3.1024.0, 2026-05-06): - Pre-flight credential failure surfaces as `name === "CredentialsProviderError"` directly on the outer error with no cause chain. The cause-walk + leading-underscore-strip in deriveAwsErrorCode remains as defensive coverage for minified SDK builds and the older #226 stack-trace pattern. - HTTP 400 with a body lacking `__type` produces err.name="Unknown" and err.message="UnknownError". wrapAwsSmError mirrors the s3 wrapper's `code !== "Unknown"` and `message !== "UnknownError"` filters so the wrapped output stays clean. Both shapes are covered by direct wrapAwsSmError tests in aws_sm_errors_test.ts (24 tests). aws_sm_test.ts adds 6 behavioural tests covering all four operations, the put create-on-missing regression guard for the instanceof → name migration, and the malformed-body defense. Bumps the inline SDK pin in aws_sm.ts from @3.1010.0 → @3.1024.0 so the new aws_sm_errors.ts and aws_sm.ts agree (matches deno.json's import map). Manifest: 2026.04.22.2 → 2026.05.06.1. Refs: swamp-club#228, swamp-club#226 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Five small follow-ups from the post-merge review of #120 (swamp-club#226). All cosmetic cleanup or one-line correctness, no behavior change beyond the AWS profile quoting.
tokenRefreshError— collapse into a single block. The first block was dead documentation (parser only attached the second).gcs_client_test.ts— it described an OAuth-mocking design that was never implemented; rewritten to match what's actually there.classifyGcpCredentialErrorJSDoc — note thatcauseNameis for callers re-classifying caught errors.send()never reaches that branch becausetokenRefreshErrorshort-circuits the session-expired path at construction time.formatAwsCredentialHintprofile quoting — wrap the profile name in double quotes inside the single-quoted command, soaws sso login --profile "my dev profile"stays valid for profiles containing spaces (uncommon but legal in AWS config). Added a multi-word-profile unit test.tokenFromMetadataServerplainError— add a comment explaining why this path intentionally does NOT go throughtokenRefreshError. The metadata-server failure remediation differs (check instance SA and IAM scopes, not rungcloud auth), so stampingname = "CredentialsProviderError"here would mislead the classifier.Manifest bumps:
2026.05.04.2→2026.05.04.3for both@swamp/s3-datastoreand@swamp/gcs-datastore.Test plan
formatAwsCredentialHintto assert double-quoted profile."my dev profile") — confirms valid POSIX shell.CredentialsProviderErrorintegration test to assert the new quoted shape.deno check / lint / fmt --check / test / install --frozenclean for both extensions.🤖 Generated with Claude Code