feat(errors): typed dependency error taxonomy on the public beads package (#4892)
* feat(errors): typed dependency error taxonomy on the public beads package
S1 of the guarded-ops initiative: let the bd CLI (and the library's own call
sites) classify dependency-write failures with errors.Is instead of
string-matching message text, without changing any message (wrap-preserving),
so existing string matchers keep working during the migration.
- Add ErrSelfDependency / ErrDependencyCycle sentinels in
internal/storage/domain and route EVERY production self-dep/cycle emitter
through them: issueops.CheckDependencyCycleInTx, the dolt cross-tier
scheduling-cycle check, the domain use-case add + bulk-add paths, and the
db SQL repository. Canonical-message sites stay byte-identical (sentinel as
the static prefix via %w, or a bare sentinel return); already-divergent
sites (bulk, db) append the sentinel with %w.
- Re-export ErrNotFound, ErrAlreadyClaimed, ErrNotClaimable, ErrSelfDependency,
ErrDependencyCycle as var aliases on the root beads package, so beads.ErrX is
identity-equal to the internal sentinel and errors.Is composes across the
boundary.
- Contract tests: wrap-preserving assertions with EXACT message byte-identity;
a real-production-return test through the domain use case; errors.Is
regression assertions on the dolt cross-tier cycle path and the db self-dep
path. The pre-existing self-dependency test passes unchanged, proving the
canonical messages stayed byte-identical.
A red-team review caught that an earlier draft converted only the same-tier
issueops path, leaving the dolt cross-tier check (which sets SkipCycleCheck and
bypasses issueops) returning an untyped string — errors.Is was false for
cross-tier cycles on the same public DoltStore. All plumbings are now uniform.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(errors): type bulk dependency self-dep and cycle rejections without changing their text
The dependency-add bulk path (proxied `bd dep add` / `bd link`, and the embedded
bulk CLI) surfaces its error verbatim via HandleErrorRespectJSON("%v", err).
Wrapping a rejection with a plain "%w" appends the sentinel's own text to an
already-complete message and changes user-facing output, so the bulk family was
left partially untyped and a scheduling self-edge was misreported as a cycle.
Type every dependency-add bulk rejection so callers can errors.Is it while the
rendered message stays byte-for-byte identical to the pre-taxonomy text:
- Route the per-edge and final domain cycle rejections through a small
string-preserving cycleError wrapper (Error() returns the legacy text,
Unwrap() returns ErrDependencyCycle).
- Guard IssueID==DependsOnID in domain addBulk for all dep types before the
cycle probe, so a scheduling self-edge is ErrSelfDependency instead of
tripping HasCycle (or the final CycleThroughEdges gate) and surfacing as a
cycle; the message matches every other self-dep site.
- Type the embedded bulk CLI final gate (cmd/bd addBulkDependenciesInTx) via the
exported domain.NewCycleError helper, so errors.Is(ErrDependencyCycle) holds
regardless of backend/plumbing.
- Lead the defensive repo-layer self-dep guard with the sentinel like every
other self-dep site instead of appending it.
Add exact-message + errors.Is assertions for the bulk cycle paths (per-edge,
final SkipPerEdgeCycleCheck, embedded final gate) and new bulk self-edge tests
(default, SkipPerEdgeCycleCheck, non-scheduling). Scope ErrDependencyCycle's doc
to the dependency-add family.
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 18:29:07 -07:00
|
|
|
package beads_test
|
|
|
|
|
|
|
|
|
|
import (
|
|
|
|
|
"context"
|
|
|
|
|
"errors"
|
feat(close): CloseIssueChecked — atomic guarded close in the engine (#4893)
* feat(close): CloseIssueChecked — atomic guarded close in the engine
`bd close` reads blockers (store.IsBlocked) and then closes in a SEPARATE
transaction — a TOCTOU where a blocker can clear (or the bead can be closed)
between the check and the close. Add CloseIssueChecked, which runs the
is_blocked guard AND the close inside ONE transaction, so the guard is atomic.
A Force option bypasses it (mirrors `bd close --force`). This lets the `bd`
CLI delegate its close guard to the engine instead of hand-rolling the
check-then-close dance.
- storage.ErrCloseBlocked sentinel + re-export beads.ErrCloseBlocked;
storage.CloseIssueOptions{Reason,Session,Force} and CloseIssueResult{Unchanged}.
- issueops.CloseIssueCheckedInTx: IsBlockedInTx guard (denormalized transitive
is_blocked — an open blocking dependency or an open blocking gate) then
CloseIssueInTx, sharing the transaction. Force skips the guard. A blocked
refusal returns ErrCloseBlocked and rolls back — no close, no closed event.
- Storage-interface method CloseIssueChecked implemented on DoltStore (perm +
wisp paths, mirroring CloseIssue's DOLT_COMMIT/wisp handling) and forwarded
on EmbeddedDoltStore, HookFiringStore (fires on_close on success like
CloseIssue), and InstrumentedStorage.
- Integration tests for the atomic-refuse property on the perm, wisp, and
embeddeddolt paths (blocked → refused + still open + zero closed events;
Force → closes; non-blocking dep does not trip; already-closed → Unchanged;
missing id → ErrNotFound).
Scope is one concern: category-aware reopen and the open-children close guard
are deliberately split into follow-ups. Builds under cgo and CGO_ENABLED=0.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(close): short-circuit already-closed before the is_blocked guard
CloseIssueCheckedInTx ran the is_blocked guard before the already-closed
detection, so a row that is already closed but still carries a stale
is_blocked=1 was refused with ErrCloseBlocked instead of the documented
idempotent Unchanged=true. A closed row can carry a stale flag after a
cross-clone Dolt merge — a state the schema explicitly models (GetStatistics
filters `is_blocked = 1 AND status <> 'closed'`) and a hand-resolved merge
conflict can leave indefinitely.
Detect the already-closed row before consulting is_blocked; the guard only
has meaning for an open->closed transition. This makes the non-force path
symmetric with Force, which already reached Unchanged=true on such a row by
skipping the guard.
Add a regression fixture that seeds a closed row with a stale is_blocked=1
(direct SQL + DOLT_COMMIT, mirroring blocked_merge_test.go's merge-staleness
seeding) and asserts non-force CloseIssueChecked returns Unchanged=true, not
ErrCloseBlocked.
Maintainer review fixup for PR #4893.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Eddie the Engineer <ci@beads.test>
2026-07-19 14:03:25 -07:00
|
|
|
"fmt"
|
feat(errors): typed dependency error taxonomy on the public beads package (#4892)
* feat(errors): typed dependency error taxonomy on the public beads package
S1 of the guarded-ops initiative: let the bd CLI (and the library's own call
sites) classify dependency-write failures with errors.Is instead of
string-matching message text, without changing any message (wrap-preserving),
so existing string matchers keep working during the migration.
- Add ErrSelfDependency / ErrDependencyCycle sentinels in
internal/storage/domain and route EVERY production self-dep/cycle emitter
through them: issueops.CheckDependencyCycleInTx, the dolt cross-tier
scheduling-cycle check, the domain use-case add + bulk-add paths, and the
db SQL repository. Canonical-message sites stay byte-identical (sentinel as
the static prefix via %w, or a bare sentinel return); already-divergent
sites (bulk, db) append the sentinel with %w.
- Re-export ErrNotFound, ErrAlreadyClaimed, ErrNotClaimable, ErrSelfDependency,
ErrDependencyCycle as var aliases on the root beads package, so beads.ErrX is
identity-equal to the internal sentinel and errors.Is composes across the
boundary.
- Contract tests: wrap-preserving assertions with EXACT message byte-identity;
a real-production-return test through the domain use case; errors.Is
regression assertions on the dolt cross-tier cycle path and the db self-dep
path. The pre-existing self-dependency test passes unchanged, proving the
canonical messages stayed byte-identical.
A red-team review caught that an earlier draft converted only the same-tier
issueops path, leaving the dolt cross-tier check (which sets SkipCycleCheck and
bypasses issueops) returning an untyped string — errors.Is was false for
cross-tier cycles on the same public DoltStore. All plumbings are now uniform.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(errors): type bulk dependency self-dep and cycle rejections without changing their text
The dependency-add bulk path (proxied `bd dep add` / `bd link`, and the embedded
bulk CLI) surfaces its error verbatim via HandleErrorRespectJSON("%v", err).
Wrapping a rejection with a plain "%w" appends the sentinel's own text to an
already-complete message and changes user-facing output, so the bulk family was
left partially untyped and a scheduling self-edge was misreported as a cycle.
Type every dependency-add bulk rejection so callers can errors.Is it while the
rendered message stays byte-for-byte identical to the pre-taxonomy text:
- Route the per-edge and final domain cycle rejections through a small
string-preserving cycleError wrapper (Error() returns the legacy text,
Unwrap() returns ErrDependencyCycle).
- Guard IssueID==DependsOnID in domain addBulk for all dep types before the
cycle probe, so a scheduling self-edge is ErrSelfDependency instead of
tripping HasCycle (or the final CycleThroughEdges gate) and surfacing as a
cycle; the message matches every other self-dep site.
- Type the embedded bulk CLI final gate (cmd/bd addBulkDependenciesInTx) via the
exported domain.NewCycleError helper, so errors.Is(ErrDependencyCycle) holds
regardless of backend/plumbing.
- Lead the defensive repo-layer self-dep guard with the sentinel like every
other self-dep site instead of appending it.
Add exact-message + errors.Is assertions for the bulk cycle paths (per-edge,
final SkipPerEdgeCycleCheck, embedded final gate) and new bulk self-edge tests
(default, SkipPerEdgeCycleCheck, non-scheduling). Scope ErrDependencyCycle's doc
to the dependency-add family.
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 18:29:07 -07:00
|
|
|
"testing"
|
|
|
|
|
|
|
|
|
|
"github.com/steveyegge/beads"
|
|
|
|
|
"github.com/steveyegge/beads/internal/storage"
|
fix(dolt): preserve transaction outcome boundaries (#5341)
* fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): retain indeterminate mixed claim updates (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): retain indeterminate guarded update errors (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): prevent lifecycle callback replay (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): classify publication commit ambiguity (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(embeddeddolt): mark commit response loss indeterminate (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): retain indeterminate claim outcomes (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): retry rollback-safe transaction setup (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): surface working-set staging failures (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): account direct commit publication failures (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(embeddeddolt): mark commit call response loss indeterminate (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): account indeterminate transaction commits (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): restore transaction circuit accounting (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(storage): preserve settlement commit boundaries (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): guard direct write circuit boundaries (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): close remaining circuit write gaps (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): account wisp dependency commits (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): record circuit success only after claim verify; cut retry/patch complexity
Maintainer review fixups for PR #5341 (three majors):
- Correctness: verify-gated claim paths (ClaimReadyIssue, UnclaimIssue,
UnclaimIssueIfAssignee, readyClaimer.ClaimNext, issueClaimer.Claim) now wrap
verify+write in withCircuitWrite, so the circuit breaker records success only
at the boundary after post-write verification returns nil — never on the bare
SQL commit ahead of verification. Adds a regression test proving a
committed-but-unverified claim does not reset the breaker.
- Maintainability: replace hasNonCoordinationPatch's 25-branch || chain with a
straight-line nonCoordinationPatchSignals []bool enumeration (cyclomatic
25 -> 3). Adds reflective exhaustiveness and count-guard tests so a new
IssuePatch/LabelPatch/MetadataPatch field fails until it is classified.
- Readability: split withRetryClassified (cognitive 33) into classifyManagedRetry
and recordRetryFailure (cognitive 3/3), preserving retry, indeterminate-commit,
and breaker-trip behavior for both callers.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dolt): record circuit success after facade update verify; skip ignored tables in commit
Maintainer review fixup for PR #5341 (attempt 2).
Major: IssueLifecycle.Update ran its write on an unmanaged context, so
withCircuitWrite recorded RecordSuccess when the SQL commit landed --
before verifiedClaimWrite's re-read could contradict it, laundering a
phantom claim into breaker optimism. Wrap the verify path in
withCircuitWrite and invoke write with the circuit-managed context so
terminal success is recorded once at the boundary, only after verify
returns nil (matching issueClaimer.Claim). The ordinary patch path keeps
its original context and eager accounting.
Minor: commitWorkingSet fed every dolt_status row into a fail-hard
DOLT_ADD loop, so an ordinary Commit's success depended on Dolt's
version-specific handling of DOLT_ADD on a dolt_ignore'd table. Filter
ignored tables with the same anti-join HasCommittablePending uses.
Nit: drop the dead nested-error branch in verifiedClaimWrite (err is
already nil there).
Regressions: facade claim and guarded-update verify failures must not
reset the circuit breaker; a dirty ignored wisp is non-committable and
Commit tolerates it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dolt/test): match commitWorkingSet dolt_ignore anti-join in sqlmock expectations
commitWorkingSet selects dirty tables with the dolt_ignore anti-join
(`SELECT s.table_name FROM dolt_status s WHERE NOT EXISTS (...)`) so ignored
wisp/lease tables stay out of fail-hard staging, but the draincall and
settlement sqlmock regression tests still expected the pre-anti-join
`SELECT table_name FROM dolt_status`. Under QueryMatcherRegexp the status query
no longer matched, so the commit path returned "could not match actual sql"
before reaching the indeterminate-commit / staging-cause / circuit-trip
assertions, failing the macOS Test job.
Update the six status-query expectations to the anti-join form. Behavioral
assertions are unchanged; this only realigns the mock's expected SQL with the
production query the review validated.
---------
Co-authored-by: CI Bot <ci@beads.test>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-06 04:34:46 -07:00
|
|
|
"github.com/steveyegge/beads/internal/storage/dolt"
|
feat(errors): typed dependency error taxonomy on the public beads package (#4892)
* feat(errors): typed dependency error taxonomy on the public beads package
S1 of the guarded-ops initiative: let the bd CLI (and the library's own call
sites) classify dependency-write failures with errors.Is instead of
string-matching message text, without changing any message (wrap-preserving),
so existing string matchers keep working during the migration.
- Add ErrSelfDependency / ErrDependencyCycle sentinels in
internal/storage/domain and route EVERY production self-dep/cycle emitter
through them: issueops.CheckDependencyCycleInTx, the dolt cross-tier
scheduling-cycle check, the domain use-case add + bulk-add paths, and the
db SQL repository. Canonical-message sites stay byte-identical (sentinel as
the static prefix via %w, or a bare sentinel return); already-divergent
sites (bulk, db) append the sentinel with %w.
- Re-export ErrNotFound, ErrAlreadyClaimed, ErrNotClaimable, ErrSelfDependency,
ErrDependencyCycle as var aliases on the root beads package, so beads.ErrX is
identity-equal to the internal sentinel and errors.Is composes across the
boundary.
- Contract tests: wrap-preserving assertions with EXACT message byte-identity;
a real-production-return test through the domain use case; errors.Is
regression assertions on the dolt cross-tier cycle path and the db self-dep
path. The pre-existing self-dependency test passes unchanged, proving the
canonical messages stayed byte-identical.
A red-team review caught that an earlier draft converted only the same-tier
issueops path, leaving the dolt cross-tier check (which sets SkipCycleCheck and
bypasses issueops) returning an untyped string — errors.Is was false for
cross-tier cycles on the same public DoltStore. All plumbings are now uniform.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(errors): type bulk dependency self-dep and cycle rejections without changing their text
The dependency-add bulk path (proxied `bd dep add` / `bd link`, and the embedded
bulk CLI) surfaces its error verbatim via HandleErrorRespectJSON("%v", err).
Wrapping a rejection with a plain "%w" appends the sentinel's own text to an
already-complete message and changes user-facing output, so the bulk family was
left partially untyped and a scheduling self-edge was misreported as a cycle.
Type every dependency-add bulk rejection so callers can errors.Is it while the
rendered message stays byte-for-byte identical to the pre-taxonomy text:
- Route the per-edge and final domain cycle rejections through a small
string-preserving cycleError wrapper (Error() returns the legacy text,
Unwrap() returns ErrDependencyCycle).
- Guard IssueID==DependsOnID in domain addBulk for all dep types before the
cycle probe, so a scheduling self-edge is ErrSelfDependency instead of
tripping HasCycle (or the final CycleThroughEdges gate) and surfacing as a
cycle; the message matches every other self-dep site.
- Type the embedded bulk CLI final gate (cmd/bd addBulkDependenciesInTx) via the
exported domain.NewCycleError helper, so errors.Is(ErrDependencyCycle) holds
regardless of backend/plumbing.
- Lead the defensive repo-layer self-dep guard with the sentinel like every
other self-dep site instead of appending it.
Add exact-message + errors.Is assertions for the bulk cycle paths (per-edge,
final SkipPerEdgeCycleCheck, embedded final gate) and new bulk self-edge tests
(default, SkipPerEdgeCycleCheck, non-scheduling). Scope ErrDependencyCycle's doc
to the dependency-add family.
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 18:29:07 -07:00
|
|
|
"github.com/steveyegge/beads/internal/storage/domain"
|
|
|
|
|
"github.com/steveyegge/beads/internal/types"
|
|
|
|
|
)
|
|
|
|
|
|
fix(dolt): preserve transaction outcome boundaries (#5341)
* fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fixup! fix(dolt): preserve transaction outcome boundary (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): retain indeterminate mixed claim updates (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): retain indeterminate guarded update errors (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): prevent lifecycle callback replay (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): classify publication commit ambiguity (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(embeddeddolt): mark commit response loss indeterminate (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): retain indeterminate claim outcomes (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): retry rollback-safe transaction setup (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): surface working-set staging failures (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): account direct commit publication failures (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(embeddeddolt): mark commit call response loss indeterminate (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): account indeterminate transaction commits (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): restore transaction circuit accounting (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(storage): preserve settlement commit boundaries (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): guard direct write circuit boundaries (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): close remaining circuit write gaps (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): account wisp dependency commits (ga-f7v2ft.79.1)
Agent-Signature: codex-unknown-model-unknown-reasoning on behalf of CI Bot
* fix(dolt): record circuit success only after claim verify; cut retry/patch complexity
Maintainer review fixups for PR #5341 (three majors):
- Correctness: verify-gated claim paths (ClaimReadyIssue, UnclaimIssue,
UnclaimIssueIfAssignee, readyClaimer.ClaimNext, issueClaimer.Claim) now wrap
verify+write in withCircuitWrite, so the circuit breaker records success only
at the boundary after post-write verification returns nil — never on the bare
SQL commit ahead of verification. Adds a regression test proving a
committed-but-unverified claim does not reset the breaker.
- Maintainability: replace hasNonCoordinationPatch's 25-branch || chain with a
straight-line nonCoordinationPatchSignals []bool enumeration (cyclomatic
25 -> 3). Adds reflective exhaustiveness and count-guard tests so a new
IssuePatch/LabelPatch/MetadataPatch field fails until it is classified.
- Readability: split withRetryClassified (cognitive 33) into classifyManagedRetry
and recordRetryFailure (cognitive 3/3), preserving retry, indeterminate-commit,
and breaker-trip behavior for both callers.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dolt): record circuit success after facade update verify; skip ignored tables in commit
Maintainer review fixup for PR #5341 (attempt 2).
Major: IssueLifecycle.Update ran its write on an unmanaged context, so
withCircuitWrite recorded RecordSuccess when the SQL commit landed --
before verifiedClaimWrite's re-read could contradict it, laundering a
phantom claim into breaker optimism. Wrap the verify path in
withCircuitWrite and invoke write with the circuit-managed context so
terminal success is recorded once at the boundary, only after verify
returns nil (matching issueClaimer.Claim). The ordinary patch path keeps
its original context and eager accounting.
Minor: commitWorkingSet fed every dolt_status row into a fail-hard
DOLT_ADD loop, so an ordinary Commit's success depended on Dolt's
version-specific handling of DOLT_ADD on a dolt_ignore'd table. Filter
ignored tables with the same anti-join HasCommittablePending uses.
Nit: drop the dead nested-error branch in verifiedClaimWrite (err is
already nil there).
Regressions: facade claim and guarded-update verify failures must not
reset the circuit breaker; a dirty ignored wisp is non-committable and
Commit tolerates it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dolt/test): match commitWorkingSet dolt_ignore anti-join in sqlmock expectations
commitWorkingSet selects dirty tables with the dolt_ignore anti-join
(`SELECT s.table_name FROM dolt_status s WHERE NOT EXISTS (...)`) so ignored
wisp/lease tables stay out of fail-hard staging, but the draincall and
settlement sqlmock regression tests still expected the pre-anti-join
`SELECT table_name FROM dolt_status`. Under QueryMatcherRegexp the status query
no longer matched, so the commit path returned "could not match actual sql"
before reaching the indeterminate-commit / staging-cause / circuit-trip
assertions, failing the macOS Test job.
Update the six status-query expectations to the anti-join form. Behavioral
assertions are unchanged; this only realigns the mock's expected SQL with the
production query the review validated.
---------
Co-authored-by: CI Bot <ci@beads.test>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-06 04:34:46 -07:00
|
|
|
func TestReExportCommitIndeterminate(t *testing.T) {
|
|
|
|
|
t.Parallel()
|
|
|
|
|
|
|
|
|
|
if beads.ErrCommitIndeterminate != storage.ErrCommitIndeterminate {
|
|
|
|
|
t.Error("beads.ErrCommitIndeterminate is not the shared storage sentinel value (identity broken)")
|
|
|
|
|
}
|
|
|
|
|
if dolt.ErrCommitIndeterminate != storage.ErrCommitIndeterminate {
|
|
|
|
|
t.Error("dolt.ErrCommitIndeterminate is not the shared storage sentinel value (identity broken)")
|
|
|
|
|
}
|
|
|
|
|
wrapped := fmt.Errorf("update issue: %w", storage.ErrCommitIndeterminate)
|
|
|
|
|
if !errors.Is(wrapped, beads.ErrCommitIndeterminate) {
|
|
|
|
|
t.Errorf("errors.Is(wrapped, beads.ErrCommitIndeterminate) = false; err = %v", wrapped)
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
feat(close): CloseIssueChecked — atomic guarded close in the engine (#4893)
* feat(close): CloseIssueChecked — atomic guarded close in the engine
`bd close` reads blockers (store.IsBlocked) and then closes in a SEPARATE
transaction — a TOCTOU where a blocker can clear (or the bead can be closed)
between the check and the close. Add CloseIssueChecked, which runs the
is_blocked guard AND the close inside ONE transaction, so the guard is atomic.
A Force option bypasses it (mirrors `bd close --force`). This lets the `bd`
CLI delegate its close guard to the engine instead of hand-rolling the
check-then-close dance.
- storage.ErrCloseBlocked sentinel + re-export beads.ErrCloseBlocked;
storage.CloseIssueOptions{Reason,Session,Force} and CloseIssueResult{Unchanged}.
- issueops.CloseIssueCheckedInTx: IsBlockedInTx guard (denormalized transitive
is_blocked — an open blocking dependency or an open blocking gate) then
CloseIssueInTx, sharing the transaction. Force skips the guard. A blocked
refusal returns ErrCloseBlocked and rolls back — no close, no closed event.
- Storage-interface method CloseIssueChecked implemented on DoltStore (perm +
wisp paths, mirroring CloseIssue's DOLT_COMMIT/wisp handling) and forwarded
on EmbeddedDoltStore, HookFiringStore (fires on_close on success like
CloseIssue), and InstrumentedStorage.
- Integration tests for the atomic-refuse property on the perm, wisp, and
embeddeddolt paths (blocked → refused + still open + zero closed events;
Force → closes; non-blocking dep does not trip; already-closed → Unchanged;
missing id → ErrNotFound).
Scope is one concern: category-aware reopen and the open-children close guard
are deliberately split into follow-ups. Builds under cgo and CGO_ENABLED=0.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(close): short-circuit already-closed before the is_blocked guard
CloseIssueCheckedInTx ran the is_blocked guard before the already-closed
detection, so a row that is already closed but still carries a stale
is_blocked=1 was refused with ErrCloseBlocked instead of the documented
idempotent Unchanged=true. A closed row can carry a stale flag after a
cross-clone Dolt merge — a state the schema explicitly models (GetStatistics
filters `is_blocked = 1 AND status <> 'closed'`) and a hand-resolved merge
conflict can leave indefinitely.
Detect the already-closed row before consulting is_blocked; the guard only
has meaning for an open->closed transition. This makes the non-force path
symmetric with Force, which already reached Unchanged=true on such a row by
skipping the guard.
Add a regression fixture that seeds a closed row with a stale is_blocked=1
(direct SQL + DOLT_COMMIT, mirroring blocked_merge_test.go's merge-staleness
seeding) and asserts non-force CloseIssueChecked returns Unchanged=true, not
ErrCloseBlocked.
Maintainer review fixup for PR #4893.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Eddie the Engineer <ci@beads.test>
2026-07-19 14:03:25 -07:00
|
|
|
// TestReExportCloseBlocked proves the public beads.ErrCloseBlocked alias is the
|
|
|
|
|
// same value as the internal sentinel and composes through errors.Is when
|
|
|
|
|
// wrapped — the property CloseIssueChecked callers rely on to detect a guard
|
|
|
|
|
// refusal without importing internal/storage.
|
|
|
|
|
func TestReExportCloseBlocked(t *testing.T) {
|
|
|
|
|
t.Parallel()
|
|
|
|
|
|
|
|
|
|
if beads.ErrCloseBlocked != storage.ErrCloseBlocked {
|
|
|
|
|
t.Error("beads.ErrCloseBlocked is not the internal sentinel value (identity broken)")
|
|
|
|
|
}
|
|
|
|
|
wrapped := fmt.Errorf("x: %w", beads.ErrCloseBlocked)
|
|
|
|
|
if !errors.Is(wrapped, beads.ErrCloseBlocked) {
|
|
|
|
|
t.Errorf("errors.Is(wrapped, beads.ErrCloseBlocked) = false; err = %v", wrapped)
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
feat(engine): guarded write ops — atomic metadata merge, row-version CAS, typed dep errors + events, bd close delegation (#4911)
* feat(metadata): MergeMetadata — atomic single-key metadata merge
SlotSet did a read-modify-write across TWO transactions: GetIssue (tx 1),
metadata[key]=value in memory, then UpdateIssue (tx 2). Two concurrent SlotSet
calls — even on DIFFERENT keys — both read the same base metadata and each
wrote back its whole blob, so the second clobbered the first's key. SlotClear
had the same bug. Add MergeMetadata, which does the read-modify-write inside
ONE transaction (so a concurrent conflict is retried and re-reads), stores a
JSON value (nested objects/arrays, not just strings), and reimplement SlotSet
and SlotClear on top of it so they inherit the atomicity.
- issueops.MergeMetadataInTx / DeleteMetadataInTx: read the metadata (routed
issues/wisps; missing issue -> ErrNotFound), merge/delete the one key, and
write the whole object back THROUGH UpdateIssueInTx — so the operation keeps
everything the old SlotSet got from UpdateIssue (the EventUpdated history
event with actor attribution, the configured metadata-schema validation, and
updated_at), now atomic. The read and write share the caller's transaction,
so a concurrent merge of a different key is retried and re-read, never
clobbered.
- Storage.MergeMetadata implemented on DoltStore (perm withRetryTx +
DOLT_ADD(issues,events) + DOLT_COMMIT; wisp path no commit) and
EmbeddedDoltStore (withConn); forwarded on InstrumentedStorage; auto-promoted
on HookFiringStore; no-op on the configStore mock. SlotSet/SlotClear rebuilt
on it (a string value marshals to a JSON string, byte-compatible with the
historical rewrite).
- Cross-backend conformance covers MergeMetadata + atomic SlotClear; the
8-goroutine concurrent-no-clobber test proves the B1 fix on real Dolt, and
a test asserts the EventUpdated event and schema validation are preserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(concurrency): expose a per-row version on Issue reads (RowVersion)
The engine's internal row_lock column — a random non-zero int64 the engine
rewrites on every status/ownership-mutating write and on the generic update
path — was not surfaced on the public types.Issue. Consumers that wanted a
fine-grained optimistic-concurrency token had only updated_at, which is stored
at second granularity, so two same-second writes were indistinguishable. Expose
row_lock read-only as types.Issue.RowVersion so a caller can tell same-second
writes apart.
- types.Issue gains RowVersion int64 (json:"-": a Go-only concurrency token,
never on the CLI/export JSON wire — it is opaque and equality-only). Added
row_lock to the canonical sqlbuild.IssueBaseColumns and a matching scan target
in issueops.ScanIssueFrom; both engine write stacks, wisps, and the counts /
dependents parallel scans share those, so the field hydrates everywhere from a
single-point change. Read-only — no CAS/write behavior (that is a follow-up).
- Doc is precise about coverage: RowVersion changes on claim/close/unclaim and
the generic update path; it does NOT change on some direct-UPDATE paths
(restore, compaction text), which bump updated_at — so a complete change key
combines RowVersion with updated_at, status, and labels. A created row is
already non-zero (create stamps row_lock); 0 only appears on legacy rows
backfilled by migration 0054's DEFAULT 0.
- Tests: RowVersion hydrates on read and on the list path, changes on a mutating
write, distinguishes two same-second writes (identical updated_at, distinct
RowVersion), and never appears on any JSON surface.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(deps): return a typed error on dependency type-conflict
Adding a dependency between a pair that already has an edge of a different type
is a deterministic rejection, but the two engine write paths disagreed on how
they surfaced it: one returned a typed *DependencyTypeConflictError, the other a
plain fmt.Errorf string. So a caller could errors.As the conflict on one path
but had to string-match the message on the other. Make both paths return the
same typed error, and re-export the two dependency-conflict types on the public
beads package so `bd` (and the library's own call sites) classify by errors.As
instead of parsing message text.
- issueops.AddDependencyInTx now returns *domain.DependencyTypeConflictError for
the type-conflict case (byte-identical message — the struct's Error() is the
exact former string, so no string-matcher changes). The hierarchy/cross-type
case already returned *domain.DependencyHierarchyConflictError on both paths;
self-dependency and cycle are already typed sentinels.
- beads.go re-exports DependencyTypeConflictError and
DependencyHierarchyConflictError as type aliases, so errors.As against the
public type matches the value the engine returns.
- Tests assert errors.As + the four fields + the byte-identical message on both
write paths (permanent issues->issues and the wisp-source BeginTx/Commit seam),
and lock the re-exports; the deterministic conflict is returned immediately
(never retried as a transient serialization error).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(deps): emit dependency_added / dependency_removed events
`bd dep add` and `bd dep remove` recorded no event, so the events feed never
surfaced dependency changes even though the event types (types.EventDependencyAdded
/ EventDependencyRemoved) have existed since the original SQLite backend, which
emitted them on every add/remove. The emission was dropped when that backend was
removed for the Dolt-only migration and never ported to the current write paths.
Restore it on both Dolt write plumbings, matching the original shape (an events
row on the source issue: "Added dependency: <src> <type> <target>" /
"Removed dependency on <target>").
- issueops.AddDependencyInTx / RemoveDependencyInTx record the event via
RecordEventInTable on the source's event table (wisp-routed), on the genuine
add/remove only — the idempotent same-type re-add and the no-op remove of a
missing edge record nothing.
- The event is committed with the edge: DoltStore.AddDependency/RemoveDependency
DOLT_ADD `events`, and the transaction-path (RunInTransaction / batch /
graph-apply) methods mark the source's event table dirty (events or wisp_events)
so StageAndCommit commits it — without this the event row would dangle in the
working set.
- Emission is gated to the explicit dep-add/remove verb on both plumbings: the
proxied-server repo only records when the use-case sets DepInsertOpts.EmitEvent,
which create-with-deps (implicit parent-child / --deps / waits-for, via a direct
repo Insert) does not — so `bd create --parent` produces the same history on
either backend (the embedded create path via PersistDependencies likewise emits
nothing).
- Tests on both plumbings: add/remove emit exactly once and commit (verified via
`events AS OF 'HEAD'`), idempotent re-add and no-op remove emit nothing, wisp
sources route to wisp_events, and create-with-deps emits no dependency_added.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(close): optional ExpectedVersion CAS on CloseIssueChecked
CloseIssueChecked guards a close on is_blocked in-transaction, but a caller that
read an issue and then closes it has no way to say "only if it hasn't changed
since I read it" — a lost-update window. Add an optional compare-and-swap on the
row's version (the RowVersion / row_lock token): when the caller supplies an
ExpectedVersion, the close proceeds only if the row still has that version, else
it refuses with a typed ErrVersionMismatch. The version read and the close share
one transaction, so it is a true CAS with no read-then-write gap.
- storage.ErrVersionMismatch (re-exported as beads.ErrVersionMismatch) and
CloseIssueOptions.ExpectedVersion *int64 (nil disables the check; a pointer so
nil "no check" is distinct from "require version 0").
- issueops.CheckVersionInTx reads row_lock (wisp-routed; ErrNotFound on a missing
row) and returns ErrVersionMismatch on divergence. CloseIssueCheckedInTx runs
it FIRST — before the is_blocked guard and before the Force short-circuit, so
Force bypasses only the guard, never the CAS (the version check is an
orthogonal precondition). A mismatch returns before any write, so the tx rolls
back leaving the issue open with no `closed` event (atomic refuse).
- The CAS has two limbs: the read-side check catches a writer that committed
before the close began, and on the retry-wrapped permanent path a commit-time
row_lock conflict is replayed by withRetryTx so the re-read refuses.
- Tests on both stores: match closes, stale refuses atomically (still open, zero
closed events), a committed concurrent write invalidates a captured version,
nil is unchanged behavior, Force does not bypass, missing id → ErrNotFound,
wisp sources route to the wisps table, and an already-closed re-close with the
post-close version stays idempotent (Unchanged).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(close): document RowVersion CAS coverage boundary on ExpectedVersion
Both adoption reviewers flagged that the public ExpectedVersion godoc and the
shared CloseIssueCheckedInTx doc could be misread as a full-row "unchanged"
check. RowVersion (row_lock) only tracks lifecycle/ownership writes
(status/assignee/started_at), so concurrent label, dependency, rename, or
is_blocked writes leave it untouched and are intentionally outside this CAS
boundary. Document that on both sites, grounded in the freshRowLock invariant.
Doc-only; no behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(cli): bd close delegates to the engine's guarded close
The embedded `bd close` path ran its own is_blocked pre-check and then closed in
a separate call — a read-then-write TOCTOU that duplicated a guard the engine
provides atomically. The verb now builds CloseIssueOptions and delegates to
CloseIssueChecked, which runs the guard and the close in ONE transaction; the
CLI's duplicated guard is deleted. --force maps to Options.Force; a blocked
close refuses with the blockers named and the --force hint.
Behavior-preserving on purpose (verified against the previous binary):
- The engine guard now uses the exact historical CLI predicate — refuse only on
a LIVE direct blocker (blocked && len(blockers) > 0), not on the bare
denormalized is_blocked column. A transitively-blocked child (is_blocked
inherited from a blocked parent, zero direct blockers) still closes without
--force, and a stale is_blocked with since-closed blockers self-heals, exactly
as before.
- An already-closed issue stays an idempotent success: it still appears in the
--json array and the text report (same output shape), still exits 0, but no
longer produces a spurious closed->closed audit entry, a no-op commit, or the
real-close side effects (molecule auto-close, newly-unblocked, claim-next).
Tests cover: direct blocker refuses atomically (still open) and closes with
--force; transitively-blocked and stale-is_blocked beads close without --force;
already-closed emits in --json alone and in a mixed batch; idempotent exit 0 —
on the engine (both stores) and through the bd binary.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(update): UpdateIssueChecked — optional ExpectedVersion CAS on the update
UpdateIssue applies a field-map update with no optimistic-concurrency option, so
a caller that wants "update only if unchanged since I read it" had to
read-then-write — a lost-update window. Add UpdateIssueChecked: when the caller
supplies an ExpectedVersion (the RowVersion / row_lock token), the version read
and the update share ONE transaction, refusing with the typed ErrVersionMismatch
if the row has moved — a true compare-and-swap. Nil disables the check and is
byte-identical to UpdateIssue, which is left untouched on the hot path.
- storage.UpdateIssueOptions{ExpectedVersion *int64} (aliased as
beads.UpdateIssueOptions) + Storage.UpdateIssueChecked, mirroring the
CloseIssueChecked pattern: CheckVersionInTx runs first inside the same
transaction on every route — the permanent withRetryTx path, the wisp path,
AND the demote route (an update carrying no_history/wisp routes through the
demote flow; a pure demoteToWispInTx extraction lets the check compose
atomically with the row move — verified byte-identical to the old
DemoteToWisp).
- Implemented on DoltStore and EmbeddedDoltStore; InstrumentedStorage wraps it;
HookFiringStore fires on_update only on success (never on a refused update).
- Tests on both stores: match updates and bumps RowVersion; stale refuses
atomically (field unchanged, zero updated events); a committed concurrent
write invalidates a captured version; nil behaves exactly like UpdateIssue;
wisp match/stale; demote-route match (migrates + applies) and stale (still in
issues, unchanged); missing id → ErrNotFound; the hook decorator fires once on
success and never on refusal.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor(cli): proxied bd close delegates to a library checked close
The proxied-server `bd close` path duplicated the close guard in the CLI:
it queried blockers via the dependency use-case, refused with its own message,
and then closed in a separate use-case call. Move the guard into the library —
IssueUseCase gains CloseIssueChecked / CloseWispChecked, which run the same
live-direct-blocker predicate (refuse only on blocked && len(blockers) > 0)
and then the untouched unchecked close — and thin the CLI verb to a single
delegated call. Both `bd close` paths now enforce the guard in the library and
emit the byte-identical refusal: the same storage.ErrCloseBlocked sentinel
("cannot close blocked issue: <id> is blocked by [...]") plus the
"(use --force to override)" hint.
- Guard and close run on the one pinned unit-of-work connection (the whole
close batch is a single transaction), and the guard is read-only and refuses
before any write — a refused id leaves the shared batch transaction clean,
and an all-refused batch skips the commit entirely, exactly as before.
- The unchecked CloseIssue / CloseWisp are untouched: internal closes (molecule
auto-close, gate closes, bd todo done) intentionally stay unguarded.
- Behavior-parity preserved: guard ordering (validate → epic-child → gate →
blocked), exit codes, JSON/batch shape, already-closed handling, audit fields,
and commit-message contents are unchanged; a transitively-blocked bead
(is_blocked with no direct open blocker) still closes without --force, and
--force still bypasses. The guard-failure diagnostic now uses the embedded
path's generic wording (further convergence; nothing matched the old text).
- Tests: use-case level on real Dolt (direct blocker refuses with blockers
named + still open, transitive-only closes, force closes, already-closed
parity, wisp refuse/force) plus the proxied CLI integration suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): cross-backend dependency-event + RowVersion parity
Maintainer review fixups for PR #4911 (adopt-pr review loop):
- RowVersion: the proxied/domain create (insertIssueRow) and generic Update
paths never stamped or rewrote row_lock, so an issue written through the
proxied-server backend read back RowVersion 0 on create and unchanged on
update -- a stale CAS token that violated the types.Issue.RowVersion
contract on that backend. Stamp a fresh non-zero row_lock on insert (and the
duplicate-key rewrite) and append RowLockClause() to the generic update,
mirroring the classic issueops insert/update invariant. Adds domain/db
regression tests proving CreateIssue/CreateWisp are non-zero and
UpdateIssue/UpdateWisp change it.
- Dependency events: embedded issueops AddDependencyInTx/RemoveDependencyInTx
recorded history unconditionally, so structural create-with-deps
(bd create --parent/--deps/--waits-for) and reparent (bd update --parent)
emitted dependency_added/removed on embedded but were silent on the proxied
backend, diverging issue history for identical commands. Gate the embedded
emit on EmitEvent and thread it through AddDependencyWithOptions /
RemoveDependencyWithOptions on both the store and transaction interfaces:
the plain AddDependency/RemoveDependency are the no-event structural default
(create-with-deps, reparent), while the explicit dep verbs (bd dep add/remove,
bd link, bd relate/unrelate) pass EmitEvent -- matching the proxied
DepInsertOpts.EmitEvent gate. DoltStore stages the events table only when an
event was actually written (GH#2455). Corrects the false
"parity with PersistDependencies" comments and adds symmetric embedded/proxied
dep-event coverage for create-with-parent/deps and structural removal.
- Retain iteration-1 fixups: heal molecules on already-closed re-close,
idempotent proxied checked-close, and documented public Storage growth.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(storage): drop always-constant event param from fireDependencyHookByID (unparam)
golangci-lint (latest, v2.12.2 in CI) unparam flags fireDependencyHookByID:
its `event` parameter always receives hooks.EventUpdate. The parameter was
already effectively constant before this PR (both base call sites passed
EventUpdate); this PR adds two more call sites, all EventUpdate. Remove the
dead parameter and hardcode hooks.EventUpdate at the single runner.Run call.
All four call sites pass hooks.EventUpdate, so behavior is unchanged; go
build, go vet, and a scoped golangci-lint run pass clean.
Maintainer fixup during adoption finalize (PR #4911).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Eddie the Engineer <ci@beads.test>
2026-07-21 18:13:03 -07:00
|
|
|
// TestReExportVersionMismatch proves the public beads.ErrVersionMismatch alias
|
|
|
|
|
// is the same value as the internal sentinel and composes through errors.Is when
|
|
|
|
|
// wrapped — the property a CloseIssueChecked caller relies on to detect an
|
|
|
|
|
// optimistic-concurrency refusal without importing internal/storage.
|
|
|
|
|
func TestReExportVersionMismatch(t *testing.T) {
|
|
|
|
|
t.Parallel()
|
|
|
|
|
|
|
|
|
|
if beads.ErrVersionMismatch != storage.ErrVersionMismatch {
|
|
|
|
|
t.Error("beads.ErrVersionMismatch is not the internal sentinel value (identity broken)")
|
|
|
|
|
}
|
|
|
|
|
wrapped := fmt.Errorf("x: %w", beads.ErrVersionMismatch)
|
|
|
|
|
if !errors.Is(wrapped, beads.ErrVersionMismatch) {
|
|
|
|
|
t.Errorf("errors.Is(wrapped, beads.ErrVersionMismatch) = false; err = %v", wrapped)
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// TestUpdateIssueOptionsIsExported proves the public beads.UpdateIssueOptions
|
|
|
|
|
// alias is usable from outside the module and its ExpectedVersion compare-and-
|
|
|
|
|
// swap field round-trips — the type a caller names to opt a
|
|
|
|
|
// Storage.UpdateIssueChecked into optimistic concurrency without importing
|
|
|
|
|
// internal/storage. The zero value must leave ExpectedVersion nil (no check).
|
|
|
|
|
func TestUpdateIssueOptionsIsExported(t *testing.T) {
|
|
|
|
|
t.Parallel()
|
|
|
|
|
|
|
|
|
|
v := int64(7)
|
|
|
|
|
opts := beads.UpdateIssueOptions{ExpectedVersion: &v}
|
|
|
|
|
if opts.ExpectedVersion == nil || *opts.ExpectedVersion != 7 {
|
|
|
|
|
t.Fatalf("ExpectedVersion did not round-trip through the exported alias: %+v", opts)
|
|
|
|
|
}
|
|
|
|
|
if (beads.UpdateIssueOptions{}).ExpectedVersion != nil {
|
|
|
|
|
t.Fatal("zero-value UpdateIssueOptions must have a nil ExpectedVersion (no check)")
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
feat(errors): typed dependency error taxonomy on the public beads package (#4892)
* feat(errors): typed dependency error taxonomy on the public beads package
S1 of the guarded-ops initiative: let the bd CLI (and the library's own call
sites) classify dependency-write failures with errors.Is instead of
string-matching message text, without changing any message (wrap-preserving),
so existing string matchers keep working during the migration.
- Add ErrSelfDependency / ErrDependencyCycle sentinels in
internal/storage/domain and route EVERY production self-dep/cycle emitter
through them: issueops.CheckDependencyCycleInTx, the dolt cross-tier
scheduling-cycle check, the domain use-case add + bulk-add paths, and the
db SQL repository. Canonical-message sites stay byte-identical (sentinel as
the static prefix via %w, or a bare sentinel return); already-divergent
sites (bulk, db) append the sentinel with %w.
- Re-export ErrNotFound, ErrAlreadyClaimed, ErrNotClaimable, ErrSelfDependency,
ErrDependencyCycle as var aliases on the root beads package, so beads.ErrX is
identity-equal to the internal sentinel and errors.Is composes across the
boundary.
- Contract tests: wrap-preserving assertions with EXACT message byte-identity;
a real-production-return test through the domain use case; errors.Is
regression assertions on the dolt cross-tier cycle path and the db self-dep
path. The pre-existing self-dependency test passes unchanged, proving the
canonical messages stayed byte-identical.
A red-team review caught that an earlier draft converted only the same-tier
issueops path, leaving the dolt cross-tier check (which sets SkipCycleCheck and
bypasses issueops) returning an untyped string — errors.Is was false for
cross-tier cycles on the same public DoltStore. All plumbings are now uniform.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(errors): type bulk dependency self-dep and cycle rejections without changing their text
The dependency-add bulk path (proxied `bd dep add` / `bd link`, and the embedded
bulk CLI) surfaces its error verbatim via HandleErrorRespectJSON("%v", err).
Wrapping a rejection with a plain "%w" appends the sentinel's own text to an
already-complete message and changes user-facing output, so the bulk family was
left partially untyped and a scheduling self-edge was misreported as a cycle.
Type every dependency-add bulk rejection so callers can errors.Is it while the
rendered message stays byte-for-byte identical to the pre-taxonomy text:
- Route the per-edge and final domain cycle rejections through a small
string-preserving cycleError wrapper (Error() returns the legacy text,
Unwrap() returns ErrDependencyCycle).
- Guard IssueID==DependsOnID in domain addBulk for all dep types before the
cycle probe, so a scheduling self-edge is ErrSelfDependency instead of
tripping HasCycle (or the final CycleThroughEdges gate) and surfacing as a
cycle; the message matches every other self-dep site.
- Type the embedded bulk CLI final gate (cmd/bd addBulkDependenciesInTx) via the
exported domain.NewCycleError helper, so errors.Is(ErrDependencyCycle) holds
regardless of backend/plumbing.
- Lead the defensive repo-layer self-dep guard with the sentinel like every
other self-dep site instead of appending it.
Add exact-message + errors.Is assertions for the bulk cycle paths (per-edge,
final SkipPerEdgeCycleCheck, embedded final gate) and new bulk self-edge tests
(default, SkipPerEdgeCycleCheck, non-scheduling). Scope ErrDependencyCycle's doc
to the dependency-add family.
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 18:29:07 -07:00
|
|
|
// TestReExportedSentinelIdentity proves each public sentinel is the SAME value
|
|
|
|
|
// as the internal one it aliases, so errors.Is composes across the package
|
|
|
|
|
// boundary without any bridging.
|
|
|
|
|
func TestReExportedSentinelIdentity(t *testing.T) {
|
|
|
|
|
t.Parallel()
|
|
|
|
|
|
|
|
|
|
cases := []struct {
|
|
|
|
|
name string
|
|
|
|
|
exported error
|
|
|
|
|
internal error
|
|
|
|
|
}{
|
|
|
|
|
{"ErrNotFound", beads.ErrNotFound, storage.ErrNotFound},
|
|
|
|
|
{"ErrAlreadyClaimed", beads.ErrAlreadyClaimed, storage.ErrAlreadyClaimed},
|
|
|
|
|
{"ErrNotClaimable", beads.ErrNotClaimable, storage.ErrNotClaimable},
|
feat(engine): guarded write ops — atomic metadata merge, row-version CAS, typed dep errors + events, bd close delegation (#4911)
* feat(metadata): MergeMetadata — atomic single-key metadata merge
SlotSet did a read-modify-write across TWO transactions: GetIssue (tx 1),
metadata[key]=value in memory, then UpdateIssue (tx 2). Two concurrent SlotSet
calls — even on DIFFERENT keys — both read the same base metadata and each
wrote back its whole blob, so the second clobbered the first's key. SlotClear
had the same bug. Add MergeMetadata, which does the read-modify-write inside
ONE transaction (so a concurrent conflict is retried and re-reads), stores a
JSON value (nested objects/arrays, not just strings), and reimplement SlotSet
and SlotClear on top of it so they inherit the atomicity.
- issueops.MergeMetadataInTx / DeleteMetadataInTx: read the metadata (routed
issues/wisps; missing issue -> ErrNotFound), merge/delete the one key, and
write the whole object back THROUGH UpdateIssueInTx — so the operation keeps
everything the old SlotSet got from UpdateIssue (the EventUpdated history
event with actor attribution, the configured metadata-schema validation, and
updated_at), now atomic. The read and write share the caller's transaction,
so a concurrent merge of a different key is retried and re-read, never
clobbered.
- Storage.MergeMetadata implemented on DoltStore (perm withRetryTx +
DOLT_ADD(issues,events) + DOLT_COMMIT; wisp path no commit) and
EmbeddedDoltStore (withConn); forwarded on InstrumentedStorage; auto-promoted
on HookFiringStore; no-op on the configStore mock. SlotSet/SlotClear rebuilt
on it (a string value marshals to a JSON string, byte-compatible with the
historical rewrite).
- Cross-backend conformance covers MergeMetadata + atomic SlotClear; the
8-goroutine concurrent-no-clobber test proves the B1 fix on real Dolt, and
a test asserts the EventUpdated event and schema validation are preserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(concurrency): expose a per-row version on Issue reads (RowVersion)
The engine's internal row_lock column — a random non-zero int64 the engine
rewrites on every status/ownership-mutating write and on the generic update
path — was not surfaced on the public types.Issue. Consumers that wanted a
fine-grained optimistic-concurrency token had only updated_at, which is stored
at second granularity, so two same-second writes were indistinguishable. Expose
row_lock read-only as types.Issue.RowVersion so a caller can tell same-second
writes apart.
- types.Issue gains RowVersion int64 (json:"-": a Go-only concurrency token,
never on the CLI/export JSON wire — it is opaque and equality-only). Added
row_lock to the canonical sqlbuild.IssueBaseColumns and a matching scan target
in issueops.ScanIssueFrom; both engine write stacks, wisps, and the counts /
dependents parallel scans share those, so the field hydrates everywhere from a
single-point change. Read-only — no CAS/write behavior (that is a follow-up).
- Doc is precise about coverage: RowVersion changes on claim/close/unclaim and
the generic update path; it does NOT change on some direct-UPDATE paths
(restore, compaction text), which bump updated_at — so a complete change key
combines RowVersion with updated_at, status, and labels. A created row is
already non-zero (create stamps row_lock); 0 only appears on legacy rows
backfilled by migration 0054's DEFAULT 0.
- Tests: RowVersion hydrates on read and on the list path, changes on a mutating
write, distinguishes two same-second writes (identical updated_at, distinct
RowVersion), and never appears on any JSON surface.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(deps): return a typed error on dependency type-conflict
Adding a dependency between a pair that already has an edge of a different type
is a deterministic rejection, but the two engine write paths disagreed on how
they surfaced it: one returned a typed *DependencyTypeConflictError, the other a
plain fmt.Errorf string. So a caller could errors.As the conflict on one path
but had to string-match the message on the other. Make both paths return the
same typed error, and re-export the two dependency-conflict types on the public
beads package so `bd` (and the library's own call sites) classify by errors.As
instead of parsing message text.
- issueops.AddDependencyInTx now returns *domain.DependencyTypeConflictError for
the type-conflict case (byte-identical message — the struct's Error() is the
exact former string, so no string-matcher changes). The hierarchy/cross-type
case already returned *domain.DependencyHierarchyConflictError on both paths;
self-dependency and cycle are already typed sentinels.
- beads.go re-exports DependencyTypeConflictError and
DependencyHierarchyConflictError as type aliases, so errors.As against the
public type matches the value the engine returns.
- Tests assert errors.As + the four fields + the byte-identical message on both
write paths (permanent issues->issues and the wisp-source BeginTx/Commit seam),
and lock the re-exports; the deterministic conflict is returned immediately
(never retried as a transient serialization error).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(deps): emit dependency_added / dependency_removed events
`bd dep add` and `bd dep remove` recorded no event, so the events feed never
surfaced dependency changes even though the event types (types.EventDependencyAdded
/ EventDependencyRemoved) have existed since the original SQLite backend, which
emitted them on every add/remove. The emission was dropped when that backend was
removed for the Dolt-only migration and never ported to the current write paths.
Restore it on both Dolt write plumbings, matching the original shape (an events
row on the source issue: "Added dependency: <src> <type> <target>" /
"Removed dependency on <target>").
- issueops.AddDependencyInTx / RemoveDependencyInTx record the event via
RecordEventInTable on the source's event table (wisp-routed), on the genuine
add/remove only — the idempotent same-type re-add and the no-op remove of a
missing edge record nothing.
- The event is committed with the edge: DoltStore.AddDependency/RemoveDependency
DOLT_ADD `events`, and the transaction-path (RunInTransaction / batch /
graph-apply) methods mark the source's event table dirty (events or wisp_events)
so StageAndCommit commits it — without this the event row would dangle in the
working set.
- Emission is gated to the explicit dep-add/remove verb on both plumbings: the
proxied-server repo only records when the use-case sets DepInsertOpts.EmitEvent,
which create-with-deps (implicit parent-child / --deps / waits-for, via a direct
repo Insert) does not — so `bd create --parent` produces the same history on
either backend (the embedded create path via PersistDependencies likewise emits
nothing).
- Tests on both plumbings: add/remove emit exactly once and commit (verified via
`events AS OF 'HEAD'`), idempotent re-add and no-op remove emit nothing, wisp
sources route to wisp_events, and create-with-deps emits no dependency_added.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(close): optional ExpectedVersion CAS on CloseIssueChecked
CloseIssueChecked guards a close on is_blocked in-transaction, but a caller that
read an issue and then closes it has no way to say "only if it hasn't changed
since I read it" — a lost-update window. Add an optional compare-and-swap on the
row's version (the RowVersion / row_lock token): when the caller supplies an
ExpectedVersion, the close proceeds only if the row still has that version, else
it refuses with a typed ErrVersionMismatch. The version read and the close share
one transaction, so it is a true CAS with no read-then-write gap.
- storage.ErrVersionMismatch (re-exported as beads.ErrVersionMismatch) and
CloseIssueOptions.ExpectedVersion *int64 (nil disables the check; a pointer so
nil "no check" is distinct from "require version 0").
- issueops.CheckVersionInTx reads row_lock (wisp-routed; ErrNotFound on a missing
row) and returns ErrVersionMismatch on divergence. CloseIssueCheckedInTx runs
it FIRST — before the is_blocked guard and before the Force short-circuit, so
Force bypasses only the guard, never the CAS (the version check is an
orthogonal precondition). A mismatch returns before any write, so the tx rolls
back leaving the issue open with no `closed` event (atomic refuse).
- The CAS has two limbs: the read-side check catches a writer that committed
before the close began, and on the retry-wrapped permanent path a commit-time
row_lock conflict is replayed by withRetryTx so the re-read refuses.
- Tests on both stores: match closes, stale refuses atomically (still open, zero
closed events), a committed concurrent write invalidates a captured version,
nil is unchanged behavior, Force does not bypass, missing id → ErrNotFound,
wisp sources route to the wisps table, and an already-closed re-close with the
post-close version stays idempotent (Unchanged).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(close): document RowVersion CAS coverage boundary on ExpectedVersion
Both adoption reviewers flagged that the public ExpectedVersion godoc and the
shared CloseIssueCheckedInTx doc could be misread as a full-row "unchanged"
check. RowVersion (row_lock) only tracks lifecycle/ownership writes
(status/assignee/started_at), so concurrent label, dependency, rename, or
is_blocked writes leave it untouched and are intentionally outside this CAS
boundary. Document that on both sites, grounded in the freshRowLock invariant.
Doc-only; no behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(cli): bd close delegates to the engine's guarded close
The embedded `bd close` path ran its own is_blocked pre-check and then closed in
a separate call — a read-then-write TOCTOU that duplicated a guard the engine
provides atomically. The verb now builds CloseIssueOptions and delegates to
CloseIssueChecked, which runs the guard and the close in ONE transaction; the
CLI's duplicated guard is deleted. --force maps to Options.Force; a blocked
close refuses with the blockers named and the --force hint.
Behavior-preserving on purpose (verified against the previous binary):
- The engine guard now uses the exact historical CLI predicate — refuse only on
a LIVE direct blocker (blocked && len(blockers) > 0), not on the bare
denormalized is_blocked column. A transitively-blocked child (is_blocked
inherited from a blocked parent, zero direct blockers) still closes without
--force, and a stale is_blocked with since-closed blockers self-heals, exactly
as before.
- An already-closed issue stays an idempotent success: it still appears in the
--json array and the text report (same output shape), still exits 0, but no
longer produces a spurious closed->closed audit entry, a no-op commit, or the
real-close side effects (molecule auto-close, newly-unblocked, claim-next).
Tests cover: direct blocker refuses atomically (still open) and closes with
--force; transitively-blocked and stale-is_blocked beads close without --force;
already-closed emits in --json alone and in a mixed batch; idempotent exit 0 —
on the engine (both stores) and through the bd binary.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(update): UpdateIssueChecked — optional ExpectedVersion CAS on the update
UpdateIssue applies a field-map update with no optimistic-concurrency option, so
a caller that wants "update only if unchanged since I read it" had to
read-then-write — a lost-update window. Add UpdateIssueChecked: when the caller
supplies an ExpectedVersion (the RowVersion / row_lock token), the version read
and the update share ONE transaction, refusing with the typed ErrVersionMismatch
if the row has moved — a true compare-and-swap. Nil disables the check and is
byte-identical to UpdateIssue, which is left untouched on the hot path.
- storage.UpdateIssueOptions{ExpectedVersion *int64} (aliased as
beads.UpdateIssueOptions) + Storage.UpdateIssueChecked, mirroring the
CloseIssueChecked pattern: CheckVersionInTx runs first inside the same
transaction on every route — the permanent withRetryTx path, the wisp path,
AND the demote route (an update carrying no_history/wisp routes through the
demote flow; a pure demoteToWispInTx extraction lets the check compose
atomically with the row move — verified byte-identical to the old
DemoteToWisp).
- Implemented on DoltStore and EmbeddedDoltStore; InstrumentedStorage wraps it;
HookFiringStore fires on_update only on success (never on a refused update).
- Tests on both stores: match updates and bumps RowVersion; stale refuses
atomically (field unchanged, zero updated events); a committed concurrent
write invalidates a captured version; nil behaves exactly like UpdateIssue;
wisp match/stale; demote-route match (migrates + applies) and stale (still in
issues, unchanged); missing id → ErrNotFound; the hook decorator fires once on
success and never on refusal.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor(cli): proxied bd close delegates to a library checked close
The proxied-server `bd close` path duplicated the close guard in the CLI:
it queried blockers via the dependency use-case, refused with its own message,
and then closed in a separate use-case call. Move the guard into the library —
IssueUseCase gains CloseIssueChecked / CloseWispChecked, which run the same
live-direct-blocker predicate (refuse only on blocked && len(blockers) > 0)
and then the untouched unchecked close — and thin the CLI verb to a single
delegated call. Both `bd close` paths now enforce the guard in the library and
emit the byte-identical refusal: the same storage.ErrCloseBlocked sentinel
("cannot close blocked issue: <id> is blocked by [...]") plus the
"(use --force to override)" hint.
- Guard and close run on the one pinned unit-of-work connection (the whole
close batch is a single transaction), and the guard is read-only and refuses
before any write — a refused id leaves the shared batch transaction clean,
and an all-refused batch skips the commit entirely, exactly as before.
- The unchecked CloseIssue / CloseWisp are untouched: internal closes (molecule
auto-close, gate closes, bd todo done) intentionally stay unguarded.
- Behavior-parity preserved: guard ordering (validate → epic-child → gate →
blocked), exit codes, JSON/batch shape, already-closed handling, audit fields,
and commit-message contents are unchanged; a transitively-blocked bead
(is_blocked with no direct open blocker) still closes without --force, and
--force still bypasses. The guard-failure diagnostic now uses the embedded
path's generic wording (further convergence; nothing matched the old text).
- Tests: use-case level on real Dolt (direct blocker refuses with blockers
named + still open, transitive-only closes, force closes, already-closed
parity, wisp refuse/force) plus the proxied CLI integration suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): cross-backend dependency-event + RowVersion parity
Maintainer review fixups for PR #4911 (adopt-pr review loop):
- RowVersion: the proxied/domain create (insertIssueRow) and generic Update
paths never stamped or rewrote row_lock, so an issue written through the
proxied-server backend read back RowVersion 0 on create and unchanged on
update -- a stale CAS token that violated the types.Issue.RowVersion
contract on that backend. Stamp a fresh non-zero row_lock on insert (and the
duplicate-key rewrite) and append RowLockClause() to the generic update,
mirroring the classic issueops insert/update invariant. Adds domain/db
regression tests proving CreateIssue/CreateWisp are non-zero and
UpdateIssue/UpdateWisp change it.
- Dependency events: embedded issueops AddDependencyInTx/RemoveDependencyInTx
recorded history unconditionally, so structural create-with-deps
(bd create --parent/--deps/--waits-for) and reparent (bd update --parent)
emitted dependency_added/removed on embedded but were silent on the proxied
backend, diverging issue history for identical commands. Gate the embedded
emit on EmitEvent and thread it through AddDependencyWithOptions /
RemoveDependencyWithOptions on both the store and transaction interfaces:
the plain AddDependency/RemoveDependency are the no-event structural default
(create-with-deps, reparent), while the explicit dep verbs (bd dep add/remove,
bd link, bd relate/unrelate) pass EmitEvent -- matching the proxied
DepInsertOpts.EmitEvent gate. DoltStore stages the events table only when an
event was actually written (GH#2455). Corrects the false
"parity with PersistDependencies" comments and adds symmetric embedded/proxied
dep-event coverage for create-with-parent/deps and structural removal.
- Retain iteration-1 fixups: heal molecules on already-closed re-close,
idempotent proxied checked-close, and documented public Storage growth.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(storage): drop always-constant event param from fireDependencyHookByID (unparam)
golangci-lint (latest, v2.12.2 in CI) unparam flags fireDependencyHookByID:
its `event` parameter always receives hooks.EventUpdate. The parameter was
already effectively constant before this PR (both base call sites passed
EventUpdate); this PR adds two more call sites, all EventUpdate. Remove the
dead parameter and hardcode hooks.EventUpdate at the single runner.Run call.
All four call sites pass hooks.EventUpdate, so behavior is unchanged; go
build, go vet, and a scoped golangci-lint run pass clean.
Maintainer fixup during adoption finalize (PR #4911).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Eddie the Engineer <ci@beads.test>
2026-07-21 18:13:03 -07:00
|
|
|
{"ErrVersionMismatch", beads.ErrVersionMismatch, storage.ErrVersionMismatch},
|
feat(errors): typed dependency error taxonomy on the public beads package (#4892)
* feat(errors): typed dependency error taxonomy on the public beads package
S1 of the guarded-ops initiative: let the bd CLI (and the library's own call
sites) classify dependency-write failures with errors.Is instead of
string-matching message text, without changing any message (wrap-preserving),
so existing string matchers keep working during the migration.
- Add ErrSelfDependency / ErrDependencyCycle sentinels in
internal/storage/domain and route EVERY production self-dep/cycle emitter
through them: issueops.CheckDependencyCycleInTx, the dolt cross-tier
scheduling-cycle check, the domain use-case add + bulk-add paths, and the
db SQL repository. Canonical-message sites stay byte-identical (sentinel as
the static prefix via %w, or a bare sentinel return); already-divergent
sites (bulk, db) append the sentinel with %w.
- Re-export ErrNotFound, ErrAlreadyClaimed, ErrNotClaimable, ErrSelfDependency,
ErrDependencyCycle as var aliases on the root beads package, so beads.ErrX is
identity-equal to the internal sentinel and errors.Is composes across the
boundary.
- Contract tests: wrap-preserving assertions with EXACT message byte-identity;
a real-production-return test through the domain use case; errors.Is
regression assertions on the dolt cross-tier cycle path and the db self-dep
path. The pre-existing self-dependency test passes unchanged, proving the
canonical messages stayed byte-identical.
A red-team review caught that an earlier draft converted only the same-tier
issueops path, leaving the dolt cross-tier check (which sets SkipCycleCheck and
bypasses issueops) returning an untyped string — errors.Is was false for
cross-tier cycles on the same public DoltStore. All plumbings are now uniform.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(errors): type bulk dependency self-dep and cycle rejections without changing their text
The dependency-add bulk path (proxied `bd dep add` / `bd link`, and the embedded
bulk CLI) surfaces its error verbatim via HandleErrorRespectJSON("%v", err).
Wrapping a rejection with a plain "%w" appends the sentinel's own text to an
already-complete message and changes user-facing output, so the bulk family was
left partially untyped and a scheduling self-edge was misreported as a cycle.
Type every dependency-add bulk rejection so callers can errors.Is it while the
rendered message stays byte-for-byte identical to the pre-taxonomy text:
- Route the per-edge and final domain cycle rejections through a small
string-preserving cycleError wrapper (Error() returns the legacy text,
Unwrap() returns ErrDependencyCycle).
- Guard IssueID==DependsOnID in domain addBulk for all dep types before the
cycle probe, so a scheduling self-edge is ErrSelfDependency instead of
tripping HasCycle (or the final CycleThroughEdges gate) and surfacing as a
cycle; the message matches every other self-dep site.
- Type the embedded bulk CLI final gate (cmd/bd addBulkDependenciesInTx) via the
exported domain.NewCycleError helper, so errors.Is(ErrDependencyCycle) holds
regardless of backend/plumbing.
- Lead the defensive repo-layer self-dep guard with the sentinel like every
other self-dep site instead of appending it.
Add exact-message + errors.Is assertions for the bulk cycle paths (per-edge,
final SkipPerEdgeCycleCheck, embedded final gate) and new bulk self-edge tests
(default, SkipPerEdgeCycleCheck, non-scheduling). Scope ErrDependencyCycle's doc
to the dependency-add family.
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 18:29:07 -07:00
|
|
|
{"ErrSelfDependency", beads.ErrSelfDependency, domain.ErrSelfDependency},
|
|
|
|
|
{"ErrDependencyCycle", beads.ErrDependencyCycle, domain.ErrDependencyCycle},
|
feat(issueops): name the two dependency-endpoint refusals (bd-yby99.9)
DependencyEditor.AddDependencies refuses an edge whose SOURCE names no row,
and one whose TARGET names no row this database can see the absence of. Every
other refusal the role can raise hands a caller something to branch on —
ErrSelfDependency, ErrDependencyCycle, *DependencyTypeConflictError,
*DependencyHierarchyConflictError — and these two handed back "an error", with
different text on each backend. A programmatic consumer could not tell a ghost
endpoint from an infrastructure failure, and the role's own doc said so rather
than promising the anonymity would last.
Both now carry an identity: ErrDependencySourceNotFound and
ErrDependencyTargetNotFound, wrapped by a *DependencyEndpointNotFoundError that
names the refused edge and which of its endpoints was absent. Two sentinels
rather than one because the two are separate answers — a ghost source is always
a bad id, while a target is refused only where this database would have held
it. What is NOT refused is unchanged: an "external:" reference and another
repository's id are still stored as external targets.
The store-backed body and the cross-tier target precheck mint the refusal where
they already read the row. The domain repository has no such read: it learns of
the absence from a foreign-key violation, so it reads both endpoints back on
the transaction that refused, only on the refusal path, and never downgrades
the refusal to a probe's failure. Taking the identity out of the driver's
constraint name would have been the thing a typed refusal exists to avoid. One
user-visible consequence: the proxied-server path now renders "issue <id> not
found" where it used to render a raw foreign-key violation, which is what the
embedded path has always said.
The guarded create classifies the new type where it classified the raw
foreign-key signal, so a missing dependency, parent or waits-for target is
still ErrValidation wrapping ErrNotFound on every backend.
RemoveDependency is deliberately untouched. A removal that finds no edge is
Removed false with a nil error, so there is no refusal there to name.
The shared contract asserts the sentinel and the typed fields for each endpoint
in TWO POSITIONS: alone in a request, and mid-batch where the refusal competes
with the rollback of an edge already written, the half that also reads the
graph back at zero edges. Both cases were verified red on all three backends
before the production change — "issue <id> not found" untyped on the two store
paths, "Error 1452 ... Foreign key violation" on the unit-of-work path.
Agent-Signature: claude-code-claude-opus-5-unknown-reasoning on behalf of CI Bot
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-07 19:45:54 +00:00
|
|
|
{"ErrDependencySourceNotFound", beads.ErrDependencySourceNotFound, domain.ErrDependencySourceNotFound},
|
|
|
|
|
{"ErrDependencyTargetNotFound", beads.ErrDependencyTargetNotFound, domain.ErrDependencyTargetNotFound},
|
feat(validate): typed length bounds for assignee, owner, and label (#4894)
The assignee, owner, and label columns are VARCHAR(255), but the engine never
validated their length: an over-255 assignee/owner made the row INSERT/UPDATE
fail with a raw backend "data too long" error, and an over-255 label went
through INSERT IGNORE and was SILENTLY TRUNCATED — storing a label the caller
never sent. Validate these three fields up front and return a typed
ErrFieldTooLong so `bd` (and the library's own call sites) get a clean, typed
rejection instead of a raw SQL error or silent corruption.
- types.MaxFieldLen (255), types.ErrFieldTooLong (re-exported as
beads.ErrFieldTooLong), and types.CheckFieldLen using utf8.RuneCountInString
— rune count, not bytes, so a multibyte value up to 255 characters fits the
VARCHAR(255) column and passes while a 256-rune value fails.
- Guard every raw assignee/owner/label write on BOTH engine write stacks:
the embedded issueops/DoltStore path (ValidateWithCustom + ValidateForImport
for assignee/owner, UpdateIssueInTx pre-pass, AddLabelInTx and PersistLabels
for labels, ClaimIssueInTx for the actor written as assignee) AND the
proxied-server domain/db (uow) path (issue insertIssueRow + Update + Claim,
label Insert) — which a first pass missed, so the silent-truncation bug
persisted there.
- Tests on both stacks: over-length create/update/label/claim reject with
ErrFieldTooLong and persist nothing (single and bulk); a 255-rune multibyte
value round-trips unchanged and a 256-rune one is rejected (rune-count proof
end to end); a 255-char value stores intact.
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-19 23:03:07 -07:00
|
|
|
{"ErrFieldTooLong", beads.ErrFieldTooLong, types.ErrFieldTooLong},
|
feat(errors): typed dependency error taxonomy on the public beads package (#4892)
* feat(errors): typed dependency error taxonomy on the public beads package
S1 of the guarded-ops initiative: let the bd CLI (and the library's own call
sites) classify dependency-write failures with errors.Is instead of
string-matching message text, without changing any message (wrap-preserving),
so existing string matchers keep working during the migration.
- Add ErrSelfDependency / ErrDependencyCycle sentinels in
internal/storage/domain and route EVERY production self-dep/cycle emitter
through them: issueops.CheckDependencyCycleInTx, the dolt cross-tier
scheduling-cycle check, the domain use-case add + bulk-add paths, and the
db SQL repository. Canonical-message sites stay byte-identical (sentinel as
the static prefix via %w, or a bare sentinel return); already-divergent
sites (bulk, db) append the sentinel with %w.
- Re-export ErrNotFound, ErrAlreadyClaimed, ErrNotClaimable, ErrSelfDependency,
ErrDependencyCycle as var aliases on the root beads package, so beads.ErrX is
identity-equal to the internal sentinel and errors.Is composes across the
boundary.
- Contract tests: wrap-preserving assertions with EXACT message byte-identity;
a real-production-return test through the domain use case; errors.Is
regression assertions on the dolt cross-tier cycle path and the db self-dep
path. The pre-existing self-dependency test passes unchanged, proving the
canonical messages stayed byte-identical.
A red-team review caught that an earlier draft converted only the same-tier
issueops path, leaving the dolt cross-tier check (which sets SkipCycleCheck and
bypasses issueops) returning an untyped string — errors.Is was false for
cross-tier cycles on the same public DoltStore. All plumbings are now uniform.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(errors): type bulk dependency self-dep and cycle rejections without changing their text
The dependency-add bulk path (proxied `bd dep add` / `bd link`, and the embedded
bulk CLI) surfaces its error verbatim via HandleErrorRespectJSON("%v", err).
Wrapping a rejection with a plain "%w" appends the sentinel's own text to an
already-complete message and changes user-facing output, so the bulk family was
left partially untyped and a scheduling self-edge was misreported as a cycle.
Type every dependency-add bulk rejection so callers can errors.Is it while the
rendered message stays byte-for-byte identical to the pre-taxonomy text:
- Route the per-edge and final domain cycle rejections through a small
string-preserving cycleError wrapper (Error() returns the legacy text,
Unwrap() returns ErrDependencyCycle).
- Guard IssueID==DependsOnID in domain addBulk for all dep types before the
cycle probe, so a scheduling self-edge is ErrSelfDependency instead of
tripping HasCycle (or the final CycleThroughEdges gate) and surfacing as a
cycle; the message matches every other self-dep site.
- Type the embedded bulk CLI final gate (cmd/bd addBulkDependenciesInTx) via the
exported domain.NewCycleError helper, so errors.Is(ErrDependencyCycle) holds
regardless of backend/plumbing.
- Lead the defensive repo-layer self-dep guard with the sentinel like every
other self-dep site instead of appending it.
Add exact-message + errors.Is assertions for the bulk cycle paths (per-edge,
final SkipPerEdgeCycleCheck, embedded final gate) and new bulk self-edge tests
(default, SkipPerEdgeCycleCheck, non-scheduling). Scope ErrDependencyCycle's doc
to the dependency-add family.
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 18:29:07 -07:00
|
|
|
}
|
|
|
|
|
for _, tc := range cases {
|
|
|
|
|
if tc.exported != tc.internal {
|
|
|
|
|
t.Errorf("beads.%s is not the internal sentinel value (identity broken)", tc.name)
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// stubDepRepo satisfies domain.DependencySQLRepository via the embedded
|
|
|
|
|
// interface; only the two methods dependencyUseCaseImpl.add consults before
|
|
|
|
|
// returning a self-dep/cycle sentinel are implemented. Any other call would
|
|
|
|
|
// nil-panic, which keeps the stub honest about the exact surface these
|
|
|
|
|
// branches touch.
|
|
|
|
|
type stubDepRepo struct {
|
|
|
|
|
domain.DependencySQLRepository
|
|
|
|
|
hasCycle bool
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (s stubDepRepo) ValidateBlockingHierarchy(context.Context, *types.Dependency) error {
|
|
|
|
|
return nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (s stubDepRepo) HasCycle(context.Context, string, string) (bool, error) {
|
|
|
|
|
return s.hasCycle, nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// TestReExportedSentinelCatchesRealProductionError drives ACTUAL converted
|
|
|
|
|
// production returns — the domain dependency use case's self-dep and cycle
|
|
|
|
|
// branches — and asserts the PUBLIC aliases match them via errors.Is. This is
|
|
|
|
|
// the property the re-export exists to provide, verified end to end through
|
|
|
|
|
// real code rather than by wrapping a sentinel with itself.
|
|
|
|
|
func TestReExportedSentinelCatchesRealProductionError(t *testing.T) {
|
|
|
|
|
t.Parallel()
|
|
|
|
|
|
|
|
|
|
uc := domain.NewDependencyUseCase(stubDepRepo{})
|
|
|
|
|
selfErr := uc.AddDependency(context.Background(),
|
|
|
|
|
&types.Dependency{IssueID: "a", DependsOnID: "a", Type: types.DepBlocks}, "tester")
|
|
|
|
|
if !errors.Is(selfErr, beads.ErrSelfDependency) {
|
|
|
|
|
t.Errorf("errors.Is(real self-dep err, beads.ErrSelfDependency) = false; err = %v", selfErr)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
uc = domain.NewDependencyUseCase(stubDepRepo{hasCycle: true})
|
|
|
|
|
cycleErr := uc.AddDependency(context.Background(),
|
|
|
|
|
&types.Dependency{IssueID: "a", DependsOnID: "b", Type: types.DepBlocks}, "tester")
|
|
|
|
|
if !errors.Is(cycleErr, beads.ErrDependencyCycle) {
|
|
|
|
|
t.Errorf("errors.Is(real cycle err, beads.ErrDependencyCycle) = false; err = %v", cycleErr)
|
|
|
|
|
}
|
|
|
|
|
}
|
feat(validate): typed length bounds for assignee, owner, and label (#4894)
The assignee, owner, and label columns are VARCHAR(255), but the engine never
validated their length: an over-255 assignee/owner made the row INSERT/UPDATE
fail with a raw backend "data too long" error, and an over-255 label went
through INSERT IGNORE and was SILENTLY TRUNCATED — storing a label the caller
never sent. Validate these three fields up front and return a typed
ErrFieldTooLong so `bd` (and the library's own call sites) get a clean, typed
rejection instead of a raw SQL error or silent corruption.
- types.MaxFieldLen (255), types.ErrFieldTooLong (re-exported as
beads.ErrFieldTooLong), and types.CheckFieldLen using utf8.RuneCountInString
— rune count, not bytes, so a multibyte value up to 255 characters fits the
VARCHAR(255) column and passes while a 256-rune value fails.
- Guard every raw assignee/owner/label write on BOTH engine write stacks:
the embedded issueops/DoltStore path (ValidateWithCustom + ValidateForImport
for assignee/owner, UpdateIssueInTx pre-pass, AddLabelInTx and PersistLabels
for labels, ClaimIssueInTx for the actor written as assignee) AND the
proxied-server domain/db (uow) path (issue insertIssueRow + Update + Claim,
label Insert) — which a first pass missed, so the silent-truncation bug
persisted there.
- Tests on both stacks: over-length create/update/label/claim reject with
ErrFieldTooLong and persist nothing (single and bulk); a 255-rune multibyte
value round-trips unchanged and a 256-rune one is rejected (rune-count proof
end to end); a 255-char value stores intact.
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-19 23:03:07 -07:00
|
|
|
|
feat(engine): guarded write ops — atomic metadata merge, row-version CAS, typed dep errors + events, bd close delegation (#4911)
* feat(metadata): MergeMetadata — atomic single-key metadata merge
SlotSet did a read-modify-write across TWO transactions: GetIssue (tx 1),
metadata[key]=value in memory, then UpdateIssue (tx 2). Two concurrent SlotSet
calls — even on DIFFERENT keys — both read the same base metadata and each
wrote back its whole blob, so the second clobbered the first's key. SlotClear
had the same bug. Add MergeMetadata, which does the read-modify-write inside
ONE transaction (so a concurrent conflict is retried and re-reads), stores a
JSON value (nested objects/arrays, not just strings), and reimplement SlotSet
and SlotClear on top of it so they inherit the atomicity.
- issueops.MergeMetadataInTx / DeleteMetadataInTx: read the metadata (routed
issues/wisps; missing issue -> ErrNotFound), merge/delete the one key, and
write the whole object back THROUGH UpdateIssueInTx — so the operation keeps
everything the old SlotSet got from UpdateIssue (the EventUpdated history
event with actor attribution, the configured metadata-schema validation, and
updated_at), now atomic. The read and write share the caller's transaction,
so a concurrent merge of a different key is retried and re-read, never
clobbered.
- Storage.MergeMetadata implemented on DoltStore (perm withRetryTx +
DOLT_ADD(issues,events) + DOLT_COMMIT; wisp path no commit) and
EmbeddedDoltStore (withConn); forwarded on InstrumentedStorage; auto-promoted
on HookFiringStore; no-op on the configStore mock. SlotSet/SlotClear rebuilt
on it (a string value marshals to a JSON string, byte-compatible with the
historical rewrite).
- Cross-backend conformance covers MergeMetadata + atomic SlotClear; the
8-goroutine concurrent-no-clobber test proves the B1 fix on real Dolt, and
a test asserts the EventUpdated event and schema validation are preserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(concurrency): expose a per-row version on Issue reads (RowVersion)
The engine's internal row_lock column — a random non-zero int64 the engine
rewrites on every status/ownership-mutating write and on the generic update
path — was not surfaced on the public types.Issue. Consumers that wanted a
fine-grained optimistic-concurrency token had only updated_at, which is stored
at second granularity, so two same-second writes were indistinguishable. Expose
row_lock read-only as types.Issue.RowVersion so a caller can tell same-second
writes apart.
- types.Issue gains RowVersion int64 (json:"-": a Go-only concurrency token,
never on the CLI/export JSON wire — it is opaque and equality-only). Added
row_lock to the canonical sqlbuild.IssueBaseColumns and a matching scan target
in issueops.ScanIssueFrom; both engine write stacks, wisps, and the counts /
dependents parallel scans share those, so the field hydrates everywhere from a
single-point change. Read-only — no CAS/write behavior (that is a follow-up).
- Doc is precise about coverage: RowVersion changes on claim/close/unclaim and
the generic update path; it does NOT change on some direct-UPDATE paths
(restore, compaction text), which bump updated_at — so a complete change key
combines RowVersion with updated_at, status, and labels. A created row is
already non-zero (create stamps row_lock); 0 only appears on legacy rows
backfilled by migration 0054's DEFAULT 0.
- Tests: RowVersion hydrates on read and on the list path, changes on a mutating
write, distinguishes two same-second writes (identical updated_at, distinct
RowVersion), and never appears on any JSON surface.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(deps): return a typed error on dependency type-conflict
Adding a dependency between a pair that already has an edge of a different type
is a deterministic rejection, but the two engine write paths disagreed on how
they surfaced it: one returned a typed *DependencyTypeConflictError, the other a
plain fmt.Errorf string. So a caller could errors.As the conflict on one path
but had to string-match the message on the other. Make both paths return the
same typed error, and re-export the two dependency-conflict types on the public
beads package so `bd` (and the library's own call sites) classify by errors.As
instead of parsing message text.
- issueops.AddDependencyInTx now returns *domain.DependencyTypeConflictError for
the type-conflict case (byte-identical message — the struct's Error() is the
exact former string, so no string-matcher changes). The hierarchy/cross-type
case already returned *domain.DependencyHierarchyConflictError on both paths;
self-dependency and cycle are already typed sentinels.
- beads.go re-exports DependencyTypeConflictError and
DependencyHierarchyConflictError as type aliases, so errors.As against the
public type matches the value the engine returns.
- Tests assert errors.As + the four fields + the byte-identical message on both
write paths (permanent issues->issues and the wisp-source BeginTx/Commit seam),
and lock the re-exports; the deterministic conflict is returned immediately
(never retried as a transient serialization error).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(deps): emit dependency_added / dependency_removed events
`bd dep add` and `bd dep remove` recorded no event, so the events feed never
surfaced dependency changes even though the event types (types.EventDependencyAdded
/ EventDependencyRemoved) have existed since the original SQLite backend, which
emitted them on every add/remove. The emission was dropped when that backend was
removed for the Dolt-only migration and never ported to the current write paths.
Restore it on both Dolt write plumbings, matching the original shape (an events
row on the source issue: "Added dependency: <src> <type> <target>" /
"Removed dependency on <target>").
- issueops.AddDependencyInTx / RemoveDependencyInTx record the event via
RecordEventInTable on the source's event table (wisp-routed), on the genuine
add/remove only — the idempotent same-type re-add and the no-op remove of a
missing edge record nothing.
- The event is committed with the edge: DoltStore.AddDependency/RemoveDependency
DOLT_ADD `events`, and the transaction-path (RunInTransaction / batch /
graph-apply) methods mark the source's event table dirty (events or wisp_events)
so StageAndCommit commits it — without this the event row would dangle in the
working set.
- Emission is gated to the explicit dep-add/remove verb on both plumbings: the
proxied-server repo only records when the use-case sets DepInsertOpts.EmitEvent,
which create-with-deps (implicit parent-child / --deps / waits-for, via a direct
repo Insert) does not — so `bd create --parent` produces the same history on
either backend (the embedded create path via PersistDependencies likewise emits
nothing).
- Tests on both plumbings: add/remove emit exactly once and commit (verified via
`events AS OF 'HEAD'`), idempotent re-add and no-op remove emit nothing, wisp
sources route to wisp_events, and create-with-deps emits no dependency_added.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(close): optional ExpectedVersion CAS on CloseIssueChecked
CloseIssueChecked guards a close on is_blocked in-transaction, but a caller that
read an issue and then closes it has no way to say "only if it hasn't changed
since I read it" — a lost-update window. Add an optional compare-and-swap on the
row's version (the RowVersion / row_lock token): when the caller supplies an
ExpectedVersion, the close proceeds only if the row still has that version, else
it refuses with a typed ErrVersionMismatch. The version read and the close share
one transaction, so it is a true CAS with no read-then-write gap.
- storage.ErrVersionMismatch (re-exported as beads.ErrVersionMismatch) and
CloseIssueOptions.ExpectedVersion *int64 (nil disables the check; a pointer so
nil "no check" is distinct from "require version 0").
- issueops.CheckVersionInTx reads row_lock (wisp-routed; ErrNotFound on a missing
row) and returns ErrVersionMismatch on divergence. CloseIssueCheckedInTx runs
it FIRST — before the is_blocked guard and before the Force short-circuit, so
Force bypasses only the guard, never the CAS (the version check is an
orthogonal precondition). A mismatch returns before any write, so the tx rolls
back leaving the issue open with no `closed` event (atomic refuse).
- The CAS has two limbs: the read-side check catches a writer that committed
before the close began, and on the retry-wrapped permanent path a commit-time
row_lock conflict is replayed by withRetryTx so the re-read refuses.
- Tests on both stores: match closes, stale refuses atomically (still open, zero
closed events), a committed concurrent write invalidates a captured version,
nil is unchanged behavior, Force does not bypass, missing id → ErrNotFound,
wisp sources route to the wisps table, and an already-closed re-close with the
post-close version stays idempotent (Unchanged).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(close): document RowVersion CAS coverage boundary on ExpectedVersion
Both adoption reviewers flagged that the public ExpectedVersion godoc and the
shared CloseIssueCheckedInTx doc could be misread as a full-row "unchanged"
check. RowVersion (row_lock) only tracks lifecycle/ownership writes
(status/assignee/started_at), so concurrent label, dependency, rename, or
is_blocked writes leave it untouched and are intentionally outside this CAS
boundary. Document that on both sites, grounded in the freshRowLock invariant.
Doc-only; no behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(cli): bd close delegates to the engine's guarded close
The embedded `bd close` path ran its own is_blocked pre-check and then closed in
a separate call — a read-then-write TOCTOU that duplicated a guard the engine
provides atomically. The verb now builds CloseIssueOptions and delegates to
CloseIssueChecked, which runs the guard and the close in ONE transaction; the
CLI's duplicated guard is deleted. --force maps to Options.Force; a blocked
close refuses with the blockers named and the --force hint.
Behavior-preserving on purpose (verified against the previous binary):
- The engine guard now uses the exact historical CLI predicate — refuse only on
a LIVE direct blocker (blocked && len(blockers) > 0), not on the bare
denormalized is_blocked column. A transitively-blocked child (is_blocked
inherited from a blocked parent, zero direct blockers) still closes without
--force, and a stale is_blocked with since-closed blockers self-heals, exactly
as before.
- An already-closed issue stays an idempotent success: it still appears in the
--json array and the text report (same output shape), still exits 0, but no
longer produces a spurious closed->closed audit entry, a no-op commit, or the
real-close side effects (molecule auto-close, newly-unblocked, claim-next).
Tests cover: direct blocker refuses atomically (still open) and closes with
--force; transitively-blocked and stale-is_blocked beads close without --force;
already-closed emits in --json alone and in a mixed batch; idempotent exit 0 —
on the engine (both stores) and through the bd binary.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(update): UpdateIssueChecked — optional ExpectedVersion CAS on the update
UpdateIssue applies a field-map update with no optimistic-concurrency option, so
a caller that wants "update only if unchanged since I read it" had to
read-then-write — a lost-update window. Add UpdateIssueChecked: when the caller
supplies an ExpectedVersion (the RowVersion / row_lock token), the version read
and the update share ONE transaction, refusing with the typed ErrVersionMismatch
if the row has moved — a true compare-and-swap. Nil disables the check and is
byte-identical to UpdateIssue, which is left untouched on the hot path.
- storage.UpdateIssueOptions{ExpectedVersion *int64} (aliased as
beads.UpdateIssueOptions) + Storage.UpdateIssueChecked, mirroring the
CloseIssueChecked pattern: CheckVersionInTx runs first inside the same
transaction on every route — the permanent withRetryTx path, the wisp path,
AND the demote route (an update carrying no_history/wisp routes through the
demote flow; a pure demoteToWispInTx extraction lets the check compose
atomically with the row move — verified byte-identical to the old
DemoteToWisp).
- Implemented on DoltStore and EmbeddedDoltStore; InstrumentedStorage wraps it;
HookFiringStore fires on_update only on success (never on a refused update).
- Tests on both stores: match updates and bumps RowVersion; stale refuses
atomically (field unchanged, zero updated events); a committed concurrent
write invalidates a captured version; nil behaves exactly like UpdateIssue;
wisp match/stale; demote-route match (migrates + applies) and stale (still in
issues, unchanged); missing id → ErrNotFound; the hook decorator fires once on
success and never on refusal.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor(cli): proxied bd close delegates to a library checked close
The proxied-server `bd close` path duplicated the close guard in the CLI:
it queried blockers via the dependency use-case, refused with its own message,
and then closed in a separate use-case call. Move the guard into the library —
IssueUseCase gains CloseIssueChecked / CloseWispChecked, which run the same
live-direct-blocker predicate (refuse only on blocked && len(blockers) > 0)
and then the untouched unchecked close — and thin the CLI verb to a single
delegated call. Both `bd close` paths now enforce the guard in the library and
emit the byte-identical refusal: the same storage.ErrCloseBlocked sentinel
("cannot close blocked issue: <id> is blocked by [...]") plus the
"(use --force to override)" hint.
- Guard and close run on the one pinned unit-of-work connection (the whole
close batch is a single transaction), and the guard is read-only and refuses
before any write — a refused id leaves the shared batch transaction clean,
and an all-refused batch skips the commit entirely, exactly as before.
- The unchecked CloseIssue / CloseWisp are untouched: internal closes (molecule
auto-close, gate closes, bd todo done) intentionally stay unguarded.
- Behavior-parity preserved: guard ordering (validate → epic-child → gate →
blocked), exit codes, JSON/batch shape, already-closed handling, audit fields,
and commit-message contents are unchanged; a transitively-blocked bead
(is_blocked with no direct open blocker) still closes without --force, and
--force still bypasses. The guard-failure diagnostic now uses the embedded
path's generic wording (further convergence; nothing matched the old text).
- Tests: use-case level on real Dolt (direct blocker refuses with blockers
named + still open, transitive-only closes, force closes, already-closed
parity, wisp refuse/force) plus the proxied CLI integration suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): cross-backend dependency-event + RowVersion parity
Maintainer review fixups for PR #4911 (adopt-pr review loop):
- RowVersion: the proxied/domain create (insertIssueRow) and generic Update
paths never stamped or rewrote row_lock, so an issue written through the
proxied-server backend read back RowVersion 0 on create and unchanged on
update -- a stale CAS token that violated the types.Issue.RowVersion
contract on that backend. Stamp a fresh non-zero row_lock on insert (and the
duplicate-key rewrite) and append RowLockClause() to the generic update,
mirroring the classic issueops insert/update invariant. Adds domain/db
regression tests proving CreateIssue/CreateWisp are non-zero and
UpdateIssue/UpdateWisp change it.
- Dependency events: embedded issueops AddDependencyInTx/RemoveDependencyInTx
recorded history unconditionally, so structural create-with-deps
(bd create --parent/--deps/--waits-for) and reparent (bd update --parent)
emitted dependency_added/removed on embedded but were silent on the proxied
backend, diverging issue history for identical commands. Gate the embedded
emit on EmitEvent and thread it through AddDependencyWithOptions /
RemoveDependencyWithOptions on both the store and transaction interfaces:
the plain AddDependency/RemoveDependency are the no-event structural default
(create-with-deps, reparent), while the explicit dep verbs (bd dep add/remove,
bd link, bd relate/unrelate) pass EmitEvent -- matching the proxied
DepInsertOpts.EmitEvent gate. DoltStore stages the events table only when an
event was actually written (GH#2455). Corrects the false
"parity with PersistDependencies" comments and adds symmetric embedded/proxied
dep-event coverage for create-with-parent/deps and structural removal.
- Retain iteration-1 fixups: heal molecules on already-closed re-close,
idempotent proxied checked-close, and documented public Storage growth.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(storage): drop always-constant event param from fireDependencyHookByID (unparam)
golangci-lint (latest, v2.12.2 in CI) unparam flags fireDependencyHookByID:
its `event` parameter always receives hooks.EventUpdate. The parameter was
already effectively constant before this PR (both base call sites passed
EventUpdate); this PR adds two more call sites, all EventUpdate. Remove the
dead parameter and hardcode hooks.EventUpdate at the single runner.Run call.
All four call sites pass hooks.EventUpdate, so behavior is unchanged; go
build, go vet, and a scoped golangci-lint run pass clean.
Maintainer fixup during adoption finalize (PR #4911).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Eddie the Engineer <ci@beads.test>
2026-07-21 18:13:03 -07:00
|
|
|
// insertErrDepRepo returns preset errors from the two methods the domain
|
|
|
|
|
// dependency use-case consults before its typed-conflict passthrough branches:
|
|
|
|
|
// ValidateBlockingHierarchy (hierarchy conflict) and Insert (type conflict). Any
|
|
|
|
|
// other call nil-panics through the embedded interface, keeping the stub honest
|
|
|
|
|
// about the surface these branches touch.
|
|
|
|
|
type insertErrDepRepo struct {
|
|
|
|
|
domain.DependencySQLRepository
|
|
|
|
|
hierarchyErr error
|
|
|
|
|
insertErr error
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (r insertErrDepRepo) ValidateBlockingHierarchy(context.Context, *types.Dependency) error {
|
|
|
|
|
return r.hierarchyErr
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (r insertErrDepRepo) HasCycle(context.Context, string, string) (bool, error) {
|
|
|
|
|
return false, nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (r insertErrDepRepo) Insert(context.Context, *types.Dependency, string, domain.DepInsertOpts) error {
|
|
|
|
|
return r.insertErr
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// TestReExportedDependencyConflictTypes proves the public
|
|
|
|
|
// beads.DependencyTypeConflictError and beads.DependencyHierarchyConflictError
|
|
|
|
|
// aliases are the SAME struct types the engine returns: driving ACTUAL domain
|
|
|
|
|
// use-case passthrough returns (the conflict is passed through unwrapped, the
|
|
|
|
|
// property both write stacks now share), errors.As classifies each through the
|
|
|
|
|
// public alias and reads its fields — no message parsing.
|
|
|
|
|
func TestReExportedDependencyConflictTypes(t *testing.T) {
|
|
|
|
|
t.Parallel()
|
|
|
|
|
|
|
|
|
|
// Type conflict: a different-type edge already exists between the pair.
|
|
|
|
|
typeConflict := &domain.DependencyTypeConflictError{
|
|
|
|
|
IssueID: "a", DependsOnID: "b", ExistingType: "blocks", RequestedType: "related",
|
|
|
|
|
}
|
|
|
|
|
uc := domain.NewDependencyUseCase(insertErrDepRepo{insertErr: typeConflict})
|
|
|
|
|
err := uc.AddDependency(context.Background(),
|
|
|
|
|
&types.Dependency{IssueID: "a", DependsOnID: "b", Type: types.DepRelated}, "tester")
|
|
|
|
|
var gotType *beads.DependencyTypeConflictError
|
|
|
|
|
if !errors.As(err, &gotType) {
|
|
|
|
|
t.Fatalf("errors.As(real type-conflict err, *beads.DependencyTypeConflictError) = false; err = %v", err)
|
|
|
|
|
}
|
|
|
|
|
if gotType.IssueID != "a" || gotType.DependsOnID != "b" ||
|
|
|
|
|
gotType.ExistingType != "blocks" || gotType.RequestedType != "related" {
|
|
|
|
|
t.Errorf("extracted type-conflict fields = %+v, want {a b blocks related}", gotType)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// Hierarchy conflict: a blocking edge would gate an issue on its ancestor.
|
|
|
|
|
hierConflict := &domain.DependencyHierarchyConflictError{
|
|
|
|
|
IssueID: "child", BlockerID: "ancestor", BlockerIsAncestor: true,
|
|
|
|
|
}
|
|
|
|
|
uc = domain.NewDependencyUseCase(insertErrDepRepo{hierarchyErr: hierConflict})
|
|
|
|
|
err = uc.AddDependency(context.Background(),
|
|
|
|
|
&types.Dependency{IssueID: "child", DependsOnID: "ancestor", Type: types.DepBlocks}, "tester")
|
|
|
|
|
var gotHier *beads.DependencyHierarchyConflictError
|
|
|
|
|
if !errors.As(err, &gotHier) {
|
|
|
|
|
t.Fatalf("errors.As(real hierarchy-conflict err, *beads.DependencyHierarchyConflictError) = false; err = %v", err)
|
|
|
|
|
}
|
|
|
|
|
if gotHier.IssueID != "child" || gotHier.BlockerID != "ancestor" || !gotHier.BlockerIsAncestor {
|
|
|
|
|
t.Errorf("extracted hierarchy-conflict fields = %+v, want {child ancestor true}", gotHier)
|
|
|
|
|
}
|
feat(issueops): name the two dependency-endpoint refusals (bd-yby99.9)
DependencyEditor.AddDependencies refuses an edge whose SOURCE names no row,
and one whose TARGET names no row this database can see the absence of. Every
other refusal the role can raise hands a caller something to branch on —
ErrSelfDependency, ErrDependencyCycle, *DependencyTypeConflictError,
*DependencyHierarchyConflictError — and these two handed back "an error", with
different text on each backend. A programmatic consumer could not tell a ghost
endpoint from an infrastructure failure, and the role's own doc said so rather
than promising the anonymity would last.
Both now carry an identity: ErrDependencySourceNotFound and
ErrDependencyTargetNotFound, wrapped by a *DependencyEndpointNotFoundError that
names the refused edge and which of its endpoints was absent. Two sentinels
rather than one because the two are separate answers — a ghost source is always
a bad id, while a target is refused only where this database would have held
it. What is NOT refused is unchanged: an "external:" reference and another
repository's id are still stored as external targets.
The store-backed body and the cross-tier target precheck mint the refusal where
they already read the row. The domain repository has no such read: it learns of
the absence from a foreign-key violation, so it reads both endpoints back on
the transaction that refused, only on the refusal path, and never downgrades
the refusal to a probe's failure. Taking the identity out of the driver's
constraint name would have been the thing a typed refusal exists to avoid. One
user-visible consequence: the proxied-server path now renders "issue <id> not
found" where it used to render a raw foreign-key violation, which is what the
embedded path has always said.
The guarded create classifies the new type where it classified the raw
foreign-key signal, so a missing dependency, parent or waits-for target is
still ErrValidation wrapping ErrNotFound on every backend.
RemoveDependency is deliberately untouched. A removal that finds no edge is
Removed false with a nil error, so there is no refusal there to name.
The shared contract asserts the sentinel and the typed fields for each endpoint
in TWO POSITIONS: alone in a request, and mid-batch where the refusal competes
with the rollback of an edge already written, the half that also reads the
graph back at zero edges. Both cases were verified red on all three backends
before the production change — "issue <id> not found" untyped on the two store
paths, "Error 1452 ... Foreign key violation" on the unit-of-work path.
Agent-Signature: claude-code-claude-opus-5-unknown-reasoning on behalf of CI Bot
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-07 19:45:54 +00:00
|
|
|
|
|
|
|
|
// Endpoint miss: the target names no row this database holds. The domain
|
|
|
|
|
// use case passes it through unwrapped for the same reason the two
|
|
|
|
|
// conflicts above are passed through.
|
|
|
|
|
missing := &domain.DependencyEndpointNotFoundError{
|
|
|
|
|
IssueID: "a", DependsOnID: "ghost", MissingID: "ghost",
|
|
|
|
|
Err: domain.ErrDependencyTargetNotFound,
|
|
|
|
|
}
|
|
|
|
|
uc = domain.NewDependencyUseCase(insertErrDepRepo{insertErr: missing})
|
|
|
|
|
err = uc.AddDependency(context.Background(),
|
|
|
|
|
&types.Dependency{IssueID: "a", DependsOnID: "ghost", Type: types.DepBlocks}, "tester")
|
|
|
|
|
var gotMissing *beads.DependencyEndpointNotFoundError
|
|
|
|
|
if !errors.As(err, &gotMissing) {
|
|
|
|
|
t.Fatalf("errors.As(real endpoint-miss err, *beads.DependencyEndpointNotFoundError) = false; err = %v", err)
|
|
|
|
|
}
|
|
|
|
|
if !errors.Is(err, beads.ErrDependencyTargetNotFound) {
|
|
|
|
|
t.Errorf("errors.Is(real endpoint-miss err, beads.ErrDependencyTargetNotFound) = false; err = %v", err)
|
|
|
|
|
}
|
|
|
|
|
if gotMissing.IssueID != "a" || gotMissing.DependsOnID != "ghost" || gotMissing.MissingID != "ghost" {
|
|
|
|
|
t.Errorf("extracted endpoint-miss fields = %+v, want {a ghost ghost}", gotMissing)
|
|
|
|
|
}
|
feat(engine): guarded write ops — atomic metadata merge, row-version CAS, typed dep errors + events, bd close delegation (#4911)
* feat(metadata): MergeMetadata — atomic single-key metadata merge
SlotSet did a read-modify-write across TWO transactions: GetIssue (tx 1),
metadata[key]=value in memory, then UpdateIssue (tx 2). Two concurrent SlotSet
calls — even on DIFFERENT keys — both read the same base metadata and each
wrote back its whole blob, so the second clobbered the first's key. SlotClear
had the same bug. Add MergeMetadata, which does the read-modify-write inside
ONE transaction (so a concurrent conflict is retried and re-reads), stores a
JSON value (nested objects/arrays, not just strings), and reimplement SlotSet
and SlotClear on top of it so they inherit the atomicity.
- issueops.MergeMetadataInTx / DeleteMetadataInTx: read the metadata (routed
issues/wisps; missing issue -> ErrNotFound), merge/delete the one key, and
write the whole object back THROUGH UpdateIssueInTx — so the operation keeps
everything the old SlotSet got from UpdateIssue (the EventUpdated history
event with actor attribution, the configured metadata-schema validation, and
updated_at), now atomic. The read and write share the caller's transaction,
so a concurrent merge of a different key is retried and re-read, never
clobbered.
- Storage.MergeMetadata implemented on DoltStore (perm withRetryTx +
DOLT_ADD(issues,events) + DOLT_COMMIT; wisp path no commit) and
EmbeddedDoltStore (withConn); forwarded on InstrumentedStorage; auto-promoted
on HookFiringStore; no-op on the configStore mock. SlotSet/SlotClear rebuilt
on it (a string value marshals to a JSON string, byte-compatible with the
historical rewrite).
- Cross-backend conformance covers MergeMetadata + atomic SlotClear; the
8-goroutine concurrent-no-clobber test proves the B1 fix on real Dolt, and
a test asserts the EventUpdated event and schema validation are preserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(concurrency): expose a per-row version on Issue reads (RowVersion)
The engine's internal row_lock column — a random non-zero int64 the engine
rewrites on every status/ownership-mutating write and on the generic update
path — was not surfaced on the public types.Issue. Consumers that wanted a
fine-grained optimistic-concurrency token had only updated_at, which is stored
at second granularity, so two same-second writes were indistinguishable. Expose
row_lock read-only as types.Issue.RowVersion so a caller can tell same-second
writes apart.
- types.Issue gains RowVersion int64 (json:"-": a Go-only concurrency token,
never on the CLI/export JSON wire — it is opaque and equality-only). Added
row_lock to the canonical sqlbuild.IssueBaseColumns and a matching scan target
in issueops.ScanIssueFrom; both engine write stacks, wisps, and the counts /
dependents parallel scans share those, so the field hydrates everywhere from a
single-point change. Read-only — no CAS/write behavior (that is a follow-up).
- Doc is precise about coverage: RowVersion changes on claim/close/unclaim and
the generic update path; it does NOT change on some direct-UPDATE paths
(restore, compaction text), which bump updated_at — so a complete change key
combines RowVersion with updated_at, status, and labels. A created row is
already non-zero (create stamps row_lock); 0 only appears on legacy rows
backfilled by migration 0054's DEFAULT 0.
- Tests: RowVersion hydrates on read and on the list path, changes on a mutating
write, distinguishes two same-second writes (identical updated_at, distinct
RowVersion), and never appears on any JSON surface.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(deps): return a typed error on dependency type-conflict
Adding a dependency between a pair that already has an edge of a different type
is a deterministic rejection, but the two engine write paths disagreed on how
they surfaced it: one returned a typed *DependencyTypeConflictError, the other a
plain fmt.Errorf string. So a caller could errors.As the conflict on one path
but had to string-match the message on the other. Make both paths return the
same typed error, and re-export the two dependency-conflict types on the public
beads package so `bd` (and the library's own call sites) classify by errors.As
instead of parsing message text.
- issueops.AddDependencyInTx now returns *domain.DependencyTypeConflictError for
the type-conflict case (byte-identical message — the struct's Error() is the
exact former string, so no string-matcher changes). The hierarchy/cross-type
case already returned *domain.DependencyHierarchyConflictError on both paths;
self-dependency and cycle are already typed sentinels.
- beads.go re-exports DependencyTypeConflictError and
DependencyHierarchyConflictError as type aliases, so errors.As against the
public type matches the value the engine returns.
- Tests assert errors.As + the four fields + the byte-identical message on both
write paths (permanent issues->issues and the wisp-source BeginTx/Commit seam),
and lock the re-exports; the deterministic conflict is returned immediately
(never retried as a transient serialization error).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(deps): emit dependency_added / dependency_removed events
`bd dep add` and `bd dep remove` recorded no event, so the events feed never
surfaced dependency changes even though the event types (types.EventDependencyAdded
/ EventDependencyRemoved) have existed since the original SQLite backend, which
emitted them on every add/remove. The emission was dropped when that backend was
removed for the Dolt-only migration and never ported to the current write paths.
Restore it on both Dolt write plumbings, matching the original shape (an events
row on the source issue: "Added dependency: <src> <type> <target>" /
"Removed dependency on <target>").
- issueops.AddDependencyInTx / RemoveDependencyInTx record the event via
RecordEventInTable on the source's event table (wisp-routed), on the genuine
add/remove only — the idempotent same-type re-add and the no-op remove of a
missing edge record nothing.
- The event is committed with the edge: DoltStore.AddDependency/RemoveDependency
DOLT_ADD `events`, and the transaction-path (RunInTransaction / batch /
graph-apply) methods mark the source's event table dirty (events or wisp_events)
so StageAndCommit commits it — without this the event row would dangle in the
working set.
- Emission is gated to the explicit dep-add/remove verb on both plumbings: the
proxied-server repo only records when the use-case sets DepInsertOpts.EmitEvent,
which create-with-deps (implicit parent-child / --deps / waits-for, via a direct
repo Insert) does not — so `bd create --parent` produces the same history on
either backend (the embedded create path via PersistDependencies likewise emits
nothing).
- Tests on both plumbings: add/remove emit exactly once and commit (verified via
`events AS OF 'HEAD'`), idempotent re-add and no-op remove emit nothing, wisp
sources route to wisp_events, and create-with-deps emits no dependency_added.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(close): optional ExpectedVersion CAS on CloseIssueChecked
CloseIssueChecked guards a close on is_blocked in-transaction, but a caller that
read an issue and then closes it has no way to say "only if it hasn't changed
since I read it" — a lost-update window. Add an optional compare-and-swap on the
row's version (the RowVersion / row_lock token): when the caller supplies an
ExpectedVersion, the close proceeds only if the row still has that version, else
it refuses with a typed ErrVersionMismatch. The version read and the close share
one transaction, so it is a true CAS with no read-then-write gap.
- storage.ErrVersionMismatch (re-exported as beads.ErrVersionMismatch) and
CloseIssueOptions.ExpectedVersion *int64 (nil disables the check; a pointer so
nil "no check" is distinct from "require version 0").
- issueops.CheckVersionInTx reads row_lock (wisp-routed; ErrNotFound on a missing
row) and returns ErrVersionMismatch on divergence. CloseIssueCheckedInTx runs
it FIRST — before the is_blocked guard and before the Force short-circuit, so
Force bypasses only the guard, never the CAS (the version check is an
orthogonal precondition). A mismatch returns before any write, so the tx rolls
back leaving the issue open with no `closed` event (atomic refuse).
- The CAS has two limbs: the read-side check catches a writer that committed
before the close began, and on the retry-wrapped permanent path a commit-time
row_lock conflict is replayed by withRetryTx so the re-read refuses.
- Tests on both stores: match closes, stale refuses atomically (still open, zero
closed events), a committed concurrent write invalidates a captured version,
nil is unchanged behavior, Force does not bypass, missing id → ErrNotFound,
wisp sources route to the wisps table, and an already-closed re-close with the
post-close version stays idempotent (Unchanged).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(close): document RowVersion CAS coverage boundary on ExpectedVersion
Both adoption reviewers flagged that the public ExpectedVersion godoc and the
shared CloseIssueCheckedInTx doc could be misread as a full-row "unchanged"
check. RowVersion (row_lock) only tracks lifecycle/ownership writes
(status/assignee/started_at), so concurrent label, dependency, rename, or
is_blocked writes leave it untouched and are intentionally outside this CAS
boundary. Document that on both sites, grounded in the freshRowLock invariant.
Doc-only; no behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(cli): bd close delegates to the engine's guarded close
The embedded `bd close` path ran its own is_blocked pre-check and then closed in
a separate call — a read-then-write TOCTOU that duplicated a guard the engine
provides atomically. The verb now builds CloseIssueOptions and delegates to
CloseIssueChecked, which runs the guard and the close in ONE transaction; the
CLI's duplicated guard is deleted. --force maps to Options.Force; a blocked
close refuses with the blockers named and the --force hint.
Behavior-preserving on purpose (verified against the previous binary):
- The engine guard now uses the exact historical CLI predicate — refuse only on
a LIVE direct blocker (blocked && len(blockers) > 0), not on the bare
denormalized is_blocked column. A transitively-blocked child (is_blocked
inherited from a blocked parent, zero direct blockers) still closes without
--force, and a stale is_blocked with since-closed blockers self-heals, exactly
as before.
- An already-closed issue stays an idempotent success: it still appears in the
--json array and the text report (same output shape), still exits 0, but no
longer produces a spurious closed->closed audit entry, a no-op commit, or the
real-close side effects (molecule auto-close, newly-unblocked, claim-next).
Tests cover: direct blocker refuses atomically (still open) and closes with
--force; transitively-blocked and stale-is_blocked beads close without --force;
already-closed emits in --json alone and in a mixed batch; idempotent exit 0 —
on the engine (both stores) and through the bd binary.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(update): UpdateIssueChecked — optional ExpectedVersion CAS on the update
UpdateIssue applies a field-map update with no optimistic-concurrency option, so
a caller that wants "update only if unchanged since I read it" had to
read-then-write — a lost-update window. Add UpdateIssueChecked: when the caller
supplies an ExpectedVersion (the RowVersion / row_lock token), the version read
and the update share ONE transaction, refusing with the typed ErrVersionMismatch
if the row has moved — a true compare-and-swap. Nil disables the check and is
byte-identical to UpdateIssue, which is left untouched on the hot path.
- storage.UpdateIssueOptions{ExpectedVersion *int64} (aliased as
beads.UpdateIssueOptions) + Storage.UpdateIssueChecked, mirroring the
CloseIssueChecked pattern: CheckVersionInTx runs first inside the same
transaction on every route — the permanent withRetryTx path, the wisp path,
AND the demote route (an update carrying no_history/wisp routes through the
demote flow; a pure demoteToWispInTx extraction lets the check compose
atomically with the row move — verified byte-identical to the old
DemoteToWisp).
- Implemented on DoltStore and EmbeddedDoltStore; InstrumentedStorage wraps it;
HookFiringStore fires on_update only on success (never on a refused update).
- Tests on both stores: match updates and bumps RowVersion; stale refuses
atomically (field unchanged, zero updated events); a committed concurrent
write invalidates a captured version; nil behaves exactly like UpdateIssue;
wisp match/stale; demote-route match (migrates + applies) and stale (still in
issues, unchanged); missing id → ErrNotFound; the hook decorator fires once on
success and never on refusal.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor(cli): proxied bd close delegates to a library checked close
The proxied-server `bd close` path duplicated the close guard in the CLI:
it queried blockers via the dependency use-case, refused with its own message,
and then closed in a separate use-case call. Move the guard into the library —
IssueUseCase gains CloseIssueChecked / CloseWispChecked, which run the same
live-direct-blocker predicate (refuse only on blocked && len(blockers) > 0)
and then the untouched unchecked close — and thin the CLI verb to a single
delegated call. Both `bd close` paths now enforce the guard in the library and
emit the byte-identical refusal: the same storage.ErrCloseBlocked sentinel
("cannot close blocked issue: <id> is blocked by [...]") plus the
"(use --force to override)" hint.
- Guard and close run on the one pinned unit-of-work connection (the whole
close batch is a single transaction), and the guard is read-only and refuses
before any write — a refused id leaves the shared batch transaction clean,
and an all-refused batch skips the commit entirely, exactly as before.
- The unchecked CloseIssue / CloseWisp are untouched: internal closes (molecule
auto-close, gate closes, bd todo done) intentionally stay unguarded.
- Behavior-parity preserved: guard ordering (validate → epic-child → gate →
blocked), exit codes, JSON/batch shape, already-closed handling, audit fields,
and commit-message contents are unchanged; a transitively-blocked bead
(is_blocked with no direct open blocker) still closes without --force, and
--force still bypasses. The guard-failure diagnostic now uses the embedded
path's generic wording (further convergence; nothing matched the old text).
- Tests: use-case level on real Dolt (direct blocker refuses with blockers
named + still open, transitive-only closes, force closes, already-closed
parity, wisp refuse/force) plus the proxied CLI integration suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): cross-backend dependency-event + RowVersion parity
Maintainer review fixups for PR #4911 (adopt-pr review loop):
- RowVersion: the proxied/domain create (insertIssueRow) and generic Update
paths never stamped or rewrote row_lock, so an issue written through the
proxied-server backend read back RowVersion 0 on create and unchanged on
update -- a stale CAS token that violated the types.Issue.RowVersion
contract on that backend. Stamp a fresh non-zero row_lock on insert (and the
duplicate-key rewrite) and append RowLockClause() to the generic update,
mirroring the classic issueops insert/update invariant. Adds domain/db
regression tests proving CreateIssue/CreateWisp are non-zero and
UpdateIssue/UpdateWisp change it.
- Dependency events: embedded issueops AddDependencyInTx/RemoveDependencyInTx
recorded history unconditionally, so structural create-with-deps
(bd create --parent/--deps/--waits-for) and reparent (bd update --parent)
emitted dependency_added/removed on embedded but were silent on the proxied
backend, diverging issue history for identical commands. Gate the embedded
emit on EmitEvent and thread it through AddDependencyWithOptions /
RemoveDependencyWithOptions on both the store and transaction interfaces:
the plain AddDependency/RemoveDependency are the no-event structural default
(create-with-deps, reparent), while the explicit dep verbs (bd dep add/remove,
bd link, bd relate/unrelate) pass EmitEvent -- matching the proxied
DepInsertOpts.EmitEvent gate. DoltStore stages the events table only when an
event was actually written (GH#2455). Corrects the false
"parity with PersistDependencies" comments and adds symmetric embedded/proxied
dep-event coverage for create-with-parent/deps and structural removal.
- Retain iteration-1 fixups: heal molecules on already-closed re-close,
idempotent proxied checked-close, and documented public Storage growth.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(storage): drop always-constant event param from fireDependencyHookByID (unparam)
golangci-lint (latest, v2.12.2 in CI) unparam flags fireDependencyHookByID:
its `event` parameter always receives hooks.EventUpdate. The parameter was
already effectively constant before this PR (both base call sites passed
EventUpdate); this PR adds two more call sites, all EventUpdate. Remove the
dead parameter and hardcode hooks.EventUpdate at the single runner.Run call.
All four call sites pass hooks.EventUpdate, so behavior is unchanged; go
build, go vet, and a scoped golangci-lint run pass clean.
Maintainer fixup during adoption finalize (PR #4911).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Eddie the Engineer <ci@beads.test>
2026-07-21 18:13:03 -07:00
|
|
|
}
|
|
|
|
|
|
feat(validate): typed length bounds for assignee, owner, and label (#4894)
The assignee, owner, and label columns are VARCHAR(255), but the engine never
validated their length: an over-255 assignee/owner made the row INSERT/UPDATE
fail with a raw backend "data too long" error, and an over-255 label went
through INSERT IGNORE and was SILENTLY TRUNCATED — storing a label the caller
never sent. Validate these three fields up front and return a typed
ErrFieldTooLong so `bd` (and the library's own call sites) get a clean, typed
rejection instead of a raw SQL error or silent corruption.
- types.MaxFieldLen (255), types.ErrFieldTooLong (re-exported as
beads.ErrFieldTooLong), and types.CheckFieldLen using utf8.RuneCountInString
— rune count, not bytes, so a multibyte value up to 255 characters fits the
VARCHAR(255) column and passes while a 256-rune value fails.
- Guard every raw assignee/owner/label write on BOTH engine write stacks:
the embedded issueops/DoltStore path (ValidateWithCustom + ValidateForImport
for assignee/owner, UpdateIssueInTx pre-pass, AddLabelInTx and PersistLabels
for labels, ClaimIssueInTx for the actor written as assignee) AND the
proxied-server domain/db (uow) path (issue insertIssueRow + Update + Claim,
label Insert) — which a first pass missed, so the silent-truncation bug
persisted there.
- Tests on both stacks: over-length create/update/label/claim reject with
ErrFieldTooLong and persist nothing (single and bulk); a 255-rune multibyte
value round-trips unchanged and a 256-rune one is rejected (rune-count proof
end to end); a 255-char value stores intact.
Co-authored-by: Test User <test@test.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-19 23:03:07 -07:00
|
|
|
// TestReExportFieldTooLong proves the public beads.ErrFieldTooLong alias is the
|
|
|
|
|
// same value as the internal types sentinel and composes through errors.Is when
|
|
|
|
|
// wrapped — the property length-validation callers rely on to detect an
|
|
|
|
|
// over-length assignee/owner/label without importing internal/types. It also
|
|
|
|
|
// checks the MaxFieldLen constant re-export tracks the source of truth.
|
|
|
|
|
func TestReExportFieldTooLong(t *testing.T) {
|
|
|
|
|
t.Parallel()
|
|
|
|
|
|
|
|
|
|
if beads.ErrFieldTooLong != types.ErrFieldTooLong {
|
|
|
|
|
t.Error("beads.ErrFieldTooLong is not the internal sentinel value (identity broken)")
|
|
|
|
|
}
|
|
|
|
|
wrapped := fmt.Errorf("x: %w", beads.ErrFieldTooLong)
|
|
|
|
|
if !errors.Is(wrapped, beads.ErrFieldTooLong) {
|
|
|
|
|
t.Errorf("errors.Is(wrapped, beads.ErrFieldTooLong) = false; err = %v", wrapped)
|
|
|
|
|
}
|
|
|
|
|
if beads.MaxFieldLen != types.MaxFieldLen {
|
|
|
|
|
t.Errorf("beads.MaxFieldLen = %d, want types.MaxFieldLen = %d", beads.MaxFieldLen, types.MaxFieldLen)
|
|
|
|
|
}
|
|
|
|
|
}
|