diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..3348d49 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,208 @@ +# Publication reconciliation remediation + +Status: open findings from review of commits `4dff808` through `599ad81`, +against planning baseline `447b42a`. + +The [feature roadmap](publication-reconciliation.md) defines the requested +behavior; the [implementation plan](implementation.md) 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 + +**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 + +**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 + +**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 + +**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 + +**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 + +**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.