test(cli): skip the serve-scope symlink test when the platform refuses the link - #1042
test(cli): skip the serve-scope symlink test when the platform refuses the link#1042Vasanthdev2004 wants to merge 1 commit into
Conversation
…s the link Creating a symlink on Windows needs SeCreateSymbolicLinkPrivilege, which an ordinary developer account does not hold, so this reported a failure with nothing wrong in the code: "A required privilege is not held by the client". CI runners are elevated, so it only ever appeared locally, where it cost a round of triage on #1003 before being ruled environmental and would do the same to any contributor on a stock Windows machine. The two other symlink tests in this package, and most of the rest of the repo, already answer this way. Swept the remaining os.Symlink-then-Fatal sites rather than assuming: internal/release passes, the one in internal/agent is never reached, and the unix-only files cannot hit it. This was the outlier. Closes #1021
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (1)
Limit details: You’ve used all 5 included reviews currently available. Your 19 included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughThe symlink setup in ChangesTest environment handling
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The lexical-path test now skips on systems that cannot create symlinks, avoiding false failures on unelevated Windows accounts without changing product behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Usage-based review receipt
Note This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing. Comment |
Greptile SummaryThis PR makes the lexical-path serve-scope test portable to environments where creating symbolic links is unavailable.
Confidence Score: 5/5The PR appears safe to merge. The change is confined to test behavior and consistently treats unavailable symlink creation as an unmet environmental prerequisite without modifying production code.
|
| Filename | Overview |
|---|---|
| internal/cli/serve_test.go | Updates one symlink-dependent test to skip when its environmental prerequisite is unavailable; no actionable defect was identified. |
Reviews (1): Last reviewed commit: "test(cli): skip the serve-scope symlink ..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
1 participant
Closes #1021.
os.Symlinkon Windows needsSeCreateSymbolicLinkPrivilege, which an ordinary developer account does not hold.TestBuildServeScopeKeepsLexicalPathscalled it and treated the refusal as a test failure, so on a stock Windows machine the package reported:with nothing wrong in the code. CI runners are elevated, so it never showed there. It cost a round of triage on #1003 before being ruled environmental, and it has been the only failure in
internal/clion my machine all week; the package is green with this.The fix is the answer the rest of the repo already gives: skip when the platform will not make the link. Both other symlink tests in this package do exactly that, as do the ones in
internal/agent,internal/pathjail,internal/plugins,internal/repomap,internal/sandboxand others.I swept the remaining
os.Symlink-then-t.Fatalsites rather than assuming this was the only one, and ran them:internal/releasepasses here.internal/agent/freeform_tool_test.gois never reached on this box.internal/peermsg/private_dir_unix_test.goandtransport_unix_test.goare unix-only files, where the call cannot fail this way.internal/installtestandinternal/npmwrapperbuild node package layouts rather than exercising link semantics.So this was the outlier, and the change stays to the one test.
A skip is the honest answer here specifically because the missing privilege is an environmental prerequisite the fixture does not control and cannot arrange. It is not a fixture-controlled condition being skipped past, which would hide a real failure. Restoring the
t.Fatalreproduces the failure above on an unelevated account.Summary by CodeRabbit