Audit code quality and deduplication opportunities
This commit is contained in:
512
docs/roadmap/audit.md
Normal file
512
docs/roadmap/audit.md
Normal file
@@ -0,0 +1,512 @@
|
||||
# Code Quality And Deduplication Audit
|
||||
|
||||
## Executive summary
|
||||
|
||||
Overall code quality is strong for the current project stage. The repository generally follows the documented architecture: CLI parsing stays in `internal/cli`, orchestration stays in `internal/app`, storage behavior is behind `internal/storage`, adapters are thin enough for their protocols, and source manifest semantics are mostly centralized through `pkg/bundle`.
|
||||
|
||||
The top three cleanup targets are:
|
||||
|
||||
1. Backend open-config construction in `internal/app/backends.go`.
|
||||
2. HTTP URL validation shared by config and destination state.
|
||||
3. Configured-source command scaffolding for `validate` and `inspect`.
|
||||
|
||||
The codebase appears ready for a limited cleanup pass. I did not find a major architectural risk or evidence that a broad rewrite is warranted. The best follow-up work is a sequence of small, behavior-preserving refactors with focused tests.
|
||||
|
||||
This report belongs in `docs/roadmap/audit.md` because it describes future cleanup opportunities rather than implemented behavior.
|
||||
|
||||
## Repository map reviewed
|
||||
|
||||
Reviewed documentation:
|
||||
|
||||
- `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/app.md`
|
||||
- `docs/internal/bundle.md`
|
||||
- `docs/internal/config.md`
|
||||
- `docs/internal/publish.md`
|
||||
- `docs/internal/state.md`
|
||||
- `docs/internal/storage.md`
|
||||
- `docs/internal/transform.md`
|
||||
- `docs/integrations/markdown.md`
|
||||
- `docs/roadmap/implementation.md`
|
||||
|
||||
Reviewed implementation areas:
|
||||
|
||||
- `cmd/distributor`
|
||||
- `pkg/bundle`
|
||||
- `internal/app`
|
||||
- `internal/cli`
|
||||
- `internal/config`
|
||||
- `internal/bundle`
|
||||
- `internal/state`
|
||||
- `internal/storage`
|
||||
- `internal/storage/fake`
|
||||
- `internal/adapters/local`
|
||||
- `internal/adapters/ssh`
|
||||
- `internal/adapters/s3`
|
||||
- `internal/publish`
|
||||
- `internal/transform`
|
||||
- `internal/transform/markdown`
|
||||
- `internal/notify`
|
||||
- `internal/testutil`
|
||||
- `examples`
|
||||
|
||||
Major execution paths reviewed:
|
||||
|
||||
- `run` config load, source discovery, destination selection, publish planning, dry-run, execution, summary, JSON output, and notification handoff.
|
||||
- `validate` and `inspect` in local-path and configured-source modes.
|
||||
- `manifest create`.
|
||||
- Local, SSH/SFTP, S3, and fake storage behavior.
|
||||
- Source manifest validation, destination state comparison, Markdown-to-HTML output planning, link generation, and replacement safety.
|
||||
|
||||
No important repository areas were deliberately skipped. There is no `internal/stage`, `internal/modules`, `internal/validators`, `internal/artifacts`, `internal/manifest`, `internal/schema`, or `internal/report` tree in this repository, so those requested audit categories were mapped to the actual bundle, state, config, publish, transform, storage, and CLI packages.
|
||||
|
||||
## High-confidence deduplication opportunities
|
||||
|
||||
### 1. Centralize backend open-config construction
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/app/backends.go`
|
||||
- `internal/config/config.go`
|
||||
- `internal/config/defaults.go`
|
||||
- `internal/adapters/ssh/options.go`
|
||||
- `internal/adapters/s3/options.go`
|
||||
- `internal/app/backends_test.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
`internal/app/backends.go` has parallel source and destination conversion paths. `sourceOpenConfig` and `destinationOpenConfig` both copy `path`, SSH host/user/port/key/known-hosts/host-key-policy/read-only fields into `storage.OpenConfig` with nearly the same code (`internal/app/backends.go:123`, `internal/app/backends.go:136`, `internal/app/backends.go:166`, `internal/app/backends.go:180`). S3 fields are centralized only partially through `addS3Config` (`internal/app/backends.go:149`).
|
||||
|
||||
Why it matters:
|
||||
|
||||
Source and destination backend shapes are intentionally similar. Any new executable backend field, credential policy, SSH option, S3 option, or dry-run mutation flag must be added to two mapping paths. That creates a realistic drift risk between `run` and configured-source `validate`/`inspect`, because all three paths share source opening but destinations have a separate config struct.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
Add a small app-local backend spec adapter, for example:
|
||||
|
||||
- `backendOpenSpec` with backend name, path, SSH fields, S3 fields, credentials, and role label.
|
||||
- `openConfigForBackend(spec, readOnlyKnownHosts, environment)`.
|
||||
- thin constructors such as `sourceBackendSpec(config.Backend)` and `destinationBackendSpec(config.Destination)`.
|
||||
|
||||
Keep this in `internal/app`; do not move concrete adapter construction into `internal/config`. Config should continue to own YAML shape, defaults, and validation only.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Update `internal/app/backends_test.go` to assert one shared source/destination conversion table for local, SSH, S3, explicit S3 credentials, and dry-run known-host behavior.
|
||||
- Add a regression test that `validate` configured-source mode and `run` source opening produce the same SSH/S3 `storage.OpenConfig` for equivalent source config.
|
||||
- Keep adapter-specific tests unchanged.
|
||||
|
||||
Risk level:
|
||||
|
||||
Low to medium. The refactor touches runtime backend opening, but the behavior can be covered with existing backend factory tests and fake backends.
|
||||
|
||||
### 2. Share HTTP URL validation rules
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/config/validate.go`
|
||||
- `internal/state/validate.go`
|
||||
- `internal/publish/links.go`
|
||||
- `internal/config/validate_test.go`
|
||||
- `internal/state/distributor_test.go`
|
||||
- `internal/publish/links_test.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
`validateLinkBaseURL` validates `http`/`https`, host presence, no query string, and no fragment for `links.base_url` (`internal/config/validate.go:201`). `validateStateURL` repeats the same scheme, host, query, and fragment checks for `links.primary_url` and output URLs (`internal/state/validate.go:86`). `publish.OutputURL` parses and mutates a URL independently while assuming config already enforced the base URL shape (`internal/publish/links.go:28`).
|
||||
|
||||
Why it matters:
|
||||
|
||||
These values are related parts of the same public URL contract. If URL policy changes, for example allowing fragments in one place or requiring canonical path handling, config validation and persisted state validation could drift. State validation also returns a raw parse error while config wraps parse failures as `must be a valid URL`, so user-facing and operator-facing diagnostics can diverge.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
Create a small internal URL policy helper in the narrowest suitable package. Two reasonable homes are:
|
||||
|
||||
- `internal/state`, if state remains the canonical persisted URL contract and config calls a state helper would be acceptable; or
|
||||
- a new small package such as `internal/link` if avoiding a config-to-state dependency is preferred.
|
||||
|
||||
The helper should validate absolute HTTP(S) URLs without query or fragment, and optionally expose a base-url-specific wrapper for config error wording. `publish.OutputURL` can continue to build URLs, but should use the same parser/validator for base URLs before mutation.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- One table-driven URL policy test for valid `http`, valid `https`, missing host, unsupported scheme, query, fragment, and parse failure.
|
||||
- Keep config and state tests focused on field context and wiring.
|
||||
- Keep `publish/links_test.go` for path semantics and `index.html` directory-style URLs.
|
||||
|
||||
Risk level:
|
||||
|
||||
Low. This is a small policy centralization with existing tests in all affected packages.
|
||||
|
||||
### 3. Share configured-source CLI scaffolding for `validate` and `inspect`
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/cli/validate.go`
|
||||
- `internal/cli/inspect.go`
|
||||
- `internal/cli/source_mode.go`
|
||||
- `internal/app/source_select.go`
|
||||
- `internal/cli/root_test.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
`validateCommand` and `inspectCommand` define the same `--config`, `--pipeline`, `--bundle`, and `--format` flags; parse output format the same way; parse at most one optional path; call the same `validateInspectModeOK`; and then construct nearly identical app options (`internal/cli/validate.go:17`, `internal/cli/inspect.go:17`). The app layer already centralizes the actual source selection in `selectSourceBundles` and `selectSourceBundlesFromConfig` (`internal/app/source_select.go:28`, `internal/app/source_select.go:60`).
|
||||
|
||||
Why it matters:
|
||||
|
||||
These commands intentionally support the same two modes. Future changes to configured source mode, such as a new source-only diagnostic flag or error wording, would need coordinated edits in both command files and CLI tests.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
Keep command entrypoints explicit, but introduce a small `parseSourceCommandFlags(command, args, stderr)` helper returning `{path, configPath, pipelineID, bundlePath, outputFormat}`. `validateCommand` and `inspectCommand` would then differ only in help text and the app function they call.
|
||||
|
||||
Do not introduce a CLI framework or a generic command engine.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Keep command-specific smoke tests.
|
||||
- Add or preserve one table covering local path, too many paths, config without pipeline, pipeline without config, bundle without config, path plus config, invalid format, and JSON format for both commands.
|
||||
|
||||
Risk level:
|
||||
|
||||
Low. This is confined to CLI parsing and has strong existing tests.
|
||||
|
||||
### 4. Centralize publish output metadata projection
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/publish/output.go`
|
||||
- `internal/publish/execute.go`
|
||||
- `internal/app/run.go`
|
||||
- `internal/notify/notify.go`
|
||||
- `internal/state/distributor.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
The same publish output metadata is projected into several destination structs:
|
||||
|
||||
- `stateOutputs` maps `publish.Output` to `state.OutputFile` (`internal/publish/output.go:100`).
|
||||
- `managedOutputPaths` and `existingManagedOutputPaths` extract output paths (`internal/publish/output.go:116`, `internal/publish/execute.go:106`).
|
||||
- `runOutputsFromPlan` maps `publish.Output` to JSON output records (`internal/app/run.go:542`).
|
||||
- `notifyEvent` maps `publish.Output` to notification output records (`internal/app/run.go:434`).
|
||||
|
||||
Why it matters:
|
||||
|
||||
Output metadata is a central contract: path, kind, source path, transform, URL, digest, and size appear in destination state, JSON output, and notifications. Adding or renaming an output metadata field would require edits in several projection functions. A missed update could make state, JSON, and notification output disagree.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
Do not force all external output shapes into a single struct, because JSON/state/notification schemas are not identical. Instead, add small package-local helpers with clear names, for example:
|
||||
|
||||
- `publish.Output.StateFile() state.OutputFile`
|
||||
- `publish.Output.ManagedPath() string`
|
||||
- possibly `publish.Output.PublicMetadata()` if app and notify projections grow.
|
||||
|
||||
Keep app-owned JSON result structs in `internal/app`, but reduce repeated field-by-field copying where the destination schema is identical enough.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Add focused publish tests for output-to-state conversion, including URL and generated transform fields.
|
||||
- Add an app JSON regression test that generated outputs include `path`, `kind`, `source_path`, `transform`, `url`, `sha256`, and `size`.
|
||||
- Add a notification mapping test for generated output metadata.
|
||||
|
||||
Risk level:
|
||||
|
||||
Low. The refactor is mechanical but should be covered because it touches user-visible JSON and destination state.
|
||||
|
||||
## Medium-confidence opportunities
|
||||
|
||||
### 1. Extract shared storage walk emission behavior carefully
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/adapters/local/backend.go`
|
||||
- `internal/adapters/ssh/backend.go`
|
||||
- `internal/adapters/s3/backend.go`
|
||||
- `internal/storage/backend.go`
|
||||
- `internal/storage/fake/backend.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
Local, SSH, and S3 backends each implement an inner `emit` function that checks context, applies `WalkOptions.Limit`, increments a visit counter, handles `storage.ErrStopWalk`, and wraps callback errors as storage walk errors (`internal/adapters/local/backend.go:164`, `internal/adapters/ssh/backend.go:212`, `internal/adapters/s3/backend.go:186`). Each backend also implements `HasAny` as `Walk(... Limit: 1)` (`internal/adapters/local/backend.go:221`, `internal/adapters/ssh/backend.go:247`, `internal/adapters/s3/backend.go:225`).
|
||||
|
||||
Why it matters:
|
||||
|
||||
Walk semantics are part of the storage contract. A change to limit behavior or `ErrStopWalk` handling would need repeated edits across adapters. Fake backend semantics already differ slightly because it handles `ErrStopWalk` in its own loop.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
Consider a tiny storage helper that wraps emission state, such as `storage.NewWalkEmitter(ctx, backendName, opts, fn)`, plus a default `storage.HasAny(ctx, backend, prefix)` helper. Keep traversal mechanics in adapters. Avoid extracting directory walking, SFTP behavior, S3 pagination, or deletion logic.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Shared storage contract tests for `WalkOptions.Limit`, callback error wrapping, stopping with `ErrStopWalk`, and `HasAny` for empty prefix, missing prefix, file prefix, and directory/prefix content.
|
||||
- Run adapter tests after any change.
|
||||
|
||||
Risk level:
|
||||
|
||||
Medium. The duplication is real, but adapters have protocol-specific walk behavior. Keep the helper narrow.
|
||||
|
||||
### 2. Unify digest format validation without expanding public API accidentally
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `pkg/bundle/digest.go`
|
||||
- `internal/bundle/digest.go`
|
||||
- `internal/state/validate.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
`pkg/bundle` has a private `validateDigest` using a `sha256:<64 lowercase hex>` regex (`pkg/bundle/digest.go:12`). `internal/bundle` repeats the regex to expose `ValidateDigest` for state validation (`internal/bundle/digest.go:10`, `internal/state/validate.go:77`).
|
||||
|
||||
Why it matters:
|
||||
|
||||
Digest format is a source manifest and destination state invariant. If the digest grammar ever changes, the public package and internal state validation must be updated together.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
Either expose `pkg/bundle.ValidateDigest` as part of the producer-facing API, or keep the internal duplicate if the project does not want that API commitment. If exposed, document it in `pkg/bundle` tests and use it from `internal/bundle`.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Move existing digest format tests to cover the public function if exposed.
|
||||
- Add state validation coverage that proves state uses the same digest validator.
|
||||
|
||||
Risk level:
|
||||
|
||||
Medium. The code change is small, but adding a public API has compatibility implications.
|
||||
|
||||
### 3. Reduce local test fixture builders after behavior cleanup
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/app/run_test.go`
|
||||
- `internal/testutil/fixtures.go`
|
||||
- `internal/cli/root_test.go`
|
||||
- `internal/publish/force_test.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
`internal/testutil` already provides source bundle, fake source bundle, minimal config, fan-out config, destination state, and state-reading helpers. `internal/app/run_test.go` still maintains a local `testBundleOptions`, local source-bundle adapter, several YAML config builders, file assertions, fake-backend assertions, and state helpers (`internal/app/run_test.go:1237`, `internal/app/run_test.go:1275`, `internal/app/run_test.go:1318`, `internal/app/run_test.go:1349`, `internal/app/run_test.go:1410`, `internal/app/run_test.go:1437`). CLI and publish tests also have small local file/fake helpers.
|
||||
|
||||
Why it matters:
|
||||
|
||||
The large app test file is valuable coverage, but local fixture helpers make it easier for config examples and test setup patterns to drift from shared fixtures. This is mostly maintenance friction rather than production risk.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
After production cleanup is complete, move only generally reusable helpers into `internal/testutil`: config builder variants for publish policy, path mapping, links, and Markdown transform; file assertion helpers; fake backend assertion helpers. Keep highly specific scenario builders local.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- This is a test-only cleanup. Run `go test ./internal/app ./internal/cli ./internal/publish`.
|
||||
- Avoid rewriting test cases and helpers in the same commit if failures would become hard to diagnose.
|
||||
|
||||
Risk level:
|
||||
|
||||
Low to medium. Test-only, but high-volume edits can obscure behavior changes if combined with production refactors.
|
||||
|
||||
### 4. Split run output formatting from run orchestration
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/app/run.go`
|
||||
- `docs/internal/app.md`
|
||||
- `internal/app/run_test.go`
|
||||
- `internal/cli/root_test.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
`internal/app/run.go` contains orchestration, destination selection, fixed-path warnings, text plan lines, JSON result types, summary counters, failure aggregation, notification mapping, secret warnings, and SSH warnings in one file. There is some duplication between text and JSON summary/status projection (`internal/app/run.go:591`, `internal/app/run.go:611`) and between plan error line formatting and JSON error action creation (`internal/app/run.go:264`, `internal/app/run.go:494`, `internal/app/run.go:529`).
|
||||
|
||||
Why it matters:
|
||||
|
||||
This is the largest production file and it carries multiple responsibilities. The current boundaries are not wrong, but future run output changes may require touching orchestration code, making regressions more likely.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
Split `internal/app/run.go` into package-local files by responsibility, for example `run_output.go`, `run_summary.go`, `run_warnings.go`, and `run_selection.go`. Keep the package and public behavior unchanged. This is file organization and small helper extraction, not a new reporting subsystem.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Preserve existing app and CLI output tests.
|
||||
- Add focused tests for summary text/JSON consistency if the summary helpers are extracted.
|
||||
|
||||
Risk level:
|
||||
|
||||
Medium. Mostly mechanical, but output text and JSON are public behavior.
|
||||
|
||||
## Boundary and responsibility concerns
|
||||
|
||||
The primary boundary concern is `internal/app/backends.go`. It is acceptable for app-level wiring to import concrete adapters, but app also owns string-keyed conversion from config structs to adapter option maps. This is currently the right package, but the duplicated source/destination mapping should be narrowed through a shared app-local conversion helper.
|
||||
|
||||
`internal/app/run.go` also mixes orchestration with output presentation and notification projection. This is not a boundary violation because app owns top-level command output, but it is a maintenance concern. Keep formatting in `internal/app`; do not move command output into `internal/publish`, because publish should remain focused on destination planning and execution.
|
||||
|
||||
`internal/config` correctly owns config defaults and validation. It should not open backends or resolve storage paths. `ValidatePublishTransformPolicy` being shared with publish planning is appropriate because it prevents config and runtime policy drift.
|
||||
|
||||
`internal/publish` correctly depends on `internal/storage`, `internal/state`, and transform interfaces rather than concrete adapters. Link generation in publish is destination publication behavior and fits the package, but URL validation should share policy with config/state.
|
||||
|
||||
The storage adapters contain some similar code, but most of it is protocol-specific implementation. Do not move filesystem, SFTP, or S3 semantics into core packages.
|
||||
|
||||
## Path, key, and naming construction review
|
||||
|
||||
Path and naming construction is mostly centralized enough:
|
||||
|
||||
- Source manifest filename is centralized through `pkg/bundle.ManifestName` and re-exported by `internal/bundle`.
|
||||
- Destination state filename and path helpers are centralized through `storage.StateFileName`, `storage.StatePath`, and `storage.ManagedBundleTargets` (`internal/storage/path.go:9`, `internal/storage/path.go:38`, `internal/storage/path.go:52`).
|
||||
- Runtime storage paths use `storage.Join`, `storage.ValidatePath`, `storage.ValidatePrefix`, and `storage.DisplayPath`.
|
||||
- S3 prefix normalization is centralized in `internal/config/s3.go`.
|
||||
- Public URL path construction is in `internal/publish/links.go`.
|
||||
|
||||
Exact cleanup areas:
|
||||
|
||||
- `pkg/bundle` has a private `.distributor.json` reserved-name constant while `internal/storage` has `StateFileName` (`pkg/bundle/path.go:9`, `internal/storage/path.go:9`). This is probably intentional because `pkg/bundle` cannot import `internal/storage`, but the reserved name should be documented near the public bundle contract if it is not already clear.
|
||||
- `storage.ValidatePath` and `pkg/bundle.ValidateSourcePath` intentionally share similar clean relative slash path rules, but source paths also reject reserved filenames (`pkg/bundle/path.go:11`, `internal/storage/path.go:11`). Do not merge them unless the semantic difference remains explicit.
|
||||
- S3 object-key construction should remain in the S3 adapter. Existing `objectKey`, `listPrefix`, and `logicalPathFromKey` helpers are appropriately local to S3 behavior.
|
||||
- URL construction is centralized in publish, but URL validation is duplicated and should be cleaned up as described above.
|
||||
|
||||
## Resolution and catalog review
|
||||
|
||||
There are no stage, module, schema, prompt, profile, artifact catalog, or validator catalogs in this repository.
|
||||
|
||||
Implemented resolution paths are consistent:
|
||||
|
||||
- Backend names are config constants and runtime adapter construction is registered in `internal/app`.
|
||||
- Transform names are centralized in `internal/transform`, and app registers Markdown-to-HTML once through `newTransformRegistry`.
|
||||
- Configured-source `validate` and `inspect` share `selectSourceBundles` and `selectSourceBundlesFromConfig`, so pipeline lookup, secret loading, source backend opening, bundle narrowing, and warnings are consistent.
|
||||
- Destination bundle resolution for `preserve_relative` and `fixed` mappings is centralized in app run selection.
|
||||
- Link primary selection is centralized in publish.
|
||||
|
||||
Recommended centralization:
|
||||
|
||||
- Keep transform registration in app. Do not create a plugin or catalog layer.
|
||||
- Consider extracting destination selection helpers from `run.go` into a package-local file, but not a new package.
|
||||
- Keep configured-source selection shared between validate and inspect; focus cleanup on duplicated CLI flag parsing.
|
||||
|
||||
## Config and command-loading review
|
||||
|
||||
Config loading is consistent:
|
||||
|
||||
- `run` uses the supplied config path or `config.DefaultConfigPath`.
|
||||
- Configured-source `validate` and `inspect` require an explicit `--config`, matching the documented CLI.
|
||||
- `config.LoadFile` rejects unknown YAML fields, applies defaults, then validates.
|
||||
- `run`, configured-source `validate`, and configured-source `inspect` load `secrets.directory` before opening backends.
|
||||
- S3 explicit credentials are resolved through the config environment resolver, not direct `os.Getenv`.
|
||||
|
||||
Intentional differences:
|
||||
|
||||
- Local-path `validate` and `inspect` do not load config.
|
||||
- `run` opens destinations; configured-source diagnostics open only the selected source.
|
||||
- `run --dry-run` marks SSH known-host handling read-only; configured-source diagnostics do not have a dry-run flag.
|
||||
|
||||
Likely accidental duplication:
|
||||
|
||||
- `validate` and `inspect` CLI flag setup is repeated even though the mode semantics are identical.
|
||||
- Backend source/destination open-config conversion is repeated and should be centralized within app-level wiring.
|
||||
|
||||
## State, manifest, or progress handling review
|
||||
|
||||
Destination state handling is coherent:
|
||||
|
||||
- `.distributor.json` is the sentinel and state record.
|
||||
- Publish planning inspects destination state through `storage.StatePath`, parses with `state.Parse`, and compares with `state.Compare`.
|
||||
- Replacement safety uses managed outputs from existing state for normal replacement and bounded prefix deletion for forced replacement.
|
||||
- Dry-run builds plans and output records without writing outputs or state.
|
||||
|
||||
Manifest handling is coherent:
|
||||
|
||||
- `pkg/bundle` owns public source manifest parsing, digesting, local writing, and local validation.
|
||||
- `internal/bundle` reuses the public manifest model for storage-backed discovery and validation.
|
||||
- State validation uses the same embedded manifest validation path.
|
||||
|
||||
Potential cleanup:
|
||||
|
||||
- Digest format validation is duplicated between `pkg/bundle` and `internal/bundle`. Decide whether to expose a public digest validator or keep the duplicate as a conscious public API boundary.
|
||||
- Output metadata projection into state, JSON output, and notifications should be made less repetitive before new output metadata is added.
|
||||
|
||||
Progress handling:
|
||||
|
||||
- There is no separate run checkpoint or progress store. This is consistent with current docs and architecture.
|
||||
|
||||
## Refactors to avoid
|
||||
|
||||
Avoid these refactors for the next cleanup pass:
|
||||
|
||||
- A generic workflow engine for `run`, `validate`, `inspect`, and `manifest create`.
|
||||
- A CLI framework or broad command abstraction.
|
||||
- A broad plugin architecture for backends or transforms.
|
||||
- Moving backend construction into `internal/config`.
|
||||
- Moving publish planning policy into adapters.
|
||||
- Consolidating local, SSH, and S3 deletion or traversal implementations beyond tiny shared walk helpers.
|
||||
- Merging `pkg/bundle.ValidateSourcePath` and `storage.ValidatePath` without preserving their different semantics and error contracts.
|
||||
- Rewriting destination state schema or source manifest schema as part of deduplication.
|
||||
- Replacing explicit app output structs with a generic reporting layer.
|
||||
- Large test rewrites in the same commit as production refactors.
|
||||
|
||||
## Recommended implementation sequence
|
||||
|
||||
1. Centralize backend open-config construction in `internal/app/backends.go`.
|
||||
2. Add a shared internal URL policy helper and use it from config, state, and publish link planning.
|
||||
3. Extract configured-source CLI flag parsing for `validate` and `inspect`.
|
||||
4. Add small publish output projection helpers for state outputs, managed output paths, and notification/JSON metadata where appropriate.
|
||||
5. Split `internal/app/run.go` into package-local files for run selection, warnings, summary, and output formatting.
|
||||
6. Add a narrow storage walk-emitter or `HasAny` helper only if adapter tests can prove identical semantics.
|
||||
7. Decide whether `pkg/bundle` should expose digest validation; if yes, wire `internal/bundle.ValidateDigest` through it.
|
||||
8. Move broadly reusable test fixture helpers from `internal/app/run_test.go` into `internal/testutil`.
|
||||
9. Run a dead-code and legacy-helper sweep after the above changes, keeping public behavior unchanged.
|
||||
|
||||
Each item should be a separate commit or prompt unless the implementation is very small.
|
||||
|
||||
## Test strategy
|
||||
|
||||
Tests to run for the full cleanup sequence:
|
||||
|
||||
- `go test ./internal/app ./internal/cli ./internal/config`
|
||||
- `go test ./internal/publish ./internal/state`
|
||||
- `go test ./internal/storage ./internal/storage/fake ./internal/adapters/local ./internal/adapters/ssh ./internal/adapters/s3`
|
||||
- `go test ./internal/bundle ./pkg/bundle`
|
||||
- `go test ./...` after cross-package or documentation changes
|
||||
|
||||
Tests to add before or during refactors:
|
||||
|
||||
- Backend open-config equivalence tests for source and destination SSH/S3 mapping.
|
||||
- Configured-source `validate`/`inspect` shared CLI parsing table.
|
||||
- Shared URL policy table, plus config/state field-context tests.
|
||||
- Publish output projection tests for generated outputs with URLs.
|
||||
- Run summary text/JSON consistency tests if summary logic is extracted.
|
||||
- Storage walk contract tests if a shared walk helper is introduced.
|
||||
|
||||
Validation performed during this audit:
|
||||
|
||||
- `go list ./...` was run with workspace-safe Go cache paths. It required network access to fetch missing modules and completed successfully after approval.
|
||||
- No full test suite was run because this was a report-only task and no code behavior was changed.
|
||||
|
||||
## Appendix: findings not worth acting on
|
||||
|
||||
### Adapter-specific path/key conversion
|
||||
|
||||
Local `nativePath`, SSH `nativePath`, and S3 `objectKey` all convert logical storage paths into backend-native addresses. They look similar in purpose, but their semantics differ: local must guard filesystem roots and symlink ancestors, SSH must use remote slash paths and SFTP behavior, and S3 must handle bucket/prefix object keys. Keep them local to adapters.
|
||||
|
||||
### `storage.ValidatePath` and `pkg/bundle.ValidateSourcePath`
|
||||
|
||||
Both enforce clean relative slash paths, but source bundle paths also reject reserved source/destination state filenames and use public producer-facing error wording. Keep them separate unless a future helper can preserve the different contracts.
|
||||
|
||||
### Explicit command files
|
||||
|
||||
`run`, `validate`, `inspect`, `version`, and `manifest` each having a command file is clear and appropriate. Only the shared configured-source flag parsing for `validate` and `inspect` should be extracted.
|
||||
|
||||
### Text output and JSON output are intentionally separate shapes
|
||||
|
||||
Text output is human-oriented and JSON output is a stable machine-readable envelope. Do not collapse them into one generic renderer. Small helpers for shared summary data are enough.
|
||||
|
||||
### Test-local helpers for very specific scenarios
|
||||
|
||||
Some local test helpers make individual scenarios easier to read. Move only broadly reusable builders and assertions into `internal/testutil`; leave one-off scenario setup local.
|
||||
Reference in New Issue
Block a user