Compare commits
2 Commits
78f92154ab
...
4bd5b19025
| Author | SHA1 | Date | |
|---|---|---|---|
| 4bd5b19025 | |||
| 846dd1843d |
255
docs/roadmap/audit.md
Normal file
255
docs/roadmap/audit.md
Normal file
@@ -0,0 +1,255 @@
|
||||
# Code Quality and Deduplication Audit
|
||||
|
||||
## 1. Executive summary
|
||||
|
||||
The Stage 1-8 implementation is generally clean, modular, and close to the roadmap boundaries. The codebase is ready for a limited cleanup pass before remote backend work. I did not find evidence of a major architectural problem or a need for broad redesign.
|
||||
|
||||
Top three refactoring targets:
|
||||
|
||||
1. Runtime backend and transform resolution bypasses the registries already present in `internal/storage` and `internal/transform`.
|
||||
2. Source manifest validation semantics are split between `internal/bundle` and `internal/state`, and publish/transform policy is checked in both `internal/config` and `internal/publish`.
|
||||
3. Managed deletion target construction is duplicated in the local and fake storage backends and will otherwise be repeated in SSH/SFTP and S3 adapters.
|
||||
|
||||
The recommended cleanup is a series of small, behavior-preserving commits. The public CLI, config schema, manifest schema, and destination state schema should remain stable.
|
||||
|
||||
## 2. Repository map reviewed
|
||||
|
||||
Reviewed implementation areas:
|
||||
|
||||
- `cmd/distributor`: process entrypoint.
|
||||
- `internal/cli`: root command dispatch, `run`, `validate`, `inspect`, `version`, and CLI tests.
|
||||
- `internal/app`: run orchestration, validation, inspection, notification integration, and integration-style tests.
|
||||
- `internal/config`: config structs, defaults, YAML loading, validation, examples tests.
|
||||
- `internal/bundle`: manifest parsing, digest logic, discovery, validation, fixtures, and tests.
|
||||
- `internal/state`: `.distributor.json` parsing, validation, comparison, marshaling, and tests.
|
||||
- `internal/storage`: storage interface, path helpers, typed errors, registry, fake backend, and tests.
|
||||
- `internal/adapters/local`: local storage backend and tests.
|
||||
- `internal/publish`: plan, output selection, destination inspection, safety, execution, and tests.
|
||||
- `internal/transform` and `internal/transform/markdown`: transform interface, registry, Markdown-to-HTML implementation, and tests.
|
||||
- `internal/notify`: notifier interface and no-op implementation.
|
||||
- `internal/logging`: placeholder logging setup.
|
||||
- `examples`, `docs/config.md`, `docs/cli.md`, `docs/operations.md`, and `docs/internal/*`.
|
||||
|
||||
Requested areas that do not currently exist as separate packages: `internal/stage`, `internal/modules`, `internal/validators`, `internal/artifacts`, `internal/manifest`, `internal/schema`, `internal/report`, and `pkg`.
|
||||
|
||||
Major execution paths reviewed:
|
||||
|
||||
- `distributor validate <path>`: CLI to app to local backend to bundle discovery and validation.
|
||||
- `distributor inspect <path>`: CLI to app to local backend to bundle discovery and inspection output.
|
||||
- `distributor run --config <path> [--dry-run]`: CLI to app to config loading, local source discovery, publish planning, local execution, destination state writing, and no-op notification.
|
||||
|
||||
## 3. High-confidence deduplication opportunities
|
||||
|
||||
### Runtime adapter and transform resolution bypass existing registries
|
||||
|
||||
- Affected files/packages: `internal/app/run.go`, `internal/app/validate.go`, `internal/app/inspect.go`, `internal/publish/plan.go`, `internal/storage/registry.go`, `internal/transform/registry.go`.
|
||||
- Duplicated or near-duplicated behavior: app code directly checks `config.BackendLocal` and calls `local.New` in multiple paths. Publish code directly imports `internal/transform/markdown` and constructs the Markdown transformer through `markdownTransformer`, while `internal/transform.Registry` is unused.
|
||||
- Why it matters: Stage 9 and Stage 10 will add SSH/SFTP and S3 backends. If app orchestration continues to branch on backend names directly, every command path and run mode becomes a possible drift point. Direct Markdown construction also makes future transform registration less useful.
|
||||
- Recommended refactor: introduce an app-level backend factory that converts `config.Backend` and `config.Destination` into `storage.OpenConfig` and opens through a registry. Register only local for now. Move Markdown transformer selection behind a transform registry or a small transform resolver owned by app/publish boundaries.
|
||||
- Suggested tests: keep existing local run/validate/inspect tests; add an app factory unit test for local source and destination config resolution; add a publish/app test proving HTML generation uses the registered Markdown transform.
|
||||
- Risk level: medium. The behavior should remain unchanged, but this touches orchestration wiring used by all commands.
|
||||
|
||||
### Source manifest validation rules are duplicated between bundle and state
|
||||
|
||||
- Affected files/packages: `internal/bundle/manifest.go`, `internal/bundle/validate.go`, `internal/state/distributor.go`, `internal/state/validate.go`, `internal/state/compare.go`.
|
||||
- Duplicated or near-duplicated behavior: source manifest schema version, id, digest format, timestamp presence, file path safety, duplicate file path checks, file size rules, and bundle digest validation are enforced in both bundle parsing/validation and destination state validation.
|
||||
- Why it matters: `.distributor.json` embeds the normalized source manifest model. If the source manifest contract changes, fixes will likely need to be made in multiple packages, and state validation can drift from source validation.
|
||||
- Recommended refactor: expose a single `bundle` helper for validating an in-memory normalized manifest, including duplicate paths and canonical bundle digest. Let `ParseManifest` use parsing-specific checks and then call that helper. Let `state.Validate` call the same helper for embedded source manifests.
|
||||
- Suggested tests: preserve current `bundle` and `state` validation tests; add one cross-package regression fixture proving a manifest accepted by `bundle` is accepted when embedded in state, and a digest/path violation is rejected through the shared helper.
|
||||
- Risk level: medium. The refactor is behavior-preserving but affects core contract validation.
|
||||
|
||||
### Managed deletion target construction is repeated in storage backends
|
||||
|
||||
- Affected files/packages: `internal/adapters/local/backend.go`, `internal/storage/fake/backend.go`, `internal/storage/path.go`, `internal/publish/execute.go`.
|
||||
- Duplicated or near-duplicated behavior: both local and fake backends build the managed deletion target list by joining every output path under `bundlePath`, appending `storage.StatePath(bundlePath)`, and then applying backend-specific deletion.
|
||||
- Why it matters: SSH/SFTP and S3 adapters will need the same target derivation. Duplicating it in each adapter increases the chance of inconsistent state-file handling, invalid path behavior, or root deletion safeguards.
|
||||
- Recommended refactor: add a storage helper such as `ManagedBundleTargets(bundlePath string, managedOutputPaths []string) ([]string, error)` that validates and returns exact logical targets. Keep actual deletion backend-specific.
|
||||
- Suggested tests: add storage helper tests for root bundle path, nested bundle path, invalid output path, and inclusion of `.distributor.json`; keep local and fake managed deletion tests focused on backend deletion behavior.
|
||||
- Risk level: low. Target construction is small and already deterministic.
|
||||
|
||||
### Publish/HTML policy validation is split between config and publish
|
||||
|
||||
- Affected files/packages: `internal/config/validate.go`, `internal/publish/plan.go`, `internal/publish/output.go`, `internal/config/load_test.go`, `internal/publish/output_test.go`.
|
||||
- Duplicated or near-duplicated behavior: `internal/config` validates that `publish.html` requires `transform.markdown_to_html.enabled: true` and sidecar mode. `internal/publish` independently validates the same effective policy in `validateRequest`.
|
||||
- Why it matters: config-loaded runs are protected, but tests and programmatic callers can see different error text or future behavior if config and publish validation evolve separately.
|
||||
- Recommended refactor: centralize the effective publish/transform policy check in `internal/config` or a small policy helper that both config validation and publish request validation call.
|
||||
- Suggested tests: keep the existing config rejection test and publish rejection test, but assert both route through the same allowed combinations table.
|
||||
- Risk level: low.
|
||||
|
||||
### Repeated bundle, config, and state test fixtures obscure behavior changes
|
||||
|
||||
- Affected files/packages: `internal/app/run_test.go`, `internal/cli/root_test.go`, `internal/publish/output_test.go`, `internal/publish/execute_test.go`, `internal/transform/markdown/markdown_test.go`, `internal/state/distributor_test.go`, `internal/bundle/testdata`.
|
||||
- Duplicated or near-duplicated behavior: several packages recreate the same valid bundle shape, timestamps, file contents, digest calculation, local YAML snippets, and destination state data. Some tests use filesystem fixtures, some use fake storage, and some inline JSON/YAML.
|
||||
- Why it matters: changing the bundle contract, adding output metadata, or changing default config will require edits across many test files. The duplication also makes it harder to tell which tests are exercising unique behavior.
|
||||
- Recommended refactor: add an internal test helper package or package-local shared fixtures for valid manifests, source bundles, state files, and minimal configs. Keep package-specific edge cases local.
|
||||
- Suggested tests: no behavior tests are needed for helpers themselves beyond using them; add helper-backed tests incrementally while preserving current assertions.
|
||||
- Risk level: low.
|
||||
|
||||
## 4. Medium-confidence opportunities
|
||||
|
||||
### CLI command scaffolding is lightly duplicated
|
||||
|
||||
- Affected files/packages: `internal/cli/run.go`, `internal/cli/validate.go`, `internal/cli/inspect.go`, `internal/cli/root.go`.
|
||||
- Duplicated or near-duplicated behavior: every command checks help, validates positional arguments, prints command-specific usage, calls app functions, and maps errors to exit codes. `validate` and `inspect` have nearly identical optional-path parsing.
|
||||
- Why it matters: adding `--pipeline`, output modes, or future command preflight behavior could create command drift.
|
||||
- Recommended refactor: add small CLI helpers for help detection, optional single path parsing, no-positional-argument rejection, and command usage printing. Do not introduce a large command framework unless the CLI grows substantially.
|
||||
- Suggested tests: keep existing CLI root tests; add table tests for validate/inspect path arity and run positional-argument rejection.
|
||||
- Risk level: low.
|
||||
|
||||
### Output metadata projection appears in multiple layers
|
||||
|
||||
- Affected files/packages: `internal/publish/output.go`, `internal/app/run.go`, `internal/notify/notify.go`, `internal/state/validate.go`.
|
||||
- Duplicated or near-duplicated behavior: publish outputs are converted to state output files in `stateOutputs`, and separately converted to notification outputs in `notifyEvent`.
|
||||
- Why it matters: when output metadata grows, fields can be added to state and omitted from notification, or vice versa, without a compiler-visible central projection.
|
||||
- Recommended refactor: keep destination state conversion in publish, but consider a small helper for notification output projection if output metadata changes in the next feature stage.
|
||||
- Suggested tests: extend notifier tests when output fields change.
|
||||
- Risk level: low.
|
||||
|
||||
### Empty-path display helpers are repeated
|
||||
|
||||
- Affected files/packages: `internal/bundle/validate.go`, `internal/app/inspect.go`, `internal/publish/plan.go`, `internal/publish/safety.go`.
|
||||
- Duplicated or near-duplicated behavior: `displayRoot`, `displayBundlePath`, and `displayPath` all render an empty logical path as `"."`.
|
||||
- Why it matters: this is user-facing output and error text. Minor drift can make tests brittle and logs inconsistent.
|
||||
- Recommended refactor: add a small formatting helper in the package that owns logical paths, or keep command/report formatting in app if avoiding cross-package formatting dependencies.
|
||||
- Suggested tests: update affected output/error tests to use behavior assertions rather than exact helper names.
|
||||
- Risk level: low.
|
||||
|
||||
### Backend config structs repeat fields by shape
|
||||
|
||||
- Affected files/packages: `internal/config/config.go`, `internal/config/validate.go`.
|
||||
- Duplicated or near-duplicated behavior: source `Backend` and destination `Destination` share backend fields such as `backend`, `path`, `uri`, `endpoint`, `bucket`, `prefix`, `region`, `force_path_style`, and credentials.
|
||||
- Why it matters: new backend fields need to be added in two places and passed through validation manually.
|
||||
- Recommended refactor: consider embedding a shared backend config struct in `Destination` only when adding remote backend implementations. Do not refactor now if it would complicate YAML decoding or docs.
|
||||
- Suggested tests: config load tests should cover any newly added shared fields for source and destination.
|
||||
- Risk level: low.
|
||||
|
||||
## 5. Boundary and responsibility concerns
|
||||
|
||||
- `internal/app` currently imports `internal/adapters/local` directly and branches on backend names. This is acceptable for the local MVP, but the documented architecture says app should construct backends through registries and avoid adapter-specific logic. The app layer is the right place for backend factory wiring; backend implementation details should remain in adapter packages.
|
||||
- `internal/publish` imports `internal/transform/markdown` directly. Publish should plan and execute outputs, but transform implementation selection belongs in transform/app wiring. A registry-backed resolver would better match the current package layout.
|
||||
- `internal/state` owns destination state, but it partially revalidates source manifest semantics. State should validate state-specific fields and delegate embedded source manifest contract checks to `internal/bundle`.
|
||||
- `internal/storage` owns logical path and state path helpers, but the state filename constant is private while callers and tests still hard-code `.distributor.json`. Expose the constant or provide a clearer canonical helper to reduce cross-package string reuse.
|
||||
|
||||
## 6. Path, key, and naming construction review
|
||||
|
||||
Local path safety is centralized well in `internal/storage/path.go` and the local adapter. Bundle-relative path composition generally uses `storage.Join`, and destination state paths use `storage.StatePath`.
|
||||
|
||||
Cleanup targets:
|
||||
|
||||
- `.distributor.json` is canonical in `storage.StatePath`, but the literal is also hard-coded in `bundle.ValidateSourcePath`, adapter tests, app tests, CLI tests, and docs. Exporting a canonical state filename would reduce drift.
|
||||
- Managed deletion target construction is repeated in local and fake storage backends. A storage helper should derive exact managed targets once.
|
||||
- Empty logical path display as `"."` is repeated across app, bundle, and publish.
|
||||
- Markdown sidecar naming is currently local to `internal/transform/markdown` through `strings.TrimSuffix(file.Path, ".md") + ".html"`. That is acceptable while Markdown sidecar is the only transform, but future path remapping or additional transforms should introduce a transform output naming helper rather than spreading suffix logic.
|
||||
|
||||
## 7. Resolution and catalog review
|
||||
|
||||
Backend resolution is not centralized enough for the next roadmap stage. The storage registry exists and is tested, but runtime code does not use it. App orchestration currently resolves only local backends manually.
|
||||
|
||||
Transform resolution is also not centralized enough. The transform registry exists, but publish planning directly constructs the Markdown transformer.
|
||||
|
||||
There are no separate artifact, schema, prompt, profile, module, validator, stage, source catalog, or report catalog packages in the current repository. That absence is appropriate for this application; no new catalog layer should be introduced unless a later feature creates multiple named implementations with shared resolution semantics.
|
||||
|
||||
## 8. Config and command-loading review
|
||||
|
||||
Config loading is mostly centralized: `app.Run` applies the default config path, calls `config.LoadFile`, and `config.LoadFile` handles YAML strict decoding, defaults, and validation. I did not find multiple independent config-loading paths for `run`.
|
||||
|
||||
Intentional differences:
|
||||
|
||||
- `validate` and `inspect` take direct local paths and do not load config. That matches current CLI behavior.
|
||||
- `run` supports `--config` and `--dry-run`; `validate` and `inspect` do not.
|
||||
|
||||
Likely cleanup:
|
||||
|
||||
- Keep default config path resolution close to config/app rather than in CLI.
|
||||
- Add shared CLI parsing helpers only for repeated command preflight, not for business policy.
|
||||
- Centralize publish/transform policy validation so config-loaded and programmatic publish requests cannot drift.
|
||||
|
||||
## 9. State, manifest, or progress handling review
|
||||
|
||||
Manifest handling is deterministic and well-covered: parsing is strict, digest validation is canonical, discovery sorts bundle paths, and source validation rejects unsafe paths and symlinks through storage metadata.
|
||||
|
||||
Destination state handling is generally strong: `.distributor.json` is the success marker, comparison is centralized in `internal/state`, and publish execution writes state only after outputs are written.
|
||||
|
||||
Cleanup targets:
|
||||
|
||||
- Destination state validation should delegate embedded manifest validation to `internal/bundle`.
|
||||
- State output metadata construction should remain centralized in publish; if notification output metadata grows, add a projection helper to avoid field drift.
|
||||
- Run summary and status output currently live in `internal/app/run.go`. That is acceptable for MVP. Only extract a report/formatting package if additional output formats or commands start sharing the same summaries.
|
||||
|
||||
I did not find run checkpoint, resume, retry, force, or progress-file logic in the implemented MVP.
|
||||
|
||||
## 10. Refactors to avoid
|
||||
|
||||
- Do not introduce a generic workflow engine for the pipeline. The current sequential orchestration is readable and matches MVP requirements.
|
||||
- Do not add a broad plugin architecture. Registries for storage and transforms are enough for the next stage.
|
||||
- Do not redesign the CLI around a larger framework solely to remove small parsing duplication.
|
||||
- Do not merge local and fake backends into one implementation. Their shared contract should be tested, but their storage behavior is intentionally different.
|
||||
- Do not generalize every test fixture immediately. Preserve package-local edge-case setup where it makes the behavior clearer.
|
||||
- Do not create catalog packages for artifacts, schemas, prompts, profiles, modules, stages, or reports unless the product adds multiple named implementations in those domains.
|
||||
- Do not rewrite manifest or state schemas as part of cleanup. Any schema change should be its own explicit compatibility task.
|
||||
|
||||
## 11. Recommended implementation sequence
|
||||
|
||||
1. Centralize storage names and managed target construction.
|
||||
- Export or otherwise canonicalize the destination state filename.
|
||||
- Add a storage helper for managed bundle deletion targets.
|
||||
- Update local and fake backends to use it.
|
||||
|
||||
2. Centralize source manifest model validation.
|
||||
- Add a `bundle` helper for validating normalized manifests.
|
||||
- Reuse it from manifest parsing and destination state validation.
|
||||
|
||||
3. Centralize publish/transform policy validation.
|
||||
- Move the allowed publish/transform combinations into one helper.
|
||||
- Reuse it from config validation and publish request validation.
|
||||
|
||||
4. Introduce runtime backend factory wiring.
|
||||
- Register local storage through the existing storage registry.
|
||||
- Update app run/validate/inspect paths to use the factory where appropriate.
|
||||
- Keep unsupported remote backends returning the same user-facing behavior.
|
||||
|
||||
5. Introduce transform resolver wiring.
|
||||
- Register Markdown-to-HTML through the existing transform registry.
|
||||
- Remove direct Markdown construction from publish.
|
||||
|
||||
6. Add focused CLI preflight helpers.
|
||||
- Share optional-path parsing and no-extra-argument handling.
|
||||
- Keep command bodies explicit.
|
||||
|
||||
7. Consolidate high-value test fixtures.
|
||||
- Add helpers for valid source bundles, minimal configs, destination state, and fake backend source data.
|
||||
- Migrate tests opportunistically while preserving package-specific assertions.
|
||||
|
||||
8. Revisit output/report formatting only if additional output formats are added.
|
||||
|
||||
9. Do a small dead-code sweep after the above.
|
||||
- Remove `ErrNotImplemented`, `Pipeline`, or placeholder logging only if they are still unused and not needed by planned next work.
|
||||
|
||||
## 12. Test strategy
|
||||
|
||||
Tests to add before or during cleanup:
|
||||
|
||||
- `internal/storage`: tests for managed target helper behavior, including root bundle path, nested bundle path, invalid output path, and `.distributor.json` inclusion.
|
||||
- `internal/bundle` and `internal/state`: shared manifest validation regression tests for embedded destination manifests.
|
||||
- `internal/config` and `internal/publish`: table-driven tests for allowed and rejected publish/transform policy combinations.
|
||||
- `internal/app`: backend factory tests that preserve current unsupported-backend errors and local backend behavior.
|
||||
- `internal/transform`: registry-backed Markdown resolution test if publish/app wiring moves to the registry.
|
||||
- `internal/cli`: table tests for command help, optional path handling, and rejected extra args.
|
||||
|
||||
Tests that can accompany refactors:
|
||||
|
||||
- Local and fake backend managed deletion tests after target helper extraction.
|
||||
- Existing app run tests after backend factory introduction.
|
||||
- Existing Markdown and publish output tests after transform resolver introduction.
|
||||
- CLI root tests after preflight helper extraction.
|
||||
|
||||
The full suite should be run after any cleanup implementation. For this audit report itself, no full test run is required.
|
||||
|
||||
## 13. Appendix: findings not worth acting on
|
||||
|
||||
- The local and fake backend tests intentionally cover many of the same storage contract behaviors. Keep this parity. Only extract small test helpers if setup starts obscuring assertions.
|
||||
- `config.Backend` and `config.Destination` duplicate backend-shaped fields, but this is not urgent. A premature struct embedding refactor could make YAML behavior and docs less obvious.
|
||||
- `runSummary` and text output formatting are local to app orchestration. They are not worth extracting until more commands or output formats need the same formatting.
|
||||
- `internal/logging.Configure` is currently a placeholder. Do not build a logging abstraction until real logging requirements appear.
|
||||
- `internal/app/pipeline.go` currently contains only a minimal `Pipeline` type. Treat it as harmless unless it remains unused after the next implementation pass.
|
||||
- The small `displayRoot`/`displayPath` helpers are duplicated, but this is a low-risk cleanup after higher-value policy and storage centralization.
|
||||
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