sessions_spawn: inline attachments + redaction + hardening#16761
sessions_spawn: inline attachments + redaction + hardening#16761tyler6204 merged 1 commit intoopenclaw:mainfrom
Conversation
23dd6d5 to
8f2ff3c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f2ff3c0d0
ℹ️ 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".
|
Addressed Codex feedback:\n- fixed staged attachment cleanup on spawn failure (always cleanup before registry enrollment)\n- fixed archived-run attachment cleanup in sweeper (remove retained dirs on purge)\n\nPlease re-review. |
|
Opened design discussion for maintainer feedback and scope alignment: https://github.com/openclaw/openclaw/discussions/17399\n\nThis PR implements MVP in that direction. #16344 remains image-specific subset; #17214 tracks generalized capability. |
|
@openclaw/maintainers quick request: could you please trigger/approve full CI for this PR (if required for fork workflows) and advise on any required changes for merge?\n\nI can address feedback quickly. Also happy to split scope further if you prefer a smaller first merge. |
1077c73 to
2975184
Compare
bfc1ccb to
f92900f
Compare
|
Addressed current CI breakages from latest main merge:\n\n- fixed Discord preflight role allowlist parsing to handle raw member role IDs robustly\n- tightened default media local roots (removed broad state/agents root)\n- stabilized root-guard expectation so it is deterministic across CI environments\n\nPushed in c622456. Waiting for fresh CI run. |
c622456 to
154d4e6
Compare
|
Quick CI status update after latest fixes: all functional CI lanes are green; the only remaining failure is , which appears to fail due to fork permissions when trying to post an informational comment (, HTTP 403).\n\nIf needed, I can also open a small follow-up PR to guard that workflow step so it doesn't fail on fork PRs. |
|
Quick CI status update after latest fixes: all functional CI lanes are green; the only remaining failure is formal_conformance, which appears to fail due to fork permissions when trying to post an informational comment ("Resource not accessible by integration", HTTP 403). If needed, I can also open a small follow-up PR to guard that workflow step so it doesn't fail on fork PRs. |
|
Opened a small workflow fix PR for the random failing informational check:\nhttps://github.com//pull/17497\n\nIf merged, should stop failing on fork PRs due to comment-permission 403. |
|
Correction: if PR #17497 is merged, the formal_conformance check should stop failing on fork PRs due to comment-permission 403. |
|
Related (adjacent) proposal opened for standardized review-request bundle format (schema-only):\nhttps://github.com//discussions/17510\nIssue: #17509\n\nThis is intentionally transport-agnostic; PR #16761 remains about file transfer capability. |
|
CI update: one required check is still failing on this PR. Failure:
This looks like a Windows-specific signal forwarding path/regression (likely from latest I’ll push a focused fix so |
a891424 to
11c7d67
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70a0e57bb7
ℹ️ 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".
| return { | ||
| status: "forbidden", | ||
| error: | ||
| "attachments are disabled for sessions_spawn (enable tools.sessions_spawn.attachments.enabled)", |
There was a problem hiding this comment.
Clean up provisional session when attachment setup rejects
By the time this return runs, spawnSubagentDirect has already created the child session via sessions.patch (and may have established thread binding), but no sessions.delete cleanup is performed on this rejection path. The same attachment block has other early returns for validation failures, so callers that send attachments while disabled (or otherwise invalid) can leave orphaned subagent sessions and stale thread bindings that never enter registry-based cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
| (block as { type?: unknown }).type === "toolCall" || | ||
| (block as { type?: unknown }).type === "toolUse" | ||
| ) { |
There was a problem hiding this comment.
Redact sessions_spawn payloads in functionCall blocks
repairToolCallInputs recognizes functionCall as a tool-call block, but this branch only runs sanitizeToolCallBlock for toolCall and toolUse, so functionCall entries are passed through unchanged. When a provider emits sessions_spawn as functionCall with arguments.attachments[].content, the raw attachment bytes remain in persisted transcripts, bypassing the intended content redaction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d43a86f6
ℹ️ 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".
src/agents/subagent-registry.ts
Outdated
| const shouldDeleteAttachments = cleanup === "delete" || !entry.retainAttachmentsOnKeep; | ||
| if (shouldDeleteAttachments) { | ||
| void safeRemoveAttachmentsDir(entry); |
There was a problem hiding this comment.
Delete attachments when announce cleanup succeeds
This deletion block only executes after the didAnnounce early-return path, so successful announce flows never call safeRemoveAttachmentsDir. In the common didAnnounce=true case, finalizeSubagentCleanup returns before this branch; with cleanup="delete" that means the run record is removed while the attachment directory is left on disk permanently, since later sweep logic no longer has the run metadata needed to clean it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed addressed in current HEAD (87732c8e6): finalizeSubagentCleanup calls safeRemoveAttachmentsDir in the didAnnounce=true path (subagent-registry.ts lines 694-696) before completeCleanupBookkeeping. Attachments are deleted before the run record is purged in all cleanup paths.
src/agents/subagent-spawn.ts
Outdated
| await fsPromises.rm(absDir, { recursive: true, force: true }); | ||
| const messageText = err instanceof Error ? err.message : "attachments_materialization_failed"; | ||
| return { status: "error", error: messageText }; |
There was a problem hiding this comment.
Remove child session on attachment materialization failure
When attachment staging fails, this catch only removes the attachment directory and returns an error, but it does not delete the provisional child session that was already created via sessions.patch earlier in spawnSubagentDirect. Inputs like invalid base64 or over-limit files therefore leave orphan subagent sessions that never ran and are not enrolled in registry cleanup, causing stale session buildup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
|
CI is failing in The failing assertions complain about missing/extra properties in the Is it okay to merge this, or should I re-run the CI suite until it passes? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bca6394923
ℹ️ 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".
| tools?: { sessions_spawn?: { attachments?: Record<string, unknown> } }; | ||
| } | ||
| ).tools?.sessions_spawn?.attachments; | ||
| const attachmentsEnabled = attachmentsCfg?.enabled === true; |
There was a problem hiding this comment.
Gate attachments behind a valid config key
spawnSubagentDirect enables inline attachments only when cfg.tools.sessions_spawn.attachments.enabled is true, but that key path is not valid in the current config schema (ToolsSchema is strict and does not include sessions_spawn), so adding it makes config validation fail and loadConfig() falls back to {}. This means the new attachment path cannot be enabled through normal configuration and attachment requests will always be rejected as forbidden.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
| const requestedAttachments = Array.isArray(params.attachments) | ||
| ? (params.attachments as Array<Record<string, unknown>>) | ||
| : []; | ||
| const attachAs = params.attachAs as { mountPath?: string } | undefined; |
There was a problem hiding this comment.
Reject or handle attachments for ACP runtime
The tool schema now accepts attachments for every sessions_spawn call, but in the runtime="acp" path these parsed attachments are not forwarded or validated and the spawn proceeds normally. In ACP flows this silently drops user-provided files instead of failing fast, so tasks that depend on attached input can run with missing context and no actionable error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6706aa58ae
ℹ️ 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".
| const attachmentsCfg = ( | ||
| cfg as unknown as { | ||
| tools?: { sessions_spawn?: { attachments?: Record<string, unknown> } }; | ||
| } | ||
| ).tools?.sessions_spawn?.attachments; |
There was a problem hiding this comment.
Define sessions_spawn attachment config in schema
spawnSubagentDirect reads tools.sessions_spawn.attachments to gate this feature, but that path is not part of the strict ToolsSchema (src/config/zod-schema.agent-runtime.ts), so following the returned hint (enable tools.sessions_spawn.attachments.enabled) makes config validation fail and loadConfig() falls back to {}. In practice this means attachments cannot be enabled via config and operators can accidentally invalidate their entire config when trying to turn the feature on.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
| const requestedAttachments = Array.isArray(params.attachments) | ||
| ? (params.attachments as Array<Record<string, unknown>>) | ||
| : []; | ||
| const attachAs = params.attachAs as { mountPath?: string } | undefined; |
There was a problem hiding this comment.
Reject ACP spawns when attachments are provided
The tool now accepts attachments for all sessions_spawn calls, but in the runtime="acp" path those attachments are silently ignored (they are only forwarded in the subagent branch). Calls that include attachments with ACP therefore return success without transferring files, which can break attachment-dependent workflows in a non-obvious way.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12e32e02d1
ℹ️ 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".
| const sanitized = sanitizeToolCallBlock(block); | ||
| if (sanitized !== block) { | ||
| changed = true; | ||
| messageChanged = true; | ||
| } |
There was a problem hiding this comment.
Preserve tool-name normalization in sanitized tool calls
This branch now sanitizes every toolCall/toolUse block and immediately returns, which bypasses the existing whitespace-trimming logic later in the loop; meanwhile sanitizeToolCallBlock copies block.name as-is. As a result, names like " read" are no longer normalized to "read", so downstream tool lookup can miss valid tools and produce broken tool-call/result pairing in repaired transcripts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
| } catch (err) { | ||
| await fs.rm(absDir, { recursive: true, force: true }); | ||
| const messageText = | ||
| err instanceof Error ? err.message : "attachments_materialization_failed"; | ||
| return jsonResult({ status: "error", error: messageText }); |
There was a problem hiding this comment.
Delete provisional session on attachment materialization error
On attachment staging failure, this path only removes the attachment directory and returns an error, but it does not delete childSessionKey. Since the child session is already provisioned earlier (via sessions.patch, and possibly thread binding), malformed or over-limit attachments leave orphaned child sessions and lifecycle state behind instead of rolling back the spawn attempt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fa5f9ae9e
ℹ️ 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".
| const normalized: ToolCallBlock = { | ||
| id: typeof block.id === "string" ? block.id : "unknown", | ||
| type: typeof block.type === "string" ? (block.type as "toolCall") : "toolCall", | ||
| name: typeof block.name === "string" && block.name ? block.name : "unknown", |
There was a problem hiding this comment.
Trim tool names before matching sessions_spawn redaction
sanitizeToolCallBlock now keeps block.name verbatim, so tool names like " sessions_spawn " (which still pass hasToolCallName because that path trims) no longer match the name !== "sessions_spawn" guard and skip attachment redaction entirely. In that case attachments[].content is written to persisted transcript data instead of __OPENCLAW_REDACTED__, and the untrimmed name can also miss downstream exact-name tool lookups that previously relied on normalization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a47093f939
ℹ️ 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".
| arguments: argCandidate, | ||
| }; | ||
|
|
||
| if (name !== "sessions_spawn") { |
There was a problem hiding this comment.
Trim tool name before applying sessions_spawn redaction
hasToolCallName accepts whitespace-padded names by validating block.name.trim(), but this branch compares the raw name string to "sessions_spawn"; as a result, a call like " sessions_spawn " is retained yet skips attachments[].content redaction. In that scenario, sensitive inline attachment contents can still be persisted in transcript history, and the malformed tool name is preserved for downstream processing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 265f26a22e
ℹ️ 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".
src/agents/subagent-spawn.ts
Outdated
| await callGateway({ | ||
| method: "sessions.delete", | ||
| params: { key: childSessionKey, emitLifecycleHooks: false }, | ||
| timeoutMs: 10_000, |
There was a problem hiding this comment.
Emit subagent_ended when attachment prep fails after thread bind
When thread=true has already completed subagent_spawning, the attachment error paths delete the provisional session with emitLifecycleHooks: false and return immediately, so subagent_ended is never emitted. Channel plugins like Discord rely on subagent_ended to unbind thread/session routing, so a spawn that fails during attachment gating/materialization can leave stale thread bindings behind and misroute later messages for that thread.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
| arguments: argCandidate, | ||
| }; | ||
|
|
||
| if (name !== "sessions_spawn") { | ||
| return normalized; |
There was a problem hiding this comment.
Preserve toolUse input shape when sanitizing non-spawn calls
This now normalizes every tool call block into the normalized object and returns it for all non-sessions_spawn tools, which drops the original input field and any other provider-specific block properties. Since sanitizeToolCallInputs runs over full session history before model calls, existing toolUse entries are rewritten into a different schema and can be rejected by strict providers that validate the original tool-call shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
|
trigger ci |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48623f860f
ℹ️ 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".
| } else { | ||
| nextContent.push(block); |
There was a problem hiding this comment.
Preserve tool-name trimming for non-sessions_spawn calls
This branch now passes non-sessions_spawn tool-call blocks through unchanged, so names like " read"/"exec " are no longer normalized even though hasToolCallName validates using a trimmed value. That reintroduces tool-name mismatches for transcripts where providers emit surrounding whitespace, and downstream exact-name tool dispatch can fail even though the call was accepted here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
| const result = await spawnAcpDirect( | ||
| { | ||
| task, | ||
| label: label || undefined, | ||
| agentId: requestedAgentId, |
There was a problem hiding this comment.
Reject attachments when runtime is ACP
attachments are parsed from input, but the ACP path forwards only task/session fields to spawnAcpDirect and returns success without any warning, so runtime:"acp" calls silently drop file payloads. In workflows that rely on attachment context (for example image/file analysis), this produces incorrect subagent behavior while the caller still sees an accepted spawn result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
Greptile SummaryThis PR implements robust inline attachments for Key changes verified:
Previous review feedback: Minor style issue: Confidence Score: 4/5
Last reviewed commit: 33c4c21 |
Additional Comments (1)
Since Add cleanup before returning error: } catch (err) {
// Clean up materialized attachments on spawn failure
if (attachmentAbsDir) {
await fs.rm(attachmentAbsDir, { recursive: true, force: true }).catch(() => {});
}
if (threadBindingReady) {
// ... existing hook logic ...
}
// ... rest of error handling ...
} |
|
Addressed ✅\n\nThanks for the catch — fixed the spawn-failure attachment cleanup path and pushed an update.\n\n### What changed\n- In , the spawn failure path now removes staged attachments in a guarded best-effort block (), so cleanup failures do not mask the original error.\n- Added a second hardening path: if fails after child session start, we now:\n 1) best-effort remove staged attachments,\n 2) best-effort delete the provisional child session ( with ),\n 3) return an explicit registration-failure error.\n\nThis closes the leak window where attachment dirs could be left behind on failure paths before/around run registration. |
|
Follow-up with properly formatted details (previous comment had shell formatting glitches): Addressed ✅ Thanks for the catch — fixed the spawn-failure attachment cleanup path and pushed an update. What changed:
This closes the leak window where attachment dirs could be left behind on failure paths before/around run registration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0b69eac5
ℹ️ 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".
| typeof (block as { name?: unknown }).name === "string" | ||
| ? (block as { name: string }).name.trim() | ||
| : undefined; | ||
| if (blockName === "sessions_spawn") { |
There was a problem hiding this comment.
Match sessions_spawn redaction against normalized names
Redaction only runs when blockName === "sessions_spawn", which is case-sensitive, while name validation/allowlisting treats tool names case-insensitively (trim().toLowerCase()). If a model emits a valid mixed-case variant like SESSIONS_SPAWN, this branch is skipped and attachments[].content is written to persisted transcript data unredacted, leaking inline attachment contents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
| } else { | ||
| nextContent.push(block); |
There was a problem hiding this comment.
Restore tool-name trimming before persisting repaired blocks
This branch now forwards non-sessions_spawn tool calls unchanged, so names like " read " pass validation (because checks use trim()) but remain untrimmed in repaired output. Downstream tool dispatch and transcript replay commonly key on exact tool names, so these whitespace variants can fail to resolve and break runs that previously succeeded after normalization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c6caa3cbf
ℹ️ 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".
| if (nextArgs === block.arguments && nextInput === block.input) { | ||
| return normalized; |
There was a problem hiding this comment.
Preserve toolUse input shape when no attachment redaction occurs
When a sessions_spawn tool call is stored in toolUse form without an attachments array, redactSessionsSpawnAttachmentsArgs returns the original objects and this fast path returns normalized, which only carries arguments and drops the original input. That mutates provider-specific transcript shape even though no redaction was needed, and can break replay on strict providers (the same function already notes toolUse.input must be preserved for Vertex/Anthropic-compatible payloads).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8bc86ce66
ℹ️ 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".
src/agents/subagent-spawn.ts
Outdated
| method: "sessions.delete", | ||
| params: { key: childSessionKey, emitLifecycleHooks: false }, | ||
| timeoutMs: 10_000, |
There was a problem hiding this comment.
Emit subagent end hooks on attachment staging failures
When thread=true, this attachment error path can run after thread binding succeeds, but it always deletes the provisional session with emitLifecycleHooks: false and never calls runSubagentEnded. That differs from the existing agent-start failure path and skips subagent_ended plugin cleanup for invalid/oversized attachment failures, so channel-specific teardown behavior can be missed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 505a6ad70 (fix(sessions-spawn): handle attachment edge-cases for hooks and acp). The cleanupProvisionalSession function now accepts emitLifecycleHooks and deleteTranscript options, and attachment error paths pass emitLifecycleHooks: threadBindingReady — so subagent_ended is correctly emitted when thread binding was already established before the failure.
| if (runtime === "acp") { | ||
| const result = await spawnAcpDirect( | ||
| { | ||
| task, | ||
| label: label || undefined, |
There was a problem hiding this comment.
Reject ACP runtime when attachments are supplied
attachments are accepted by the tool schema and parsed from input, but the ACP branch ignores them and calls spawnAcpDirect without any attachment handling. For runtime="acp", the spawn can still be accepted while all provided files are silently dropped, causing attachment-dependent subagent tasks to execute with missing inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 505a6ad70. The ACP branch now returns an explicit error when attachments are provided (attachments are currently unsupported for runtime=acp) before calling spawnAcpDirect, ensuring callers get a clear actionable error instead of silent file drops. Test added in sessions-spawn-tool.test.ts.
|
Closing temporarily to retrigger review bots; will reopen in ~10s. |
Additional Comments (1)
since |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 505a6ad704
ℹ️ 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".
src/agents/subagent-spawn.ts
Outdated
| emitLifecycleHooks: options?.emitLifecycleHooks === true, | ||
| deleteTranscript: options?.deleteTranscript === true, |
There was a problem hiding this comment.
Delete child session after thread-bind failure
In the bindResult.status === "error" cleanup path, the sessions.delete params read options?.emitLifecycleHooks and options?.deleteTranscript, but options is not defined in this scope. That raises a ReferenceError while building the request object, the surrounding catch swallows it, and the provisional subagent session is never deleted. This occurs when thread=true and runSubagentSpawning returns an error/not-ready result, leaving orphaned child sessions and transcripts instead of performing the intended rollback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1b349da2d
ℹ️ 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".
src/agents/subagent-registry.ts
Outdated
| const shouldDeleteAttachments = cleanup === "delete" || !entry.retainAttachmentsOnKeep; | ||
| if (shouldDeleteAttachments) { | ||
| await safeRemoveAttachmentsDir(entry); |
There was a problem hiding this comment.
Clean attachment dirs when terminating subagent runs
Attachment cleanup is only wired into finalizeSubagentCleanup/sweeper, but kill/abort flows call markSubagentRunTerminated (used by stopSubagentsForRequester and /subagents kill) which marks runs as already cleaned (cleanupHandled/cleanupCompletedAt) and bypasses this path entirely; for session-mode runs (archiveAtMs unset), that leaves attachmentsDir on disk indefinitely even when retainAttachmentsOnKeep is false. This causes persistent sensitive-file leakage after manual termination, which is a common operational path.
Useful? React with 👍 / 👎.
… and docs Add inline file attachment support for sessions_spawn (subagent runtime only): - Schema: attachments[] (name, content, encoding, mimeType) and attachAs.mountPath hint - Materialization: files written to .openclaw/attachments/<uuid>/ with manifest.json - Validation: strict base64 decode, filename checks, size limits, duplicate detection - Transcript redaction: sanitizeToolCallInputs redacts attachment content from persisted transcripts - Lifecycle cleanup: safeRemoveAttachmentsDir with symlink-safe path containment check - Config: tools.sessions_spawn.attachments (enabled, maxFiles, maxFileBytes, maxTotalBytes, retainOnSessionKeep) - Registry: attachmentsDir/attachmentsRootDir/retainAttachmentsOnKeep on SubagentRunRecord - ACP rejection: attachments rejected for runtime=acp with clear error message - Docs: updated tools/index.md, concepts/session-tool.md, configuration-reference.md - Tests: 85 new/updated tests across 5 test files Fixes: - Guard fs.rm in materialization catch block with try/catch (review concern openclaw#1) - Remove unreachable fallback in safeRemoveAttachmentsDir (review concern openclaw#7) - Move attachment cleanup out of retry path to avoid timing issues with announce loop Co-authored-by: napetrov <napetrov@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdab566540
ℹ️ 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".
| }; | ||
|
|
||
| try { | ||
| await fs.mkdir(absDir, { recursive: true, mode: 0o700 }); |
There was a problem hiding this comment.
Reject symlinked attachment roots before writing files
sessions_spawn stages attachments with fs.mkdir(absDir, { recursive: true }) and then writes to path.join(absDir, name) without verifying that .openclaw/attachments is a real directory under the child workspace. If a prior (untrusted) run has replaced that path with a symlink, staging will follow it and write attachment contents outside the intended workspace, and later cleanup will act on that external location as well; this breaks the isolation guarantees for inline attachments and can leak or delete data in attacker-chosen paths.
Useful? React with 👍 / 👎.
Fixes #17214
Image-specific request remains tracked in #16344 as a special case of general file attachments.
Summary
This Pull Request introduces robust support for passing inline attachments to sub-agents via
sessions_spawn. This enables more complex workflows where agents require specific files, such as images for analysis (e.g., in response to a user's request to "analyze this image"), code snippets, or other structured data, making it a more comprehensive solution for inter-agent communication.This feature moves beyond basic text exchange, addressing a crucial need for agents to operate on concrete file inputs.
Core Idea
The goal is to allow a spawning agent (or user) to provide files directly with a
sessions_spawncall. These files are then safely materialized into the target sub-agent's isolated workspace, making them accessible to the sub-agent's tools and thought process. This allows for scenarios like:Implemented Details
attachments[]support tosessions_spawn(accepts inline content asutf8orbase64encoded strings)..openclaw/attachments/<uuid>/.manifest.jsonfile within this directory, containing metadata for all attached files (name, size, SHA256 hash).count,totalBytes,files,relDir) in thesessions_spawntool result, allowing the spawning agent to confirm successful transfer.relDirwhere attachments can be found (themountPathconcept is explicitly reserved for future, more advanced mount-based implementations and is not used as a runtime path authority in this implementation).Security and Robustness Hardening
Extensive measures have been taken to ensure the security and integrity of attachment handling:
sessions_spawnattachmentcontentis strictly redacted from transcript persistence (bothargumentsandinputtool-call shapes) to prevent accidental logging of sensitive data.agent.callGateway(the actual sub-agent spawn) fails after attachments have been materialized, the staged attachment directory is removed according to the defined cleanup policy.cleanup=deleteorretainOnSessionKeep=false) is fully wired into the sub-agent registry'sfinalizeprocess, ensuring attachment directories are reliably removed after the sub-agent run completes or its session is archived.0700and files/manifests to0600, ensuring only the owner (the sub-agent process) can access them.Tests Run
The following tests have been run and passed:
pnpm lint --silent(static code analysis)pnpm exec vitest run --config vitest.unit.config.ts src/agents/session-transcript-repair.attachments.test.ts(unit tests for transcript redaction and base64 handling)pnpm exec vitest run --config vitest.e2e.config.ts src/agents/openclaw-tools.sessions.e2e.test.ts(end-to-end tests forsessions_spawnfunctionality)Reviewer Guide
What to look for:
Security (Critical):
sessions_spawn-tool.ts:decodeStrictBase64logic: Confirm themaxEncodedBytescalculation and its enforcement beforeBuffer.fromto prevent memory DoS.fs.mkdir,fs.writeFile): Verifymode(0700/0600) andflag: "wx"(no overwrite).try/catchblock: Ensurefs.rm(absDir, { recursive: true, force: true })is called on any failure to prevent residue.subagent-registry.ts:safeRemoveAttachmentsDir: Verify thefs.realpathandstartsWith(rootWithSep)checks are robust against path traversal for deletion.finalizeSubagentCleanup: EnsureshouldDeleteAttachmentslogic correctly triggerssafeRemoveAttachmentsDirbased oncleanupandretainAttachmentsOnKeep.session-transcript-repair.ts:sanitizeToolCallInputs: Confirmattachments[].contentis redacted for bothargumentsandinputfields insessions_spawntool calls.Correctness (High):
sessions_spawn-tool.ts:decodeStrictBase64: Test with various valid, invalid, and edge-case base64 strings (e.g., malformed padding, non-base64 chars, empty, very long inputs).maxFiles,maxTotalBytes,maxFileBytes): Ensure these are correctly enforced at various stages..../\\\^@and ensure they are rejected.relDirgeneration: Confirm it is deterministic and reliably points to the attachment location in the child workspace.relDirand the untrusted nature of attachments.subagent-registry.ts:registerSubagentRun: EnsureattachmentsDirandretainAttachmentsOnKeepare correctly recorded.finalizeSubagentCleanup: Verify the timing and conditions forsafeRemoveAttachmentsDirare correct (i.e., it doesn't try to delete before creation, and doesn't leave residue).Architecture / Design (Medium):
sessions_spawn-tool.ts:attachAs.mountPathfield is indeed not used for path resolution in the MVP, but retained as a placeholder (and documented as such in the code comments).Suggested Manual Test Cases:
readthe file at the reportedrelDir.readthe file (e.g., check its size or first few bytes).maxFileBytes. Expect rejection.totalBytesexceedmaxTotalBytes. Expect rejection (even if individual files are fine).maxFiles. Expect rejection.content. Expectattachments_invalid_base64_or_too_largeerror...///\\\^@or null bytes in thename. Expectattachments_invalid_nameerror.name. Expectattachments_duplicate_nameerror.fs.writeFilein a test environment, if possible). Verify that no residual attachment directory or files are left.sessions_spawnwith valid attachments, but ensure the actual sub-agent spawn (callGateway<...>(method: "agent", ...)fails (e.g., invalidagentId). Verify that the attachment directory is removed.cleanup=delete: Spawn an agent with attachments andcleanup="delete". Verify the attachment directory is removed after the sub-agent completes successfully.cleanup=keep(default): Spawn an agent with attachments andcleanup="keep". Verify the attachment directory is not removed after the sub-agent completes.Mentions: #16344
AI Assistance & Contributing Transparency
Greptile Summary
This PR implements inline attachment support for
sessions_spawn, enabling sub-agents to receive files (images, code snippets, etc.) as part of their spawning context. The implementation includes comprehensive security hardening based on extensive prior review feedback.Key Changes:
attachments[]parameter tosessions_spawnwith UTF-8 and base64 encoding support.openclaw/attachments/<uuid>/sanitizeToolCallInputsfs.realpath+ prefix validation for attachment deletionSecurity Posture:
Previous review rounds successfully addressed multiple critical security issues:
argumentsandinputtranscript fieldsflag: "wx")Issues Found:
safeRemoveAttachmentsDirpath resolution (line 588) - uses wrong variable in ternary condition, though functionally safe due to prior null checkConfidence Score: 4/5
Last reviewed commit: 3044941