From 6d409fb4bd5a735dc83e5e4393d1da375c9b470a Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 3 Jun 2026 18:46:10 -0500 Subject: [PATCH] Audit code quality and deduplication opportunities --- docs/roadmap/audit.md | 592 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 592 insertions(+) create mode 100644 docs/roadmap/audit.md diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..3920cf4 --- /dev/null +++ b/docs/roadmap/audit.md @@ -0,0 +1,592 @@ +# 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. + +### 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 `..` 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`.