feat(builtins): logrotate demo builtin (host-remediation series)#247
feat(builtins): logrotate demo builtin (host-remediation series)#247julesmcrt wants to merge 2 commits into
Conversation
Wraps the existing AllowedPaths Truncate capability behind a logrotate- shaped command intended for agent-driven disk remediation. Supports -h, -s SIZE (threshold), -k N (informational only — sandbox has no rename capability), and -v. Not a substitute for logrotate(8): no config, no rename-based rotation, no compression, no post-rotate scripts. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@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: ee95f8bc94
ℹ️ 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".
Logrotate previously called callCtx.StatFile to gate -s and then callCtx.Truncate, which reopens the path separately — a concurrent swap of a smaller file under the same path between the two calls could fool the threshold check. Add Sandbox.TruncateIfLarger, which opens the path with O_WRONLY, fstats the resulting fd, and ftruncates only when the fd's size is at least minSize. Stat and truncate share a single fd, so the size used for the threshold decision is the size of the inode that is actually truncated. Existing safety properties of Sandbox.Truncate (IsRegular check, write-symlink rejection, EINVAL on negative size) are preserved. Expose the primitive on CallContext, wire it up in interp's two CallContext construction sites, and refactor logrotate to drop the separate StatFile call. Addresses Codex review on PR #247. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b1b308344
ℹ️ 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".
| // on the same fd. This closes the path-stat to path-truncate | ||
| // TOCTOU window — the size used for the threshold decision is | ||
| // the size of the inode that will actually be truncated. | ||
| sizeBefore, truncated, err := callCtx.TruncateIfLarger(ctx, file, threshold, 0, false) |
There was a problem hiding this comment.
When an existing script or agent uses the standard logrotate invocation and passes a config file, this line treats that operand as the log to empty and truncates it. The upstream logrotate(8) synopsis takes config_file [config_file2 ...] operands, so with AllowedPaths covering /etc or a staged config directory, logrotate /etc/logrotate.conf would destroy the configuration instead of parsing it; consider renaming this demo command or refusing config-like operands unless an explicit destructive mode is requested. See: https://man7.org/linux/man-pages/man8/logrotate.8.html
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Opus 4.7] Acknowledged but deferring — this PR is demo-only (won't merge to main) and the destructive-by-default behavior is intentional to keep agent ergonomics close to truncate -s 0 <file>. The logrotate(8) name collision is real; the production resolution would be either a required --in-place opt-in flag (default = dry-run) or a rename to truncate-log. Leaving the thread open so the trade-off is visible.
1 participant
Summary
Adds a
logrotatebuiltin intended for the host-remediation demo. It wraps the existingAllowedPaths.Truncatecapability behind a familiar command name so an agent can free disk space by emptying runaway log files vialogrotate /var/log/app.log. Demo-only — not intended to merge to main.jules.macret/host-remediation/truncate. Re-target tomain(and rebase) if/when that branch lands.-k Nflag is recorded for-vreporting only — the sandbox has noRenamecapability today, so prior copies are not retained. This trade-off is documented in the package comment,SHELL_FEATURES.md, and the--helpoutput.Flags
-h,--help-s SIZEK/M/G/T(1024-based, leading letter case-insensitive) orKB/MB/GB/TB(1000-based)-k Nlogrotate(8)retains N rotated copies; here it is only echoed under-v-vlogrotate: PATH: truncated N bytes (keep=K)or... below threshold N, skippingWiring
builtins/logrotate/logrotate.go— implementationinterp/register_builtins.go— command registrationanalysis/symbols_builtins.go— per-command symbol allowlist (context.Context,errors.New,math.MaxInt64,strconv.ParseInt)SHELL_FEATURES.md— user-facing feature lineSandbox-safety properties
callCtx.TruncateandcallCtx.StatFile; no directosfilesystem calls.ftruncateto size 0.-s,-k) bounds-checked;parseSizerejects overflow viaMaxInt64 / multguard.Test plan
go build ./...— cleango test ./builtins/... ./interp/... ./analysis/... -timeout 120s— all greentests/scenarios/cmd/logrotate/(basic, threshold above/below, verbose truncated/skipped, help, errors: missing operand / invalid size / missing target) — all pass viago test ./tests/ -run TestShellScenarios/cmd/logrotatelogrotate -v -k 3 -s 10 <path>throughcmd/rshell --allowed-paths $TMPDIR, confirmed the file was truncated to 0 and the verbose line matched expectationgo run ./cmd/rshell→help logrotateprints the registered help textRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash) — N/A; every logrotate scenario setsskip_assert_against_bash: truebecause logrotate is not present in the bash comparison image and would always fail