Skip to content

fix(review): bound Windows owner command duration - #1009

Open
danielgap wants to merge 1 commit into
Gentleman-Programming:mainfrom
danielgap:fix/990-windows-owner-timeout
Open

danielgap wants to merge 1 commit into
Gentleman-Programming:mainfrom
danielgap:fix/990-windows-owner-timeout

Conversation

@danielgap

@danielgap danielgap commented Sep 14, 2026

Copy link
Copy Markdown

Closes #990

Summary

  • apply one shared 30-second timeout to all five Windows owner and ACL process launches
  • surface sanitized timeout diagnostics without stderr, paths, encoded PowerShell payloads, or arbitrary error text
  • add selective regression coverage for every operation under both ETIMEDOUT and killed-process failures

Changes

File Change
lib/review-candidate-view-owner.ts Centralizes the Windows command timeout and converts timeout-shaped failures into a sanitized typed error.
lib/review-candidate-view.ts Preserves the safe timeout detail through candidate owner preparation diagnostics.
tests/review-candidate-view.test.ts Covers the five operations, both timeout shapes, owner-query preservation, and sensitive-data exclusion.

Test plan

  • Focused timeout matrix: 5 operations × 2 timeout shapes
  • node --experimental-strip-types --test tests/review-candidate-view.test.ts — 137 passed, 7 skipped, 0 failed
  • pnpm typecheck — 200 recorded diagnostics, no regressions
  • pnpm run check:runtime-modules
  • pnpm run check:provider-contract
  • pnpm run test:harness
  • git diff --check
  • Independent read-only verifier — PASS

Validation notes

  • pnpm test reached 2,353 passed and 0 failed, but exited with 10 cancellations in tests/rdd-status-line.test.ts (Promise resolution is still pending). The isolated file reproduces the same 7 passed / 10 cancelled result on a clean main checkout; this PR does not touch that file.
  • Native review could not execute because the reviewer host returned a quota-exhausted 429; zero reviewers were prepared or submitted. The high-risk fallback required writer self-verification plus an independent verifier, both completed.
  • Native Windows process termination and ACL effects remain delegated to Windows CI; local regression coverage uses the existing Linux mock boundary.

Contributor checklist

  • Linked an approved issue
  • Add exactly one type:* label — maintainer action required: type:bug (fork author lacks permission)
  • No shell scripts changed (shellcheck not applicable)
  • No skills changed (agent skill testing not applicable)
  • No user-facing documentation change required
  • Conventional commit format
  • No Co-Authored-By trailers

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of Windows ownership and access-control validation by applying a consistent 30-second command timeout.
    • Windows command timeouts now produce clear candidate-view errors instead of being treated as generic validation failures.
    • Failure messages now avoid exposing sensitive file paths, command output, error details, or encoded payloads.

Copilot AI lite review requested due to automatic review settings September 14, 2026 10:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 8a5bf9d9-690b-4f30-9b5f-21b4307de7af

📥 Commits

Reviewing files that changed from the base of the PR and between e847e97 and 0812a5a.

📒 Files selected for processing (3)
  • lib/review-candidate-view-owner.ts
  • lib/review-candidate-view.ts
  • tests/review-candidate-view.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The change centralizes Windows owner-command execution with a 30-second timeout and typed timeout errors. Candidate-view preparation now reports timeout details through CandidateViewError. Tests cover all five Windows owner operations and message sanitization.

Changes

Windows owner timeout handling

Layer / File(s) Summary
Centralized Windows owner command execution
lib/review-candidate-view-owner.ts
A shared helper resolves Windows executables, applies a 30-second timeout, and raises WindowsOwnerCommandTimeoutError for timed-out or killed processes. Owner and ACL operations use the helper.
Candidate-view timeout error propagation
lib/review-candidate-view.ts
Candidate-view owner preparation maps typed command timeouts to detailed CandidateViewError instances and preserves generic handling for other failures.
Timeout and message validation
tests/review-candidate-view.test.ts
Tests verify the 30-second timeout for all five Windows owner operations and confirm that failure messages exclude raw paths, stderr, and payload data.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: alan-thegentleman

Merge Risk: ⚪ Minimal · up to 0812a

The Windows owner-command timeout behavior is covered across all five operations with no confirmed unresolved issue.

🚥 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 11 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #990 requires longer or improved Windows owner-command timeout handling and a wrapped cause in CandidateViewError diagnostics. The PR routes whoami.exe, powershell.exe, and icacls.exe th…
Out of Scope Changes check ✅ Passed The changes stay within Issue #990. The implementation changes Windows owner and ACL process execution, timeout error handling, and candidate-owner diagnostics. The added regression test covers the re…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding bounded duration handling for Windows owner commands.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@danielgap

Copy link
Copy Markdown
Author

Maintainer action requested: please add the type:bug label. GitHub rejected the fork author’s label update due to repository permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci(windows): candidate owner preparation fails when a PowerShell spawn exceeds its 5 s timeout

2 participants