# 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 duplicate-run coordinator to avoid maintaining two
similar coordination concepts.

Implementation scope:

- Delete the duplicate-run coordinator, its run record and duplicate-run error
  types, 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 search that production code does not reference
  the duplicate-run coordinator constructor, type, or error.

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`
- Search `internal` and `docs` for the removed duplicate-run coordinator symbols;
  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 "duplicate-run coordinator" 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.