feat(builtins): add logrotate#536
Conversation
|
|
@codex review |
![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: 906104e71c
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ 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". |
matt-dz
changed the title
julesmcrt
left a comment
julesmcrt
left a comment
There was a problem hiding this comment.
Shouldn't we split the PR?
The :ro/:rw path-mode suffix feels like it should be a standalone change
|
From Claude review:
openWriteFile applies O_NOFOLLOW to the final path component, so any file that is a symlink (even within a :rw root) fails with ELOOP ("too many levels of symbolic links"). In logrotate's context this is particularly visible: logrotate --dry-run --force /var/log/app.log where app.log is a common → /data/logs/app.log-style managed symlink would print: logrotate: "app.log": too many levels of symbolic links rather than something actionable. This is intentional per the symlink policy, but the error surfaced to the user doesn't say "symlinks are not supported as write targets." Adding a specific check and error before the openWriteFile call would improve diagnosability. Also worth a note in the help text / SHELL_FEATURES.md entry. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f97a64931
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ 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". |
2 participants
Summary
Adds a remediation-only
logrotatebuiltin for rshell that keeps the familiar logrotate intent while staying explicit about its safe subset: it truncates log files to zero bytes throughAllowedPaths, but does not parse configs, rename/retain copies, compress logs, write state files, or run rotate scripts.Key behavior:
--size/-s,--force/-f,--dry-run/-n,--verbose/-v, and--help/-h--sizeor--forcesymlinks are not supported as write targetsAlso extracts shared coreutils-style size parsing for
truncateandlogrotate, wires the new write-capableCallContextfield through remediation mode only, and updates docs/analyzer metadata.Validation
make fmtgo test ./allowedpaths ./interpgo test ./tests/ -run TestShellScenarios -timeout 120sRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120sgo test ./...