[hivecommons-hive]

unique_shot_path only checked -e, so same-millisecond captures could pick the same name and overwrite. It now reserves the name atomically (noclobber), and an EXIT trap removes the empty reservation if capture fails or is cancelled. Adds tests and a changelog entry. Closes #329

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kellyaa for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hivecommons-hive]

hive/awaiting-ci: CI pending — sweep will merge when green.

---

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Style review (head 618ce88) — one in-scope defect the reservation exposes, one test-readability note.

1. scrot path now breaks on every capture. The reservation at linux/hotshot-capture.sh:148 guarantees $SHOT_PATH exists before the tool runs, but scrot -s "$SHOT_PATH" / scrot "$SHOT_PATH" (:172, :174) are called without -o/--overwrite. Upstream scrot (≥1.2, i.e. everything Debian/Ubuntu/Fedora ship) opens the output with O_CREAT|O_EXCL and on EEXIST writes to <name>_000.png instead (scrot src/scrot.c scrotCheckIfOverwriteFile, man page: "By default scrot does not overwrite the output FILE"). Result: the image lands in hotshot-<ts>_000.png, the reservation stays empty, :189 dies with "screenshot file was not created", and the EXIT trap (:161) deletes the reservation — leaving a stray _000 file and no clipboard/typing. maim and grim truncate-on-open, so only scrot is affected. Fix: pass -o to both scrot invocations (and mention it in the :140-144 comment, since the reservation now depends on every tool being willing to overwrite).

2. (nit) The "simulated race" block in linux/tests/hotshot-capture.test.sh:203-210 is a plain sequential second call — $( (unique_shot_path …) ) is just a redundant extra subshell and the comment's "second shell … between probe and capture" overstates it. What it actually adds over :191-193 is that an empty reservation (not a written PNG) already blocks the name; say that and drop the inner parens.

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Security perspective — no exploitable issue found in the reservation itself (bash noclobber opens with O_EXCL, so a dangling symlink at the reserved name fails the write, [ -e ] is false and the script dies at linux/hotshot-capture.sh:149,158-159; the EXIT trap at :161 only removes a zero-byte file at a path this process created). One severe in-scope regression the diff introduces, though:

The reservation breaks the scrot backend outright. Since scrot 1.0 (2019, "by default do not overwrite the file", Debian #807139) scrot opens its output with O_CREAT|O_EXCL and, on EEXIST, silently writes to <name>_000.png instead (scrot.c: scrotCheckIfOverwriteFile, flags = O_RDWR | O_CREAT | (opt.overwrite ? O_TRUNC : O_EXCL)). With the name now pre-created at :148, scrot -s "$SHOT_PATH" / scrot "$SHOT_PATH" at :172,:174 exit 0 but write hotshot-<ts>_000.png; $SHOT_PATH stays empty, :189 dies with "screenshot file was not created", and the EXIT trap deletes the reservation. Net effect for every scrot user: no clipboard, no typed path, exit 1, plus an orphaned screenshot under an unexpected name. The suite doesn't catch this because the scrot stub at linux/tests/hotshot-capture.test.sh:511-514 unconditionally printf 'PNG' >"$out".

Fix: pass -o/--overwrite to both scrot calls at :172 and :174 (maim and grim truncate existing files, so they are fine), and make the scrot stub refuse to overwrite an existing $out unless -o is given so the regression is pinned.

— hive: agent=adjudicator-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Intent alignment (read at 618ce88): the reservation itself matches the #329 remedy (set -o noclobber + : >"$path", linux/hotshot-capture.sh:148-149) and maim/grim truncate on open, so those paths are fine. One in-scope regression:

scrot path is broken by the pre-created reservation — linux/hotshot-capture.sh:172,174 call scrot "$SHOT_PATH" with no -o. scrot ≥1.0 opens the output with O_CREAT|O_EXCL and, on EEXIST, silently writes to <name>_000.png and exits 0 (scrotCheckIfOverwriteFile, scrot src/scrot.c:557-589; ChangeLog 1.0 "by default do not overwrite the file"). With this PR $SHOT_PATH always exists (empty) when scrot runs, so every scrot capture lands in hotshot-<ts>_000.png, :189 then dies with "screenshot file was not created", and the EXIT trap at :161 deletes the empty reservation — leaving the real screenshot orphaned under a name the script never reports or copies. The e2e stub in linux/tests/hotshot-capture.test.sh:511-515 unconditionally overwrites $out, so it masks this.

Fix: pass -o/--overwrite to both scrot invocations at :172 and :174 (and ideally make the test stub refuse to overwrite without -o, mirroring real scrot).

— hive: agent=adjudicator-3 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Correctness review (head 618ce88)

Regression: every scrot capture now fails — linux/hotshot-capture.sh:148 pre-creates the empty reservation, but scrot ≥ 1.0 (2019, "by default do not overwrite the file", --overwrite/-o) opens its output with O_EXCL and on EEXIST silently writes to <name>_000.png instead (scrot src/scrot.c scrotCheckIfOverwriteFile), exiting 0. The calls at :172/:174 pass no -o, so $SHOT_PATH stays empty, :189 dies with "screenshot file was not created", the EXIT trap at :161 deletes the reservation, and the actual screenshot is orphaned as hotshot-<ts>_000.png — no clipboard, no typed path. maim (std::ofstream) and grim (fopen(…,"w")) truncate, so only the scrot fallback is affected, but it is the documented fallback at :170-175. Fix: add -o to both scrot invocations, or capture to a mktemp file in $SHOT_DIR and mv it over the reservation (tool-agnostic, and what #329 also suggested).

Minor (test adequacy): the "simulated race" at linux/tests/hotshot-capture.test.sh:203-209 is two sequential calls — functionally identical to the existing "-1 suffix" case at :191-193 — and nothing exercises the capture-tool path against a pre-existing reserved file, which is where the regression above lives.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed (correctness, delta 618ce88→abe76d6) — previous blockers fixed.

• scrot overwrite: fixed. scrot -o is now passed in both paths (linux/hotshot-capture.sh:175,177) and the header comment documents why (:145-147). Empty reservations are still cleaned up on failure by the EXIT trap (:160).
• Test adequacy: fixed. The scrot stub now mimics scrot ≥ 1.0 (_000.png on clobber without -o, linux/tests/hotshot-capture.test.sh:517-526), and the e2e asserts -o is passed and no _000 stray is left (:538-541, :1175-1176). The race test comment now matches what it does (:203-208). Suite passes at head: 286/286.

Advisory (non-blocking): -o/--overwrite was added in scrot 1.0 (2019); on a pre-1.0 scrot (e.g. EL7's 0.8) the flag is rejected and the capture dies. Those versions overwrote by default so didn't need it; acceptable given their age, noting for the record.

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor.

Review findings from ci-maintainer (folded here because this head already carries its hive review):

intent-alignment: approve. The diff implements #329's suggested fix (set -o noclobber reserve in the probe loop, linux/hotshot-capture.sh:151-153), handles the non-ENOENT failure (:162), cleans up empty reservations on exit (:164), and correctly adds scrot -o so scrot ≥1.0 overwrites the reservation rather than writing _000.png (:175,:177). 286/286 tests pass at abe76d6; no unrelated scope.

One advisory: the body and CHANGELOG.md:9 claim the empty reservation is removed on cancel/failure, but the existing failure/cancel e2e blocks (linux/tests/hotshot-capture.test.sh:504-505, :1161-1163, :1184, :1190) only check the die message and that nothing was typed — none asserts the reserved hotshot-*.png is gone. Worth a one-line find "$TMP/shots" -name 'hotshot-*.png' -empty assertion in one of them.

Review findings from reviewer (folded here because this head already carries its hive review):

Read linux/hotshot-capture.sh at abe76d6 end to end rather than just the hunk. Looks correct to me:

• unique_shot_path (lines 149–157) now reserves via (set -o noclobber; : >"$path"), and the [ -e "$path" ] || return 1 guard means a non-EEXIST failure (permissions, read-only dir) surfaces as the new die at line 162 instead of looping forever.
• The script runs under set -u only (line 22) and this is the only trap … EXIT (line 164), so nothing overrides the cleanup; every die path exits through it and removes the empty reservation, while the existing [ -s "$SHOT_PATH" ] || die at line 192 still catches a tool that exits 0 without writing.
• maim and grim open the output for writing/truncate, so the pre-existing empty file is overwritten; scrot is the only tool that refuses by default and both call sites (175, 177) pass -o. The updated stub in the tests mimics scrot's _000 fallback, so the -o assertion would fail without it.

One note, not a defect: the reservation is created before the region selector runs, so a cancelled maim -s/slurp briefly leaves an empty hotshot-*.png in the folder until the trap fires — any folder watcher polling faster than that will see a zero-byte file appear and vanish. Acceptable for this fix.

Review findings from adjudicator (folded here because this head already carries its hive review):

Security review (head abe76d6) — the atomic reservation holds for the #329 race and is a net hardening (a dangling symlink at the candidate name now fails closed at linux/hotshot-capture.sh:152 instead of being followed by the capture tool). One low-severity advisory:

Non-regular files at the candidate name are now selected, not skipped (linux/hotshot-capture.sh:151-152, low). bash's noclobber only refuses regular files; for an existing FIFO/device it opens without O_EXCL and succeeds (bash redir.c noclobber_open; reproduced on bash 5.2: mkfifo hotshot-x.png + reader → (set -o noclobber; : >hotshot-x.png) returns 0). So if $SHOT_DIR is writable by another principal (e.g. HOTSHOT_DIR pointed at a shared/sticky dir), a pre-placed FIFO named for a future millisecond is returned by unique_shot_path and maim/scrot -o/grim (:167-190) streams the screenshot into it — or blocks indefinitely with no reader. The pre-PR [ -e ] probe skipped such entries. Default ~/Pictures/hotshot is user-owned, so this is not exploitable out of the box; it's a hardening gap, not a blocker.

Suggested fix (the second option from #329): mktemp a regular file inside $SHOT_DIR, then ln "$tmp" "$path" in the loop — link(2) fails EEXIST for any existing entry (FIFO, symlink, dir) and never follows symlinks. Or, minimally, after the successful : >"$path" add [ -f "$path" ] && [ ! -L "$path" ] || return 1.

[hivecommons-hive]

Evidence bundle `#330abe76d6` sha256:b8f63f154ef929fb9bc7fa287e164f51f56b7f70f9413834638a8a43b52fa1dc — download from the dashboard or `hivectl review evidence #330`.

[github-actions]

Thank you for your contribution! Your PR has been merged.

We'd love to hear how your experience was: share feedback