fix(gateway): guard maxLines compact against active session runs#72765
fix(gateway): guard maxLines compact against active session runs#72765chinadbo wants to merge 4 commits into
Conversation
Greptile SummaryThis PR adds a missing Confidence Score: 5/5Safe to merge — a minimal, targeted fix with no observed issues. Single-file change that adds one guard matching an established pattern already used twice in the same handler. Parameters and error handling are identical to the AI compaction path. No new logic, no side-effect changes. No files require special attention. Reviews (1): Last reviewed commit: "fix(gateway): guard maxLines compact aga..." | Re-trigger Greptile |
|
The failing CI checks (
|
e63091e to
4561004
Compare
c41d0a0 to
0607295
Compare
5cec420 to
237adb0
Compare
|
Re-reviewed current head. The earlier Before merge, please add regression coverage. This PR currently touches only Live check note: current PR checks are green except the parity gate. |
|
Added the regression test as requested. The test (
All 53 tests in the file pass. |
9ede439 to
2cf2a8d
Compare
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the active-run data-loss bug is real, but this branch is now conflicting, lacks real behavior proof, and the clean mergeable replacement PR already carries the same guard in the current sessions code with stronger tests and proof. Canonical path: Use #91378 as the canonical current-main fix path and close this conflicting branch as superseded. So I’m closing this here and keeping the remaining discussion on #91378. Review detailsBest possible solution: Use #91378 as the canonical current-main fix path and close this conflicting branch as superseded. Do we have a high-confidence way to reproduce the issue? Yes from source inspection. Current main’s maxLines branch reaches transcript tail read, archive, and rewrite without the active-run interrupt used by the non-maxLines compaction branch. Is this the best way to solve the issue? No for this branch. The guard shape is right, but the clean replacement PR is the better solution because it applies to current main, preserves agent scoping, adds current-suite coverage, and supplies real behavior proof. Security review: Security review cleared: Security review cleared: this stale branch changes gateway session runtime logic and tests only, with no dependency, workflow, package, credential, or supply-chain surface changes. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 66880a5d736c. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
…ve runs The non-maxLines (LLM) compact branch interrupts an active session run before compacting, but the maxLines truncate branch read the tail, archived, and overwrote the transcript in place without that guard. Exposing `--max-lines` as a documented CLI command (this PR) would make the active-run data-loss mode tracked by openclaw#72765 easy to trigger from ordinary CLI usage. Run the same interruptSessionRunIfActive guard in the maxLines branch before reading the tail and truncating, matching the LLM compact path. Add gateway regression coverage over a real in-process Gateway: with no active run, the maxLines branch truncates the on-disk transcript 500 -> 50 and preserves the original 500 lines in the .bak archive; with an active embedded run, the maxLines branch fires the same interrupt (abort + wait-for-end) before archiving and truncating.
…ve runs The non-maxLines (LLM) compact branch interrupts an active session run before compacting, but the maxLines truncate branch read the tail, archived, and overwrote the transcript in place without that guard. Exposing `--max-lines` as a documented CLI command (this PR) would make the active-run data-loss mode tracked by openclaw#72765 easy to trigger from ordinary CLI usage. Run the same interruptSessionRunIfActive guard in the maxLines branch before reading the tail and truncating, matching the LLM compact path. Add gateway regression coverage over a real in-process Gateway: with no active run, the maxLines branch truncates the on-disk transcript 500 -> 50 and preserves the original 500 lines in the .bak archive; with an active embedded run, the maxLines branch fires the same interrupt (abort + wait-for-end) before archiving and truncating.
|
ClawSweeper applied the proposed close for this PR.
|
…ve runs The non-maxLines (LLM) compact branch interrupts an active session run before compacting, but the maxLines truncate branch read the tail, archived, and overwrote the transcript in place without that guard. Exposing `--max-lines` as a documented CLI command (this PR) would make the active-run data-loss mode tracked by openclaw#72765 easy to trigger from ordinary CLI usage. Run the same interruptSessionRunIfActive guard in the maxLines branch before reading the tail and truncating, matching the LLM compact path. Add gateway regression coverage over a real in-process Gateway: with no active run, the maxLines branch truncates the on-disk transcript 500 -> 50 and preserves the original 500 lines in the .bak archive; with an active embedded run, the maxLines branch fires the same interrupt (abort + wait-for-end) before archiving and truncating.
…ve runs The non-maxLines (LLM) compact branch interrupts an active session run before compacting, but the maxLines truncate branch read the tail, archived, and overwrote the transcript in place without that guard. Exposing `--max-lines` as a documented CLI command (this PR) would make the active-run data-loss mode tracked by openclaw#72765 easy to trigger from ordinary CLI usage. Run the same interruptSessionRunIfActive guard in the maxLines branch before reading the tail and truncating, matching the LLM compact path. Add gateway regression coverage over a real in-process Gateway: with no active run, the maxLines branch truncates the on-disk transcript 500 -> 50 and preserves the original 500 lines in the .bak archive; with an active embedded run, the maxLines branch fires the same interrupt (abort + wait-for-end) before archiving and truncating.
…ompact` (fixes #90640) (#91378) * feat(cli): add `sessions compact` command and fail loudly on CLI `/compact` `sessions.compact` was reachable only as an internal Gateway RPC — no CLI command, no docs — and `openclaw agent --message '/compact'` silently no-opped with exit 0 because the slash-command handler rejects CLI-originated senders, so the message fell through to an ordinary agent turn that compacted nothing. - Add `openclaw sessions compact <key>` wrapping the existing `sessions.compact` RPC; exit non-zero on a transport error or an `ok:false` payload so automation never mistakes a silent no-op for success. - Reject `openclaw agent --message '/compact'` with a redirect to the new command and exit 1 instead of a silent exit 0. The shared chat-side `/compact` handler is left untouched (no compatibility / message-delivery blast radius). - Strictly validate `--max-lines` and `--timeout` (positive integers only). - Document the command and the `sessions.compact` RPC in docs/cli/sessions.md. Fixes #90640. * fix(cli): inherit parent `sessions` options for `compact` `openclaw sessions compact <key>` did not merge the parent `sessions` command options the way its sibling subcommands (list/cleanup/info/…) do, so a parent-level `--agent`/`--json` was silently dropped. In particular `openclaw sessions --agent work compact <key>` compacted the default agent's session instead of the work agent's — a wrong-target session-state mutation. Merge the parent options in the compact action (parent `--agent`/`--json`, with the compact-level option taking precedence) and add regression coverage for parent `--agent`, parent `--json`, and the compact-level override. Refs #90640. * fix(cli): report pending Codex compaction and reject unsupported parent options Address two ClawSweeper review findings on the `sessions compact` command: - `sessions-compact.ts`: the Codex app-server `thread/compact/start` path returns `ok:true / compacted:false` with a pending marker, meaning the compaction was *started* asynchronously. The formatter collapsed every non-compacted success into "No compaction needed", so Codex users were told nothing happened. Report it as a started/pending compaction instead. - `register.status-health-sessions.ts`: the parent `sessions` command defines list-only options (`--store`/`--all-agents`/`--active`/`--limit`) that the compact action previously ignored. Silently dropping a parent `--store` is dangerous — the gateway resolves the target store itself, so a user could believe they targeted one store while another is mutated. Reject any unsupported inherited parent option with a clear error and a non-zero exit. Add regression tests for the pending-compaction message and the rejected parent options. Refs #90640. * fix(gateway): guard sessions.compact maxLines truncation against active runs The non-maxLines (LLM) compact branch interrupts an active session run before compacting, but the maxLines truncate branch read the tail, archived, and overwrote the transcript in place without that guard. Exposing `--max-lines` as a documented CLI command (this PR) would make the active-run data-loss mode tracked by #72765 easy to trigger from ordinary CLI usage. Run the same interruptSessionRunIfActive guard in the maxLines branch before reading the tail and truncating, matching the LLM compact path. Add gateway regression coverage over a real in-process Gateway: with no active run, the maxLines branch truncates the on-disk transcript 500 -> 50 and preserves the original 500 lines in the .bak archive; with an active embedded run, the maxLines branch fires the same interrupt (abort + wait-for-end) before archiving and truncating. * docs(cli): move sessions compact section above related links The new "Compact a session" section was inserted between the cleanup section's inline "Related:" list and the page's final "## Related" block, splitting related-link content around the command docs. Move the compact section above the related-links area and merge the orphaned "Session config" link into the single final "## Related" block. * fix(gateway): avoid no-op compact aborts Signed-off-by: sallyom <[email protected]> * fix(gateway): satisfy compact preflight lint Signed-off-by: sallyom <[email protected]> * fix(sessions): preserve compacted transcript structure --------- Signed-off-by: sallyom <[email protected]> Co-authored-by: sallyom <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>
…ompact` (fixes openclaw#90640) (openclaw#91378) * feat(cli): add `sessions compact` command and fail loudly on CLI `/compact` `sessions.compact` was reachable only as an internal Gateway RPC — no CLI command, no docs — and `openclaw agent --message '/compact'` silently no-opped with exit 0 because the slash-command handler rejects CLI-originated senders, so the message fell through to an ordinary agent turn that compacted nothing. - Add `openclaw sessions compact <key>` wrapping the existing `sessions.compact` RPC; exit non-zero on a transport error or an `ok:false` payload so automation never mistakes a silent no-op for success. - Reject `openclaw agent --message '/compact'` with a redirect to the new command and exit 1 instead of a silent exit 0. The shared chat-side `/compact` handler is left untouched (no compatibility / message-delivery blast radius). - Strictly validate `--max-lines` and `--timeout` (positive integers only). - Document the command and the `sessions.compact` RPC in docs/cli/sessions.md. Fixes openclaw#90640. * fix(cli): inherit parent `sessions` options for `compact` `openclaw sessions compact <key>` did not merge the parent `sessions` command options the way its sibling subcommands (list/cleanup/info/…) do, so a parent-level `--agent`/`--json` was silently dropped. In particular `openclaw sessions --agent work compact <key>` compacted the default agent's session instead of the work agent's — a wrong-target session-state mutation. Merge the parent options in the compact action (parent `--agent`/`--json`, with the compact-level option taking precedence) and add regression coverage for parent `--agent`, parent `--json`, and the compact-level override. Refs openclaw#90640. * fix(cli): report pending Codex compaction and reject unsupported parent options Address two ClawSweeper review findings on the `sessions compact` command: - `sessions-compact.ts`: the Codex app-server `thread/compact/start` path returns `ok:true / compacted:false` with a pending marker, meaning the compaction was *started* asynchronously. The formatter collapsed every non-compacted success into "No compaction needed", so Codex users were told nothing happened. Report it as a started/pending compaction instead. - `register.status-health-sessions.ts`: the parent `sessions` command defines list-only options (`--store`/`--all-agents`/`--active`/`--limit`) that the compact action previously ignored. Silently dropping a parent `--store` is dangerous — the gateway resolves the target store itself, so a user could believe they targeted one store while another is mutated. Reject any unsupported inherited parent option with a clear error and a non-zero exit. Add regression tests for the pending-compaction message and the rejected parent options. Refs openclaw#90640. * fix(gateway): guard sessions.compact maxLines truncation against active runs The non-maxLines (LLM) compact branch interrupts an active session run before compacting, but the maxLines truncate branch read the tail, archived, and overwrote the transcript in place without that guard. Exposing `--max-lines` as a documented CLI command (this PR) would make the active-run data-loss mode tracked by openclaw#72765 easy to trigger from ordinary CLI usage. Run the same interruptSessionRunIfActive guard in the maxLines branch before reading the tail and truncating, matching the LLM compact path. Add gateway regression coverage over a real in-process Gateway: with no active run, the maxLines branch truncates the on-disk transcript 500 -> 50 and preserves the original 500 lines in the .bak archive; with an active embedded run, the maxLines branch fires the same interrupt (abort + wait-for-end) before archiving and truncating. * docs(cli): move sessions compact section above related links The new "Compact a session" section was inserted between the cleanup section's inline "Related:" list and the page's final "## Related" block, splitting related-link content around the command docs. Move the compact section above the related-links area and merge the orphaned "Session config" link into the single final "## Related" block. * fix(gateway): avoid no-op compact aborts Signed-off-by: sallyom <[email protected]> * fix(gateway): satisfy compact preflight lint Signed-off-by: sallyom <[email protected]> * fix(sessions): preserve compacted transcript structure --------- Signed-off-by: sallyom <[email protected]> Co-authored-by: sallyom <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>
Summary
sessions.compactwithmaxLinesskips theinterruptSessionRunIfActiveguard that the AI compaction path usesarchiveFileOnDiskrenames the live transcript to.bak; the active runner continues writing to the renamed file whilewriteFileSynccreates a new truncated file at the original path — all post-compact agent output is silently lostinterruptSessionRunIfActiveguard used by the AI compaction path, mirroring the pattern also used bysessions.compaction.restoreFix
Add
interruptSessionRunIfActivecall to themaxLinesbranch ofsessions.compactbefore the transcript file is read and overwritten.Test plan
sessions.compactwithmaxLinesduring an active run now aborts the run first (same behavior as AI compaction)sessions.compactwithmaxLineswhen no run is active works unchangedpnpm check:changedpassesThanks @ioodu