distributor/docs/roadmap/audit.md

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.