Draft an implementation plan to address the items identified in the audit
This commit is contained in:
408
docs/roadmap/cleanup.md
Normal file
408
docs/roadmap/cleanup.md
Normal file
@@ -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 <packages>
|
||||
```
|
||||
|
||||
## 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.
|
||||
Reference in New Issue
Block a user