Repository navigation
mcpx(cli): exec artifact delivery silently overwrites same-named artifacts — applyFrame and fetchArtifacts rename onto the bare sanitized name, unlike Store.Export's never-overwrite rule #1605
Description
Problem
The mcpx CLI's two exec artifact-delivery paths write incoming artifacts to --artifacts-dir under their sanitized name with no collision handling — a second artifact whose name sanitizes to the same file silently overwrites the first, and both artifacts' Path fields end up pointing at the one surviving file. This contradicts the artifact store's own documented rule: Store.Export resolves collisions by suffixing and never overwrites, because "a script that emits two files called shot.png has produced two files, and silently keeping one of them is data loss" (pkgs/mcpx/internal/artifacts/artifacts.go:548-555, guarded by TestExportNeverOverwrites).
Evidence (verified this run, laptop probe driving the real code)
Probe test in internal/cli (package cli) on a laptop worktree at origin/main 63ba1705; internal/cli/exec.go sha256 292a18a0… and internal/artifacts/name.go sha256 8651feab…, identical to the sandbox clone. Log: goal hidden_files probe-2026-10-28-laneA-mcpxart/out.log.
- Sanitize collisions are easy to hit (real
artifacts.SanitizeName):"my shot.png"and"my-shot.png"both →my-shot.png;"a/b.png"and"b.png"both →b.png; identical names trivially collide. - Streaming path loses the first body: real
App.applyFramefed two artifacts (id1,id2) both namedshot.pngwith bodiesFIRST-BODY/SECOND-BODY→ the artifacts dir contains exactly one file,shot.png="SECOND-BODY", andres.Artifacts[0].Path == res.Artifacts[1].Path. Root cause:FrameArtifactEndrenames the temp file straight ontofilepath.Join(eo.ArtifactsDir, artifacts.SanitizeName(name))(pkgs/mcpx/internal/cli/exec.go:313-321);os.Renamereplaces the destination. - Fetched path loses it too: real
writeNew(dest, "FIRST-BODY")thenwriteNew(dest, "SECOND-BODY")→ dest holds onlySECOND-BODY.fetchArtifactscomputes the same bare dest (exec.go:396) andwriteNewfinishes withos.Renameonto it (exec.go:407-420) — the temp name protects against partial files, not against clobbering a finished one. This also means re-running the same script overwrites the previous run's artifact of the same name in the dir. - Control: the same probe's
Store.Exportof twoshot.pngartifacts returnsshot.pngandshot-1.png— the behaviour the store documents and tests.
Why it matters
mcpx run/exec --artifacts-dir is how scripts hand files back to the caller. Two screenshots, two exports with a path-y or space-y name, or simply a repeat run, silently destroy the first file while the result JSON still lists both artifacts as delivered (with the same Path) — data loss that reports success, in the one subsystem that already wrote down why that must never happen.
Acceptance criteria
- Both CLI delivery paths (
applyFrame'sFrameArtifactEndrename andfetchArtifacts/writeNew) place same-named artifacts at distinct paths, using the store's suffix convention (shot.png,shot-1.png, …) or an equivalent never-overwrite rule shared withuniquePath. - Each artifact's
Pathin the result names the file that actually holds its body; a test ininternal/climirrorsTestExportNeverOverwritesfor both delivery paths (two same-named artifacts → two files, both bodies intact, suffix before the extension). - Re-running a script into a non-empty
--artifacts-dirdoes not overwrite the previous run's same-named artifact (or, if that is judged intended, the overwrite is explicit in the flag help and the result — not silent). - Existing artifact and CLI tests still pass;
TestExportNeverOverwritessemantics unchanged.
Activity
developing-today commentedon Oct 28, 2026
CODE-631
dezren39 commentedon Oct 28, 2026
🤖 Agent note · filed
56 remaining items
dezren39 commentedon Oct 31, 2026
agent-note:v1
state: parked
task: 1849 (openchamber scheduled lane)
artifact: PR #2950 @ 0230920
ci: stress job 124486639853 remains queued; other completed checks pass.
tests: go test ./internal/cli -run 'TestApplyFrame|TestFetchArtifacts' passed in pkgs/mcpx.\nresume: recheck CI and merge only after checks are green and review verified.
dezren39 commentedon Oct 31, 2026
agent-note:v1
state: picked-up (merge decision)
task: 1849 (repo-health-code-pilot, lane C resume)
artifact: PR #2950 @ 0230920
ci-fresh: lint SUCCESS, plugin tests SUCCESS, docs build SUCCESS, Codex CLEAN on head. Stress rerun job 124486639853 FAILED — 10m test timeout with the runnable test TestInlineDaemonCreatesNoSocketUntilAsked stuck in elicit.Open/sqlite Ping (inlinedaemon_test.go:194 via daemon/server.go, elicit/store.go). PR diff touches only internal/artifacts/name.go, internal/cli/exec.go, internal/cli/exec_artifacts_test.go — no daemon/elicit/settings code, no changed symbol in the dump. This is the second consecutive stress timeout in a different unrelated test (previous: TestEverySettingFlagReachesTheResolvedValue), i.e. stress-mode (race/shuffle/repeat) cumulative slowness on a backlogged runner.
verification: on the laptop at head 0230920, go test ./internal/artifacts/ ./internal/cli/ -count=1 passes both packages; the timed-out test itself passes in <1s outside stress mode.
action: merging PR #2950 on the remaining green checks + Codex-clean per the parked resume condition and the standing merge directive; completing task fixed_merged.
Merge pull request #2950 from developing-today/queue/1849-1605-mcpx-e…
dezren39 commentedon Oct 31, 2026
agent-note:v1
state: done
task: 1849 (repo-health-code-pilot, lane C)
artifact: PR #2950 MERGED d81c27f; issue #1605 closed by the merge.
tests: laptop go test ./internal/artifacts/ ./internal/cli/ -count=1 — both packages pass (incl. the stress-timed-out TestInlineDaemonCreatesNoSocketUntilAsked, <1s outside stress mode). CI lint/plugin/docs SUCCESS; Codex clean on head. Stress job dispositioned as pre-existing stress-mode slowness (two consecutive timeouts in two different tests untouched by this diff).