fix: route remaining cli-backed helper runs through CLI paths#72499
fix: route remaining cli-backed helper runs through CLI paths#72499nicko-ai wants to merge 48 commits into
Conversation
openclaw-barnacle
Bot
added
gateway
Greptile SummaryThis PR routes the remaining CLI-backed helper paths — queued followup runs, Confidence Score: 4/5Safe to merge with minor caveats; no P0/P1 issues found, all significant behavioral changes are covered by tests. Only P2 findings: (1) the module-level
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/cli-runner/messaging-tool-tracker.ts
Line: 9
Comment:
**Module-level singleton risks cross-run interference**
`sendsBySessionKey` is a process-wide Map. `runPreparedCliAgent` drains it at start to purge stale state, but if two CLI agents for the **same `sessionKey`** run concurrently (e.g. a queued followup races with a cron delivery on the same session), the second agent's startup-drain will silently discard sends that the first agent's MCP calls already recorded, and the first agent's end-drain will return an empty result. No guard exists to detect or prevent this overlap.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/pi-embedded-messaging.ts
Line: 26-28
Comment:
**Expanded action set also affects non-CLI embedded runner**
`isCoreMessageToolSendAction` now includes `"reply"`, `"sendAttachment"`, and `"upload-file"` in addition to the previous `"send"` / `"thread-reply"`. This change propagates into `isMessagingToolSendAction` (used by the embedded Pi runner) and `extractMessagingToolSend` (used in `pi-embedded-subscribe.tools.ts`), not only the new CLI MCP tracker. If any of these new actions uses a different argument shape for the `to` / `target` field, `extractMessagingToolSend` may silently record an incomplete or wrong target. Confirm that argument conventions are uniform across all five action types.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/cron/isolated-agent/run-executor.ts
Line: 159-160
Comment:
**`isNewSession: false` forced for first-run isolated sessions**
When `params.job.sessionTarget === "isolated"` and `params.cronSession.isNewSession === true` (the very first run of an isolated session), `isNewSession` evaluates to `false`. `resolveSessionAuthProfileOverride` will therefore look for a stored profile binding that does not yet exist, potentially returning `undefined` and leaving the CLI agent to pick a profile itself. This is only relevant for the aliased-runtime path (`cliExecutionProvider !== providerOverride`), but it may silently select a different auth profile than intended on session creation.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: share message send action tracking" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
e15694e to
b5bbb55
Compare
|
Codex review: needs real behavior proof before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. at source level: current main still routes model auth probes and LLM slug generation through PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Land a rebased or replacement change that keeps the merged followup fix, routes probe and slug helpers through CLI dispatch, and handles cron only if the fail-closed tool-policy contract is preserved with focused proof. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main still routes model auth probes and LLM slug generation through Is this the best way to solve the issue? No, not as-is. Reusing CLI dispatch is the right direction for the remaining helpers, but this branch is conflicting, partly superseded, missing real behavior proof, and needs cron/MCP policy safety resolved before merge. Label changes:
Label justifications:
Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 1a7669bc63a0. |
b5bbb55 to
f6df367
Compare
f6df367 to
9fe2043
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
1 participant
Summary
Latest
mainalready has the primary/direct CLI backend dispatch fix from #57326.This PR keeps the remaining helper paths on the same dispatch path when the selected runtime is CLI-backed:
models status --probeSupersedes #57327.
Changes
runCliAgent()Out of scope
The stale persisted
agentHarnessId/CLI-runtime-alias pin reported later in #57326 is a separate compatibility hardening path. This PR fixes the helper paths that still bypass CLI dispatch; it does not try to migrate or canonicalize old session harness pins.Fixes #57326