From 7cd127745271ae2be217197b4c17cfef2551f15f Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Thu, 22 Oct 2026 12:25:18 +0100 Subject: [PATCH] fix(agents): give agent output a well-known home and stop lock markers leaking into new worktrees Agents saved evidence outside the worktree with no location named, so logs, PR-body drafts and validation folders piled up loose in the repository's parent directory, where no cleanup looks. Evidence now goes in
-evidence//, and Remove-MergedWorktrees.ps1 deletes entries whose newest file is older than -EvidenceRetentionDays (default 7). The issue-pr-loop skill names one home for throwaway checkouts and tool downloads too. `git worktree add` copies the source checkout's config.worktree, so a stale agent.lockName in the main checkout, or in any worktree used as a source, reached every new worktree and implicit verbs resolved the wrong Redis lock. The marker now records the worktree path it belongs to in agent.lockWorktree; a copied marker names another path and is ignored. Set-WorktreeLockMarker also refuses the main checkout. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/issue-pr-loop/SKILL.md | 7 ++++ scripts/AgentLocks.ps1 | 26 ++++++++++++-- scripts/Remove-MergedWorktrees.ps1 | 15 ++++++-- scripts/Test-AgentEvidenceCleanup.ps1 | 47 ++++++++++++++++++++++++ scripts/Test-AgentLocks.ps1 | 14 ++++++++ scripts/WorktreeCleanup.ps1 | 52 +++++++++++++++++++++++++++ 6 files changed, 155 insertions(+), 6 deletions(-) create mode 100644 scripts/Test-AgentEvidenceCleanup.ps1 diff --git a/.claude/skills/issue-pr-loop/SKILL.md b/.claude/skills/issue-pr-loop/SKILL.md index 1b0ac61..f84f0f3 100644 --- a/.claude/skills/issue-pr-loop/SKILL.md +++ b/.claude/skills/issue-pr-loop/SKILL.md @@ -123,6 +123,13 @@ $worktreeRoot = Join-Path (Split-Path $repo -Parent) ($repoName + '-worktrees') Verify the repository and intended base; examples use `origin/main`. Use a writable temporary `-worktrees` root if needed. Choose a unique worktree path when one already exists. +Write nothing loose beside the repository. Every file an agent produces outside a worktree has one home, so cleanup can find it: + +- Evidence (build and test logs, reports, PR and comment body drafts, diffs, validation projects) goes in `/-evidence//`, for example `C:/git/EnumerableAsyncProcessor-evidence/issue-123/build.log`. `Remove-MergedWorktrees.ps1` deletes an entry once its newest file is older than `-EvidenceRetentionDays` (default 7). +- Throwaway checkouts and baselines are worktrees under `$worktreeRoot`, never plain copies. +- Downloaded SDKs and CLI tools go in `C:/git/toolchains/-/`, shared by every repository. +- Never write to `C:/git` itself or to the main checkout. + Acquire the Redis lock before acting on an item: `pr-` for PR work or recovery, `issue-` for new implementation. Always invoke the absolute shared-checkout `$agentLocks` path for every verb; older branch copies can have incompatible token caches. | Command | Result | diff --git a/scripts/AgentLocks.ps1 b/scripts/AgentLocks.ps1 index f091f8e..388ef8b 100644 --- a/scripts/AgentLocks.ps1 +++ b/scripts/AgentLocks.ps1 @@ -52,7 +52,10 @@ # Lock-name resolution (renew/release/status): -LockName is OPTIONAL there. When # omitted it is resolved from, in order: (1) a marker persisted in the current # worktree's per-worktree git config (`agent.lockName`), which -# `acquire`/`renew -Worktree` writes after enabling `extensions.worktreeConfig`; +# `acquire`/`renew -Worktree` writes after enabling `extensions.worktreeConfig`, +# together with `agent.lockWorktree` (the path it was written for; a marker that +# `git worktree add` copied from another checkout is ignored; the main checkout +# never gets one); # then (2) an `issue--*` branch name -> `issue-`. `acquire` still REQUIRES an # explicit name (or an `issue--*` branch): it runs before the worktree exists, and # a PR lock's number (`pr-`) is not derivable from the branch (`issue--...`). @@ -101,16 +104,33 @@ function Get-WorktreeLockMarker { if ($LASTEXITCODE -ne 0 -or -not $enabled -or $enabled.Trim() -ne 'true') { return $null } $marker = (git config --worktree --get agent.lockName 2>$null) - if ($LASTEXITCODE -eq 0 -and $marker) { return $marker.Trim() } - return $null + if ($LASTEXITCODE -ne 0 -or -not $marker) { return $null } + # `git worktree add` copies the source checkout's config.worktree into the new + # worktree, so a marker counts only in the worktree it was written for. + $owner = (git config --worktree --get agent.lockWorktree 2>$null) + $top = (git rev-parse --show-toplevel 2>$null) + if (-not $owner -or -not $top -or (Get-MarkerPath $owner) -ne (Get-MarkerPath $top)) { return $null } + return $marker.Trim() } +function Get-MarkerPath([string]$Path) { return [IO.Path]::GetFullPath($Path.Trim()).TrimEnd('\', '/') } + function Set-WorktreeLockMarker([string]$Path) { # Linked worktrees share --local config. Enable the extension in that common # config, then persist the marker in the target worktree's config.worktree file. + # The main checkout is shared, never a work item's worktree, and new worktrees + # inherit its config.worktree, so it never gets a marker. + $gitDirectory = git -C $Path rev-parse --path-format=absolute --git-dir 2>$null + $commonDirectory = git -C $Path rev-parse --path-format=absolute --git-common-dir 2>$null + $top = git -C $Path rev-parse --show-toplevel 2>$null + if ($LASTEXITCODE -ne 0 -or -not $gitDirectory -or -not $top -or $gitDirectory -eq $commonDirectory) { + [Console]::Error.WriteLine("not writing lock marker: $Path is not a linked worktree") + return + } git -C $Path config --local extensions.worktreeConfig true 2>$null | Out-Null if ($LASTEXITCODE -ne 0) { return } git -C $Path config --worktree agent.lockName $LockName 2>$null | Out-Null + git -C $Path config --worktree agent.lockWorktree (Get-MarkerPath $top) 2>$null | Out-Null } function Resolve-OwnerIdentity([string]$Explicit) { diff --git a/scripts/Remove-MergedWorktrees.ps1 b/scripts/Remove-MergedWorktrees.ps1 index 1ba7620..3db8b91 100644 --- a/scripts/Remove-MergedWorktrees.ps1 +++ b/scripts/Remove-MergedWorktrees.ps1 @@ -33,6 +33,10 @@ # except remnants of a removal that WorktreeCleanup.ps1 recorded before a busy # directory interrupted it (Complete-PendingWorktreeRemovals). # +# AGENT EVIDENCE: +# Entries under
-evidence/ (one folder per work item) are removed once +# their newest write is older than -EvidenceRetentionDays (default 7). +# # Guards (never delete work): # - skip the main checkout and anything inside it (.claude/worktrees is harness-managed) # - skip locked worktrees (an agent session may still own them) @@ -44,7 +48,7 @@ # Run it once per loop iteration (cheap: ~3 gh calls in bulk, plus one association # call per unmatched leftover — a set that shrinks to near-zero after the first run). # -# Usage: pwsh scripts/Remove-MergedWorktrees.ps1 [-Repo owner/name] [-WhatIf] [-StaleDays n] +# Usage: pwsh scripts/Remove-MergedWorktrees.ps1 [-Repo owner/name] [-WhatIf] [-StaleDays n] [-EvidenceRetentionDays n] # Exit: 0 always (a sweep failure must not break the loop; problems are logged) [CmdletBinding()] @@ -54,7 +58,10 @@ param( # Opt-in: also remove CLEAN worktrees with no PR evidence at all (scratch / A-B # checkouts, local-only fix branches) whose HEAD commit is older than this many # days. 0 = off. Dirty worktrees are still preserved. - [int]$StaleDays = 0 + [int]$StaleDays = 0, + # Agent evidence entries whose newest file is older than this many days are removed. + [ValidateRange(1, 3650)] + [int]$EvidenceRetentionDays = 7 ) $ErrorActionPreference = 'Stop' @@ -232,8 +239,10 @@ try { # Removals that a busy directory interrupted, whatever started them. $pendingRemoved = Complete-PendingWorktreeRemovals -Repo $mainRepo -WhatIf:$WhatIf + $evidenceRemoved = Remove-StaleAgentEvidence -Root (Get-AgentEvidenceRoot -MainCheckout $mainRepo) -RetentionDays $EvidenceRetentionDays -WhatIf:$WhatIf + git -C $mainRepo worktree prune - Write-Host "sweep: removed $removed merged worktree(s), $orphansRemoved orphaned dir(s), $pendingRemoved pending removal(s)." + Write-Host "sweep: removed $removed merged worktree(s), $orphansRemoved orphaned dir(s), $pendingRemoved pending removal(s), $evidenceRemoved stale evidence entr(ies)." } catch { Warn "unexpected sweep error (ignored, loop continues): $_" diff --git a/scripts/Test-AgentEvidenceCleanup.ps1 b/scripts/Test-AgentEvidenceCleanup.ps1 new file mode 100644 index 0000000..51f3562 --- /dev/null +++ b/scripts/Test-AgentEvidenceCleanup.ps1 @@ -0,0 +1,47 @@ +# Regression tests for the agent evidence sweep in WorktreeCleanup.ps1. Needs only PowerShell 7. +$ErrorActionPreference = 'Stop' +. (Join-Path $PSScriptRoot 'WorktreeCleanup.ps1') +$tempBase = [IO.Path]::GetFullPath([IO.Path]::GetTempPath()) +$testRoot = [IO.Path]::GetFullPath((Join-Path $tempBase "evidence-cleanup-$([Guid]::NewGuid().ToString('N'))")) +if (-not $testRoot.StartsWith($tempBase, [StringComparison]::OrdinalIgnoreCase)) { throw 'Unsafe fixture path.' } + +function Assert-True([bool]$Condition, [string]$Message) { + if (-not $Condition) { throw $Message } +} + +function New-Evidence([string]$Path, [int]$AgeDays) { + New-Item -ItemType Directory -Path (Split-Path $Path -Parent) -Force | Out-Null + Set-Content -LiteralPath $Path -Value 'evidence' + (Get-Item -LiteralPath $Path).LastWriteTimeUtc = [DateTime]::UtcNow.AddDays(-$AgeDays) +} + +try { + $root = Get-AgentEvidenceRoot -MainCheckout (Join-Path $testRoot 'Repo') + Assert-True ($root -ceq (($testRoot -replace '\\', '/') + '/Repo-evidence')) "Unexpected evidence root: $root" + + New-Evidence (Join-Path $root 'issue-1/build.log') 10 + New-Evidence (Join-Path $root 'issue-2/old.log') 10 + New-Evidence (Join-Path $root 'issue-2/nested/fresh.log') 1 + New-Evidence (Join-Path $root 'pr-3-body.md') 10 + New-Evidence (Join-Path $root 'pr-4-body.md') 1 + foreach ($directory in Get-ChildItem -LiteralPath $root -Directory -Recurse) { + $directory.LastWriteTimeUtc = [DateTime]::UtcNow.AddDays(-10) + } + + $wouldRemove = Remove-StaleAgentEvidence -Root $root -RetentionDays 7 -WhatIf + Assert-True ($wouldRemove -eq 2) "Dry run counted $wouldRemove stale entries." + Assert-True (Test-Path -LiteralPath (Join-Path $root 'issue-1')) 'Dry run removed evidence.' + + $removed = Remove-StaleAgentEvidence -Root $root -RetentionDays 7 + Assert-True ($removed -eq 2) "Removed $removed stale entries." + Assert-True (-not (Test-Path -LiteralPath (Join-Path $root 'issue-1'))) 'Stale evidence folder survived.' + Assert-True (-not (Test-Path -LiteralPath (Join-Path $root 'pr-3-body.md'))) 'Stale evidence file survived.' + Assert-True (Test-Path -LiteralPath (Join-Path $root 'issue-2/old.log')) 'Folder with recent writes was removed.' + Assert-True (Test-Path -LiteralPath (Join-Path $root 'pr-4-body.md')) 'Recent evidence file was removed.' + + Assert-True ((Remove-StaleAgentEvidence -Root (Join-Path $testRoot 'missing') -RetentionDays 7) -eq 0) 'Missing root reported removals.' + Write-Host 'OK agent evidence cleanup tests passed.' +} +finally { + if (Test-Path -LiteralPath $testRoot) { Remove-Item -LiteralPath $testRoot -Recurse -Force } +} diff --git a/scripts/Test-AgentLocks.ps1 b/scripts/Test-AgentLocks.ps1 index 82297ce..450a1fd 100644 --- a/scripts/Test-AgentLocks.ps1 +++ b/scripts/Test-AgentLocks.ps1 @@ -11,6 +11,7 @@ $repo = Join-Path $testRoot 'repo' $issueWorktree = Join-Path $testRoot 'issue' $worktreeA = Join-Path $testRoot 'marker-a' $worktreeB = Join-Path $testRoot 'marker-b' +$worktreeInherited = Join-Path $testRoot 'marker-inherited' $issueLock = "issue-$issueNumber" $lockA = "pr-agentlocks-a-$suffix" @@ -126,6 +127,19 @@ try { Assert-Status $repo @('status', '-LockName', $lockB) 'HELD-BY-ME' 'marker B isolated from marker A release' Assert-ExitZero -Result (Invoke-AgentLocks $worktreeB @('release')) -Operation 'implicit marker B release' + # `git worktree add` copies the source's config.worktree; the copy names A's path. + Assert-ExitZero -Result (Invoke-AgentLocks $repo @('acquire', '-LockName', $lockA, '-Worktree', $worktreeA)) -Operation 'reacquire marker A' + Invoke-Git -C $worktreeA worktree add -b marker-inherited $worktreeInherited HEAD + $inherited = Invoke-AgentLocks $worktreeInherited @('status') + if ($inherited.ExitCode -eq 0) { throw "inherited marker resolved a lock: '$($inherited.Output)'" } + Assert-ExitZero -Result (Invoke-AgentLocks $repo @('release', '-LockName', $lockA)) -Operation 'release marker A after inheritance check' + + # The main checkout never receives a marker for new worktrees to inherit. + Assert-ExitZero -Result (Invoke-AgentLocks $repo @('acquire', '-LockName', $explicitLock, '-Worktree', $repo)) -Operation 'acquire with main checkout' + $mainMarker = git -C $repo config --worktree --get agent.lockName 2>$null + if ($mainMarker) { throw "main checkout received lock marker '$mainMarker'" } + Assert-ExitZero -Result (Invoke-AgentLocks $repo @('release', '-LockName', $explicitLock)) -Operation 'release main checkout lock' + # Explicit names always override any marker in the current worktree. Assert-ExitZero -Result (Invoke-AgentLocks $repo @('acquire', '-LockName', $explicitLock)) -Operation 'acquire explicit lock' Assert-Status $worktreeA @('status', '-LockName', $explicitLock) 'HELD-BY-ME' 'explicit status override' diff --git a/scripts/WorktreeCleanup.ps1 b/scripts/WorktreeCleanup.ps1 index 85909bf..2726e9c 100644 --- a/scripts/WorktreeCleanup.ps1 +++ b/scripts/WorktreeCleanup.ps1 @@ -99,6 +99,58 @@ function Remove-WorktreeDirectory { return -not (Test-Path -LiteralPath $Worktree) } +# AGENT EVIDENCE +# Agents keep logs, reports and PR-body drafts for a work item under +#
-evidence//, beside the worktree root, so nothing lands +# loose in the parent directory. Evidence matters only while its item is active. +function Get-AgentEvidenceRoot { + [CmdletBinding()] + param([Parameter(Mandatory)][string]$MainCheckout) + + return "$(($MainCheckout -replace '\\', '/').TrimEnd('/'))-evidence" +} + +# Removes each top-level entry of the evidence root whose newest write is older than +# the retention window. Returns the number of entries removed (or that would be). +function Remove-StaleAgentEvidence { + [CmdletBinding()] + param( + [Parameter(Mandatory)][string]$Root, + [Parameter(Mandatory)][int]$RetentionDays, + [switch]$WhatIf + ) + + if (-not (Test-Path -LiteralPath $Root -PathType Container)) { return 0 } + $cutoff = [DateTime]::UtcNow.AddDays(-$RetentionDays) + $removed = 0 + foreach ($entry in (Get-ChildItem -LiteralPath $Root -Force -ErrorAction SilentlyContinue)) { + if (($entry.Attributes -band [IO.FileAttributes]::ReparsePoint) -ne 0) { + Write-Host "sweep: skipping evidence entry that is a link: $($entry.FullName)" + continue + } + $items = @($entry) + if ($entry.PSIsContainer) { + $items += @(Get-ChildItem -LiteralPath $entry.FullName -Recurse -Force -ErrorAction SilentlyContinue) + } + $newest = ($items | Measure-Object -Property LastWriteTimeUtc -Maximum).Maximum + if ($newest -ge $cutoff) { continue } + if ($WhatIf) { + Write-Host "sweep: WOULD remove stale evidence $($entry.FullName)" + $removed++ + continue + } + if ($entry.PSIsContainer -and -not (Remove-WorktreeDirectoryLinks -Worktree $entry.FullName -Label '(evidence)')) { continue } + Remove-Item -LiteralPath $entry.FullName -Recurse -Force -ErrorAction SilentlyContinue + if (Test-Path -LiteralPath $entry.FullName) { + Write-Host "sweep: WARNING could not remove stale evidence $($entry.FullName)" + continue + } + Write-Host "sweep: removed stale evidence $($entry.FullName)" + $removed++ + } + return $removed +} + # PENDING REMOVALS # On Windows a directory cannot be deleted while any process has its current directory # inside it, for example a shell that ran Merge-Pr.ps1 from the worktree, and build or