Skip to content

fix(beads): fold the third bead-id validator onto internal/beadid, close the .. gap - #53

Open
luthermonson wants to merge 1 commit into
mainfrom
fix/beads-validid-dedup
Open

fix(beads): fold the third bead-id validator onto internal/beadid, close the .. gap#53
luthermonson wants to merge 1 commit into
mainfrom
fix/beads-validid-dedup

Conversation

@luthermonson

Copy link
Copy Markdown
Owner

What

internal/beads.validID was the third copy of the bead-id validation rule left standing after PR #45's M2 folded the ship (internal/server/beads.go) and fleet (internal/fleet/beads.go) sides onto the shared internal/beadid.Valid. This copy bounded ids at 128 (not 64) and — the real defect — had no .. (parent-directory hop) check, the exact gap beadid.Valid exists to close.

This PR folds validID onto beadid.Valid, leaving exactly one module that owns the alphabet and the .. rule.

Where validID's ids come from

Every id validID sees originates from bd's own JSON output, not remote input:

  • issueID parses the id out of bd create --json.
  • That id is persisted as BeadID in voyage state (internal/commands/sail_tracking.go) and later re-passed to the lifecycle methods (AddDependency, Start, Complete, Block, Show), each of which calls validID.

That is a genuinely different trust position from the remote input the fleet and server validate — which is why PR #45 deliberately left this copy rather than silently retighten a bound with no coverage. But a different trust position is not a reason to run a different, weaker rule: the id still becomes argv (bd show <id>) and, downstream, a path segment, so the .. hop must be refused here too.

The decision: unify, not parameterise

bd 1.1.2 mints only short hash-prefix ids (ship-8he, proj-a3f8.1.2; the beadid package doc calls 64 bytes "already generous" for exactly these). No documented bd id exceeds 64 chars, uses a character outside [A-Za-z0-9._-], or needs ... The fleet and server already validate these same bd-minted ids at 64 via beadid.Valid and must accept everything bd emits — so 64 is proven sufficient for bd output. The 128 bound was ungrounded divergence, not a real requirement.

So this is not a lossy unify: validID now delegates to beadid.Valid, the divergent copy is deleted, and the local name stays so the call sites read unchanged. internal/beadid is untouched, so no wider/parameterised variant was needed. The .. gap closes with no third silent copy remaining.

Tests

  • New TestIssueIDRejectsParentDirectoryHop in internal/beads/parse_test.go asserts the previously-unguarded parse path now rejects ids containing ... Verified it fails before the change (issueID("{\"id\":\"ship-..evil\"}") returned the id) and passes after.
  • Regression: existing valid bd ids still accepted; go build ./..., go vet ./..., and go test ./internal/beads/... all green. internal/server and internal/fleet compile unchanged (no beadid edit).

Ref: PR #45 M2.

…ose the .. gap

internal/beads.validID was the third copy of the bead-id rule left after
PR #45's M2 unified the ship and fleet sides onto internal/beadid.Valid. It
bounded at 128 and — the actual defect — omitted the ".." parent-directory
hop check, so a bd-sourced id carrying ".." passed straight through issueID
into argv and, downstream, path segments.

Every id validID sees originates from bd's own JSON output: issueID parses
it from `bd create --json`, and the lifecycle methods re-receive it as the
persisted BeadID. bd 1.1.2 mints only short hash-prefix ids (ship-8he,
proj-a3f8.1.2) that fit beadid.Valid's 64-byte bound with room to spare, and
the fleet and server already validate these same ids there — so the 128 bound
was ungrounded divergence, not a real requirement. Fold validID onto
beadid.Valid: one module owns the alphabet, no third copy, and the ..-hop gap
closes. The local name stays so the call sites read unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant