Conversation
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 19ff304
Right diagnosis and the right lever. memory.oom.group=1 makes the cgroup limit a collective death sentence, so bounding the container harder would not have helped — bounding each child is the only thing that turns a group kill into one child's ENOMEM. The RLIMIT_DATA-over-RLIMIT_AS reasoning is correct and worth the comment it got: V8's pointer-compression cages make any -v value that leaves node runnable useless as a ceiling, while -d counts the writable private anonymous mappings that actually fill the cgroup.
I confirmed the delivery mechanism in a live agent pod rather than taking it on faith. The Bash tool here spawns /usr/bin/zsh, zsh 5.9, non-interactive ($- = 569X, $0 = /usr/bin/zsh), with ulimit -d currently unlimited. So ZDOTDIR/.zshenv is the load-bearing path — and zsh reads .zshenv on every start, interactive or not, which is exactly the property this needs. BASH_ENV is defence in depth for anything that shells out to bash. The choice to stub all five zsh dotfiles and chain each to $HOME is what keeps that safe.
Two things I checked because they would have broken every tool call in the pod: the runtime-cache volume and both of its mounts are unconditional (job-manifest.ts:1601, :1611, :2151), so the unguarded . '<rlimit.sh>' in the .zshenv stub can never source a missing file; and resources.limits.toolMemoryKb is validated digits-only before interpolation, with "1048576; rm -rf /" and "$(id)" pinned as throws.
On whether half the pod limit can starve legitimate tool work — measured, not guessed: no, not on this fleet. All 22 live agent pods run claude=8Gi, so the default cap is 4 GiB per child against a 1536Mi request. A large npm ci does not approach 4 GiB. The starvation risk is real only for agents configured well below the 8Gi default, and that case has a sharper problem than starvation — see the first Suggestion.
Critical Issues (0)
None.
Important Issues (1)
- [gstack/review]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1314— a fractionalresources.limits.memorynow makes the Job manifest unbuildable, so the agent cannot launch at all.resolveToolMemoryLimitKbparses the container memory limit throughMEMORY_QUANTITY_RE(:1271), which admits only^[0-9]+(unit)?$; the call site at:2139is insidebuildJobManifestwith nothing catching it, so the throw aborts the build rather than the cap.1.5Giis a legal Kubernetes quantity — confirmed withkubectl create --dry-run=client, which round-trips it unchanged. An agent configured1.5Gi(or0.5Gi,2.5Gi) launched fine before this PR and cannot launch after it, with an error namingresources.limits.memoryrather than the tool cap that actually rejected it.job-manifest.test.tspins this as intended (expect(() => resolveToolMemoryLimitKb({}, "1.5Gi")).toThrow), so it is a decision, not an oversight — but the blast radius is the whole agent, not the feature.- Nothing on the fleet trips it today: every live agent pod is
8Gi, which is why this is Important rather than Critical. It is a latent config landmine, and the operator who plants it gets a total launch failure for a value Kubernetes accepts. - The cap is an optimisation; the manifest is the run. Suggest degrading instead of aborting — treat an unparseable container limit as "no cap" and warn, which matches the fail-open you already chose for
ulimit -d ... 2>/dev/null || trueand the readable-file-when-disabled path. Alternatively accept a decimal and floor it. Either way keep the hard throw ontoolMemoryKbitself: that value is interpolated into a shell command and must stay digits-only. The two fields differ in exactly that respect, and the current code treats them alike.
Suggestions (3)
- [native-codex]
job-manifest.ts:1314— the "half the limit" derivation's safety argument holds only while the limit is more than twice the request. The stated invariant is cap + baseline < limit; with cap = limit/2 that reduces to baseline < limit/2, and the default request is1536Mi(:1549). So for any agent with a memory limit at or below ~3Gi the group kill is still reachable by a single capped child, and the default cap does not deliver what the comment promises. Fine at the 8Gi default; worth either a floor on the derived cap or a sentence narrowing the claim, since small-limit agents are precisely the ones a halved cap also starves. - [pr-review-toolkit]
job-manifest.ts:1086—BASH_ENVis read by bash only on non-interactive start, and there is no.bashrc-equivalent stub alongside the five zsh ones. Harmless today (the tool shell is non-interactive zsh, verified above), but if the image or the harness ever moves to an interactive bash the cap stops applying silently, with no signal anywhere — the same shape as the failure this PR is fixing. A one-line comment recording why the asymmetry is acceptable would make that regression visible to whoever makes that change. - [gstack/review] The fix's own success signal goes quiet. BLO-34477 was detectable because OOMKills are a structured, countable pod-level event ("12 pods >5 GiB in 24 h"); after this, the same runaway surfaces as an
ENOMEMinside a tool result — unstructured text in a run log. That is a strictly better outcome for the run and a strictly worse one for measurement, and it means "did this work?" cannot be answered from the same dashboard that raised the question. Worth deciding now whether anything should count capped-child failures.
Strengths
- The generated shell is executed, not just string-matched: the tests install it into a real temp dir with
/bin/shand then ask each shell what its RLIMIT_DATA is, including a grandchild, which is the property that actually bounds the subtree.sh -nsyntax-checks the assembled init command too. - The negative control is the best test in the file —
POSIX sh -c(the shape that launchesclaude) is asserted to ignoreBASH_ENVand stay at baseline. Without it the PR could have cappedclaudeitself and nobody would have known until a large run died differently. hostCanLowerRlimitDataprobes the host rather than branching onprocess.platform, so a Linux box with an unusual hard limit skips honestly instead of failing for an unrelated reason. That is the right way to write a platform-conditional test.- The emptyDir-over-
$HOMEreasoning is documented with the three concrete HOME shapes that defeat the obvious approach (shared PVC, per-run runtime-cache, customworkspaceMountPath), and buys idempotency for free: per-pod, rewritten every run, no marker, no stale value to un-append, nothing to race on. BASH_ENVandZDOTDIRare classifiedSAFE_LITERALwith reasons stating they are paths carrying no credential material, and the config field is placed adjacent to the limit it derives from with the adjacency pinned by a test.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
Findings addressedprior:19ff304 important 1: fixed in 78d0008 — |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fac058c
Note on the head. You asked for 78d000863. The branch moved twice while I was reading — 78d000863 at 22:46Z, then the origin/master merge fac058ce8 at 23:07Z. This review is bound to fac058ce8, the live head. I re-read the fix across that merge before attesting: job-manifest.ts is byte-identical between the two, so the change you asked me to look at is the change I read.
That merge also resolved something I was about to raise. At 78d000863 this PR bumped the adapter 0.2.6-blockcast.9 → .10, and master already publishes .10. Because both sides move the same line to the same literal, git auto-merges it with no conflict — the bump would have silently no-op'd and this PR's code would have shipped under a version master already used for different content. At fac058ce8 the version reads .12 (leaving .11 to #1936) with the PROVENANCE manifest hash recomputed — all four hashes I compared (78d000863, fac058ce8, master, #1936's head) are distinct, so nothing was copy-pasted forward. This is the concrete form of Suggestion 3 from my 19ff304f0 review, and it is handled.
Prior Findings Dispositioned (1)
- prior:19ff304 important 1 — fixed —
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1323-1331— the derivation is now wrapped intry/catch: an unparseable container limit warns and returns0(no cap) instead of throwing out ofbuildJobManifest. This is the degrade-not-abort shape I suggested, and it keeps the part that had to stay strict — the explicittoolMemoryKbthrow survives at:1341, so the one value that is interpolated into a shell command is still digits-only. The two fields are no longer treated alike, which was the actual defect.1.5Ginow yields a launchable Job; the test that previously pinned the throw was rewritten to pin the warning, and a second test pins the survivingtoolMemoryKbthrow withwarningsasserted empty — so the two paths cannot be collapsed again without a failure.
Three things I checked because a "degrade to 0" path is only safe if 0 is genuinely safe downstream. buildToolRlimitInitShell (:1367) writes a valid # cap disabled file rather than skipping the write, so BASH_ENV still points at a readable file and no tool shell breaks on a missing source target. The catch wraps only the parse-and-halve expression, so it cannot swallow an unrelated failure. And resolveToolMemoryLimitKb has exactly one caller repo-wide (:2157), which passes containerResources.limits?.memory ?? "" — the "" branch is effectively unreachable because the limit defaults to "8Gi" at :1570, but if it ever were reached it now degrades instead of aborting, which is the better of the two behaviours.
Suggestions 1–3 from the 19ff304f0 review are unchanged by this delta and remain open; I am not re-listing them.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (4)
- [gstack/review]
job-manifest.ts:1367— in the degraded path the on-disk artifact states the wrong cause.limitKbis0, sorlimit.shreads# cap disabled (resources.limits.toolMemoryKb=0)— but the operator never settoolMemoryKb; an unreadableresources.limits.memorydid it. Whoever execs into an uncapped pod to ask "why is there no cap here?" reads a false answer and goes looking at the wrong field. Passing the reason through to the comment would make the file self-explaining. - [pr-review-toolkit]
job-manifest.ts:1291—parseMemoryQuantityToKiB's docstring still says a fractional quantity "is an operator error worth surfacing at manifest-build time rather than rounding silently." That is now false for its only caller, which catches it and warns. The function is unchanged and correct; the comment describing the policy is the thing that moved. Worth one line, because this file's comments are unusually load-bearing — my entire read of the RLIMIT_DATA-over-RLIMIT_AS reasoning came from them. - [native-codex]
job-manifest.ts:1319—console.warnis the only console call in this 2,300-line file, so the warning has no established destination: it lands in the adapter process's stdout, not the agent run log or the operator's Job view. The whole value of degrading rather than aborting is that the operator finds out they are running uncapped — the loud failure was replaced with a quiet one, which is right, but the quiet one still has to be findable. Worth deciding whether this should be a lifecycle event like the other operator-visible signals in this adapter. - [gstack/review]
config-schema.ts:143—resources.limits.toolMemoryKbis declaredtype: "number", so the UI renders a numeric input, and a decimal entered there (1.5) still hard-throws at manifest-build:Number.isSafeInteger(1.5)is false at:1340. That is the same total-launch-failure blast radius as the finding just fixed, on the sibling field. I am filing it as a Suggestion rather than an Important on purpose:1.5Giis a completely ordinary way to write a memory limit, whereas a fractional count of KiB is not something an operator plausibly types, so the trigger is far less reachable. Keeping the throw is right — this value reaches a shell. The timing is what is off: rejecting it when the config is saved would be strictly better than rejecting it when the Job is built.
Strengths
- The fix takes the narrower of the two options I offered. Accepting decimals and flooring them would have widened
MEMORY_QUANTITY_RE, which is shared with the shell-interpolated path; degrading at the call site leaves the parser's strictness intact and confines the new leniency to the one caller that can afford it. - The new tests inject a
warncollector rather than spying onconsole, and assert the message's three load-bearing parts separately (the offending value, "no RLIMIT_DATA cap", and thetoolMemoryKbremedy). The second test assertswarningsis empty on the throw path, which is what stops a future refactor from degrading the shell-interpolated field by accident. - The docstring was rewritten rather than left behind, and it states the reasoning that makes the asymmetry defensible — the container limit "was already valid before this cap existed and the cluster still enforces it." That is the correct argument: this PR added a consumer of that field, and a new consumer should not retroactively invalidate configurations the platform already accepts.
- The version collision was resolved to
.12rather than.11, i.e. the author checked what #1936 had taken rather than assuming the next integer.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f6a323e
Test-only delta on top of the fac058ce8 I approved: job-manifest.test.ts (+24/−3) and the PROVENANCE hash. git diff across the two heads touches no production file, the adapter holds at 0.2.6-blockcast.12, and the hash moved 8aadf7a3… → c98d9025… — distinct from every other head in this pair's history (19ff304f0 00ca72a9…, master 7a91abbd…, #1936's 3a0199316 8c3a5f3d…), so nothing was copy-pasted forward. The Vendored claude_k8s adapter lane — the one that was red at fac058ce8 — is success at this head, and that job is what recomputes and enforces the PROVENANCE hash, so the hash is machine-verified rather than asserted.
The skip guard is built the right way round, and that is the part worth calling out. The probe asserts a plain variable assignment through BASH_ENV, not a ulimit. That orthogonality is what makes it a guard rather than a tautology: a probe that used the same mechanism as the assertion could never let the assertion fail. Here, probe-true + assertion-fail is still a genuine red — including the case the || true in rlimit.sh would otherwise swallow, where bash does source the file but the ulimit inside it silently doesn't stick. bashDiag's hard= and uid= fields are exactly what you'd need to chase that. The separation is correct.
I read the green run's log rather than assuming the skip did or didn't fire, and it has already returned a result (run 35478202379, job 105991072346, 00:16:50Z):
[BLO-34477 test] this host's bash does not source BASH_ENV
(bash=5.2.21(1)-release uid=1001 euid=1001 gid=1001 egid=1001 path=/usr/bin/bash);
skipping the bash-under-BASH_ENV assertions
That is a useful answer, and it is not the answer the code comment predicts — see the first Suggestion. The same test's /bin/sh half passed on that host in the same run (228 tests | 1 skipped, and the skipped one is elsewhere), which narrows it hard: the file is readable, the path is fine, and hostCanLowerRlimitData was true so RLIMIT_DATA lowering is permitted there. The only thing not working on arc-light is bash's BASH_ENV sourcing itself.
I am deliberately not raising this as blocking. Before this change the bash assertion failed on arc-light; it never passed there. So this converts a red lane into an honest skip plus a diagnostic — it does not remove coverage that was working, and no production code moved. The coverage question is real but it is a follow-up, not a gate.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (3)
- [pr-review-toolkit]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.test.ts:3084-3088— the comment names three causes and the run above excludes all three, so as written it will mislead the next reader. Not a non-GNUbashon PATH:BASH_VERSION=5.2.21(1)-releaseat/usr/bin/bash. Noteuid != uid:1001/1001, andgid/egidmatch too. Not POSIX mode:ulimitDbuilds a minimal env ofPATH/HOME/BASH_ENVonly — noPOSIXLY_CORRECT— and invokes the binary asbash, so that path is unreachable by construction. Worth replacing the speculative list with what was measured plus an explicit "cause not yet identified", because the honest state is unexplained, and a comment that reads as understood is what stops anyone looking. The wake note that the identical commands succeed asrunnerand as root inside the runner image is the useful next clue: same image, different result, so the variable is the arc-light host/runtime rather than the image — worth writing down before it is lost. - [gstack/review]
job-manifest.test.ts:3086— the skip is now unconditional in practice on the only host that runs this in CI, and the sole signal is aconsole.warninside a green job. Nothing fails, nothing counts it, andBASH_ENVis a production delivery path for the cap. Recommend a tracking issue owning "does GNU bash honourBASH_ENVon arc-light, and if not why" so this does not quietly settle into permanent non-coverage. Cheap insurance given the zsh.zshenvpath is the load-bearing one — I confirmed in a live agent pod that the Bash tool spawns/usr/bin/zsh5.9 non-interactive — which is what makes this defence-in-depth rather than the primary mechanism, and therefore easy to forget. - [native-codex]
job-manifest.test.ts:3078—ulimitDno longer does what its name says: the probe uses it to captureprintfoutput, andbashDiagto capture a composite diagnostic string. It is now a generic run-and-capture-stdout helper, so a name likestdoutOfwould stop the double-take at the call site. Related and smaller: the!which("bash")early return warns nothing, so "bash absent" and "bash present but ignoringBASH_ENV" are indistinguishable in a log — immaterial on this runner, which has bash, but a secondconsole.warnthere would keep the two apart on a future host.
Strengths
- The probe is independent of the mechanism under test. That is the hard part of writing a skip guard, it is easy to get backwards, and it is right here.
- Skipping loudly with the evidence rather than
it.skip-ing the test or deleting the assertion. The diagnostic is the deliverable, and it paid for itself on its first run by refuting the hypothesis it was written to confirm. - The unconditional assertions are untouched:
/bin/shsourcing the file still reports the cap, and the "POSIXsh -cignoresBASH_ENVsoclaudeitself stays uncapped" test still pins the deliberate asymmetry the whole design rests on. bashDiagattachinghard=alongside the soft limit is the right field to capture — it is what distinguishes "not sourced" from "sourced but the lowering was refused", which the production2>/dev/null || trueis otherwise designed to hide.- Version and provenance discipline held across a second revision:
.12retained,.11still reserved for #1936, hash recomputed and CI-verified.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
Hold before merge — the CI failure at fac058c was a real delivery gap, not a runner quirk. The arc-light lane's Mechanism: bash's So: the vendored test now skips honestly on the runner, but production delivery through
Not merging at f6a323e. Ally's approval at this head covers the test change only; the delivery fix needs its own review. |
Superseded at f6a323e: this approval was issued before the BASH_ENV delivery gap was understood. Re-review at the same head found 2 Critical findings (the BASH_ENV arm does not reach a Node-spawned bash; the test guard that would have caught it is unreachable on every host). Dismissing so a stale green does not outrank the blocking review posted immediately after this.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f6a323e
Re-review on the merge hold. I approved this head earlier today; that approval was wrong and I have dismissed it. @kkroo's diagnosis reproduces exactly, and the guard that hid it is the reason my prior pass came back clean. One correction to the hold comment's scope, in the PR's favour, in Critical 1.
Critical Issues (2)
-
[native-codex]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1233— theBASH_ENVdelivery arm does not reach a bash spawned by Claude Code, so for a bash tool shell the cap is advertised but never applied. The comment here ("bash reads$BASH_ENVon every non-interactive start") and its twin at:1080state the mechanism as unconditional; it is not. bash'srun_startup_files()takes a "run by rshd/sshd" branch whenisnetconn(fileno(stdin))andSHLVL < 2, sourcing~/.bashrcand returning beforeBASH_ENVis consulted. libuv allocates child stdio pipes withsocketpair(), sogetpeernamesucceeds on fd 0 and every default-stdio bash takes that branch.-
Reproduced in the agent image itself (bash 5.2.37, Node v24.16), three spawns differing only in stdio and
SHLVL:stdio default (socketpair) ulimit=unlimited shlvl=1 fd0=socket:[387399077] stdin ignore (/dev/null) ulimit=1048576 shlvl=1 fd0=/dev/null stdio default + SHLVL=2 ulimit=1048576 shlvl=3 fd0=socket:[387399086] -
Scope correction — narrower than "the cap never applies", and this bounds the urgency. This container's tool shell is zsh, not bash:
SHELLis/usr/bin/zsh, and the live Bash-tool child reports/usr/bin/zsh,SHLVL=1,fd0=socket:[387447078]. zsh has no rshd heuristic and sources$ZDOTDIR/.zshenvunconditionally, and RLIMIT_DATA is inherited, which I measured with socket stdin:zsh -c→1048576,zsh -c 'bash -c ulimit -d'→1048576,zsh -c 'sh -c ulimit -d'→1048576. So on the current image the orphaned Bash-tool child this PR targets is capped through the ZDOTDIR arm, and any bash beneath the tool shell inherits it. What is dead is a bash spawned directly by Claude Code — i.e. any image or config whose tool shell is bash. That is a silent, config-dependent hole in a memory-safety control, andconfig-schema.ts's hint ("every shell the agent spawns") is false while it stands. -
On the three options: option 2 (
SHLVL=2in the claude container env) is the smallest verified fix — one variable, no$HOMEdependency, and it closes precisely this branch (row 3 above). Option 1 works too (I confirmed~/.bashrcis sourced on the rshd branch and applies the cap), but it reintroduces exactly what:1241-1247argues against: HOME may be a shared PVC path, a per-run path, or invisible to the init container. Whichever you pick, please keep the emptyDir file as the single source and treat the second path as delivery only — and call theSHLVL/isnetconncondition out in the comment, because the next reader will otherwise delete the "redundant" arm.
-
-
[pr-review-toolkit: tests]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.test.ts:3089—bashHonorsBashEnv()is itself subject to the bug it is guarding against, so the bash assertions are skipped on every host, permanently, and the guard converts the Critical above into a green test. This is not a weak test; it is the detector for this exact defect wired to always report clean, and it is why myfac058ce8andf6a323e9approvals came back clean.- Deterministic, not host-dependent: the probe runs through
ulimitD(:3080), which callsspawnSyncwith default stdio — socketpair stdin — and rebuilds the child environment from scratch as{ PATH, ...overrides }, soSHLVLis never inherited and the child always seesshell_level = 0. Both branch conditions are therefore satisfied on every POSIX host, soapplied === "applied"can never hold and the assertions at:3107and:3110are unreachable. Confirmed by running the helper's exact shape in this container:bashHonorsBashEnv() -> FALSE; the identical probe with a~/.bashrcpresent returns"applied", which isolates the cause to the rshd branch rather than to bash version, uid/euid, or PATH. - The in-source explanation at
:3083-3084names three causes —euid != uid, POSIX mode, a non-GNUbashon PATH — and the real cause is none of them, so the comment will actively mislead whoever revisits this. The arc-lightunlimitedreading it attributes to a runner quirk was the production defect surfacing. - Suggested shape: drop the conditional and assert the production spawn shape (default/socket stdin, no
SHLVLoverride) directly. That test fails today and passes once delivery is fixed, which is what you want from it. KeephostCanLowerRlimitData— that one probes a genuine kernel property (DarwinsetrlimitEINVAL) and skips honestly.
- Deterministic, not host-dependent: the probe runs through
Important Issues (0)
None.
Suggestions (3)
- [pr-review-toolkit: comments]
vendor/paperclip-adapter-claude-k8s/src/server/config-schema.ts:141— the hint promises the cap applies to "every shell the agent spawns"; that is true of zsh today and of bash only after Critical 1. Worth narrowing the wording or landing the fix before the string ships to operators. - [pr-review-toolkit: tests]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.test.ts:3100—bashDiagroutes throughulimitD, so its ownulimit -dreading is taken on the rshd branch. Harmless as a diagnostic, but it will printunlimitedfor a correctly-capped configuration and mislead the next debugger. - [gstack/review]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1234— once a second delivery path lands, this block is the natural home for a one-line note thatsh/dash reads neither variable and that bash consultsBASH_ENVonly when stdin is not a connected socket. The RLIMIT_DATA-vs-RLIMIT_AS rationale just below is a good model for that.
Strengths
- The probe added at
f6a323e9is what turned an "arc-light flake" into a reproducible production defect. Holding your own PR on the strength of it, rather than merging behind a green approval you already had, is the right call and is why this is caught before the fleet depends on it. - RLIMIT_DATA over RLIMIT_AS, justified by V8's ~73 GiB pointer-compression reservation against <1 GiB resident, is correct and non-obvious —
-vwould have brokenclaudeoutright. - The emptyDir-over-
$HOMErationale at:1241-1247(shared PVC, per-run paths, customworkspaceMountPath) is thorough, and the ZDOTDIR stubs chaining to the user's own$HOME/.z*files mean nothing an agent relies on is lost. - The fractional-memory finding from
19ff304f0was fixed in the right shape: degrade-and-warn for the derived limit, hard throw retained for the explicittoolMemoryKbthat is interpolated into a shell command, with both paths pinned by tests so they cannot be collapsed again. - The ZDOTDIR arm genuinely works under the production spawn shape, including grandchild inheritance — measured above. Half of this design is delivering today.
Recommended Action
- Fix Critical issues before merge.
- Consider Suggestions opportunistically.
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6eba574
Both Criticals from my f6a323e9 re-review are fixed, and I verified the mechanism rather than the diff. In a container on this image (bash 5.2.37), spawning through Node with default stdio — the production shape, socket fd 0 — with a BASH_ENV file holding ulimit -d 1048576:
BASH_ENV, no SHLVL -> unlimited shlvl=1 fd0=socket:[393263679]
BASH_ENV + SHLVL=2 -> 1048576 shlvl=3 fd0=socket:[393263688]
BASH_ENV + SHLVL=1 -> 1048576 shlvl=2 fd0=socket:[393263697]
The arm is alive. The test rewrite is the better half of the change: deleting a probe that could only ever return false, and replacing it with an unconditional assertion plus a negative case that fails if the manifest arm is removed, is exactly the shape that would have caught the original defect.
One new Important, and it is a consequence of the fix rather than a pre-existing issue — SHLVL=2 changes which startup file bash reads, not just whether it reads one. Detail below; the fix is one line and mirrors what this PR already does for zsh.
Prior Findings Dispositioned (2)
- prior:f6a323e critical 1 — fixed —
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1105—merged.SHLVL = "2"ships alongsideBASH_ENV/ZDOTDIRin the claude container env, andSHLVLis classifiedSAFE_LITERALat:689so the BLO-29804 gate admits it. Measured above: the identical spawn goesunlimited→1048576with the variable present. bash incrementsSHLVLbeforerun_startup_files()evaluatesshell_level < 2, so2clears the rshd/sshd branch with margin and$BASH_ENVis consulted. The comment at:1092-1102states the condition, the libuvsocketpair()cause and the measurement, so the arm reads as load-bearing rather than deletable. - prior:f6a323e critical 2 — fixed —
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.test.ts:3120-3140—bashHonorsBashEnv()is gone. The gate is nowitWithBash = which("bash") ? itOnCapableHost : it.skip(:3120), i.e. binary availability plus the genuine kernel-capability probe, with no mechanism-dependent self-veto. The positive assertion at:3122runs unconditionally throughbashEnvArm()(:3106), which carriesSHLVL: "2"with a comment tying it to the manifest; the negative case at:3136pins that the same spawn withoutSHLVLdoes not get the cap, so deletingmerged.SHLVLreddens the suite. The in-source comment at:3096-3105replaces the three wrong causes with the real one and explicitly tells the next reader not to reintroduce a host probe.
Critical Issues (0)
None.
Important Issues (1)
- [native-codex]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1105— clearing the rshd branch also stops bash from sourcing~/.bashrc, andrlimit.shdoes not chain back to it. On a shared-HOME agent that silently drops the pod's environment file from any directly-spawned bash.-
The rshd branch is not merely "skips
BASH_ENV" — it sources~/.bashrcinstead. SoSHLVL=2does not add a startup file, it swaps one for the other.buildToolRlimitInitShellchains every zsh stub to the user's own dotfile (:1390chain(), applied acrossZSH_DOTFILESat:1395), butrlimitFile(:1384) is the bareulimit -dline with no$HOME/.bashrcchain, andBASH_ENVpoints straight at it (:1103). -
Measured in this container against the real file,
HOME=/paperclip, same spawn shape as above:today: no BASH_ENV, no SHLVL -> ulimit_d=unlimited CCROTATE_SERVE_BASE_URL=PRESENT JAVA_HOME=PRESENT BASH_ENV only (pre-SHLVL fix) -> ulimit_d=unlimited CCROTATE_SERVE_BASE_URL=PRESENT JAVA_HOME=PRESENT this PR: BASH_ENV + SHLVL=2 -> ulimit_d=1048576 CCROTATE_SERVE_BASE_URL=ABSENT JAVA_HOME=ABSENT -
/paperclip/.bashrcis a real 2210-byte file that setsANTHROPIC_BASE_URL,CCROTATE_SERVE_BASE_URL,CCROTATE_SERVE_ANTHROPIC_BASE_URL,OPENAI_BASE_URL,CODEX_HOME,JAVA_HOME/ANDROID_HOME, sources~/.cargo/envand~/.config/ccrotate-serve/env, and prepends$HOME/bin,$HOME/.local/binand/paperclip/jdk/bintoPATH. None of those names appear in the claude container's manifest env, so for a bash that loses the file they are gone rather than re-supplied. LosingANTHROPIC_BASE_URL/CCROTATE_SERVE_*is the one I would not want to discover in production: something shelling out toclaudeorcodexfrom bash stops pointing at the rotation proxy. -
Live-reachable, and its reachability is the same argument the SHLVL fix rests on. Of 9 pods currently running a
claudecontainer, 2 haveHOME=/paperclip(ac-29033747-…,ac-b4fe9f95-…); the other 7 are isolated roots with no.bashrc, where nothing is lost. The affected population is any bash spawned directly by Claude Code or a hook — SHLVL unset, socket stdin. A bash nested under the zsh tool shell already seesSHLVL≥1and is unaffected either way. If that population is empty thenmerged.SHLVLis itself a no-op, so the change cannot be justified and this dismissed at the same time. I did not enumerate which components spawn bash directly in a shared-HOME pod; I verified only that a Nodespawn("bash", …)reproduces it exactly. -
Suggested fix, same shape you already chose for zsh: point
BASH_ENVat a bash-specific stub rather than atrlimit.sh—. '<dir>/rlimit.sh'followed bychain(".bashrc"). It has to be a separate file: the.zshenvstub and the POSIX-shtest both sourcerlimit.shdirectly, so chaining.bashrcinside it would wrongly pull a bash rc into zsh. That restores the invariant the comment at:1262already claims for zsh — the agent keeps the environment it would have had withHOMEalone, plus the cap.
-
Suggestions (3)
- [pr-review-toolkit: tests]
job-manifest.test.ts:3139— the negative case asserts.not.toBe(String(CAP_KB)), which also passes when bash fails to start andstdoutis"". It is the test protectingmerged.SHLVL, so it is worth making non-vacuous. The neighbouring POSIX-shtest at:3145already has the better pattern: capture the uncapped baseline and assert equality against it, so "unchanged" is distinguishable from "no output". - [gstack/review]
job-manifest.ts:1093— the comment states the branch condition asSHLVL < 2and the code ships2, which reads as an off-by-one to anyone who does not know bash incrementsSHLVLduring initialisation before the test is evaluated. Measured above,SHLVL=1also clears it (shlvl=2in the child). One clause — "bash increments before the test, so 1 would clear it; 2 for margin" — stops the next reader from- 1-ing it. - [pr-review-toolkit] The still-open Suggestions from the
19ff304f0,fac058ce8andf6a323e9reviews are unchanged by this delta and I am not re-listing them: theconsole.warnwith no established destination, the degraded-pathrlimit.shcomment namingtoolMemoryKbwhen an unparseableresources.limits.memorywas the cause, thetoolMemoryKbdecimal rejected at manifest-build rather than at config-save, and the fact that a capped child now surfaces as unstructuredENOMEMtext rather than a countable pod-level OOMKill. The last one is the one I would still like a decision on, since it is how anyone answers "did this work?".
Strengths
- The test fix is the substantive half. Replacing a probe that returned false on every POSIX host with an unconditional assertion on the production spawn shape converts a permanently-green detector into one that fails today if delivery breaks — and the added negative case means the manifest arm cannot be deleted as a redundant-looking assignment, which is precisely how it would have been lost.
- Mutation-testing both halves and saying so in the commit message (remove the test env arm → behavioural case fails; remove
merged.SHLVL→ manifest case fails; baseline green) is the check that distinguishes a regression test from a restatement. A fixture written for a defect can pass without the fix; this one was shown not to. - Reproduced on a second host at a different bash version (5.2.21 vs the image's 5.2.37) rather than only where it was found, so the conclusion is about bash's startup rules and not about one image.
- The
config-schema.ts:145hint was narrowed honestly rather than left aspirational — it now names bash and zsh specifically, says descendants inherit, and calls out that a baresh -coutside a tool shell gets nothing. That is the operator-facing half of the same correction. - The PROVENANCE row records the defect, the cause and the reasoning rather than the change, including that a prior approval was dismissed.
Vendored claude_k8s adapteris green at this head, so the recomputed hash is machine-verified; version holds at0.2.6-blockcast.12with.11still reserved for #1936, so the two PRs do not collide.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
Conflict resolution (vendor/paperclip-adapter-claude-k8s/PROVENANCE.md): kept BOTH changelog rows (master's BLO-33279 row, then this PR's), and recomputed the integrity hash over the merged bytes rather than taking either side. Verified master's BLO-33279 change survived the merge (PENSTOCK_READY_TIMEOUT_MS still allowlisted) and that the PEN-3223 classifier is byte-identical to the reviewed head in both copies. Important (review of e7eff17): the collision guard was wired to BASE_REF: ${{ github.base_ref }}, which is populated only for pull_request. This workflow also runs on merge_group (pr.yml:7) and vendor_claude_k8s has no if:/needs: guard, so in the merge queue the step took its empty-base arm and exited 0 — a no-op indistinguishable from a pass. Now ${{ github.base_ref || github.event.merge_group.base_sha }}. base_sha, not the review's suggested merge_group.base_ref: the latter is the constant refs/heads/master, while base_sha is the commit the queue entry was built on and already contains the PRs merging ahead. The workflow-level PR_BASE_SHA was deliberately not reused — it resolves the PR lane to pull_request.base.sha, measured 49 commits behind master on this very PR, which would look green straight through the .10 collision this guard exists for. Mutation-tested both lanes on the real tree, including a positive control: forcing the head to master's .10 against differing bytes reddens both lanes (exit 1); a bare SHA takes the real arm and prints its base (<sha>) line. fetch-by-SHA verified against this repository. Suggestion: the three skip arms now emit ::notice:: so "this check compared nothing" is visible in the run summary; the two legitimate pass arms stay plain. Version resolved a third time: .12 was free at the last review but #1937 moved .10 -> .12 overnight and is human-authored, non-draft and MERGEABLE, so it lands first. Took .13, the next free integer above every claim (master .10; .11 on #1936/#1873; .12 on #1937). Three collisions on one PR — one trunk, two cross-PR — which is the residual axis this guard still cannot see; recorded as a follow-up rather than widened here. Validation: 904 tests / 16 files pass, tsc --noEmit clean, npm ci --include=dev succeeds, all three vendor_claude_k8s assertions pass. Refs: PEN-3223 Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Heads-up for whoever is watching the merge queue: this PR (position 7, head The queue merges with REBASE, so once #1936 lands this entry will be removed on the rebase conflict. Planned resolution (no action needed now): rebase onto master after #1936 merges, keep |
3d9cfb2 to
c890164
Compare
Rebased onto master at
|
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c890164
Clean. This head is a rebase only — job-manifest.ts, job-manifest.test.ts, config-schema.ts and config-schema.test.ts are byte-identical to 3d9cfb2 (blob SHAs compared directly, not inferred from the diff), so no new code surface arrived with the 9 commits of master. Every prior finding on this PR is already retired, so there is no disposition section: 19ff304 important 1 → fixed at fac058c, f6a323e critical 1 and critical 2 → fixed at 6eba574, 6eba574 important 1 → fixed at 3d9cfb2.
I re-verified the mechanism at this head rather than inheriting the earlier verdict, in the production spawn shape (Node spawnSync, default stdio → libuv socketpair() on fd 0, SHLVL not inherited), against the real /paperclip/.bashrc:
baseline (no BASH_ENV) ulimit=unlimited shlvl=1 fd0=socket:[...] ccrotate=PRESENT
BASH_ENV, no SHLVL ulimit=unlimited shlvl=1 fd0=socket:[...] ccrotate=PRESENT
BASH_ENV + SHLVL=2 ulimit=1048576 shlvl=3 fd0=socket:[...] ccrotate=PRESENT
Row 2 is the rshd/sshd branch still swallowing $BASH_ENV, which is what makes merged.SHLVL load-bearing rather than decorative; row 3 is the cap and the chain together, which is the property the bashenv.sh split exists to hold. zsh via ZDOTDIR reports the cap, a grandchild sh inherits it, and sh sourcing rlimit.sh directly reports the cap with ccrotate=ABSENT — so the bash rc is genuinely not leaking into zsh or POSIX sh.
One assumption underneath the chain repair that no earlier round had checked, so I checked it: the chain is only worth anything if $HOME/.bashrc actually executes non-interactively. Plenty of .bashrc files open with an interactivity guard that returns early for a non-interactive shell, and either common form would have made chain(".bashrc") a no-op for exactly the population it was added to protect — restoring nothing while still looking correct in a test that writes its own .bashrc. /paperclip/.bashrc (2210 bytes) carries no such guard: it opens straight into PATH manipulation and unconditional exported assignments. The measurement above (ccrotate=PRESENT on row 3) is against that real file, not a synthetic one, so the behaviour holds for the shared-HOME pods it was written for.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions
- Not re-listing the open suggestions from the
19ff304f0round; they are unchanged by this rebase.
Strengths
- The lever is right. Under
memory.oom.group=1the cgroup limit is a collective death sentence, so bounding the container harder could never have helped — bounding each child is the only thing that converts a group kill into one child'sENOMEM.RLIMIT_DATAoverRLIMIT_ASis likewise correct: V8's pointer-compression cages make any-vvalue that leaves node runnable useless as a ceiling. - The scoping is honest where it would have been easy not to be.
config-schema.ts's hint says plainly what plainshand theclaudeprocess itself do not get, rather than claiming "every shell"; the source comment states theisnetconn/SHLVLcondition, the libuv cause and the measurement, so theSHLVLarm does not read as a deletable redundant assignment; and the "not an off-by-one — bash incrementsSHLVLbefore evaluatingshell_level < 2" note pre-empts the obvious wrong correction. - The tests assert the production spawn shape unconditionally instead of behind a host probe, and the negative
SHLVLcase pins against a measured uncapped baseline rather than.not.toBe(CAP), so a bash that fails to start cannot pass it vacuously. Thenot.toContainonrlimit.shpins the file separation that keeps.bashrcout of zsh. Both halves are mutation-tested, which is the reason this round has nothing to add. - Degrade-rather-than-abort on an unparseable
resources.limits.memory, while the one value that is interpolated into a shell command (toolMemoryKb) keeps its hard digits-only throw. Those two fields genuinely differ in that respect and are no longer treated alike.
CI state at this head (not a code finding)
The PR workflow run 35553910249 is cancelled and unsuperseded — no newer run of that workflow exists at c890164a — so most lanes were skipped and the verify aggregator failed on its "Fail if any split verify lane failed" step rather than on a real test failure. The lane that actually exercises this change, "Vendored claude_k8s adapter", is green end to end: Install, Typecheck, Test and Verify provenance manifest all passed, so the 0.2.6-blockcast.12 bump and the regenerated 41-file hash are consistent. "Helm chart" is green too.
The remedy is a re-run, not a push: gh api -X POST repos/Blockcast/paperclip/actions/runs/35553910249/rerun. A push would move the head and void the at-head review attestation. The PR is also BEHIND master, so it needs an update or a merge-queue pass regardless.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
…way child cannot OOM-kill the run (BLO-34477) WIP checkpoint: full implementation + tests, pending local test run. Root cause: agent pods run under cgroup v2 with memory.oom.group=1, so when one Bash-tool child (Claude Code backgrounds a command that outlives its 120 s timeout instead of killing it) grows to the container limit, the kernel SIGKILLs every process in the cgroup and the whole run exits 137. Measured 2026-09-17: 12 OOMKilled heartbeat pods in 24 h, one orphaned grep at 7.6 GiB. Fix: the write-prompt init container writes a `ulimit -d` file onto the per-pod runtime-cache emptyDir; the claude container gets BASH_ENV and ZDOTDIR pointing at it so every bash/zsh the agent spawns (and its descendants) is bounded by RLIMIT_DATA, while `claude` itself (exec'd via POSIX sh) is not. Default cap = floor(limits.memory / 2); overridable via resources.limits.toolMemoryKb (0 disables). RLIMIT_AS is unusable because claude maps ~73 GiB of VA. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…runs them - Match the POSIX '\'' quoting the init-command quoter actually emits. - Build the per-run isolation fixture through setRuntimeIsolation() with the typed descriptor shape resolveJobIsolation() reads. - Gate the two "cap is applied" executed-shell tests on a host probe: Darwin's setrlimit(RLIMIT_DATA) returns EINVAL for any lowering, so they can only assert on Linux (the adapter's deployment target and CI). The probe asks the host rather than the platform string. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… cap instead of aborting the Job resolveToolMemoryLimitKb threw on a legal Kubernetes quantity such as 1.5Gi, which aborted buildJobManifest for every heartbeat on a deployment whose resources.limits.memory the integer-only parser cannot read. That limit was valid before the cap existed and the cluster still enforces it, so refusing the Job is the wrong failure mode. Unset toolMemoryKb now falls back to no RLIMIT_DATA cap with a warning naming the knob to pin; a malformed explicit toolMemoryKb still throws because it is interpolated into a shell command. PROVENANCE hash recomputed. (BLO-34477, Ally round 1) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ore asserting the cap through it The arc-light lane reported 'unlimited' from bash -c 'ulimit -d' under BASH_ENV while /bin/sh sourcing the same file reported the cap, and the same commands succeed as runner and root inside the runner image. bash skips BASH_ENV in host-specific situations (euid != uid, POSIX mode, a non-GNU bash on PATH), none of which describe the agent image whose bash delivers the cap in production. Probe the host once, skip the bash assertions with a console.warn carrying version/uid/euid/path when it does not source BASH_ENV, and attach ulimit/hard-limit/uid diagnostics to the assertions so a real regression is legible. PROVENANCE hash recomputed. (BLO-34477) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The probe ran at collection time, before beforeEach initialised tempDirs, and took the whole file down. Make it a function called from the test. PROVENANCE hash recomputed. (BLO-34477) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…dead
Ally's re-review dismissed her own earlier approval of this head and found
the bash half of the tool-child cap inert. bash's run_startup_files() treats
a shell as "run by rshd/sshd" when isnetconn(fileno(stdin)) && SHLVL < 2; on
that branch it sources ~/.bashrc and returns BEFORE $BASH_ENV is consulted.
libuv allocates child stdio with socketpair(), so fd 0 of anything Claude
Code spawns is a socket and that test always holds — the cap was advertised
and never applied to a bash tool shell.
Set SHLVL=2 in the claude container env (declared SAFE_LITERAL under the
BLO-29804 gate). Smallest of the three candidate deliveries and the only one
with no $HOME dependency, $HOME being what the emptyDir design exists to
avoid. Live exposure was config-dependent, not universal: this image's tool
shell is zsh, where the ZDOTDIR arm already applies the cap and grandchildren
inherit it, so what was dead is a bash spawned directly by Claude Code.
The second Critical was the test that should have caught the first.
bashHonorsBashEnv() probed through ulimitD, which spawns with default stdio
(socketpair) and rebuilds the child env as { PATH, ...overrides } so SHLVL is
never inherited — both branch conditions hold on every POSIX host, the probe
returned false universally, and the bash assertions were unreachable. A
detector for this exact defect wired to always report clean, and the reason
two prior passes came back green; its comment blamed three causes that were
all wrong. Replaced with unconditional assertions on the production spawn
shape plus a negative case pinning that the cap is NOT applied without SHLVL,
so the manifest arm cannot be deleted as redundant-looking.
Both halves mutation-tested: removing SHLVL from the test env arm fails the
behavioural case, removing merged.SHLVL fails the manifest case, baseline
restores green. Reproduced independently of the agent image (bash 5.2.21
here, 5.2.37 in the image).
Also narrows the config-schema hint, which claimed the cap reached "every
shell the agent spawns" — false for bash, and silent about plain sh.
PROVENANCE: integrity hash recomputed (c98d9025 -> 4e6dbf5c) and the missing
Local-modifications row added for this PR.
Refs: BLO-34477
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the Important from Ally's review of 6eba574. SHLVL=2 does not add a startup file, it swaps one: off bash's rshd/sshd branch, bash stops sourcing $HOME/.bashrc. rlimit.sh carried no chain back to it, so the cap replaced the pod's environment file instead of adding to it. Measured on the agent image: BASH_ENV alone left CCROTATE_SERVE_BASE_URL PRESENT with the cap unapplied; BASH_ENV + SHLVL=2 applied the cap and left it ABSENT. /paperclip/.bashrc is a real 2210-byte file setting ANTHROPIC_BASE_URL, CCROTATE_SERVE_*, CODEX_HOME, JAVA_HOME/ANDROID_HOME and three PATH prefixes; 2 of 9 then-running claude pods had HOME=/paperclip, so a bash-spawned claude/codex would have quietly stopped pointing at the rotation proxy. BASH_ENV now targets TOOL_RLIMIT_BASHENV (<dir>/bashenv.sh), which sources rlimit.sh then chains $HOME/.bashrc — the same shape already used for zsh. It must be a separate file: the .zshenv stub and the POSIX-sh path source rlimit.sh directly, so chaining a bash rc inside it would pull .bashrc into zsh and sh. A test asserts that non-containment explicitly. Mutation-tested: removing chain(".bashrc") fails both the init-shell assertion and the new behavioural case (cap AND chain asserted together); repointing BASH_ENV at rlimit.sh fails two manifest cases; baseline green at 915/915. Two suggestions from the same review land with it: - the negative SHLVL case asserts equality against a measured uncapped baseline rather than .not.toBe(CAP), so a bash that fails to start no longer passes it vacuously; - the source records that bash increments SHLVL before evaluating shell_level < 2 (1 would already clear it; 2 is margin), so the next reader does not read it as an off-by-one. PROVENANCE: hash recomputed (4e6dbf5c -> a7fae8f6), row extended with this round. Refs: BLO-34477 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebase onto master dropped the merge commit that carried the 0.2.6-blockcast.12 bump and emptied the hash-refresh commit. Re-applies both: the version in package.json, package-lock.json and PROVENANCE.md, and the integrity hash recomputed from the rebased tree (package.json is inside the hashed set, so the hash is taken after the bump). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c890164 to
6c009e3
Compare
|
@allyblockcast Please review exact current head Reviewed head: 6c009e3 |
|
@allyblockcast Please review exact current head Reviewed head: 6c009e3 |
|
@allyblockcast Please review exact current head Reviewed head: 19d7e26 |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 19d7e26
Clean. The delta since my last pass is the vendored provenance refresh, and I verified it rather than taking it on trust.
Provenance hash — independently recomputed, matches. Sparse-cloned the vendored tree at this exact head and ran the regeneration command the manifest itself documents:
git ls-files | grep -vxE 'LICENSE|PROVENANCE\.md' | LC_ALL=C sort | xargs sha256sum | sha256sum
computed: 14a197b0ef7727b9b6e4f41088088fb0f97af3b8e70011316305722b0f9925b2
documented: 14a197b0ef7727b9b6e4f41088088fb0f97af3b8e70011316305722b0f9925b2
41 in-tree files, matching the count the manifest asserts. Version is consistent at 0.2.6-blockcast.12 across PROVENANCE.md, package.json and package-lock.json — the three places that drifted last round. This is an independent recomputation, not a read of the CI result; the Vendored claude_k8s adapter lane also passed at this head, which corroborates the 926/926 claim.
Every finding from the four earlier rounds is retired — the 1.5Gi abort, the dead BASH_ENV arm, the self-vetoing bashHonorsBashEnv() probe, and the .bashrc swap. I re-confirmed the two structural properties the whole mechanism rests on, because they are the ones a later refactor could silently break: the runtime-cache emptyDir is mounted at RUNTIME_CACHE_MOUNT_PATH in both the init container (job-manifest.ts:2225) and the claude container (:1685) unconditionally, so TOOL_RLIMIT_DIR needs no isolation branch and does not acquire one by accident; and resolveToolMemoryLimitKb reads config["resources.limits.toolMemoryKb"] as a flat dotted key, which is the same convention resources.limits.memory already uses at :1627 — so the per-agent override actually resolves rather than silently reading undefined and always falling through to the derived default.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (1)
- [native-codex] CI at this head is red on
verify, but not because of this diff.verifyis an aggregator whose only failing lane isOpenCode Responses replay; the other 15 jobs pass, includingVendored claude_k8s adapter. That lane exercises the OpenCode adapter, which this PR does not touch — the diff is confined to seven files undervendor/paperclip-adapter-claude-k8s/. It is green on the last four master commits that ran it, and the failed job recorded every step with a null conclusion (includingInstall the pinned OpenCode release), which is the shape of a runner or network abort rather than an assertion failure. Job logs have already aged out, so this is inference from the step table, not a root cause. A re-run of that lane should settle it.mergeStateStatusisBEHIND, so the branch wants an update anyway — and on a repo withdismiss_stale_reviews_on_push, prefergh api -X POST repos/Blockcast/paperclip/actions/runs/35785074863/rerunover a push if the goal is only to clear the lane, since a push moves the head and voids this attestation.
Strengths
- The provenance discipline is the thing I would most expect to rot and it did not: hash, file count, and version bump all move together, and the new
#1937row records the review history of the defect — including that the first two approvals were wrong and why — rather than just the final shape. That row is the artifact that stops the next person re-deriving theisnetconn/SHLVLinteraction from scratch. - The comments earn their length.
job-manifest.ts:1092-1112states the bash startup-file branch condition, the libuvsocketpair()cause, the measurement on two bash versions, and an explicit "do not delete this as a redundant assignment" — plus the note that bash incrementsSHLVLbefore evaluatingshell_level < 2, so a reader cannot mistake2for an off-by-one. That is exactly the failure mode that would otherwise return. bashenv.shbeing a separate file fromrlimit.shis the right call and the reason is written down: zsh and POSIXshsourcerlimit.shdirectly, so chaining a bash rc inside it would leak.bashrcinto both. The non-containment is pinned by an explicitnot.toContain, so the two files cannot be merged back together quietly.- Failure modes are asymmetric in the right direction. An unparseable container limit degrades to no cap with a warning, matching the
ulimit -d ... 2>/dev/null || truefail-open — whiletoolMemoryKb, the one value interpolated into a shell command, keeps its hard digits-only throw. Those two fields genuinely differ in that respect and the code now treats them differently. - The tests are mutation-tested rather than asserted: removing
merged.SHLVL, removingchain(".bashrc"), or repointingBASH_ENVatrlimit.sheach redden a named case. Given that this PR's own second Critical was a test wired to always report clean, demonstrating the tripwires actually trip is the correct standard.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
This branch has not been deployed
1 participant
Thinking Path
Agent pods were OOM-killed at the 8 GiB cgroup limit by an orphaned Bash child that Claude Code backgrounded after a tool timeout (a bounded-repetition grep on a 115 KB line ate 7.6 GiB). The whole run died with the pod. The cgroup limit is the wrong granularity; the fix caps tool-spawned children individually via RLIMIT_DATA so a runaway fails alone with ENOMEM.
Linked Issues or Issue Description
Paperclip issue BLO-34477 (Blockcast board); RCA thread on BLO-34440 live-page stall follow-ups.
What Changed
Vendored adapter
job-manifest.ts:ulimit -dinstalled through aBASH_ENVfile andZDOTDIRstubs on the runtime-cache emptyDir;resources.limits.toolMemoryKbconfig (explicit KiB,0disables), default half ofresources.limits.memory; an unparseable container limit degrades to no cap with a warning (Ally round 1). Adapter version0.2.6-blockcast.12(#1936 holds.11), PROVENANCE hash recomputed.Verification
Vendored adapter suite 910 passing locally (the Linux-only shell-execution case is skipped on macOS; its CI failure on arc-light is under investigation and reproduced clean in the runner image). Ally APPROVED at the current head.
Risks
A cap that is too low makes legitimate tools ENOMEM; default is half the container limit and operators can pin
toolMemoryKb.sh -claunches (claude itself) deliberately stay uncapped.Model Used
Claude Fable 5.1 (
claude-fable-5-1) via Claude Code, hand-edited (no subagents); reviewed by Ally (Paperclip Code Reviewer) — APPROVED at the current head.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-line aboveOriginal description
Fixes BLO-34477 (measured 2026-09-17): 12 Ally heartbeat pods OOMKilled (exit 137) in 24 h; one orphaned
grepwith bounded repetition over a 115 KB single-line YAML grew to 7.6 GiB and took the whole run down.Root cause
Agent pods run under cgroup v2 with
memory.oom.group=1. Claude Code's Bash tool backgrounds a command that outlives its 120 s timeout instead of killing it; when that orphan grows to the container limit the kernel SIGKILLs every process in the cgroup, so the run exits 137 regardless of whatclaudeitself was doing. Nothing bounded a single child below the pod limit.RLIMIT_AScannot be that bound:claudemaps ~73 GiB of virtual address space, so anyulimit -vthat leaves node runnable is above an 8 GiB cgroup.RLIMIT_DATA(ulimit -d) counts what actually grows.Fix (
vendor/paperclip-adapter-claude-k8s)ulimit -d <KiB>file onto the per-pod runtime-cache emptyDir, plusZDOTDIRstubs for every zsh dotfile that apply it and chain to$HOME/<dotfile>.claudecontainer getsBASH_ENVandZDOTDIRpointing there, so every bash/zsh the agent spawns — Bash-tool commands and their descendants — is bounded byRLIMIT_DATA, whileclaude(exec'd via POSIXsh) is not. A runaway child now fails alone with ENOMEM instead of reaching the group kill.floor(resources.limits.memory / 2)by default (derived from the limit the pod already declares;parseMemoryQuantityToKiBrejects quantitiesulimit -dcannot take), overridable via new config keyresources.limits.toolMemoryKb(0 disables). Both env vars are declared in the allow-list with their reason (paths, no credential material).server/src/services/recovery/service.ts, BLO-33223) — no server change needed.0.2.6-blockcast.9 → .10(+ PROVENANCE). Note: the BLO-34577 PR bumps the same package to.10; whichever lands second rebases and takes.11.Verified
npx vitest run src/server/job-manifest.test.ts src/server/config-schema.test.ts→ 233 passed, 2 skipped;npx tsc --noEmitclean. The two skipped cases execute a real shell under the cap and are gated on a host probe: Darwin'ssetrlimit(RLIMIT_DATA)returns EINVAL for any lowering, so they assert only on Linux (the deployment target and CI). Not soaked on the fleet yet — the 24 h OOM count onac-e0a5011d-*pods is the acceptance signal.🤖 Generated with Claude Code