Preserve fixture writer timeout diagnostics

[justin808]

Why

Follow-up to merged #824 / #823. The fixture writer timeout path could append a secondary cleanup failure after the primary timeout, obscuring the actionable diagnostic.

What changed

• Clear the writer PID after the timeout path reaps it.
• Add a focused timeout regression in a dedicated stack-test file, keeping repository_test.bash within its 180-line cap.
• Preserve the existing live mutable-pack race coverage.

How to review and verify

Inspect the timeout branch in test/agent_stack/repository_test.bash and the focused failure-path test in test/agent_stack/repository_timeout_test.bash.

Test plan

• bin/validate - passed clean on f70ae01a4ffbb938623bbd8023d481260a61965d.
• ruby bin/lint - passed (RuboCop, ShellCheck, markdownlint-cli2, yamllint, actionlint).
• Focused timeout and existing live mutable-pack stack tests - passed.
• Changelog classification recorded (deferred_to_update_changelog).

Agent details

Commands and results

• Independent local review: clean.
• Full validation and lint passed before publication.

Decision log

• Non-blocking: The original test file had a 180-line architecture cap.
  • Decision: Place the new focused test in its own sourced stack-test file.
  • Why: This keeps the repair small and the existing live-race coverage intact.
  • Review later: None

---

Note

Low Risk
Test-only changes to fixture teardown and a new regression test; no production or install behavior.

Overview
When the agent-stack fixture temporary-pack writer hits its start deadline, the timeout path now unsets the writer PID after reaping it so fixture_clone_writer_finish on exit does not treat the process as still running and append a secondary "temporary-pack writer failed" message on top of the primary "temporary-pack writer did not start" failure.

A dedicated stack test file adds test_fixture_clone_writer_timeout_preserves_primary_failure, wired into bin/agent-stack-test.bash, which forces the timeout via stubbed Ruby/sleep and asserts only the start-timeout diagnostic appears. This keeps repository_test.bash under its line cap without changing the existing live mutable-pack race coverage.

^{Reviewed by Cursor Bugbot for commit f70ae01. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 250bbe17-7ecf-44c8-b051-cedb496bed00

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review

The core fix is correct and minimal: in test/agent_stack/repository_test.bash, fixture_clone_writer_start's timeout branch now unsets AGENT_STACK_FIXTURE_RACE_WRITER_PID after reaping it, so the trap ... EXIT-invoked fixture_clone_writer_finish (which checks -n "${AGENT_STACK_FIXTURE_RACE_WRITER_PID:-}") no longer re-waits on an already-reaped PID and appends a spurious "temporary-pack writer failed" message on top of the real "did not start" diagnostic. One-line, targeted, no side effects on the success path.

The new regression test (test/agent_stack/repository_timeout_test.bash) is well constructed: it stubs RUBY_BIN to fail only on the -rtimeout invocation (falling back to the real ruby otherwise), and overrides sleep to fast-forward SECONDS to the deadline via bash's dynamic scoping of the caller's local deadline, so the timeout path triggers deterministically without a real 10s wait. It correctly asserts both that the primary failure message is present and that the secondary "failed" message is absent.

One minor scope nit posted inline: the new test lives in its own file (plus a new source line in bin/agent-stack-test.bash) to keep repository_test.bash under a "180-line cap" cited in the PR description, but no such cap is actually enforced anywhere in the repo (no lint rule, no docs). Appending the ~30 lines directly to the existing file would have been a smaller change with no new file/wiring. Not blocking.

No security, performance, or correctness concerns found otherwise.

[justin808]

🤖 Codex

Review follow-up and current-main integration are complete for db433fe36e6245d43a22ae30a405686f70cb4049.

• Resolved the sole prior-head thread as OPTIONAL: the focused timeout test file remains because it keeps the fixture/writer-diagnostics scenario independently readable. The claimed 180-line cap was not a repository convention.
• Rebased the unchanged one-commit candidate onto current main (c1fe5e3af7513fd98e117f87bf1034c4e7d389c7) and force-updated the branch with an exact old-head lease.
• Local bin/validate completed before the push. Current-head lint and CodeRabbit are green; claude-review and hosted validate are still running.

Next: after those exact-head checks settle, re-fetch review data and re-run current-head security/readiness before applying auto_merge_when_gates_pass.

[claude]

Review

Reviewed the diff (bin/agent-stack-test.bash, test/agent_stack/repository_test.bash, test/agent_stack/repository_timeout_test.bash).

Correctness of the fix: The unset AGENT_STACK_FIXTURE_RACE_WRITER_PID added to the timeout branch in repository_test.bash is correct and minimal. It prevents fixture_clone_writer_finish's trap from calling wait a second time on a PID that was already reaped by the timeout path, which is exactly what would produce the spurious "temporary-pack writer failed" message on top of the primary "did not start" diagnostic. No unintended effect on the non-timeout (successful writer) path, since unset only executes inside the branch that's about to fail anyway.

New regression test (repository_timeout_test.bash): traced through this carefully, including two tricky bits that are easy to get wrong but are done correctly here:

• The heredoc uses <<BASH (unquoted, so $RUBY_BIN bakes in the real ruby path at doc-creation time), but correctly escapes \${1:-} and \$@ so those are deferred to the generated script's own runtime arguments instead of being expanded immediately against the test function's (empty) $@. Got this wrong on first read and had to double-check — it's correct as written.
• The sleep() { SECONDS="$deadline"; } override relies on bash's dynamic scoping of local deadline from fixture_clone_writer_start's call frame; this works as intended and the shellcheck disable=SC2154,SC2329 comment correctly explains why static analysis can't see the cross-file connection.

Scope: small and proportional to the bug — one production-code line, one test registration line, and a focused 30-line test kept in its own file to respect the existing 180-line cap on repository_test.bash. No speculative abstractions, flags, or helpers introduced.

No blocking issues found.