Reject truncated exec approval commands [AI] by pgondhi987 · Pull Request #81001 · openclaw/openclaw

pgondhi987 · 2026-05-12T11:13:44Z

Summary

Problem: exec approval display text could be shorter than the command later executed by the gateway host path.
Why it matters: approval clients need the approval payload to represent the command that will run.
What changed: approval registration now rejects commands whose sanitized approval display would be truncated or suppressed, before creating or broadcasting a pending approval.
What did NOT change (scope boundary): command execution, allowlist evaluation, token handling, pairing, and approval resolution semantics are otherwise unchanged.

AI-assisted: Yes

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

N/A

Real behavior proof (required for external PRs)

Behavior or issue addressed: long host exec approval requests whose display text would omit trailing command content.
Real environment tested: Not run in this metadata step.
Exact steps or command run after this patch: Not run in this metadata step.
Evidence after change: Regression coverage added in src/gateway/server-methods/server-methods.test.ts.
Observed result after change: Expected handler behavior is an INVALID_REQUEST response with no pending approval broadcast when the command display would be truncated.
What was not tested: Runtime proof and package test commands were not run while drafting metadata.
Before evidence (optional but encouraged): N/A

Root Cause (if applicable)

Root cause: the approval registration path stored the bounded display-sanitized command, while the async gateway exec continuation still held the original raw command.
Missing detection / guardrail: no handler-level regression covered a long approval command whose displayed payload omitted trailing content.
Contributing context (if known): the display sanitizer intentionally caps approval prompt size, but registration did not treat truncation as a reason to reject the approval request.

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file: src/gateway/server-methods/server-methods.test.ts
Scenario the test should lock in: a gateway exec approval request with command text beyond the approval display limit is rejected and does not broadcast a pending approval.
Why this is the smallest reliable guardrail: the gateway handler is where display sanitization, approval record creation, and pending approval broadcast meet.
Existing test that already covers this (if any): None identified.
If no new test is added, why not: N/A

User-visible / Behavior Changes

Long exec approval requests that exceed the approval display limit now fail approval registration instead of appearing as pending approvals.

Diagram (if applicable)

Before:
[long exec command] -> [truncated approval record] -> [raw command continuation can run]

After:
[long exec command] -> [display-limit rejection] -> [no pending approval]

Security Impact (required)

New permissions/capabilities? (No)
Secrets/tokens handling changed? (No)
New/changed network calls? (No)
Command/tool execution surface changed? (Yes)
Data access scope changed? (No)
If any Yes, explain risk + mitigation: exec approval registration now refuses commands that cannot be fully represented in the approval display, preventing approval records from diverging from the command continuation.

Repro + Verification

Environment

OS: Not run in this metadata step
Runtime/container: Not run in this metadata step
Model/provider: N/A
Integration/channel (if any): Gateway exec approvals
Relevant config (redacted): N/A

Steps

Submit an exec approval request for a gateway-host command whose sanitized display would exceed the approval display limit.
Observe the approval request response.
Confirm no pending approval request is broadcast.

Expected

The request is rejected with INVALID_REQUEST.
No pending approval is created or broadcast.

Actual

Not run in this metadata step.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios: Reviewed the changed gateway handler flow and regression test coverage from the branch diff context.
Edge cases checked: soft display truncation and hard display suppression both reject before approval record creation.
What you did not verify: package test commands, runtime reproduction, and live gateway behavior.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? (Yes)
Config/env changes? (No)
Migration needed? (No)
If yes, exact upgrade steps: N/A

Risks and Mitigations

Risk: very long commands that previously entered the approval queue now fail before approval.
- Mitigation: rejection is limited to commands the approval display cannot fully represent.

clawsweeper · 2026-05-12T11:18:46Z

Codex review: needs real behavior proof before merge.

Summary
The PR rejects exec approval requests whose sanitized command display would be truncated or suppressed before creating or broadcasting a pending approval.

Reproducibility: yes. from source inspection: on current main a command over the sanitizer display cap is shortened in request.command, while the gateway-host continuation later runs the original params.command after approval. I did not run the live gateway flow in this read-only review.

Real behavior proof
Needs real behavior proof before merge: The PR body says no real environment, exact post-patch command, runtime proof, package tests, or live gateway behavior were run; the contributor should add redacted terminal/log/screenshot/video proof and update the PR body so ClawSweeper can re-review.

Next step before merge
The PR is a security-sensitive command approval hardening change with a protected maintainer label and missing real behavior proof, so it needs explicit maintainer handling rather than an automated repair lane.

Security
Cleared: The diff narrows the exec approval command surface and does not add dependencies, downloads, workflow changes, new permissions, or secret-handling expansion.

Review details

Best possible solution:

Keep the hardening in the gateway approval registration path if maintainers accept the behavior change, require redacted after-fix real behavior proof, and land only after the focused gateway test/check lane passes.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection: on current main a command over the sanitizer display cap is shortened in request.command, while the gateway-host continuation later runs the original params.command after approval. I did not run the live gateway flow in this read-only review.

Is this the best way to solve the issue?

Yes, the proposed fix is on the narrow owner boundary: reject approval registration before manager.create(...) when the command display cannot fully represent the command. The remaining blocker is proof and maintainer approval, not a different code direction.

What I checked:

Protected label/live PR state: Live PR metadata shows this PR is open, authored by an external contributor, and labeled gateway, maintainer, and size: S; the protected maintainer label requires explicit maintainer handling rather than cleanup closure. (37f3ed50c502)
Current-main truncation behavior: Current main stores sanitizeExecApprovalDisplayText(effectiveCommandText) in the approval request payload, so long commands can be shortened before broadcast/list/get approval clients see them. (src/gateway/server-methods/exec-approval.ts:258, 30c17fed2077)
Current-main display caps: The sanitizer has a 16 KiB soft display cap that appends a truncation marker and a 256 KiB hard input cap that suppresses the full text with a marker. (src/infra/exec-approval-command-display.ts:41, 30c17fed2077)
Raw gateway command continuation: After approval, the gateway-host path continues by running runExecProcess({ command: params.command, ... }), so the command that executes remains the original request command, not the shortened approval display payload. (src/agents/bash-tools.exec-host-gateway.ts:542, 30c17fed2077)
PR diff hardening path: The PR adds sanitizeExecApprovalDisplayTextWithStatus(...) and rejects truncated or oversized command displays with INVALID_REQUEST before creating an approval record or broadcasting the pending approval. (src/gateway/server-methods/exec-approval.ts:249, 37f3ed50c502)
Regression coverage in PR: The PR adds a handler-level test asserting an over-limit gateway command is rejected with EXEC_APPROVAL_COMMAND_DISPLAY_LIMIT and no approval broadcast is emitted. (src/gateway/server-methods/server-methods.test.ts:1005, 37f3ed50c502)

Likely related people:

steipete: GitHub history shows recent exec approval command-analysis refactors, and local blame for the checked-out sanitizer/handler/test lines points to Peter Steinberger in the current shallow checkout. (role: recent area contributor; confidence: high; commits: bd0e10a2f68f, 3f7e6eebc2f5, 7a9f770a636a; files: src/gateway/server-methods/exec-approval.ts, src/infra/exec-approval-command-display.ts, src/gateway/server-methods/server-methods.test.ts)
jesse-merhi: Recent commits changed exec command highlighting through the gateway approval handler and gateway host path, adjacent to the PR's command display and span handling. (role: recent adjacent contributor; confidence: medium; commits: 79c2ed9065d0, 297a16453661; files: src/gateway/server-methods/exec-approval.ts, src/agents/bash-tools.exec-host-gateway.ts)
drobison00: Prior sanitizer hardening introduced the control-character escaping, redaction-before-truncation, bounded input size, and truncation behavior this PR now turns into a rejection condition for approval registration. (role: display sanitizer contributor; confidence: medium; commits: 503b748a8ee1, 681931345b3e; files: src/infra/exec-approval-command-display.ts, src/agents/bash-tools.exec-host-gateway.ts)
pashpashpash: Recent diagnostics and trajectory-export work routed sensitive command flows through exec approval and touched both the gateway approval handler and sanitizer paths. (role: approval flow contributor; confidence: medium; commits: 6ce1058296cc; files: src/gateway/server-methods/exec-approval.ts, src/infra/exec-approval-command-display.ts, src/agents/bash-tools.exec-host-gateway.ts)

Remaining risk / open question:

No local tests or live runtime proof were run during this read-only review, and the PR body says the contributor did not run runtime proof or package test commands.
This changes command execution approval failure behavior for very long commands, so the protected maintainer label should be honored before merge.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 30c17fed2077.

pgondhi987 · 2026-05-12T11:27:46Z

Not applicable to this automation stage; changelog/release-note and external real behavior proof requirements are handled outside auto-pr stages.

Quoted comment from @clawsweeper:

Codex review: needs real behavior proof before merge.

Summary
The PR rejects exec approval requests whose sanitized command display would be truncated or suppressed before creating or broadcasting a pending approval.

Reproducibility: yes. from source inspection: on current main a command over the sanitizer display cap is shortened in request.command, while the gateway-host continuation later runs the original params.command after approval. I did not run the live gateway flow in this read-only review.

Real behavior proof
Needs real behavior proof before merge: The PR body says no real environment, exact post-patch command, runtime proof, package tests, or live gateway behavior were run; the contributor should add redacted terminal/log/screenshot/video proof and update the PR body so ClawSweeper can re-review.

Next step before merge
The PR is a security-sensitive command approval hardening change with a protected maintainer label and missing real behavior proof, so it needs explicit maintainer handling rather than an automated repair lane.

Security
Cleared: The diff narrows the exec approval command surface and does not add dependencies, downloads, workflow changes, new permissions, or secret-handling expansion.

Review details

Best possible solution:

Keep the hardening in the gateway approval registration path if maintainers accept the behavior change, require redacted after-fix real behavior proof, and land only after the focused gateway test/check lane passes.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection: on current main a command over the sanitizer display cap is shortened in request.command, while the gateway-host continuation later runs the original params.command after approval. I did not run the live gateway flow in this read-only review.

Is this the best way to solve the issue?

Yes, the proposed fix is on the narrow owner boundary: reject approval registration before manager.create(...) when the command display cannot fully represent the command. The remaining blocker is proof and maintainer approval, not a different code direction.

What I checked:

Protected label/live PR state: Live PR metadata shows this PR is open, authored by an external contributor, and labeled gateway, maintainer, and size: S; the protected maintainer label requires explicit maintainer handling rather than cleanup closure. (37f3ed50c502)

Current-main truncation behavior: Current main stores sanitizeExecApprovalDisplayText(effectiveCommandText) in the approval request payload, so long commands can be shortened before broadcast/list/get approval clients see them. (src/gateway/server-methods/exec-approval.ts:258, 30c17fed2077)

Current-main display caps: The sanitizer has a 16 KiB soft display cap that appends a truncation marker and a 256 KiB hard input cap that suppresses the full text with a marker. (src/infra/exec-approval-command-display.ts:41, 30c17fed2077)

Raw gateway command continuation: After approval, the gateway-host path continues by running runExecProcess({ command: params.command, ... }), so the command that executes remains the original request command, not the shortened approval display payload. (src/agents/bash-tools.exec-host-gateway.ts:542, 30c17fed2077)

PR diff hardening path: The PR adds sanitizeExecApprovalDisplayTextWithStatus(...) and rejects truncated or oversized command displays with INVALID_REQUEST before creating an approval record or broadcasting the pending approval. (src/gateway/server-methods/exec-approval.ts:249, 37f3ed50c502)

Regression coverage in PR: The PR adds a handler-level test asserting an over-limit gateway command is rejected with EXEC_APPROVAL_COMMAND_DISPLAY_LIMIT and no approval broadcast is emitted. (src/gateway/server-methods/server-methods.test.ts:1005, 37f3ed50c502)

Likely related people:

steipete: GitHub history shows recent exec approval command-analysis refactors, and local blame for the checked-out sanitizer/handler/test lines points to Peter Steinberger in the current shallow checkout. (role: recent area contributor; confidence: high; commits: bd0e10a2f68f, 3f7e6eebc2f5, 7a9f770a636a; files: src/gateway/server-methods/exec-approval.ts, src/infra/exec-approval-command-display.ts, src/gateway/server-methods/server-methods.test.ts)

jesse-merhi: Recent commits changed exec command highlighting through the gateway approval handler and gateway host path, adjacent to the PR's command display and span handling. (role: recent adjacent contributor; confidence: medium; commits: 79c2ed9065d0, 297a16453661; files: src/gateway/server-methods/exec-approval.ts, src/agents/bash-tools.exec-host-gateway.ts)

drobison00: Prior sanitizer hardening introduced the control-character escaping, redaction-before-truncation, bounded input size, and truncation behavior this PR now turns into a rejection condition for approval registration. (role: display sanitizer contributor; confidence: medium; commits: 503b748a8ee1, 681931345b3e; files: src/infra/exec-approval-command-display.ts, src/agents/bash-tools.exec-host-gateway.ts)

pashpashpash: Recent diagnostics and trajectory-export work routed sensitive command flows through exec approval and touched both the gateway approval handler and sanitizer paths. (role: approval flow contributor; confidence: medium; commits: 6ce1058296cc; files: src/gateway/server-methods/exec-approval.ts, src/infra/exec-approval-command-display.ts, src/agents/bash-tools.exec-host-gateway.ts)

Remaining risk / open question:

No local tests or live runtime proof were run during this read-only review, and the PR body says the contributor did not run runtime proof or package test commands.

This changes command execution approval failure behavior for very long commands, so the protected maintainer label should be honored before merge.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 30c17fed2077.

* fix: reject truncated exec approval commands * docs: add changelog entry for PR merge

fix: reject truncated exec approval commands

37f3ed5

openclaw-barnacle Bot added gateway Gateway runtime size: S maintainer Maintainer-authored PR labels May 12, 2026

pgondhi987 added 2 commits May 12, 2026 14:03

Merge remote-tracking branch 'origin/main' into fix/fix-622

2a0f5e8

docs: add changelog entry for PR merge

165d748

pgondhi987 merged commit 731af9c into openclaw:main May 12, 2026
20 of 22 checks passed

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 24, 2026

Reject truncated exec approval commands [AI] (openclaw#81001)

a85689b

* fix: reject truncated exec approval commands * docs: add changelog entry for PR merge

jameslcowan pushed a commit to jameslcowan/openclaw that referenced this pull request Jun 2, 2026

Reject truncated exec approval commands [AI] (openclaw#81001)

7954fd4

* fix: reject truncated exec approval commands * docs: add changelog entry for PR merge

sablehead pushed a commit to sablehead/openclaw that referenced this pull request Jun 10, 2026

Reject truncated exec approval commands [AI] (openclaw#81001)

2c30a52

* fix: reject truncated exec approval commands * docs: add changelog entry for PR merge

clawsweeper Bot mentioned this pull request Jun 21, 2026

fix(approvals): summarize long approval prompts #80141

Open

steipete mentioned this pull request Jul 3, 2026

fix(exec): avoid splitting surrogate pairs in approval display #99566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Reject truncated exec approval commands [AI]#81001

Reject truncated exec approval commands [AI]#81001
pgondhi987 merged 3 commits into
openclaw:mainfrom
pgondhi987:fix/fix-622

pgondhi987 commented May 12, 2026

Uh oh!

clawsweeper Bot commented May 12, 2026 •

edited

Loading

Uh oh!

pgondhi987 commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

pgondhi987 commented May 12, 2026

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Real behavior proof (required for external PRs)

Root Cause (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Diagram (if applicable)

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Risks and Mitigations

Uh oh!

clawsweeper Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgondhi987 commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented May 12, 2026 •

edited

Loading