spec-kitty / spec-kitty/spec-kitty

merge-retention: forecast surface unification + resolver wording (non-blocking follow-ups from #3826 squad)

Closed
#3,833 1 comment 0 reactions 0 assignees View on GitHub
priority:P3 reliability stage:implemented status:claimed status:ready tech-debt
Dominant language
Python
Stars
1.6k
Forks
165
Avg merge
14h 52m
Merged PRs (30d)
303

Description

## Summary

Non-blocking follow-ups from the **#3826** (`merge-honor-retention-policy`, #3131) landing second-opinion squad. Both lenses (fail-closed correctness + whack-a-field teardown enumeration) cleared #3826 to **ship, no MAJOR** — every mission-owned, merge-reachable branch/worktree deletion routes through the single `resolve_merge_retention` authority. These are residual tidy-ups, none data-loss.

## Items

1. **Dry-run forecast reads retention off a different placement surface than the executor.** The three live teardown paths resolve the meta dir via `PRIMARY_METADATA`; `merge/forecast.py` resolves `feature_dir_for_preview` via `MissionArtifactKind.WORK_PACKAGE_TASK` (deliberately — the review-artifact consistency gate needs that surface) and then reads the **retention** decision off the same dir (`forecast.py:~231`). Both resolve to `kitty-specs//` for every topology **today**, so the preview matches the real merge. But it's a second, independent derivation of "where `meta.json` lives" — if the `WORK_PACKAGE_TASK` home ever diverges from `PRIMARY_METADATA` for some topology, the dry-run would show "will delete" while the real merge keeps (misleading preview; never data loss, since dry-run deletes nothing). Resolve retention off a dedicated `PRIMARY_METADATA` dir, and add a test asserting dry-run retention == executor retention across topologies. Note: the inline comment at `forecast.py:~228` mislabels `feature_dir_for_preview` as "already the PRIMARY meta dir" — correct it when unifying (it's the `WORK_PACKAGE_TASK` surface).

2. **`resolve_merge_retention` is described as "pure" but performs I/O.** It reads `meta.json` via `read_retention_from_meta` → `load_meta_fail_closed`. This matches the established `resolve_merge_target_branch` seam (consistent, not a defect), but the "pure resolver" wording (commit messages / docstring) overstates it. Soften to "resolver" so a future reader doesn't assume it's I/O-free.

3. **`spec-kitty mission-type delete` teardown is retention-ungated (future-scope note).** `cli/commands/mission_type.py:~900,~950` force-delete lane branches/worktrees with no retention consult. This is correct **today** — it's the explicit destroy command where deletion is the operator's intent, and it is **not** reachable from `merge`, so it is out of #3131's remit. Flagged only so any future expansion of retention scope beyond `merge` considers it.

## Provenance

Surfaced by the #3826 landing squad (reviewer-renata fail-closed lens; paula-patterns teardown-site enumeration). Refs #3826, #3131. Non-blocking; suggest `priority:P3` / `tech-debt`.

Separately noted (pre-existing, already tracked, **not** for this issue): the orchestrator-api `merge-mission` coord teardown deletes the mission/coord branch but leaves the coord marker/worktree — a retain-more asymmetry (stranded-husk class, #3086-adjacent), not data loss.

Contributor guide

Open the contributing guide

Research direction

Start in merge/forecast.py around lines 228-231 and trace the three live teardown paths that use PRIMARY_METADATA, then inspect resolve_merge_retention and its read_retention_from_meta/load_meta_fail_closed path. Add coverage that dry-run and executor retention agree across topologies, correct the misleading comment, and soften "pure" to "resolver"; leave the mission_type.py force-delete lane as future scope.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, python
Domain
cli, tooling
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
66/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.