resume_arm_time.py emits a crontab line naming whatever checkout ran it, so arming from a worktree pins a path that will be deleted

[jaylfc]

CARD TITLE (intent, not commit subject): resume_arm_time.py emits a crontab line naming whatever checkout ran it, so arming from a worktree pins a path that will be deleted

Autonomous build of board card tsk-bxdfvz.

Files:
changelog.d/tsk-bxdfvz-helper-path-guard.md | 3 ++
scripts/resume_arm_time.py | 47 +++++++++++++++++++++--
tests/test_resume_arm_time.py | 59 +++++++++++++++++++++++++++--
3 files changed, 102 insertions(+), 7 deletions(-)

[qodo-code-review]

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

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 4 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: 207d2cde-88d5-4fec-a7f4-af6e51145a4b

📥 Commits

Reviewing files that changed from the base of the PR and between 0778149 and 7e77379.

📒 Files selected for processing (3)

• changelog.d/tsk-bxdfvz-helper-path-guard.md
• scripts/resume_arm_time.py
• 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.}

[gitar-bot]

Important

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

_Gitar

[kilo-code-bot]

WARNING: _is_under_temp misses resolved macOS temp paths

_HELPER_PATH is computed with os.path.realpath(__file__), but the temp-root list only contains the unresolved paths. On macOS, /tmp and /var/tmp are symlinks to /private/tmp and /private/var/tmp respectively. After realpath, a checkout under /tmp would resolve to /private/tmp/..., which is not in the check list, so the guard silently fails to detect it.

---

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

[kilo-code-bot]

WARNING: _is_in_linked_worktree false-positives on git submodules

Git submodules also have .git as a file (containing gitdir: ...), but they are not ephemeral like linked worktrees. The script would refuse to run from a submodule checkout, which is overly broad and could block legitimate non-ephemeral use cases.

---

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

[kilo-code-bot]

SUGGESTION: _validate_helper_path() is called late in main()

This validation runs after all the crontab derivation computation. If the script is invoked from an ephemeral location, it still performs the full derivation before refusing to emit the block. Moving the call to the very start of main() would fail fast and avoid unnecessary work.

---

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

[kilo-code-bot]

SUGGESTION: test_guard_refuses_linked_worktree uses a hardcoded absolute path

Path("/home/jay/Development/fake-wt-for-test") may not exist or be writable in all test environments (e.g., CI containers, different users). Using tmp_path would make the test hermetic and avoid leaving directories behind.

---

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

[kilo-code-bot]

Code Review Summary

Status: 4 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
WARNING	2
SUGGESTION	2

Issue Details (click to expand)

WARNING

File	Line	Issue
scripts/resume_arm_time.py	503	_is_under_temp misses resolved macOS temp paths (/tmp and /var/tmp resolve to /private/tmp and /private/var/tmp on macOS)
scripts/resume_arm_time.py	515	_is_in_linked_worktree false-positives on git submodules (.git is a file in submodules, blocking legitimate non-ephemeral checkouts)

SUGGESTION

File	Line	Issue
scripts/resume_arm_time.py	788	_validate_helper_path() is called late in main() after all computation instead of at the start
tests/test_resume_arm_time.py	188	test_guard_refuses_linked_worktree uses a hardcoded absolute path that may not exist in all environments

Files Reviewed (3 files)

• scripts/resume_arm_time.py - 3 issues
• tests/test_resume_arm_time.py - 1 issue

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 69.4K · Output: 20.3K · Cached: 753.5K}

[jaylfc]

BLOCKED, and the guard itself is the right idea. Refusing to emit a crontab block when the helper resolves under a temp directory or a linked worktree is a real safety property, it is the only genuinely new thing in this PR, and I want it to land. The changelog fragment is present and all three files end in 0x0a, so the trailing-newline problem that hit the last four PRs did not recur here. What blocks it is that most of the diff is already on master, and the part that is not would take two working tests backwards.

Revision card: tsk-if2fpm. Closing this PR per close-on-block. The branch exec/tsk-bxdfvz is kept and stays fetchable.

1. The central change already landed in #354

_HELPER_PATH = os.path.realpath(__file__) and all three call sites are already on master. They arrived in #354, commit 10d60ab8, "derive the helper path from __file__ so armed crontab lines name the canonical in-repo script".

$ git show origin/master:scripts/resume_arm_time.py | grep -n '_HELPER_PATH'
57:_HELPER_PATH = os.path.realpath(__file__)
508:    marker_prefix = _HELPER_PATH + "#"
509:    helper = _HELPER_PATH
544:    marker = f"{_HELPER_PATH}#{fire_type}-{ts}"

So the PR re-adds a constant that exists and re-fixes three call sites that are already fixed. That is why the trial merge conflicts:

$ git merge origin/exec/tsk-bxdfvz     # onto origin/master
CONFLICT (content): Merge conflict in tests/test_resume_arm_time.py

This is worth saying plainly because the card title describes the OLD behaviour, where the script emitted a hardcoded .taos-fleet-tools path. That was true when the card was written. It is not true of master any more.

2. The test half would revert two assertions master already has in a stronger form

This is the part that matters, and it is the third time in this repo that a branch built from a stale base has quietly given back a win master already held. The last one was #360 dropping the README newline that #358 had won.

_marker, line 43. Master derives the expected path independently of the module under test:

SCRIPT = REPO / "scripts" / "resume_arm_time.py"    # computed by the test file
...
return f"{SCRIPT}#{fire_type}-{ts}"                 # master

This PR rewrites it to read the value back out of the thing being tested:

return f"{resume_arm_time._HELPER_PATH}#{fire_type}-{ts}"   # this PR

Both sides of the comparison then come from the same source, so the assertion passes whatever path the module computes, including a wrong one. It cannot fail. That is precisely the defect card tsk-vqfjsm was opened to fix, so this change would undo that card's premise while it is still in flight as #364.

test_do_fire_runs_as_subprocess. Master derives the marker from system_crontab_block() output, which couples the test to the arming code that actually produces the marker. This PR reverts it to a hand-written literal plus armed_at.

Neither revert is visible in the PR's own diff, because against the stale base both edits read as improvements. Only the trial merge shows it.

3. test_guard_refuses_linked_worktree writes outside the sandbox, on the live box

fake_wt = Path("/home/jay/Development/fake-wt-for-test")
fake_wt.mkdir(parents=True, exist_ok=True)
(fake_wt / ".git").write_text("gitdir: /some/real/.git/worktrees/fake\n")

It creates a real directory next to the working checkout, writes into it, and rmdirs it in a finally. It also requests the tmp_path fixture and never uses it. Since #337 the standing rule for this file is that every test is sandboxed.

My first probe for this was ambiguous and I want to flag that rather than hide it: I polled for the directory during a run and never saw it, which proves nothing, because the directory exists for only a fraction of a second. The positive control is decisive. With a directory of that name already present, the test fails:

E       OSError: [Errno 39] Directory not empty: '/home/jay/Development/fake-wt-for-test'

So it engages that path for real, and it removes a directory it did not create. (I placed that sentinel myself and removed it afterwards; the suite is back to 9 passed and no residue is left on the box.)

There is a real reason the author avoided tmp_path, and the revision has to deal with it: tmp_path lives under /tmp, and _validate_helper_path checks _is_under_temp first, so a tmp_path-based fake worktree trips the temp branch and never reaches the worktree branch. The clean way out is to test _is_in_linked_worktree() directly as a unit, since that function has no temp dependency of its own.

One thing to keep

The monkeypatch this PR adds to test_canonical_derivation_emits_date_pinned_one_shot is not optional, and the revision must keep it. Once the guard exists, that existing test fails without it, because the suite is routinely run from a linked worktree under /tmp and the guard correctly refuses there. I confirmed this by grafting the guard onto master's test file without the monkeypatch:

FAILED tests/test_resume_arm_time.py::test_canonical_derivation_emits_date_pinned_one_shot
1 failed, 8 passed

Run as-is on its own branch the PR is green, 9 passed, so this is not a defect in the PR. It is a property of the guard that the revision needs to carry forward deliberately, and it is worth stating in the changelog: after this lands, main() is environment-sensitive by design.

What the revision should contain

Branch from origin/master at 66de364c or later, then keep only _is_under_temp, _is_in_linked_worktree, _validate_helper_path, the one call in main(), the monkeypatch above, a sandboxed worktree test, and the changelog fragment reworded to describe the guard alone. Leave _marker and test_do_fire_runs_as_subprocess exactly as master has them. Full end state is on tsk-if2fpm.

[jaylfc]

Correction to point 2 above, and a collision the revision lane needs to know about.

I called both of master's versions "strictly stronger". That is right for _marker and wrong for test_do_fire_runs_as_subprocess, and the difference matters because tsk-if2fpm tells you to leave that test exactly as master has it.

I measured master against an API-preserving mutant, _HELPER_PATH = "/wrong/nowhere/resume_arm_time.py" with nothing else changed, and ran the whole file:

notices the wrong path      test_do_fire_posts_resume_due_to_bus
                            test_do_fire_bus_failure_writes_visible_record

does NOT notice             test_system_crontab_block_names_usr_bin_python3
                            test_do_fire_runs_as_subprocess
                            test_canonical_derivation_emits_date_pinned_one_shot
                            test_do_fire_crontab_read_failure_is_visible
                            test_helper_imports_getpass

2 failed, 5 passed

So master's test_do_fire_runs_as_subprocess passes with the helper path pointing at a directory that does not exist. Deriving the marker from system_crontab_block() does couple the test to the arming code, which is a real gain and catches a format divergence between arming and firing. But it also means both sides of the comparison come out of the module under test, so the test cannot see a wrong path. It is stronger in one dimension and weaker in another, not strictly stronger, and I should not have written it that way.

What does not change: this PR's combination is still weaker overall, because it rewrites _marker to _HELPER_PATH as well, which makes the _marker path self-consistent too. _marker on master uses the independently derived SCRIPT, and that one genuinely is strictly stronger.

The collision. tsk-if2fpm says leave test_do_fire_runs_as_subprocess as master has it. tsk-vqfjsm, in flight as #364, is chartered to remove exactly the vacuity that test still has. Treat tsk-vqfjsm as the authority for that test and leave it alone here, so the two revisions do not fight over the same lines. tsk-if2fpm is only asking you not to revert it; it is not asserting the line is finished.