Clean up CLI launch logs in temp (SBS-888) - #313
Conversation
Every `codexbar` process appended a PID-scoped log under the system temp dir and never removed it. `statusline` is invoked once per editor render (SBS-271), so an active session left hundreds to thousands of files. PID reuse appended to an existing file, so a colliding PID grew without bound. Windows `%TEMP%` is not reliably purged. Four changes: - `statusline` writes no launch log at all. It reads cached state and reports through its host, so there is no launch failure a temp file would explain. Checked off raw argv, before clap. - The log opens truncating, so a reused PID cannot append across runs. - A run that exits SUCCESS removes its own log. Steady state is no files. - Leftovers from failed or killed runs are swept after 24 hours, bounded to 512 removals per run. `Cli::parse()` exits the process itself for `--help`, `--version`, and usage errors, which skipped the cleanup and stranded a file. Use `try_parse()` and remove the log before deferring to `Error::exit()`, which keeps clap's own output and exit codes. A run that fails still keeps its log.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 92 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ceiling | d217335 | Commit Preview URL Branch Preview URL |
Aug 16 2026, 02:55 AM |
Automated reviewNew in this pass: 6 issues.
Also noted:
Resolved since the previous pass: 6. For coding agents: fix BLOCK and FIX IF QUICK findings now; everything else is tracked or informational; never exceed one CodeRev fix round per PR. Advisory. Findings generated by |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Three findings from the advisory review, all real. 1. The truncating open followed symlinks. PIDs are predictable, so on a shared /tmp someone can pre-create the next run's log name as a link to a file they want destroyed; the truncating open would empty the target. The previous append-only code was not destructive, so this was a regression introduced by this branch. Unlink any existing entry first (unlinking a symlink never touches the target), then open with `create_new`, which is O_CREAT|O_EXCL and refuses to follow a link planted in the race window. Worst case now is losing the log, not the victim file. `append_launch_log` no longer creates, so it cannot resurrect a path that start refused. 2. `skips_launch_log` only checked argv[1], so `codexbar --verbose statusline` still wrote a log on every editor render. Scan all args instead. No global flag takes `statusline` as a value; `--log-level` is restricted to log levels. 3. The sweep bound capped removals, not the walk, so every non-statusline run still read_dir'd the whole temp root. Launch logs now live in `<temp>/codexbar-launch-logs/` and the sweep is scoped to it. The directory is refused if it is not a real directory, so the name cannot be used to redirect writes or the sweep. New tests: planted-symlink target survives, symlinked log directory is refused, append does not create, and statusline is detected behind each global flag. The symlink test was confirmed to fail against the truncating open.
The last round guarded the shared temp dir; this removes the exposure instead. Review found the guards were not enough: - `start_launch_log` swallowed a failed `create_new`, and `append_launch_log` then opened the same path for append, which follows a symlink. An attacker owning the directory could plant a link the victim cannot unlink, so the launch header was written into their chosen target. Not the old truncating hole; a second one behind it. - `launch_log_dir_in` only refused a symlink, not a directory another user had already created at the well-known name. Both come from putting a predictably named file in a world-writable directory. Every fix for that is a patch on the location. Write to `dirs::cache_dir()/Ceiling/launch-logs/` instead, which is inside the user's home, so squatting and link planting do not apply, and the sweep never walks a directory holding unrelated files. The symlink refusals stay as defence in depth, and append now requires a regular file. Also from review: - Logs older versions wrote to the temp root were orphaned by the move, so the accumulated files this issue is about would never be cleaned. Sweep that legacy location too, age-bounded, never following a link. - `skips_launch_log` matched the token anywhere, so `codexbar --provider statusline` skipped the log for a run that fails. Resolve the actual subcommand, consuming values of the flags that take one. Unknown flags are assumed to take none, which errs toward writing. - `missing_subcommand` returns USAGE_ERROR through the `Ok` path, so bare `codexbar` kept a file every run. Keep logs only for real failures. Verified end to end: statusline, --verbose statusline, --help, --version, bad subcommand, bare codexbar, and a successful usage all leave zero files; a genuine failure keeps one; the temp root stays clean.
btsouth
added
coderev
Four security findings on the launch-log path, plus the argv and sweep follow-ups from the same review. The log directory is now created 0700 and refused unless the current uid owns it and no other account can reach it. A directory another user pre-creates at the well-known name could hold planted per-PID symlinks, read what we write, or - after a sudo first run - lock every later unprivileged run out. The rule itself lives in `classify_log_dir`, split from the filesystem so it is covered on Windows CI, where the unix branch never runs. The log file opens with `O_NOFOLLOW` alongside `O_CREAT | O_EXCL`, and `start_launch_log` now reports whether the file is ours. `main` drops the path when it is not, so a link that could not be unlinked - the victim cannot write the squatted directory - no longer gets the launch header appended into its target. `skips_launch_log` resolves the subcommand with clap instead of reading argv by hand. `argv[1]` missed `--verbose statusline` and put the per-render path back on the write-and-sweep route; a bare token scan would have misread `usage --provider statusline` and dropped the log for the run that needs one. The sweep caps entries examined, not only removals. The pass over the old temp-root location retires itself with a marker once nothing is left there, so steady-state runs never read_dir a `%TEMP%` holding tens of thousands of files. Changelog says that rather than implying a permanent temp cleanup.
1 participant
Summary
codexbarprocess appended a PID-scoped log under the system temp dir and never removed it.statuslineis invoked once per editor render (SBS-271), so an active session left hundreds to thousands of files.%TEMP%is not reliably purged across reboots.Fix
statuslinewrites no launch log at all. It reads cached state and reports failures through its host, so there is no launch problem a temp file would explain. Checked off raw argv, before clap, sincemainwrites before parsing..write(true).truncate(true)), so a reused PID cannot append across unrelated runs.SUCCESSremoves its own log. Steady state is zero files.metadatais only paid for actual launch logs, not every file in temp.A run that fails still keeps its log, which is the only case worth a post-mortem.
The non-obvious part
Cli::parse()callsstd::process::exititself for--help,--version, and usage errors. That skippedmain's cleanup entirely and stranded a file every time. Caught it in end-to-end testing, not review. Fixed withtry_parse()+ removing the log before deferring toError::exit(), which preserves clap's own output and exit codes.Verification
End-to-end against a scratch
%TEMP%, before and after:statuslinex3--version--helpusage -p claude(exit 0)account switch claude no-such-account(exit 1)Tests
7 tests in
main.rs, 6 new:only_statusline_skips_the_launch_loglaunch_log_names_are_recognized_without_matching_neighboursstarting_a_launch_log_truncates_a_reused_pathsweep_removes_stale_launch_logs_but_spares_mine_and_unrelated_filessweep_keeps_launch_logs_inside_the_age_boundsweep_tolerates_a_missing_directoryTest plan
cargo test --bin codexbar- 7 passedcargo clippy --all-targets -- -D warningscleancargo fmt --checkcleancargo test --lib- 1012 passed, 1 failedNote
Move CLI launch logs to a private per-user cache directory
<cache>/Ceiling/launch-logs/instead of the system temp folder, with the directory created at0o700on Unix and ownership/mode verified before use.statuslinesubcommand skips log creation entirely; successful runs and usage errors delete their log on exit; only genuine failures retain logs.O_CREAT|O_EXCLandO_NOFOLLOWon Unix to prevent symlink attacks, and appending refuses to write through non-regular files or symlinks.Macroscope summarized d217335.