Skip to content

cleanup sessions: squash-merged work is classified as "park", so the plan opens duplicate draft PRs - #799

Merged
CSalcedoDataBI merged 12 commits into
nextfrom
issue-755-cleanup-sessions-squash-merged-work-is
Oct 24, 2026
Merged

CSalcedoDataBI merged 12 commits into
nextfrom
issue-755-cleanup-sessions-squash-merged-work-is

Conversation

@CSalcedoDataBI

Copy link
Copy Markdown
Owner

Closes #755

/cleanup sessions planned `park` for branches whose work had reached the
default branch through a different, squash-merged PR, so -Force would
open duplicate draft PRs (4 of 5 park rows in the 2026-09-29 repro).

- Containment check: `git merge-tree --write-tree origin/<default> <branch>`;
  when the merged tree equals the default tree the branch adds nothing ->
  new `delete-contained` action, re-proven right before `branch -D`.
- Superseded hint -> `needs-decision` naming the PR, never a deletion:
  its own closed PR says "superseded by #N" (or the PR URL), or the merge
  conflicts and every referenced issue/PR was done by a merged PR.
- Any git/gh failure falls back to the old disposition (park).

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

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review: sin bloqueantes. La lógica es sólida: el borrado solo ocurre con prueba exacta (merge-tree igual al árbol de main), se vuelve a verificar justo antes de branch -D, y cualquier fallo de git/gh degrada a park (nunca borra). Los casos con sesión abierta, worktree sucio o PR propio siguen mandando.

Observaciones menores (no bloquean):

  • Cobertura: no hay test del camino de ejecución (-Force) de delete-contained: ni el re-chequeo que falla con Set-Failed cuando la rama ya no está contenida, ni el caso de rama checked-out en la carpeta principal. Un test de integración con repo temporal (como el de merge-tree) lo cubriría.
  • Semántica: si el trabajo se revirtió a propósito en main, el árbol también coincide y la rama se borra como "contained". Es aceptable (el commit sigue en el reflog/historial), pero vale la pena mencionarlo en verbs-sessions.md.
  • Robustez: Get-BranchContainment / git log reciben $Branch sin --; una rama con nombre que empiece por - se interpretaría como opción (improbable, pero barato de blindar).
  • El hint "superseded by" acepta comentarios de cualquier autor; como solo produce needs-decision, el riesgo es nulo.

…ides wip-park (#755)

Review round 1 (claude-review):
- merge-tree/rev-parse now use refs/remotes/origin/<default> and
  refs/heads/<branch>, so a local "origin/main" branch or a same-named tag
  cannot make the proof be about another commit than `branch -D` deletes.
- The superseded hint is ignored over uncommitted changes (wip-park saves
  them) and for merged-advanced branches (post-merge commits are new work).

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

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review (#799 / #755): no encontré bugs bloqueantes. El diseño es conservador: la prueba exacta (merge-tree --write-tree) borra, la heurística solo pregunta, y cualquier fallo de git/gh cae a park. La re-verificación antes de branch -D y los refs calificados están bien resueltos.

Observaciones menores (no bloquean):

  • Cobertura: los tests cubren las funciones puras y un repo real para Get-SweepContainment, pero no Get-BranchContainment (refs refs/remotes/origin/<default>) ni el re-chequeo en el camino -Force. Un test de integración con un origin local lo cubriría.
  • Regex de "superseded by": también coincide con frases como "not superseded by #N". Es solo una pista que acaba en needs-decision, riesgo bajo; podrías anclarlo al inicio de frase.
  • origin/<default> puede estar desactualizado sin fetch. Es seguro (solo genera menos contained), pero conviene mencionarlo en la doc.

…fs (#755)

Review round 2 (claude-review):
- merge-tree honours merge drivers (merge=ours keeps main's side), so a
  "contained" answer is now also proven with a plain diff: no path the
  branch changed since the merge base may differ from origin's default.
- A missing branch ref made merge-tree exit 1, read as "conflict"; the
  ref is verified first and that case is "unknown".
- The superseded hint reads commit subjects with qualified refs and
  gives no hint when git log fails.
- Get-BranchContainment moves above the dot-source guard, so the tests
  run it on real repos (squash-merge, local "origin/main" branch,
  merge=ours driver, missing ref).

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

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Sin hallazgos bloqueantes. La prueba de contención (merge-tree más diff de rutas, con refs completos y re-verificación antes de branch -D) falla hacia park/unknown, nunca hacia borrar. Los tests cubren los casos de riesgo: merge driver ours, rama llamada origin/main, dirty y merged-advanced.

Nit opcional: el regex de superseded by coincide también con comentarios como "not superseded by #N". Es solo un hint que termina en needs-decision, así que es de bajo impacto.

CSalcedoDataBI and others added 2 commits October 23, 2026 18:11
…ross-repo refs (#755)

Review round 3 (claude-review):
- The plain-diff proof downgraded the usual case (main edited a file
  after the squash) to "adds", so it was parked again. It now applies
  only to paths with a custom merge driver (git check-attr merge, from
  the working tree and both sides' trees).
- "owner/repo#12", "C#12" and another repo's PR URL are no longer read
  as this repo's numbers in the superseded hint.
- check-attr gets paths as argument batches: piped into git on Windows
  every path gained a trailing CR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Find-FilesPruned emitted entries in file-system order: NTFS returns them
sorted, ext4 in hash order. Find-AgentDefinition ("first candidate in
root order") and Get-SkillInventory (overlap pair order) therefore gave
different answers on ubuntu, which failed Pester (ubuntu) on PR #799.
Each directory's files and subdirectories are now sorted ordinally,
case-insensitively, before they are used.

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

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review (34b2100, Find-FilesPruned.ps1)

El fix es correcto y acotado: ordenar cada listado hace determinista el "first found", y el orden pre-order se conserva. Sin bugs bloqueantes. Dos observaciones menores:

  1. Desempate en FS case-sensitive (ext4): OrdinalIgnoreCase trata Agent.md y agent.md como iguales, y List.Sort es inestable, así que ese par puede salir en orden distinto entre ejecuciones; justo el no-determinismo que se quiere eliminar. Sugerencia: comparar con OrdinalIgnoreCase y, si da 0, desempatar con [string]::CompareOrdinal.
  2. Tests: no se añade ningún test que fije el orden. Un caso Pester que cree archivos/subdirectorios en orden no alfabético (b, a, C) y afirme el orden devuelto evitaría regresiones y valida el fix en ubuntu.

Nota: en el timeout parcial el listado en curso se ordena y se añade igual; es inocuo, solo señalo que el resultado parcial incluye esos archivos.

… proof (#755)

Review round 4 (claude-review):
- git output for the proof is read as UTF-8 with -z (NUL-split): no
  C-quoting, no console code page, no CR, so a non-ASCII or quoted name
  of a merge-driven file cannot make the proof match nothing.
- Each merge-driven path is compared blob by blob; a path neither side
  has is "unknown", never "nothing differs".
- check-attr batches are bounded by length; a git that fails to start
  is exit -1 instead of the previous call's stale 0.
- Documents the git 2.40 floor (check-attr --source); older git parks.

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

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Sin hallazgos bloqueantes. Revisé el diff en Cleanup-Sessions.ps1, Find-FilesPruned.ps1, la documentación y los tests, pero no ejecuté los tests ni el script.

  • Borrado seguro: delete-contained exige merge-tree limpio con el árbol igual al de main. Además se vuelve a probar justo antes de branch -D, se excluyen las ramas con cambios sin commitear o con sesión abierta, y cualquier fallo de git o gh cae a park/unknown.
  • Merge drivers: el caso merge=ours está cubierto con la comparación blob a blob.
  • Sugerencia menor (no bloqueante): la re-prueba antes de borrar usa refs/remotes/origin/<default> tal como estaba al planificar, sin git fetch. Es seguro porque solo puede dar falsos negativos, pero conviene dejarlo documentado.
  • Tests: la decisión pura (Get-SweepItemAction, Get-SweepContainment, Get-SweepSupersededHint) tiene buena cobertura.

…Item

The order test took Get-ChildItem -Recurse as its oracle, which on ext4
is hash order - the nondeterminism the walker now removes. The expected
order is written out; on Windows it is still checked against
Get-ChildItem, which returns the same sorted pre-order there.

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

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Revisión (sha ea40cd9): sin bloqueantes. La lógica es conservadora: borra solo con prueba exacta (merge-tree con árbol idéntico), repite la prueba justo antes de branch -D, y cualquier fallo de git/gh degrada a park/needs-decision. Los tests cubren bien la tabla de decisión y hay un caso con repo real. Revisé el diff hasta donde se pudo leer (el resto de los tests quedó truncado).

Observaciones menores, no bloqueantes:

  1. Ref origin/<default> posiblemente desactualizada: la prueba usa refs/remotes/origin/* sin git fetch. Una ref vieja da falsos adds/conflict (seguro: park), nunca un falso contained. Aun así conviene documentar que se asume un fetch previo, o hacerlo antes de medir.
  2. "Contained" incluye trabajo revertido en main: si el contenido se añadió y luego se revirtió en main, el árbol no cambia y la rama se borra. Es coherente con la definición, pero conviene una línea en verbs-sessions.md.
  3. Límite de 8000 caracteres por lote en check-attr: si un solo path supera ese límite, el lote queda con un único elemento. Raro, pero vale un test con paths largos.
  4. Find-FilesPruned: el orden OrdinalIgnoreCase ya no coincide con el de Get-ChildItem en NTFS para casos límite (símbolos entre mayúsculas y minúsculas). Es cosmético; el comentario del código ya fija el contrato (determinista entre SO).

CSalcedoDataBI and others added 2 commits October 23, 2026 18:55
…nt (#755)

Review round 5 (claude-review):
- merge.default naming a driver sends every unspecified path through it,
  so those paths now get the blob-by-blob proof too.
- Only the -z terminator is dropped: an empty value (`merge=`) inside
  check-attr output no longer shifts the triplets after it.

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

Review round 5 (claude-review): OrdinalIgnoreCase ranks names that
differ only by case as equal, and List.Sort is unstable, so on a
case-sensitive file system their order still followed hash order.

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

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review del commit 2beb9bd (desempate case-sensitive en Find-FilesPruned.ps1).

La lógica es correcta: OrdinalIgnoreCase primero y Ordinal como desempate da un orden total, así que el List.Sort inestable ya no depende del orden hash en ext4. Se aplica igual a archivos y subdirectorios. No veo problemas de seguridad.

Un solo punto, menor: el commit no añade ningún test. Convendría uno en ExpertRoleResolution.Tests.ps1 (o uno específico del walker) que cree README.md y readme.md en el mismo directorio y compruebe el orden. Hay que saltarlo en file systems case-insensitive (Windows/macOS), donde no se pueden crear los dos nombres a la vez. Sin ese test, la regresión que corrige este commit puede volver sin que nadie lo note.

Review round 6 (claude-review): a scriptblock comparator re-enters
PowerShell on every comparison, an unbounded cost after the deadline
checks on a wide directory. Each listing now sorts precomputed keys
(upper-invariant name, NUL, name) with a native ordinal sort - the same
order.

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

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Sin hallazgos bloqueantes en 4f23ee3.

Revisé Cleanup-Sessions.ps1 (delete-contained y la pista de 'superseded'), Find-FilesPruned.ps1 (orden determinista) y los tests. No ejecuté los tests.

  • Borrado seguro: la prueba de contención se repite justo antes de branch -D. Los refs van completamente calificados. Los drivers de merge (merge=ours, merge.default) se tratan como 'unknown' o 'adds', y cualquier fallo de git o gh vuelve a park, nunca a borrar.
  • Orden de listados: Sort-PrunedListing usa una clave por nombre y un sort ordinal nativo. El desempate sensible a mayúsculas es estable y el orden de pre-orden se mantiene.
  • Nit (opcional): la pista superseded by #N se busca en cualquier comentario del PR cerrado, así que una cita de esa frase también la dispara. Es solo una pista que acaba en needs-decision, no en borrado, así que el riesgo es bajo.

🤖 Generated with Claude Code

Review round 6 (claude-review): [int] on "#12345678901" throws, and
under ErrorActionPreference Stop that ended the whole sweep. Numbers
that do not fit are skipped.

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

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Review (Cleanup-Sessions.ps1, #755)

Sin bloqueantes. Lo revisado está bien resuelto:

  • La prueba de contención con merge-tree --write-tree compara el árbol resultante con el de la default. Usa refs completas y vuelve a probar justo antes de branch -D.
  • Los merge drivers personalizados y merge.default no pueden falsear la contención.
  • Las referencias #NNN se leen con TryParse.
  • El hint de "superseded" solo produce needs-decision, nunca un borrado.
  • Hay tests nuevos que cubren las funciones puras y un repo real.

Menores (no bloquean):

  1. Get-BranchContainment documenta "git 2.40+" por check-attr --source, mientras que Get-SweepContainment menciona 2.38 (merge-tree --write-tree). Unifica el comentario para que el requisito real sea claro. En git más viejo cae a unknown, que es seguro.
  2. Get-BranchSupersededHint hace catch { return '' } sin dejar rastro. Es seguro porque cae a park, pero un Write-Verbose ayudaría a depurar fallos de gh.

No revisé por completo los cambios en Find-FilesPruned.ps1 ni en los tests de ExpertRoleResolution; el diff de esos dos es pequeño y no vi nada raro.

Two PowerShell traps made the round-6 sort a no-op, hidden on Windows
because NTFS already lists names in sorted order:
- binding to [object[]] handed the function a copy, so an in-place sort
  was lost - the sorted array is now returned;
- without explicit casts PowerShell bound an Array.Sort overload that
  sorted only the keys.
A unit test calls Sort-PrunedListing directly, so a sort that does
nothing fails on every OS, not only on ext4.

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

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Sin hallazgos bloqueantes. Revisé Cleanup-Sessions.ps1 y Find-FilesPruned.ps1 (lectura estática; no ejecuté los tests de Pester).

  • delete-contained: la prueba con merge-tree --write-tree está bien resuelta. Usa refs completos, verifica los merge drivers y vuelve a probar la contención justo antes de branch -D. Ante la duda cae a unknown y se mantiene el comportamiento anterior (park).
  • Hint "superseded": solo produce needs-decision, nunca borra. Es la decisión correcta para una heurística.
  • Sort-PrunedListing: devolver el array y fijar la sobrecarga con los casts corrige el no-op anterior. Hay test directo.
  • Sugerencia menor: en Invoke-SweepGit -Z, $out -join "n"asume que PowerShell partió la salida solo por\n. Un path con salto de línea o con \r` en Windows podría alterarse. Es un caso muy improbable y falla hacia 'unknown'/'adds' (seguro), así que no bloquea.

@CSalcedoDataBI

Copy link
Copy Markdown
Owner Author

External review - claude-review (Claude API, clean context)

Reviewed commit: ef36ef88dd6de67a53372baec11daf0df6c9f877

8 rounds at --effort xhigh. Rounds 1-6 found 14 real defects, all fixed with tests: unqualified refs in the deletion proof; hint overriding wip-park and merged-advanced; merge drivers (attributes and merge.default) faking containment; byte-exact -z paths; missing ref read as conflict; stale exit code on launch failure; cross-repo refs; int overflow; walker sort (Linux order, case ties, no-op sort). Rounds 7 and 8 returned nothing. Out of scope, reported: archive verdict matches only exact worktree paths (pre-existing #734 code).

@CSalcedoDataBI

Copy link
Copy Markdown
Owner Author

On hold: waiting for the Anthropic plugin directory review. Do not merge yet.

The agentic-board plugin is in its first review in the Anthropic plugin directory, and the directory tracks main. Every merge to main becomes a new version that replaces the one under review and restarts the queue. This PR stays on its branch until the plugin leaves review, or until the owner decides to merge the whole batch at once.

Batch waiting together: #799, #800, #801, #802, #803, #804.

@CSalcedoDataBI
CSalcedoDataBI changed the base branch from main to next October 24, 2026 13:14
@CSalcedoDataBI
CSalcedoDataBI merged commit 019c701 into next Oct 24, 2026
5 checks passed
@CSalcedoDataBI
CSalcedoDataBI deleted the issue-755-cleanup-sessions-squash-merged-work-is branch October 24, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on-hold-directory Waiting for the Anthropic plugin directory review before merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cleanup sessions: squash-merged work is classified as "park", so the plan opens duplicate draft PRs

1 participant