In file tools/license-server/src/config.rs around lines 62-65:

`token_ttl_days` accepts zero or negative values, which would create already-expired tokens. Consider validating it's positive after parsing.

Evidence trail:
tools/license-server/src/config.rs:12 - `token_ttl_days: i64` (signed type allows negatives)
tools/license-server/src/config.rs:62-65 - parsing without positive validation
tools/license-server/src/routes/refresh.rs:64 - `exp_ts = chrono::Utc::now().timestamp() + (state.config.token_ttl_days * 86400)`
tools/license-server/src/routes/webhook.rs:685 - same expiration calculation

In file crates/tirith/src/cli/license_cmd.rs around line 260:

If `to_writer_pretty` fails after partial output, the `println!()` still runs, producing malformed JSON. Consider skipping the newline on error.

Evidence trail:
crates/tirith/src/cli/license_cmd.rs lines 260-264 at REVIEWED_COMMIT: `if serde_json::to_writer_pretty(std::io::stdout().lock(), &val).is_err() { eprintln!("tirith: failed to write JSON output"); } println!();` - The `println!()` is outside the if block and runs unconditionally regardless of whether `to_writer_pretty` succeeded or failed.

In file crates/tirith-core/src/audit_upload.rs around line 175:

Write errors are silently discarded with `let _ =`. If the write fails, callers won't know and data may be lost or duplicated. Consider returning a `Result` and propagating errors.

Evidence trail:
crates/tirith-core/src/audit_upload.rs lines 174-181 at REVIEWED_COMMIT: `rewrite_spool` function uses `let _ = fs::write(...)` discarding the Result. Lines 145-163 show `rewrite_spool` is called after successfully uploading audit events, meaning a failed write would leave sent events in the spool for potential re-upload.

In file crates/tirith-core/src/policy.rs around line 745:

Environment variables won't be cleaned up if assertions panic, potentially polluting other tests. Consider using a RAII guard pattern or `std::panic::catch_unwind` to ensure cleanup runs regardless of test outcome.

Evidence trail:
crates/tirith-core/src/policy.rs lines 730-764 at REVIEWED_COMMIT: Test function `test_discover_skips_remote_fetch_below_team_tier` sets env vars at lines 745-747, runs assertions at lines 749-756, and only cleans up at lines 758-760. The `_guard` at line 732 is a mutex lock, not an env var cleanup guard. Rust assertions panic on failure, preventing cleanup code from running.

In file crates/tirith-core/src/rules/command.rs around line 522:

Suggestion: Align `is_private_ip` docs and behavior. The doc says “private/reserved,” but the code only checks IPv4 RFC1918 and skips 127/8; IPv6 isn’t handled. Either narrow the doc to “IPv4 RFC1918 only (excluding loopback)” or extend detection to cover 169.254/16, 100.64/10, 127/8 (if SSRF‑focused), and IPv6 private/link‑local (e.g., fc00::/7, fe80::/10).

Evidence trail:
crates/tirith-core/src/rules/command.rs lines 522-534 at REVIEWED_COMMIT. Doc comment on line 522 states 'Check if an IPv4 address is in a private/reserved range (excluding loopback).' Implementation lines 523-533 only check RFC1918 private ranges (10/8, 172.16/12, 192.168/16), not other reserved ranges like 169.254/16 (link-local) or 100.64/10 (CGNAT) that fall under the 'reserved' category. Note: The doc does explicitly say 'IPv4' and 'excluding loopback', so those parts of the issue's claims are less accurate.

In file crates/tirith-core/src/license.rs around line 378:

The code falls back to `Tier::Pro` on lines 382 and 384, but the doc comment states it should fall back to `Tier::Community`. Consider changing both fallbacks to `Tier::Community` to match the documented behavior.

Evidence trail:
crates/tirith-core/src/license.rs lines 370-395 at REVIEWED_COMMIT: Doc comment on lines 374-375 states "Invalid, expired, or missing keys silently fall back to Community". Implementation on line 382 returns `Tier::Pro` in the unwrap_or_else closure, and line 384 returns `Tier::Pro` in the None arm.

In file crates/tirith-core/src/checkpoint.rs around line 439:

`purge` updates in‑memory state even when `remove_dir_all` fails, which undercounts entries/bytes. Suggest only mutating `all`/`total`/counters after a successful delete and keeping the item when deletion fails.

Evidence trail:
crates/tirith-core/src/checkpoint.rs lines 420-485 (REVIEWED_COMMIT):
- Line ~440: `return false;` is outside the match block, so items are removed from `all.retain()` even when `remove_dir_all` fails
- Lines ~447-448: `all.pop()` removes item before `remove_dir_all` is called
- Lines ~466-468: `all.pop()` and `total -= oldest.total_bytes` execute before `remove_dir_all`

In file crates/tirith-core/src/checkpoint.rs around line 264:

`is_ascii_hexdigit()` accepts both uppercase and lowercase hex. If stored files use lowercase but a crafted manifest contains uppercase, the file lookup silently fails. Consider enforcing lowercase only in validation.

Evidence trail:
crates/tirith-core/src/checkpoint.rs lines 262-267 (validate_sha256_filename function with comment claiming lowercase validation but using is_ascii_hexdigit), line 645 (sha256_file uses {:x} format producing lowercase), lines 297-303 (file lookup using manifest sha256 and skip-on-missing behavior)

In file crates/tirith-core/src/checkpoint.rs around line 255:

This validation rejects `..` but allows absolute paths like `/etc/passwd`. Consider also rejecting `RootDir` components to prevent arbitrary file overwrites.

Evidence trail:
crates/tirith-core/src/checkpoint.rs lines 252-260 (validate_restore_path function only checks for ParentDir), line 292 (function called on manifest entry), lines 305-315 (path used as destination for fs::copy), line 723 test explicitly shows absolute paths pass validation: `assert!(validate_restore_path("/absolute/path/file.txt").is_ok());`

In file crates/tirith-core/src/policy.rs around line 355:

`enforce_fail_mode` is unused in `discover()`; auth errors always fail closed. If it’s meant to control auth‑error fallback, consider checking it before calling `fail_closed_policy()`. Otherwise remove the field.

Evidence trail:
crates/tirith-core/src/policy.rs lines 105-108 (field definition with doc comment), line 257 (default value), lines 355-358 (auth error handling that ignores the field). git_grep for 'enforce_fail_mode' confirms only 2 occurrences in the entire codebase (definition and default assignment).

In file crates/tirith-core/src/mcp/resources.rs around line 51:

Consider passing `cwd` to `Policy::discover` to match the working directory used by the scan. Currently `None` is passed, which may load a different policy than what `scan_single_file()` uses internally.

Evidence trail:
- crates/tirith-core/src/mcp/resources.rs line 40: `cwd = std::env::current_dir()`
- crates/tirith-core/src/mcp/resources.rs line 51: `Policy::discover(None)`
- crates/tirith-core/src/mcp/resources.rs line 52: `scan::scan(&config)` called
- crates/tirith-core/src/scan.rs line 105: `scan_single_file(file_path)` called
- crates/tirith-core/src/scan.rs line 154: `cwd = file_path.parent().map(|p| p.display().to_string())`
- crates/tirith-core/src/scan.rs line 170: `Policy::discover(cwd.as_deref())`
- crates/tirith-core/src/policy.rs lines 573-576: `discover_policy_path()` uses `cwd` or falls back to `std::env::current_dir().ok()`

In file crates/tirith-core/src/rules/cloaking.rs around lines 115-117:

If Chrome fetch fails (status=0) but others succeed, `baseline_body` is empty, causing false positives. Consider checking if the baseline fetch succeeded and either returning an error or selecting another successful response as baseline.

Evidence trail:
crates/tirith-core/src/rules/cloaking.rs lines 97-107 (failed fetches push empty body with status=0), lines 116-118 (baseline is Chrome at index 0, baseline_body is used without success check), lines 137-138 (only non-baseline empty bodies are skipped), lines 142-144 (comparison and cloaking threshold check)

In file crates/tirith/src/cli/license_cmd.rs around line 125:

`Policy::discover()` is called twice (lines 126 and 130), which can perform network I/O and return different policy instances. Consider calling it once and extracting both `policy_server_url` and `policy_server_api_key` from the same instance.

Evidence trail:
crates/tirith/src/cli/license_cmd.rs lines 125-132 (REVIEWED_COMMIT): Shows two separate `Policy::discover(None)` calls in `or_else` closures.
crates/tirith-core/src/policy.rs lines 306-360 (REVIEWED_COMMIT): Shows `Policy::discover()` implementation which calls `fetch_remote_policy()` at line 334, confirming network I/O capability.

In file crates/tirith-core/src/checkpoint.rs around line 520:

`backup_file` does separate reads for hashing, copying, and size, so the source can change/disappear and yield inconsistent hash/content/size. Suggest opening once and streaming: compute hash and size from the same read and write to the destination (or a temp), and base dedup/size on that.

Evidence trail:
crates/tirith-core/src/checkpoint.rs lines 520-545 (backup_file function): sha256_file(path) at line 521, fs::copy(path, &dst) at line 525, path.metadata() at line 528. crates/tirith-core/src/checkpoint.rs lines 633-645 (sha256_file function): opens file, reads in 8KB chunks, computes hash, closes file. Commit: REVIEWED_COMMIT

In file crates/tirith-core/src/mcp/tools.rs around line 327:

Policy base dir is inconsistent with `scan_single_file` (`Policy::discover(None)` vs the file’s parent), which can misapply `dlp_custom_patterns`. Suggest discovering the policy from the file’s parent and using it for redaction in both `tirith_scan_file` and `tirith_verify_mcp_config`.

Evidence trail:
- tools.rs:327: `let policy = crate::policy::Policy::discover(None);`
- tools.rs:329-331: `match scan::scan_single_file(&path)` then `redact_findings(..., &policy.dlp_custom_patterns)`
- scan.rs:156: `let cwd = file_path.parent().map(|p| p.display().to_string());`
- scan.rs:172: `let policy = crate::policy::Policy::discover(cwd.as_deref());`
- policy.rs:573-576: `discover_policy_path` shows that `None` falls back to `std::env::current_dir()` while a provided `cwd` uses that path as the starting point
- Commit: REVIEWED_COMMIT

In file crates/tirith-core/src/audit_aggregator.rs around line 118:

When only one timestamp parses successfully, falling back to lexicographic comparison mixes parsed and unparsed formats incorrectly. Consider handling the case where one parses but the other doesn't separately—perhaps skip filtering or return an error—rather than using string comparison with mismatched formats.

Evidence trail:
crates/tirith-core/src/audit_aggregator.rs lines 100-137 at REVIEWED_COMMIT - shows the `parse_ts` function and `filter_records` function with the `_` wildcard pattern at lines ~117-123 that falls through to lexicographic comparison for all non-both-parsed cases including when only one timestamp parses successfully.

In file crates/tirith-core/src/audit.rs around line 292:

Test removes `TIRITH_LOG` at line 259 but doesn't restore it in cleanup (lines 292-295). Consider adding `unsafe { std::env::remove_var("TIRITH_LOG") };` to cleanup for consistency, or saving/restoring its original value.

Evidence trail:
crates/tirith-core/src/audit.rs lines 250-298 (REVIEWED_COMMIT): Line 259 shows `unsafe { std::env::remove_var("TIRITH_LOG") };`. Lines 292-295 show cleanup removing only `XDG_STATE_HOME`, `TIRITH_API_KEY`, `TIRITH_SERVER_URL`, `TIRITH_LICENSE` - no restoration of TIRITH_LOG. Lines 254-258 show the vars being SET at setup.

In file crates/tirith-core/src/scan.rs around line 170:

Suggestion: Avoid discovering policy twice. `engine::analyze` already discovers policy; calling `Policy::discover` again for paranoia filtering can yield inconsistent settings if the remote policy changes. Consider threading a single discovered policy through analysis and filtering (e.g., pass it into `analyze()` or reuse the policy from the verdict).

Evidence trail:
- crates/tirith-core/src/scan.rs lines 167, 170-172 (REVIEWED_COMMIT): Shows `engine::analyze(&ctx)` called at line 167, then `Policy::discover` called again at line 170 for paranoia filtering
- crates/tirith-core/src/engine.rs line 424 (REVIEWED_COMMIT): Shows `Policy::discover(ctx.cwd.as_deref())` called inside `analyze()`
- crates/tirith-core/src/policy.rs lines 306-391 (REVIEWED_COMMIT): Shows `discover()` can fetch from remote server via `policy_client::fetch_remote_policy()` at line 333, confirming remote policy could change between calls

In file crates/tirith/src/cli/scan.rs around line 215:

On JSON serialization failure, avoid emitting any `stdout` (including the trailing newline). Suggest returning immediately after logging the error.

Evidence trail:
crates/tirith/src/cli/scan.rs lines 215-219 at REVIEWED_COMMIT: The `println!();` statement executes unconditionally after the error handling block, meaning a trailing newline is written to stdout even when `serde_json::to_writer_pretty` fails.

In file tools/license-server/src/main.rs around line 237:

`Path::new("database.db").parent()` returns `Some("")`, not `None`, so the `/data` fallback is never used for relative paths. Consider checking if the parent is empty before falling back.

Evidence trail:
tools/license-server/src/main.rs lines 237-240 at REVIEWED_COMMIT: shows `.parent().unwrap_or(std::path::Path::new("/data"))` pattern. Rust std::path::Path documentation at https://doc.rust-lang.org/src/std/path.rs.html lines 2564-2587 confirms: "Returns None if the path terminates in a root or prefix, or if it's the empty string" and shows example `assert_eq!(grand_parent, Some(Path::new("")));` for parent of single-component relative path. Also https://doc.rust-lang.org/std/path/struct.PathBuf.html states: "This means it returns Some("") for relative paths with one component."

In file tools/license-server/src/routes/webhook.rs around line 628:

Consider logging errors from `insert_dead_letter` instead of silently discarding them with `let _ =`. If dead letter insertion fails, there's no visibility into why unresolvable product events aren't being recorded for retry.

Evidence trail:
tools/license-server/src/routes/webhook.rs lines 620-640 at REVIEWED_COMMIT - shows `let _ = state.db.insert_dead_letter(...)` pattern where the Result is explicitly discarded. The `error!` macro call at lines 624-628 logs the unresolvable product issue but does not log any failure from the `insert_dead_letter` call itself.

In file crates/tirith-core/src/checkpoint.rs around line 185:

If writing `meta.json` or `manifest.json` fails, the checkpoint directory is left with orphaned files but no manifest. Consider cleaning up `cp_dir` on write failures, similar to the empty manifest case.

Evidence trail:
crates/tirith-core/src/checkpoint.rs lines 131-133 (directory creation), lines 135-164 (file backup), lines 166-169 (empty manifest cleanup), line 185 (meta.json write with no cleanup on failure), lines 190-191 (manifest.json write with no cleanup on failure). Verified at REVIEWED_COMMIT.

In file crates/tirith-core/src/checkpoint.rs around line 587:

TOCTOU race between `is_symlink()` check and `backup_file()`. Consider using `symlink_metadata()` and checking `file_type().is_symlink()` immediately before reading, or opening the file with `O_NOFOLLOW` semantics to prevent following symlinks atomically.

Evidence trail:
crates/tirith-core/src/checkpoint.rs lines 587-610: `is_symlink()` check at line 587, `backup_file()` call at line ~610 (viewed via view tool with range [575, 610]). crates/tirith-core/src/checkpoint.rs lines 520-545: `backup_file()` function calls `sha256_file(path)` at line 521 and `fs::copy(path, &dst)` at line 525. crates/tirith-core/src/checkpoint.rs lines 634-646: `sha256_file()` uses `fs::File::open(path)` which follows symlinks by default. The TOCTOU race exists between the symlink check and the file operations.

In file crates/tirith-core/src/license.rs around line 1295:

If `RELEASE_TAG` is `v0.3-beta`, `parts[1]` becomes `"3-beta"` and `.parse::<u32>()` panics. Consider splitting on `-` first to strip pre-release suffixes before parsing.

Evidence trail:
crates/tirith-core/src/license.rs lines 1285-1297 at REVIEWED_COMMIT: code strips 'v' prefix, splits on '.', and parses parts[0] and parts[1] directly as u32 without handling pre-release suffixes. Verified that `"3-beta".parse::<u32>()` would return Err and trigger the panic in unwrap_or_else.

In file tools/license-server/src/webhook_verify.rs around line 39:

Casting negative `max_age_secs` to `u64` wraps to a huge value, disabling expiration. Consider using `max_age_secs.unsigned_abs()` or validating the input is positive.

Evidence trail:
tools/license-server/src/webhook_verify.rs lines 21, 37-40: `max_age_secs: i64` parameter at line 21, cast at line 39 via `max_age_secs as u64`. No validation exists that `max_age_secs` must be positive. Rust `as` cast on negative i64 to u64 performs wrapping conversion.

In file crates/tirith-core/src/mcp/dispatcher.rs around line 200:

Serialization failures return `json!({})` as a success response, which sends malformed data to clients. Consider returning a `JsonRpcResponse::err()` with an internal error code instead.

Evidence trail:
crates/tirith-core/src/mcp/dispatcher.rs lines 200-206 at REVIEWED_COMMIT: The `tools/call` handler uses `serde_json::to_value(result).unwrap_or_else(|e| { ... json!({}) })` wrapped in `JsonRpcResponse::ok()`, confirming that serialization failures return an empty JSON object as a success response.

In file crates/tirith-core/src/engine.rs around line 76:

Suggestion: `env` option parsing in `find_inline_bypass`/`resolve_env_wrapper` is too narrow. Combined short flags and flags with arguments can cause the arg to be treated as the command. Consider parsing `env` per POSIX (support combined flags and arg-taking options like `-u`, `-C`, `-S`) before locating the command.

Evidence trail:
1. crates/tirith-core/src/engine.rs lines 76-83 (`find_inline_bypass`): Only `-u` handled as taking argument
2. crates/tirith-core/src/engine.rs lines 230-236 (`resolve_env_wrapper`): Same pattern, only `-u` handled
3. crates/tirith-core/src/rules/command.rs lines 94-95 (`resolve_env_from_args`): `env_value_flags = ["-u"]`
4. GNU coreutils env manpage documents `-C DIR` and `-S STRING` as argument-taking options not handled by this code

In file crates/tirith/src/cli/check.rs around line 66:

`Policy::discover()` is called twice per invocation—once inside `engine::analyze()` and again at line 66. For Team-tier users with a policy server configured, this doubles network requests. Consider having `analyze()` return the policy it loaded, or caching the result.

Evidence trail:
- crates/tirith/src/cli/check.rs:63 - calls `engine::analyze(&ctx)`
- crates/tirith/src/cli/check.rs:66 - calls `Policy::discover(ctx.cwd.as_deref())`
- crates/tirith-core/src/engine.rs:477 - inside `analyze()`, calls `Policy::discover(ctx.cwd.as_deref())`
- crates/tirith-core/src/policy.rs:306-391 - `Policy::discover()` implementation shows network call at line 333 via `fetch_remote_policy()` for Team-tier users with policy server configured

In file crates/tirith/src/cli/check.rs around lines 92-93:

When `approval_check` is true and approval is needed, the early return at line 93 skips writing `last_trigger.json`. Consider adding `last_trigger::write_last_trigger(&verdict, cmd)` before the return to maintain consistency with the non-approval path.

Evidence trail:
crates/tirith/src/cli/check.rs lines 74-93 (early return after approval_check path), lines 134-136 (write_last_trigger call that is skipped by the early return at line 93)

In file crates/tirith-core/src/engine.rs around line 767:

In `paste.rs`, `filter_findings_by_paranoia` mutates the verdict *before* `log_verdict` is called, so the audit log captures only filtered findings. Since this is documented as an "output-layer filter" and ADR-13 says "the engine always detects everything," consider calling `log_verdict` before filtering, or cloning findings for audit.

Evidence trail:
crates/tirith/src/cli/paste.rs lines 69 and 79-85 (filter_findings_by_paranoia called before log_verdict); crates/tirith-core/src/engine.rs lines 758-759 (doc comment: 'This is an output-layer filter — the engine always detects everything (ADR-13)'); crates/tirith-core/src/audit.rs lines 66-70 (log_verdict extracts rule_ids from verdict.findings); docs/threat-model.md line 55 (ADR-13: 'All detection rules run regardless of tier')

In file crates/tirith-core/src/policy.rs around line 688:

On Windows, the cached policy file is created with default permissions since `mode(0o600)` only applies on Unix. If this file may contain sensitive data, consider adding Windows-specific permission handling (e.g., using `windows-acl` or similar), or document why default permissions are acceptable on Windows.

Evidence trail:
crates/tirith-core/src/policy.rs lines 680-697 at REVIEWED_COMMIT: The `cache_remote_policy` function has a comment stating 'Write with restricted permissions (owner-only)' at line 686. The permission restriction `opts.mode(0o600)` is applied inside `#[cfg(unix)]` block (lines 689-692), meaning Windows receives no special permission handling. The Policy struct (lines 21-50) shows the cached data includes security settings like `fail_mode`, `allow_bypass_env`, `paranoia`, `severity_overrides`, `additional_known_domains`, and allowlists.

In file crates/tirith-core/src/policy.rs around line 336:

`cache_remote_policy` silently returns `Ok(())` when `remote_policy_cache_path()` is `None`, and the result is discarded here. If caching silently fails, the `"cached"` fail mode won't work as expected. Consider logging a warning when the cache path cannot be determined.

Evidence trail:
crates/tirith-core/src/policy.rs line 336 (REVIEWED_COMMIT): `let _ = cache_remote_policy(&yaml);` - result discarded
crates/tirith-core/src/policy.rs lines 680-698 (REVIEWED_COMMIT): `cache_remote_policy` returns `Ok(())` when `remote_policy_cache_path()` is `None` without logging
crates/tirith-core/src/policy.rs lines 671-678 (REVIEWED_COMMIT): `remote_policy_cache_path()` can return `None`
crates/tirith-core/src/policy.rs lines 354-368 (REVIEWED_COMMIT): "cached" fail mode depends on `load_cached_remote_policy()` which also returns `None` when cache path is unavailable

In file crates/tirith/src/cli/license_cmd.rs around lines 100-108:

TOCTOU race: if another process deletes the file between `path.exists()` and `remove_file`, this returns an error even though the goal (no file) was achieved. Consider removing the existence check and treating `ErrorKind::NotFound` from `remove_file` as success.

Evidence trail:
crates/tirith/src/cli/license_cmd.rs lines 100-108 at REVIEWED_COMMIT: Line 100 checks `if !path.exists()`, line 105 calls `std::fs::remove_file(&path)`. The TOCTOU race exists between these two operations - if another process deletes the file between them, `remove_file` returns NotFound error and the code returns failure (return 1) even though the goal was achieved.

In file crates/tirith-core/src/url_validate.rs around line 27:

Hostnames that resolve to private IPs bypass the `is_private_ip` check since `host.parse::<IpAddr>()` only matches literal IP addresses. Consider resolving the hostname via DNS and checking the resolved addresses, or documenting this as a known limitation if DNS resolution is intentionally deferred to a later layer.

Evidence trail:
crates/tirith-core/src/url_validate.rs lines 26-31 (REVIEWED_COMMIT): shows `host.parse::<IpAddr>()` only succeeds for literal IPs, and `is_private_ip()` is only called if parse succeeds. git_grep for 'dns|resolve|lookup|ToSocketAddrs' found no DNS resolution in url_validate.rs or its callers. docs/roadmap.md:19 mentions 'DNS-based threat intelligence lookups' as a future feature. Rust std lib `FromStr for IpAddr` only parses literal IP strings, not hostnames.

In file crates/tirith-core/src/webhook.rs around line 123:

`serde_json::json!` already escapes strings, so `sanitize_for_json(command_preview)` causes double-escaping. Consider passing `command_preview` directly.

Evidence trail:
crates/tirith-core/src/webhook.rs lines 117-125 (the `json!` macro usage with `sanitize_for_json(command_preview)`), lines 250-257 (the `sanitize_for_json` function that pre-escapes strings for JSON by using `serde_json::Value::String(...).to_string()` and stripping quotes). The double-escaping occurs because `sanitize_for_json` returns already-escaped content, and `json!` + `.to_string()` escapes it again.