fix: fail CLI releases early on npm token mismatch - #1387
Conversation
|
Claude (@claude) review |
|
Greptile (@greptileai) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 2m 6s —— View job Code Review
Overall this is a clean, well-structured preflight. The injection of Medium-priority
Node 20 ships with npm v10, which restructured the Consider testing Low-priorityMissing test for auth failure ( The four tests cover non-owner, empty owner list, and read-only access, but there's no test for the case where Silently defaulting to
Nitpicks
The
Things that look good
|
✅ Tests passed — 1423/1427
|
Greptile SummaryThis PR adds an early npm access check for CLI releases. The main changes are:
Confidence Score: 4/5The npm preflight can block valid CLI releases until its real npm output handling is tightened.
packages/browseros-agent/scripts/build/cli/npm-access.ts Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/scripts/build/cli/npm-access.ts:108-110
**Collaborator Access Shape Mismatch**
The release workflow now depends on this parser accepting the real `npm access list collaborators ... --json` output from the Node 20 npm CLI. If npm returns collaborator records as an array, or omits package owners from the collaborator ACL after `owner ls` already proved ownership, this lookup returns an empty access string and rejects a valid publish token before the release can continue.
### Issue 2 of 2
packages/browseros-agent/scripts/build/cli/npm-access.ts:141-142
**Stdout Diagnostics Are Dropped**
When an npm command exits non-zero and writes its structured error to stdout, this helper ignores that output and falls back to the generic process error message. A failed preflight can then hide the registry response that explains why the release was blocked.
```suggestion
const stderr = 'stderr' in error ? String(error.stderr).trim() : ''
const stdout = 'stdout' in error ? String(error.stdout).trim() : ''
return stderr || stdout || error.message
```
Reviews (1): Last reviewed commit: "fix: verify cli npm collaborator access" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const collaborators = parsed as Record<string, unknown> | ||
| const access = collaborators[user] ?? collaborators[user.toLowerCase()] | ||
| return typeof access === 'string' ? access : '' |
There was a problem hiding this comment.
The release workflow now depends on this parser accepting the real npm access list collaborators ... --json output from the Node 20 npm CLI. If npm returns collaborator records as an array, or omits package owners from the collaborator ACL after owner ls already proved ownership, this lookup returns an empty access string and rejects a valid publish token before the release can continue.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/scripts/build/cli/npm-access.ts
Line: 108-110
Comment:
**Collaborator Access Shape Mismatch**
The release workflow now depends on this parser accepting the real `npm access list collaborators ... --json` output from the Node 20 npm CLI. If npm returns collaborator records as an array, or omits package owners from the collaborator ACL after `owner ls` already proved ownership, this lookup returns an empty access string and rejects a valid publish token before the release can continue.
How can I resolve this? If you propose a fix, please make it concise.| const stderr = 'stderr' in error ? String(error.stderr).trim() : '' | ||
| return stderr || error.message |
There was a problem hiding this comment.
When an npm command exits non-zero and writes its structured error to stdout, this helper ignores that output and falls back to the generic process error message. A failed preflight can then hide the registry response that explains why the release was blocked.
| const stderr = 'stderr' in error ? String(error.stderr).trim() : '' | |
| return stderr || error.message | |
| const stderr = 'stderr' in error ? String(error.stderr).trim() : '' | |
| const stdout = 'stdout' in error ? String(error.stdout).trim() : '' | |
| return stderr || stdout || error.message |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/scripts/build/cli/npm-access.ts
Line: 141-142
Comment:
**Stdout Diagnostics Are Dropped**
When an npm command exits non-zero and writes its structured error to stdout, this helper ignores that output and falls back to the generic process error message. A failed preflight can then hide the registry response that explains why the release was blocked.
```suggestion
const stderr = 'stderr' in error ? String(error.stderr).trim() : ''
const stdout = 'stdout' in error ? String(error.stdout).trim() : ''
return stderr || stdout || error.message
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Greptile SummaryThis PR moves npm credential checks earlier in the CLI release flow. The main changes are:
Confidence Score: 4/5The changed npm preflight can block valid CLI releases.
packages/browseros-agent/scripts/build/cli/npm-access.ts; packages/browseros-agent/apps/cli/README.md Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/scripts/build/cli/npm-access.ts:78
**Username Filter Hides Access**
When the release workflow runs this preflight with a valid owner token, the optional `<user>` argument can make `npm access list collaborators` return entries keyed by teams or omit the individual owner entry. `parseCollaboratorAccess` then looks only for `collaborators[user]`, treats the missing key as no access, and aborts the release before any assets are uploaded.
### Issue 2 of 2
packages/browseros-agent/apps/cli/README.md:94
**Read-Write Requirement Missing**
This release note says the secret only needs to authenticate as an npm owner, but the new preflight also rejects tokens without `read-write` collaborator access. A release engineer can follow the README, rotate the secret to an owner account that fails the collaborator-access check, and hit an unexpected release failure.
```suggestion
The `NPM_TOKEN` release secret must authenticate as an npm owner of `browseros-cli` with read-write package access; the workflow checks this before uploading CDN assets or creating the GitHub release.
```
Reviews (2): Last reviewed commit: "fix: verify cli npm collaborator access" | Re-trigger Greptile |
| 'list', | ||
| 'collaborators', | ||
| packageName, | ||
| user, |
There was a problem hiding this comment.
When the release workflow runs this preflight with a valid owner token, the optional <user> argument can make npm access list collaborators return entries keyed by teams or omit the individual owner entry. parseCollaboratorAccess then looks only for collaborators[user], treats the missing key as no access, and aborts the release before any assets are uploaded.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/scripts/build/cli/npm-access.ts
Line: 78
Comment:
**Username Filter Hides Access**
When the release workflow runs this preflight with a valid owner token, the optional `<user>` argument can make `npm access list collaborators` return entries keyed by teams or omit the individual owner entry. `parseCollaboratorAccess` then looks only for `collaborators[user]`, treats the missing key as no access, and aborts the release before any assets are uploaded.
How can I resolve this? If you propose a fix, please make it concise.
…, and close-done kept its own copy of the rule (#360) `tri why` warned ahead: all nine remaining accepted branches conflict, so nothing can land, close-done will close nothing, and the pipeline stops as soon as those boundaries are all that is left. Four of the nine were finished work. THE ROUTE NO COMPARISON OF BYTES CAN TAKE. `isLanded` had four routes - ancestry, an identical merged tree, a patch-id match, and a hand-applied change. Every one of them compares CONTENT. They are all blind to the thing this loop does constantly: when a bee's branch goes stale, I re-cut its change against the current base and squash-merge THAT. The carry is a new commit with a new tree and a new patch-id, so nothing content-shaped connects it back, and the bee's original branch becomes permanent debt - re-offered every round, conflicting every round, holding its boundary fenced for ever. So read the message. L1 of this repository is "no code merged without `Closes #N`", which makes the message a load-bearing record and not a courtesy: browseros-ai#1310 landed as PR #330 browseros-ai#1308 landed as PR #331 browseros-ai#1362 `Closes browseros-ai#1362` in a base commit browseros-ai#1421 carried by a commit that says "Carries the browseros-ai#1421 work it belongs with" Nine conflicting branches became six. The matching is done in JavaScript rather than in git's regex, because two rounds ago a BRE read as a JavaScript regex convicted a bee - the dialect belongs somewhere it is known. AND THE TIGHTENING BROKE THE CASE IT WAS WRITTEN FOR. I required `(#N)` to end the line, to reject "unlike (browseros-ai#1421), this does X". One minute later browseros-ai#1310 stopped being recognised: a squash subject here reads `feat(queen): explain idle paid slots (browseros-ai#1310) (#330)` - the issue first, then the pull request. A trailing CHAIN of references is a subject; a parenthesis in the middle of a sentence is not. CLOSE-DONE KEPT ITS OWN COPY OF THE RULE. It had the tree test and nothing else, for weeks, while `land.mjs` grew four more routes it never learned. A rule transcribed twice is two rules that agree until somebody edits one - which is L2 of this repository, and it had happened here in the file that decides whether an issue may be closed. It asks `land.mjs` now. What remains is real debt and is reported as such: browseros-ai#1387, browseros-ai#1302 and browseros-ai#1303 carry 880 insertions of finished work outside the base, all three with their issues already CLOSED - the inverse of the false statement close-done exists to prevent. A conflict is still reported for a person and never resolved by guessing. selftest 144 pass 0 fail. Co-authored-by: Dmitrii Vasilev <trackgmedernj@hotmail.com>
1 participant
Summary
browseros-clithat checks the token user and read-write collaborator access.make npm-version, and the npm preflight before CDN upload or GitHub release creation.NPM_TOKENownership requirement.Design
The release still publishes npm after GitHub release assets exist, so npm postinstall can fetch the matching
cli/vX.Y.Zbinaries. The new preflight catches bad npm credentials before those public release side effects.Test plan
bun test scripts/build/cli/npm-access.test.ts scripts/build/cli/release-policy.test.tsactionlint ../../.github/workflows/release-cli.ymlbun run check(exit 0; existing Claw warnings still printed)cd apps/cli && gofmt -l . && go vet ./... && go build ./... && go test ./...browseros-cli@0.3.1npm publish --dry-run --access publicNote
The existing failed
cli/v0.3.1tag run still needs a valid npm token withbrowseros-cliread-write access; this PR prevents future runs from discovering that only after partial release side effects.