Create a cleanup roadmap to address the items identified in the audit

docs/roadmap/cleanup.md

@@ -0,0 +1,400 @@
+# Code Quality Cleanup Roadmap
+
+## Current Baseline
+
+The codebase has completed the local, SSH/SFTP, S3, public bundle package,
+manifest creation, JSON output, path mapping, link generation, and HTTP upload
+work documented in the current user and internal docs.
+
+The audit in `docs/roadmap/audit.md` found no major architectural risk. The
+remaining cleanup work should be narrow, behavior-preserving, and focused on
+reducing drift in upload staging, runtime config setup, run reporting, backend
+config handling, output projection, path validation tests, and internal
+coordination code.
+
+One intentional behavior change is part of this cleanup roadmap: malformed
+authenticated upload archives should be rejected before `202 Accepted`, rather
+than accepted and later marked failed. Valid staged uploads should still run
+asynchronously after admission.
+
+## Cleanup Principles
+
+- Preserve public CLI behavior, config schema, manifest schema, destination
+  state schema, backend behavior, and JSON envelopes unless a stage explicitly
+  says otherwise.
+- Keep config parsing and validation in `internal/config`.
+- Keep CLI parsing in `internal/cli`.
+- Keep upload archive policy in `internal/ingest`; keep HTTP routing,
+  authentication, and status projection in `internal/app`.
+- Keep backend-specific filesystem, SSH/SFTP, and S3 behavior in adapter
+  packages.
+- Prefer small package-local helpers over broad abstractions.
+- Add or strengthen tests before refactoring behavior that affects public
+  output, upload admission, path safety, or run reporting.
+
+## Active Cleanup Stages
+
+Implement these stages in order. Each stage should be small enough for one
+implementation prompt and should leave the repository passing the listed focused
+tests before moving to the next stage.
+
+## Stage 1: HTTP Upload Staging Boundary
+
+Goal:
+
+Move archive validation and upload body staging fully behind `internal/ingest`,
+stop app-layer full-body buffering, and reject malformed archives before
+returning `202 Accepted`.
+
+Implementation scope:
+
+- Add an ingestion-owned content-type helper, such as
+  `ValidateContentType(contentType string) error`, and remove duplicated
+  content-type policy from the HTTP handler.
+- Replace the current handler-side `readUploadBody` buffering with streaming
+  staging through `internal/ingest`.
+- Introduce a two-step upload coordinator admission model:
+  - reserve a run id and queue slot before consuming the request body;
+  - stage and validate the archive using that reserved run id;
+  - enqueue only a successfully staged local bundle root for async execution.
+- Keep queue-full rejection before reading the body.
+- Preserve `401` for missing or invalid bearer tokens, `415` for unsupported
+  content type, `413` for oversized uploads, and `503` for a full queue.
+- Return a pre-acceptance `400` for malformed tar/gzip content or invalid
+  staged bundles.
+- Preserve async queued/running/succeeded/failed status after a valid staged
+  bundle is accepted.
+- Do not add durable queues, idempotency keys, zstd, or new routes.
+
+Current-behavior documentation updates:
+
+- Update `docs/cli.md`, `docs/config.md`, `docs/operations.md`,
+  `docs/troubleshooting.md`, `docs/internal/app.md`, and
+  `docs/internal/ingest.md` only as needed to describe the new
+  pre-acceptance failure boundary.
+
+Tests:
+
+- `go test ./internal/ingest ./internal/app ./internal/cli`
+- Queue-full upload rejection does not read the request body.
+- Unsupported content type is rejected through ingestion-owned policy.
+- Oversized uploads return `413` and do not retain a staged run.
+- Malformed tar/gzip content returns `400` before a run id is issued.
+- Valid tar and tar.gz uploads return `202` after staging and still transition
+  through async status.
+- HTTP responses and status records do not leak bearer tokens or secret values.
+
+Completion criteria:
+
+- `internal/app` no longer buffers the full upload body before staging.
+- A valid accepted upload has a committed staged bundle root before the `202`
+  response is sent.
+- Invalid archive content cannot create an accepted run id.
+
+## Stage 2: Runtime Config And Secret Setup Helper
+
+Goal:
+
+Centralize runtime config path resolution, config loading, secret loading,
+environment resolver creation, and secret-conflict warning projection in one
+app-layer helper.
+
+Implementation scope:
+
+- Add a small `internal/app` runtime setup helper that:
+  - defaults an empty config path to `config.DefaultConfigPath`;
+  - calls `config.LoadFile`;
+  - calls `config.LoadSecretEnvironment`;
+  - exposes the loaded config, config path, `config.Environment`, and
+    `[]OutputWarning` for secret conflicts.
+- Use the helper from `Run`, `RunPipeline`, `RunPipelineWithLocalSource`,
+  configured `Validate`/`Inspect`, and `Serve` where applicable.
+- Keep `manifest create` outside runtime config loading.
+- Keep YAML structs, defaults, validation, and secret-directory parsing in
+  `internal/config`.
+- Preserve app test injection points for backend factories and upload handler
+  tests.
+
+Current-behavior documentation updates:
+
+- Update `docs/internal/app.md` if helper boundaries or flow descriptions
+  change. User-facing docs should not change unless observable behavior changes.
+
+Tests:
+
+- `go test ./internal/config ./internal/app ./internal/cli`
+- Default config path behavior remains unchanged.
+- Secret conflict warnings still appear in text and JSON output for `run`,
+  configured `validate`, and configured `inspect`.
+- `serve` still fails startup safely for missing, empty, or duplicate upload
+  tokens without leaking values.
+- Explicit S3 credential references still resolve through the config-owned
+  environment resolver.
+
+Completion criteria:
+
+- Runtime commands no longer repeat config path defaulting and secret loading.
+- Config policy remains owned by `internal/config`.
+
+## Stage 3: Run Destination Processing Extraction
+
+Goal:
+
+Reduce complexity in the main run loop while preserving run report behavior,
+warning ordering, action ordering, failure aggregation, and text/JSON output.
+
+Implementation scope:
+
+- Extract narrow helpers from `internal/app/run.go` for destination-scoped
+  processing.
+- Centralize destination-scoped failure recording so one helper updates
+  `runFailures`, `runSummary`, `RunReport.Actions`, and pipeline events.
+- Centralize normalization of partial `publish.Plan` identity fields before
+  converting plans to run action records.
+- Keep app orchestration explicit; do not introduce a generic workflow engine,
+  stage framework, or broad runner abstraction.
+- Preserve independent destination fan-out and partial-result behavior.
+
+Current-behavior documentation updates:
+
+- Update `docs/internal/app.md` only if helper names or package layout
+  descriptions materially change.
+
+Tests:
+
+- `go test ./internal/app ./internal/publish ./internal/state`
+- Destination open failures for multiple selected bundles keep action records,
+  output errors, summary counters, and pipeline events aligned.
+- JSON partial-result output remains unchanged when destination planning or
+  execution fails after a report exists.
+- Fixed-path dry-run warnings appear in the same order as before.
+- Existing run text output assertions continue to pass.
+
+Completion criteria:
+
+- `run.go` delegates destination-scoped record/failure bookkeeping to helpers.
+- No public output shape or ordering changes.
+
+## Stage 4: Backend Config Normalized View
+
+Goal:
+
+Reduce source/destination backend config drift while preserving the current YAML
+schema and public config behavior.
+
+Implementation scope:
+
+- Add package-local normalized backend view helpers in `internal/config` for
+  source and destination backend fields.
+- Use the normalized view to reduce duplication in backend defaulting and
+  validation where it remains clearer than the current paired code.
+- Keep `config.Backend` and `config.Destination` YAML structs and tags
+  unchanged.
+- Preserve destination-only policy fields on `Destination`.
+- Preserve `http_upload` as source-only and invalid for destinations.
+- Update app backend opening only if the normalized view provides clearer
+  handoff without leaking config internals.
+
+Current-behavior documentation updates:
+
+- None expected unless internal docs mention the old paired implementation
+  shape in a way that becomes misleading.
+
+Tests:
+
+- `go test ./internal/config ./internal/app`
+- Equivalent local, SSH, and S3 source/destination validation remains
+  consistent.
+- Defaults for SSH port/host key policy, S3 region/prefix/force-path-style, and
+  HTTP upload staging fields remain unchanged.
+- `http_upload` remains valid only for sources.
+
+Completion criteria:
+
+- Adding a future backend field has one obvious defaulting/validation path.
+- Public config files and examples continue to load unchanged.
+
+## Stage 5: Command Output Projection Cleanup
+
+Goal:
+
+Reduce drift in bundle and file metadata projection for app command JSON
+results.
+
+Implementation scope:
+
+- Add small app-local projection helpers for bundle summaries and manifest file
+  records used by `validate`, `inspect`, and `manifest create`.
+- Use `time.RFC3339` consistently instead of equivalent literal layouts.
+- Preserve existing JSON envelope fields, command names, command-specific result
+  field names, text output, and fatal error behavior.
+- Do not redesign CLI JSON output or HTTP JSON responses.
+
+Current-behavior documentation updates:
+
+- None expected unless tests reveal current docs are stale.
+
+Tests:
+
+- `go test ./internal/app ./internal/cli`
+- JSON output for `validate`, `inspect`, and `manifest create` remains
+  structurally stable.
+- RFC3339 timestamps remain unchanged, including offset-preserving source
+  timestamps where current behavior preserves them.
+- Text output remains unchanged.
+
+Completion criteria:
+
+- Bundle/file projection logic is shared where semantics match.
+- Command-specific result structs remain easy to read.
+
+## Stage 6: Archive And Source Path Validation Alignment
+
+Goal:
+
+Protect path safety by aligning archive path tests with source and storage path
+policy, without blurring archive-specific rules.
+
+Implementation scope:
+
+- Add mirrored path-safety table tests around `internal/ingest`, `pkg/bundle`,
+  `internal/bundle`, and `internal/storage` where useful.
+- Keep archive-specific directory handling, root-level manifest rules, duplicate
+  file rejection, symlink rejection, hardlink rejection, and special-entry
+  rejection in `internal/ingest`.
+- Centralize code only if the helper can preserve clear archive semantics and
+  current error behavior.
+- Do not add new public `pkg/bundle` APIs unless the existing public API cannot
+  safely support the needed shared behavior.
+
+Current-behavior documentation updates:
+
+- None expected unless implementation changes error boundaries or internal
+  package descriptions.
+
+Tests:
+
+- `go test ./pkg/bundle ./internal/bundle ./internal/ingest ./internal/storage`
+- Absolute paths, traversal, backslashes, empty paths, dot segments, nested
+  manifests, `.distributor.json` handling, symlinks, hardlinks, devices, and
+  sockets remain covered.
+- Archive directories remain accepted where safe.
+
+Completion criteria:
+
+- Path safety policy has regression coverage across archive staging, source
+  bundle validation, and storage logical path validation.
+- Any code sharing is smaller and clearer than the duplicated logic it replaces.
+
+## Stage 7: Pipeline Run Coordinator Removal
+
+Goal:
+
+Remove the currently unused internal `PipelineRunCoordinator` to avoid
+maintaining two similar coordination concepts.
+
+Implementation scope:
+
+- Delete `PipelineRunCoordinator`, `PipelineRunRecord`,
+  `DuplicatePipelineRunError`, related helpers, and their tests.
+- Remove or rewrite `docs/internal/app.md` sections that describe the removed
+  coordinator.
+- Keep `UploadCoordinator`; do not merge upload queueing with the removed
+  duplicate-run coordinator.
+- Before deletion, confirm with `rg` that production code does not reference
+  `NewPipelineRunCoordinator`, `PipelineRunCoordinator`, or
+  `DuplicatePipelineRunError`.
+
+Current-behavior documentation updates:
+
+- Update `docs/internal/app.md` because it currently documents the coordinator
+  as an internal implemented component.
+
+Tests:
+
+- `go test ./internal/app ./internal/cli`
+- `rg -n "PipelineRunCoordinator|NewPipelineRunCoordinator|DuplicatePipelineRunError" internal docs`
+  should show no stale references after removal.
+
+Completion criteria:
+
+- No production, test, or internal documentation references remain for the
+  removed coordinator.
+- Upload coordination behavior is unchanged.
+
+## Stage 8: Narrow CLI And Test Helper Cleanup
+
+Goal:
+
+Apply only low-risk CLI setup and test fixture cleanup that remains useful after
+the earlier stages.
+
+Implementation scope:
+
+- Add tiny CLI helpers for repeated `flag.FlagSet` setup or output-format
+  parsing only where command behavior remains obvious.
+- Keep the standard-library CLI; do not introduce a CLI framework.
+- Keep `manifest create` interspersed positional parsing local unless another
+  command now needs the same parsing behavior.
+- Expand `internal/testutil` only for repeated setup touched by earlier stages.
+- Do not rewrite tests wholesale just to use shared helpers.
+
+Current-behavior documentation updates:
+
+- None expected unless CLI help or syntax changes. This stage should avoid such
+  changes.
+
+Tests:
+
+- `go test ./internal/cli ./internal/app`
+- CLI usage-error tests remain stable.
+- `manifest create <path> --id x`, `manifest create --id x <path>`, missing
+  flag values, invalid `--format`, and help output remain covered.
+
+Completion criteria:
+
+- Remaining CLI/test cleanup is small, readable, and behavior-preserving.
+- No public CLI syntax or output changes.
+
+## Refactors To Avoid
+
+- Do not introduce a generic workflow engine or stage framework.
+- Do not add a CLI framework.
+- Do not merge local, SSH, S3, and fake backend adapter implementations.
+- Do not collapse `pkg/bundle` and `internal/bundle`.
+- Do not move destination state comparison into `internal/publish` or
+  `internal/app`.
+- Do not redesign CLI JSON envelopes.
+- Do not change HTTP JSON response shapes except where Stage 1 requires
+  pre-acceptance error behavior.
+- Do not add durable upload queues, retry workers, zstd support, in-app TLS,
+  idempotency keys, browser UI, or other feature work.
+- Do not rewrite tests wholesale to use new fixture helpers.
+
+## Validation
+
+After each implementation stage, run the stage-specific tests listed above.
+
+After all cleanup stages:
+
+```sh
+go test ./...
+```
+
+Recommended consistency checks:
+
+```sh
+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 "PipelineRunCoordinator|NewPipelineRunCoordinator|DuplicatePipelineRunError" internal docs
+```
+
+The cleanup is complete when:
+
+- all staged tests and `go test ./...` pass;
+- current-behavior docs describe the implemented Stage 1 upload failure
+  boundary;
+- `docs/roadmap/audit.md` findings have either been addressed or consciously
+  left in place as noted in this cleanup roadmap;
+- no completed cleanup behavior is documented only as future work.