258 lines
13 KiB
Markdown
258 lines
13 KiB
Markdown
|
|
# Design: Clean
|
|||
|
|
|
|||
|
|
## Context
|
|||
|
|
|
|||
|
|
The check pipeline (build step #3) is in place: `scanner.py` + `report_builder.py` +
|
|||
|
|
`validate_report.py` + `state_store.py` together produce a schema-valid
|
|||
|
|
`.dochygiene/report.json` and `report.md`, then stamp `last_check`. The applier is
|
|||
|
|
also in place: `patch_applier.py` exists, has 263 passing tests, and knows how to
|
|||
|
|
apply every kind in the `KIND_TABLE` via per-file transactions with a content-hash
|
|||
|
|
guard. What is missing is the orchestration skill that loads that report, gates
|
|||
|
|
dangerous entries, invokes the applier, handles generative entries, and produces a
|
|||
|
|
single reviewable git commit.
|
|||
|
|
|
|||
|
|
Constraints that shape this design:
|
|||
|
|
|
|||
|
|
- **Invariant #4 (no accumulating artifacts):** keep only the latest report pair.
|
|||
|
|
The clean skill must not write new state files; `last_clean` is the only new state
|
|||
|
|
entry.
|
|||
|
|
- **Invariant #5 (git-safe single commit):** clean/committed tree required or an auto
|
|||
|
|
WIP checkpoint; the cleanup output is exactly one commit. NEVER two commits.
|
|||
|
|
- **Invariant #6 (deterministic-first):** apply, stage, and commit are scripts.
|
|||
|
|
Generative distillation is the only model pass during clean.
|
|||
|
|
- **Invariant #7 (safety-tier gating):** `auto` ops apply without prompt; `confirm`
|
|||
|
|
ops escalate. Gating does NOT relax under `sweep`.
|
|||
|
|
- **Invariant #8 (content guard):** the applier reads `expected_sha256` from the
|
|||
|
|
report; if a file changed since check, the entire file batch is skipped (not just
|
|||
|
|
the conflicting entry). The guard is a content hash, not mtime — but per the
|
|||
|
|
META-RULE, invariant #8's wording ("mtime guard") must not change without human
|
|||
|
|
approval.
|
|||
|
|
|
|||
|
|
## Goals / Non-Goals
|
|||
|
|
|
|||
|
|
**Goals:**
|
|||
|
|
|
|||
|
|
- A `hygiene-clean` skill that loads the report, gates confirms, runs the applier,
|
|||
|
|
handles generative entries via Sonnet, stages precisely, commits once, and stamps
|
|||
|
|
`last_clean`.
|
|||
|
|
- A `workflows/distill.md` Sonnet workflow for generative entries (LOOP-GUARD via
|
|||
|
|
subagent pointer).
|
|||
|
|
- Live routing in `commands/hygiene.md` for `clean` and `sweep` (replacing Phase 3
|
|||
|
|
stubs).
|
|||
|
|
- Integration tests covering the invariant #5 and #7 GAPs identified in the design.
|
|||
|
|
|
|||
|
|
**Non-Goals:**
|
|||
|
|
|
|||
|
|
- `token_estimate` weighting (`injection_frequency`, `weighted_tokens`, rollup) — v2
|
|||
|
|
bonus (PRD phase 5).
|
|||
|
|
- Inbound-link rewriting after `move-to-archive` — v1 move only; next check flags new
|
|||
|
|
orphans.
|
|||
|
|
- The live model-classification regression harness — separate, manually invoked.
|
|||
|
|
|
|||
|
|
## Decisions
|
|||
|
|
|
|||
|
|
### D1. Per-file transaction with descending-anchor application — the content-hash guard lives in `patch_applier.py`
|
|||
|
|
|
|||
|
|
The core correctness invariant: applying edits one-at-a-time to a file is wrong
|
|||
|
|
because each write shifts line numbers, causing subsequent anchors to land on the
|
|||
|
|
wrong lines and potentially tripping the guard on stale content. The resolution:
|
|||
|
|
|
|||
|
|
1. **Read file bytes once.**
|
|||
|
|
2. **Verify `sha256(bytes) == expected_sha256`** for every anchored entry on that
|
|||
|
|
file. On any mismatch, skip the **entire file batch** (reason:
|
|||
|
|
`content-changed-since-check`, recommendation: re-analysis) and continue other
|
|||
|
|
files.
|
|||
|
|
3. **Apply anchored edits in memory, descending by `anchor.start_line`** — edits to
|
|||
|
|
later lines first so earlier-line anchors remain valid.
|
|||
|
|
4. **Apply `insert-frontmatter` last** (it prepends, shifting all lines; doing it
|
|||
|
|
last means descending-anchor edits see the correct line numbers).
|
|||
|
|
5. **Write once; stage once.**
|
|||
|
|
|
|||
|
|
The guard is a content hash only — no mtime pre-filter. Per-entry `generated_at` in
|
|||
|
|
the report records the file's hash instant (distinct from the envelope/`last_check`
|
|||
|
|
stamp); the applier does not use `generated_at`, only `expected_sha256`.
|
|||
|
|
|
|||
|
|
**Alternative considered:** apply edits sequentially with line-offset tracking — rejected:
|
|||
|
|
complex, error-prone across multiple edit kinds, and untestable in isolation.
|
|||
|
|
|
|||
|
|
### D2. `insert-frontmatter`: re-derive at apply time (resolves the deferred hole from `add-check`)
|
|||
|
|
|
|||
|
|
The `add-check` design recorded a deferred hole: `insert-frontmatter` has
|
|||
|
|
`has_anchor = False` and therefore carries **no** `expected_sha256`, so invariant #8
|
|||
|
|
cannot protect it at apply time via the normal content-hash path.
|
|||
|
|
|
|||
|
|
Resolution (implemented in `patch_applier.py`): at apply time, re-read the file and
|
|||
|
|
parse its frontmatter fresh:
|
|||
|
|
- Key already present with the target value → **idempotent no-op** (success).
|
|||
|
|
- Key already present with a different value → **skip** with reason
|
|||
|
|
`frontmatter-key-conflict`; NEVER overwrite.
|
|||
|
|
- Key absent → insert `key: value` (creating a `---` block if needed) as the
|
|||
|
|
**last** in-memory step (after all descending-anchor edits).
|
|||
|
|
|
|||
|
|
This re-derivation replaces the missing hash guard for frontmatter; it is
|
|||
|
|
idempotent and does not trust any cached state. This resolves the deferred hole
|
|||
|
|
noted in `add-check` and in `CLAUDE.md`.
|
|||
|
|
|
|||
|
|
### D3. Incompatible-ops detection — skip the file, never compose
|
|||
|
|
|
|||
|
|
Per file, before any in-memory editing, the applier checks:
|
|||
|
|
- Does `move-to-archive` co-occur with any other op on the same file?
|
|||
|
|
- Do any anchor ranges overlap?
|
|||
|
|
|
|||
|
|
If either is true, skip the entire file with reason `incompatible-ops-on-file` and
|
|||
|
|
record a recommendation for re-analysis. V1 never attempts to compose or sequence
|
|||
|
|
these — the interaction is undefined and the risk of silent corruption is too high.
|
|||
|
|
|
|||
|
|
**Alternative considered:** attempt partial application (apply non-conflicting ops) —
|
|||
|
|
rejected: partial application of overlapping anchors produces unpredictable results
|
|||
|
|
and leaves the file in a state that cannot be re-analyzed cleanly.
|
|||
|
|
|
|||
|
|
### D4. Safety-tier gating — one batch-confirm before any mutation
|
|||
|
|
|
|||
|
|
Invariant #7 requires `auto` ops to apply without prompt and `confirm` ops to
|
|||
|
|
escalate. The skill reads `safety_tier` from the report (derived by `report_builder`
|
|||
|
|
via `derive_safety_tier`) — never recomputes or trusts a model.
|
|||
|
|
|
|||
|
|
Gating sequence:
|
|||
|
|
1. Partition entries into `auto+deterministic` → mechanical; `confirm+deterministic`
|
|||
|
|
(i.e., `delete-range`) → approval; `generative` (always `confirm`) → approval.
|
|||
|
|
2. **Gate confirm-tier FIRST**, before any mutation: present a single batch-confirm
|
|||
|
|
list (path · category · op · tokens · why) with per-entry opt-out. Visually
|
|||
|
|
distinguish `delete-range` (irreversible) from reversible `auto` ops.
|
|||
|
|
3. Approved set = all `auto` entries + approved `confirm` entries.
|
|||
|
|
4. Apply in that order.
|
|||
|
|
|
|||
|
|
Under `sweep` (`/hygiene sweep` = check-then-clean), the confirm gate runs
|
|||
|
|
identically — the convenience path does NOT bypass invariant #7.
|
|||
|
|
|
|||
|
|
Audit of approvals is folded into the commit body (no separate
|
|||
|
|
`.dochygiene/decisions` file — avoids new artifact, respects invariant #4).
|
|||
|
|
|
|||
|
|
### D5. Git safety — WIP checkpoint, single commit, precise staging, rollback
|
|||
|
|
|
|||
|
|
**Pre-flight:**
|
|||
|
|
1. Resolve `project_root` via `StateStore`.
|
|||
|
|
2. `git status --porcelain`. Clean → `baseline = HEAD`. Dirty → auto-create a WIP
|
|||
|
|
checkpoint commit of user work FIRST (per invariant #5 "or auto WIP checkpoint");
|
|||
|
|
`baseline = that checkpoint`. The cleanup commit is still exactly one.
|
|||
|
|
3. Tracked-files-only: untracked candidate docs are skipped and reported (they have no
|
|||
|
|
git history to roll back to).
|
|||
|
|
|
|||
|
|
**Staging (precise):**
|
|||
|
|
- Non-move ops: applier writes the file; skill calls `git add <staged_paths>`.
|
|||
|
|
- `move-to-archive`: applier calls `git mv` (stages both source removal and dest
|
|||
|
|
add); skill must NOT `git add` the dest again (double-add corrupts the index).
|
|||
|
|
- NEVER `git add -A` or `git add .` (the report lives in gitignored `.dochygiene/`
|
|||
|
|
and must never enter the commit).
|
|||
|
|
|
|||
|
|
**Single commit** via `git-context commit-apply --message-stdin` (gem 0.4.0). The
|
|||
|
|
commit message format:
|
|||
|
|
|
|||
|
|
```
|
|||
|
|
docs: hygiene cleanup (N edits across M files)
|
|||
|
|
|
|||
|
|
Auto: <count> (move-to-archive ×a, replace-text ×b, dedupe ×c, freeze ×d)
|
|||
|
|
Confirmed: <count> (delete-range ×e, distill ×f, split ×g)
|
|||
|
|
Skipped (re-analysis recommended): <count>
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
No `Co-Authored-By`, no AI attribution, no emoji, no trailers (house rule confirmed
|
|||
|
|
by user). Do NOT delegate to the full commit skill (its grouping/secret-scan logic
|
|||
|
|
may split into multiple commits, violating invariant #5).
|
|||
|
|
|
|||
|
|
**Failure handling:**
|
|||
|
|
- Hard failure (applier exit 2, `git mv` fail, write error) → `git restore`/`reset`
|
|||
|
|
to `baseline`; abort with a structured error.
|
|||
|
|
- Partial success (applier exit 1, some file batches guard-skipped) is NOT a
|
|||
|
|
rollback — commit what applied, report skipped files with `re-analysis recommended`.
|
|||
|
|
|
|||
|
|
**Mid-run `last_clean` stamp:** stamped only after the commit succeeds, to the commit
|
|||
|
|
instant (not the run-start instant, so `last_clean` always points at an actual commit).
|
|||
|
|
|
|||
|
|
### D6. Generative entries — live-read freshness, Sonnet subagent, skill writes
|
|||
|
|
|
|||
|
|
Generative entries (`distill`, `split`, and any unknown `op_type`) always require
|
|||
|
|
`confirm` gating. After approval, the skill (not the applier) handles them:
|
|||
|
|
|
|||
|
|
1. Confirm the file still exists (a `move-to-archive` for the same file earlier in
|
|||
|
|
the run would make it gone).
|
|||
|
|
2. Read the **live** file contents now (not from the report cache — generative entries
|
|||
|
|
have no `expected_sha256`, so the only freshness guarantee is a live read).
|
|||
|
|
3. Dispatch to a Sonnet subagent pointing at `workflows/distill.md` (LOOP-GUARD —
|
|||
|
|
the subagent reads that workflow doc, not SKILL.md, to avoid recursion).
|
|||
|
|
4. Subagent returns new prose (or new-primary + archived-section for split).
|
|||
|
|
5. Skill writes + stages the result.
|
|||
|
|
|
|||
|
|
A file with BOTH a generative entry AND a deterministic entry is treated as having
|
|||
|
|
`incompatible-ops-on-file` (see D3) — v1 never composes; re-analysis recommended.
|
|||
|
|
|
|||
|
|
### D7. `sweep` is command-level sequencing, not a third skill
|
|||
|
|
|
|||
|
|
`/hygiene sweep` → invoke `hygiene-check` skill, then invoke `hygiene-clean` skill,
|
|||
|
|
passing `--scope`/`--category` to both. This is wired in `commands/hygiene.md`, not
|
|||
|
|
in a third skill or by having `clean` call `check` internally.
|
|||
|
|
|
|||
|
|
No double-commit: `check` writes only to the gitignored `.dochygiene/`; the single
|
|||
|
|
clean commit is the only git commit produced by the sweep.
|
|||
|
|
|
|||
|
|
**Alternative considered:** a `hygiene-sweep` skill — rejected: `sweep` is purely
|
|||
|
|
`check ; clean` with shared flags; a dedicated skill adds ceremony with no new logic.
|
|||
|
|
|
|||
|
|
## Risks / Trade-offs
|
|||
|
|
|
|||
|
|
- **[delete-range is irreversible]** → Confirm-gated (invariant #7); the batch-confirm
|
|||
|
|
display MUST visually distinguish deletes from reversible auto ops. Mitigated by the
|
|||
|
|
WIP checkpoint (user can always `git reset --hard <checkpoint>`) but the content is
|
|||
|
|
gone from that commit. Documented.
|
|||
|
|
- **[move-to-archive creates link orphans]** → V1 move only; the next `/hygiene check`
|
|||
|
|
flags new orphans via the `broken_reference` signal. Documented in the clean
|
|||
|
|
summary as a follow-up action.
|
|||
|
|
- **[generative second-guard hole]** → Generative entries have no `expected_sha256`;
|
|||
|
|
freshness is guaranteed only via the live read at dispatch time. Do not cache the
|
|||
|
|
span between the report and the Sonnet call. If the file changes between skill start
|
|||
|
|
and Sonnet dispatch, the live read picks up the change (safe); do not add a
|
|||
|
|
pre-write hash check (would need a second read, adding complexity with minimal
|
|||
|
|
benefit in practice).
|
|||
|
|
- **[file with both generative + deterministic entries]** → Incompatible-ops-on-file;
|
|||
|
|
entire file is skipped, re-analysis recommended. V1 never composes.
|
|||
|
|
- **[`insert-frontmatter` key conflict]** → Preserved (skip + `frontmatter-key-conflict`
|
|||
|
|
reason), never overwritten. The idempotent re-derivation in D2 covers this.
|
|||
|
|
- **[tracked-files-only]** → Untracked candidate docs are skipped and reported. A
|
|||
|
|
user who adds a doc without `git add`-ing it won't see it cleaned. Documented.
|
|||
|
|
- **[invariant #8 naming vs. mechanism]** → Invariant #8 is titled "mtime guard" but
|
|||
|
|
the enforcement is a whole-file content sha256. The applier docstring notes this;
|
|||
|
|
the invariant wording is not changed without human approval (META-RULE).
|
|||
|
|
|
|||
|
|
## Migration Plan
|
|||
|
|
|
|||
|
|
Additive only — no existing behavior changes. Deploy order:
|
|||
|
|
|
|||
|
|
1. `workflows/distill.md` (no dependencies beyond existing scripts).
|
|||
|
|
2. `skills/hygiene-clean/SKILL.md` (depends on `distill.md` and the already-built
|
|||
|
|
`patch_applier.py`).
|
|||
|
|
3. Integration tests (`tests/test_clean_integration.py`).
|
|||
|
|
4. `commands/hygiene.md` update (stub replacement only — replaces the "Phase 4"
|
|||
|
|
stubs; no structural change to the command file).
|
|||
|
|
5. CONTEXT.md updates (`scripts/CONTEXT.md`, `skills/hygiene-clean/CONTEXT.md`).
|
|||
|
|
|
|||
|
|
Rollback: remove `skills/hygiene-clean/SKILL.md`, `workflows/distill.md`, and the
|
|||
|
|
integration tests; revert `commands/hygiene.md` to Phase 3 stubs. The deterministic
|
|||
|
|
core, report-builder, state-store, and `patch_applier.py` are untouched.
|
|||
|
|
|
|||
|
|
## Open Questions (resolved — recorded as decisions)
|
|||
|
|
|
|||
|
|
1. **Insert-frontmatter at apply time** → re-derive at apply time (D2); the deferred
|
|||
|
|
hole from `add-check` is now resolved.
|
|||
|
|
2. **Guard mechanism** → content hash only (`expected_sha256`), no mtime pre-filter.
|
|||
|
|
Invariant #8 wording preserved (META-RULE).
|
|||
|
|
3. **Commit attribution** → NONE (`git-context` house rule, user-confirmed). No
|
|||
|
|
`Co-Authored-By`, no AI attribution, no emoji.
|
|||
|
|
4. **Generative freshness** → live read at dispatch time is the only guard; no cached
|
|||
|
|
span (D6).
|
|||
|
|
5. **Sweep is command-level** → wired in `commands/hygiene.md`, not a third skill (D7).
|
|||
|
|
6. **Partial success** → NOT a rollback; commit what applied, report skipped (D5).
|
|||
|
|
7. **Audit trail** → folded into the commit body; no `.dochygiene/decisions` file
|
|||
|
|
(respects invariant #4).
|
|||
|
|
8. **`git mv` double-staging** → the applier stages `git mv` both sides; skill must
|
|||
|
|
NOT `git add` the dest path again (D5).
|