fix(release): fully sanitize npm lookup diagnostics - #3321
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens the release preflight diagnostics in scripts/ci/publish-npm-packages.sh by further sanitizing npm registry lookup failures (credentials and absolute paths), and updates the associated regression tests to the project’s BDD test API with a scripts lockfile update to keep --frozen compatibility.
Changes:
- Expand npm lookup stderr sanitization to redact full Bearer credentials and broader absolute path formats (POSIX, Windows drive, UNC) while avoiding registry URL corruption.
- Migrate the publish preflight regression suite to
#veryfront/testing/bdd.tsand add assertions for the new sanitization behavior. - Update
scripts/deno.lockto include the additional dependency required by the BDD import graph under--frozen.
Verification:
- Not run as part of this review. PR description reports:
bash -n scripts/ci/publish-npm-packages.sh- focused test: 1 group / 5 steps passed with
--frozen deno check --config=scripts/test.deno.json --frozen scripts/ci/publish-npm-packages.test.tsdeno task verify:quick
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/ci/publish-npm-packages.sh | Strengthens npm lookup stderr sanitization for release preflight failures. |
| scripts/ci/publish-npm-packages.test.ts | Converts to BDD tests and adds coverage for new sanitization expectations. |
| scripts/deno.lock | Adds a missing frozen-lock entry needed by the updated test import graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Absolute-path redaction previously consumed closing double quotes because each path pattern matched every non-whitespace character. The sanitizer now stops at quote delimiters and regression coverage exercises quoted POSIX, Windows-drive, and UNC paths. Constraint: Registry diagnostics must redact machine-local paths without corrupting the surrounding npm message. Rejected: Rebalance quotes after sanitization | delimiter-aware matching is smaller and preserves the original syntax. Confidence: high Scope-risk: narrow Reversibility: clean Tested: focused release-script BDD suite, format, lint, typecheck, Bash syntax, and git diff check Not-tested: full repository suite
npm failures can report absolute paths inside quotes, brackets, and file URIs. Sanitize these structured forms before the existing unquoted fallback so complete paths are removed without consuming their delimiters or altering registry URLs. Constraint: The release helper must remain portable across the Bash and sed implementations used locally and in GitHub Actions. Rejected: Replace sed with a new parser dependency | unnecessary dependency and release-path complexity. Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep structured path rules ahead of the unquoted fallback so paths containing spaces are redacted as one value. Tested: Focused 5-step BDD suite, bash -n, Deno fmt/lint/check, deno task verify:quick, git diff --check. Not-tested: Live npm diagnostic variants outside the covered POSIX, Windows, UNC, quoted, bracketed, and file URI forms.
|
Independent review found that npm diagnostics could still leak local paths when values used single quotes, brackets, spaces inside quotes, or Regression coverage now verifies POSIX and Windows-style paths, preserved closing quotes and brackets, Verification passed: focused BDD (5/5 steps), |
Npm registry diagnostics can wrap fallback token and path values in quotes, parentheses, or comma-delimited text. The sanitizer now stops fallback matches before those delimiters while preserving the existing redaction output shape for credentials and local paths. Constraint: Scoped to PR #3321 npm release diagnostic sanitizer follow-up. Rejected: Rewrite sanitizer away from sed | too broad for a release-script follow-up. Confidence: high Scope-risk: narrow Directive: Keep fallback redaction patterns delimiter-aware when adding new token or path forms. Tested: Focused regression showed RED before the sanitizer change, then passed: deno test --config=scripts/test.deno.json --frozen --allow-all scripts/ci/publish-npm-packages.test.ts. Tested: bash -n scripts/ci/publish-npm-packages.sh; deno fmt --check --config=scripts/test.deno.json scripts/ci/publish-npm-packages.test.ts; deno lint --config=scripts/test.deno.json scripts/ci/publish-npm-packages.test.ts; deno check --config=scripts/test.deno.json --frozen scripts/ci/publish-npm-packages.test.ts; git diff --check. Tested: deno task verify:quick. Not-tested: ./scripts/hooks/pre-push is red before E2E execution because it references missing tests/e2e/playwright.config.ts; deno task test:e2e:playwright is red from mixed Playwright 1.60.0 and 1.59.0 loads; full deno test was interrupted after unrelated hosted/tool/cache network timeout failures.
|
Follow-up pushed in Addressed the remaining delimiter leak in Regression coverage in Verification:
Broader gates not green for unrelated existing/tooling issues:
No merge queued. |
Codex exact-head review and merge confidenceReviewed SHA: Findings: none. Evidence:
Local verification on the reviewed head:
Merge confidence: 95%. Reasoning: the change is narrow, release-script only, covered by delimiter-specific regressions and full exact-head CI, and directly addresses the previously confirmed diagnostic redaction gap. Residual risk is limited to unobserved npm diagnostic formatting variants outside the covered quoted, bracketed, parenthesized, comma-delimited, POSIX, Windows, UNC, file URI, Bearer, query-token, and Review-Gate: |
Summary
Verification
bash -n scripts/ci/publish-npm-packages.sh--frozendeno check --config=scripts/test.deno.json --frozen scripts/ci/publish-npm-packages.test.tsdeno task verify:quickFollow-up to #3317, which merged while its two review threads were being addressed.