base-layer/releasekit
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 4 Wiki Activity

Record publication recovery review findings

Browse Source
This commit is contained in:
Eric Rakestraw
2026-11-02 20:24:44 +00:00
parent 599ad81bec
commit 3280f734ef
1 changed files with 208 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

208
docs/roadmap/remediation.md Normal file
View File

@@ -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.