fix(agent): revert path-normalizing dedup and symlink-resolved storage

Two pieces of the review-hardening turned out to be risky in production:

1. deduplicateMedia used filepath.Abs on each MediaResult.Path to
   normalize the dedup key. filepath.Abs depends on the process CWD,
   which varies across deployment environments. In containers with a
   non-standard CWD, relative paths from parseMediaResult were being
   rewritten into keys that could collide with or differ from
   absolute paths from tools — leading to legitimate media entries
   being silently eaten.

2. extractMediaFromContent overwrote the stored path with the
   EvalSymlinks-resolved path. When the workspace is a bind-mount or
   contains ancestor symlinks, the stored path diverged from what the
   tool originally wrote and queued into rs.mediaResults, so dedup
   against tool-origin entries failed and the same file could be
   delivered twice (or fail altogether if the resolved path wasn't
   reachable from the downstream reader's FS view).

Revert both to exact-string behavior. Keep EvalSymlinks for the
containment check only — the symlink-escape P0 remains closed, but
the stored path now matches what the tool wrote.
This commit is contained in:
viettranx committed 2026-04-18 16:19:23 +07:00
1 parent e03a11031f
commit 4988f57a9c
1 file changed
+13 -15
+13 -15
View File
@@ -107,6 +107,11 @@ func extractMediaFromContent(content, workspace string) []MediaResult {
// Resolve ancestor symlinks THEN check containment. A purely lexical
// Rel check would pass "<ws>/<symlink-dir>/secret" when symlink-dir
// points outside the workspace; EvalSymlinks closes that escape.
// NOTE: resolved is used ONLY for containment — the stored path stays
// `cleaned` so downstream readers (channel senders, history) use the
// same path semantics as the tool that wrote the file. Overwriting
// with the resolved path caused dedup misses in production when
// workspace contains bind-mounts / dir symlinks.
resolved, err := filepath.EvalSymlinks(cleaned)
if err != nil {
continue
@@ -115,7 +120,6 @@ func extractMediaFromContent(content, workspace string) []MediaResult {
if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
continue
}
cleaned = resolved
if _, dup := seen[cleaned]; dup {
continue
}
@@ -128,12 +132,12 @@ func extractMediaFromContent(content, workspace string) []MediaResult {
return results
}
// deduplicateMedia removes duplicate media results, normalizing each path
// so legacy entries from parseMediaResult (raw tool output — may contain
// "./" segments or relative forms) collapse cleanly with entries from
// extractMediaFromContent (already Clean'd + Abs'd). Prevents double-send
// when a tool returns MEDIA:./x.mp3 and the LLM echoes it back in a form
// that resolves to the same file.
// deduplicateMedia removes duplicate media results by path, keeping the first
// occurrence. Exact-string match is the ONLY safe comparison: filepath.Abs
// normalization depends on the process CWD, which varies across deployment
// environments and was observed to drop legitimate entries in production.
// The tiny cost of an occasional aliased-path duplicate (e.g. "./x" vs "/abs/x")
// is preferable to silently eating a real attachment.
func deduplicateMedia(media []MediaResult) []MediaResult {
if len(media) <= 1 {
return media
@@ -141,16 +145,10 @@ func deduplicateMedia(media []MediaResult) []MediaResult {
seen := make(map[string]bool, len(media))
result := make([]MediaResult, 0, len(media))
for _, m := range media {
key := m.Path
if abs, err := filepath.Abs(key); err == nil {
key = filepath.Clean(abs)
} else {
key = filepath.Clean(key)
}
if seen[key] {
if seen[m.Path] {
continue
}
seen[key] = true
seen[m.Path] = true
result = append(result, m)
}
return result