mirror of
https://github.com/gastownhall/beads.git
synced 2026-09-14 20:17:24 +08:00
Eddie the Engineer
85cd362666
Maintainer review fixup for the proxy parity branch, iterations 1-2.
Blockers and majors from the review synthesis and quality scorecard.
Blockers:
- cmd/bd/migrate_dolt_mode.go: resuming from migratePrepared rebuilt the
migration plan from the current argv instead of replaying the recorded
one. A resume that omitted or changed --idle-timeout wrote a sidecar the
target_configured check then rejected, and because that check runs only
after the phase advances past migratePrepared, the block never ran again
— the workspace stayed wedged in proxied-server mode with no in-tool
recovery. Replay j.Sidecar and warn when the supplied value is ignored.
- internal/tracker/store.go: NewStore double-wrapped already-adapted values.
directStore embeds the storage.Storage interface, so *directStore also
satisfies storage.Storage and was re-wrapped as
&directStore{Storage: &directStore{...}}; the inner value can never
satisfy storage.IssueLifecycleStore, which silently degraded
ApplyIssueUpdate to a bare UpdateIssue — no labels, no lifecycle
transaction. Match the tracker's own adapters by concrete type first.
Testing value.(Store) first, as the review suggested, does not work:
storage.Storage is a strict superset of Store, so a raw direct store
satisfies Store too and would be returned un-adapted, relocating the same
bug. The branch order is documented at the call site.
Majors:
- internal/tracker/store.go: directStore.ApplyIssueUpdate now calls
syncIssueLabels, which normalizes both label sets (trim, drop empty) and
restores the semantics the pull path had before this seam moved behind
ApplyIssueUpdate. The un-normalized local copy churned remove+add on
whitespace-only differences and let an empty label reach AddLabel; it is
deleted rather than left as a second, divergent implementation.
- Both regressions were invisible to the suite. Adds
TestNewEngineKeepsDirectStoreAdaptationIntact, TestNewStoreIsIdempotent,
TestDirectStoreApplyIssueUpdateNormalizesLabels, and
TestMigrateToProxiedServer_ResumeReplaysRecordedIdleTimeout (changed /
omitted / never subtests, each also asserting the reverse-migration escape
hatch still recovers). All four were verified to fail against the
pre-fix production code with the predicted symptoms.
Non-gating follow-ups from the scorecard, fixed while nearby:
- cmd/bd/proxy_capability.go: treat ProxyOutcomeNA as "this command has no
such flag" rather than a refusal, so an assert on an N/A row cannot return
a non-nil error carrying an empty message.
- cmd/bd/errors.go: emit the rule's real Mutates value in the JSON refusal
envelope instead of a hardcoded false.
- cmd/bd/main.go: the doctor pre-provider check returned unconditionally,
so a nil result (every doctor subcommand) skipped the legacy-store guard
and autocommit-mode resolution that other skipsStoreInit commands run.
Return only on a real refusal.
- internal/tracker/engine.go: type NewEngine's store parameter as Store so
the seam names its contract, and drop the now-dead applyPullIssueUpdate.
- CHANGELOG.md: record the proxied `bd ready --claim` row-cap refusal, which
was a behavior change shipped without a release note.
Iteration 2 -- gating major (uowStore/directStore external_ref divergence):
- internal/tracker/uow_store.go: GetIssueByExternalRef resolved through a
single merged search over both storage planes, which has no plane
preference: it is a UNION ALL ordered by content and terminating in
`id ASC`, so one external_ref present on both planes resolved to whichever
row's id sorted first. The direct backend is explicitly issues-first --
issueops.GetIssueByExternalRefInTx queries `issues` and falls through to
`wisps` only on sql.ErrNoRows, and says why ("so that pushed ephemeral
beads are found during pull dedup"). The consumer is the pull dedup, which
updates whatever row comes back, so the divergence was a silent write to
the wrong local bead under --proxied-server only. Query the issues plane
first via SkipWisps, then fall back.
The fallback is the merged search rather than the review's literal
Ephemeral=true: that filter contributes an `ephemeral = 1` predicate the
direct backend's `SELECT id FROM wisps WHERE external_ref = ?` does not
have, and wisp-plane rows legitimately carry ephemeral=0 (NoHistory beads;
see types.IssueFilter.EphemeralTier). Taking it literally would have traded
one divergence for a narrower one. Verified: implementing the prescription
fails the ephemeral_flag_clear subtest below.
- internal/tracker/uow_store_engine_test.go: the engine fake now models plane
routing (SkipWisps / Ephemeral / merged) instead of one flat map, so the
contract is expressible at all. Adds
TestUOWStoreGetIssueByExternalRefPrefersIssuePlane, whose wisp id sorts
first so "first merged hit" fails it, and
TestUOWStoreGetIssueByExternalRefFallsBackToWispPlane over both ephemeral
values. Verified to fail against the pre-fix uow_store.go.
- internal/tracker/conformance: the shared suite gains a Wisps plane and
external_ref_resolution_prefers_issue_plane, so this is a contract both
backends are held to rather than one adapter's unit test.
Iteration 2 -- non-gating follow-ups from the scorecard:
- cmd/bd/migrate_dolt_mode.go: scope the migratePrepared sidecar-equivalence
check in runMigrateFromProxiedServer to reverse journals. A forward journal
at that phase records the sidecar it is about to write, so a crash in the
pre-write window leaves none on disk -- the correct state -- and the
unscoped check refused the documented escape hatch by naming a missing
file, stranding the workspace. Tested against TargetMode's forward value so
the guard stays live for both reverse targets, "server" and
"shared-server". Adds
TestMigrateFromProxiedServer_ForwardJournalAtPreparedIsNotSidecarRefused,
verified to fail against the unscoped guard.
- cmd/bd/linear.go: buildLinearClientAPIOnly returned the OAuth client inline
and never reached the WithEndpoint block, so a configured
linear.api_endpoint was honored for an API key and ignored for OAuth. Hoist
the endpoint application after the branch, as Tracker.Init does, and add
the OAuth mirror of the existing endpoint test.
- cmd/bd/ready_max_rows_proxied_test.go: pin the proxied --max-rows refusal
on `bd ready --claim`. It lives in RunE above the dispatch and nothing
below it can enforce a cap, but the existing coverage all sits below it.
Covers both cap sources (--max-rows and BEADS_MAX_ROWS) and the negative
case that the refusal is conditioned on a live cap, not on --claim.
- engdocs/design/ownership-handoff-contract.md: trim the trailing blank line
at EOF flagged by `git diff --check`.
Iteration 3 -- non-gating residual on the iteration-2 fix:
- internal/tracker/uow_store.go: the issues-first fix above split one logical
lookup across two read transactions. uow.RunTxRead opens a fresh unit of
work per call, so calling s.SearchIssues twice left the wisp fallback's
premise -- "the issues plane is already known empty" -- without a shared
snapshot: a concurrent same-ref insert into the issues plane landing between
the two reads could let the merged fallback return the wisp, re-creating the
wrong-row write that ordering exists to prevent. The consumer is the pull
dedup loop, so it runs per imported issue. Both plane reads now share a
single RunTxRead, which is also the shape the direct backend uses (both
planes inside one withReadTx). Plane ordering is unchanged.
- internal/tracker/uow_store_engine_test.go: the engine fake counts the units
of work it hands out, and
TestUOWStoreGetIssueByExternalRefUsesOneReadTransaction pins that count at 1
across the two paths that actually perform both reads -- the wisp fallback
and the miss. An issues-plane hit short-circuits after one read and would
pass either way, so it is not a case. Verified to fail against the
two-transaction version with "opened 2, want 1".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>