Files
releasekit/docs/roadmap/remediation.md

32 KiB
Raw Blame History

Publication reconciliation remediation

Status: open findings from review of commits 4dff808 through 599ad81, against planning baseline 447b42a.

The feature roadmap defines the requested behavior; the implementation plan supplies the intended implementation. The broad package structure is in place: the root facade owns public evidence and recovery types, internal/publish owns observation, recording, and discovery, internal/git owns scoped endpoints and ref queries, and the CLI owns command parsing, presentation, and exit classification. The findings below identify remaining correctness, boundary, and contract gaps.

Review evidence

The full offline suite (go test ./...) and compiled facade/dependency checks (go run ./internal/apicheck) passed with Go 1.26.8. Additional disposable probes used local Git repositories, synthetic credentials, controlled Git subprocesses, and failing local output. They reproduced the findings below without changing production code or adding repository tests. Passing existing tests does not cover these failure combinations.

R01: Preserve candidate failures and keep storage failures separate — P1

Completed: Capture and storage errors now remain separate. Focused publication regressions passed for invalid/missing notes, retained Git causes, saved partial evidence, and successful publication after storage lookup failure.

Relevant code: internal/publish/publish.go:110, especially the assignment at line 120 and the error check at line 127.

candidate.Capture can return useful repository/identity fields together with an error, such as a missing or invalid release note. When its repository field is present, the new storage initialization assigns resolveRecordDir's error to the same err variable before the capture error is checked.

This produces two opposite defects:

  • Successful storage resolution erases the capture error. Publication continues with a candidate that failed capture. The probe used a release note with the wrong product heading: Check rejected it, while Publish ran the product callback, returned published with a nil error, and pushed the annotated tag to the local remote.
  • Failed storage resolution replaces a successful capture with a publication failure. A controlled failure of the Git common-directory lookup returned not_attempted, ran no product callback, and returned a capture release candidate error as well as RecordError.

Expected behavior: A capture error stops publication while preserving useful partial evidence. A recording/storage error is reported separately and leaves the publication result governed by its existing checks and push outcome.

Remediation: Keep capture and storage errors in separate variables. Preserve the original capture failure across record initialization/finalization; use RecordError exclusively for recording failures. Add regressions for invalid and missing notes, capture-error cause preservation, and a valid candidate whose storage-directory lookup fails. Verify that rejected candidates run no product checks or tag creation, and that storage lookup failure permits an otherwise successful publication.

R02: Validate the filename that will actually be written — P2

Completed: Each generated filename is checked before that exact path is created, including collision retries. Deterministic selective-ignore tests passed and confirm that rejected writes leave the candidate clean and existing records unchanged.

Relevant code: internal/publish/record.go:68, record.go:85, and record.go:134; the pre-push save in internal/publish/publish.go.

The override guard generates a random proposed filename and checks its Git ignore status. The creation loop then generates a different random filename and writes that file without checking its ignore status.

Filename-dependent ignore rules therefore allow the check and actual write to disagree. A probe using selective/attempt-[0-7]*.json in .gitignore successfully saved a record starting with attempt-c...; Git reported that actual record as untracked. The save returned success. The reverse mismatch can also reject a name even though another name the writer would choose is ignored.

Expected behavior: The storage safeguard applies to the exact path passed to exclusive creation. A failed storage choice leaves the handoff unavailable without introducing unignored candidate contents.

Consequences: During publication, saving occurs after the final clean-tree guard. An unchecked actual path can therefore leave the worktree dirty while publication succeeds, and can expose a sensitive record to a later ordinary git add operation. The already captured release tag does not itself include that new file.

Remediation: Generate one candidate path, validate that path, and use the same path for exclusive creation. Repeat that sequence if a filename collision requires another candidate. Add deterministic coverage with filename-dependent ignore rules; verify that successful writes are ignored and rejected choices leave the worktree clean. A naming seam can make this test deterministic rather than relying on random retries.

R03: Allow an explicit override into Git metadata — P2

Completed: The override guard recognizes Git-discovered private and shared metadata directories. Recording tests passed for ordinary, linked-worktree, and separate-Git-directory layouts, alongside rejection of unignored worktree files.

Relevant code: internal/publish/record.go:142, especially the worktree containment and git check-ignore handling at lines 148–159.

For a conventional checkout, the default store is inside .git/releasekit/publication-attempts. Selecting that same directory explicitly activates the override guard. It treats the path as worktree content and demands an ignore-rule match, although Git metadata is already excluded from tracked source and ordinary status reporting.

The probe saved a default record successfully, then selected its parent directory as the explicit override. The second save failed with record_write and an assertion that the metadata path was inside the worktree and unignored.

Expected behavior: The agreed storage choices include Git metadata, external locations, and Git-ignored worktree locations. An override changes the location rather than imposing a requirement to add ignore rules for metadata. The rejection is an additional storage prohibition beyond the intended policy.

Remediation: Identify Git metadata through Git's directory discovery and recognize that it is outside candidate contents, including linked-worktree and separate-Git-directory layouts. Apply ordinary ignore checks to actual worktree-content paths. Add coverage for explicitly selecting the default store and other private locations in Git metadata, retaining rejection of genuine unignored worktree contents.

R04: Restore cancellation across storage and recovery phases — P2

Completed: Storage and discovery share context-aware Git adapters; later observation classifies canceled tag validation and queries consistently. Tests passed for already canceled discovery and cancellation synchronized with active root/metadata lookups, ignore checks, publication storage, and tag operations. Focused Git, facade, and CLI recovery checks also passed; immediate failed-push reconciliation retains its existing bounded context.

Relevant code: internal/publish/record.go:112 and record.go:152; internal/publish/attempts.go:31 and attempts.go:74; internal/publish/reconcile.go:109 and reconcile.go:130.

Cancellation behavior depends on which phase is active:

  • resolveRecordDir and the ignore guard use exec.Command without the caller's context. A probe held the common-directory lookup, canceled publication, and observed that the call remained blocked until the controlled Git command was explicitly released.
  • Discovery resolves a default/relative store before checking ctx.Err(), and wraps all resolution failures as storage_enumeration. An already canceled default-store request consequently returned that kind instead of canceled.
  • recordDirectoryFor uses raw exec.CommandContext, which can return only a killed-process error when cancellation occurs during Git execution. The active-discovery probe returned storage_enumeration and errors.Is(err, context.Canceled) == false.
  • Later observation checks cancellation in some branches but not the tag-ref validation and QueryTag error branches. Canceling an active tag query returned remote_observation, even though its error chain correctly retained context.Canceled.

Expected behavior: Caller cancellation stops cancellable Git work and is reported as canceled, with the context cause retained. Discovery preserves already obtained entries. Immediate failed-push reconciliation retains its separately selected bounded recovery context.

Remediation: Consolidate the duplicated storage-directory/Git command work behind the shared internal Git/process boundary, passing the relevant context and preserving its cause. Normalize recovery errors at each failed operation using the known context cause, rather than assigning a phase kind first and losing cancellation. Keep partial evidence finalization and recording warnings separate from the original publication failure. Add coordinated tests for already canceled and actively canceled default/relative discovery, blocked storage Git commands, and cancellation during tag validation/query. Preserve the existing immediate-recovery timeout semantics.

R05: Redact recovery paths and diagnostic fields at display boundaries — P2

Completed: Facade errors and CLI recovery displays now redact recognized HTTP credentials in paths, metadata, diagnostics, and handoff commands. Public error and CLI human/JSON regressions passed; raw typed causes and private record contents remain intact.

Relevant code: recovery.go:53; cmd/releasekit/recovery.go:76, its JSON DTO construction, and human path displays; cmd/releasekit/main.go's record displays.

PublicationRecoveryError.Error redacts its wrapped cause but interpolates its own RecordPath directly. The CLI also copies record paths and diagnostic messages directly into human and JSON output. Safe evidence formatting protects the endpoint field, but does not provide a general safe-display boundary for these other strings.

Passing the synthetic value https://audit-user:audit-secret@example.invalid/attempt.json to LoadPublicationEvidence reproduced the issue. The nested file-read cause was redacted, while the facade error's path still contained audit-secret. reconcile --record with the same invalid local-record selection also emitted the synthetic credential in JSON output. This can expose a credential-bearing URL pasted into the wrong argument, despite the documented credential-safe error and diagnostic behavior.

Expected behavior: Recognized endpoint credentials are redacted throughout human and structured diagnostics. Raw values remain available in typed evidence, private records, and the filesystem arguments used for the actual operation.

Remediation: Apply the existing diagnostic/URL formatter to every relevant display string, including recovery-error paths and CLI path/identity/diagnostic fields, before formatting or JSON encoding. Keep raw operation inputs and private serialization intact. Add public-error and CLI human/JSON regressions covering synthetic credential-bearing paths, causes, and metadata, alongside the existing endpoint-only case. No new credential-storage policy is needed.

R06: Propagate structured-output write failures — P2

Completed: Both recovery commands now report JSON encoder failures on standard error and return status 1. Failing-writer regressions passed alongside ordinary recovery status tests; observed publication state remains unchanged. The CLI reference records the reporting-failure status.

Relevant code: cmd/releasekit/recovery.go:242 and its two writeJSON call sites.

writeJSON explicitly discards the error returned by json.Encoder.Encode. Command status is then chosen entirely from observation/discovery results, even when no structured result reaches the caller.

The probe enumerated an empty existing store with --json, directing stdout to /dev/full. It returned status 0 with no stderr diagnostic despite the failed write. The same helper is used by successful reconciliation.

Expected behavior: Automation can distinguish successful output delivery from an output failure. A local reporting failure remains separate from the already observed remote publication state.

Remediation: Return and handle the encoder error. Report the local output failure on stderr and return an appropriate operational failure status, while preserving the actual observation in the implementation rather than converting it to unknown remote state. Document that output-failure status in the CLI contract. Add focused failing-writer coverage for both structured commands; ordinary writable-output state/status behavior should stay unchanged.

Integration audit evidence

A subsequent audit examined the interaction with existing consumer launchers, repository discovery, candidate cleanliness, and the maintainer runbook. The production code remains identical to the code covered by the full-suite/API checks above. A fresh focused run of the existing launcher, Woodpecker, publication-handoff, and lost-output recovery integration tests passed with Go 1.26.8. Disposable probes reproduced the additional issues below. R01–R06 remain open and are preserved rather than duplicated here.

R07: Make the recovery handoff survive a cold Go-tool invocation — P2

Completed: CLI recovery prefers the selected module's Go tool. API-only consumers receive privately retained alternate module/checksum files under Git metadata; native consumers retain the executable-path fallback. Assembled tests passed after cold executables disappeared and launcher files were removed, covering both adoption styles, relative replacements, clean candidates, and recovery exit statuses 0–3. CLI, consumer, and internal guidance is updated.

Relevant code: cmd/releasekit/main.go:134 (publicationRecoveryCommand); examples/multi-module/release.sh; docs/consumers/adoption.md's module-selected Go-tool workflow; cmd/releasekit/publication_handoff_test.go.

The new handoff always uses os.Executable() as the command to run later. Existing consumer guidance and the maintained launcher execute ReleaseKit through go tool releasekit. On an uncached tool build, Go 1.26.8 executes a temporary build executable and removes it when the Go driver exits. A warm invocation uses a surviving cached executable, so its behavior differs.

Evidence: A disposable tool-registered consumer used the current CLI with a cold tool build and a local remote that rejected the tag push. Publication returned status 1 and saved a complete record, but its printed recovery command named /tmp/go-build.../exe/releasekit. That executable was already absent when the driver returned, and copying the command returned status 127. A surviving CLI reconciled the same record successfully to not_published with status 1. The existing handoff test builds a persistent executable directly and therefore does not exercise this launch lifecycle.

Expected behavior: The copyable handoff works with supported consumer launch methods after the publication process has exited. Routine first use on a fresh machine/cache should not require reconstructing another invocation manually. The existing fallback guidance for a subsequently unavailable executable does not supply a working handoff for this ordinary cold-build case.

Decision: Prefer a module-selected Go invocation for recovery where the consumer setup supports it. Provide an explicit, durable recovery path for API-only consumers using temporary tool declarations. ReleaseKit owns the handoff mechanics and supports both adoption styles. This follows the existing tool-selection workflow and keeps routine binary lifecycle management out of consumer code.

Remediation: Make the recovery invocation independent of an ephemeral Go-tool executable using the selected module-based approach and an explicit API-only recovery path. Account for the maintained launcher's temporary -modfile cleanup and its support for ordinary package dependencies as well as registered tools; a bare go tool command cannot assume every consumer has a permanent tool declaration. Keep explicit record selection and distinct exit statuses. Add an assembled cold-build test that executes the printed command after the original Go driver and launcher have exited, rather than only testing a directly built CLI or a warm cache.

R08: Reuse the existing repository-path semantics for record storage — P2

Completed: R04's shared resolver reuses the established Git root and metadata-path normalization. Added regressions passed for repository roots and separate metadata directories ending in whitespace, default/relative read-write agreement, and rejection of absolute unignored worktree stores. Existing ordinary and linked-worktree recording tests also passed.

Relevant code: internal/publish/record.go:112, especially line 117; internal/publish/attempts.go:74, especially line 82; the existing internal/git/source.go:39 and source.go:50 repository-root helpers.

The pre-existing root resolver removes Git's output newline and resolves filesystem aliases, preserving valid whitespace in the path itself. The new recording and discovery resolvers independently use strings.TrimSpace, which changes repository identity when a directory name ends in whitespace. Recording then uses that altered root both for relative overrides and for deciding whether an absolute override lies inside the candidate worktree.

Evidence: In a temporary checkout whose final directory component was consumer , the existing git.RepositoryRoot preserved the exact path and the existing candidate Check completed successfully. The new code produced three inconsistent results:

  • A relative .releasekit/attempts override tried to use the different sibling path under consumer and failed its Git ignore check, despite the real .releasekit/ location being ignored.
  • After a record was saved in the correct absolute ignored location, relative discovery selected the wrong sibling directory and returned a successful empty collection. The actual record still existed in the requested store.
  • An absolute override inside the actual worktree was misclassified as external because containment used the trimmed root. It successfully wrote an unignored record, which Git reported as untracked.

Expected behavior: Publication, discovery, relative overrides, and worktree containment use the same repository identity already supported by the library. Path data remains intact. This failure is independent of R02's filename mismatch: the containment decision itself uses the wrong root.

Remediation: Reuse internal/git.RepositoryRoot and share metadata-path resolution through the Git boundary, coordinating with R04 rather than adding another resolver. Remove output terminators without stripping path characters. Apply the same preservation to common-directory output. Add focused regressions for a valid root ending in whitespace, relative-store read/write agreement, and absolute unignored-path rejection. Preserve existing filesystem-alias and linked-worktree behavior instead of introducing new pathname restrictions.

R09: Align maintainer recovery instructions with the source bootstrap — P2

Completed: Runbook discovery and reconciliation use the prepared source-built CLI, and the roadmap recovery link targets the current heading. Disposable local checks executed both snippets with no global releasekit, confirming discovery and the expected incomplete-evidence reconciliation status.

Relevant code/docs: scripts/release:9; docs/release.md:104 and release.md:149; docs/roadmap/publication-reconciliation.md:21.

The existing maintainer procedure builds and executes .releasekit/bin/releasekit. It does not install an executable named releasekit on PATH. The new lost-output recovery instructions instead run releasekit publication-attempts and releasekit reconcile without identifying the executable that the procedure prepared.

Evidence: With Go and Git available on PATH and only the source-built CLI in .releasekit/bin, the newly documented discovery command returned 127 (releasekit: not found). The equivalent command using ./.releasekit/bin/releasekit returned 0 and listed the saved attempt. A globally installed CLI could also be a different version from the source-built one.

The recovery-section rename additionally left the feature roadmap's link to docs/release.md#resolve-failures pointing at a heading that no longer exists.

Expected behavior: The repository's executable runbook uses the CLI prepared by its own prerequisites, and its recovery links resolve to the current owner. It requires no additional global installation for lost-output recovery.

Remediation: Use the source-built executable for discovery and reconciliation in the maintainer runbook, consistent with its existing delivery commands. Retain explicit instructions for selecting another supported CLI when the original executable is genuinely unavailable. Repair the renamed-section link. Verify the recovery snippets in an environment without a global releasekit executable; keep generic consumer command examples in their consumer-facing documentation.

R10: Preserve endpoint bytes across the private-record handoff — P2

Completed: Private records retain ordinary version-1 JSON and select a version-2 base64 byte representation for non-UTF-8 fields. Both versions load; unknown fields remain ignorable and malformed known byte fields report format errors. Round-trip tests passed for all evidence fields, and real Git observations distinguish the original byte path from a replacement-character decoy consistently before and after loading. The record-format guide is updated.

Relevant code: internal/publish/record.go:77 and its evidence decoding; the string fields in internal/publish/reconcile.go's Evidence; the existing Git endpoint resolution/query path in internal/git/remote.go and tag.go.

On POSIX, the existing Git transport can use local repository paths containing bytes that are not valid UTF-8. Typed evidence retains those bytes in its Go strings. The new private JSON codec uses json.Marshal directly on those strings, which replaces invalid UTF-8 with the Unicode replacement character without reporting an error. Loading the record therefore supplies a different endpoint to the existing Git observer.

Evidence: A disposable valid candidate's publication attempt targeted a local repository whose path contained a non-UTF-8 byte. Its hook rejected the push; publication returned not_published with a complete saved record and the exact original endpoint in typed evidence. A second local repository, named using the replacement-character form of that path, held the same intended annotated tag object. Direct typed reconciliation correctly returned not_published at the original destination. Loading the saved record changed its endpoint to the second repository and record-based reconciliation returned published there. The record save/load reported no error.

Expected behavior: Direct and record-loaded evidence identify the same captured destination. The private handoff retains exact transport identity, including pathname bytes supported by the existing Git boundary, or reports that the handoff could not be saved faithfully. Diagnostic JSON is a display format and can use safe displays; the private record supplies actual operation inputs and must preserve their identity.

Decision: Use a lossless private-record representation for supported pathname and endpoint bytes. This preserves existing publication inputs and keeps record-loaded recovery consistent with direct typed evidence. Recording failures remain separate from the publication outcome.

Remediation: Make private serialization lossless for supported evidence, using an explicit byte representation where needed. Keep the original typed evidence and publication behavior intact; avoid adding a general UTF-8 restriction to previously valid Git publication inputs. Cover endpoint/path round trips and equivalent direct-versus-loaded observations, including a distinct replacement-character path, in focused regression tests.

Algorithmic audit scope and evidence

This audit retains the planned flat record store, complete local discovery, deterministic ordering of selected record paths, versioned JSON format, and exact-tag observation semantics. It does not propose indexes, pagination, automatic retention, or changes to those selected algorithms.

No unexpected quadratic algorithm was found in the added production code. Classification operates on a fixed number of fields and bounded-length object IDs. Record encoding/decoding uses a fixed number of linear passes over a record. Facade and CLI result conversions are linear in returned entries and displayed data. Full-result discovery necessarily retains the records its API returns. The refinements below concern work outside those necessary operations and one partial-result correctness defect.

The existing codec, discovery, and classification tests passed in a fresh focused run with Go 1.26.8. Complexity analysis used the implementation and the local Go standard-library source, without benchmarks or real-world performance measurements. Disposable functional probes confirmed the parser cause loss and partial-enumeration result loss. The latter used a temporary source overlay only to make the directory-reader boundary return real entries with a controlled error; repository production and test files were unchanged.

R11: Filter directory entries before ordering them — P3

Relevant code: internal/publish/attempts.go:41 through the subsequent filter and sort.Strings(paths).

os.ReadDir already sorts every directory entry by filename. Discovery then filters for JSON record files and sorts their full paths again. Consequently, an overridden store containing many unrelated files pays to order names that will never appear in the result. This is an implementation choice beyond the planned requirement to order selected records.

Complexity: Let D be all directory entries, R the selected JSON file candidates, and B their total bytes read. Treating name comparisons as unit operations, the current directory ordering has worst-case O(D log D) cost before the same O(B) record work. Filtering an unsorted enumeration and sorting only selected paths costs O(D + R log R + B). When R is bounded and D grows, directory processing improves from O(D log D) to O(D). When nearly every entry is a record, the planned sorting bound remains unchanged. The second sort currently receives already ordered paths; it is redundant, rather than evidence of a quadratic algorithm.

Expected behavior: Keep complete discovery and deterministic path ordering for attempts and their diagnostics. Unrelated directory entries need no order.

Remediation opportunity: Use the standard os.File.ReadDir interface to enumerate without implicit sorting, filter candidates, and order those paths once. Batched reads can also reduce intermediate directory-entry storage from O(D) to O(batch size + R), excluding retained result data. Preserve partial enumeration errors as described in R13 and cancellation behavior from R04. Keep this a local enumeration refinement; no public file-count limits or changes to record selection are required.

R12: Reject trailing JSON without constructing a discarded value — P3

Relevant code: internal/publish/record.go:24, particularly the second decoder.Decode(&trailing) and its error handling at lines 34–37.

After decoding the envelope, the loader decodes another complete JSON value into any solely to check whether one exists. A second value always makes the record invalid. Nevertheless, an appended array or object is fully parsed and materialized before being discarded. Only trailing whitespace and EOF are acceptable at that point.

Complexity: For a first envelope of L bytes and an appended value of T bytes containing M nodes, the second decode adds Theta(T) parsing work and up to Theta(M) object/array storage. Rejecting at the first non-whitespace byte after the envelope needs no second value tree. A standard-library complete- document unmarshal of the already loaded bytes can reject that trailing input during syntax validation before constructing it. This removes avoidable parsing and allocations; the existing whole-file read still has its Theta(L + T) I/O and buffer bound. It is not an overall quadratic-to-linear claim.

There is also a correctness inconsistency: a non-EOF error from that second decode is replaced by a new message without wrapping the original error. Functional probes showed that an incomplete first document retained io.ErrUnexpectedEOF, while an incomplete trailing document lost it. A syntax error in the first document was available through errors.As as *json.SyntaxError; the corresponding trailing syntax error was not.

Expected behavior: Require one complete document, retain supported partial evidence/additional-field behavior, and preserve available parsing causes in the typed record_format error. Invalid extra data needs no materialized tree.

Remediation opportunity: Simplify loading to a complete-document unmarshal into the versioned envelope, retaining existing format/evidence validation and cause wrapping. An equivalent trailing-data check can also work if it avoids decoding an arbitrary second value and retains real parser failures. Add focused single-document, extra-document, trailing-whitespace, and malformed-suffix coverage. Keep raw record identity handling coordinated with R10; no new size caps or public input restrictions are needed.

R13: Retain entries returned with a directory-enumeration error — P2

Relevant code: internal/publish/attempts.go:41 through its immediate error return at lines 42–44; Stage 5's partial-result requirement in the implementation plan.

Go's os.ReadDir can return both entries read successfully and a later read error. Discovery checks the error first and immediately returns its still-empty result, dropping every returned entry. Thus a filesystem enumeration failure can hide readable attempts that were already discovered, even though the API and CLI support returning attempts alongside a top-level failure.

Evidence: A disposable probe supplied the directory-reader boundary with the real entry for one valid record plus a controlled enumeration error, an outcome permitted by os.ReadDir's contract. ListPublicationAttempts returned zero attempts with storage_enumeration. Without the injected error, it returned that same record successfully. This was fault injection at the read boundary, not a change to the discovery algorithm under review.

Expected behavior: Preserve readable attempts and per-record diagnostics from returned entries while reporting the top-level enumeration failure. The operation remains incomplete and retains its non-success CLI classification.

Complexity: Processing a returned prefix of d entries containing r record candidates and b readable bytes takes the same ordinary filtering, ordering, and decoding work: O(d + r log r + b) with R11's refinement. The existing os.ReadDir approach instead retains its O(d log d + b) bound. Neither correction requires another directory scan: both retain the names already enumerated and perform the normal record loads for those names.

Remediation: Retain the enumeration error, process available entries using the normal discovery path, and return both the collected result and that error. Handle zero-entry failures and cancellation consistently with R04. Add a narrow fault-injected regression for entries-plus-error, verifying readable attempts, retained causes, and API/CLI partial-result behavior. This concerns data preservation, separate from R04's cancellation/error classification.