Review artifacts are keyed by repository basename and wall-clock time, so reviews can read or overwrite each other

[paskal]

Three places key a review artifact by something that is not unique, so one review can read or overwrite another's data. Each has a different precondition, spelled out below; none needs an attacker. Everything here was reproduced against 1b71635.

Set up one scratch directory for all three reproductions:

REPO=/path/to/revdiff          # a checkout, for the module context
base=$(mktemp -d) && echo "$base"

1. History is keyed by the repository basename

app/history/history.go:107 derives the history subdirectory from filepath.Base(p.Path). In production that Path is the working directory or the git root (app/history_save.go:21-24), so it is the repository root's basename. Both copies of the reader resolve the same way (.claude-plugin/skills/revdiff/scripts/read-latest-history.sh:34, plugins/codex/skills/revdiff/scripts/read-latest-history.sh:35) and then print that directory's newest file.

Two checkouts whose root directories share a name therefore share one history directory: ~/code/proj and ~/review/proj, or a second clone made for a bisect or a rebuild. It needs no concurrency at all, only that both have been reviewed at some point.

mkdir -p "$base/a/proj" "$base/b/proj" "$base/hist"
( cd "$base/b/proj" && git init -q . )

cat > "$base/save.go" <<'EOF'
package main

import (
	"os"

	"github.com/umputun/revdiff/app/history"
)

func main() {
	history.New(os.Args[1]).Save(history.Params{
		Annotations: "## f.txt:2 (+)\n" + os.Args[3] + "\n",
		Path:        os.Args[2],
	})
}
EOF

( cd "$REPO" && go run "$base/save.go" "$base/hist" "$base/a/proj" "note written while reviewing repo A" )
( cd "$base/b/proj" && REVDIFF_HISTORY_DIR="$base/hist" \
    "$REPO/.claude-plugin/skills/revdiff/scripts/read-latest-history.sh" )

Observed, standing in repo B:

# Review: 2026-08-20 08:18:55
path: /tmp/.../a/proj

## Annotations

## f.txt:2 (+)
note written while reviewing repo A

The path: header names the other checkout. The skills use this script as their durable fallback (.claude-plugin/skills/revdiff/SKILL.md, "Using Existing Review History"; plugins/pi/skills/revdiff/SKILL.md:114), so the agent receives review comments written about a different repository and starts editing against them. Of the three, this is the one I would fix first.

Save goes through the real service here rather than through the TUI, since driving the TUI needs a terminal; the only production step skipped is saveHistory filling Path, which the lines cited above show is the repository root.

2. History filenames collide at millisecond resolution

app/history/history.go:58-59 builds the filename from time.Now().Format("2006-01-02T15-04-05.000"), and fsutil.AtomicWriteFile renames onto that path (app/fsutil/fsutil.go:29), so a save landing in the same millisecond as an earlier one replaces it with no error. The precondition is strict: two saves into the same subdirectory within the same millisecond. Concurrent Save calls show the mechanism:

cat > "$base/collide.go" <<'EOF'
package main

import (
	"fmt"
	"os"
	"sync"

	"github.com/umputun/revdiff/app/history"
)

func main() {
	var wg sync.WaitGroup
	for i := range 20 {
		wg.Add(1)
		go func(i int) {
			defer wg.Done()
			history.New(os.Args[1]).Save(history.Params{
				Annotations: fmt.Sprintf("## f.txt:1 (+)\nsave number %d\n", i),
				Path:        "/tmp/proj",
			})
		}(i)
	}
	wg.Wait()
	fmt.Println("saves requested: 20")
}
EOF

mkdir -p "$base/hist2"
( cd "$REPO" && go run "$base/collide.go" "$base/hist2" )
find "$base/hist2" -name '*.md' | wc -l

Fewer than twenty files survive, and the count swings widely with how tightly the goroutines land: runs here gave 10, 9 and 1. Twenty goroutines is not the real usage pattern, and in practice this needs two reviews of the same repository quitting in the same millisecond, so it is much the least likely of the three. It is worth mentioning mainly because mechanism 1 funnels unrelated repositories into one directory, which is what puts two writers there at all.

3. The output-file fallback selects by mtime across everything in the temp directory

launch-revdiff.sh:19 creates the output file with mktemp, passes it to revdiff as --output, prints the file's contents on completion (print_output_and_exit), and removes it in an EXIT trap. The caller never learns the path, by design.

That is fine until the launcher call times out. Then the caller has neither the contents nor the path, and the skills fall back to picking the newest matching file anywhere in the temp directory:

.claude-plugin/skills/revdiff/SKILL.md:159
plugins/codex/skills/revdiff/SKILL.md:188
plugins/codex/skills/revdiff-plan/SKILL.md:100

output_file="$(ls -t "${TMPDIR:-/tmp}"/revdiff-output-* 2>/dev/null | head -1)"

Two ways that picks the wrong file.

Another live review. With a second review open (two agent sessions, or one diff review plus one plan review), whichever was flushed last wins:

export TMPDIR="$base/tmp" && mkdir -p "$TMPDIR"
A=$(mktemp "$TMPDIR/revdiff-output-XXXXXX"); printf '## a.go:1 (+)\nreview A: fix the retry loop\n' > "$A"
sleep 1
B=$(mktemp "$TMPDIR/revdiff-output-XXXXXX"); printf '## b.go:9 (-)\nreview B: keep this check\n' > "$B"

output_file="$(ls -t "${TMPDIR:-/tmp}"/revdiff-output-* 2>/dev/null | head -1)"
cat "$output_file"

Observed: review B: keep this check. Session A then works from session B's annotations, on a change they were never written about.

Leftovers from an earlier review, with nothing concurrent at all. Cleanup is the launcher's EXIT trap, and once the launcher has died on the timeout nothing is left to run it again. revdiff is still open in the overlay and writes to that same path when the user quits or presses O, so the file either survives the launcher's death or reappears just after it, and either way stays in the temp directory indefinitely. A later review that times out and finds no newer output file selects that leftover and replays a previous review's annotations. cat does not remove it either, so it can be selected more than once.

Options

Mechanism 1. Either key the subdirectory by basename plus a short hash of the canonical root (proj-9f3c1a2b), or keep the directory as it is and have the reader filter on the canonicalised path: header that every entry already carries. Whichever way, the legacy bare-basename directory has to be filtered by that header too, not merely tried as a fallback: reading the newest legacy entry blindly reintroduces exactly this bug for history written before the change. The header-only route changes no layout but leaves the two repositories interleaved in one directory, so browsing it by hand and any ls -t tooling stay wrong; I would take the hashed directory, with header filtering on the legacy path. Note the blast radius is wider than the two reader scripts: README.md:714 and README.md:263 document the layout, and plugins/pi/skills/revdiff/SKILL.md:114 tells agents to read the newest file from that directory.

Mechanism 2. Append a short random suffix, for example os.CreateTemp with the timestamp as prefix and .md as suffix, which preserves both the *.md glob and the mtime ordering the readers depend on. Nanosecond precision narrows the window without closing it.

Mechanism 3. The glob exists only because the caller has no path, so give it one: let the caller create the capture file and pass it to the launcher, which uses it instead of its own mktemp. The skill then reads back exactly the file it named, and removes it afterwards on both the synchronous and the timeout path, which also stops leftovers accumulating. That needs a launcher flag or environment variable plus a matching change in all three skills, and the ownership rule has to be stated in the skills or the leftovers come back. Filtering the existing glob by owner and mtime age would narrow the mis-selection without removing it.

The history layout is user-visible and mechanism 3 changes the launcher contract, so I did not want to choose either inside one.

[umputun]

all three reproduce against 1b71635. Pushing back on the proposed fix for each though.

mechanism 1 - the problem is wider than you describe, and the hashed directory doesn't work.

wider: DetectVCS accepts .git as a file (app/diff/vcs.go:35), so worktrees and submodules are their own roots and the key becomes a worktree directory name. Under .claude/worktrees/<name>/ that is fix, wip, a branch name. Those collide across unrelated repos far more readily than two clones both named proj.

doesn't work: DetectVCS starts from os.Getwd() (app/renderer_setup.go:25), which returns the logical path, while the reader uses git rev-parse --show-toplevel, which returns the physical one. Through a symlinked parent:

go Getwd:     .../skew/link/proj
git toplevel: .../skew/real/proj

basename keying is immune to that because both end in proj. Hash the canonical root and the two sides produce different keys for one checkout, the reader finds no directory, and the skill reports that the user quit without annotating. That trades a visible wrong read for a silent empty one.

so the header filter, in the reader only. Your objection to it was that ls -t tooling stays wrong, but nothing runs ls -t against the history directory: read-latest-history.sh:39-45 compares with -nt, and the pi instruction is prose to an agent that can equally be told to check the header. No layout change, no migration, no legacy special case, and the header has been unconditional since 238c47f.

mechanism 2 - rarer than 1, but not only for the reason you give. SIGHUP is registered (app/signal.go:56) and delivered session-wide, and on the signal path history is the only save. One dropped ssh connection fires simultaneous saves in every revdiff under that session, which is the case #262 was built for. That is a single kernel event, not two people quitting in the same millisecond.

os.CreateTemp with .md as the suffix isn't the way to fix it. Reserving the name creates an empty *.md that is the newest by mtime for the whole write window, so the reader picks it and prints nothing, and permanently so if the process dies in between. Writing through that handle instead drops the atomic rename at app/fsutil/fsutil.go:29, which the skills state as a guarantee in prose. The fix has to keep AtomicWriteFile and make the target name unique before the rename.

mechanism 3 - agreed, but narrower than stated and not with a new flag. launch-plan-review.sh:61 uses a plan-review-output- prefix and carries no glob fallback, so the always-on planning hook isn't involved at all. Only the codex /revdiff-plan path shares the revdiff-output-* glob.

I'd narrow the glob first, as a skills-only edit: -user "$(id -u)" -mmin -120 removes the stale-leftover half outright and leaves only two concurrent live reviews. A launcher env var can follow if that half actually bites. One thing it has to account for: resolve-launcher.sh lets users override the launcher, and an override predating the change ignores the variable, so the narrowed glob stays as a second try rather than being deleted.

separately, the docs justify that fallback on the wrong property: safe because the write is atomic, so "there is never a partial read" (.claude-plugin/skills/revdiff/SKILL.md:163). Atomicity gives a complete file, not the right one. That is probably why this went unnoticed, and I'll fix the wording either way.