distributor/docs/roadmap/audit.md

Code Quality and Deduplication Audit

1. Executive summary

Overall code quality is strong. The repository has clear package boundaries, good current-behavior documentation, focused adapter packages, and tests close to most implemented behavior. The most important cleanup opportunities are narrow and behavior-preserving rather than architectural.

Top three refactoring targets:

1. HTTP upload admission, body staging, and archive validation are split across internal/app and internal/ingest in a way that duplicates size and content-type policy and buffers uploads in memory.
2. Runtime config loading, default config path selection, secret loading, and warning projection are repeated across app entrypoints.
3. Run orchestration mixes destination processing, failure aggregation, warning recording, and report event ordering in one large loop, making future changes harder to review safely.

The codebase appears ready for a limited cleanup pass. I do not see a major architectural risk that requires a redesign before the next release.

2. Repository map reviewed

Reviewed policy and current-behavior documentation:

• AGENTS.md
• README.md
• docs/policy/architecture.md
• docs/policy/development.md
• docs/policy/documentation.md
• docs/config.md
• docs/cli.md
• docs/operations.md
• docs/troubleshooting.md
• docs/internal/*.md
• docs/roadmap/http.md
• docs/roadmap/implementation.md

Reviewed implementation areas:

• cmd/distributor: executable entrypoint.
• internal/cli: root command, version, run, serve, validate, inspect, and manifest create parsing.
• internal/app: run orchestration, configured source diagnostics, backend factory, manifest creation, CLI output, HTTP upload server, upload coordinator, and pipeline coordinator.
• internal/config: config structs, defaults, validation, quantity parsing, S3/SSH helpers, and secrets resolver.
• internal/bundle: storage-backed source discovery and validation.
• pkg/bundle: public manifest model, digest logic, manifest building, local validation, and local bundle writer.
• internal/storage and internal/storage/fake: backend interface, path helpers, walk helpers, typed errors, and fake backend.
• internal/adapters/local, internal/adapters/ssh, and internal/adapters/s3: runtime storage adapters.
• internal/ingest: HTTP upload archive staging.
• internal/publish: destination planning, output selection, link projection, state writing, cleanup, and force replacement.
• internal/state: destination state parsing, validation, comparison, and JSON projection.
• internal/transform and internal/transform/markdown: transform registry and Markdown rendering.
• internal/link, internal/notify, internal/logging, and internal/testutil.
• examples, package tests, and package testdata.

Requested areas that are absent as separate packages:

• internal/stage
• internal/modules
• internal/validators
• internal/artifacts
• internal/manifest
• internal/schema
• internal/report
• public pkg packages other than pkg/bundle

Those absences are consistent with current architecture policy; the corresponding behavior lives in narrower existing packages.

3. High-confidence deduplication opportunities

HTTP upload body handling should be owned by ingestion

Affected files/packages:

• internal/app/upload_http.go
• internal/app/upload_coordinator.go
• internal/ingest/archive.go
• internal/app/upload_http_test.go
• internal/app/upload_http_integration_test.go
• internal/ingest/archive_test.go

Duplicated or near-duplicated behavior:

• internal/app/upload_http.go validates upload content types in supportedUploadContentType, while internal/ingest/archive.go validates the same content types in archiveFormat.
• internal/app/upload_http.go enforces upload size in readUploadBody, while internal/ingest/archive.go enforces upload size again in writeLimited.
• The HTTP handler reads the full upload body into memory before submission, then the coordinator passes a bytes.Reader to ingestion.

Why it matters:

• The app transport layer now partially owns archive policy that should belong to internal/ingest.
• Large accepted uploads are buffered in memory even though ingestion already has streaming-to-disk mechanics.
• Future archive formats, content types, or upload limit changes would need coordinated edits in multiple packages.

Recommended refactor:

• Move supported content-type checking behind an ingestion-owned helper, for example ingest.ValidateContentType or ingest.IsSupportedContentType.
• Change upload admission so the request body is streamed to staging exactly once before the HTTP handler returns 202 Accepted.
• Keep queue-full rejection before reading the body.
• Queue a staged local bundle root, not an unread request body. This preserves async distribution while keeping HTTP request lifetime separate from later pipeline execution.
• Keep UploadCoordinator responsible for queueing, status, per-pipeline serialization, and execution. Keep internal/ingest responsible for archive format, size, extraction, cleanup, and source bundle validation.

Suggested tests:

• HTTP handler rejects full queues without reading the body.
• HTTP handler streams a valid body to ingestion and returns 202 only after staging succeeds.
• Unsupported content types are rejected through the ingestion-owned content-type policy.
• Oversized uploads are rejected without retaining a staged run.
• Accepted upload status still transitions through queued/running/succeeded or failed without depending on an open HTTP request body.

Risk level:

• Medium. The behavior change is internal but touches admission timing and async execution boundaries. It should be implemented in a focused prompt with existing HTTP integration tests extended first.

Runtime config and secret setup should have one app-level helper

Affected files/packages:

• internal/app/run.go
• internal/app/source_select.go
• internal/app/serve.go
• internal/app/backends.go
• internal/app/run_warnings.go
• internal/config
• internal/app/*_test.go

Duplicated or near-duplicated behavior:

• Defaulting an empty config path to config.DefaultConfigPath appears in Run, RunPipeline, RunPipelineWithLocalSource, and Serve.
• Config loading and secret loading are separate repeated steps in buildRunReport, selectSourceBundlesFromConfig, and Serve.
• Secret conflict warnings are projected in run and configured source diagnostics, while serve loads secrets without using or exposing conflict warning metadata.
• Backend factory construction from a config environment is repeated through provider plumbing.

Why it matters:

• Config and secret precedence is a public operational policy.
• A future change to config discovery, secret conflict reporting, or runtime environment construction could drift between run, serve, validate, and inspect.
• Tests for secrets and credential resolution need to cover several entrypoints today.

Recommended refactor:

• Add a small app-level runtime setup helper, for example loadRuntimeConfig(optionsConfigPath string) (runtimeConfig, error).
• The helper should own default config path selection, config.LoadFile, config.LoadSecretEnvironment, and conversion of secret conflicts into OutputWarning values.
• Keep config parsing and validation in internal/config; the helper should not duplicate config policy.
• Let run, configured validate/inspect, and serve call the helper and then apply command-specific behavior.

Suggested tests:

• One focused app test proving default config path selection remains unchanged where injection permits it.
• Existing secret conflict JSON/text warning tests for run, validate, and inspect.
• Serve startup test proving duplicate and missing upload tokens still fail without leaking values.
• S3 explicit credential tests proving the resolver is still used through the helper.

Risk level:

• Low to medium. This is a straightforward centralization but touches several command entrypoints.

Run destination processing needs a narrow helper boundary

Affected files/packages:

• internal/app/run.go
• internal/app/run_output.go
• internal/app/run_failures.go
• internal/app/run_selection.go
• internal/publish
• internal/app/run_test.go

Duplicated or near-duplicated behavior:

• Destination backend open failures and publish planning/execution failures each manually add runFailures, record summary failure counts, append RunActionRecord, and append pipeline event indexes.
• publish.Build error handling patches missing Plan identity fields inline before converting the plan to a run action.
• Fixed-path warning emission is interleaved with destination selection and publish plan handling.

Why it matters:

• run --format json depends on exact action ordering, warnings, partial failures, and summary counters.
• Future changes to actions, links, notifications, or HTTP upload reports could accidentally update one failure path but not another.
• The current loop is correct but dense enough that small behavior changes are hard to review.

Recommended refactor:

• Extract a narrow runDestination or destinationRunner helper that processes one destination and returns action records, warnings, summary deltas, and failures.
• Add a helper for recording a destination-scoped failure that updates runFailures, runSummary, RunReport.Actions, and pipeline events in one place.
• Add a helper that normalizes partial publish.Plan identity fields before action projection.
• Do not introduce a generic workflow engine or stage abstraction.

Suggested tests:

• Preserve existing run text output golden assertions.
• Preserve JSON partial-result behavior when planning fails after destination processing begins.
• Add one focused test where destination open fails for multiple selected bundles and verify action records, output errors, and summary counters stay aligned.
• Add one fixed-path dry-run warning test after extraction to verify event ordering.

Risk level:

• Medium. The refactor is behavior-preserving but touches the most important user-facing workflow.

Archive path validation duplicates source path policy with a different error surface

Affected files/packages:

• internal/ingest/archive.go
• pkg/bundle/path.go
• internal/storage/path.go
• internal/ingest/archive_test.go

Duplicated or near-duplicated behavior:

• cleanArchivePath, pkg/bundle.ValidateSourcePath, and storage.ValidatePath all enforce clean slash-separated relative paths with no backslashes, no absolute paths, and no dot segments.
• Archive staging needs slightly different policy because directories are allowed and manifest.json is allowed only at the root, so the duplication is not completely mechanical.

Why it matters:

• Path safety is high-risk behavior.
• Future changes to source path rules could miss archive extraction, especially around backslashes, reserved names, or dot segments.

Recommended refactor:

• Keep archive-specific rules in internal/ingest, but use a shared path-checking primitive where possible.
• A good shape is an exported pkg/bundle.ValidatePathSegmented only if it fits the public producer API, or an internal helper in ingestion that delegates file-entry validation to pkg/bundle.ValidateSourcePath for regular files after handling directory-specific exceptions.
• Preserve current archive-specific errors and tests.

Suggested tests:

• Table tests shared or mirrored across bundle path validation and archive path cleaning for absolute paths, traversal, backslashes, dot segments, empty names, root manifest.json, nested manifest.json, and .distributor.json.
• Regression tests proving directories are still accepted in archives but symlinks and hardlinks remain rejected.

Risk level:

• Low to medium. Path validation changes need careful tests, but the desired change can be small.

4. Medium-confidence opportunities

Source and destination backend config shapes could expose a normalized view

Affected files/packages:

• internal/config/config.go
• internal/config/defaults.go
• internal/config/validate.go
• internal/app/backends.go
• internal/config/*_test.go
• internal/app/backends_test.go

Duplicated or near-duplicated behavior:

• config.Backend and config.Destination duplicate backend fields for local, SSH, S3, and credentials.
• Defaults for source backends and destination backends are implemented in separate functions.
• App backend opening converts both shapes into backendOpenSpec.

Why it matters:

• New backend fields must be added to both YAML structs, defaulting paths, validation paths, app open-spec conversion, docs, and tests.
• The current pattern is easy to understand but likely to drift as more backend-specific fields are added.

Recommended refactor:

• Keep the YAML shape unchanged for compatibility.
• Add package-local helpers in internal/config that return a normalized backend view for either source or destination.
• Use that view for shared backend defaulting and validation where it improves clarity.
• Keep destination-only fields such as publish, transfer, links, and path_mapping on Destination.

Suggested tests:

• Existing source and destination backend validation tests should continue to pass.
• Add a table test that validates equivalent local, SSH, and S3 source/destination backend field requirements through the shared view.
• Add a test that http_upload remains source-only.

Risk level:

• Medium. This reduces future drift, but the current duplication is understandable and does not need to be the first cleanup.

CLI command scaffolding is mostly shared, but manifest create has special parsing

Affected files/packages:

• internal/cli/run.go
• internal/cli/serve.go
• internal/cli/source_mode.go
• internal/cli/manifest.go
• internal/cli/version.go
• internal/cli/root_test.go

Duplicated or near-duplicated behavior:

• Several commands repeat flag.NewFlagSet, SetOutput, help handling, format parsing, and usage exit handling.
• manifest create uses splitManifestCreateArgs to allow a positional bundle path before flags, unlike Go's default flag behavior.

Why it matters:

• CLI syntax and error behavior are public.
• A broad CLI helper could accidentally obscure command-specific parsing, but a narrow helper could reduce repeated setup and invalid-format handling.

Recommended refactor:

• Do not introduce a CLI framework.
• Consider a tiny helper for common FlagSet creation and output-format parsing after higher-value app/config cleanup.
• Keep manifest create custom parsing local unless another command needs the same interspersed positional behavior.

Suggested tests:

• Preserve current CLI usage-error tests.
• Add explicit tests for manifest create <path> --id x, manifest create --id x <path>, and invalid missing flag values before any parser cleanup.

Risk level:

• Low if kept narrow; medium if over-generalized.

Output DTOs repeat bundle metadata projection

Affected files/packages:

• internal/app/validate.go
• internal/app/inspect.go
• internal/app/manifest.go
• internal/app/run_output.go
• internal/app/output.go

Duplicated or near-duplicated behavior:

• inspect and manifest create both project bundle file metadata into command-specific JSON structs.
• validate, inspect, and manifest create each define local result types and file record types.
• RFC3339 formatting uses both time.RFC3339 and the equivalent literal layout string.

Why it matters:

• JSON output is now a public interface.
• Repeated projection can drift in field names, timestamp formatting, or path display rules.

Recommended refactor:

• Add a small app-local projection helper for bundle summaries and manifest file records.
• Use time.RFC3339 instead of literal RFC3339 layouts.
• Keep command-specific result structs where the command output semantics differ.

Suggested tests:

• JSON structural tests for validate, inspect, and manifest create before and after the helper.
• A timestamp-format assertion using an offset timestamp to confirm current behavior is preserved.

Risk level:

• Low.

PipelineRunCoordinator overlaps conceptually with UploadCoordinator

Affected files/packages:

• internal/app/run_coordinator.go
• internal/app/upload_coordinator.go
• docs/internal/app.md
• internal/app/run_coordinator_test.go
• internal/app/upload_coordinator_test.go

Duplicated or near-duplicated behavior:

• Both coordinators define run records, statuses, timestamps, status transitions, context handling, and active pipeline protection.
• The upload coordinator additionally queues, stages, expires status records, and serializes same-pipeline upload execution.

Why it matters:

• The concepts are similar enough to confuse future contributors.
• However, the behavior is not identical: one rejects duplicate active runs, while the other queues accepted uploads.

Recommended refactor:

• Do not merge the coordinators now.
• Review whether PipelineRunCoordinator is still needed as an exported app-level helper. If it is intended for future transports, document that role clearly. If not, remove it and its tests in a separate dead-code cleanup.
• If both remain, extract only tiny shared timestamp/status helpers if a real third coordinator appears.

Suggested tests:

• If retained, keep existing duplicate-run tests.
• If removed, run go test ./internal/app ./internal/cli and verify no current behavior depended on it.

Risk level:

• Low for documentation clarification, medium for removal because it is exported from an internal package and documented for maintainers.

5. Boundary and responsibility concerns

The major boundaries are sound:

• CLI parsing stays in internal/cli.
• Config defaults and validation stay in internal/config.
• Backend-specific filesystem, SFTP, and S3 behavior stays in adapters.
• Manifest semantics are centralized in pkg/bundle, with internal/bundle adding storage-backed discovery and validation.
• Destination state comparison stays in internal/state.
• Publish planning/execution stays in internal/publish.
• Transform implementation is behind internal/transform.

Concerns worth addressing:

• HTTP upload request-body staging currently crosses the app/ingest boundary. The app transport layer should not own body buffering and archive size enforcement beyond admission and HTTP status projection.
• Runtime config setup is app-layer behavior, but it is repeated rather than named. A runtime setup helper would clarify the boundary between internal/config and command-specific execution.
• internal/app/run.go owns too many destination-loop details. Extracting a destination processing helper would keep orchestration in app while reducing local complexity.

Recommended homes:

• Upload archive policy: internal/ingest.
• HTTP route/auth/status mapping: internal/app/upload_http.go.
• Queueing/status/execution: internal/app/upload_coordinator.go.
• Runtime config plus secret setup: a small helper in internal/app, using internal/config.
• Path and state filenames: keep in internal/storage.

6. Path, key, and naming construction review

Centralized and healthy areas:

• storage.StateFileName, storage.StatePath, storage.ManagedBundleTargets, storage.Join, storage.DisplayPath, and logical path validation are used in core publication and tests.
• S3 object-key mapping is contained in internal/adapters/s3.
• SSH and local native path conversion stay inside their adapters.
• Link URL construction is isolated in internal/publish/links.go and URL validation in internal/link.
• Manifest name and schema version are centralized in pkg/bundle, with internal/bundle aliases.

Areas needing cleanup:

• Archive path cleaning duplicates much of source/storage path policy and should either delegate to a shared primitive or be tightly covered by mirrored tests.
• Upload run ID construction is isolated, but the shape is partly policy. Keep tests around <pipeline_id>.<utc_timestamp>.<random_suffix> before changing coordinator code.
• Some app tests still construct destination state and source paths locally. internal/testutil already covers many cases; additional helper use should be opportunistic, not a sweeping test rewrite.

7. Resolution and catalog review

Named concept resolution is mostly consistent:

• Backend names are defined in internal/config/defaults.go.
• Runtime backend construction is app-owned through backendFactory and the storage registry.
• Transform names are defined in internal/transform, and app wiring owns concrete registration.
• Publish/transform policy combinations use config.ValidatePublishTransformPolicy.
• Configured source selection for validate and inspect is shared in source_select.go.

Potential refinements:

• A normalized backend config view would make backend field resolution less repetitive across source and destination config.
• Transform and backend registries should remain separate; there is no evidence that a generic registry abstraction would help.
• No separate catalog package is needed for the current feature set.

8. Config and command-loading review

Config loading is reliable and strict:

• YAML known-field checking is enabled.
• Defaults are applied before validation.
• Validation collects multiple field errors.
• Secrets are loaded without mutating os.Environ.
• Explicit S3 credential references use the config-owned resolver.

Likely accidental duplication:

• Default config path selection and config.LoadFile are repeated in several app entrypoints.
• Secret loading is repeated in run, source diagnostics, and serve.
• Secret conflict warning projection is not represented by one runtime setup result.

Intentional differences:

• serve loads upload tokens and does not produce CLI JSON output.
• validate and inspect support local-path shortcut mode, while run and serve are config-driven.
• manifest create is local filesystem producer tooling and does not load app config.

Recommended cleanup:

• Centralize runtime config and secret setup in internal/app.
• Keep CLI flag parsing local to command files.
• Keep manifest create outside runtime config loading.

9. State, manifest, or progress handling review

Manifest handling is in good shape:

• pkg/bundle owns manifest parsing, digest grammar, source path validation, canonical bundle digest, local manifest building, and local bundle writing.
• internal/bundle delegates normalized manifest semantics to pkg/bundle and adds storage-backed validation.
• Destination state embeds the normalized manifest and validates through internal/bundle/pkg/bundle.

State handling is in good shape:

• .distributor.json parsing, validation, JSON projection, and comparison live in internal/state.
• Publish execution writes destination state only after outputs are written.
• Managed replacement deletes only state-listed outputs plus .distributor.json; forced replacement is explicit and bounded.

Progress/status handling:

• RunReport is the core run result model and supports JSON partial-result output.
• HTTP upload status is memory-only and documented as such.
• PipelineRunCoordinator and UploadCoordinator overlap conceptually but have different policies. Avoid merging unless product behavior converges.

Gaps:

• HTTP upload staging currently stores the request body in memory before queueing. This is both a quality gap and a mismatch with the intended ingestion boundary.
• There is no durable upload status, but this is documented as deferred work and should not be addressed in cleanup.

10. Refactors to avoid

Avoid these changes in the cleanup pass:

• Do not introduce a generic workflow engine or stage framework. The current explicit workflow is easier to audit.
• Do not add a CLI framework. The standard-library CLI is sufficient and policy-approved.
• Do not merge local, SSH, S3, and fake adapters behind a shared implementation layer. Their semantics differ enough that generic helpers would likely hide important behavior.
• Do not collapse pkg/bundle and internal/bundle. The public producer API and storage-backed distributor validation have different responsibilities.
• Do not move destination state comparison into publish or app orchestration.
• Do not redesign JSON output envelopes while doing cleanup.
• Do not add durable queues, retry workers, HTTP TLS, zstd, or browser UI under the banner of refactoring. These are feature work.
• Do not rewrite tests wholesale to use a new fixture system. Add helpers only where they reduce immediate duplication around changed code.

11. Recommended implementation sequence

1. HTTP upload staging boundary cleanup.

   • Move supported content-type policy to internal/ingest.
   • Stop buffering accepted uploads in upload_http.go.
   • Queue staged bundle roots rather than request bodies.
   • Extend HTTP upload tests first.

2. Runtime config setup helper.

   • Add an app-level helper for default config path, config load, secret load, environment resolver, and secret warnings.
   • Use it from run, configured validate/inspect, and serve where applicable.
   • Preserve command-specific behavior.

3. Run destination processing extraction.

   • Add small helpers for destination-scoped failure recording and plan identity normalization.
   • Extract one-destination processing only if the helper remains readable.
   • Preserve action ordering and report output.

4. Backend config normalized view.

   • Add source/destination backend view helpers in internal/config.
   • Use them for defaulting and validation if tests show the shape remains clear.
   • Keep YAML structs and public config unchanged.

5. Bundle output projection cleanup.

   • Add app-local helpers for file record and bundle summary projection.
   • Use time.RFC3339 consistently.
   • Preserve command-specific JSON field names.

6. Archive/source path validation test alignment.

   • Add mirrored path safety tests around ingestion and bundle path validation.
   • Only centralize code if the helper does not blur archive directory semantics.

7. Coordinator intent cleanup.

   • Decide whether PipelineRunCoordinator is retained for internal future use.
   • If retained, clarify comments/docs. If removed, do it as a separate dead-code commit.

8. Test helper cleanup.

   • Expand internal/testutil only for repeated setup touched by the previous refactors.
   • Avoid moving every test fixture.

12. Test strategy

Tests to add before refactoring:

• HTTP upload handler test proving queue-full rejection does not consume the body.
• HTTP upload test proving accepted upload staging completes before 202 Accepted.
• Ingestion content-type policy tests exposed through the new helper.
• Run report test covering destination open failure for multiple selected bundles.
• CLI JSON tests for inspect and manifest create timestamp formatting before projection cleanup.

Tests to run with each cleanup stage:

• HTTP upload cleanup: go test ./internal/ingest ./internal/app ./internal/cli
• Config setup cleanup: go test ./internal/config ./internal/app ./internal/cli
• Run processing cleanup: go test ./internal/app ./internal/publish ./internal/state
• Backend config view cleanup: go test ./internal/config ./internal/app
• Output projection cleanup: go test ./internal/app ./internal/cli
• Path validation cleanup: go test ./pkg/bundle ./internal/bundle ./internal/ingest ./internal/storage
• Final cleanup validation: go test ./...

Useful read-only checks:

• rg -n "LoadFile\\(|LoadSecretEnvironment\\(|DefaultConfigPath" internal/app internal/cli
• rg -n "application/x-tar|application/gzip|application/x-gzip" internal docs
• rg -n "2006-01-02T15:04:05Z07:00" internal pkg
• rg -n "manifest.json|\\.distributor.json|StatePath|DisplayPath" internal pkg

13. Appendix: findings not worth acting on

Adapter ReadFile and WriteFile wrappers:

• Local, SSH, S3, and fake backends each implement byte helpers in terms of stream helpers. This is small duplication but appropriate because each adapter owns error translation and metadata semantics.

Adapter traversal implementation:

• Local filesystem walking, SFTP walking, and S3 pagination look similar at the interface level but are semantically different. Keep traversal mechanics in adapters and shared callback behavior in storage.WalkEmitter.

State and manifest raw JSON parsing:

• pkg/bundle and internal/state both parse raw JSON with pointer fields to detect missing required fields. The schemas and error contexts differ, so a generic required-field parser would not be worth the complexity.

CLI help text:

• Help text repeats command names and flags. This is acceptable in a small hand-written CLI and keeps command files readable.

Test fixture strings:

• Some tests inline YAML snippets or expected output strings despite internal/testutil. Inline data is often clearer for edge cases. Only centralize fixture setup when tests are already being changed for a behavior-preserving refactor.

HTTP JSON response helpers:

• HTTP API responses use simple JSON objects rather than the CLI JSON envelope. This is intentional because HTTP status codes and route-specific responses are not the same public interface as CLI command output.

Public and internal bundle validation:

• pkg/bundle.ValidateBundle is local-filesystem producer validation; internal/bundle.Validate is storage-backed distributor validation. Keep both, with shared manifest semantics delegated through pkg/bundle.