[don-petry]

Problem

Part 2 of "Test suites share fixed files under /tmp" (#2271). Part 1 (#2296) gave five of the six dev-lead sidecar files a per-test directory. The sixth, dev-lead-session-output.txt, was left on a fixed path because moving it rewrites lines that "fix(tests): make every negated bats assertion able to fail" (#2264) was also rewriting. That has merged.

The session log is the most exposed of the six: test_fix_reviews.bats seeds it and then asserts on its absence, and test_engine_writer.bats and test_engine_fallback.bats assert on its content, so two sessions on one machine overwrite each other's file. It showed up again today: run several suites side by side and test_fix_reviews and test_engine_fallback lose tests that pass alone.

Design

The same single definition as part 1. run_writer writes, and dev-lead-fix-issue.sh and dev-lead-fix-reviews.sh read, "$DEV_LEAD_SIDECAR_DIR/dev-lead-session-output.txt"; with the variable unset that is /tmp/dev-lead-session-output.txt, as before. The suites already export the variable from part 1.

The six "relocation pending" rows are removed from scripts/lib/fixed-tmp-paths.tsv, so the static check now fails if a literal comes back.

After this, the one fixed path under /tmp still shared between tests is /tmp/cascade in scripts/review-one-pr.sh, which needs a change on three sides and is tracked as #2302.

Risk

• Production: with the variable unset the path is byte for byte what it was. No workflow sets it or reads the file.
• The conflict with fix(tests): make every negated bats assertion able to fail, and lint for new ones (#2263) #2264 was resolved by keeping that pull request's || false form on every negated line and changing only the path.
• From the QA advisory on part 1, not done here: a suite-level default for a suite that forgets the export. "fix(engine): keep the token ledger out of test suites" (fix(engine): keep the token ledger out of test suites and out of the model CLI's environment #2311) is adding suite hooks to every test directory; the default belongs in those hooks once it merges, to avoid two pull requests editing the same six lines.

Test plan

• test_engine_writer 43 of 43, test_engine_fallback 38 of 38, test_fix_issue 32 of 32, the fixed-path check's suite 18 of 18.
• test_fix_reviews: 272 pass and 1 fails. The failure ("dev-lead test-regression guard runs PR test code as the harness user: job secrets are still readable via /proc and the real checkout #2143: the remote head moved during the test job") fails the same way on unmodified main on this machine, alone and in the full file, so it is not from this change. CI is the authority.
• scripts/check-fixed-tmp-paths.sh exits 0 with the six rows removed; the negation lint from fix(tests): make every negated bats assertion able to fail, and lint for new ones (#2263) #2264 exits 0; shellcheck --severity=warning -x clean on the three scripts.
• From the original work on this change: with a background loop writing garbage to the default paths, test_engine_writer went from 41 passing and 2 failing on main to 43 passing.
• Not run: CI.

Rollback

Revert the squash commit; the six exception rows return with it.

Monitoring

None beyond CI. A failure in test_check_fixed_tmp_paths.bats after another pull request merges names the file that added a fixed path.

Closes #2271

🤖 Generated with Claude Code

[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, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 57 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 95b49c23-0f17-4f62-8506-0e38948e8c73

---

📥 Commits

Reviewing files that changed from the base of the PR and between 5797da5 and 6ae6ce2.

---

⛔ Files ignored due to path filters (1)

• scripts/lib/fixed-tmp-paths.tsv is excluded by !**/*.tsv

---

📒 Files selected for processing (8)

• docs/dev-lead/spec.md
• scripts/dev-lead-fix-issue.sh
• scripts/dev-lead-fix-reviews.sh
• scripts/engine.sh
• tests/dev-lead/unit/test_engine_fallback.bats
• tests/dev-lead/unit/test_engine_writer.bats
• tests/dev-lead/unit/test_fix_reviews.bats
• tests/test_check_fixed_tmp_paths.bats

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.}

[don-petry]

dev-lead is withholding action on this item.

It is labeled dev-lead:hands-off (deliberately kept off-limits to dev-lead (e.g. changes to the dev-lead automation itself)), so dev-lead will not pick it up while that label is present. This notice is posted once so the withhold is visible rather than looking like a stalled run.

To re-enable automated pickup: remove the dev-lead:hands-off label.

[don-petry]

QA Lead — test-risk advisory

Risk tier: LOW. This is a test-isolation fix. With the variable unset the production path is still /tmp, and both handler scripts source engine.sh (which sets that default) before they read the file.

What I'd shore up (highest leverage first):

• The static check got looser. The old check required the quoted form "$DEV_LEAD_SIDECAR_DIR/<file>". The new sed drops the quotes, so an unquoted reference now passes as well. Please put the quoted form back as the rule and allow only the one ::warning:: string as an exception, so the guard doesn't drift.
• Nothing proves the readers use the variable. No test fails if read_session_summary or session_snippet silently reads /tmp again. The background-garbage-loop experiment in the test plan shows the fix works, but it ran once by hand. A cheap permanent version: one test per reader that sets the sidecar dir to an empty per-test directory and asserts the "no session log" branch is taken.
• No suite-level default yet. A new suite that forgets the export still writes to shared /tmp without any warning. Deferring this to the fix(engine): keep the token ledger out of test suites and out of the model CLI's environment #2311 hooks is reasonable, but give it a tracked follow-up so it isn't lost.
• The known failure (dev-lead test-regression guard runs PR test code as the harness user: job secrets are still readable via /proc and the real checkout #2143, "remote head moved") depends on the machine. If CI is green, record it as a separate flaky/environment-dependent test with its own ticket. Don't let it become a failure everyone learns to ignore.

Escalate? no

Advisory only — I comment, I do not change code. Ground: BMAD Test Architecture
(bmad-tea v1.19.0). Opt out on this item with the qa-lead:hands-off label.

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-11T22:10:06Z.

[codeant-ai]

CodeAnt Nitpicks

1 code suggestion

1. When mktemp fails, this cleanup is skipped and the previous session log remains; handlers can then attribute stale output to the current run.

Stale reference · scripts/engine.sh:2007

[codeant-ai]

CodeAnt PR Risk: Low Risk

• The PR appears safe to merge; it moves the redacted session log to the configured sidecar directory.
• The writer, handlers, and tests use the same path, while the documented default remains /tmp.

Assessed commit: 6ae6ce24c352

[gemini-code-assist]

Code Review

This pull request relocates the redacted session log file (dev-lead-session-output.txt) from the hardcoded /tmp directory to the configurable $DEV_LEAD_SIDECAR_DIR directory, aligning it with other sidecar files to ensure proper isolation during concurrent test runs. The changes update the documentation, scripts, and associated unit tests, and clean up the pending relocation entries in fixed-tmp-paths.tsv. There are no review comments provided, and I have no additional feedback to offer as the changes are clean and complete.

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-11T22:31:43Z.

[don-petry]

Review round 1, head 6ae6ce24. From the QA advisory:

• The static check got looser: right, restored. The rule is again the exact quoted expression. The one other spelling (the path as text inside run_writer's warning message) is allowed by a pattern for that message only, and the test requires it to match exactly one line. An unquoted reference in a reader now fails the test.
• "Nothing proves the readers use the variable": existing tests do, checked by mutation. With read_session_summary put back on the fixed path, all six post_no_changes session-log tests in test_fix_reviews fail; with session_snippet put back, fix-issue: engine-error embeds redacted session-output snippet fails. Both seed the log through the variable and assert on what the reader returns. A bare literal in either script also fails scripts/check-fixed-tmp-paths.sh, now that the exception rows are gone.
• No suite-level default yet: tracked as 2315.
• The #2143 test that fails locally: it passes in CI on this pull request (unit is green) and fails on macOS on unmodified main, so it is a local toolchain difference, like the others listed for this machine, not a flaky test in CI. I have not opened a ticket for it.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-12T02:39:29Z.