# 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 --id x`, `manifest create --id x `, 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. ### Duplicate-run coordination overlaps conceptually with upload coordination Affected files/packages: - `internal/app/upload_coordinator.go` - `docs/internal/app.md` - `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 duplicate-run coordination 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 `..` 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. - Duplicate-run coordination and upload coordination 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 duplicate-run coordination 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`.