Internal Implementation CQ Checks

[spxrogers]

Tracking issue for the internal code-quality review of internal/cli and the core model (internal/source, internal/adapter, internal/state, internal/render boundaries).

Overall verdict from the review: the interface hardening (secret fencing, Resolved/Canonical type split, guard tests, fail-closed defaults) is in excellent shape. The implementation behind it has one systemic weakness: domain logic accreting in internal/cli as hand-rolled, duplicated walks over under-modeled data — stringly state keys, sniffed FileOps, parallel structs for single artifacts — held together by comments and pinning tests where types and shared functions should be.

Each fix is a sub-issue below. The first six are in the review's priority order (highest leverage first); the remaining ones cover a verified latent bug and consolidation work the review also surfaced.

1. Typed state keys in internal/state
2. Unify [secrets] validation (live check/doctor disagreement)
3. Shared plan→drift-classification walk for status/diff/reconcile/explain
4. FileOp modeling: typed Action + explicit cleanup-op kind
5. Route plugin upgrade re-apply through the real apply pipeline
6. Restructure reconcileRun into a session type
7. Bug: addMarketplaceSource swallows cache-rename failure
8. Unify pluginTOMLSpec with source.PluginSpec
9. CLI duplication & boundary cleanup batch

[spxrogers]

Checkpoint — 3 of 9 done, #229 is next

Recording state here so it survives a session compaction / model switch.

Completed

sub-issue	PR	merged as
#227 typed state keys	#236	e452f7d
#228 unify [secrets] validation	#237	f6aa686
#229 prerequisite only	#240	e0b051b

main is at e0b051b.

#229 was split into three PRs

A plan review flagged that the original single-PR plan bundled a refactor with five user-visible behaviour changes — the hardest kind to review and the easiest to get silently wrong. The split:

• PR-A — the destination-read shape gate. ✅ Merged as fix(cli): stop status, diff and reconcile hanging on a non-regular destination #240. Lands first so the shared walk inherits one destination-read policy instead of four, keeping the unification a pure refactor.
• PR-B — the refactor itself, strictly behaviour-preserving. ← next
• PR-C — the two semantic changes: mode drift (the four walks currently give four different answers for one chmod'd file) and symlink handling. Each gets the review a policy change deserves rather than riding along inside a refactor.

What PR-B needs to know

The four copies are not equivalent — that is the whole risk. The planning pass produced a 25-axis difference matrix classifying each divergence as an accidental bug, a deliberate variation needing a parameter, or presentation that belongs in the caller. Highlights that shaped the split:

• Home is internal/render (new planwalk.go), not internal/drift — drift is a zero-dependency pure classifier and the walk must stat and read destinations. No import cycle; docs/components.md already claims render depends on drift, so this makes an existing false doc claim true.
• Shape: a plain slice-returning WalkPlanItems, not a Go 1.23 iterator — the repo uses none, errors cannot travel out of one, and all four copies swallow read errors today regardless.
• Three traps the plan body got wrong and the review caught:
  1. filterMatched = len(items) > 0 is a regression — diff sets that flag on op match, before the branch, and synthesized orphan-cleanup ops carry {} content so they yield zero items. It must be set as a side effect inside the match predicate.
  2. The characterization harness must not sort the whole projection, or it goes blind to exactly the item-ordering the refactor promises to preserve.
  3. Deleting modeDrifted breaks a test that calls it directly; it has to be retired in the same commit.
• Three of the four call sites have no unit oracle today. A characterization harness should land first and be green against unmodified code before any conversion.

Issues opened along the way

All carry measurements and a suggested shape:

• A non-regular agentsync.toml hangs every reading surface (check refuses, everything else blocks) #238 — a FIFO agentsync.toml hangs secret list / status / doctor while check refuses cleanly.
• reconcile [o]verride writes ${env:…}/${secret:…} literally into the live destination #239 — reconcile [o]verride writes ${env:…} literally into the live destination. Note the direction: it writes the template, not the secret — config corruption, not disclosure.
• apply and apply --dry-run hang forever on a non-regular destination #241 — apply, apply --dry-run and reconcile --auto-override hang on a non-regular destination.
• import <agent> hangs forever on a non-regular destination (all adapters) #242 — import <agent> and doctor hang, via adapter.ReadFileOptional (27 callers) plus 31 direct reads.
• Repo bloat trimming: comment density is growing 3x faster than code #243 — repo bloat: production comment:code has gone 0.32 → 0.50 since June (code +41%, comments +121%).

#241 and #242 are cheap to verify when someone takes them. internal/cli/dest_fifo_e2e_unix_test.go already carries t.Skip rows for every affected command, each verified to genuinely hang when un-skipped — deleting one skip: string inherits a working assertion.

#229 also has a comment cataloguing what each surface currently does with a refused destination, the symlink split, and the EACCES-classifies-as-Orphan case.

---

Generated by Claude Code

[spxrogers]

Status checkpoint, 2026-09-04. 3 of 9 sub-issues closed; #229 landed as three PRs.

Sub-issue	Outcome
#227	#236 → e452f7d
#228	#237 → f6aa686
#229	#240 → e0b051b (dest-read shape gate), #244 → 7f2d4ad (one shared plan→drift walk), #247 → fd7b40c (mode measured against op.Mode; symlink policy mirrored on the read side)

Next: #230 (typed Action, explicit cleanup-op kind), then #231–#235 in order.

Filed along the way: #238, #239, #241, #242 (hangs and a templated-override bug), #243 (bloat trimming, with a post-#247 note on the characterization harness's fate), #245 (reconcile ignores mode-only drift), #246 (apply's order-dependent shared-file delete), #248 (Writer.Write chmods through a link before the symlink policy is consulted).

---

Generated by Claude Code

[spxrogers]

Status checkpoint — 2026-09-05

Done (4 of 9):

• Introduce typed state keys in internal/state #227 → fix(cli): stop status, diff and reconcile hanging on a non-regular destination #240
• Unify [secrets] validation — check and doctor disagree on backend "env" #228 → fix(cli): stop status, diff and reconcile hanging on a non-regular destination #240
• Extract a shared plan→drift-classification walk for status/diff/reconcile/explain #229 → refactor(cli): one shared plan→drift walk behind status, diff, reconcile and explain (#229 PR-B) #244 (shared plan→drift walk) + fix(cli): mode drift measured against what apply writes; symlinked destinations follow apply's policy on every surface (#229 PR-C) #247 (mode-drift and symlink-destination policy on every surface)
• FileOp modeling: typed Action and an explicit cleanup-op kind #230 → refactor(adapter): typed FileOp.Action and an explicit OpCleanup kind (#230) #249 (typed FileOp.Action with a zero value of write; explicit OpCleanup kind built only by adapter.NewCleanupOp, enforced by an AST guard in both directions; OwnedKeys doc corrected)

Follow-ups filed along the way, not part of this series: #245 (reconcile ignores mode-only drift), #246 (apply's order-dependent shared-file delete), #248 (Writer.Write chmods through a link before the symlink policy is consulted), #250 (type MergeStrategy, with the measured blast radius and the guard-side note).

Next, in order: #231 (route plugin upgrade re-apply through the real apply pipeline) — planning now on claude/issue-231-apply-pipeline from fca1c26 — then #232, #233, #234, #235. Same loop per sub-issue: plan + execution spec, independent spec review, execution, PR, four-lens review loop, merge on approval.

---

Generated by Claude Code