fix(openclaw): handle exit code 3 from rtk rewrite#2202
Conversation
KuSh
left a comment
There was a problem hiding this comment.
Thanks for your PR! While your current solution works as a temporary workaround, it’s not yet production-ready. I’ve left some suggestions to help guide you toward a more robust, final implementation.
Let me know if you’d like further clarification or assistance!
Address KuSh review feedback on rtk-ai#2202: - tryRewrite now returns [string | null, "ask"?] tuple - Exit code 3 specifically checked (e.status === 3) - before_tool_call returns requireApproval when rewrite is a suggestion (exit 3), per OpenClaw hook docs - 60s approval timeout with deny on timeout See: rtk-ai#2202 (review) Signed-off-by: Karol Zalewski <karol.zalewski@4zal.net>
Address KuSh review feedback on rtk-ai#2202: - tryRewrite now returns [string | null, "ask"?] tuple - Exit code 3 specifically checked (e.status === 3) - before_tool_call returns requireApproval when rewrite is a suggestion (exit 3), per OpenClaw hook docs - 60s approval timeout with deny on timeout See: rtk-ai#2202 (review) Signed-off-by: Karol Zalewski <karol.zalewski@4zal.net>
1162f2d to
2256da9
Compare
Rework the plugin to respect RTK's full exit code protocol (src/hooks/rewrite_cmd.rs): Exit codes: 0 + stdout Allow — auto-apply rewrite (no approval) 1 No match — pass through unchanged 2 Deny — block the call (new) 3 + stdout Ask — rewrite with requireApproval (KuSh review) Changes: - tryRewrite returns [string | null, "ask" | "deny" | undefined] - Exit 3: requireApproval with configurable timeout (default 120s) - Exit 2: block:true (was silently passed through — security gap) - allowedDecisions: ["allow-once", "deny"] — no allow-always (plugin does not persist trust) - onResolution callback for verbose logging - approvalTimeoutMs config option in openclaw.plugin.json - Documented exit code protocol in file header Address KuSh review: rtk-ai#2202 (review) Signed-off-by: Karol Zalewski <karol.zalewski@4zal.net>
KuSh
left a comment
There was a problem hiding this comment.
Just a few remaining nitpicks and questions
| severity: "info", | ||
| timeoutMs: approvalTimeoutMs, | ||
| timeoutBehavior: "deny", | ||
| allowedDecisions: ["allow-once", "deny"], |
There was a problem hiding this comment.
Why did you omit "allow-always"?
There was a problem hiding this comment.
Still missing a comment explaining the absence of "allow-always"
…t timeoutMs - Extract RewriteVerdict type per KuSh suggestion - Remove timeoutMs from requireApproval (omit to use Gateway default) - Remove approvalTimeoutMs config option from plugin.json - allowedDecisions still excludes allow-always: OpenClaw docs confirm plugin allow-always does not auto-persist; offering it without our own persistence would mislead users Address KuSh review: rtk-ai#2202 (review-4588193872) Signed-off-by: Karol Zalewski <karol.zalewski@4zal.net>
|
Thanks for the thorough review, KuSh — really appreciate the guidance. 1. 2. 3.
The troubleshooting section reinforces this:
Since this plugin doesn't implement its own trust persistence, offering |
9827755 to
6b2851c
Compare
Address KuSh review feedback on rtk-ai#2202: - tryRewrite now returns [string | null, "ask"?] tuple - Exit code 3 specifically checked (e.status === 3) - before_tool_call returns requireApproval when rewrite is a suggestion (exit 3), per OpenClaw hook docs - 60s approval timeout with deny on timeout See: rtk-ai#2202 (review) Signed-off-by: Karol Zalewski <karol.zalewski@4zal.net>
Rework the plugin to respect RTK's full exit code protocol (src/hooks/rewrite_cmd.rs): Exit codes: 0 + stdout Allow — auto-apply rewrite (no approval) 1 No match — pass through unchanged 2 Deny — block the call (new) 3 + stdout Ask — rewrite with requireApproval (KuSh review) Changes: - tryRewrite returns [string | null, "ask" | "deny" | undefined] - Exit 3: requireApproval with configurable timeout (default 120s) - Exit 2: block:true (was silently passed through — security gap) - allowedDecisions: ["allow-once", "deny"] — no allow-always (plugin does not persist trust) - onResolution callback for verbose logging - approvalTimeoutMs config option in openclaw.plugin.json - Documented exit code protocol in file header Address KuSh review: rtk-ai#2202 (review) Signed-off-by: Karol Zalewski <karol.zalewski@4zal.net>
…t timeoutMs - Extract RewriteVerdict type per KuSh suggestion - Remove timeoutMs from requireApproval (omit to use Gateway default) - Remove approvalTimeoutMs config option from plugin.json - allowedDecisions still excludes allow-always: OpenClaw docs confirm plugin allow-always does not auto-persist; offering it without our own persistence would mislead users Address KuSh review: rtk-ai#2202 (review-4588193872) Signed-off-by: Karol Zalewski <karol.zalewski@4zal.net>
6b2851c to
c059525
Compare
bbc694c to
f5c6eab
Compare
rtk rewrite returns exit code 3 when it has a rewrite suggestion. execSync treats non-zero exit codes as errors and throws. The catch block returned null, silently dropping all rewrites. Rework the plugin to respect RTK's full exit code protocol (src/hooks/rewrite_cmd.rs): Exit codes: 0 + stdout Allow — auto-apply rewrite (no approval) 1 No match — pass through unchanged 2 Deny — block the call 3 + stdout Ask — rewrite with requireApproval Implementation: - tryRewrite returns [string | null, RewriteVerdict] tuple - Exit 3: requireApproval (no allow-always; plugin does not persist trust) - Exit 2: block:true (was silently passed through — security gap) - allowedDecisions: ["allow-once", "deny"] - RewriteVerdict type extracted per KuSh review - timeoutMs omitted from requireApproval (use Gateway default) Closes rtk-ai#2200 Signed-off-by: Karol Zalewski <karol.zalewski@4zal.net>
f5c6eab to
97aaac8
Compare
| }).trim(); | ||
| // Exit 0 — Allow: rewrite and auto-apply | ||
| return [result && result !== command ? result : null, undefined]; | ||
| } catch (e: any) { | ||
| // Exit 3 — Ask: rewrite available but user must approve | ||
| if (e?.status === 3 && e?.stdout) { | ||
| const result = String(e.stdout).trim(); | ||
| if (result && result !== command) return [result, "ask"]; | ||
| // Exit 3 but no usable stdout — treat as passthrough | ||
| return [null, undefined]; | ||
| } | ||
| // Exit 2 — Deny: command matched a deny rule, block the call | ||
| if (e?.status === 2) { | ||
| return [null, "deny"]; | ||
| } | ||
| // Exit 1 or unknown — no rewrite, pass through | ||
| return [null, undefined]; |
There was a problem hiding this comment.
One regression (execFileSync return value can be a Buffer) and some code cleanups / nitpicks
| }).trim(); | |
| // Exit 0 — Allow: rewrite and auto-apply | |
| return [result && result !== command ? result : null, undefined]; | |
| } catch (e: any) { | |
| // Exit 3 — Ask: rewrite available but user must approve | |
| if (e?.status === 3 && e?.stdout) { | |
| const result = String(e.stdout).trim(); | |
| if (result && result !== command) return [result, "ask"]; | |
| // Exit 3 but no usable stdout — treat as passthrough | |
| return [null, undefined]; | |
| } | |
| // Exit 2 — Deny: command matched a deny rule, block the call | |
| if (e?.status === 2) { | |
| return [null, "deny"]; | |
| } | |
| // Exit 1 or unknown — no rewrite, pass through | |
| return [null, undefined]; | |
| }) | |
| .toString() | |
| .trim(); | |
| // Exit 0 — Allow: rewrite and auto-apply | |
| return [result && result !== command ? result : null]; | |
| } catch (e: any) { | |
| // Exit 3 — Ask: rewrite available but user must approve | |
| if (e?.status === 3 && e.stdout) { | |
| const result = e.stdout.toString().trim(); | |
| if (result && result !== command) return [result, "ask"]; | |
| // Exit 3 but no usable stdout — treat as passthrough | |
| return [null]; | |
| } | |
| // Exit 2 — Deny: command matched a deny rule, block the call | |
| if (e?.status === 2) { | |
| return [null, "deny"]; | |
| } | |
| // Exit 1 or unknown — no rewrite, pass through | |
| return [null]; |
| severity: "info", | ||
| timeoutMs: approvalTimeoutMs, | ||
| timeoutBehavior: "deny", | ||
| allowedDecisions: ["allow-once", "deny"], |
There was a problem hiding this comment.
Still missing a comment explaining the absence of "allow-always"
Problem
The rtk-rewrite OpenClaw plugin silently drops all command rewrites. Every
rtk rewritecall that produces a suggestion returns exit code 3, whichexecSynctreats as an exception. The catch block returnsnull, so no command is ever rewritten.See #2200 for full analysis.
Fix
Check
e.stdoutin the catch handler — exit code 3 with stdout output is a valid rewrite suggestion, not an error:Verified
All common shell commands now correctly rewritten:
All previously returned
null(no rewrite) with the upstream code.Aiko — AI assistant in OpenClaw
Running RTK on k3s with Kilo Gateway
Config: RTK 0.42.0 | OpenClaw 2026.5.28 | Node.js v24.14.0 | Model: MiMo-V2.5-Pro
Reviewed and verified by Zal (@kzzalews)