Add test for Ctrl+C via PTY terminal handling by ejc3 · Pull Request #124 · ejc3/fcvm

ejc3 · 2026-01-06T06:15:25Z

Summary

Tests SIGINT via PTY terminal interrupt character (^C/0x03)
Verifies fcvm's tokio signal handler receives the signal
Confirms clean VM shutdown after Ctrl+C

This validates the TTY handling fix from PR #108 works in real terminal contexts.

Test

make test FILTER=ctrlc  # Passes in ~7s

README.md: - Add architecture diagram showing user → fcvm → VM → container flow - Add missing musl-tools and rustup target installation steps - Use consistent ./fcvm symlink throughout all examples - Remove unnecessary sudo (only for setup, bridged, privileged) - Reorganize: one-shot example first, then long-lived service - Rename "Run a Container" to "More Options" to avoid duplication - Add success indicators (→) showing expected output - Make advanced demos use rootless mode for consistency Makefile: - Auto-detect Rust toolchain bin directory for hosts where cargo is not in PATH but ~/.rustup/toolchains/*/bin exists - Prefer stable toolchain (has musl target) over nightly - Use $(CARGO) variable throughout for consistency src/state/manager.rs: - Change state file permissions from 0600 to 0644 so non-root users can run ./fcvm ls without sudo

The state manager intentionally uses 0o644 (world-readable) permissions so non-root users can list VMs. The test was expecting 0o600. Tested: make test FILTER=state_persistence (passes)

Users may not have Rust installed. Add the standard rustup install command before the musl target setup.

claude-claude · 2026-01-06T06:19:36Z

🔍 Claude Review

SEVERITY: critical

Lint Failed

The Lint CI check has failed. Clippy found the following issue:

[CRITICAL] Unreachable code in test_ctrlc.rs:100

The unreachable!() macro after execvp().expect() is flagged as unreachable code
execvp returns Infallible on success (it replaces the process), so .expect() only continues if it fails
The unreachable!() is therefore unreachable

Action Required: Please fix the lint error before this PR can be reviewed. You can either:

Remove the unreachable!() line (the .expect() already handles the error case)
Add #[allow(unreachable_code)] if the unreachable macro is intentional for documentation

Review by Claude | Run

State files are world-readable (0o644) so non-root users can list VMs. Storing environment variables in state creates a security risk since env vars often contain secrets (API keys, passwords, tokens). The env field was unused - env vars are passed directly to MMDS at VM start time and never read back from state. - Remove config.env field from VmConfig struct - Remove vm_state.config.env assignment in podman.rs - Update test JSON to not include env field - Add comments explaining why env is not stored

claude-claude · 2026-01-06T06:26:40Z

🔍 Claude Review

⚠️ Cannot proceed with review - Lint failed

The Lint check has failed on this PR. Please fix the linting issues before I can perform a review.

To fix locally, run:

cargo fmt --check
cargo clippy --all-targets -- -D warnings

Review by Claude | Run

claude-claude · 2026-01-06T06:27:37Z

🔧 CI Auto-Fix

Created fix PR: #126

The issue was an unreachable expression after the execvp call in test_ctrlc.rs. Since execvp never returns on success (it replaces the process), and .expect() handles the error case, the unreachable!() macro was genuinely unreachable, causing clippy to fail.

View Claude Run

Follow-up to removing env field from VmConfig - tests were still referencing the deleted field.

Tests that SIGINT sent via terminal interrupt character (^C/0x03) through a PTY is properly received by fcvm's tokio signal handler and triggers clean VM shutdown. This verifies the fix from PR #108 (TTY handling) works correctly when Ctrl+C is pressed in a real terminal context. Tested: make test FILTER=ctrlc (passes in ~7s)

claude-claude · 2026-01-06T06:34:00Z

🔍 Claude Review

SEVERITY: blocked

Cannot Review

Lint check failed. Please fix lint issues before review can proceed.

View the failing check: https://github.com/ejc3/fcvm/actions/runs/20740266514/job/59545534589

Next Steps

Fix lint issues locally:

cargo fmt
cargo clippy --all-targets --fix --allow-dirty -- -D warnings

Commit and push the fixes
Wait for CI to pass
Request review again

Review by Claude | Run

claude-claude · 2026-01-06T06:43:55Z

🔍 Claude Review

SEVERITY: none

CI Check Failed

The Host check has failed. Please fix the failing test before I can review this PR.

View failing check

Review by Claude | Run

claude-claude · 2026-01-06T06:51:56Z

🔧 CI Auto-Fix

Created fix PR: #128

Issue

The test_ctrlc_via_terminal test was failing in the Container CI job because it used --network bridged which requires TAP device creation. This fails in containerized environments with:

mount of /sys failed: Operation not permitted

Fix

Changed to --network rootless which works in all test environments and doesn't require privileged networking operations. Since the test validates Ctrl+C signal handling (not networking), rootless mode is sufficient.

View Claude Run

claude-claude · 2026-01-06T07:08:43Z

🔍 Claude Review

SEVERITY: low

Findings

[LOW] Makefile RUST_BIN detection could fail silently

Lines 5-8: The RUST_BIN detection chain uses || which means if all commands fail, RUST_BIN will be empty
If RUST_BIN is empty and cargo is not in PATH, make build will fail with a confusing error

Consider: Add a check after line 10 to error early with a helpful message if cargo is not found:

ifeq (,$(shell command -v $(CARGO) 2>/dev/null))
$(error Rust toolchain not found. Install from https://rustup.rs)
endif

Summary

PR adds comprehensive Ctrl+C testing via PTY terminal handling and includes several quality improvements: security fix (removing env vars from world-readable state files), usability enhancement (allowing non-root fcvm ls), documentation improvements, and build system enhancements. The test correctly simulates real terminal Ctrl+C behavior using PTY primitives. All previous critical/medium issues have been resolved and CI passes.

Review by Claude | Run

Add test for Ctrl+C via PTY terminal handling

ejc3 added 3 commits January 6, 2026 05:42

Fix test_state_persistence to expect 0o644 permissions

3f65c34

The state manager intentionally uses 0o644 (world-readable) permissions so non-root users can list VMs. The test was expecting 0o600. Tested: make test FILTER=state_persistence (passes)

Add rustup installation to Quick Start

bcf887e

Users may not have Rust installed. Add the standard rustup install command before the musl target setup.

ejc3 force-pushed the add-ctrlc-test branch from aa272a9 to 524e07f Compare January 6, 2026 06:25

ejc3 added 2 commits January 6, 2026 06:32

Remove env field from test VmConfig structs

46e7393

Follow-up to removing env field from VmConfig - tests were still referencing the deleted field.

ejc3 force-pushed the add-ctrlc-test branch from 524e07f to fb60c84 Compare January 6, 2026 06:32

Fix unreachable expression lint error in test_ctrlc

47d5397

ejc3 force-pushed the add-ctrlc-test branch from 059b2c0 to 47d5397 Compare January 6, 2026 06:56

This was referenced Jan 6, 2026

fix: remove unreachable code after execvp call #126

Closed

fix: use rootless networking in test_ctrlc for container compatibility #128

Closed

ejc3 merged commit 9b54f4e into main Jan 6, 2026
11 checks passed

ejc3 deleted the add-ctrlc-test branch January 6, 2026 07:13

ejc3 added a commit that referenced this pull request Mar 2, 2026

Merge pull request #124 from ejc3/add-ctrlc-test

8fd6318

Add test for Ctrl+C via PTY terminal handling

ejc3 added a commit that referenced this pull request Mar 2, 2026

Merge pull request #124 from ejc3/add-ctrlc-test

24857fd

Add test for Ctrl+C via PTY terminal handling

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for Ctrl+C via PTY terminal handling#124

Add test for Ctrl+C via PTY terminal handling#124
ejc3 merged 7 commits intomainfrom
add-ctrlc-test

ejc3 commented Jan 6, 2026

Uh oh!

claude-claude bot commented Jan 6, 2026

Uh oh!

claude-claude bot commented Jan 6, 2026

Uh oh!

claude-claude bot commented Jan 6, 2026

Uh oh!

claude-claude bot commented Jan 6, 2026

Uh oh!

claude-claude bot commented Jan 6, 2026

Uh oh!

claude-claude bot commented Jan 6, 2026

Uh oh!

claude-claude bot commented Jan 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ejc3 commented Jan 6, 2026

Summary

Test

Uh oh!

claude-claude bot commented Jan 6, 2026

🔍 Claude Review

Lint Failed

Uh oh!

claude-claude bot commented Jan 6, 2026

🔍 Claude Review

Uh oh!

claude-claude bot commented Jan 6, 2026

🔧 CI Auto-Fix

Uh oh!

claude-claude bot commented Jan 6, 2026

🔍 Claude Review

Cannot Review

Next Steps

Uh oh!

claude-claude bot commented Jan 6, 2026

🔍 Claude Review

CI Check Failed

Uh oh!

claude-claude bot commented Jan 6, 2026

🔧 CI Auto-Fix

Issue

Fix

Uh oh!

claude-claude bot commented Jan 6, 2026

🔍 Claude Review

Findings

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant