Add BrainLayer newsyslog rotation#299
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@codex review |
|
@greptileai review |
|
@coderabbitai review |
|
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60781526b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| chown "$OWNER:$GROUP" "$log" | ||
| chmod 0644 "$log" |
There was a problem hiding this comment.
When the installer is run as the normal LaunchAgent user, the root-owned replacement-file case described in the README leaves existing logs owned by root/admin, but this loop uses plain chown/chmod under set -e; the first root-owned *.log aborts the script instead of fixing the broken append path. Running the whole script with sudo is not a safe workaround either, because OWNER and LOG_DIR are then derived from the root environment. Use privileged chown/chmod while targeting the invoking user’s log directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Same change: the script no longer relies on root HOME when invoked via sudo, and privileged ownership repair is explicit on each existing log file.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7e049ff21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| mkdir -p "$LOG_DIR" | ||
| sudo mkdir -p /etc/newsyslog.d | ||
| sudo install -o root -g wheel -m 0644 "$SRC" "$DST" |
There was a problem hiding this comment.
When this installer is run by any account other than etanheyman (or with BRAINLAYER_LOG_OWNER/BRAINLAYER_LOG_DIR set), it computes the intended owner and log directory but then copies the literal config unchanged, so newsyslog still watches /Users/etanheyman/... and creates files as etanheyman:staff. The launchd plists are rendered to the invoking user's $HOME, so those users' logs will not be rotated and the root-owned replacement-file problem this script is meant to prevent remains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 99938f7. The installer now renders the checked-in config through a temp file, replacing both the canonical log directory and owner:group with the selected BRAINLAYER_LOG_DIR and OWNER:GROUP before installing /etc/newsyslog.d/brainlayer.conf.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99938f70c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # BrainLayer user LaunchAgent logs. | ||
| # owner:group is explicit so rotated files remain writable by user-level daemons. | ||
| # logfilename owner:group mode count size when flags | ||
| /Users/etanheyman/Library/Logs/brainlayer/brainbar.out.log etanheyman:staff 644 7 1024 * JN |
There was a problem hiding this comment.
For long-running LaunchAgents such as brainbar, watch, drain, and enrichment, their StandardOutPath/StandardErrorPath file descriptors stay open after newsyslog renames the file. The N flag explicitly means no process is signaled, so after a size-triggered rotation these daemons can keep writing to the rotated or unlinked inode instead of the new .log, defeating the rotation and potentially continuing disk growth. This config needs a post-rotate restart/reopen mechanism for the launchd jobs, or it should not rotate held-open logs this way.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93b55d3. The newsyslog drop-in no longer rotates held-open long-running StandardOutPath/StandardErrorPath logs. It now covers finite scheduled jobs only, and the README/config document that BrainBar, watch, enrichment, and drain need a coupled launchd restart or pid-file signal path before they can be safely added.
|
|
||
| sudo mkdir -p "$LOG_DIR" | ||
| sudo mkdir -p /etc/newsyslog.d | ||
| sudo install -o root -g wheel -m 0644 "$RENDERED_CONFIG" "$DST" |
There was a problem hiding this comment.
If a runtime substitution is invalid, for example BRAINLAYER_LOG_GROUP is misspelled, this writes /etc/newsyslog.d/brainlayer.conf before the later chown/newsyslog -nv failure stops the script. Because macOS newsyslog reads drop-ins from /etc/newsyslog.d, the machine is left with a broken system rotation config even though the installer exits unsuccessfully; validate the rendered file and owner/group before replacing the live drop-in.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93b55d3. The installer now validates the rendered temp config with sudo newsyslog -nv before creating/replacing /etc/newsyslog.d/brainlayer.conf, after creating and ownership-validating the target log dir.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93b55d3063
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| OWNER_GROUP_ESCAPED="$(escape_sed_replacement "$OWNER:$GROUP")" | ||
|
|
||
| sed \ | ||
| -e "s#/Users/etanheyman/Library/Logs/brainlayer#$LOG_DIR_ESCAPED#g" \ |
There was a problem hiding this comment.
When the invoking user's home or BRAINLAYER_LOG_DIR contains whitespace, this substitution writes the path into newsyslog.conf as a bare field. newsyslog.conf lines are whitespace-delimited, so a path like /Users/Jane Doe/Library/Logs/brainlayer is parsed as /Users/Jane plus shifted columns, making the installer fail validation instead of supporting the space-preserving home lookup that the new test/README advertise. Please reject such paths with a clear error or render them in a format newsyslog actually accepts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18042edd14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for log in "$LOG_DIR"/*.log; do | ||
| [ -e "$log" ] || continue | ||
| sudo chown "$OWNER:$GROUP" "$log" |
There was a problem hiding this comment.
When this installer is run by an admin for a BRAINLAYER_LOG_OWNER whose log directory already exists or is user-writable, a *.log symlink in that directory is expanded by the glob and passed to sudo chown without -h; the macOS chown(8) docs indicate -h is the option that changes the link itself, so this follows the link and can transfer ownership/chmod of arbitrary files outside the log directory. Please skip symlinks or operate only on verified regular files before doing privileged chown/chmod.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a958d49. The ownership repair loop now skips symlinks and non-regular paths before any sudo chown/chmod, so user-controlled log symlinks are not followed.
| OWNER_GROUP_ESCAPED="$(escape_sed_replacement "$OWNER:$GROUP")" | ||
|
|
||
| sed \ | ||
| -e "s#/Users/etanheyman/Library/Logs/brainlayer#$LOG_DIR_ESCAPED#g" \ |
There was a problem hiding this comment.
When BRAINLAYER_LOG_DIR or the resolved OWNER_HOME contains spaces, this substitution writes the raw path into the config. macOS newsyslog.conf(5) documents that each config line's fields are separated with whitespace, so sudo newsyslog -nv -f "$RENDERED_CONFIG" will parse the path as multiple fields and reject or misconfigure the install despite the added test claiming home paths with spaces are preserved. Render a newsyslog-safe escaped path or reject such paths explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a958d49. The installer now rejects whitespace-containing LOG_DIR values before rendering because newsyslog.conf is whitespace-delimited; README and tests document that explicit limitation.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a0cbf99. Configure here.
| SRC="$SCRIPT_DIR/newsyslog.d/brainlayer.conf" | ||
| DST="/etc/newsyslog.d/brainlayer.conf" | ||
| OWNER="${BRAINLAYER_LOG_OWNER:-${SUDO_USER:-$(id -un)}}" | ||
| GROUP="${BRAINLAYER_LOG_GROUP:-staff}" |
There was a problem hiding this comment.
GROUP variable not validated for whitespace like LOG_DIR
Low Severity
BRAINLAYER_LOG_GROUP is never validated for whitespace or existence. The script explicitly rejects whitespace in LOG_DIR because "newsyslog.conf is whitespace-delimited," but the same reasoning applies to the owner:group field. Unlike OWNER, which is implicitly validated via id -u (which rejects non-existent users), GROUP passes through unchecked. A whitespace-containing value would produce a malformed newsyslog config line where field boundaries shift.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a0cbf99. Configure here.
1 participant
Summary
launchd/newsyslog.d/brainlayer.confcovering BrainLayer LaunchAgent logs under~/Library/Logs/brainlayer/.launchd/install-newsyslog.shto install the drop-in into/etc/newsyslog.d/brainlayer.confand normalize existing log ownership/mode.root:admin/root-owned replacement file footgun and why entries pinetanheyman:staff.Rotation policy
brainbar,watch,drain, maintenance logs: 7 archives, 1024 KB threshold.enrichment: 7 archives, 5120 KB threshold.backup-daily: 4 archives, 512 KB threshold.644, flagsJN, and owner/groupetanheyman:staffso user LaunchAgents can keep appending after rotation.Tests
bash -n launchd/install-newsyslog.shPYTHONPATH=src pytest tests/test_newsyslog_config.py -qruff check src/ tests/ && ruff format --check src/ tests/git diff --checkVerification limitation
newsyslog -nv -f launchd/newsyslog.d/brainlayer.confwas attempted and confirmed macOS requires root privileges.sudo newsyslog -nv -f launchd/newsyslog.d/brainlayer.confcould not run in this non-interactive shell because sudo requires a password/TTY.Note
Medium Risk
Adds a root-run installer that writes to
/etc/newsyslog.dand changes on-disk log ownership/permissions, so mistakes could affect system log rotation or file access. Scope is contained to BrainLayer log paths and includes validation/tests to reduce risk.Overview
Adds a
newsyslogdrop-in (launchd/newsyslog.d/brainlayer.conf) to rotate finite BrainLayer LaunchAgent*.out.log/*.err.logfiles with explicitowner:group, mode644, andJNflags so rotated files remain writable by user daemons.Introduces
launchd/install-newsyslog.shto render the template config for a chosenBRAINLAYER_LOG_OWNER/GROUP/DIR, pre-fix existing log directory/file ownership+mode, validate vianewsyslog -nv, and install into/etc/newsyslog.d/brainlayer.conf.Documents the macOS root-owned rotated file footgun and intentionally excludes long-running/held-open logs; adds Python tests asserting config coverage, exclusions, and installer behavior.
Reviewed by Cursor Bugbot for commit a0cbf99. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add newsyslog log rotation config and installer for BrainLayer LaunchAgent logs
dscl, renders the template config withsed, repairs ownership/permissions on existing log files, validates withnewsyslog -nv, and installs to/etc/newsyslog.d/brainlayer.conf.Macroscope summarized a0cbf99.