Skip to content

fix(update): normalize short Windows home paths for Scoop npm - #6494

Merged
lidge-jun merged 1 commit into
devfrom
codex/train-windows-final-fixes
Oct 3, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/train-windows-final-fixes

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

Summary

Fix final Windows CI's Scoop npm lookup failure when USERPROFILE uses an 8.3 alias. The npm candidate and entry were physical paths, but the allowed persist-bin path retained the short home spelling and rejected the same directory.

Canonicalize only the configured home before appending the fixed Scoop persist path. Do not resolve the full persist path into an arbitrary redirected target; existing containment, cwd and fail-closed checks stay intact. This repairs Windows4 in final run37107354988; startup/Nous fixture findings are separate follow-ups.

Verification

  • Reproduced the exact existing test failure on real Windows with TEMP set to an actual 8.3 alias: 14 passed,1 failed before repair.
  • Patched existing Windows suite: 17 passed,0 failed. Separate real-alias probe:6/6. Native runtime Bun1.3.14; hosted Bun1.4.0 confirmation pending.
  • Injected short/long home cases cover both nodejs variants; cwd/redirect negatives remain rejected. Deliberately resolving the entire persist-bin path fails regression tests.
  • Independent security review PASS. Root typecheck, privacy, structure, file-size and diff checks passed. No full local suites, installs, live config or credential changes.
  • Required PR CI pending; final comprehensive dev lane=all will run after all Windows follow-ups land.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Maintainer integration

Owner lidge-jun elects integration into dev at 2b01f50f1f14ab06b87f8445122d38eca3b7fd8f. Exact-head pull_request CI 37109086281, attempt1, passed all requested jobs; independent security/native regression proof is recorded above. Final all-platform dispatch remains pending after the test-lifecycle follow-up. This is maintainer integration, not self-approval.

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 3, 2026 08:15
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-03T08:18:22.667525Z 2b01f50 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added the bug Something isn't working label Oct 3, 2026
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 595b4d93-dd36-4180-a883-919932cab82d
📥 Commits

Reviewing files that changed from the base of the PR and between b254139 and 2b01f50.

📒 Files selected for processing (3)
  • src/update/npm-invocation.mjs
  • structure/ops/service-and-sidecars.md
  • tests/update/update-npm-invocation.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The Scoop Node.js npm-invocation check now derives its persistent-bin path from the resolved USERPROFILE. Windows tests cover short-name and physical home paths, canonical layouts, and redirected or unresolved paths.

Changes

Scoop path validation

Layer / File(s) Summary
Resolve home and verify Scoop paths
src/update/npm-invocation.mjs, tests/update/update-npm-invocation.test.ts, structure/ops/service-and-sidecars.md
The check uses the resolved USERPROFILE to derive the persistent-bin path. Tests cover canonical layouts and redirected or unresolved paths for nodejs and nodejs-lts. Documentation describes the physical-home resolution and suffix rule.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 2b01f

The Scoop npm lookup fix is ready to merge after normal checks; no actionable issue remains in the reviewed change.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 2b01f

The change narrowly repairs Windows home-path alias handling while preserving executable containment, launch-directory exclusion, and rejection on resolution failure. No new caller, privilege path, or security bypass was identified. Runtime confirmation remains incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure is executable selection for Windows updater and npm cache-preflight operations under their existing process authority. The supplied same-name references in log guarding and automation persistence do not establish broader reachability.

Security Findings and Attack Paths

  • inferred — Redirecting the admitted Scoop bin or npm candidate outside its authorized physical location remains blocked by the inspected predicate. Resolving only the home does not replace the fixed persist suffix with the redirect destination. This supports rejecting a PR-introduced redirect-bypass concern within the reviewed scope.

Trust Boundaries and Controls

  • observed — The changed trust decision recognizes physical home identity while retaining logical entry identity and physical containment checks. Resolution exceptions return false, and downstream consumers reject a missing invocation before spawning.

Resilience and Maintainability Implications

  • observed — The added regression cases distinguish home-alias normalization from unsafe normalization of the complete persist path, helping preserve the executable-trust boundary against future control drift.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: normalizing short Windows home paths to fix Scoop npm lookup.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lidge-jun
lidge-jun merged commit 4b98328 into dev Oct 3, 2026
38 checks passed
@lidge-jun
lidge-jun deleted the codex/train-windows-final-fixes branch October 3, 2026 08:25
This was referenced Oct 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant