crueber/walhub

Watch

Fork 0

Annotated tag creation from the UI (follow-up to #253) #263

New issue

Closed

opened 2026-09-10 01:04:59 +00:00 by crueber · 4 comments

crueber commented

2026-09-10 01:04:59 +00:00

Owner

Follow-up to #253 (PR #262 shipped lightweight-only v1: a non-empty message on POST /{o}/{r}/api/tags is rejected 422, never silently downgraded).

Server-side annotated tags need a tag OBJECT in the object store (type/tagger/message), which no WAL entry kind carries today (proto is append-only/frozen). Options from #253: (a) extend the WAL with a tag-object entry kind (schema revision), or (b) construct the tag object server-side via the git binary and publish it as a PUSH-shaped entry.

Acceptance: POST with message writes the tag object, records NewPeeled, shows tagger/message in git; lightweight path unchanged; events identical.

Follow-up to #253 (PR #262 shipped lightweight-only v1: a non-empty `message` on POST `/{o}/{r}/api/tags` is rejected 422, never silently downgraded). Server-side annotated tags need a tag OBJECT in the object store (type/tagger/message), which no WAL entry kind carries today (proto is append-only/frozen). Options from #253: (a) extend the WAL with a tag-object entry kind (schema revision), or (b) construct the tag object server-side via the git binary and publish it as a PUSH-shaped entry. Acceptance: POST with `message` writes the tag object, records NewPeeled, shows tagger/message in git; lightweight path unchanged; events identical.

crueber referenced this issue

2026-09-10 01:05:13 +00:00

Create a tag at a given commit from the UI (commit view action), server-side via the WAL ref-update path #253

crueber commented

2026-09-10 02:30:34 +00:00

Author

Owner

Design ruling, #263 (annotated tag creation from the UI): rule (b) — git-binary-constructed tag object published as a PUSH-shaped entry. Reject (a).

Verdict

Implement (b): construct the tag object server-side with the git binary, pack the single object through the existing ingest path, and publish it as an EntryKindPush entry whose Txn carries the refs/tags/<name> create with NewPeeled set. No new WAL entry kind, no proto change, no core internal/wal change.

Winner mechanics (b), step by step

All git spawns run in the repo git-dir (GIT_DIR=<repo>, GIT_TERMINAL_PROMPT=0, bounded pool + ctx timeout, 8 KiB stderr discipline per docs/go/04_git.md §2). Two new pinned argv (both require a 04_git.md "Decisions & deviations" amendment in the implementing change, law 2 + law 12):

Construct + validate + write (one spawn): git mktag, tag content on stdin. mktag — verified present in git 2.53, the floor version — applies strict fsck to the tag body, writes the loose object, and prints the tag oid. It is chosen over git hash-object -t tag -w --stdin precisely because hash-object performs no fsck: it would accept a malformed tagger line/date that later fails fsck on fetch or breaks git show. Canonical stdin bytes, server-rendered (LF, trailing newline):
```
object <commit-sha>
type commit
tag <name>
tagger <name> <email> <unixtime> <tz>
<blank>
<message>
```
Pack the single object: <oid>\n | git pack-objects --stdout (plain oid list on stdin, no --revs), capturing pack bytes from stdout. One object, no deltas; flags minimal.
Ingest via the existing Layer.Ingest path (git index-pack --stdin --keep --rev-index --threads=0 [--fsck-objects], thin=false — the pack is complete), yielding the PreparedPack (checksum + local pack/idx paths + sizes + count=1). Move order idx→rev→pack and zero-object rules are unchanged.
Publish as PUSH through the existing funnel — Publish(PublishRequest{Pack: prepared, Txn: {Updates: [{Name: "refs/tags/<name>", OldOid: zero, NewOid: <tag-oid>, NewPeeled: <commit-sha>}]}, Meta: {principal, request_id, ...}}). publish.go maps Pack != nil to EntryKindPush and CASes the manifest; the bucket ACK (pack PUTs + log slot + manifest CAS) precedes the HTTP 201 (law 4 — never ACK before the bucket ACKs). The loose object from step 1 is serving-copy-local only and is not the replication unit; the pack PUT is what carries the object to every replica, which is why a REF_UPDATE-only publish would be a correctness hole (replicas materializing from packs alone would advertise a ref whose object they lack).
Ref + peel recording: the Txn create is CAS create (old-oid zero → present tag fails verify → 409, same as the #253 lightweight path, race-safe). NewPeeled carries the peeled commit sha, so snapshots/advertisements render the ^<peeled> continuation and peeled: lines with zero new code — the field and its readers already exist (proto.RefUpdate.NewPeeled, Ref.Peeled). Lightweight path unchanged (REF_UPDATE, no Pack, no NewPeeled).
Events identical: the events bridge keys off txn ref updates by ref kind, and commitLocal already folds txns for both PUSH and REF_UPDATE — but the implementing change must add a test proving a PUSH-shaped refs/tags/* create emits the identical tag event as a REF_UPDATE create (acceptance criterion, not assumed).

Failure semantics: mktag non-zero → 400/422, nothing published (malformed tagger/message never reaches the store); pack-objects/ingest failure → 5xx, nothing published; CAS conflict on existing tag → 409 via the verify step; CAS 412-restart handled by the existing ladder (pack PUTs are create-if-absent, safe to replay). No orphan risk beyond what §5.4 already covers.

### Concurrency note for the implementer: no new locks; per-repo serialization comes from the existing publisher single-flight + ingest lock (law 3, lock order ingest → wal sync preserved). The three spawns are sequential within the request (dependent), each pool-bounded with request-ctx cancellation.

Loser (a), with rationale

(a) — a new EntryKind (e.g. 6) carrying the tag object — is rejected:

Law 5 (frozen proto + fixtures): walgit.v1 wire compat with the Rust implementation is byte-pinned by golden fixtures. An append-only enum add is legal proto, but every reader (replay, remote reader, checkpoint fold, compaction, events, Rust side) must learn the kind, and the compat matrix doubles for a payload the store already carries as opaque pack bytes. Schema revision for a data-plane blob is disproportionate.
Law 8 (seams): (a) touches frozen core (internal/store/proto + internal/wal); (b) touches only the feature package (internal/tags) plus two pinned argv — core Publish already accepts Pack + Txn, proven by commitLocal folding both PUSH and REF_UPDATE txns.
Round-trip cost (law 6): (a) still needs the object bytes uploaded and referenced; it saves nothing over the single-object pack, which reuses the budgeted push path (≤ 5 requests).

Ruled sub-questions

Tagger identity: authed principal, server-rendered. Principal carries only Name (no email), so the server synthesizes the tagger line as <sanitized-principal-name> <<sanitized-name>@walhub.local> with server wall-clock time + local tz offset. Sanitize: strip <, >, \n, \r; empty-after-sanitize → 400. Rationale: the tag must attribute the human who clicked (auditability — Meta.principal records the same name), while the fixed domain marks it as server-minted rather than claiming a verified external email. Anonymous → 401 before any of this (write gate, unchanged).
Message validation: non-empty after trim (empty/whitespace = lightweight path, not an annotated tag — and per #253, a non-empty message is never silently downgraded); reject NUL bytes and non-UTF8; normalize to a single trailing \n.
Size caps: message ≤ 64 KiB (400 otherwise); the resulting single-object pack is then O(KiB) and trivially inside server.max_push_bytes. Rationale: tag messages are annotations, not blobs; the cap keeps the UI path off the bulk-bytes budget while leaving generous headroom (Linux kernel release notes fit in single KiBs).

# Design ruling, #263 (annotated tag creation from the UI): rule (b) — git-binary-constructed tag object published as a PUSH-shaped entry. Reject (a). ## Verdict Implement **(b)**: construct the tag object server-side with the `git` binary, pack the single object through the existing ingest path, and publish it as an `EntryKindPush` entry whose `Txn` carries the `refs/tags/<name>` create with `NewPeeled` set. **No new WAL entry kind, no proto change, no core `internal/wal` change.** ## Winner mechanics (b), step by step All git spawns run in the repo git-dir (`GIT_DIR=<repo>`, `GIT_TERMINAL_PROMPT=0`, bounded pool + ctx timeout, 8 KiB stderr discipline per `docs/go/04_git.md` §2). Two new pinned argv (both require a `04_git.md` "Decisions & deviations" amendment in the implementing change, law 2 + law 12): 1. **Construct + validate + write (one spawn): `git mktag`**, tag content on stdin. `mktag` — verified present in git 2.53, the floor version — applies strict fsck to the tag body, writes the loose object, and prints the tag oid. It is chosen over `git hash-object -t tag -w --stdin` precisely because `hash-object` performs **no** fsck: it would accept a malformed tagger line/date that later fails fsck on fetch or breaks `git show`. Canonical stdin bytes, server-rendered (LF, trailing newline): ``` object <commit-sha> type commit tag <name> tagger <name> <email> <unixtime> <tz> <blank> <message> ``` 2. **Pack the single object: `<oid>\n | git pack-objects --stdout`** (plain oid list on stdin, no `--revs`), capturing pack bytes from stdout. One object, no deltas; flags minimal. 3. **Ingest via the existing `Layer.Ingest` path** (`git index-pack --stdin --keep --rev-index --threads=0 [--fsck-objects]`, `thin=false` — the pack is complete), yielding the `PreparedPack` (checksum + local pack/idx paths + sizes + count=1). Move order idx→rev→pack and zero-object rules are unchanged. 4. **Publish as PUSH** through the existing funnel — `Publish(PublishRequest{Pack: prepared, Txn: {Updates: [{Name: "refs/tags/<name>", OldOid: zero, NewOid: <tag-oid>, NewPeeled: <commit-sha>}]}, Meta: {principal, request_id, ...}})`. `publish.go` maps `Pack != nil` to `EntryKindPush` and CASes the manifest; **the bucket ACK (pack PUTs + log slot + manifest CAS) precedes the HTTP 201** (law 4 — never ACK before the bucket ACKs). The loose object from step 1 is serving-copy-local only and is **not** the replication unit; the pack PUT is what carries the object to every replica, which is why a REF_UPDATE-only publish would be a correctness hole (replicas materializing from packs alone would advertise a ref whose object they lack). 5. **Ref + peel recording:** the `Txn` create is CAS create (old-oid zero → present tag fails verify → 409, same as the #253 lightweight path, race-safe). `NewPeeled` carries the peeled commit sha, so snapshots/advertisements render the `^<peeled>` continuation and `peeled:` lines with zero new code — the field and its readers already exist (`proto.RefUpdate.NewPeeled`, `Ref.Peeled`). Lightweight path unchanged (REF_UPDATE, no `Pack`, no `NewPeeled`). 6. **Events identical:** the events bridge keys off txn ref updates by ref kind, and `commitLocal` already folds txns for both PUSH and REF_UPDATE — but the implementing change must add a test proving a PUSH-shaped `refs/tags/*` create emits the identical tag event as a REF_UPDATE create (acceptance criterion, not assumed). **Failure semantics:** `mktag` non-zero → 400/422, nothing published (malformed tagger/message never reaches the store); `pack-objects`/ingest failure → 5xx, nothing published; CAS conflict on existing tag → 409 via the verify step; CAS 412-restart handled by the existing ladder (pack PUTs are create-if-absent, safe to replay). No orphan risk beyond what §5.4 already covers. **`### Concurrency` note for the implementer:** no new locks; per-repo serialization comes from the existing publisher single-flight + ingest lock (law 3, lock order ingest → wal sync preserved). The three spawns are sequential within the request (dependent), each pool-bounded with request-ctx cancellation. ## Loser (a), with rationale (a) — a new `EntryKind` (e.g. 6) carrying the tag object — is rejected: - **Law 5 (frozen proto + fixtures):** `walgit.v1` wire compat with the Rust implementation is byte-pinned by golden fixtures. An append-only enum add is *legal* proto, but every reader (replay, remote reader, checkpoint fold, compaction, events, Rust side) must learn the kind, and the compat matrix doubles for a payload the store already carries as opaque pack bytes. Schema revision for a data-plane blob is disproportionate. - **Law 8 (seams):** (a) touches frozen core (`internal/store/proto` + `internal/wal`); (b) touches only the feature package (`internal/tags`) plus two pinned argv — core `Publish` already accepts `Pack + Txn`, proven by `commitLocal` folding both PUSH and REF_UPDATE txns. - **Round-trip cost (law 6):** (a) still needs the object bytes uploaded and referenced; it saves nothing over the single-object pack, which reuses the budgeted push path (≤ 5 requests). ## Ruled sub-questions - **Tagger identity: authed principal, server-rendered.** `Principal` carries only `Name` (no email), so the server synthesizes the tagger line as `<sanitized-principal-name> <<sanitized-name>@walhub.local>` with server wall-clock time + local tz offset. Sanitize: strip `<`, `>`, `\n`, `\r`; empty-after-sanitize → 400. Rationale: the tag must attribute the human who clicked (auditability — `Meta.principal` records the same name), while the fixed domain marks it as server-minted rather than claiming a verified external email. Anonymous → 401 before any of this (write gate, unchanged). - **Message validation:** non-empty after trim (empty/whitespace = lightweight path, not an annotated tag — and per #253, a non-empty message is never silently downgraded); reject NUL bytes and non-UTF8; normalize to a single trailing `\n`. - **Size caps:** message ≤ **64 KiB** (400 otherwise); the resulting single-object pack is then O(KiB) and trivially inside `server.max_push_bytes`. Rationale: tag messages are annotations, not blobs; the cap keeps the UI path off the bulk-bytes budget while leaving generous headroom (Linux kernel release notes fit in single KiBs).

crueber referenced this issue from a commit

2026-09-10 03:03:02 +00:00

Fix #263: annotated tags via mktag+PUSH (ruling b)

crueber referenced this issue from a pull request that will close it,

2026-09-10 03:03:05 +00:00

Fix #263: annotated tags via mktag+PUSH #267

crueber commented

2026-09-10 03:03:11 +00:00

Author

Owner

Annotated tags implemented per the ruling (b) in PR #267 (branch fix/issue-263, ready for review — not merged). mktag → pack-objects --stdout → Layer.Ingest → PUSH publish with Txn + NewPeeled; no new WAL kind, no proto change. Tagger synthesis + sanitization, 64 KiB cap, 400/5xx/409 failure map as ruled. E2E proves the tag object + peel survive a wiped-cache restart; events parity proven PUSH-vs-REF_UPDATE. internal/tags at 97.9% coverage, -race green, 528/528 node tests.

crueber commented

2026-09-10 03:22:00 +00:00

Author

Owner

Review of PR #267 (fix/issue-263, commit bdad949 + review nit 892ac90), verified in scratch worktree against ruling (b) on issue #263:

RULING COMPLIANCE — all elements verified in code, not assumed:

mktag over hash-object with fsck rationale: internal/tags/git.go CreateTagObject uses argv [mktag], stdin body; comment + 04_git.md amendment state the hash-object rejection reason. PASS
pack-objects flags minimal: [pack-objects --stdout], plain oid list on stdin, no --revs (git.go PackObject). PASS
ingest thin=false + fsck: cmd/walhub/tags.go ingestTagPack calls Ingest(..., false, true) = thin=false, fsck=true. PASS
PUSH publish (not REF_UPDATE): CreateAnnotatedTag sets PublishRequest.Pack; publish.go maps Pack!=nil to EntryKindPush (verified L333-335). PASS
NewPeeled set to peeled commit; CAS create old-oid zero; conflict wording maps to 409. PASS
Loose object not replication unit: PreparedPack (pack+idx PUT, create-if-absent) is what Publish uploads; e2e test proves ref+object survive wiped-cache registry restart. PASS
Tagger synthesis: renderTagger strips <>\n\r, empty-after-sanitize 400, fixed @walhub.local domain, wall-clock + local tz; anonymous hits 401 at write gate first. PASS
Message validation: NUL/non-UTF8/64KiB reject (400), single trailing \n, whitespace-only routes to lightweight, non-empty never downgraded. PASS
Failure map: mktag reject 400, pack/ingest 5xx, exists 409, anon 401; tests assert nothing published and mktag unreached on every failure path. PASS (ruling allows 400/422; 400 chosen, 422 mapping retained harmlessly)
Tag.SHA semantic (tag oid vs commit): documented on Tag struct + CreateInput; no consumer broken — releases ResolveRef peels ^{commit} so release-from-annotated-tag works; UI shows name only; SDK passthrough. Lightweight path byte-identical (early REF_UPDATE path untouched, no Pack/NewPeeled). PASS
Events parity test genuine: PUSH vs REF_UPDATE entries compared with reflect.DeepEqual modulo envelope EntryKind. PASS
No new WAL kind / zero proto change / zero core change: git diff shows zero delta in internal/store (incl. proto), internal/wal, internal/git. PASS
Law 2+12: both argv pinned in 04_git.md amendment, 14_extensibility.md Decisions entry, same change. PASS
Coverage: internal/tags 97.9%, internal/events 98.8% (gate >=95%). No new non-stdlib imports; go.mod/go.sum/package.json untouched.

TEST RESULTS (scratch worktree /tmp/pr267):

go test -race ./internal/tags/... : ok (includes mktag round-trip, validation, failure-map, conflict tests)
go test -race -run TestTags ./cmd/walhub : ok (lightweight e2e + annotated e2e through real WAL)
go test -race ./internal/events/... : ok (incl. parity test)
node --test web/test/unit/*.test.js : 528/528 pass (initial 7 file failures were missing node_modules in scratch only; resolved via symlink to main-worktree deps, read-only)
gofmt clean; go vet clean (tags, events, cmd/walhub)
No browser run (UI change is one optional text field; unit tests + reasoning per review instructions — noted explicitly).

FIX APPLIED: stale tagWire comment ('the created lightweight tag') now covers annotated responses; pushed as 892ac90 to origin/fix/issue-263, tags tests re-run green.

MERGE RECOMMENDATION: ready to merge.

Review of PR #267 (fix/issue-263, commit bdad949 + review nit 892ac90), verified in scratch worktree against ruling (b) on issue #263: RULING COMPLIANCE — all elements verified in code, not assumed: - mktag over hash-object with fsck rationale: internal/tags/git.go CreateTagObject uses argv [mktag], stdin body; comment + 04_git.md amendment state the hash-object rejection reason. PASS - pack-objects flags minimal: [pack-objects --stdout], plain oid list on stdin, no --revs (git.go PackObject). PASS - ingest thin=false + fsck: cmd/walhub/tags.go ingestTagPack calls Ingest(..., false, true) = thin=false, fsck=true. PASS - PUSH publish (not REF_UPDATE): CreateAnnotatedTag sets PublishRequest.Pack; publish.go maps Pack!=nil to EntryKindPush (verified L333-335). PASS - NewPeeled set to peeled commit; CAS create old-oid zero; conflict wording maps to 409. PASS - Loose object not replication unit: PreparedPack (pack+idx PUT, create-if-absent) is what Publish uploads; e2e test proves ref+object survive wiped-cache registry restart. PASS - Tagger synthesis: renderTagger strips <>\n\r, empty-after-sanitize 400, fixed @walhub.local domain, wall-clock + local tz; anonymous hits 401 at write gate first. PASS - Message validation: NUL/non-UTF8/64KiB reject (400), single trailing \n, whitespace-only routes to lightweight, non-empty never downgraded. PASS - Failure map: mktag reject 400, pack/ingest 5xx, exists 409, anon 401; tests assert nothing published and mktag unreached on every failure path. PASS (ruling allows 400/422; 400 chosen, 422 mapping retained harmlessly) - Tag.SHA semantic (tag oid vs commit): documented on Tag struct + CreateInput; no consumer broken — releases ResolveRef peels ^{commit} so release-from-annotated-tag works; UI shows name only; SDK passthrough. Lightweight path byte-identical (early REF_UPDATE path untouched, no Pack/NewPeeled). PASS - Events parity test genuine: PUSH vs REF_UPDATE entries compared with reflect.DeepEqual modulo envelope EntryKind. PASS - No new WAL kind / zero proto change / zero core change: git diff shows zero delta in internal/store (incl. proto), internal/wal, internal/git. PASS - Law 2+12: both argv pinned in 04_git.md amendment, 14_extensibility.md Decisions entry, same change. PASS - Coverage: internal/tags 97.9%, internal/events 98.8% (gate >=95%). No new non-stdlib imports; go.mod/go.sum/package.json untouched. TEST RESULTS (scratch worktree /tmp/pr267): - go test -race ./internal/tags/... : ok (includes mktag round-trip, validation, failure-map, conflict tests) - go test -race -run TestTags ./cmd/walhub : ok (lightweight e2e + annotated e2e through real WAL) - go test -race ./internal/events/... : ok (incl. parity test) - node --test web/test/unit/*.test.js : 528/528 pass (initial 7 file failures were missing node_modules in scratch only; resolved via symlink to main-worktree deps, read-only) - gofmt clean; go vet clean (tags, events, cmd/walhub) - No browser run (UI change is one optional text field; unit tests + reasoning per review instructions — noted explicitly). FIX APPLIED: stale tagWire comment ('the created lightweight tag') now covers annotated responses; pushed as 892ac90 to origin/fix/issue-263, tags tests re-run green. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-10 03:23:22 +00:00

crueber commented

2026-09-10 03:23:23 +00:00

Author

Owner

Fixed by PR #267 incl. review comment fix (ruling (b) verified element-by-element; 97.9%/98.8% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-10 03:23:23 +00:00

Merge pull request 'Fix #263: annotated tags via mktag+PUSH' (#267) from fix/issue-263 into main

crueber added the

Rows
Columns