Skip to content
This repository was archived by the owner on Feb 15, 2026. It is now read-only.

fix: cross-platform signal handling for Windows#8

Merged
NicolasRitouet merged 2 commits intomainfrom
fix/windows-signal-handling
Feb 7, 2026
Merged

fix: cross-platform signal handling for Windows#8
NicolasRitouet merged 2 commits intomainfrom
fix/windows-signal-handling

Conversation

@NicolasRitouet
Copy link
Copy Markdown
Member

@NicolasRitouet NicolasRitouet commented Feb 6, 2026

Summary

  • syscall.SIGHUP doesn't exist on Windows, preventing compilation
  • Extracted signal list into build-tagged files (signals_unix.go, signals_windows.go)
  • Unix: SIGINT + SIGTERM + SIGHUP | Windows: SIGINT + SIGTERM

Test plan

  • All 7 injector tests pass on macOS
  • Full CLI builds clean
  • Verify Windows cross-compilation: GOOS=windows go build ./...

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor
    • Signal handling restructured to use platform-specific configurations (Unix vs Windows). Behavior for forwarding signals to child processes remains unchanged; this improves maintainability and clarifies platform-dependent behavior.

SIGHUP doesn't exist on Windows, causing compilation failure. Extract
signal list into platform-specific files using Go build tags.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 6, 2026

📝 Walkthrough

Walkthrough

Extracts platform-specific signal lists from the injector into build-tagged files (signals_unix.go, signals_windows.go) and replaces hard-coded signals in injector.go with a package-level signals variable referenced from those files. No control-flow changes.

Changes

Cohort / File(s) Summary
Core injector
internal/injector/injector.go
Replaced in-file hard-coded signal list with a reference to a package-level signals variable; forwarding loop unchanged.
Platform signal definitions
internal/injector/signals_unix.go, internal/injector/signals_windows.go
Added build-tagged files that declare the package-level signals slice: Unix file defines SIGINT, SIGTERM, SIGHUP; Windows file defines SIGINT, SIGTERM and documents Windows behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I nibble signals, split in two,
Unix three, and Windows two,
Build tags guide which list to send,
One injector, same old end,
Hops of code — a tidy view.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main change: extracting platform-specific signal handling to fix Windows compilation by removing the unavailable SIGHUP signal.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/windows-signal-handling

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 and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@internal/injector/signals_windows.go`:
- Line 10: The signals slice currently includes syscall.SIGTERM which cannot be
forwarded to child processes on Windows; update the signals declaration
(signals) to either remove syscall.SIGTERM on Windows or add a clear comment
explaining that SIGTERM is only used for console shutdown notifications and will
not be forwarded by cmd.Process.Signal (so forwarding attempts will fail/return
an error); ensure the change references the signals variable and notes behavior
of cmd.Process.Signal to make the platform limitation explicit.

Comment thread internal/injector/signals_windows.go
@NicolasRitouet NicolasRitouet merged commit 646b8a9 into main Feb 7, 2026
10 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant