Agents/media: restore agent-scoped and iMessage attachment roots#39034
Agents/media: restore agent-scoped and iMessage attachment roots#39034vincentkoc wants to merge 10 commits into
Conversation
openclaw-barnacle
Bot
added
channel: whatsapp-web
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56b5340a72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| agentId?: string, | ||
| ): readonly string[] { | ||
| const roots = buildMediaLocalRoots(resolveStateDir()); | ||
| for (const attachmentRoot of resolveIMessageAttachmentRoots({ cfg })) { |
There was a problem hiding this comment.
getAgentScopedMediaLocalRoots now merges iMessage roots via resolveIMessageAttachmentRoots({ cfg }), but this call omits accountId, so channels.imessage.accounts.<id>.attachmentRoots are never included. For setups that define attachment directories only at the account level (and not globally), image/message local-path validation still fails with path-not-allowed, so iMessage attachment handling remains broken for those accounts despite this fix.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR restores two broken media-access paths: agent-scoped workspace roots for cron/hook sessions in the message tool, and iMessage attachment roots for the image tool. The fixes are targeted and well-tested. Verified findings:
The core bug fixes (message-tool agentId fallback and wildcard local-root matching) are correct and address real regressions, but these three issues warrant attention before merging. Confidence Score: 3/5
Last reviewed commit: 56b5340 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const roots = buildMediaLocalRoots(resolveStateDir()); | ||
| for (const attachmentRoot of resolveIMessageAttachmentRoots({ cfg })) { | ||
| if (!roots.includes(attachmentRoot)) { | ||
| roots.push(attachmentRoot); | ||
| } | ||
| } |
There was a problem hiding this comment.
DEFAULT_IMESSAGE_ATTACHMENT_ROOTS unconditionally broadened
resolveIMessageAttachmentRoots always appends DEFAULT_IMESSAGE_ATTACHMENT_ROOTS (/Users/*/Library/Messages/Attachments) regardless of whether iMessage is configured in cfg. This means every call to getAgentScopedMediaLocalRoots — including from image-tool runs in deployments that do not use iMessage at all — will include that default wildcard root in the local-media allowlist.
The PR description states: "no new media directories were exposed beyond the existing agent workspace and configured iMessage attachment roots", but DEFAULT_IMESSAGE_ATTACHMENT_ROOTS is not a user-configured root; it is a hard-coded default. On macOS this path exists for any local user account, silently expanding the allowlist without explicit opt-in.
Consider gating the default roots on iMessage actually being configured:
| const roots = buildMediaLocalRoots(resolveStateDir()); | |
| for (const attachmentRoot of resolveIMessageAttachmentRoots({ cfg })) { | |
| if (!roots.includes(attachmentRoot)) { | |
| roots.push(attachmentRoot); | |
| } | |
| } | |
| const iMessageRoots = | |
| cfg.channels?.imessage | |
| ? resolveIMessageAttachmentRoots({ cfg }) | |
| : []; | |
| for (const attachmentRoot of iMessageRoots) { | |
| if (!roots.includes(attachmentRoot)) { | |
| roots.push(attachmentRoot); | |
| } | |
| } |
This still fixes the parity bug for configured iMessage deployments while preserving the previous security boundary for non-iMessage deployments.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/media/local-roots.ts
Line: 44-49
Comment:
**DEFAULT_IMESSAGE_ATTACHMENT_ROOTS unconditionally broadened**
`resolveIMessageAttachmentRoots` always appends `DEFAULT_IMESSAGE_ATTACHMENT_ROOTS` (`/Users/*/Library/Messages/Attachments`) regardless of whether iMessage is configured in `cfg`. This means every call to `getAgentScopedMediaLocalRoots` — including from image-tool runs in deployments that do not use iMessage at all — will include that default wildcard root in the local-media allowlist.
The PR description states: *"no new media directories were exposed beyond the existing agent workspace and configured iMessage attachment roots"*, but `DEFAULT_IMESSAGE_ATTACHMENT_ROOTS` is not a user-configured root; it is a hard-coded default. On macOS this path exists for any local user account, silently expanding the allowlist without explicit opt-in.
Consider gating the default roots on iMessage actually being configured:
```suggestion
const iMessageRoots =
cfg.channels?.imessage
? resolveIMessageAttachmentRoots({ cfg })
: [];
for (const attachmentRoot of iMessageRoots) {
if (!roots.includes(attachmentRoot)) {
roots.push(attachmentRoot);
}
}
```
This still fixes the parity bug for configured iMessage deployments while preserving the previous security boundary for non-iMessage deployments.
How can I resolve this? If you propose a fix, please make it concise.| agentId: | ||
| options?.agentId?.trim() || | ||
| resolveSessionAgentId({ sessionKey: options?.agentSessionKey, config: cfg }), |
There was a problem hiding this comment.
Missing test for zero-argument fallback to default agent ID
The new test at line 120 covers the agentId: "cron-agent" explicit-override case. However, there is no test for the new behavior when both agentId and agentSessionKey are absent: the code now resolves to a default agent ID instead of undefined. Adding a test for this path would prevent a silent regression if the fallback logic is ever changed:
| agentId: | |
| options?.agentId?.trim() || | |
| resolveSessionAgentId({ sessionKey: options?.agentSessionKey, config: cfg }), | |
| it("falls back to default agent ID when neither agentId nor sessionKey is provided", async () => { | |
| mockSendResult(); | |
| const tool = createMessageTool({ config: {} as never }); | |
| await tool.execute("1", { action: "send", target: "telegram:123", message: "hi" }); | |
| const call = mocks.runMessageAction.mock.calls[0]?.[0]; | |
| expect(call?.agentId).toBeDefined(); | |
| expect(call?.sessionKey).toBeUndefined(); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/tools/message-tool.ts
Line: 779-781
Comment:
**Missing test for zero-argument fallback to default agent ID**
The new test at line 120 covers the `agentId: "cron-agent"` explicit-override case. However, there is no test for the new behavior when *both* `agentId` and `agentSessionKey` are absent: the code now resolves to a default agent ID instead of `undefined`. Adding a test for this path would prevent a silent regression if the fallback logic is ever changed:
```suggestion
it("falls back to default agent ID when neither agentId nor sessionKey is provided", async () => {
mockSendResult();
const tool = createMessageTool({ config: {} as never });
await tool.execute("1", { action: "send", target: "telegram:123", message: "hi" });
const call = mocks.runMessageAction.mock.calls[0]?.[0];
expect(call?.agentId).toBeDefined();
expect(call?.sessionKey).toBeUndefined();
});
```
How can I resolve this? If you propose a fix, please make it concise.| agentId: | ||
| options?.requesterAgentIdOverride ?? | ||
| resolveSessionAgentId({ | ||
| sessionKey: options?.agentSessionKey, | ||
| config: options?.config, | ||
| }), |
There was a problem hiding this comment.
Inconsistent empty-string handling vs message-tool
The image-tool path uses nullish coalescing (??) on line 84, so an empty-string requesterAgentIdOverride is forwarded as-is. The message-tool path (line 780 in message-tool.ts) explicitly uses .trim() || (logical OR), so an empty override string falls through to resolveSessionAgentId.
If requesterAgentIdOverride is ever set to "" by a misconfigured caller, the two tools would resolve agentId differently. Consider aligning to use .trim() || for consistency with the message-tool pattern.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/openclaw-tools.ts
Line: 83-88
Comment:
**Inconsistent empty-string handling vs message-tool**
The image-tool path uses nullish coalescing (`??`) on line 84, so an empty-string `requesterAgentIdOverride` is forwarded as-is. The message-tool path (line 780 in `message-tool.ts`) explicitly uses `.trim() ||` (logical OR), so an empty override string falls through to `resolveSessionAgentId`.
If `requesterAgentIdOverride` is ever set to `""` by a misconfigured caller, the two tools would resolve `agentId` differently. Consider aligning to use `.trim() ||` for consistency with the message-tool pattern.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
clawsweeper
Bot
added
the
rating: 🌊 off-meta tidepool
|
Codex review: found issues before merge. Latest ClawSweeper review: 2026-05-24 04:13 UTC / May 24, 2026, 12:13 AM ET. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
PR Surface View PR surface stats
Summary Reproducibility: yes. by source inspection: current main still gives the image tool default/workspace roots only, while the PR-side root merge omits accountId and always pulls in default iMessage roots. I did not run a live iMessage or cron/hook flow in this read-only review. 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: Refresh or replace the branch on current main with an account-aware, configured-root-only media access contract in the active media loader while preserving current main's message-tool agent plumbing. Do we have a high-confidence way to reproduce the issue? Yes by source inspection: current main still gives the image tool default/workspace roots only, while the PR-side root merge omits accountId and always pulls in default iMessage roots. I did not run a live iMessage or cron/hook flow in this read-only review. Is this the best way to solve the issue? No. The direction is valid, but the mergeable fix should be a current-main, account-aware, configured-root-only contract rather than globally adding default iMessage roots and patching the old loader path. 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 d0751111a472. |
|
ClawSweeper PR egg 🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress. Hatch commandComment Hatchability rules:
What is this egg doing here?
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
merge-risk: 🚨 message-delivery
|
Closing this stale branch in favor of a fresh current-main fix. Thanks @vincentkoc for identifying and proving the original media-root regressions here. I rechecked current I'll open a new narrow PR that keeps the useful direction from this branch, fixes only the remaining current bug, and credits this work. |
|
Superseded by a fresh current-main fix for the remaining iMessage attachment-root bug. |
|
Replacement PR is open here: #86569. Thanks again @vincentkoc. I carried forward the useful issue/root-cause direction from this PR, kept the fix scoped to the remaining current-main iMessage image-tool attachment-root bug, and added focused regression coverage plus changed-check proof there. |
2 participants
Summary
agentSessionKey, and the image tool could not read iMessage attachments after 2026.2.26 becauseattachmentRootsnever reached its local allowlist.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
imagetool can read iMessage attachment paths allowed by configuredattachmentRoots, including wildcard roots such as/Users/*/Library/Messages/Attachments.CHANGELOG.mdnow includes entries for both fixes with thanks to @ravz, @macminikenny1, and @vincentkoc.Security Impact (required)
NoNoNoNoNoRepro + Verification
Environment
channels.imessage.attachmentRootsSteps
agentSessionKey.attachmentRoots, send an attachment, and use theimagetool on that local path./tmp/*/Library/Messages/Attachments.Expected
Actual
path-not-allowed, iMessage attachments were rejected by the image tool, and wildcard local roots never matched.Evidence
Human Verification (required)
What you personally verified (not just CI), and how:
Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
message-tool,image-tool,local-roots, andweb/media.src/agents/tools/message-tool.ts,src/agents/openclaw-tools.ts,src/agents/tools/image-tool.ts,src/media/local-roots.ts,src/web/media.tsRisks and Mitigations