ConduitIO / ConduitIO/conduit

generate capture: follow-ups from #2814's round-6 review (median provenance, carried-forward scoring, publish-shell coverage)

Open
#2,821 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
610
Forks
63
Avg merge
12h 28m
Merged PRs (30d)
57

Description

Follow-ups from the round-6 review of #2814. None of these blocked that merge; filing so they
don't get lost.

## 1. Extend the median shape-guard to compare corpus identity, not just counts

`preserveMediansIfNothingPromoted`'s guard is `prior.RequestCount == len(requests) && prior.Passes
== passes`. That admits a corpus whose *count* is unchanged but whose *membership* changed — swap
one request id for another, or edit a prompt under an unchanged id. On a run that promotes nothing,
the prior manifest's medians then publish unchanged under this run's refreshed `capturedAt` /
`corpusCommitSha` / `captureCommand`, with nothing marking their provenance. That is structurally
the same defect the guard was written for.

The reviewer could not construct a reachable instance that is *materially* wrong on a *loadable*
tree: an id swap leaves an orphaned file that `checkBijection` rejects, and a same-id prompt edit
leaves the deliberate `.yaml` + `.missing.yaml` coexistence — both of which
`TestLoadTranscripts_CommittedTreeLoadsCleanly` turns red on the very PR `publish` opens. So the
harm is bounded to "a foreign median published on a PR that CI is already failing."

Cheap fix: the prior manifest already carries `RequestOutcomes`, and `idSetEqual` is already
extracted. Two lines in the same guard.

## 2. Decide whether carried-forward ids should be excluded from scoring rather than flagged

This is the design question that will otherwise keep generating review rounds.

Today, on a **first** run, 27/28 with one chronic 429 gives `missingCount: 1` and a routine
`[!NOTE]`. On any **re-run** of the same corpus, the identical 27/28 escalates to
`[!WARNING] PARTIAL capture, review before merging` with body text saying to treat it as an
incident — because the flaky id is now carried forward from disk, which trips the
unreliable-median flag.

That is correct by design (the alternative is round 5's fabricated 0.667, published as routine),
and `TestRunCapture_ChronicPerPassMiss_CapturedViaDiskCarryForward_FlagsUnreliable` asserts exactly
this. But re-runs become the common case once a corpus is committed, so a `[!WARNING]` per flaky
request may train readers to ignore that banner — which is the failure mode the banner exists to
prevent.

The cleaner long-term answer is probably to exclude carried-forward ids from the *scored* corpus
for that run, rather than scoring them and then flagging the median unreliable. That is a design
change, not a review finding.

## 3. Carry `RequestOutcome.Unusable` alongside `FailureReason` on the carry-forward path

`classifyRequestOutcome` sets `FailureReason` when a carried-forward request also failed this run,
but not `Unusable`, even though `unusable[req.ID]` is computed for the same id set. A reader of a
carried-forward outcome sees `failureReason: provider error` but cannot tell a billed refusal from
a 429 — a distinction they *can* make when `Captured` is false.

Code and doc currently agree (`Unusable`'s doc says "always false when `Captured` is true"), so
this is an asymmetry rather than a bug. Fixing it means threading `unusable` into
`classifyRequestOutcome` **and** changing that documented invariant — deliberately deferred off the
merge for that reason.

## 4. The `publish` job's shell has no test harness at all

`generate-capture.yml`'s `publish` job — the `git rm --ignore-unmatch` tombstone loop, the
`manifest_path` derivation, the banner-rendering arms — is reasoned about by reading and is
`actionlint`-clean, but nothing exercises it. Several rounds of review findings landed in exactly
this shell (the `-unormal` collapse, the `git rm` exit-128, the line-anchored greps), which is what
you'd expect of code with no coverage.

Worth considering a `bats`-style or plain-shell harness that runs the job's steps against a scratch
git repo, the way the round-1..6 reviewers each did by hand.

Found during the round-6 review of #2814.

Contributor guide

Open the contributing guide

Research direction

Start with preserveMediansIfNothingPromoted, idSetEqual, classifyRequestOutcome, and the related capture tests, especially TestLoadTranscripts_CommittedTreeLoadsCleanly and TestRunCapture_ChronicPerPassMiss_CapturedViaDiskCarryForward_FlagsUnreliable. Then inspect generate-capture.yml's publish job and decide how the carry-forward scoring behavior and shell coverage should be specified. Done means the selected design decisions, outcome documentation, regression tests, and publish-job harness are agreed and covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, go, shell
Domain
ci-cd, data-engineering, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
32/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.