Add MCP server with JSON-RPC lifecycle, gateway, and cloaking detection#47
Add MCP server with JSON-RPC lifecycle, gateway, and cloaking detection#47
Conversation
push_segment() incorrectly treated VAR=VALUE as the command token. Now skips leading environment variable assignments to find the real command. Adds pub is_env_assignment() helper for use by engine bypass detection. Fixes: TIRITH=0 curl evil.com now correctly identifies curl as command. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add command-aware output-flag skipping for curl (-o/--output) and wget (-O/-OFILE/--output-document). Extract URLs from command+args instead of raw segment text to avoid matching URLs in env-prefix values. Add conservative non-TLD file extensions (.png, .jpg, .mp4, etc.) to schemeless host exclusion list. Fixes issue #33. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MCP server: JSON-RPC 2.0 stdio server with tools for check-command, check-url, check-paste, scan-file, scan-directory, verify-mcp-config, and fetch-cloaking. Includes path traversal protection and resources. Gateway: transparent JSON-RPC proxy that intercepts shell commands in MCP tool calls and runs them through tirith analysis before forwarding. Setup CLI: `tirith setup <tool>` automates hook installation for Claude Code, Codex, Cursor, VS Code, and Windsurf. Atomic writes, symlink safety, drift detection, dry-run support, zshenv guard, managed blocks. Also adds: cloaking detection rule, checkpoint module, embedded hook assets, client integration docs, and troubleshooting guide. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| } | ||
| } else { | ||
| if exists { | ||
| let _ = fs_helpers::run_cli("codex", &["mcp", "remove", "tirith-gateway"]); |
There was a problem hiding this comment.
🟢 Low setup/tools.rs:262
The run_cli error from mcp remove is silently discarded. If removal fails (e.g., timeout, permission error), the subsequent add may fail with a confusing "already exists" message. Consider propagating or logging the removal error.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/tools.rs around line 262:
The `run_cli` error from `mcp remove` is silently discarded. If removal fails (e.g., timeout, permission error), the subsequent `add` may fail with a confusing "already exists" message. Consider propagating or logging the removal error.
Evidence trail:
crates/tirith/src/cli/setup/tools.rs lines 261-264 at REVIEWED_COMMIT showing `let _ = fs_helpers::run_cli("codex", &["mcp", "remove", "tirith-gateway"]);` followed by `let add_out = fs_helpers::run_cli("codex", &add_args)?;`
crates/tirith/src/cli/setup/fs_helpers.rs line 253 at REVIEWED_COMMIT showing `pub fn run_cli(cmd: &str, args: &[&str]) -> Result<std::process::Output, String>` - confirms run_cli returns a Result that is being silently discarded.
| } | ||
| } | ||
|
|
||
| if dry_run { |
There was a problem hiding this comment.
🟢 Low setup/mod.rs:263
In dry-run mode, the symlink safety check from atomic_write is bypassed. Consider adding the same symlink check before the "would write" message to ensure dry-run output accurately reflects what would happen.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/mod.rs around line 263:
In dry-run mode, the symlink safety check from `atomic_write` is bypassed. Consider adding the same symlink check before the "would write" message to ensure dry-run output accurately reflects what would happen.
Evidence trail:
crates/tirith/src/cli/setup/mod.rs lines 263-269: dry-run path returns early with "would write" message before calling atomic_write
crates/tirith/src/cli/setup/fs_helpers.rs lines 73-87: atomic_write function contains symlink safety check that would error if path is a symlink
Test at crates/tirith/src/cli/setup/fs_helpers.rs line 466: `atomic_write_refuses_symlink` confirms symlink check is intentional safety behavior
| hooks.iter().any(|h| { | ||
| h.get("command") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|cmd| cmd.contains("tirith")) |
There was a problem hiding this comment.
🟢 Low setup/merge.rs:266
The deduplication logic searches for "tirith" in the command, but if hook_command doesn't contain that string, repeated calls will create duplicates. Consider using a more robust identifier (e.g., a fixed marker or matching against the actual hook_command value).
- .map(|cmd| cmd.contains("tirith"))
+ .map(|cmd| cmd == hook_command)🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/merge.rs around line 266:
The deduplication logic searches for `"tirith"` in the command, but if `hook_command` doesn't contain that string, repeated calls will create duplicates. Consider using a more robust identifier (e.g., a fixed marker or matching against the actual `hook_command` value).
Evidence trail:
crates/tirith/src/cli/setup/merge.rs:266 - `cmd.contains("tirith")` hardcoded search string
crates/tirith/src/cli/setup/merge.rs:212 - function signature takes `hook_command: &str`
crates/tirith/src/cli/setup/merge.rs:244 - `new_entry` uses `hook_command` value
crates/tirith/src/cli/setup/merge.rs:302 - `arr.push(new_entry)` when no match found
crates/tirith/src/cli/setup/merge.rs:761 - test uses "python3 hook.py" without 'tirith'
crates/tirith/src/cli/setup/tools.rs:39-44 - production commands contain 'tirith-check.py'
| "cursor" => setup_cursor(&opts), | ||
| "vscode" => setup_vscode(&opts), | ||
| "windsurf" => setup_windsurf(&opts), | ||
| _ => Err(format!( |
There was a problem hiding this comment.
🟢 Low setup/mod.rs:129
The _ => Err(...) branch is unreachable since resolve_scope already returns an error for unknown tools. Consider removing this dead code or adding an unreachable!() macro to document the invariant.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/mod.rs around line 129:
The `_ => Err(...)` branch is unreachable since `resolve_scope` already returns an error for unknown tools. Consider removing this dead code or adding an `unreachable!()` macro to document the invariant.
Evidence trail:
crates/tirith/src/cli/setup/mod.rs lines 88, 124-132, 136-163 at REVIEWED_COMMIT. Line 88 shows `resolve_scope(tool, scope)?` called with `?` operator. Lines 136-163 show `resolve_scope` returns Err for unknown tools. Lines 124-132 show the match statement with all 5 known tools explicitly handled before the `_` wildcard.
| let total = buf.len() + avail_len; | ||
| // Drain remaining bytes until newline or EOF | ||
| loop { | ||
| match reader.fill_buf() { | ||
| Ok([]) => return Err(total), | ||
| Ok(b) => { | ||
| if let Some(pos) = b.iter().position(|&c| c == b'\n') { | ||
| reader.consume(pos + 1); | ||
| return Err(total); | ||
| } | ||
| let len = b.len(); | ||
| reader.consume(len); |
There was a problem hiding this comment.
🟢 Low cli/gateway.rs:1027
The total variable isn't updated in the inner drain loop, so Err(total) understates the actual bytes skipped. Consider accumulating the consumed bytes in the inner loop before returning.
- let total = buf.len() + avail_len;
+ let mut total = buf.len() + avail_len;
// Drain remaining bytes until newline or EOF
loop {
match reader.fill_buf() {
Ok([]) => return Err(total),
Ok(b) => {
if let Some(pos) = b.iter().position(|&c| c == b'\n') {
reader.consume(pos + 1);
- return Err(total);
+ return Err(total + pos + 1);
}
let len = b.len();
+ total += len;
reader.consume(len);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/gateway.rs around lines 1027-1038:
The `total` variable isn't updated in the inner drain loop, so `Err(total)` understates the actual bytes skipped. Consider accumulating the consumed bytes in the inner loop before returning.
Evidence trail:
crates/tirith/src/cli/gateway.rs lines 1027-1041 at REVIEWED_COMMIT. Line 1027 initializes `total = buf.len() + avail_len`. The inner loop at lines 1029-1041 calls `reader.consume()` multiple times (line 1034: `pos + 1` bytes, line 1037: `len` bytes) but never updates `total`. All returns of `Err(total)` (lines 1030, 1034, 1039) use the original value, not accounting for bytes consumed in loop iterations.
| }; | ||
|
|
||
| let resp = JsonRpcResponse::ok(id, serde_json::to_value(&result).unwrap()); | ||
| serde_json::to_string(&resp).unwrap_or_default() |
There was a problem hiding this comment.
🟢 Low cli/gateway.rs:901
serde_json::to_string(...).unwrap_or_default() can emit an empty string, producing invalid JSON-RPC. Suggest treating response serialization as infallible (use unwrap() for JsonRpcResponse), or handle/log the error and emit a valid error object instead. Apply this consistently wherever responses are serialized.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/gateway.rs around line 901:
`serde_json::to_string(...).unwrap_or_default()` can emit an empty string, producing invalid JSON-RPC. Suggest treating response serialization as infallible (use `unwrap()` for `JsonRpcResponse`), or handle/log the error and emit a valid error object instead. Apply this consistently wherever responses are serialized.
Evidence trail:
crates/tirith/src/cli/gateway.rs lines 901, 793, 831, 933 - all use `serde_json::to_string(...).unwrap_or_default()` pattern. crates/tirith-core/src/mcp/types.rs lines 9-42 - JsonRpcResponse struct definition showing it derives Serialize and contains Value types. String::default() returns empty string per Rust stdlib. Empty string is not valid JSON-RPC per the JSON-RPC 2.0 specification.
crates/tirith/src/cli/gateway.rs
Outdated
| #[test] | ||
| fn test_recursion_depth() { | ||
| // Verify the depth check logic | ||
| assert!(1u32 >= 1); // depth=1 would trigger abort |
There was a problem hiding this comment.
🟢 Low cli/gateway.rs:1366
This test is a tautology (1u32 >= 1 always passes) and doesn't actually test recursion depth behavior. Consider either implementing a real test that exercises the recursion depth logic, or removing this placeholder to avoid false confidence in test coverage.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/gateway.rs around line 1366:
This test is a tautology (`1u32 >= 1` always passes) and doesn't actually test recursion depth behavior. Consider either implementing a real test that exercises the recursion depth logic, or removing this placeholder to avoid false confidence in test coverage.
Evidence trail:
crates/tirith/src/cli/gateway.rs lines 1362-1367 at REVIEWED_COMMIT: The test `test_recursion_depth()` contains only `assert!(1u32 >= 1)` which is a constant expression always evaluating to true. crates/tirith/src/cli/gateway.rs line 282 shows actual recursion depth logic exists (`eprintln!("tirith gateway: recursion detected (depth={depth}), aborting")`) that the test should be exercising but doesn't.
…C ignore - Fix clippy: assertions_on_constants in gateway recursion depth test - Gate zshenv integration tests behind zsh_available() check so they skip gracefully on CI runners without zsh installed - Fix redundant closure clippy lints in golden_fixtures - Add .cargo/audit.toml ignoring RUSTSEC-2026-0009 (time crate DoS, not exploitable in our usage, fix requires Rust 1.88) - Add same ignore to deny.toml
|
|
||
| // Symlink safety: refuse to overwrite a symlink target (or broken symlink). | ||
| // Always use symlink_metadata — path.exists() misses broken symlinks. | ||
| match fs::symlink_metadata(path) { |
There was a problem hiding this comment.
🟢 Low setup/fs_helpers.rs:73
There's a TOCTOU race between symlink_metadata (line 73) and rename (line 89)—another process could replace the target with a symlink in between. Consider documenting this limitation, or using platform-specific APIs like renameat2 with RENAME_NOREPLACE on Linux for stronger guarantees.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/fs_helpers.rs around line 73:
There's a TOCTOU race between `symlink_metadata` (line 73) and `rename` (line 89)—another process could replace the target with a symlink in between. Consider documenting this limitation, or using platform-specific APIs like `renameat2` with `RENAME_NOREPLACE` on Linux for stronger guarantees.
Evidence trail:
crates/tirith/src/cli/setup/fs_helpers.rs lines 71-92 at REVIEWED_COMMIT: Line 73 calls `fs::symlink_metadata(path)` to check for symlinks, line 89 calls `fs::rename(&tmp, path)`. The race window between these two operations is a classic TOCTOU vulnerability pattern. The code comments at lines 71-72 explicitly state this is for 'Symlink safety' but the implementation cannot guarantee atomicity.
| severity = f.get("severity", "") | ||
| parts.append(f"[{severity}] {title}" if severity else title) | ||
| reason = "Tirith: " + "; ".join(parts) | ||
| except json.JSONDecodeError: |
There was a problem hiding this comment.
🟡 Medium hooks/tirith-check.py:137
If findings is not iterable or contains non-dict items, the resulting TypeError/AttributeError propagates to the outer handler and exits 0 (fail-open), bypassing the block. Consider wrapping the findings loop in a broader except Exception or validating types before iterating.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file .claude/hooks/tirith-check.py around line 137:
If `findings` is not iterable or contains non-dict items, the resulting `TypeError`/`AttributeError` propagates to the outer handler and exits 0 (fail-open), bypassing the block. Consider wrapping the findings loop in a broader `except Exception` or validating types before iterating.
Evidence trail:
.claude/hooks/tirith-check.py lines 127-147 at REVIEWED_COMMIT:
- Line 129: `findings = verdict.get("findings", [])`
- Line 132: `for f in findings:` - iterates over findings
- Line 133: `f.get(...)` - calls .get() on each item
- Line 137: `except json.JSONDecodeError:` - only catches JSON parsing errors
- Lines 143-147: Outer handler `except Exception: sys.exit(0)` with comment '# Fail open on any unexpected error'
|
|
||
| fn shutdown_child(child: &mut Child, abnormal: bool) -> i32 { | ||
| // Check if already exited | ||
| if let Ok(Some(_)) = child.try_wait() { |
There was a problem hiding this comment.
🟢 Low cli/gateway.rs:948
If try_wait() returns Err, the code silently continues and may send signals to an invalid or recycled PID. Consider logging these errors or returning early, since an OS error often means the child is in an unrecoverable state.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/gateway.rs around line 948:
If `try_wait()` returns `Err`, the code silently continues and may send signals to an invalid or recycled PID. Consider logging these errors or returning early, since an OS error often means the child is in an unrecoverable state.
Evidence trail:
crates/tirith/src/cli/gateway.rs lines 946-986 at REVIEWED_COMMIT - shutdown_child function uses `if let Ok(Some(_)) = child.try_wait()` pattern that ignores `Err` returns, then proceeds to call `libc::kill(child.id() as i32, libc::SIGTERM)` without verifying child validity.
| if policy.max_message_bytes == 0 { | ||
| return Err("max_message_bytes must be > 0".to_string()); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
🟡 Medium cli/gateway.rs:131
Consider validating that timeout_ms is greater than zero. A value of 0 creates a zero-duration timeout, causing recv_timeout to immediately return Err(Timeout), treating all guarded calls as timeouts rather than analyzing them.
if policy.max_message_bytes == 0 {
return Err("max_message_bytes must be > 0".to_string());
}
+ if policy.timeout_ms == 0 {
+ return Err("timeout_ms must be > 0".to_string());
+ }
Ok(())🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/gateway.rs around lines 131-134:
Consider validating that `timeout_ms` is greater than zero. A value of `0` creates a zero-duration timeout, causing `recv_timeout` to immediately return `Err(Timeout)`, treating all guarded calls as timeouts rather than analyzing them.
Evidence trail:
crates/tirith/src/cli/gateway.rs lines 113-133 (validate_policy_values function - validates max_message_bytes > 0 but NOT timeout_ms), line 564 (Duration::from_millis(config.policy.timeout_ms)), line 565 (rx.recv_timeout(timeout)), lines 624-644 (Err(_) branch handles timeout by forwarding or denying based on fail_mode). Rust std docs confirm recv_timeout with zero duration returns Timeout error immediately if channel is empty.
| if std::process::Command::new("zsh") | ||
| .arg("--version") | ||
| .output() | ||
| .is_err() | ||
| { |
There was a problem hiding this comment.
🟢 Low setup/zshenv.rs:312
The check uses .is_err() which only detects spawn failures, not a broken zsh installation that exits non-zero. Consider using zsh_available() helper (or matching its logic with .is_ok_and(|o| o.status.success())) for consistency.
- if std::process::Command::new("zsh")
- .arg("--version")
- .output()
- .is_err()
- {
+ if !zsh_available() {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/zshenv.rs around lines 312-316:
The check uses `.is_err()` which only detects spawn failures, not a broken zsh installation that exits non-zero. Consider using `zsh_available()` helper (or matching its logic with `.is_ok_and(|o| o.status.success())`) for consistency.
Evidence trail:
crates/tirith/src/cli/setup/zshenv.rs lines 310-316 (`.is_err()` check), lines 330-335 (`zsh_available()` helper using `.is_ok_and(|o| o.status.success())`). Verified at REVIEWED_COMMIT.
- Merge origin/main (glibc build fix) - Fix single & segment boundary in split_raw_words (security) - Use exact match == TIRITH=0 (prevents false bypass) - Skip flags in resolve_command_wrapper - Remove dead code in is_tirith_command - Remove quote-strip from is_env_assignment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| let mut in_managed_block = false; | ||
| for line in working_text.lines() { | ||
| if line.contains(begin_marker) { | ||
| in_managed_block = true; | ||
| continue; | ||
| } | ||
| if line.contains(end_marker) { | ||
| in_managed_block = false; | ||
| continue; | ||
| } | ||
| if !in_managed_block && hooks_key_re.is_match(line) { |
There was a problem hiding this comment.
🟢 Low setup/merge.rs:380
The in_managed_block tracking (lines 380-389) is dead code—working_text never contains the markers here since either remove_managed_block() stripped them or they never existed. Consider removing this logic or adding a comment explaining why it's kept (e.g., defensive coding).
- let mut in_managed_block = false;
for line in working_text.lines() {
- if line.contains(begin_marker) {
- in_managed_block = true;
- continue;
- }
- if line.contains(end_marker) {
- in_managed_block = false;
- continue;
- }
- if !in_managed_block && hooks_key_re.is_match(line) {
+ if hooks_key_re.is_match(line) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/merge.rs around lines 380-390:
The `in_managed_block` tracking (lines 380-389) is dead code—`working_text` never contains the markers here since either `remove_managed_block()` stripped them or they never existed. Consider removing this logic or adding a comment explaining why it's kept (e.g., defensive coding).
Evidence trail:
crates/tirith/src/cli/setup/merge.rs lines 340-360 (working_text assignment logic), lines 380-389 (the in_managed_block tracking being claimed as dead), lines 503-537 (remove_managed_block implementation showing markers are stripped). Key observations: has_begin && !force causes early return at line 344; has_begin && force leads to remove_managed_block() which strips markers; !has_begin means raw never had markers. All paths to line 380 result in working_text without markers.
| let mut suppressing = false; | ||
|
|
||
| for line in text.lines() { | ||
| if line.contains(begin_marker) { |
There was a problem hiding this comment.
🟢 Low setup/merge.rs:512
If begin_marker appears while already suppressing, it's silently skipped. Consider returning an error for duplicate BEGIN markers to surface file corruption.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/merge.rs around line 512:
If `begin_marker` appears while already suppressing, it's silently skipped. Consider returning an error for duplicate BEGIN markers to surface file corruption.
Evidence trail:
crates/tirith/src/cli/setup/merge.rs lines 500-540 at REVIEWED_COMMIT - The `remove_managed_block` function shows: (1) line 512-515 sets `suppressing = true` on begin_marker with no check for existing suppression state, (2) line 517-521 returns error for END without BEGIN, (3) line 527-529 returns error for missing END, but no error for duplicate BEGIN markers.
Resolved 19 conflicts by keeping main's improved code. Used main's engine.rs. Added missing fields to Finding/Verdict constructors in gateway tests. Added dead_code allows for setup/gateway modules not yet wired to CLI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| "gateway", "run", | ||
| "--upstream-bin", "tirith", | ||
| "--upstream-arg", "mcp-server", | ||
| "--config", "~/.config/tirith/gateway.yaml" |
There was a problem hiding this comment.
🟡 Medium clients/vscode.md:108
The ~ in --config ~/.config/tirith/gateway.yaml won't expand when VS Code spawns the process directly. Consider using an absolute path (e.g., /Users/you/.config/tirith/gateway.yaml) or adding a note that users must replace ~ with their actual home directory.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file mcp/clients/vscode.md around line 108:
The `~` in `--config ~/.config/tirith/gateway.yaml` won't expand when VS Code spawns the process directly. Consider using an absolute path (e.g., `/Users/you/.config/tirith/gateway.yaml`) or adding a note that users must replace `~` with their actual home directory.
| false, | ||
| false, | ||
| ); | ||
| let _ = output_tx.send(build_deny_response(id, &verdict, elapsed).into_bytes()); |
There was a problem hiding this comment.
🟢 Low cli/gateway.rs:606
Silently ignoring output_tx.send() failures (lines 606, 655) could leave clients waiting indefinitely if the channel closes. Consider logging send failures or propagating the error.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/gateway.rs around line 606:
Silently ignoring `output_tx.send()` failures (lines 606, 655) could leave clients waiting indefinitely if the channel closes. Consider logging send failures or propagating the error.
Evidence trail:
crates/tirith/src/cli/gateway.rs lines 606 and 651-656 at REVIEWED_COMMIT show `let _ = output_tx.send(...)` pattern which explicitly discards the Result<(), SendError>. Line 606: `let _ = output_tx.send(build_deny_response(id, &verdict, elapsed).into_bytes());` and lines 651-656: `let _ = output_tx.send(build_fail_mode_deny(id, "analysis timed out", elapsed, true, true).into_bytes());`. The `let _ =` pattern in Rust explicitly ignores the Result, meaning if the channel receiver has been dropped, the send fails silently with no logging. This is a factual claim about error handling - the send results are verifiably being ignored, and this could indeed cause issues with debugging or detecting when clients don't receive expected responses.
| if !suppressing { | ||
| result.push(line); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟢 Low setup/zshenv.rs:189
If input has BEGIN_PREFIX without matching END_MARKER, all subsequent content is silently dropped. Consider either returning an error for malformed input, or resetting suppressing at the end and keeping those lines.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/setup/zshenv.rs around line 189:
If input has `BEGIN_PREFIX` without matching `END_MARKER`, all subsequent content is silently dropped. Consider either returning an error for malformed input, or resetting `suppressing` at the end and keeping those lines.
Evidence trail:
crates/tirith/src/cli/setup/zshenv.rs lines 173-196 (remove_guard_blocks function showing suppressing logic), lines 142-168 (validate_marker_pairing function that catches unpaired markers but is separate), line 47 (validate_marker_pairing called before remove_guard_blocks in offer_zshenv_guard), lines 92 and 123 (remove_guard_blocks call sites). REVIEWED_COMMIT.
|
|
||
| fn shutdown_child(child: &mut Child, abnormal: bool) -> i32 { | ||
| // Check if already exited | ||
| if let Ok(Some(_)) = child.try_wait() { |
There was a problem hiding this comment.
🟢 Low cli/gateway.rs:951
The child's actual exit status is discarded—try_wait() and wait() return ExitStatus but it's ignored. Consider using the child's exit code (e.g., status.code().unwrap_or(1)) instead of only relying on abnormal, so upstream failures propagate correctly.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/gateway.rs around line 951:
The child's actual exit status is discarded—`try_wait()` and `wait()` return `ExitStatus` but it's ignored. Consider using the child's exit code (e.g., `status.code().unwrap_or(1)`) instead of only relying on `abnormal`, so upstream failures propagate correctly.
Evidence trail:
crates/tirith/src/cli/gateway.rs:949-987 (function shutdown_child), crates/tirith/src/cli/gateway.rs:465 (caller using return value as exit_code), crates/tirith/src/cli/gateway.rs:487 (exit_code returned to caller)
| let cwd = std::env::current_dir() | ||
| .ok() | ||
| .map(|p| p.display().to_string()); | ||
| thread::spawn(move || { |
There was a problem hiding this comment.
🟡 Medium cli/gateway.rs:551
Spawned analysis thread is never joined on timeout, causing unbounded thread accumulation under sustained timeouts. Consider using a bounded thread pool or aborting/tracking orphaned threads.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/tirith/src/cli/gateway.rs around line 551:
Spawned analysis thread is never joined on timeout, causing unbounded thread accumulation under sustained timeouts. Consider using a bounded thread pool or aborting/tracking orphaned threads.
Evidence trail:
crates/tirith/src/cli/gateway.rs lines 551-567 (REVIEWED_COMMIT): thread::spawn at line 551 discards the JoinHandle, recv_timeout at line 567 handles timeout at Err(_) branch (lines 627-660) without joining or tracking the spawned thread.
The merge conflict resolution took main's main.rs which lacked the Gateway and Setup enum variants, GatewayAction enum, and match arms. This left 4,520 lines of gateway/setup code unreachable. Restored all CLI wiring and removed dead_code allows that are no longer needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Test plan
🤖 Generated with Claude Code
Note
Add MCP gateway with JSON-RPC lifecycle and installable client hooks that deny shell commands via Tirith with default timeout 10000ms and max_message_bytes 1048576
Introduce a
tirith gatewayCLI and config, addtirith setupto install client hooks (Claude Code, Codex, Cursor, VS Code, Windsurf), embed hook assets, and implement fail-open/closed policies and warn-action handling across guarded shell tools.📍Where to Start
Start with the CLI entrypoints in
maindispatching to gateway and setup: crates/tirith/src/main.rs, then review gateway config processing in crates/tirith/src/cli/gateway.rs.Macroscope summarized d306b57.