Skip to content

fix(workspace files API): EIC parity for ListFiles + DeleteFile (#2999 PR-A) - #3002

Merged
HongmingWang-Rabbit merged 1 commit into
stagingfrom
fix/files-eic-list-delete-symmetry
May 6, 2026
Merged

fix(workspace files API): EIC parity for ListFiles + DeleteFile (#2999 PR-A)#3002
HongmingWang-Rabbit merged 1 commit into
stagingfrom
fix/files-eic-list-delete-symmetry

Conversation

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor

User-visible bug

Canvas Files tab returns "0 files / No config files yet" for every SaaS workspace, every root (/configs, /home, /workspace, /plugins). Reported by user (canvas screenshot, hongming.moleculesai.app, "Hongming Personal Brand Agent" — claude-code, T4, online). See #2999 for the umbrella issue + the four follow-up PRs.

Root cause

ListFiles in templates.go was missing the SSH-via-EIC branch that ReadFile (PR #2785) and WriteFile (PR #1702) already have. On SaaS, dockerCli is nil → findContainer returns "" → falls through to resolveTemplateDir which only matches baked-in template names. For a user-named workspace it matches nothing, so the handler silently returns []fileEntry{}.

DeleteFile had the same gap — right-click delete (introduced in PR-C of this issue) would silently no-op once #1 was fixed.

Fix

  1. Extracted the EIC tunnel dance into a single withEICTunnel(ctx, instanceID, fn) SSOT. Refactored existing writeFileViaEIC + readFileViaEIC to use it. Added listFilesViaEIC + deleteFileViaEIC on the same scaffold. The LogLevel=ERROR fix from PR fix(files-eic): silence ssh known-hosts warning that 500'd Hermes config load #2822 now lives in one eicSSHSession.sshArgs() helper instead of being duplicated per op.

  2. Factored remote shell strings into pure functions (buildInstallShell / buildCatShell / buildRmShell / buildFindShell / parseFindOutput) so the wire shape is testable without a real EIC tunnel.

  3. Refactored resolveWorkspaceFilePath(runtime, root, relPath) to honor ?root=. New rule: /configs (or empty / unrecognized) → runtime managed-config dir via workspaceFilePathPrefix (preserves the existing config.yaml round-trip on hermes/claude-code); /home / /workspace / /plugins → literal absolute path on the EC2 host. List/Read/Write/Delete now agree on what file a tree row points to — pre-fix List would say "/home contents" but Read/Write would route to /configs.

  4. ListFiles + DeleteFile dispatch on instance_id != "" → EIC helper. Errors from the EIC path produce 500 (not silent fall-through to local-Docker, which would mask the failure as "0 files" — the exact user-visible symptom).

  5. Added ?root= validation gate to WriteFile + DeleteFile so an out-of-allowlist root is rejected before the resolver runs.

Test coverage (≥4 new test files, ≥17 new tests)

Test Pins
TestResolveWorkspaceFilePath_RuntimeIndirection /configs → runtime prefix per-runtime
TestResolveWorkspaceFilePath_LiteralRoots /home, /workspace, /plugins literal
TestResolveWorkspaceRootPath directory-only translation used by listFilesViaEIC
TestSSHArgs_HardenedFlags centralised ssh option set
TestEicSSHSessionSingleSourceForSSHFlags behaviour-based AST gate — no copy-pasted ssh args
TestBuildInstallShell / TestBuildCatShell / TestBuildRmShell / TestBuildFindShell remote shell shape
TestBuildFindShell_DepthForwarding depth not hard-coded
TestParseFindOutput / TestParseFindOutput_EmptyInput parser symmetry with container path
TestListFiles_EICDispatch_Success / Error sqlmock dispatch on instance_id != ""
TestListFiles_EICBranch_NotTakenForSelfHosted back-compat — instance_id == "" stays on Docker path
TestDeleteFile_EICDispatch_Success / Error same shape for delete
TestListFiles_RootValidation / TestDeleteFile_RootValidation ?root=/etc → 400 before any DB / EIC

Verification

  • go build ./... clean
  • go test ./... clean (full workspace-server suite)
  • Will be live-verified against staging on hongming.moleculesai.app after merge: open Files tab → expect populated /home + /configs + /workspace listings (not "0 files"); right-click delete on /configs/old.yaml → expect file removed on the EC2 host.

Three weakest spots (hostile self-review)

  1. TestEicSSHSessionSingleSourceForSSHFlags counts source occurrences of LogLevel=ERROR. A future refactor that legitimately moves the literal (e.g. into a constant) would trigger a false positive. Failure message points to the load-bearing constraint (must appear in sshArgs); operator can adjust. Acceptable — the alternative (no gate) is what let PR fix(files-eic): silence ssh known-hosts warning that 500'd Hermes config load #2822's regression slip in originally.

  2. eicFileWriteTimeout constant kept as a back-compat alias for prior tests. Documented + safe to remove on the next pass.

  3. The resolver tests pin runtime → prefix map values (/home/ubuntu/.hermes, /configs, etc.). A future runtime addition that ships a new prefix needs the test updated. Intentional — silent prefix changes orphan saved files, so a test failure on map edit IS the right signal.

Rollout / rollback

  • Rollout: standard staging → main auto-promote; live-verify on hongming.moleculesai.app once deployed.
  • Rollback: revert this commit. All four pre-existing self-hosted code paths (local-Docker, ephemeral-volume, host template-dir) are unchanged.

Follow-ups (separate PRs against #2999)

Refs #2999.

🤖 Generated with Claude Code

#2999 PR-A)

## User-visible bug

Canvas Files tab returns "0 files / No config files yet" for every
SaaS workspace, every root (/configs, /home, /workspace, /plugins).
Reported by user (canvas screenshot, hongming.moleculesai.app,
Hongming Personal Brand Agent — claude-code, T4, online).

## Root cause

`ListFiles` (templates.go) was missing the SSH-via-EIC branch that
ReadFile (PR #2785) and WriteFile (PR #1702) already have. On SaaS,
dockerCli is nil → findContainer returns "" → falls through to
host-side resolveTemplateDir which only matches baked-in template
names. For a user-named workspace it matches nothing, so the handler
silently returns []fileEntry{}.

DeleteFile had the same gap — right-click delete (introduced in PR-C
of this issue) would silently no-op once #1 was fixed.

## Fix

1. Extracted shared EIC plumbing into `withEICTunnel` (closure-based,
   single SSOT for keypair → key push → tunnel → port-wait → cleanup).
   Refactored writeFileViaEIC + readFileViaEIC to use it. Added
   listFilesViaEIC + deleteFileViaEIC on the same scaffold. The
   `LogLevel=ERROR` shim from PR #2822 now lives in one
   `eicSSHSession.sshArgs()` helper instead of being duplicated per
   helper — the next time we need to tweak ssh options, one place.

2. Factored remote shell strings into pure functions
   (buildInstallShell / buildCatShell / buildRmShell / buildFindShell
   + parseFindOutput) so the wire shape can be pinned without booting
   a real EIC tunnel.

3. Refactored `resolveWorkspaceFilePath(runtime, root, relPath)` to
   honor `?root=`. New rule: `/configs` (or empty / unrecognized) →
   runtime managed-config dir via workspaceFilePathPrefix (preserves
   the v1 ReadFile/WriteFile behaviour where canvas's Config tab
   GETs/PUTs config.yaml without specifying a root and lands in the
   right per-runtime dir); `/home`, `/workspace`, `/plugins` →
   literal absolute path on the EC2 host. List/Read/Write/Delete now
   agree on what file a tree row points to — pre-fix List would say
   "/home contents" but Read/Write would route to /configs.

4. ListFiles + DeleteFile dispatch on instance_id != "" → EIC helper.
   Errors from the EIC path produce 500 (not silent fall-through to
   local-Docker, which would mask the failure as "0 files" — the
   exact user-visible symptom).

5. Added ?root= validation gate to WriteFile + DeleteFile so an
   out-of-allowlist root is rejected before the resolver runs.

## Test coverage

- TestResolveWorkspaceFilePath_RuntimeIndirection — pins the
  /configs → runtime prefix translation per-runtime (hermes,
  claude-code, langgraph, external, unknown). Catches the regression
  where a future edit accidentally drops the runtime indirection.

- TestResolveWorkspaceFilePath_LiteralRoots — pins /home,
  /workspace, /plugins as literal pass-through regardless of
  runtime. Catches the symmetric regression where the literal roots
  start getting rewritten to the runtime prefix (which would mean
  the FilesTab "/home" selector silently routes to /configs on
  hermes).

- TestResolveWorkspaceRootPath — directory-only translation used
  by listFilesViaEIC, same indirection rules.

- TestSSHArgs_HardenedFlags — pins the centralised ssh option set
  (LogLevel=ERROR + hardening). Catches drift in the
  one-place-where-ssh-flags-live.

- TestEicSSHSessionSingleSourceForSSHFlags — behaviour-based AST
  gate (per memory). Counts s.sshArgs() callers (must be ≥4 —
  list/read/write/delete) and asserts LogLevel=ERROR appears
  exactly once in the source. Fires if anyone copy-pastes a raw
  ssh args slice instead of going through the helper.

- TestBuildInstallShell / TestBuildCatShell / TestBuildRmShell /
  TestBuildFindShell — pure-function tests pinning the remote
  command shape. Catches regression like "rm -f silently becomes
  rm -rf" or "find loses node_modules pruning" without needing a
  real EC2.

- TestBuildFindShell_DepthForwarding — catches a regression where
  the helper hard-codes a depth instead of using the caller's value.

- TestParseFindOutput / TestParseFindOutput_EmptyInput — pin the
  TYPE|SIZE|REL parser. Empty-input case explicitly returns []
  not nil so the JSON wire shape stays a list.

- TestListFiles_EICDispatch_Success / Error — sqlmock-driven
  handler test. Verifies instance_id != "" routes to listFilesViaEIC
  and surfaces errors as 500 (does NOT silently fall through to
  local-Docker, which is the exact regression-mode of the original
  bug).

- TestListFiles_EICBranch_NotTakenForSelfHosted — back-compat
  guard: instance_id == "" must NOT enter the EIC branch (would
  break self-hosted operators).

- TestDeleteFile_EICDispatch_Success / Error — same shape for
  DeleteFile.

- TestListFiles_RootValidation / TestDeleteFile_RootValidation —
  ?root=/etc must 400 before any DB query or EIC call.

## Verification

- `go build ./...` clean
- `go test ./...` clean (full workspace-server suite)
- Will be live-verified against staging on hongming.moleculesai.app
  after merge: open Files tab → expect populated /home + /configs +
  /workspace listings (not "0 files"); right-click delete on
  /configs/old.yaml → expect file removed on the EC2 host.

## Three weakest spots (hostile self-review)

1. The LogLevel=ERROR drift gate counts source occurrences. A
   future refactor that intentionally moves the literal somewhere
   else (e.g. into a constant) would trigger a false positive. The
   gate's failure message points to the load-bearing constraint
   (must appear in sshArgs); operator can adjust.

2. `eicFileWriteTimeout` constant kept as an alias for back-compat
   with prior tests. Documented as intentional + safe to remove on
   the next pass.

3. The resolver tests pin the runtime → prefix map values
   (`/home/ubuntu/.hermes`, `/configs`, etc.). A future runtime
   addition that ships a new prefix needs the test updated. This
   is intentional — silent prefix changes orphan saved files, so a
   test failure on map edit IS the right signal.

## Follow-up (RFC #2312 subtask 2)

Long-term the right fix is to drop EIC entirely and HTTP-forward to
the workspace's own URL (RFC #2312). That's a substantially larger
refactor across 5 surfaces (chat upload, files, templates, plugins,
terminal) and out of scope for this bug-fix PR. Tracked separately
under that RFC.

Refs #2999.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant