chore: update pull request - #103
Conversation
…-19xp) Concurrent agents had no way to stop their own suite, so they reached for `pkill -f "fm-test-run.sh --changed"`. Every agent invokes the runner with a near-identical command line, so that pattern also matched a sibling agent's unrelated run and killed it at 86/146 scripts. The truncated log then looked like a short clean run: a reader counting failures off it concluded the suite passed while both agents were validating branches for merge. Give the runner a run identity and an owner path for stopping it: - Each executing run registers run_id/pid/root/selection under FM_TEST_RUN_REGISTRY (default $TMPDIR/fm-test-run-registry) and removes that record on every exit path. Dead records are pruned on read, so a crashed run leaves nothing a later --stop could act on. - `--list-runs` reports live runs; `--stop [<run-id>]` stops by identity. With no id it stops only the runs registered from THIS repository root, so a concurrent agent's run in another worktree is never a candidate. - A signalled run traps TERM/INT/HUP and prints exactly one `FM_TEST_ABORTED <iso> run_id= signal= completed= selected=` terminator in place of FM_TEST_SUMMARY, then exits 128+signal. Silent truncation becomes loud truncation, which matters independently of the stop verb. The stop signals the run's own subtree before waiting: bash defers a trap until the foreground command returns, so TERMing only the run would leave it blocked mid-test and its terminator unwritten until the grace period killed it silently. AGENTS.md section 8 already named this hazard class for watchers; it now names the test runner too, since the reasoning is identical and the runner previously had no owner path the way watchers do.
📝 WalkthroughWalkthroughThe behavior-test runner now registers active runs in a per-user registry. It supports listing and stopping runs by identity or repository root, handles process-tree termination, records aborted runs, cleans up state, and adds documentation and tests. ChangesScoped behavior-test run control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant fm-test-run.sh
participant RunRegistry
participant TestProcessTree
Operator->>fm-test-run.sh: invoke --list-runs or --stop
fm-test-run.sh->>RunRegistry: find matching live run
fm-test-run.sh->>TestProcessTree: signal registered process tree
TestProcessTree-->>fm-test-run.sh: emit FM_TEST_ABORTED
fm-test-run.sh->>RunRegistry: remove run record
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/fm-test-run.sh`:
- Around line 232-242: Update register_run to write the complete registry record
to a temporary file in RUN_REGISTRY, then atomically rename it to the final .run
path only after the write succeeds. Ensure the temporary file is created in the
same directory and cleaned up on failure, while preserving prune_run_registry’s
handling of finalized records.
- Around line 121-125: Update the RUN_REGISTRY default in the shell script to
include the current user’s identifier in the generated path, while preserving
FM_TEST_RUN_REGISTRY as the explicit override. Ensure the default registry
directory is distinct for different users even when TMPDIR falls back to /tmp.
- Around line 1532-1536: Validate the extracted STOP_RUN_ID in the --stop=*
argument branch before setting STOP_REQUESTED or continuing. Reject an empty
value with the script’s existing invalid-argument behavior, and only invoke the
single-run stop path when a non-empty run ID was supplied; preserve the no-ID
behavior for the separate --stop form.
- Around line 301-305: Validate FM_TEST_RUN_STOP_GRACE/STOP_GRACE_SECONDS during
configuration before the wait loop, rejecting any non-numeric value with the
existing configuration-error behavior. Ensure the loop around pid_is_alive only
receives a validated integer so invalid input cannot trigger set -e before
SIGKILL escalation or suppress the FM_TEST_STOPPED output.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15f9725e-ae23-490e-8601-2667748ec6de
📒 Files selected for processing (4)
AGENTS.mdbin/fm-test-run.shdocs/scripts.mdtests/fm-test-run.test.sh
| # Where executing runs register their identity so a concurrent agent can stop | ||
| # one exactly instead of pattern-matching a shared command line. Per-user by | ||
| # default because TMPDIR is per-user on macOS; FM_TEST_RUN_REGISTRY overrides it | ||
| # for tests and for anyone deliberately partitioning runs further. | ||
| RUN_REGISTRY="${FM_TEST_RUN_REGISTRY:-${TMPDIR:-/tmp}/fm-test-run-registry}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make the default registry path user-specific.
The comment states the registry is per-user, but that only holds where TMPDIR is per-user. On Linux, TMPDIR is often unset, so the default resolves to the shared /tmp/fm-test-run-registry. Two consequences follow:
- If another user creates that directory first,
mkdir -psucceeds,chmod 0700fails, and the|| trueinregister_runhides the failure. Records with repository paths become readable by other users. - A record written by another user supplies the
pidthatstop_one_runsignals.--stopthen sendsTERMandKILLto a pid this runner never started.
Add the user id to the default path so runs from different users never share a directory.
🔒 Proposed fix
-RUN_REGISTRY="${FM_TEST_RUN_REGISTRY:-${TMPDIR:-/tmp}/fm-test-run-registry}"
+RUN_REGISTRY="${FM_TEST_RUN_REGISTRY:-${TMPDIR:-/tmp}/fm-test-run-registry-$(id -u)}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Where executing runs register their identity so a concurrent agent can stop | |
| # one exactly instead of pattern-matching a shared command line. Per-user by | |
| # default because TMPDIR is per-user on macOS; FM_TEST_RUN_REGISTRY overrides it | |
| # for tests and for anyone deliberately partitioning runs further. | |
| RUN_REGISTRY="${FM_TEST_RUN_REGISTRY:-${TMPDIR:-/tmp}/fm-test-run-registry}" | |
| # Where executing runs register their identity so a concurrent agent can stop | |
| # one exactly instead of pattern-matching a shared command line. Per-user by | |
| # default because TMPDIR is per-user on macOS; FM_TEST_RUN_REGISTRY overrides it | |
| # for tests and for anyone deliberately partitioning runs further. | |
| RUN_REGISTRY="${FM_TEST_REGISTRY:-${TMPDIR:-/tmp}/fm-test-run-registry-$(id -u)}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/fm-test-run.sh` around lines 121 - 125, Update the RUN_REGISTRY default
in the shell script to include the current user’s identifier in the generated
path, while preserving FM_TEST_RUN_REGISTRY as the explicit override. Ensure the
default registry directory is distinct for different users even when TMPDIR
falls back to /tmp.
| prune_run_registry() { | ||
| local file pid | ||
| [ -d "$RUN_REGISTRY" ] || return 0 | ||
| for file in "$RUN_REGISTRY"/*.run; do | ||
| [ -f "$file" ] || continue | ||
| pid=$(run_record_field "$file" pid || true) | ||
| if [ -z "$pid" ] || ! pid_is_alive "$pid"; then | ||
| rm -f "$file" | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Write registry records atomically.
register_run writes the record in place with a redirect. prune_run_registry can read the same file while that write is in progress. If the pid= line is not yet present, run_record_field returns nothing, the record counts as dead, and rm -f deletes a live run's record. That run then becomes unreachable by --stop, which is the exact failure this feature removes.
The window is small, but concurrent runs are the stated use case: every registering run runs prune_run_registry indirectly whenever another agent calls --list-runs or --stop.
Write to a temporary file in the same directory, then rename it.
🛠️ Proposed fix
file=$(run_registry_file "$RUN_ID")
+ local tmp_file="$file.tmp.$$"
{
printf 'run_id=%s\n' "$RUN_ID"
printf 'pid=%s\n' "$$"
printf 'root=%s\n' "$ROOT"
printf 'fm_home=%s\n' "${FM_HOME:--}"
printf 'json=%s\n' "${JSON_PATH:--}"
printf 'selection=%s\n' "$SELECTION_DESC"
printf 'started=%s\n' "$RUN_STARTED_ISO"
- } >"$file" || die "could not write run record: $file"
+ } >"$tmp_file" || die "could not write run record: $file"
+ mv -f "$tmp_file" "$file" || die "could not publish run record: $file"
RUN_REGISTERED_FILE=$fileAlso applies to: 249-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/fm-test-run.sh` around lines 232 - 242, Update register_run to write the
complete registry record to a temporary file in RUN_REGISTRY, then atomically
rename it to the final .run path only after the write succeeds. Ensure the
temporary file is created in the same directory and cleaned up on failure, while
preserving prune_run_registry’s handling of finalized records.
| waited=0 | ||
| while [ "$waited" -lt "$STOP_GRACE_SECONDS" ] && pid_is_alive "$pid"; do | ||
| sleep 1 | ||
| waited=$((waited + 1)) | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate STOP_GRACE_SECONDS before the wait loop.
FM_TEST_RUN_STOP_GRACE reaches [ "$waited" -lt "$STOP_GRACE_SECONDS" ] unchecked. If a caller exports a non-numeric value, test reports integer expression expected and returns 2. Under set -e the runner then exits after it already sent TERM, so the SIGKILL escalation never runs and the caller gets no FM_TEST_STOPPED line.
Reject a non-numeric value at configuration time.
🛡️ Proposed fix at line 129
STOP_GRACE_SECONDS="${FM_TEST_RUN_STOP_GRACE:-10}"
+case "$STOP_GRACE_SECONDS" in
+ ''|*[!0-9]*) die "FM_TEST_RUN_STOP_GRACE must be a whole number of seconds: $STOP_GRACE_SECONDS" ;;
+esac🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/fm-test-run.sh` around lines 301 - 305, Validate
FM_TEST_RUN_STOP_GRACE/STOP_GRACE_SECONDS during configuration before the wait
loop, rejecting any non-numeric value with the existing configuration-error
behavior. Ensure the loop around pid_is_alive only receives a validated integer
so invalid input cannot trigger set -e before SIGKILL escalation or suppress the
FM_TEST_STOPPED output.
| --stop=*) | ||
| STOP_REQUESTED=1 | ||
| STOP_RUN_ID=${1#--stop=} | ||
| shift | ||
| ;; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject an empty --stop= value.
--stop= sets STOP_RUN_ID to the empty string. stop_runs then takes the no-id branch and stops every run registered from this root. The caller used the = form to name one run, so the fallback stops runs the caller did not name.
🛠️ Proposed fix
--stop=*)
STOP_REQUESTED=1
STOP_RUN_ID=${1#--stop=}
+ [ -n "$STOP_RUN_ID" ] \
+ || die "--stop= needs a run id (use bare --stop for this root; see --list-runs)"
shift
;;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --stop=*) | |
| STOP_REQUESTED=1 | |
| STOP_RUN_ID=${1#--stop=} | |
| shift | |
| ;; | |
| --stop=*) | |
| STOP_REQUESTED=1 | |
| STOP_RUN_ID=${1#--stop=} | |
| [ -n "$STOP_RUN_ID" ] \ | |
| || die "--stop= needs a run id (use bare --stop for this root; see --list-runs)" | |
| shift | |
| ;; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/fm-test-run.sh` around lines 1532 - 1536, Validate the extracted
STOP_RUN_ID in the --stop=* argument branch before setting STOP_REQUESTED or
continuing. Reject an empty value with the script’s existing invalid-argument
behavior, and only invoke the single-run stop path when a non-empty run ID was
supplied; preserve the no-ID behavior for the separate --stop form.
|
Landing without external review: CodeRabbit is rate-limited and never reviewed this PR. All CI checks green; internal pipeline review clean. (Delegated merge sweep, 2026-08-22) |
1 participant
Intent
fm-test-run.sh has no scoped stop, so agents reach for broad pkill and kill sibling agents' suites (robots-19xp). Concurrent agents invoke the runner with near-identical command lines ('--changed --base origin/main'), so 'pkill -f' matched and killed an unrelated sibling agent's run at 86/146 scripts; the truncated log then looked like a short clean run, so a reader counting failures off it concluded the suite passed. Fix: give fm-test-run.sh a run identity and a scoped stop so no agent needs pkill. Each executing run registers run_id/pid/root/selection under FM_TEST_RUN_REGISTRY and removes the record on every exit path, with dead records pruned on read; --list-runs reports live runs; --stop [] stops by identity, and with no id stops only runs registered from THIS repository root so a concurrent agent's run in another worktree is never a candidate; a signalled run traps TERM/INT/HUP and prints exactly one FM_TEST_ABORTED terminator in place of FM_TEST_SUMMARY and exits 128+signal, so truncation is loud rather than silent (that terminator matters independently of the stop verb). AGENTS.md section 8, which already named this hazard class for watchers, now names the test runner too.
What Changed
Final changed paths and statuses:
Risk Assessment
✅ Low: The path-traversal fix is correctly applied, all original feature behaviors are intact, and no new issues were found in either the feature code or the fix-round commit.
Testing
Three new contract tests covering identity-scoped stop, root-scoped bare stop, and dead-record pruning all pass. The end-to-end demonstration confirms that a stopped run exits 143, emits exactly one FM_TEST_ABORTED terminator naming the run_id and signal, and writes no FM_TEST_SUMMARY—making truncated logs loudly self-identifying rather than silently passing.
Evidence: Scoped stop test results (all 3 pass)
ok - --stop <run-id> stops exactly that run and its log identifies itself as aborted ok - bare --stop is scoped to this root and never reaches a concurrent run ok - --stop refuses with no live run and dead records are prunedEvidence: End-to-end demo: --list-runs, --stop, FM_TEST_ABORTED terminator
FM_TEST_RUN run_id=fm-test-run-1786133150955-67575 pid=67575 started=2026-08-07T20:05:50Z root=<repo> selection=scripts --- fm-test-run: stopping run fm-test-run-1786133150955-67575 (pid 67575) FM_TEST_STOPPED run_id=fm-test-run-1786133150955-67575 pid=67575 --- FM_TEST_ABORTED 2026-08-07T20:05:51Z run_id=fm-test-run-1786133150955-67575 signal=TERM completed=0 selected=1Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-test-run.sh:332- The run-id shape checkfm-test-run-[0-9]*-[0-9]*uses bashcaseglob matching where*matches any characters including/. A caller-supplied id likefm-test-run-1/../secret-9passes the check, andrun_registry_file "$want"then produces a path outside the registry directory thatstop_one_runsubsequently passes torm -f. The comment on the guard explicitly claims it 'keeps a caller-supplied id from resolving to a path outside the registry' — that claim is false. Fix: add*/*) die ...as the first branch before the format check so slashes are rejected unconditionally.🔧 Fix: reject slash-in-run-id before format check
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash /tmp/run-stop-tests2.sh— sourced fm_await/fm_stop_fixture/fm_registered_run_count helpers and the three new test functions from tests/fm-test-run.test.sh (lines 770–917), then called test_stop_targets_one_run_and_marks_the_log_aborted, test_stop_without_id_never_reaches_another_roots_run, test_stop_refuses_when_nothing_is_registeredManual end-to-end demo: started a slow fixture run, called--list-runs, called--stop <run-id>, verified the log containsFM_TEST_ABORTED ... signal=TERMand noFM_TEST_SUMMARY✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation