Repository navigation
fix(monitor): forward explicit env settings to the tmux loop pane (#378) - #385
Conversation
A running tmux server gives new panes its own global environment, so every env-only setting (CB_* thresholds, MAX_CONSECUTIVE_*, CLAUDE_MODEL, OPTIONAL_SECTIONS, ...) was lost in --monitor mode and .ralphrc won in the pane. setup_tmux_session now prepends KEY=%q-value for each non-empty _env_* snapshot to the loop pane command. The list is derived from the snapshots, so new keys are forwarded automatically. It runs before main() loads .ralphrc, so every forwarded value is the user's own; CLI flags are still appended after and win in the child.
…ale SYNC_* comment (#378) Internal review: the %q prefix shares RALPH_DIR's bash-compatible pane shell assumption, and env SYNC_* now rides in the env prefix rather than being dropped.
|
Claude finished @frankbria's task in 7m 0s —— View job GLM precision bug-hunting review — PR #385
✅ GLM review: no defects found.
What was verified before concluding (every verdict against code read in this checkout, not the diff alone):
One theoretical edge was investigated and not reported as a finding, for the record: on macOS's bash 3.2, if its |
Cross-family review — codex (pre-PR pass at b6745d2; the only later commit, dca1cb4, touches CLAUDE.md only)opencode/GLM (the first choice) stalled with no output for 180 s, which is an outage signature, so per the reviewer chain this pass used codex.
Verdict: no actionable findings. codex also ran a small probe confirming that |
Demo verification (Showboat,
|
| Criterion | this branch | main | Status |
|---|---|---|---|
| Env-only settings reach the loop despite a running server | CB_NO_PROGRESS_THRESHOLD=[10], CLAUDE_MODEL=[claude-x; touch PWNED] |
both <unset> |
VERIFIED |
%q quoting, no injection |
value arrives byte-exact; no PWNED file created |
(nothing forwarded) | VERIFIED |
| Unset keys not forwarded | MAX_CONSECUTIVE_TEST_LOOPS=[<unset>] |
<unset> |
VERIFIED |
| Every key (table-driven) | mock test covers all 54 _env_* keys parsed from ralph_loop.sh; quoting test checks ;, $(…), backticks, quotes, \, |, <, >, tab and newline values byte-exact |
— | VERIFIED |
|
Code Review: fix(monitor) forward explicit env settings to the tmux loop pane Overview This PR closes a genuine gap: when a tmux server is already running, new pane processes inherit the server environment rather than the calling shell's, so env-only settings (circuit breaker thresholds, CLAUDE_MODEL, etc.) silently reverted to defaults or .ralphrc values inside the loop. The fix is clean and well-targeted. Implementation Core mechanic is idiomatic and self-maintaining. Using the indirect expansion pattern to enumerate snapshots means a newly snapshotted key is forwarded automatically without maintaining a separate list. This is the right approach. printf '%q' is the correct tool here. It produces bash-safe quoted output that survives re-evaluation in the pane shell. The injection test validates this end-to-end with metacharacters, whitespace, and command substitution syntax - all arriving byte-exact. Precedence chain is preserved. The loop_env prefix is inserted between env_prefix (RALPH_DIR) and the ralph command, and CLI flags still append after it. The child's own load_ralphrc() and env* restore then applies, giving: CLI > env prefix > .ralphrc - consistent with the non-monitor path. Concerns / Observations No. 1 - SYNC_* values ride the env prefix even with --sandbox docker - verify child behavior is safe. When SANDBOX_PROVIDER=docker and the user set SYNC_INCLUDE in their environment, the new loop_env prefix now forwards SYNC_INCLUDE=value to the child process. The child's _env_SYNC_INCLUDE restore would then set SYNC_INCLUDE in the child. The comment update says the docker child 'ignores it just like a plain run does' - this appears correct since the docker sandbox sync validation only fires on the CLI flags path - but it is worth confirming no new warning or error surfaces in the child when SYNC_INCLUDE is non-empty with docker provider. The comment change is an improvement over the previous wording ('merely not forwarded'), so no code change is needed - just flagging it as worth a smoke test. No. 2 - Test helper _pane0_cmd uses a glob strip that could mismatch for unusual session names. The shortest-match glob correctly strips the 'tmux send-keys -t session ' prefix for all practical tmux session names. Fine for a test helper - just noting it as a limit. No. 3 - The injection test PWNED-file checks are relative to CWD. The check correctly verifies no injection happened. Since the mock ralph writes to OUT_DIR, this will not produce false negatives. Minor observation only. Test Coverage Three focused tests match the three acceptance criteria from the PR:
The table-driven key enumeration (reading from the real ralph_loop.sh) is the right approach - avoids the hand-copy drift problem documented in the test standards. CLAUDE.md Update The added sentence accurately describes the new behavior and is placed in the right section. No issues. Verdict This is a clean, well-tested fix for a real operational bug. The implementation approach (snapshot enumeration + printf '%q' quoting) is maintainable and secure. The one item worth a quick check is whether SYNC_* appearing in the docker child environment triggers any new log noise, but it is not blocking. Looks good to merge. |
|
Post-merge disposition: nothing filed. Dropped with justification: (1) the non-bash pane shell limitation. It predates this PR (the RALPH_DIR prefix has it too) and only matters for values that need |
Summary
Fixes #378.
ralph --monitorstarts the loop by typing a command into a tmux pane. When a tmux server is already running, that pane gets the server's environment, not the environment of the shell that ranralph --monitor. Until now onlyRALPH_DIRand CLI-flag-backed settings were put on the pane command, so env-only settings never reached the loop. That covers theCB_*thresholds,MAX_CONSECUTIVE_*,CLAUDE_MODEL,OPTIONAL_SECTIONSand more. In the pane,.ralphrcor the default won.setup_tmux_sessionnow prependsKEY=$(printf '%q' value)to the loop pane's command for every non-empty_env_*snapshot:"${!_env_@}", so a newly snapshotted key is forwarded without anyone maintaining a list.main()loads.ralphrc, so a non-empty snapshot is always the user's own value. Empty or unset keys aren't forwarded, so.ralphrcstill applies in the pane..ralphrc([P2.2] Environment does not override .ralphrc for CB_* thresholds, MAX_CONSECUTIVE_*, CLAUDE_MIN_VERSION #369), so an explicit flag keeps winning.ralph_monitor.shreads none of these keys.Acceptance Criteria
KEY=value ralph --monitorruns the loop withvaluefor every env-only.ralphrckey. A table-driven mock test sets every_env_*key parsed fromralph_loop.sh(54) and asserts each one is forwarded.%q-quoted. A test runs the logged pane command with bash against a fakeralph. Values containing;,$(…), backticks, quotes,\,|,<,>, a tab and a newline arrive byte-exact, and no injection side effect happens.Test Plan
npm test: 1364/1364 pass%q, no empty-value check, prefix not applied)SYNC_*comment). Two rebutted: a real-child precedence test would duplicate the [P2.2] Environment does not override .ralphrc for CB_* thresholds, MAX_CONSECUTIVE_*, CLAUDE_MIN_VERSION #369 tests plus the demo below, and the mocktmuxis exported (export -f).setup_tmux_sessionwas then run with them set. The loop pane's actual process receivedCB_NO_PROGRESS_THRESHOLD=10andCLAUDE_MODEL='claude-x; touch PWNED'byte-exact. On main both arrive unset.Known Limitations / Intentionally Deferred
%qprefix assumes a bash-compatible pane shell, as theRALPH_DIRprefix already did. With a non-bashdefault-shell(fish, dash), theKEY=value cmdform or$'…'quoting may not work._env_FOOwould be forwarded asFOO. Nothing in ralph uses that name pattern outside the snapshots..ralphrckeys).Closes #378