From 1340418a2b9fbcfd8e60fee57bf1e7bcba0daaf0 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 3 Jun 2026 19:07:47 -0500 Subject: [PATCH] Create a cleanup roadmap to address the items identified in the audit --- docs/roadmap/cleanup.md | 400 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 400 insertions(+) create mode 100644 docs/roadmap/cleanup.md diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md new file mode 100644 index 0000000..df910c4 --- /dev/null +++ b/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 --id x`, `manifest create --id x `, 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.