fix(bootstrap): gate npm registry config on corp gcloud account#321
Open
evansenter wants to merge 1 commit into
Open
fix(bootstrap): gate npm registry config on corp gcloud account#321evansenter wants to merge 1 commit into
evansenter wants to merge 1 commit into
Conversation
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>
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On
./bootstrap.sh -p,configure_npm_registryrangcloud artifacts print-settings npmagainst the internalah-3p-staging-npmArtifact Registry repo for any active gcloud account. On a personal account (e.g.gmail.com) that call returnsPERMISSION_DENIED, so the function fell into itselsebranch and printed:every single
-prun. It's harmless — public npm installs already pass--registry https://registry.npmjs.org/explicitly and no~/.npmrcentry is needed — but it's noise on personal machines.Fix
Add a
CORP_NPM_DOMAIN="@anthropic.com"constant and skipconfigure_npm_registrysilently 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.shthat load the real constant + function frombootstrap.sh(viased/grep, same pattern as the existingis_gateway_hosttests), stubgcloud/npmonPATH, and assert via a marker file:npm registry skips personal gcloud account—gmail.comaccount never reachesprint-settings, never writes~/.npmrcnpm registry runs for corp gcloud account—@anthropic.comaccount does bothMutation-verified: removing the gate flips the personal-account test to red.
make checkgreen (lint, 37 bootstrap tests, hooks).🤖 Generated with Claude Code