Skip to content

fix(bootstrap): gate npm registry config on corp gcloud account#321

Open
evansenter wants to merge 1 commit into
mainfrom
fix/npm-registry-corp-gate
Open

fix(bootstrap): gate npm registry config on corp gcloud account#321
evansenter wants to merge 1 commit into
mainfrom
fix/npm-registry-corp-gate

Conversation

@evansenter

Copy link
Copy Markdown
Owner

Problem

On ./bootstrap.sh -p, configure_npm_registry ran gcloud artifacts print-settings npm against the internal ah-3p-staging-npm Artifact Registry repo for any active gcloud account. On a personal account (e.g. gmail.com) that call returns PERMISSION_DENIED, so the function fell into its else branch and printed:

  Warning: Failed to configure NPM credentials automatically

every single -p run. It's harmless — public npm installs already pass --registry https://registry.npmjs.org/ explicitly and no ~/.npmrc entry is needed — but it's noise on personal machines.

Fix

Add a CORP_NPM_DOMAIN="@anthropic.com" constant and skip configure_npm_registry silently unless the active gcloud account ends in that domain. Corp machines behave exactly as before; personal machines no-op cleanly.

Tests

Two new behavioral tests in tests/test-bootstrap.sh that load the real constant + function from bootstrap.sh (via sed/grep, same pattern as the existing is_gateway_host tests), stub gcloud/npm on PATH, and assert via a marker file:

  • npm registry skips personal gcloud accountgmail.com account never reaches print-settings, never writes ~/.npmrc
  • npm registry runs for corp gcloud account@anthropic.com account does both

Mutation-verified: removing the gate flips the personal-account test to red. make check green (lint, 37 bootstrap tests, hooks).

🤖 Generated with Claude Code

configure_npm_registry tried `gcloud artifacts print-settings npm` against
the internal ah-3p-staging-npm repo for any active gcloud account. On a
personal account (e.g. gmail.com) that call PERMISSION_DENIEDs, so every
`./bootstrap.sh -p` printed "Failed to configure NPM credentials
automatically" — a useless warning since public npm uses the explicit
registry.npmjs.org and no ~/.npmrc entry is needed.

Skip the step silently unless the active account ends in $CORP_NPM_DOMAIN
(@anthropic.com). Adds two mutation-verified behavioral tests that load the
real constant + function and assert print-settings is reached (and ~/.npmrc
written) only for a corp account.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prompt: claude-review.md

Code Review — Summary

Gates configure_npm_registry on the active gcloud account ending in @anthropic.com, silencing the spurious PERMISSION_DENIED warning on personal machines. The fix is minimal, correctly placed, and covered by two behavioral tests that load the real constant + function from bootstrap.sh (via sed/grep, same pattern as the is_gateway_host tests).

Notes

  • Gate logic is correct: the leading @ in the constant anchors the suffix match, so lookalike domains (e.g. foo@evil-anthropic.com) cannot falsely match — only genuine @anthropic.com accounts proceed.
  • Placement is right: the gate sits after the command-v / active-account checks and before the print-settings call, so personal accounts no-op silently as intended (no warning).
  • Tests cover both branches; the corp path also verifies ~/.npmrc is written, and the env assignments are command-scoped (no PATH/HOME leakage). The sed extraction correctly relies on the function's closing brace at column 0.

Verdict: APPROVE — Clean, well-tested fix with no Critical/Important/Suggestion findings.

(Note: posted via gh pr review because the gh api inline-review path was blocked by the sandbox; this review has no inline comments, so no information is lost.)

Automated review by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant