Skip to content

test(talos): judge only the helper's own source checkouts in cleanup - #5285

Merged
devantler merged 1 commit into
mainfrom
claude/test-fold-checkout-scope-5221
Oct 24, 2026
Merged

devantler merged 1 commit into
mainfrom
claude/test-fold-checkout-scope-5221

Conversation

@devantler

Copy link
Copy Markdown
Contributor

Why

A check that proves the Talos release audit cleans up after itself reported a leak whenever some other tool had left an unrelated file in the shared temporary folder, which happens inside the Mac sandbox some agents use. That turned a clean run into a false failure on unrelated changes.

What

The check now looks only at the copies the audit itself creates, so unrelated files no longer count, while a copy the audit really leaves behind is still caught. Nothing the platform deploys changes.

Fixes #5221

🤖 Generated with Claude Code

The fold-input cleanup test required the whole temporary directory to be
empty, so an unrelated file another tool leaves there (a macOS sandbox writes
xcrun's cache) read as a leaked source checkout. Name the helper's checkout
prefix once, count only entries that carry it, plant such a cache file in the
test, and add a control proving a retained checkout is still found.

Fixes #5221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 1e8dad9 and 7e1daf2.

Files selected for processing: 2
  • scripts/reconcile-talos-kernel-args/audited_inputs.go
  • scripts/reconcile-talos-kernel-args/main_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Recent review details
Additional comments: 2
scripts/reconcile-talos-kernel-args/audited_inputs.go: 1

275-278: LGTM!

Also applies to: 287-287

scripts/reconcile-talos-kernel-args/main_test.go: 1

678-705: LGTM!

Also applies to: 712-712, 727-729, 732-735, 737-755

Walkthrough

The helper now uses a constant for its temporary checkout prefix; the prefix value is unchanged. Cleanup tests identify helper-owned checkouts by that prefix. They verify cleanup after successful and failed source reads, preserve an unrelated xcrun_db cache, and detect a deliberately retained checkout.

Priority: Low

Severity of issue fixed: Low

Merge Risk: Minimal · up to 7e1da

The cleanup test now ignores the reported unrelated cache entry while still checking for audit-owned checkout leaks.

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Linked Issues check Inconclusive The implementation evidence supports the coding objectives in #5221. resolveFoldInputsWithCleanup uses the owned-checkout prefix, and the test summary reports successful-read cleanup, failed-read cl… Provide reviewable results for the complete package suite in both required environments and for the required CI verification. The available evidence does not establish whether those acceptance criteria passed.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title names the test and the change: the cleanup check now judges only the helper's own source checkouts. It matches the changeset, which limits the check to entries with the checkout prefix and a…
Description check Passed The description gives the reason (a false leak failure from an unrelated file in the shared temporary folder) and the change (the check looks only at the audit's own copies). It also states that a rea…
Out of Scope Changes check Passed The changes stay within #5221. The production change names the temporary checkout prefix without changing deployment behavior. The added fixtures and controls test owned-checkout cleanup and unrelated…
Docstring Coverage Passed Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files.
Full details: Title check

Explanation

The title names the test and the change: the cleanup check now judges only the helper's own source checkouts. It matches the changeset, which limits the check to entries with the checkout prefix and adds an unrelated cache fixture. The title is specific and short, with no vague terms.

Full details: Description check

Explanation

The description gives the reason (a false leak failure from an unrelated file in the shared temporary folder) and the change (the check looks only at the audit's own copies). It also states that a real leftover copy is still caught. This matches the test changes in main_test.go and the prefix constant use in audited_inputs.go.

Full details: Out of Scope Changes check

Explanation

The changes stay within #5221. The production change names the temporary checkout prefix without changing deployment behavior. The added fixtures and controls test owned-checkout cleanup and unrelated temporary-directory pollution. No unrelated platform or production behavior change is shown.

Full details: Linked Issues check

Explanation

The implementation evidence supports the coding objectives in #5221. resolveFoldInputsWithCleanup uses the owned-checkout prefix, and the test summary reports successful-read cleanup, failed-read cleanup, unrelated xcrun_db pollution, and detection of a retained owned checkout. The existing injected cleanup-error and source-identity controls remain in the reviewed files. The PR provides no results for the required complete package suite in the macOS sandbox or host environment, and no required CI verification evidence.

Resolution

Provide reviewable results for the complete package suite in both required environments and for the required CI verification. The available evidence does not establish whether those acceptance criteria passed.

  • 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

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

@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer (interactive Claude Code session)

Readiness at 7e1daf26.

  • What I tried as a user: ran the complete scripts/reconcile-talos-kernel-args package suite on the macOS host (go test -count=1), as a contributor validating an unrelated change would: it passes.
  • The false failure is reproduced on any host: the cleanup test now plants an unrelated xcrun_db file in its temporary directory, the file the macOS sandbox leaves there. With the old assertion (temporary directory must be empty) that file alone fails the test: planting it into main's unchanged test fails with temporary source checkout remains after reading 1.2.3: 1 entries. With the new assertion, which counts only entries carrying the helper's own checkout prefix, it passes, and the test also asserts the unrelated file was left alone.
  • A real leak still fails: the new TestOwnedCheckoutsFindsARetainedCheckout runs the resolver with a removal that keeps the checkout, and requires exactly that checkout to be found beside the unrelated file. The injected cleanup-error test is unchanged and passes.
  • Not run: the Codex macOS sandbox itself. The planted file stands in for what that sandbox writes, so the sandbox-only condition is now exercised on every host and in CI.
  • Checks: go vet and gofmt clean; hosted CI green at this head.
  • Review: CodeRabbit reviewed this head with no actionable comments (status Review completed, after its first request at 17:28Z was refused by its rate limit); no review threads.
  • Scope: test-only apart from naming the existing checkout prefix once; nothing the platform deploys changes, so there is no runtime surface to evaluate after deployment.

@devantler
devantler marked this pull request as ready for review October 24, 2026 18:35
@devantler
devantler added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit c488afa Oct 24, 2026
43 checks passed
@devantler
devantler deleted the claude/test-fold-checkout-scope-5221 branch October 24, 2026 18:50
@github-project-automation github-project-automation Bot moved this from 🫴 Ready to ✅ Done in 🌊 Project Board Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Limit source cleanup assertions to the helper temporary checkout

1 participant