Add a cleanup roadmap to address the findings in the code quality audit
This commit is contained in:
537
docs/roadmap/cleanup.md
Normal file
537
docs/roadmap/cleanup.md
Normal file
@@ -0,0 +1,537 @@
|
||||
# Cleanup Implementation Roadmap
|
||||
|
||||
This roadmap converts the findings in `docs/roadmap/audit.md` into staged cleanup work. It is written for LLM coding agents that will implement one stage at a time.
|
||||
|
||||
The cleanup goal is to reduce drift before remote backend work without changing public CLI behavior, config semantics, manifest/state schemas, or local MVP behavior.
|
||||
|
||||
## Global Cleanup Rules
|
||||
|
||||
Every implementation stage must:
|
||||
|
||||
1. read `AGENTS.md`, `docs/policy/architecture.md`, `docs/policy/documentation.md`, and this roadmap before editing;
|
||||
2. implement only the current stage;
|
||||
3. preserve current public CLI behavior and config behavior unless the stage explicitly says otherwise;
|
||||
4. keep cleanup behavior-preserving and avoid broad rewrites;
|
||||
5. add or update focused tests for the changed behavior;
|
||||
6. run relevant package tests, and run `go test ./...` when the stage touches cross-package behavior;
|
||||
7. update implemented internal docs only when an internal contract actually changes;
|
||||
8. leave user-facing docs unchanged unless public behavior changes;
|
||||
9. avoid implementing future remote backend features as part of cleanup.
|
||||
|
||||
If Go cache or module cache permissions fail, use workspace-safe temporary caches:
|
||||
|
||||
```bash
|
||||
GOCACHE=/private/tmp/distributor-gocache GOMODCACHE=/private/tmp/distributor-gomodcache go test ./...
|
||||
```
|
||||
|
||||
## Stage 1: Centralize Storage State Names and Managed Delete Targets
|
||||
|
||||
### Goal
|
||||
|
||||
Make state-file naming and managed deletion target construction canonical before SSH/SFTP and S3 adapters are added.
|
||||
|
||||
### Implementation
|
||||
|
||||
In `internal/storage`:
|
||||
|
||||
- Export the destination state filename as:
|
||||
|
||||
```go
|
||||
const StateFileName = ".distributor.json"
|
||||
```
|
||||
|
||||
- Update `StatePath` to use `StateFileName`.
|
||||
- Add:
|
||||
|
||||
```go
|
||||
func ManagedBundleTargets(bundlePath string, managedOutputPaths []string) ([]string, error)
|
||||
```
|
||||
|
||||
Required behavior:
|
||||
|
||||
- `bundlePath` is validated as a prefix, so `""` is valid.
|
||||
- each managed output path is validated as a file path below `bundlePath`;
|
||||
- returned targets contain each managed output target followed by the `.distributor.json` target;
|
||||
- root bundle path returns output paths unchanged plus `.distributor.json`;
|
||||
- nested bundle path returns `bundlePath/output` plus `bundlePath/.distributor.json`;
|
||||
- invalid output paths fail before any backend deletion occurs;
|
||||
- duplicate output paths do not need to be de-duplicated in this stage.
|
||||
|
||||
Update local and fake backends to call `storage.ManagedBundleTargets` inside `DeleteManagedBundle`. Keep actual deletion, missing-file handling, directory pruning, and backend-specific error translation in the backend implementations.
|
||||
|
||||
Replace code/test references to literal `.distributor.json` with `storage.StateFileName` where the code already imports or reasonably can import `internal/storage`. Do not contort docs or unrelated tests only to remove literals from prose.
|
||||
|
||||
### Tests
|
||||
|
||||
Add or update tests for:
|
||||
|
||||
- `StatePath("") == ".distributor.json"`;
|
||||
- `StatePath("bundle") == "bundle/.distributor.json"`;
|
||||
- managed targets for root and nested bundle paths;
|
||||
- invalid managed output path rejection;
|
||||
- local and fake managed deletion still delete only listed outputs plus state;
|
||||
- local and fake managed deletion still preserve unlisted files.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/storage ./internal/storage/fake ./internal/adapters/local ./internal/bundle ./internal/publish
|
||||
```
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- Managed target derivation lives in one storage helper.
|
||||
- Local and fake backend behavior is unchanged.
|
||||
- No broad recursive delete behavior is introduced.
|
||||
|
||||
## Stage 2: Centralize Normalized Source Manifest Validation
|
||||
|
||||
### Goal
|
||||
|
||||
Make `internal/bundle` the single owner of source manifest semantics, including embedded source manifests in destination state.
|
||||
|
||||
### Implementation
|
||||
|
||||
In `internal/bundle`, add a model-level validation helper:
|
||||
|
||||
```go
|
||||
func ValidateManifest(manifest Manifest) error
|
||||
```
|
||||
|
||||
Required behavior:
|
||||
|
||||
- validate `SchemaVersion == 1`;
|
||||
- validate non-empty `ID`;
|
||||
- validate top-level `Digest` format;
|
||||
- validate non-zero `Created`;
|
||||
- require at least one file;
|
||||
- validate every file path with `ValidateSourcePath`;
|
||||
- validate every file digest format;
|
||||
- reject negative file sizes;
|
||||
- reject duplicate logical file paths;
|
||||
- recompute `BundleDigest(manifest.Files)` and require it to match `manifest.Digest`.
|
||||
|
||||
Keep `ParseManifest` responsible for raw JSON parsing, missing-field detection, RFC3339 timestamp parsing, and trailing-data detection. After building the normalized `Manifest`, call `ValidateManifest` for model-level validation. Preserve current error substrings where practical, especially for existing tests that assert user-facing diagnostics.
|
||||
|
||||
In `internal/state`, replace `validateEmbeddedManifest` logic with delegation to `bundle.ValidateManifest`, wrapping the error as `state source.manifest: ...` where current callers expect state context.
|
||||
|
||||
Do not change the manifest JSON schema, destination state schema, digest algorithm, timestamp normalization policy, or source path policy.
|
||||
|
||||
### Tests
|
||||
|
||||
Add or update tests for:
|
||||
|
||||
- `bundle.ValidateManifest` accepts the existing valid fixture manifest;
|
||||
- `bundle.ValidateManifest` rejects bad schema version, empty id, bad digest, zero created time, empty files, unsafe paths, duplicate paths, negative size, and bundle digest mismatch;
|
||||
- `state.Validate` rejects the same embedded manifest violations through the shared helper;
|
||||
- existing manifest parser and destination state parser tests continue to pass.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/bundle ./internal/state ./internal/publish ./internal/app
|
||||
```
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- Source manifest contract semantics are implemented once in `internal/bundle`.
|
||||
- Destination state validation delegates embedded source manifest semantics to `internal/bundle`.
|
||||
- Existing local MVP behavior is unchanged.
|
||||
|
||||
## Stage 3: Centralize Publish and Transform Policy Validation
|
||||
|
||||
### Goal
|
||||
|
||||
Prevent drift between config validation and publish request validation for allowed source/html/transform combinations.
|
||||
|
||||
### Implementation
|
||||
|
||||
Keep ownership in `internal/config`, because the policy is expressed in config types and used by config validation.
|
||||
|
||||
Add a helper such as:
|
||||
|
||||
```go
|
||||
func ValidatePublishTransformPolicy(publish PublishPolicy, transform Transform) error
|
||||
```
|
||||
|
||||
Required behavior:
|
||||
|
||||
- fail when both `publish.Source` and `publish.HTML` are false;
|
||||
- when `publish.HTML` is true, require `transform.MarkdownToHTML != nil`;
|
||||
- when `publish.HTML` is true, require `transform.MarkdownToHTML.Enabled == true`;
|
||||
- when `publish.HTML` is true, require `transform.MarkdownToHTML.Mode == TransformModeSidecar`;
|
||||
- when Markdown-to-HTML is configured and enabled, reject any mode other than `TransformModeSidecar`;
|
||||
- when Markdown-to-HTML is configured but disabled, allow empty mode or `TransformModeSidecar` and reject other modes;
|
||||
- return concise errors that can be wrapped with config field context.
|
||||
|
||||
Update `internal/config.Validate` to use this helper while preserving contextual error messages such as `pipelines[0].destinations[0].transform...`.
|
||||
|
||||
Update `internal/publish.validateRequest` to use the same helper for programmatic requests. The publish-layer error may be less field-path-specific, but it must remain actionable.
|
||||
|
||||
Do not change defaulting behavior in `ApplyDefaults`.
|
||||
|
||||
### Tests
|
||||
|
||||
Add table tests covering:
|
||||
|
||||
- source-only publish allowed;
|
||||
- html-only publish with enabled sidecar transform allowed;
|
||||
- source-plus-html with enabled sidecar transform allowed;
|
||||
- no outputs rejected;
|
||||
- html without transform rejected;
|
||||
- html with disabled transform rejected;
|
||||
- html with wrong mode rejected;
|
||||
- enabled Markdown transform with wrong mode rejected.
|
||||
|
||||
Apply the table at both config validation and publish request validation levels.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/config ./internal/publish ./internal/app
|
||||
```
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- Effective publish/transform policy is checked through one helper.
|
||||
- Config-loaded and programmatic publish paths cannot drift on this policy.
|
||||
|
||||
## Stage 4: Introduce App Backend Factory Wiring
|
||||
|
||||
### Goal
|
||||
|
||||
Move runtime backend construction toward the storage registry before remote backends are implemented.
|
||||
|
||||
### Implementation
|
||||
|
||||
Create an app-level backend factory, preferably in `internal/app/backends.go`.
|
||||
|
||||
Required shape:
|
||||
|
||||
- The factory owns a `*storage.Registry`.
|
||||
- The default factory registers only the local backend for now.
|
||||
- Local backend registration maps storage open config key `path` to `local.New(path)`.
|
||||
- Source and destination config conversion stays in `internal/app`; adapter packages must not import config types.
|
||||
- Unsupported SSH/SFTP and S3 execution must continue to fail clearly as not implemented for execution.
|
||||
|
||||
Suggested API:
|
||||
|
||||
```go
|
||||
type backendFactory struct {
|
||||
registry *storage.Registry
|
||||
}
|
||||
|
||||
func newBackendFactory() *backendFactory
|
||||
func (f *backendFactory) openSource(ctx context.Context, source config.Backend) (storage.Backend, error)
|
||||
func (f *backendFactory) openDestination(ctx context.Context, destination config.Destination) (storage.Backend, error)
|
||||
func (f *backendFactory) openLocalPath(ctx context.Context, path string) (storage.Backend, error)
|
||||
```
|
||||
|
||||
Use the factory from:
|
||||
|
||||
- `app.Run` for pipeline sources and destinations;
|
||||
- `app.Validate` for direct local path validation;
|
||||
- `app.Inspect` for direct local path inspection.
|
||||
|
||||
Keep `validate` and `inspect` direct-path commands local-only in this stage. Do not add config-driven validation or remote validation.
|
||||
|
||||
Keep public error behavior stable enough that current tests continue to assert meaningful substrings. It is acceptable to update exact error text if the new text is clearer and tests assert stable behavior rather than brittle phrasing.
|
||||
|
||||
### Tests
|
||||
|
||||
Add tests for:
|
||||
|
||||
- factory opens a local source backend;
|
||||
- factory opens a local destination backend;
|
||||
- factory opens a direct local path;
|
||||
- factory rejects unsupported source backend with a clear execution-not-implemented error;
|
||||
- factory rejects unsupported destination backend with a clear execution-not-implemented error.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/app ./internal/cli
|
||||
```
|
||||
|
||||
Then run:
|
||||
|
||||
```bash
|
||||
go test ./...
|
||||
```
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- `internal/app` no longer directly constructs local backends in multiple command paths.
|
||||
- Backend construction goes through one app-level factory and storage registry.
|
||||
- No SSH/SFTP or S3 backend implementation is added.
|
||||
|
||||
## Stage 5: Introduce Transform Resolver Wiring
|
||||
|
||||
### Goal
|
||||
|
||||
Remove direct Markdown transform construction from `internal/publish` and make transform selection explicit and testable.
|
||||
|
||||
### Implementation
|
||||
|
||||
Do not make `internal/transform` import `internal/transform/markdown`; that would create the wrong dependency direction. The app layer should own default transform registration.
|
||||
|
||||
In `internal/publish`, define a narrow resolver interface:
|
||||
|
||||
```go
|
||||
type TransformerResolver interface {
|
||||
Get(name string) (transform.Transformer, bool)
|
||||
}
|
||||
```
|
||||
|
||||
Add a resolver field to `publish.Request`, for example:
|
||||
|
||||
```go
|
||||
Transformers TransformerResolver
|
||||
```
|
||||
|
||||
Update output planning so HTML generation:
|
||||
|
||||
- looks up `transform.MarkdownToHTML` through the resolver;
|
||||
- fails clearly if the resolver is nil or the Markdown transformer is not registered;
|
||||
- uses the resolved transformer to generate outputs.
|
||||
|
||||
Create app-level transform registry wiring, preferably in `internal/app/transforms.go`:
|
||||
|
||||
- create a `transform.Registry`;
|
||||
- register `transform.MarkdownToHTML` with `markdown.New()`;
|
||||
- pass the registry into every publish request created by `app.Run`.
|
||||
|
||||
Update publish tests to use either:
|
||||
|
||||
- a tiny fake resolver and fake transformer for publish package tests; or
|
||||
- a local registry assembled in the test.
|
||||
|
||||
Use app tests to prove the real Markdown transformer remains wired for end-to-end local HTML publication.
|
||||
|
||||
### Tests
|
||||
|
||||
Add or update tests for:
|
||||
|
||||
- publish planning fails when HTML is requested and no transformer resolver is supplied;
|
||||
- publish planning fails when Markdown transformer is missing from the resolver;
|
||||
- publish planning uses a registered transformer;
|
||||
- app local HTML publication still produces `report.html`;
|
||||
- existing Markdown transformer tests remain focused on Markdown rendering.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/publish ./internal/transform ./internal/transform/markdown ./internal/app
|
||||
```
|
||||
|
||||
Then run:
|
||||
|
||||
```bash
|
||||
go test ./...
|
||||
```
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- `internal/publish` no longer imports `internal/transform/markdown`.
|
||||
- App wiring registers the MVP Markdown transform explicitly.
|
||||
- Transform behavior and public CLI behavior are unchanged.
|
||||
|
||||
## Stage 6: Add Focused CLI Preflight Helpers
|
||||
|
||||
### Goal
|
||||
|
||||
Reduce small CLI parsing drift without hiding command behavior behind a large framework.
|
||||
|
||||
### Implementation
|
||||
|
||||
Keep the hand-written standard-library CLI. Do not introduce a new CLI dependency.
|
||||
|
||||
Add small helpers in `internal/cli`, such as:
|
||||
|
||||
```go
|
||||
func parseOptionalPathArg(stderr io.Writer, command string, args []string) (string, bool)
|
||||
func rejectPositionalArgs(stderr io.Writer, command string, args []string) bool
|
||||
```
|
||||
|
||||
Use them to simplify:
|
||||
|
||||
- `validateCommand`;
|
||||
- `inspectCommand`;
|
||||
- run positional argument rejection after flag parsing.
|
||||
|
||||
Keep each command's help text local to that command. Keep `hasHelp`, `fail`, and root dispatch behavior simple and explicit.
|
||||
|
||||
Do not add aliases, output modes, config loading for validate/inspect, or new flags.
|
||||
|
||||
### Tests
|
||||
|
||||
Add table tests for:
|
||||
|
||||
- `validate` with zero args returns app-level required-path error;
|
||||
- `validate` with one arg succeeds for a valid bundle;
|
||||
- `validate` with two args returns usage;
|
||||
- `inspect` with zero args returns app-level required-path error;
|
||||
- `inspect` with one arg succeeds for a valid bundle;
|
||||
- `inspect` with two args returns usage;
|
||||
- `run` rejects extra positional args after flags.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/cli ./internal/app
|
||||
```
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- CLI command bodies are still readable.
|
||||
- Common preflight parsing behavior is centralized where it is actually shared.
|
||||
- Public CLI behavior remains unchanged.
|
||||
|
||||
## Stage 7: Add Test Fixture Helper Foundation
|
||||
|
||||
### Goal
|
||||
|
||||
Create shared test helpers for high-value fixtures without forcing every test to use them immediately.
|
||||
|
||||
### Implementation
|
||||
|
||||
Create `internal/testutil` for test support used by multiple internal packages.
|
||||
|
||||
This package may contain regular Go files even though it is intended only for tests. Production code must not import `internal/testutil`.
|
||||
|
||||
Initial helper coverage:
|
||||
|
||||
- valid source bundle data:
|
||||
- default id `weather.daily.brentwood.2026-05-30`;
|
||||
- default created time `2026-05-30T11:10:00Z`;
|
||||
- default files `report.md` with `# Report\nSunny.\n` and `summary.txt` with `Summary\n`;
|
||||
- filesystem source bundle writer;
|
||||
- fake-backend source bundle writer;
|
||||
- minimal local config writer;
|
||||
- fan-out local config writer;
|
||||
- destination state writer;
|
||||
- destination state reader.
|
||||
|
||||
Helpers should return normal project types such as `bundle.Manifest`, `bundle.Bundle`, and `state.DistributorState`.
|
||||
|
||||
Do not move edge-case test logic into `testutil`. Tests for invalid manifests, collisions, symlinks, failures, and backend-specific behavior should remain close to the package being tested.
|
||||
|
||||
### Tests
|
||||
|
||||
Do not add tests for `testutil` itself unless helpers contain nontrivial logic not covered by consuming tests.
|
||||
|
||||
Migrate only one or two low-risk test files in this stage to prove the helpers work. Good candidates:
|
||||
|
||||
- `internal/publish/execute_test.go`;
|
||||
- `internal/transform/markdown/markdown_test.go`.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/testutil ./internal/publish ./internal/transform/markdown
|
||||
```
|
||||
|
||||
Then run:
|
||||
|
||||
```bash
|
||||
go test ./...
|
||||
```
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- A shared fixture foundation exists.
|
||||
- At least two packages use it successfully.
|
||||
- The migration is incremental and does not obscure package-specific assertions.
|
||||
|
||||
## Stage 8: Migrate High-Value Duplicate Test Fixtures
|
||||
|
||||
### Goal
|
||||
|
||||
Reduce the largest remaining test fixture duplication after the helper foundation is proven.
|
||||
|
||||
### Implementation
|
||||
|
||||
Migrate duplicated valid bundle/config/state setup in:
|
||||
|
||||
- `internal/app/run_test.go`;
|
||||
- `internal/cli/root_test.go`;
|
||||
- `internal/publish/output_test.go`;
|
||||
- `internal/state/distributor_test.go`, where helper use improves clarity.
|
||||
|
||||
Keep tests local when custom setup makes the behavior clearer than a shared helper. Do not chase 100 percent fixture reuse.
|
||||
|
||||
Preserve all existing behavioral assertions.
|
||||
|
||||
### Tests
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/app ./internal/cli ./internal/publish ./internal/state
|
||||
```
|
||||
|
||||
Then run:
|
||||
|
||||
```bash
|
||||
go test ./...
|
||||
```
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- The largest repeated valid bundle/config/state setup is centralized.
|
||||
- Edge-case tests remain readable.
|
||||
- No production code imports `internal/testutil`.
|
||||
|
||||
## Stage 9: Final Dead-Code and Low-Value Cleanup Sweep
|
||||
|
||||
### Goal
|
||||
|
||||
Remove or defer remaining low-value cleanup items after the higher-impact centralization work is complete.
|
||||
|
||||
### Implementation
|
||||
|
||||
Review and decide on these items:
|
||||
|
||||
- remove `app.ErrNotImplemented` if it is still unused;
|
||||
- remove or expand `internal/app/pipeline.go` if the placeholder `Pipeline` type is still unused;
|
||||
- keep `internal/logging.Configure` if it is still a planned extension point, otherwise remove it only if no code or docs reference it;
|
||||
- decide whether empty-path display helpers should remain local or move to a single helper;
|
||||
- leave `config.Backend` and `config.Destination` field duplication alone unless remote backend implementation work is starting immediately.
|
||||
|
||||
Do not add:
|
||||
|
||||
- generic workflow engine;
|
||||
- plugin architecture;
|
||||
- broad CLI framework;
|
||||
- schema rewrites;
|
||||
- remote backend behavior;
|
||||
- output/report formatting package unless it is now clearly shared by multiple commands.
|
||||
|
||||
### Tests
|
||||
|
||||
Run the full suite:
|
||||
|
||||
```bash
|
||||
go test ./...
|
||||
```
|
||||
|
||||
If removals affect docs or internal docs, update only implemented-behavior docs.
|
||||
|
||||
### Completion Criteria
|
||||
|
||||
- Obvious dead code is removed or explicitly left in place for a documented reason.
|
||||
- Remaining duplication is either low-value or intentionally deferred.
|
||||
- The codebase is ready to resume roadmap work on remote backends.
|
||||
|
||||
## Deferred Cleanup
|
||||
|
||||
Do not implement these as part of the cleanup roadmap unless a later roadmap explicitly promotes them:
|
||||
|
||||
- embedding a shared backend config struct into `config.Destination`;
|
||||
- generic output/report formatting package;
|
||||
- broader test fixture migration beyond the high-value repeated fixtures;
|
||||
- remote backend implementations;
|
||||
- force overwrite behavior;
|
||||
- generic pipeline/workflow engine;
|
||||
- broad plugin system.
|
||||
|
||||
Reference in New Issue
Block a user