From 004283fc0a9ba6ea5689ee11b623a35a4d6858fa Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 2 Jun 2026 13:30:21 -0500 Subject: [PATCH] Draft an implementation plan to address the items identified in the audit --- docs/roadmap/cleanup.md | 408 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 408 insertions(+) create mode 100644 docs/roadmap/cleanup.md diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md new file mode 100644 index 0000000..c1e89d2 --- /dev/null +++ b/docs/roadmap/cleanup.md @@ -0,0 +1,408 @@ +# Cleanup Implementation Roadmap + +This roadmap turns the audit findings in `docs/roadmap/audit.md` into staged +implementation work for LLM coding agents. + +Each stage is intended to be implemented in order. Treat each stage as a +separate prompt or commit unless the change is trivially small. Before starting +any stage, read: + +- `docs/policy/architecture.md` +- `docs/policy/development.md` +- `docs/policy/documentation.md` + +## Global Rules + +- Preserve public CLI behavior, config semantics, source manifest schema, + destination state schema, implemented backend behavior, and documented output + contracts unless a stage explicitly says otherwise. +- Do not introduce external dependencies. +- Do not create a generic workflow engine, CLI framework, plugin system, broad + adapter abstraction, or generic reporting layer. +- Keep concrete backend construction in `internal/app`. +- Keep backend-specific filesystem, SSH/SFTP, and S3 details inside adapters. +- Keep user-facing documentation unchanged unless public behavior changes. + These stages are intended to be behavior-preserving. +- After each stage, run the tests listed for that stage. If Go cache + permissions fail in a restricted environment, use: + +```sh +GOCACHE=/private/tmp/distributor-gocache GOMODCACHE=/private/tmp/distributor-gomodcache go test +``` + +## Stage 1: Backend Open-Config Unification + +Goal: make source and destination backend opening use one app-local conversion +path while preserving current runtime behavior. + +Implement: + +- In `internal/app`, introduce a package-local backend-open spec that can + represent both `config.Backend` sources and `config.Destination` + destinations. +- The spec must include backend name, path, SSH fields, S3 fields, credentials, + and a role label for error wording where needed. +- Add two thin constructors: + - one from `config.Backend`; + - one from `config.Destination`. +- Replace separate source/destination SSH and S3 `storage.OpenConfig` + construction with one helper that: + - sets the local path key; + - copies SSH host, user, port, key file, known-hosts path, host-key policy, + and read-only known-hosts flag; + - copies S3 endpoint, bucket, prefix, region, force-path-style setting, and + resolved explicit credentials; + - resolves explicit S3 credentials only through `config.Environment`. +- Keep adapter registration and concrete adapter imports in `internal/app`. +- Do not move backend opening, credential resolution, or runtime support checks + into `internal/config`. + +Tests: + +- Update `internal/app/backends_test.go` to assert equivalent source and + destination conversion for local, SSH, S3, S3 explicit credentials, and + dry-run known-host behavior. +- Add or preserve a regression test proving configured-source validation and + run source opening produce equivalent SSH/S3 open config for the same source + settings. +- Run: + +```sh +go test ./internal/app +``` + +Acceptance criteria: + +- `run`, configured-source `validate`, and configured-source `inspect` still + open local, SSH, and S3 sources as before. +- `run --dry-run` still makes SSH known-host writes read-only. +- S3 explicit credentials still prefer resolved environment/secrets-directory + values and never call `os.Getenv` directly from backend wiring. + +## Stage 2: Shared Link URL Policy + +Goal: centralize HTTP URL validation for configured links and persisted link +state. + +Implement: + +- Add a small `internal/link` package. +- In that package, define the canonical link URL rule: + - URL must parse successfully; + - scheme must be `http` or `https`; + - host must be present; + - query string must be absent; + - fragment must be absent. +- Use the helper from: + - config validation for `links.base_url`; + - state validation for `links.primary_url`; + - state validation for output `url`; + - publish link planning before building output URLs. +- Preserve existing field-context error messages from config and state callers + where tests assert them. The helper may return concise common errors, while + callers add context such as `links.base_url` or `state outputs[i].url`. +- Keep URL path construction and primary URL selection in `internal/publish`. + +Tests: + +- Add table-driven tests for `internal/link` covering valid `http`, valid + `https`, missing host, unsupported scheme, query string, fragment, and parse + failure. +- Keep config tests focused on config field context and accepted primary-link + policies. +- Keep state tests focused on persisted state validation context. +- Keep publish tests focused on URL path semantics and `index.html` + directory-style URLs. +- Run: + +```sh +go test ./internal/config ./internal/state ./internal/publish +``` + +Acceptance criteria: + +- Config, state, and publish all enforce the same link URL policy. +- Existing public URL generation behavior is unchanged. +- No config package dependency on `internal/state` is introduced. + +## Stage 3: Configured-Source CLI Parsing + +Goal: remove duplicate `validate` and `inspect` configured-source flag parsing +without obscuring command behavior. + +Implement: + +- In `internal/cli`, add a small helper that parses the shared source-diagnostic + command flags for a command name: + - `--config`; + - `--pipeline`; + - `--bundle`; + - `--format`; + - at most one optional local path. +- The helper should return a simple parsed value containing path, config path, + pipeline id, bundle path, and normalized output format. +- Keep `validateCommand` and `inspectCommand` as explicit entrypoints. +- Keep command help text separate and command-specific. +- Keep `validateInspectModeOK` behavior or fold it into the shared helper with + the same error semantics. +- Do not introduce a CLI framework, generic command engine, or new root command + dispatch model. + +Tests: + +- Add or preserve CLI tests for both commands covering: + - local path mode; + - configured source mode; + - configured source JSON output; + - too many local paths; + - local path combined with config flags; + - `--pipeline` without `--config`; + - `--bundle` without `--config`; + - `--config` without `--pipeline`; + - invalid `--format`. +- Run: + +```sh +go test ./internal/cli ./internal/app +``` + +Acceptance criteria: + +- `validate` and `inspect` still support the exact documented modes. +- Help output remains command-specific. +- User-facing error wording remains compatible with current tests. + +## Stage 4: Publish Output Projection Helpers + +Goal: reduce drift when publish output metadata is projected into state, run +JSON output, managed cleanup paths, and notifications. + +Implement: + +- Add package-local helper methods or functions in `internal/publish` for: + - converting a `publish.Output` to `state.OutputFile`; + - extracting a managed destination output path; + - converting output slices to state output slices; + - converting output slices to managed path slices. +- Replace existing duplicate state-output and managed-path projections with the + new helpers. +- Keep app-owned JSON result structs in `internal/app`. +- Keep notify-owned event structs in `internal/notify`. +- In `internal/app`, use publish helpers only where they reduce duplicate field + mapping without changing JSON or notification schemas. +- Do not collapse state, JSON, and notification schemas into one shared public + type. + +Tests: + +- Add publish tests for output-to-state conversion, including source output, + generated output, transform name, URL, digest, and size. +- Add or preserve app JSON regression coverage proving generated output records + include path, kind, source path, transform, URL, digest, and size. +- Add or preserve notification mapping coverage for generated output metadata. +- Run: + +```sh +go test ./internal/publish ./internal/app +``` + +Acceptance criteria: + +- Destination state output metadata is unchanged. +- Run JSON output metadata is unchanged. +- Notification event metadata is unchanged. +- Managed cleanup still deletes only recorded output paths plus state through + existing storage safety helpers. + +## Stage 5: Run Orchestration File Split + +Goal: make `internal/app/run.go` easier to maintain by moving package-local +helpers into focused files without changing behavior. + +Implement: + +- Split current run-related helpers into package-local files such as: + - `run_selection.go` for destination bundle selection and fixed-path helpers; + - `run_warnings.go` for secret and SSH warning helpers; + - `run_output.go` for text and JSON action projection helpers; + - `run_summary.go` for summary counters and summary result projection; + - `run_failures.go` for run failure aggregation and partial-result detection. +- Keep `Run`, `runConfig`, and `runConfigWithBackendFactory` as the primary + orchestration functions. +- Do not change exported APIs, text output, JSON output, warning wording, + summary counters, failure aggregation, fixed-path selection, notification + behavior, or dry-run behavior. +- Do not create a separate reporting package. + +Tests: + +- Add focused tests for summary text and JSON projection if extracting summary + code exposes a natural package-local test seam. +- Preserve existing app and CLI output tests. +- Run: + +```sh +go test ./internal/app ./internal/cli +``` + +Acceptance criteria: + +- `run.go` is smaller and orchestration-focused. +- All run output remains byte-for-byte compatible where tests currently assert + exact output. +- JSON partial-failure behavior remains unchanged. + +## Stage 6: Storage Walk Helper + +Goal: centralize the narrow shared parts of storage walk callback handling +without moving backend traversal semantics out of adapters. + +Implement: + +- Add an `internal/storage` helper for walk emission that owns only: + - context checks before callback emission; + - `WalkOptions.Limit` counting; + - `ErrStopWalk` handling; + - callback error wrapping as storage walk errors. +- Use this helper in local, SSH, and S3 adapters only where the semantics match. +- Add `storage.HasAny(ctx, backend, prefix)` as a shared helper implemented via + `Walk` with non-recursive limit-one traversal. +- Update adapter `HasAny` implementations to call the shared helper if doing so + preserves current behavior. +- Do not centralize: + - local filesystem walking; + - SSH/SFTP directory walking; + - S3 pagination or object listing; + - deletion behavior; + - native path or object key construction; + - symlink handling. + +Tests: + +- Add storage-level tests for the walk emitter or helper behavior: + - limit stops after the expected number of emissions; + - `ErrStopWalk` stops without error; + - callback errors are wrapped as storage walk errors; + - context cancellation is honored. +- Add or preserve `HasAny` behavior tests for empty backends, missing prefixes, + file prefixes, directory prefixes, and object-prefix cases where applicable. +- Run: + +```sh +go test ./internal/storage ./internal/storage/fake ./internal/adapters/local ./internal/adapters/ssh ./internal/adapters/s3 +``` + +Acceptance criteria: + +- Adapter traversal order and recursive/non-recursive behavior are unchanged. +- Backend-specific path, key, symlink, deletion, and pagination logic remains + adapter-local. +- `HasAny` behavior is consistent with existing tests. + +## Stage 7: Digest Validation Centralization + +Goal: make `pkg/bundle` the canonical home for digest format validation. + +Implement: + +- Expose `pkg/bundle.ValidateDigest(value string) error`. +- Use the same lowercase `sha256:<64 hex>` rule currently used by manifest + validation. +- Update `pkg/bundle.ValidateManifest` to call the public validator. +- Update `internal/bundle.ValidateDigest` to delegate to + `pkg/bundle.ValidateDigest`. +- Keep digest formatting and bundle digest generation unchanged. +- Treat this as a public producer-facing API addition. Do not remove or rename + existing public APIs. + +Tests: + +- Add public package tests for `pkg/bundle.ValidateDigest` covering valid + digest, uppercase hex rejection, missing prefix, wrong algorithm, short hex, + long hex, and non-hex characters. +- Add or preserve state validation coverage proving destination state output + SHA-256 validation uses the same rule. +- Run: + +```sh +go test ./pkg/bundle ./internal/bundle ./internal/state +``` + +Acceptance criteria: + +- There is one canonical digest validation rule. +- Producer-facing code can validate digest strings directly. +- Existing manifest and state validation behavior is unchanged. + +## Stage 8: Test Fixture Cleanup + +Goal: move broadly reusable test helpers into `internal/testutil` after +production refactors are complete. + +Implement: + +- Move only reusable helpers into `internal/testutil`, such as: + - local config builders for publish policy, path mapping, links, and Markdown + transform combinations; + - file assertion helpers; + - fake backend file assertion helpers; + - state read/write helpers already duplicated in package tests. +- Keep scenario-specific setup local to the package test that uses it. +- Do not import `internal/testutil` from production code. +- Do not combine this stage with production refactors. +- Prefer small helper names that describe behavior over generic fixture-builder + abstractions. + +Tests: + +- Run: + +```sh +go test ./internal/app ./internal/cli ./internal/publish +``` + +Acceptance criteria: + +- Test behavior and coverage are unchanged. +- Test helpers reduce repeated setup without hiding scenario-specific behavior. +- No production package imports `internal/testutil`. + +## Stage 9: Dead-Code And Legacy Sweep + +Goal: remove obsolete helpers left behind by the previous stages. + +Implement: + +- Search for unused or now-duplicative helpers after stages 1 through 8. +- Remove only clearly obsolete code. +- Keep this limited to dead code, duplicate helpers, and stale comments created + by earlier cleanup. +- Do not perform unrelated refactors. +- Do not change user-facing docs unless a previous stage intentionally changed + public behavior. + +Tests: + +- Run: + +```sh +go test ./... +``` + +Acceptance criteria: + +- Full test suite passes. +- No stale helper names remain from the old implementation paths. +- No public behavior, schemas, or documented workflows change. + +## Final Cleanup Acceptance + +After all stages are complete: + +- Run `go test ./...`. +- Re-read `docs/roadmap/audit.md` and confirm every high-confidence and + medium-confidence opportunity has been addressed or intentionally deferred in + a short note. +- Confirm no refactor-to-avoid item from the audit was introduced. +- Confirm `git status --short` contains only intentional changes for the final + stage.