557 lines
32 KiB
Markdown
557 lines
32 KiB
Markdown
# 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
|
||
|
||
**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
|
||
|
||
**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.
|