feat(builtins): add free builtin for host memory pressure verification#555
Conversation
Reports total/used/free/shared/buff-cache/available memory and swap usage via /proc/meminfo on Linux (free, free -h only, per the accepted candidate scope). macOS and Windows report "not supported" rather than a partial breakdown, since neither exposes buffers/cache/shared through a syscall this shell can call without cgo.
humanBytes pre-rounded with a custom round-half-away-from-zero helper,
which silently diverges from procps-ng's plain printf("%.1f") at exact
decimal ties (e.g. 1310720 bytes is exactly 1.25Mi: old code printed
1.3Mi, real free prints 1.2Mi). Format directly with fmt.Sprintf
instead, which applies the same round-half-to-even Go and C share.
Also drops a doc comment referencing a meminfo_windows.go that was
never added, and adds direct unit tests for writeOutput's derived
columns (used-memory underflow fallback, swap row, human mode), which
were previously only reachable via the Linux-gated integration test.
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@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: 41ee6d63ce
ℹ️ 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".
used now follows GNU free's actual MemTotal-MemAvailable formula instead of the naive Total-Free-BuffCache subtraction, which under-reports used memory under cgroup/container accounting. Human-readable values >= 10 now truncate instead of round, matching procps-ng's scale_size (e.g. 65.926 GiB prints "65Gi", not "66Gi").
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3781d64134
ℹ️ 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".
Truncating human-readable sizes >= 10 with math.Trunc + strconv.FormatFloat fails the builtin symbol allowlist check (analysis/symbols_builtins.go). Use a uint64 conversion (also truncates toward zero, val is always in [10, 1024)) with the already-allowlisted strconv.FormatUint instead.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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
What does this PR do?
Adds a
freebuiltin that reports host memory and swap usage (total/used/free/shared/buff-cache/available), scoped tofreeandfree -hper the accepted-candidate design brief. Backed by a newbuiltins/internal/meminfopackage that reads/proc/meminfoon Linux, bypassingAllowedPathsthe same wayss/ip route/dfalready do (hardcoded kernel pseudo-file path, never derived from user input). macOS and Windows intentionally return "not supported" rather than a partial implementation — neither exposes the buffers/cache/shared breakdown through a syscall this shell can call without cgo, and reporting those columns as a literal0would misrepresent "not available" as "none."Motivation
Item from the rshell command roadmap: "Host-level memory pressure verification" (
free), listed as "no gate needed." Per the design brief onorigin/alex/rshell_cmds_triage:candidates.md, this is the first-pass host-memory snapshot used at the start and end of memory-leak investigations, complementingps(per-process memory) and the plannedvmstat(pressure trends over repeated samples — explicitly out of scope forfree).Researched past PR review comments on
df/ss/ip route/ps/logrotatebefore implementing, to avoid repeating prior mistakes (sandbox-bypass documentation gaps, integer overflow in derived columns, pflag explicit-value handling, help-flag correctness, Windows struct-offset bugs). SeeAGENTS.md's Security Design Decisions and Testing sections for thefree-specific entries this PR adds.Testing
builtins/internal/meminfo/meminfo_linux_test.go: fixture-based parser tests (happy path,MemAvailablefallback for pre-3.14 kernels, malformed/unitless line skipping, overflow, context cancellation, oversized-line handling).builtins/free/free_internal_test.go:humanBytes(including exact round-half-to-even decimal ties),writeOutput(including the cgroup-underflow fallback branch for theusedcolumn).builtins/free/free_test.go: help/error paths (cross-platform), plusruntime.GOOS-gated happy-path/not-supported checks.tests/scenarios/cmd/free/: help + error-path scenarios (skip_assert_against_bash: true—freeships inprocps-ng, not installed in thedebian:bookworm-slimcomparison image).code-reviewskill +codex exec reviewin parallel; one real bug found and fixed (custom rounding in-hdiverged fromfree's actual round-half-to-even at exact ties) and one Codex finding investigated and rejected as factually incorrect (see commit41ee6d63).go build/go vet/go test ./...clean on darwin; cross-compiled and vetted clean forlinux/amd64andwindows/amd64. Could not exercise the Linux happy path against a live container in this environment (Docker Desktop here requires an org sign-in) — worth a sanity check on a real Linux box or in CI.Checklist
README.md,SHELL_FEATURES.md,AGENTS.md)