Skip to content

chore(#1150): remove rsync from sandbox image - #2120

Merged
rh-hemartin merged 2 commits into
mainfrom
agent/1150-remove-rsync-from-sandbox
Jun 12, 2026
Merged

chore(#1150): remove rsync from sandbox image#2120
rh-hemartin merged 2 commits into
mainfrom
agent/1150-remove-rsync-from-sandbox

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

PR #761 replaced ssh/scp/rsync with OpenShell native transport (openshell sandbox exec/upload/download), but the Containerfile was not updated. Remove the stale rsync dependency to reduce image size and attack surface.

Changes:

  • Remove rsync from apt-get install in
    images/sandbox/Containerfile
  • Remove rsync mention from the Containerfile header comment
  • Update images/README.md tool list to drop rsync
  • Update images/code/Containerfile base image comment to
    list jq instead of rsync

Note: make lint could not run due to Go module cache permission errors in the sandbox (infrastructure issue, not related to this change). No Go code was modified.


Closes #1150

Post-script verification

  • Branch is not main/master (agent/1150-remove-rsync-from-sandbox)
  • Secret scan passed (gitleaks — 1281fa9973718b519e2a2404f247cf26baf6521f..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

PR #761 replaced ssh/scp/rsync with OpenShell native transport
(openshell sandbox exec/upload/download), but the Containerfile
was not updated. Remove the stale rsync dependency to reduce
image size and attack surface.

Changes:
- Remove rsync from apt-get install in
  images/sandbox/Containerfile
- Remove rsync mention from the Containerfile header comment
- Update images/README.md tool list to drop rsync
- Update images/code/Containerfile base image comment to
  list jq instead of rsync

Note: make lint could not run due to Go module cache
permission errors in the sandbox (infrastructure issue,
not related to this change). No Go code was modified.

Closes #1150
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown

Site preview

Preview: https://341e69f8-site.fullsend-ai.workers.dev

Commit: c6bae71698b0a012abb703a9551ee6ce1245bfbf

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:38 PM UTC · Completed 1:46 PM UTC
Commit: 4ed6da4 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [stale-reference] hack/patch-fullsend-repo:51 — This script invokes rsync -a --checksum ... to sync scaffold files into a cloned repo. It is a developer host utility (uses gh repo clone, mktemp -d, gh pr create) and never runs inside a sandbox container. Removing rsync from the sandbox image has no impact on this script.

  • [stale-doc] docs/ADRs/0030-openshell-sandbox-interaction-model.md:143 — ADR 0030 documents the original decision to use rsync for repo extraction. As an ADR (historical record of a point-in-time decision), this is not actively misleading — the ADR's own Consequences section already anticipates the rsync replacement. Optionally add a supersession note referencing PR Replace SSH/SCP/rsync with OpenShell native CLI commands #761.

Info

  • [stale-doc] docs/ADRs/0030-openshell-sandbox-interaction-model.md:175 — References rsync security filters as protections against sandbox escape vectors. These protections have been migrated to sanitizeDownload() in internal/sandbox/sandbox.go. No action required.
Previous run

Review

Findings

Medium

  • [stale-doc] docs/architecture.md:595 — Architecture diagram references "rsync repo back (--no-links, exclude .git/hooks/)" as part of the extraction flow. The actual implementation in internal/sandbox/sandbox.go now uses SafeDownload + sanitizeDownload (OpenShell native download). The text is inaccurate.
    Remediation: Update line 595 to reflect the current mechanism (e.g., "download repo back (sanitize symlinks, exclude .git/hooks/)" via SafeDownload).

Low

  • [stale-reference] hack/patch-fullsend-repo:51 — This script invokes rsync -a --checksum ... to sync scaffold files into a cloned repo. This is a developer host utility (uses gh repo clone, mktemp -d, gh pr create) and never runs inside a sandbox container. Removing rsync from the sandbox image has no impact on this script.

  • [stale-doc] docs/ADRs/0030-openshell-sandbox-interaction-model.md:143 — ADR 0030 documents the original decision to use rsync for repo extraction. As an ADR (historical record of a point-in-time decision), this is not actively misleading — the ADR's own Consequences section already anticipates the rsync replacement. Optionally add a supersession note referencing PR Replace SSH/SCP/rsync with OpenShell native CLI commands #761.

Info

  • [stale-doc] docs/ADRs/0030-openshell-sandbox-interaction-model.md:175 — References rsync security filters as protections against sandbox escape vectors. These protections have been migrated to sanitizeDownload() in internal/sandbox/sandbox.go. No action required.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 10, 2026
@ralphbean
ralphbean requested a review from maruiz93 June 10, 2026 21:07

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. One non-blocking note inline.


# ---------------------------------------------------------------------------
# Install rsync (for safe repo write-back) and jq (JSON parsing in scripts).
# Install jq (JSON parsing in agent scripts and skills).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[minor] docs/architecture.md:595 still says "rsync repo back" in the extraction flow diagram. Might be worth updating to reflect SafeDownload while we're here — but not blocking on it.

@rh-hemartin

Copy link
Copy Markdown
Member

/fs-fix Fix this "[minor] docs/architecture.md:595 still says "rsync repo back" in the extraction flow diagram. Might be worth updating to reflect SafeDownload while we're here — but not blocking on it."

@rh-hemartin rh-hemartin self-assigned this Jun 11, 2026
@fullsend-ai-coder

fullsend-ai-coder Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 2:27 PM UTC · Completed 2:31 PM UTC
Commit: 6bfb10b · View workflow run →

Replace stale "rsync repo back" reference with SafeDownload in the
architecture diagram's extraction flow, consistent with the OpenShell
native transport migration (PR #761).

Addresses review feedback on #2120
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (human-triggered)

Updated the extraction flow diagram in docs/architecture.md to replace the stale rsync reference with SafeDownload, consistent with the OpenShell native transport migration.

Fixed (1):

  1. docs/architecture.md:595 still says rsync repo back in extraction flow diagram (docs/architecture.md): Replaced 'rsync repo back (--no-links, exclude .git/hooks/)' with 'SafeDownload repo (sanitize symlinks, strip hooks)' in the extraction flow ASCII diagram, preserving box alignment.

Tests: passed

Updated by fullsend fix agent

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 2:34 PM UTC
Commit: c6bae71 · View workflow run →

@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 11, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:34 PM UTC · Completed 2:42 PM UTC
Commit: c6bae71 · View workflow run →

@rh-hemartin
rh-hemartin added this pull request to the merge queue Jun 12, 2026
Merged via the queue into main with commit 072da72 Jun 12, 2026
12 checks passed
@rh-hemartin
rh-hemartin deleted the agent/1150-remove-rsync-from-sandbox branch June 12, 2026 06:49
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 6:53 AM UTC · Completed 6:58 AM UTC
Commit: c6bae71 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2120 — Remove rsync from sandbox image

Workflow quality: Good. The code agent produced a clean initial change in ~5 minutes. The review agent correctly identified a stale "rsync" reference in docs/architecture.md:595 that the code agent missed. A human triggered /fs-fix, the fix agent resolved it, and the second review approved. One rework cycle total — the system working as designed.

No new proposals. The single improvement opportunity (code agent should search for all references to a removed term) is already covered by existing open issues:

  • #1222 — Triage agent: recommend old-value searches for rename/default-change tasks
  • #1733 — Code agent should proactively update documentation when implementing user-facing features
  • #1061 — Improve docs-staleness scanning to find all instances in one pass

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

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove rsync from base sandbox image — no longer used after OpenShell native transport

2 participants