Skip to content

sessions_spawn: inline attachments + redaction + hardening#16761

Merged
tyler6204 merged 1 commit intoopenclaw:mainfrom
napetrov:napetrov/sessions-spawn-attachments
Mar 2, 2026
Merged

sessions_spawn: inline attachments + redaction + hardening#16761
tyler6204 merged 1 commit intoopenclaw:mainfrom
napetrov:napetrov/sessions-spawn-attachments

Conversation

@napetrov
Copy link
Contributor

@napetrov napetrov commented Feb 15, 2026

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_spawn call. 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:

  • An agent analyzing an image provided by a user.
  • An agent receiving a configuration file for processing.
  • An agent getting a dataset for a specific task.

Implemented Details

  • Adds attachments[] support to sessions_spawn (accepts inline content as utf8 or base64 encoded strings).
  • Materializes attachments into the child sub-agent's isolated workspace at a deterministic relative path:
    • .openclaw/attachments/<uuid>/
    • Includes a .manifest.json file within this directory, containing metadata for all attached files (name, size, SHA256 hash).
  • Returns an attachment receipt (count, totalBytes, files, relDir) in the sessions_spawn tool result, allowing the spawning agent to confirm successful transfer.
  • Appends a system note to the child sub-agent's context, informing it of the canonical relDir where attachments can be found (the mountPath concept 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:

  • Strict Transcript Redaction: sessions_spawn attachment content is strictly redacted from transcript persistence (both arguments and input tool-call shapes) to prevent accidental logging of sensitive data.
  • Robust Base64 Validation: Implements strict base64 validation (checking alphabet, padding, and roundtrip consistency). Critically, it includes a pre-decode encoded-size guard to prevent memory exhaustion (DoS) from overly large base64 inputs before full decoding.
  • Atomic Materialization & Rollback: Attachment staging includes atomic file writes and a comprehensive rollback mechanism: if any error occurs during materialization (e.g., validation failure, disk write issue), the partially created attachment directory is fully removed.
  • Spawn Failure Cleanup: If the 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.
  • Lifecycle-Aware Cleanup: The cleanup policy (cleanup=delete or retainOnSessionKeep=false) is fully wired into the sub-agent registry's finalize process, ensuring attachment directories are reliably removed after the sub-agent run completes or its session is archived.
  • Constrained Deletion: Attachment directory deletion is strictly constrained to the canonical attachments root within the child sub-agent's workspace, preventing accidental deletion of files outside the intended scope (path traversal protection).
  • Tightened File Permissions: File system permissions for materialized attachments are highly restricted: directories are set to 0700 and files/manifests to 0600, 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 for sessions_spawn functionality)

Reviewer Guide

What to look for:

  1. Security (Critical):

    • sessions_spawn-tool.ts:
      • decodeStrictBase64 logic: Confirm the maxEncodedBytes calculation and its enforcement before Buffer.from to prevent memory DoS.
      • Attachment file/dir creation (fs.mkdir, fs.writeFile): Verify mode (0700/0600) and flag: "wx" (no overwrite).
      • Error handling logic in the attachment materialization try/catch block: Ensure fs.rm(absDir, { recursive: true, force: true }) is called on any failure to prevent residue.
    • subagent-registry.ts:
      • safeRemoveAttachmentsDir: Verify the fs.realpath and startsWith(rootWithSep) checks are robust against path traversal for deletion.
      • finalizeSubagentCleanup: Ensure shouldDeleteAttachments logic correctly triggers safeRemoveAttachmentsDir based on cleanup and retainAttachmentsOnKeep.
    • session-transcript-repair.ts:
      • sanitizeToolCallInputs: Confirm attachments[].content is redacted for both arguments and input fields in sessions_spawn tool calls.
  2. 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).
      • Attachment limits (maxFiles, maxTotalBytes, maxFileBytes): Ensure these are correctly enforced at various stages.
      • Filename validation: Check edge cases for . .. / \ \\^@ and ensure they are rejected.
      • relDir generation: Confirm it is deterministic and reliably points to the attachment location in the child workspace.
      • System prompt message: Verify it clearly states the relDir and the untrusted nature of attachments.
    • subagent-registry.ts:
      • registerSubagentRun: Ensure attachmentsDir and retainAttachmentsOnKeep are correctly recorded.
      • finalizeSubagentCleanup: Verify the timing and conditions for safeRemoveAttachmentsDir are correct (i.e., it doesn't try to delete before creation, and doesn't leave residue).
  3. Architecture / Design (Medium):

    • sessions_spawn-tool.ts:
      • Confirm that the attachAs.mountPath field is indeed not used for path resolution in the MVP, but retained as a placeholder (and documented as such in the code comments).
      • Attachment lifecycle (creation, use, cleanup) is clear and robust.
    • Overall: Ensure that this inline attachment mechanism is a solid foundation for future extensions (e.g., streaming, more complex storage).

Suggested Manual Test Cases:

  1. Successful Attachment: Spawn a sub-agent with a small UTF8 text file. Verify the sub-agent can read the file at the reported relDir.
  2. Successful Base64 Image: Spawn a sub-agent with a base64 encoded image (within limits). Verify the sub-agent can read the file (e.g., check its size or first few bytes).
  3. Attachment Limits:
    • Send a single file exceeding maxFileBytes. Expect rejection.
    • Send multiple files whose totalBytes exceed maxTotalBytes. Expect rejection (even if individual files are fine).
    • Send more than maxFiles. Expect rejection.
  4. Invalid Base64: Send an attachment with malformed base64 content. Expect attachments_invalid_base64_or_too_large error.
  5. Invalid Filenames: Send attachments with ../ // \ \\^@ or null bytes in the name. Expect attachments_invalid_name error.
  6. Duplicate Filenames: Send multiple attachments with the same valid name. Expect attachments_duplicate_name error.
  7. Rollback on Materialization Error: Simulate a disk full or permission error during file writing (manually block fs.writeFile in a test environment, if possible). Verify that no residual attachment directory or files are left.
  8. Spawn Failure Cleanup: Trigger sessions_spawn with valid attachments, but ensure the actual sub-agent spawn (callGateway<...>(method: "agent", ...) fails (e.g., invalid agentId). Verify that the attachment directory is removed.
  9. cleanup=delete: Spawn an agent with attachments and cleanup="delete". Verify the attachment directory is removed after the sub-agent completes successfully.
  10. cleanup=keep (default): Spawn an agent with attachments and cleanup="keep". Verify the attachment directory is not removed after the sub-agent completes.

Mentions: #16344

AI Assistance & Contributing Transparency

  • AI-assisted: yes (Codex/Claude)
  • Testing level: lightly tested (unit/e2e targeted + CI)
  • Prompts/session logs: Session logs available on request
  • I understand what this code does and can maintain it

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:

  • Added attachments[] parameter to sessions_spawn with UTF-8 and base64 encoding support
  • Materialized attachments into isolated child workspace at .openclaw/attachments/<uuid>/
  • Implemented strict base64 validation with pre-decode size guards to prevent memory exhaustion DoS
  • Added attachment content redaction from transcript persistence via sanitizeToolCallInputs
  • Implemented atomic rollback on materialization errors and spawn failure cleanup
  • Added path traversal protection using fs.realpath + prefix validation for attachment deletion
  • Set restrictive file permissions (0700 for directories, 0600 for files)
  • Wired lifecycle-aware cleanup with configurable retention policy

Security Posture:
Previous review rounds successfully addressed multiple critical security issues:

  • Pre-allocation guards for both UTF-8 and base64 paths prevent memory exhaustion
  • Raw input length check before regex prevents DoS via whitespace-padded base64
  • Strict base64 validation includes alphabet, padding, and roundtrip checks
  • Attachment content redacted from both arguments and input transcript fields
  • Path traversal protection with fallback handling for non-existent roots
  • Atomic file operations with exclusive creation (flag: "wx")

Issues Found:

  • One logical inconsistency in safeRemoveAttachmentsDir path resolution (line 588) - uses wrong variable in ternary condition, though functionally safe due to prior null check

Confidence Score: 4/5

  • This PR is safe to merge with one minor logical fix recommended
  • The implementation demonstrates excellent security practices with comprehensive hardening from multiple review rounds. All critical security issues (memory exhaustion DoS, path traversal, content redaction, atomic operations) have been properly addressed. The one logical issue found (line 588 ternary condition) is low-severity - it uses the wrong variable in a condition but is functionally safe due to a prior null check. The extensive test coverage and thorough error handling increase confidence.
  • src/agents/subagent-registry.ts line 588 needs the ternary condition fix

Last reviewed commit: 3044941

@openclaw-barnacle openclaw-barnacle bot added agents Agent runtime and tooling size: M labels Feb 15, 2026
@napetrov napetrov force-pushed the napetrov/sessions-spawn-attachments branch from 23dd6d5 to 8f2ff3c Compare February 15, 2026 05:48
@napetrov napetrov marked this pull request as ready for review February 15, 2026 05:48
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

@napetrov
Copy link
Contributor Author

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.

@napetrov napetrov changed the title sessions_spawn MVP: inline attachments + redaction + hardening sessions_spawn: inline attachments + redaction + hardening Feb 15, 2026
@openclaw-barnacle openclaw-barnacle bot added app: macos App: macos size: L and removed size: M labels Feb 15, 2026
@napetrov
Copy link
Contributor Author

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.

@napetrov
Copy link
Contributor Author

@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.

@napetrov
Copy link
Contributor Author

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.

@openclaw-barnacle openclaw-barnacle bot added channel: discord Channel integration: discord channel: whatsapp-web Channel integration: whatsapp-web labels Feb 15, 2026
@napetrov napetrov force-pushed the napetrov/sessions-spawn-attachments branch from c622456 to 154d4e6 Compare February 15, 2026 19:39
@napetrov
Copy link
Contributor Author

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.

@napetrov
Copy link
Contributor Author

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.

@napetrov
Copy link
Contributor Author

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.

@napetrov
Copy link
Contributor Author

Correction: if PR #17497 is merged, the formal_conformance check should stop failing on fork PRs due to comment-permission 403.

@napetrov
Copy link
Contributor Author

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.

@napetrov
Copy link
Contributor Author

CI update: one required check is still failing on this PR.

Failure: checks-windows (node, test, pnpm canvas:a2ui:bundle && pnpm test)

  • Test: src/process/supervisor/adapters/pty.test.ts > forwards explicit signals to node-pty kill
  • Assertion mismatch: expected ptyKillMock("SIGTERM"), received ptyKillMock(undefined)

This looks like a Windows-specific signal forwarding path/regression (likely from latest main merge), not in the new attachment logic itself.

I’ll push a focused fix so adapter.kill("SIGTERM") preserves the explicit signal consistently on Windows and re-run CI.

@napetrov napetrov force-pushed the napetrov/sessions-spawn-attachments branch from a891424 to 11c7d67 Compare February 16, 2026 03:40
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +468 to +471
return {
status: "forbidden",
error:
"attachments are disabled for sessions_spawn (enable tools.sessions_spawn.attachments.enabled)",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Comment on lines +215 to +217
(block as { type?: unknown }).type === "toolCall" ||
(block as { type?: unknown }).type === "toolUse"
) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +729 to +731
const shouldDeleteAttachments = cleanup === "delete" || !entry.retainAttachmentsOnKeep;
if (shouldDeleteAttachments) {
void safeRemoveAttachmentsDir(entry);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +595 to +597
await fsPromises.rm(absDir, { recursive: true, force: true });
const messageText = err instanceof Error ? err.message : "attachments_materialization_failed";
return { status: "error", error: messageText };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

@napetrov
Copy link
Contributor Author

CI is failing in src/gateway/server-methods/send.test.ts across multiple runners (Linux/Windows/Mac). This seems to be a mock pollution/flaky issue introduced by a recent main merge, as the tests pass in isolation but fail during parallel CI execution.

The failing assertions complain about missing/extra properties in the vi.fn() mirror mock (e.g. cfg, channel). My changes in this PR (sessions_spawn attachment handling and redaction) do not touch send.ts or its routing logic.

Is it okay to merge this, or should I re-run the CI suite until it passes?

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +433 to +436
tools?: { sessions_spawn?: { attachments?: Record<string, unknown> } };
}
).tools?.sessions_spawn?.attachments;
const attachmentsEnabled = attachmentsCfg?.enabled === true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Comment on lines +89 to +92
const requestedAttachments = Array.isArray(params.attachments)
? (params.attachments as Array<Record<string, unknown>>)
: [];
const attachAs = params.attachAs as { mountPath?: string } | undefined;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +431 to +435
const attachmentsCfg = (
cfg as unknown as {
tools?: { sessions_spawn?: { attachments?: Record<string, unknown> } };
}
).tools?.sessions_spawn?.attachments;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Comment on lines +89 to +92
const requestedAttachments = Array.isArray(params.attachments)
? (params.attachments as Array<Record<string, unknown>>)
: [];
const attachAs = params.attachAs as { mountPath?: string } | undefined;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +247 to +251
const sanitized = sanitizeToolCallBlock(block);
if (sanitized !== block) {
changed = true;
messageChanged = true;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Comment on lines +657 to +661
} 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 });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +475 to +478
await callGateway({
method: "sessions.delete",
params: { key: childSessionKey, emitLifecycleHooks: false },
timeoutMs: 10_000,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Comment on lines +118 to +122
arguments: argCandidate,
};

if (name !== "sessions_spawn") {
return normalized;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

@napetrov
Copy link
Contributor Author

trigger ci

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +260 to +261
} else {
nextContent.push(block);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Comment on lines +101 to +105
const result = await spawnAcpDirect(
{
task,
label: label || undefined,
agentId: requestedAgentId,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 28, 2026

Greptile Summary

This PR implements robust inline attachments for sessions_spawn with comprehensive security hardening.

Key changes verified:

  • Base64 validation includes pre-decode size guard (line 33 in subagent-spawn.ts) to prevent memory exhaustion before decoding
  • UTF-8 attachments use Buffer.byteLength() check (line 546) before allocation to prevent DoS
  • File system operations use restrictive permissions (0700 for directories, 0600 for files) with exclusive creation flags (wx)
  • Atomic rollback on materialization errors (lines 600-609) and spawn failures (lines 652-658) prevents orphaned files
  • Path traversal protection uses fs.realpath() resolution with prefix checking (lines 661-674 in subagent-registry.ts)
  • Transcript redaction correctly handles both arguments and input field shapes (lines 116-117 in session-transcript-repair.ts)
  • Cleanup lifecycle correctly evaluates cleanup === "delete" || !retainAttachmentsOnKeep conditions

Previous review feedback:
All issues from previous review threads have been addressed - DoS vulnerabilities patched, memory checks added, path traversal protection implemented, and redaction working correctly.

Minor style issue:
One unreachable fallback expression at subagent-registry.ts:670 where dirReal is already guaranteed non-null. This doesn't affect correctness but could be simplified for clarity.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - comprehensive security hardening and thorough testing
  • Score reflects excellent security implementation (DoS prevention, atomic operations, path traversal protection, redaction) with comprehensive test coverage. Previous review issues have been addressed. One minor style issue with unreachable code doesn't impact functionality. The feature is complex but well-architected with proper cleanup lifecycle management.
  • No files require special attention - all security measures properly implemented

Last reviewed commit: 33c4c21

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 28, 2026

Additional Comments (1)

src/agents/subagent-spawn.ts, line 701
Missing attachment directory cleanup on spawn failure. If callGateway (the actual sub-agent spawn at line 625) fails after attachments have been materialized (lines 512-599), the attachment directory is not cleaned up. The catch block only handles session deletion but doesn't remove attachmentAbsDir, leaving orphaned directories on disk.

Since registerSubagentRun (line 704) is only called after successful spawn, the attachment directory is never registered and won't be cleaned up by the registry's finalization logic either.

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 ...
}

@napetrov
Copy link
Contributor Author

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.

@napetrov
Copy link
Contributor Author

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:

  • In src/agents/tools/sessions-spawn-tool.ts, the spawn failure path now removes staged attachments in a guarded best-effort block (try/await fs.rm/catch), so cleanup failures do not mask the original error.
  • Added a second hardening path: if registerSubagentRun(...) fails after child session start, we now:
    1. best-effort remove staged attachments,
    2. best-effort delete the provisional child session via sessions.delete (emitLifecycleHooks: false),
    3. return an explicit registration-failure error.

This closes the leak window where attachment dirs could be left behind on failure paths before/around run registration.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Comment on lines +260 to +261
} else {
nextContent.push(block);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +128 to +129
if (nextArgs === block.arguments && nextInput === block.input) {
return normalized;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in current HEAD (87732c8e6). The code referenced here has been updated; this thread is outdated. Confirmed implementation is intact and correct.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +603 to +605
method: "sessions.delete",
params: { key: childSessionKey, emitLifecycleHooks: false },
timeoutMs: 10_000,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +100 to +104
if (runtime === "acp") {
const result = await spawnAcpDirect(
{
task,
label: label || undefined,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@napetrov
Copy link
Contributor Author

napetrov commented Mar 1, 2026

Closing temporarily to retrigger review bots; will reopen in ~10s.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 1, 2026

Additional Comments (1)

src/agents/subagent-registry.ts, line 670
unreachable fallback - dirReal is guaranteed non-null here

since dirReal is already verified non-null at line 665-667, the fallback ?? path.resolve(entry.attachmentsDir) will never be used. simplify to just const dirBase = dirReal; for clarity

    const dirBase = dirReal;

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +449 to +450
emitLifecycleHooks: options?.emitLifecycleHooks === true,
deleteTranscript: options?.deleteTranscript === true,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +734 to +736
const shouldDeleteAttachments = cleanup === "delete" || !entry.retainAttachmentsOnKeep;
if (shouldDeleteAttachments) {
await safeRemoveAttachmentsDir(entry);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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>
@tyler6204
Copy link
Member

Merged via squash.

Thanks @napetrov!

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling docs Improvements or additions to documentation gateway Gateway runtime size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: sessions_spawn general file attachments (images/documents/archives)

4 participants