fix(core): propagate SHELL→TERMINAL_SHELL rename into direct-shell allowlists (follow-up to #12021) - #12052
Conversation
…lowlists (follow-up to #12021) PR #12021 renamed the terminal action SHELL → TERMINAL_SHELL but left the hardcoded shell-name allowlists in message.ts still listing only the old names. Both sets are normalized via normalizeActionIdentifier (TERMINAL_SHELL → TERMINALSHELL), so the renamed action was never a member: - WEAK_DIRECT_REPLY_OVERRIDE_ACTIONS - SHELL_DIRECT_ACTIONS Effect (lean-chat, ELIZA_PLUGIN_SET=lean-chat, no plugin-coding-tools, so TERMINAL_SHELL is the only shell action): inference resolved the action via its RUN_IN_TERMINAL simile, but shouldPreferDirectCurrentCandidateActions did SHELL_DIRECT_ACTIONS.has("TERMINALSHELL") → false and the .every() weak-override check also failed, so "run df -h on this VPS" no longer fast-pathed to the shell action and fell through to the general planner. Add "TERMINAL_SHELL" to both sets and to the two findAvailableActionName lookup lists (defensive; they already matched via simile). Add a regression test pinning the lean-chat path end-to-end (proven to fail pre-fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
The regression
PR #12021 renamed the terminal action
SHELL→TERMINAL_SHELL(packages/agent/src/actions/terminal.ts) but did not propagate the new name into two hardcoded shell-name allowlists inpackages/core/src/services/message.ts:WEAK_DIRECT_REPLY_OVERRIDE_ACTIONSSHELL_DIRECT_ACTIONSBoth sets are built by mapping their raw entries through
normalizeActionIdentifier(.toUpperCase().replace(/_/g, "")), so"TERMINAL_SHELL"normalizes to"TERMINALSHELL". Neither set listed it, so the renamed action was never a member.Lean-chat repro
In a lean-chat deployment (
ELIZA_PLUGIN_SET=lean-chat, noplugin-coding-tools),TERMINAL_SHELLis the only shell action. For a message likerundf -hon this VPS:inferDirectCurrentRequestCandidateActionsstill resolvesTERMINAL_SHELL(via itsRUN_IN_TERMINALsimile), sodirectCandidateActions = ["TERMINAL_SHELL"].shouldPreferDirectCurrentCandidateActionsthen doesSHELL_DIRECT_ACTIONS.has("TERMINALSHELL")→false(and thecandidateActions.every(...)weak-override check also fails), so the turn is not promoted to the direct shell path and silently falls through to the general planner.The fix
"TERMINAL_SHELL"toWEAK_DIRECT_REPLY_OVERRIDE_ACTIONSandSHELL_DIRECT_ACTIONS.findAvailableActionNameshell-name lookup lists (inmessage.tsinferAckIntentCandidateActionsandmessage/direct-action-heuristics.ts). These already resolved via simile; adding the canonical name is defensive and placed after"SHELL"so mixed deployments (coding-toolsSHELLpresent) keep their existing priority.prompt-compaction.tsalready lists bothSHELLandTERMINAL_SHELL;planner-loop.tsname === "SHELL"is the coding-tools trajectory summary (unrelated); the many"SHELL"env-var/$SHELL references are unrelated.Test
Added a regression test in
message-routing-live-regression.test.tsthat drives the real seam with a lean-chat fixture (onlyTERMINAL_SHELL+REPLY): it asserts inference resolves["TERMINAL_SHELL"]andshouldPreferDirectCurrentCandidateActionsreturnstrue. Verified it fails pre-fix (returnedfalse) and passes after.Verification
bun run --cwd packages/core typecheck→ cleanmessage-routing-live-regression.test.ts→ 41 passed;direct-action-heuristics.test.ts→ 5 passed