Revise PR #364: the vacuity finding is correct and proven, but the fix is 2 test lines, not a rename of the module constant

[jaylfc]

CARD TITLE (intent, not commit subject): Revise PR #364: the vacuity finding is correct and proven, but the fix is 2 test lines, not a rename of the module constant

Autonomous build of board card tsk-u22koy.

Files:
changelog.d/tsk-vqfjsm-20250818000000.md | 2 ++
tests/test_resume_arm_time.py | 19 +++++++++++++++++++
2 files changed, 21 insertions(+)

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[coderabbitai]

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

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

How can I continue?

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.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f2ffc806-299e-4eb5-9204-bbc149b8dde0

📥 Commits

Reviewing files that changed from the base of the PR and between a252ecd and a19216e.

📒 Files selected for processing (2)

• changelog.d/tsk-vqfjsm-20250818000000.md
• tests/test_resume_arm_time.py

---

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

[kilo-code-bot]

[WARNING]: monkeypatch.setattr on taosmd_service.a2a_send has no effect on the subprocess

This test patches taosmd_service.a2a_send in the parent process, then invokes the script via subprocess.run(["/usr/bin/python3", str(SCRIPT), ...]) (line 304). The child process has its own interpreter and module namespace — the monkeypatch does not carry over. Consequently:

1. The mock is completely ineffective; the real a2a_send runs in the subprocess.
2. The assertions at lines 316–317 (assert "[RESUME DUE]" in record) require the bus post to FAIL so the fallback log is written. If the A2A bus happens to be reachable in the test environment, no log is written and the test fails.
3. The comment "Mock the bus posting to succeed" is misleading: the mock doesn't work, and the test is implicitly testing the failure path while claiming to test the success path.

The test is non-deterministic — its pass/fail depends on external state (whether the A2A bus is running). To make it deterministic, either avoid the subprocess and call do_fire directly in-process, or inject the mock inside the subprocess (e.g., via a wrapper script or environment-driven shim).

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	0
WARNING	1
SUGGESTION	0

Issue Details (click to expand)

WARNING

File	Line	Issue
tests/test_resume_arm_time.py	293	monkeypatch.setattr on a2a_send has no effect on the subprocess; test is non-deterministic

Files Reviewed (2 files)

• changelog.d/tsk-vqfjsm-20250818000000.md - no issues
• tests/test_resume_arm_time.py - 1 issue

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 114.1K · Output: 19.6K · Cached: 236.4K}

[jaylfc]

BLOCK, and the first thing to say is that the core of this is correct: the two assertions added to test_system_crontab_block_names_usr_bin_python3 are exactly what tsk-u22koy asked for, and I verified they work. Carried forward as tsk-tsoylj, which starts from this branch rather than from scratch. The branch is kept.

What is right, measured

With _HELPER_PATH mutated back to the hardcoded path the original defect had:

test file	result
with this PR's two lines	1 failed
master's version	1 passed

They assert against block, the real return value of system_crontab_block, so they engage production code and genuinely close the vacuity. Keep them verbatim.

Why it is blocked

The other 17 lines do not survive a probe. Both findings below are proven by running something, not by reading.

1. The a2a_send mock is dead code

test_do_fire_runs_as_subprocess runs the script through:

subprocess.run(["/usr/bin/python3", str(SCRIPT), "--fire", "primary", ...])

monkeypatch.setattr rebinds an attribute in the parent interpreter. It cannot reach a separate process. Proof, deleting the comment, the import, sent, fake_send and the setattr:

control (as shipped):        1 passed
entire mock block deleted:   1 passed

Identical. The block has no effect on any outcome. Its comment claims it makes "the log contain the path from the crontab line", which is not something it does, and sent is never asserted on. A mock that cannot take effect is worse than no mock, because the next reader believes the subprocess's bus behaviour is controlled.

2. The new assertion in that test is a property of the test file alone

assert f"/usr/bin/python3 {SCRIPT} --fire primary {fire_dt.isoformat()}" in initial_crontab

initial_crontab is an f-string the test builds a few lines earlier, interpolating resume_arm_time._HELPER_PATH. So a test-built string is checked against a test-built substring, and neither proc nor record, the actual outputs of the subprocess, is consulted.

Proof: hoisted to immediately after initial_crontab is defined and followed by a hard stop, it still passes.

E  SystemExit: PROBE: reached the assertion without ever running the subprocess

It evaluates true without the subprocess running at all. It sits after the record assertions and is commented as though it strengthened them, which is the misleading part. It is also redundant, since it can only fail in cases where the hunk-1 assertions already fail.

This matters because it is the same shape as the defect being fixed. tsk-u22koy exists because a test asserted something that could not fail. Half this fix reintroduces that, one test lower down.

3. The fragment ships without a terminating newline

Last byte is 0x2e. Sixth in a row. Worth being precise about why: the gate that catches exactly this was blocked along with #368 and is not on master, so nothing is checking. ls scripts/ has no such script and no workflow references one. That is tsk-n3eubn, and this PR is live evidence it is not merely tidiness.

4. The fragment names the wrong card and the wrong year

changelog.d/tsk-vqfjsm-20250818000000.md. tsk-vqfjsm is the closed predecessor card that PR #364 executed; this PR executes tsk-u22koy. The stamp reads 2025 for 2026 work. Every other fragment is tsk-<id>-<short-description>.md with no date.

5. Trailing whitespace on three added lines.

Verification I ran

trial merge into current master:  clean, ZERO deletions, only the 2 intended files
full suite on the trial merge:    1648 passed, 12 skipped

Two notes on that suite number, since neither is obvious. Master had already moved to 53bde752 and #372 changed this very file, so the base here is stale; the merge is clean and reverts nothing, which only a trial merge could establish. And this PR adds no new test functions, so the count is unchanged by construction. A stable suite count is not evidence that anything here works, which is the whole reason both findings above came from probes instead.