Skip to content

fix(cli): prevent self-destruction via --all commands#604

Merged
Wirasm merged 7 commits intomainfrom
fix/issue-600-self-exclusion-all-commands
Feb 26, 2026
Merged

fix(cli): prevent self-destruction via --all commands#604
Wirasm merged 7 commits intomainfrom
fix/issue-600-self-exclusion-all-commands

Conversation

@Wirasm
Copy link
Copy Markdown
Owner

@Wirasm Wirasm commented Feb 26, 2026

Summary

When honryu (or any agent) runs kild stop --all or kild destroy --all from inside its own session, it includes itself in the operation — killing its own process mid-execution and leaving the fleet unmanaged.

Root Cause

The --all iteration loops in handle_stop_all() and handle_destroy_all() had no self-exclusion filter. Self-detection primitives existed ($KILD_SESSION_BRANCH env var, CWD-based worktree path match) but were only wired into agent-status.

Changes

File Change
crates/kild/src/commands/helpers.rs Add resolve_self_branch() — tries $KILD_SESSION_BRANCH then CWD fallback
crates/kild/src/commands/stop.rs Filter self from --all loop; warn on explicit self-stop
crates/kild/src/commands/destroy.rs Filter self from --all loop; warn on explicit self-destroy

Testing

  • cargo fmt --check passes
  • cargo clippy --all -- -D warnings passes
  • cargo test --all passes (3 new tests for resolve_self_branch)
  • cargo build --all clean

Validation

cargo fmt --check && cargo clippy --all -- -D warnings && cargo test --all && cargo build --all

Fixes #600

…agent

Root cause: open_session() has no guard checking session status before
spawning. Fix: add AlreadyActive error + active-session guard with daemon
liveness check, mirroring the AlreadyExists guard in create_session().
When honryu (or any agent) runs `kild stop --all` or `kild destroy --all`
from inside its own session, it now skips itself and prints a note. Explicit
single-branch commands (`kild stop honryu`) still work but print a warning.

Changes:
- Add `resolve_self_branch()` helper using $KILD_SESSION_BRANCH + CWD fallback
- Filter calling session from stop --all and destroy --all loops
- Add self-targeting warnings for explicit stop/destroy commands
- Add unit tests for resolve_self_branch()

Fixes #600
Sessions created with --main use the project root as worktree_path,
causing any CWD inside the project to false-positive match. The env var
path ($KILD_SESSION_BRANCH) is authoritative for --main sessions.
@Wirasm
Copy link
Copy Markdown
Owner Author

Wirasm commented Feb 26, 2026

Self-Review

Summary

Fix is clean and addresses the root cause: --all loops now detect and skip the calling session via resolve_self_branch(), which tries $KILD_SESSION_BRANCH (fast path for claude/codex agents) then falls back to CWD worktree matching.

Findings

Strengths

  • Self-detection is generic (not honryu-specific) — works for any agent session
  • Env var fast path avoids unnecessary list_sessions() call in the common case
  • --all self-exclusion is a hard skip (not just a warning), preventing accidental self-destruction
  • Explicit single-branch commands warn but don't block (intentional design — explicit naming = intentional action)

Post-Review Fix Applied

  • CWD fallback for --main sessions: find_session_by_worktree_path uses starts_with(), so --main sessions (whose worktree_path is the project root) would match any CWD inside the project. Added guard to skip CWD fallback when session.use_main_worktree is true — the env var path is authoritative for these sessions.

Design Decision: Warn vs Block on Explicit Self-Targeting

The explicit path (kild stop honryu) intentionally warns but proceeds. Rationale: --all is where accidental self-inclusion happens (blind bulk operation). Naming yourself explicitly is an intentional action — blocking it would require a --force flag that stop doesn't have, and the warning is sufficient for awareness.

Checklist

  • Fix addresses root cause from investigation
  • Code follows codebase patterns (collapsible if, color helpers, structured logging)
  • Tests cover the resolve_self_branch() helper (3 tests: env var set, empty, unset)
  • Edge case: --main CWD false positive handled
  • No security concerns

Changes warn-only self-targeting to a hard block: explicit
`kild stop <self>` and `kild destroy <self>` now require --force.
Adds --force flag to `kild stop` command.

Addresses review feedback: an agent reading only stdout would
silently self-destruct with warn-only behavior.
@Wirasm
Copy link
Copy Markdown
Owner Author

Wirasm commented Feb 26, 2026

PR Review Summary

Reviewed by 4 specialized agents: code quality, silent failures, test coverage, docs impact.

Critical Issues (3 found)

Agent Issue Location
code-reviewer --force flag silently ignored with kild stop --force --all — clap accepts the combo but handle_stop_all() never reads force. Add .conflicts_with("all") to the force arg. session.rs:208-214
silent-failure-hunter current_dir() failure in resolve_self_branch() silently returns None with no log — self-protection evaporates if CWD is deleted (common during cleanup). Add warn! before returning None. helpers.rs:26
silent-failure-hunter find_session_by_worktree_path().ok()?? swallows SessionError silently — permission denied or corrupted session files defeat self-detection with no trace. Add warn! before returning None. helpers.rs:27

Important Issues (4 found)

Agent Issue Location
code-reviewer error! level used for deliberate policy blocks (cli.stop_blocked, cli.destroy_blocked) — these are expected behavior, not errors. Use warn! instead. stop.rs:45-49, destroy.rs:41-45
code-reviewer --force help text says "required when stopping own session" but doesn't mention it's ignored with --all. Tied to critical issue #1 — adding conflicts_with resolves both. session.rs:212
silent-failure-hunter Force-override self-stop has no structured log event — only eprintln!. Add warn!(event = "cli.stop_self_forced", ...) before falling through. stop.rs:51-57
silent-failure-hunter Same for force-override self-destroy — no structured event. Add warn!(event = "cli.destroy_self_forced", ...). destroy.rs:48-53

Test Gaps (3 found)

Criticality Gap Notes
8/10 --force flag on stop has no CLI parse test Inconsistent with existing pattern (test_cli_destroy_all_with_force exists for destroy)
8/10 Self-block guard logic in stop.rs and destroy.rs is entirely untested The primary safety feature of the PR has no behavioral test
5/10 Env var mutation in tests not thread-safe KILD_SESSION_BRANCH mutated without mutex — // SAFETY: single-threaded comment is not enforced

Documentation Updates

File Change
README.md Added --force flag to kild stop section; clarified --all skips self when run inside a session

Committed as 6adc537 on the PR branch.

Strengths

  • Core logic is correct — resolve_self_branch(), the --all filtering, and single-branch guards work as intended
  • CWD fallback correctly guards against --main session false-positives
  • Consistent retain pattern in both stop.rs and destroy.rs
  • Structured logging events on all new guard branches
  • The 3 env var path tests for resolve_self_branch cover the meaningful fast-path cases

Verdict: NEEDS FIXES

Priority:

  1. Add .conflicts_with("all") to --force on stop_command() — one-liner, prevents silent flag swallowing
  2. Add warn! logs to resolve_self_branch() fallback failures — prevents silent self-protection bypass
  3. Add structured log events for --force override paths
  4. Downgrade error!warn! on policy block events
  5. Add CLI parse test for stop --force and behavioral tests for the guard logic

- Add warn! logs to resolve_self_branch() CWD and session lookup
  fallback failures (prevents silent self-protection bypass)
- Add .conflicts_with("all") to --force on stop_command() so
  `kild stop --force --all` is rejected by clap instead of silently
  ignoring the flag
- Downgrade error! → warn! on policy block events (cli.stop_failed,
  cli.destroy_failed) — these are expected behavior, not errors
- Add structured log events for --force override paths
  (cli.stop_self_forced, cli.destroy_self_forced)
- Add mutex to serialize env-var-mutating tests for thread safety
- Add CLI parse tests for stop --force, stop -f, and
  stop --force --all conflict
@Wirasm Wirasm merged commit 2aa6f4d into main Feb 26, 2026
6 checks passed
@Wirasm Wirasm deleted the fix/issue-600-self-exclusion-all-commands branch February 26, 2026 16:23
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.

fix: honryu should not be able to stop/destroy itself via --all commands

1 participant