fix(token-metrics): key usage sidecar per-call, not by $$ (concurrency)

[don-petry]

Why

Follow-up to #456. The merged token-usage capture keys its sidecar file by $$:
${TOKEN_LOG_FILE}.last-usage.$$. But scripts/review-one-pr.sh runs the tier-2 engines concurrently:

run_agentic prompts/deep-review.md ... &      # background job
( run_duck prompts/rubber-duck.md ... ) &     # background subshell

In bash, $$ stays the parent shell PID inside both background subshells, so the two concurrent calls collide on the same sidecar — one engine's usage can overwrite, or be read by, the other before _record_engine_tokens logs it. This was flagged by chatgpt-codex-connector (P2) on #456.

BASHPID alone doesn't fix it either: the writer runs in the chain's cmd | tee pipe subshell (one BASHPID) while the reader runs in the calling shell (a different BASHPID) — they'd never agree on the path.

Fix

Key the sidecar to the per-call mktemp temp — unique per concurrent call, and shared writer↔reader via an exported env var:

• Each run_* exports _ENGINE_USAGE_OUT="${per_call_tmp}.usage" after creating its temp. It's inherited by the engine's pipeline subshell, so writer and reader agree; it's unique per call, so concurrent run_agentic/run_duck never share a file.
• _engine_usage_sidecar prefers _ENGINE_USAGE_OUT; the $$ form remains only as a fallback for non-concurrent direct callers.

Tests

• Concurrency isolation — two jobs backgrounded with an identical $$ (the exact failure mode) each read back their own usage (111 vs 222), with a widened race window.
• Per-call-key and fallback assertions for _engine_usage_sidecar; corrected the previous test that wrongly claimed $$ "isolates parallel invocations".
• Made the pre-existing collect_org_jsonl malformed-JSONL test deterministic (jq 1.7 exits 0 on NUL bytes — switched to truncated JSON, which jq rejects across versions).

Test plan

• shellcheck --severity=warning on all touched scripts
• bats — token_metrics (41), engine_writer (37), token_report (17), model_pricing (13), fleet_report (40): 0 failures
• Reproduced the collision and verified isolation after the fix

Resolves the open review thread on #456.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed token usage tracking to prevent concurrent operations from overwriting each other's usage data.

• Tests

  • Enhanced test coverage for concurrent token usage tracking scenarios.
  • Updated test utilities to use deterministic input validation.

[coderabbitai]

Warning

Review limit reached

@don-petry, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 49 minutes and 43 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4444bbb6-0baa-4f92-ba99-e3ab67202cc7

📥 Commits

Reviewing files that changed from the base of the PR and between 3c65b64 and a77aa42.

📒 Files selected for processing (4)

• scripts/engine.sh
• scripts/lib/token-metrics.sh
• tests/dev-lead/unit/test_engine_writer.bats
• tests/dev-lead/unit/test_token_metrics.bats

📝 Walkthrough

Walkthrough

Engine invocations now export a unique _ENGINE_USAGE_OUT sidecar path per call, enabling _engine_usage_sidecar to isolate concurrent token-usage writes. Four engine functions initialize per-call paths, and tests verify both fallback and per-call behavior alongside concurrency safety. One test fix improves determinism.

Changes

Concurrent token-usage sidecar isolation

Layer / File(s)	Summary
Sidecar path keying logic and documentation scripts/lib/token-metrics.sh	_engine_usage_sidecar now returns _ENGINE_USAGE_OUT when set, else falls back to $$-keyed path. Documentation explains why per-call exported paths prevent collisions in concurrent pipeline subshells.
Per-call sidecar initialization in engine entrypoints scripts/engine.sh	run_triage, run_agentic, run_duck, and run_writer each unset any prior _ENGINE_USAGE_OUT, create a unique temp file, and export a per-call ${temp}.usage sidecar path for downstream token-metrics collection.
Sidecar behavior and concurrency test coverage tests/dev-lead/unit/test_token_metrics.bats	Tests verify _engine_usage_sidecar falls back to $$-keyed paths when _ENGINE_USAGE_OUT is unset, confirm per-call paths are used when exported, and include a race test running parallel parse_engine_usage subshells to verify isolated sidecar files have no cross-read.
Test determinism fix for jq-failure case tests/token_report.bats	collect_org_jsonl jq-failure test now uses a deterministic truncated JSON string instead of binary-corrupted bytes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• petry-projects/.github-private#334: This PR refines the token-logging instrumentation by introducing per-call sidecar path coordination via _ENGINE_USAGE_OUT, directly building on the token-usage collection infrastructure from PR #334.
• petry-projects/.github-private#456: This PR updates tests/token_report.bats (the collect_org_jsonl test), which is part of the token-reporting suite introduced in PR #456.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately describes the main change: fixing token-metrics to use per-call unique identifiers instead of $$ for concurrency safety in sidecar keying.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/token-usage-sidecar-concurrency

---

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 and usage tips.}

[don-petry]

Dev-Lead — review-changes (no-changes)

No changes were needed for this PR.

[gemini-code-assist]

Code Review

This pull request introduces a per-call usage-sidecar key (_ENGINE_USAGE_OUT) using unique mktemp paths across several runner functions in scripts/engine.sh and scripts/lib/token-metrics.sh. This change ensures that concurrent engine calls remain isolated and do not collide on a shared $$-keyed sidecar file. Additionally, a flaky test in tests/token_report.bats was fixed by replacing binary input with truncated JSON. The review feedback correctly points out that exporting _ENGINE_USAGE_OUT without declaring it local pollutes the calling shell's environment, and suggests using local -x to safely scope the variable to the respective functions while still exporting it to subshells.

[Copilot]

Pull request overview

This PR fixes a concurrency bug in token-usage capture where the “usage sidecar” file was keyed by $$, causing collisions when tier-2 engines run concurrently (as in scripts/review-one-pr.sh). It switches to a per-call key derived from a unique mktemp path and shared between the pipeline subshell (writer) and the parent shell (reader) via an exported env var, and updates tests to validate isolation and remove a flaky jq-version-dependent fixture.

Changes:

• Key engine usage sidecar files per call via exported _ENGINE_USAGE_OUT (mktemp-derived) with a $$ fallback.
• Add/adjust unit tests to validate per-call keying and concurrent isolation, and fix an incorrect prior assertion about $$.
• Make a malformed-JSONL test deterministic by using truncated JSON instead of NUL bytes.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
scripts/lib/token-metrics.sh	Updates _engine_usage_sidecar to prefer _ENGINE_USAGE_OUT for per-call concurrency-safe keying, with $$ fallback.
scripts/engine.sh	Exports per-call _ENGINE_USAGE_OUT in each run_* entrypoint based on a unique mktemp path.
tests/dev-lead/unit/test_token_metrics.bats	Adds tests for per-call keying and concurrent isolation; corrects the prior $$ behavior explanation.
tests/token_report.bats	Replaces flaky NUL-byte fixture with truncated JSON to ensure jq failure is deterministic across versions.

[donpetry-bot]

Automated review — APPROVED ✓

Risk: LOW
Reviewed commit: aebff67372c5adfb5412d5362c69939d3d7cb896
Review mode: triage-approved (single reviewer)

Summary

Focused follow-up to #456 that fixes a real concurrency bug in the token-usage sidecar. scripts/review-one-pr.sh backgrounds run_agentic & and (run_duck) & simultaneously; in bash, $$ stays the parent PID in both subshells, so the prior ${TOKEN_LOG_FILE}.last-usage.$$ key collided. The fix keys the sidecar to a per-call mktemp path exported via _ENGINE_USAGE_OUT — unique by construction per call, and inherited by the engine's cmd | tee pipeline subshell so writer (subshell) and reader (calling shell) agree. The $$ form is retained as a fallback for non-concurrent direct callers. The PR body and updated _engine_usage_sidecar header comment correctly explain why neither $$ nor BASHPID alone solves both requirements.

Linked issue analysis

No linked issue; PR resolves the open chatgpt-codex-connector P2 review thread on #456 ("key usage sidecars by BASHPID"). The fix matches the substance of that feedback while documenting why BASHPID alone is insufficient.

Findings

• Correctness: The mechanism is sound. export _ENGINE_USAGE_OUT in each run_* is inherited by the immediately-following pipeline subshell where parse_engine_usage writes the sidecar, and the same exported value is visible to the parent when it later calls _record_engine_tokens. The new bats test (concurrent calls sharing $$ stay isolated) reproduces the failure mode with a widened race window (sleep 0.3) and asserts the 111/222 values do not cross-contaminate.
• Test hygiene: Good correction — the prior path includes shell PID to isolate parallel invocations test was asserting a false invariant; it's rightly renamed to a fallback assertion and the doc now points to the concurrency test for the real guarantee. The jq 1.7 / NUL-byte fix in token_report.bats is a sensible portability tightening.
• Maintainability (non-blocking): gemini-code-assist flagged four sites (scripts/engine.sh:503,578,703,860) suggesting local -x _ENGINE_USAGE_OUT=... instead of export _ENGINE_USAGE_OUT=.... The current code is functionally correct because every run_* re-exports a fresh path before the engine pipeline runs and _record_engine_tokens is invoked synchronously inside the same run_* call. However, local -x would prevent the variable from leaking into the parent shell's environment after the function returns (where it would point at a stale temp path that any other future caller of _engine_usage_sidecar would pick up instead of the $$ fallback). Worth considering in a follow-up; not blocking this PR.
• Security: No security surface — bash test/CI tooling, no secrets, no auth, no CodeQL/Gitleaks/AgentShield findings.

CI status

All required checks green: CodeQL, ShellCheck, bats, Lint (validate-agent-profiles, gh-aw-compile, Compile agentic workflows), Secret scan (gitleaks), Agent Security Scan, AgentShield, SonarCloud (Quality Gate passed, 0 new issues), unit-tests, Tests, Dependency audit (ecosystems detected, no audits required). The in-progress Dev-Lead Agent / dispatch is this review run itself.

---

Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.

[don-petry]

Dev-Lead — fix-reviews (applied)

Changes committed and pushed.

[don-petry]

Dev-Lead — review-changes (applied)

Changes committed and pushed.

[don-petry]

Dev-Lead — rate-limited (intent: review-changes)

PR: #460
The retry cron will re-attempt automatically. Rate limit resets at: 2026-06-07T13:50:30Z

[don-petry]

Note

@don-petry I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears.
Rate limit resets at: 2026-06-07T13:50:30Z

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@scripts/engine.sh`:
- Around line 499-505: The conditional guards that export the mktemp-derived
sidecar use the POSIX [ -n "$_tok_tmp" ] test; update each occurrence in the
functions run_triage, run_agentic, run_duck, and run_writer to use Bash's [[ -n
"$_tok_tmp" ]] form when deciding to export _ENGINE_USAGE_OUT (the mktemp result
stored in _tok_tmp) so the guard matches the repo's required Bash conditional
style and avoids subtle word-splitting/regex differences.

In `@scripts/lib/token-metrics.sh`:
- Around line 149-153: In _engine_usage_sidecar replace the POSIX test [ -n
"${_ENGINE_USAGE_OUT:-}" ] with the Bash conditional [[ -n
"${_ENGINE_USAGE_OUT:-}" ]] so the script follows the repo's Bash guideline;
update the conditional that decides between printing "$_ENGINE_USAGE_OUT" and
printing "${TOKEN_LOG_FILE}.last-usage.$$" to use [[ ... ]] (refer to the
_ENGINE_USAGE_OUT variable and TOKEN_LOG_FILE in the existing if/else block).

🪄 Autofix (Beta)

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1d34f9e1-7465-44f3-a7f7-6a5e548783c9

📥 Commits

Reviewing files that changed from the base of the PR and between 0c51ebd and 3c65b64.

📒 Files selected for processing (4)

• scripts/engine.sh
• scripts/lib/token-metrics.sh
• tests/dev-lead/unit/test_token_metrics.bats
• tests/token_report.bats

[don-petry]

Dev-Lead — fix-reviews (applied)

Changes committed and pushed.

[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

[don-petry]

Dev-Lead — review-changes (no-changes)

No changes were needed for this PR.

[don-petry]

@donpetry-bot please review