docs(plans): add S10 Maven local implementation plan

Moaz Khalifarequested to merge
mkhalifa3/plan-s10-maven-local into main

🎯 Context

Decomposes S10 Maven local into 12 MR-sized steps. Work item #21 (closed).

Per the /planner skill: research and approach signed off via the plan-MR merge gate; no step MR may open before this lands on main.

This MR has been revised across multiple review cycles (S06 spec + S27 Phase 1 plan alignment, compression pass against the spec-mirror anti-pattern, pr-review-toolkit code-reviewer pass, plan-exit-review, blob-storage foundation alignment). The shape and key decisions below reflect the current plan, not the initial draft.

Resolved blockers:

  • "How to FK to blob_storage_attachments" (review thread) — the foundation MR for the table (plan merged 2026-05-21; Step 1 implementation MR pending) lands the partitioned table + shared CreateBlobStorageAttachment(ctx, q Querier, nsID, sha256) free function + ErrParentBlobMissing sentinel. maven_files actually carries two blob FKs (composite into blob_storage_attachments, direct into blob_storage_blobs(namespace_id, sha256) per ADR-007:735-736), so Step 2 is gated on the foundation MR and S06 Step 6 (which lands blob_storage_blobs + the attachments → blobs FK, expanded per the foundation plan). Step 8b and Step 9 call the shared helper from inside the format tx, map ErrParentBlobMissing to 500, and inherit the foundation plan's MR-order gate: no INSERT-callsite MR may merge before S06 Step 6.

🧭 Plan shape

  • 12 steps, sized 200–400 LOC each target (~500 ceiling for genuinely indivisible concerns; vendored / generated / binary excluded), per /planner skill rules.
  • Step 8 pre-split into 8a + 8b based on pr-review-toolkit feedback — 8a ships the pre-stream slice (SetReadDeadline + MaxBytesReader + find-or-create + version-cap with 501-stub on the stream branch); 8b ships stream + commit + duplicate handling + concurrent test harness + test-data fixture generator. Eliminates late-MR-open size-decision pressure.
  • Mermaid DAG + per-step Depends on: field (per !200 (merged) /planner conventions).
    • Parallel-runnable cohorts: roots (Steps 1 + 3 — both Depends on: —; Step 2 is foundation-gated and opens once the blob_storage_attachments foundation MR + S06 Step 6 merge); format internals (Steps 6/8a/9 sibling-parallel sharing predecessors 4 + 5); external-gated tail (Steps 10 + 11 each gate on a separate external dep).
    • Sequential coupling: 8b after 8a; 7 after 6 (shared digest.go + DigestStore).
  • Exhaustive AC 1–33 → step matrix as authoritative coverage contract.
  • Direct S06 import. Maven imports internal/storage types directly — BlobStore, Session, BlobContent (with IsRedirect() / RedirectURL() / Reader() / Size()), BlobOpenOption (with WithDeliveryMode + WithForceStream), BlobCommitOption (with WithBlobMetadata), BlobUserMetadata{SHA1 *SHA1Sum}, sentinels. No Maven-side facade. Maven calls StoreBlob / OpenBlob / DeleteBlob / NewSession + the Session lifecycle (Write / Close / Commit / Cancel / UploadID); S06's BlobInfo / ResumeSession / GetSessionStatus exist on the interface but Maven never calls them (single-shot PUTs, no resume, no SHA-256 lookups). Integration tests + dev/CI binary consume internal/storage/stub (S06 Steps 2–4); when S06 Steps 7–9 + 16/17 merge, the composition root swaps to the real BlobStore transparently through the interface — no Maven step required.
  • S08 stubbed at the middleware boundary. If the request reaches the Maven handler, auth + authz have been resolved by middleware. S08 today does not load namespace / repository into the request context; the Maven handler loads both itself in Step 5 pre-flight (shared NamespaceStore.FindBySlug + Step 4's FindRepository, which wraps shared RepositoryStore.FindByName). Step 6's OpenBlob call-site reads delivery_mode_override from the Namespace row Step 5 loaded — no context-value source, no Identity / NamespaceMeta placeholders.
  • Step 10 consumes S27's jobsriver.Client Phase 1 (Maven is a named Phase 1 consumer per S27 spec). EnqueueTx[T] is atomic with the caller's *sql.Tx per S27 spec AC #5. Per-Maven-package dedup contract satisfied via WithUnique(UniqueByArgs()) + the data-model invariant that (NamespaceID, RepositoryID) is functionally determined by PackageID at the data layer.
  • Step 11 runs the registry-conformance Maven runner.
  • Invariant guardrails (Steps 4 + 5) in a single invariants_test.go with two AST-walk diagnostics: TODO(s18-buffered-counters) markers on direct-UPDATE sites (Step 4) + http.Error-ban in the package outside test files (Step 5, AC 30).
  • Explicit interface satisfaction on *store (var _ DownloadStore = (*store)(nil) × 4) so consumer-interface signature drift fails at the implementation site, not at handler wire-up.

📦 Key design decisions

  • Greenfield rewrite from spec. Monolith (app/services/packages/maven/*) referenced for behavior edges only.
  • Path traversal & router invariant (AC 19, T-02 threat model). Primary .. defense is net/http.ServeMux cleaning; Step 5's segment rejection is defense-in-depth. 9-wire-form variant matrix on both GET and PUT lives in Step 5's handler tests; router-swap MRs MUST keep it green.
  • Archive-class downloads_count allowlist {jar, war, ear, aar, zip} — matches monolith framing; .pom / .module / metadata don't bump.
  • SHA-1 hand-off via WithBlobMetadata. Step 8b passes WithBlobMetadata(BlobUserMetadata{SHA1: ...}) to Session.Commit; S06 writes the value as x-amz-meta-checksum-sha1 / x-goog-meta-checksum-sha1 atomically with the move. Redirect-mode responses carry the SHA-1 header from the storage backend on the final 200.
  • MD5 redirect-mode asymmetry (per merged S10 spec amendment): MD5 never persisted as backend object metadata; proxy mode emits from maven_files.md5 column; .md5 sidecar GET/HEAD short-circuits with 422 md5_disallowed_under_fips before any DB lookup under FIPS-on.
  • Handler-owned redirect emit: BlobContent.IsRedirect() == true → write 302 Found + Location: <RedirectURL()> per S10 spec API Contracts. Plan adopts S06's value-object API; the S10 spec's older "S06 owns serve path" wording is flagged as a stale-relative-to-S06-surface ambiguity for post-implementation spec amendment.
  • Handler-owned Range/206/416 dispatch on the stream path (parse Range, validate against BlobContent.Size(), write 200 / 206 + Content-Range / 416 + bytes */<size>). Per S06 spec BlobStore docstring: format handlers own the redirect-vs-stream branch.
  • HEAD bypasses OpenBlob structurally (row + headers + empty body; no signed URL ever generated). Honors spec's "HEAD is never redirected" by construction, not by ignoring IsRedirect() after a wasted call.
  • Counter emissions deferred to S18 / S20 / S22. Every direct-UPDATE call-site (BumpAccessTimestamps, BumpRepoDownloadCount, BumpRepoCounters, MarkRepoLastUpdated) ships as a TODO-marked no-op stub in Step 4. The mechanism (buffered pipeline, batched flush, etc.) is owned by S18 (download tracking) + S20 (lifecycle) + S22 (storage accounting) per the two-stage development model. Marker convention enforced by invariants_test.go diagnostic #1.
  • CPU-bound publish observation. Maven primary PUTs run four hash operations per byte split across the Maven handler (SHA-1 + SHA-512 + MD5 unless FIPS) and S06's Session (incremental SHA-256). Treated as a CPU-hot path during deployment sizing; exact CPU budget at the ADR 004 5 GB ceiling is hardware-dependent and should be measured against production CPU profile. Publish concurrency protection is platform-territory (deployment-level HTTP-server max-conns / upstream proxy) and out of scope for this plan.
  • Reconciler enqueue contract (T-06). Per-Maven-package enqueue dedup + idempotent execution under at-least-once delivery. Phase 1 binds dedup to WithUnique(UniqueByArgs()) — operationally equivalent to per-Maven-package because (NamespaceID, RepositoryID) is functionally determined by PackageID at the data layer.
  • Reconciler WithForceStream() mandatory — server-side caller cannot follow a redirect. Two complementary tests: call-site mock assertion + behavioral guard (stub returns IsRedirect() == true → reconciler returns clean typed error instead of corrupting/panicking/hanging).
  • No EarlyDuplicate fast-path — authoritative SQL conflict check at commit time is sufficient; spec fast-path is "optional optimization".
  • Storage-quota 429 mapping — deferred. internal/storage does not declare a quota sentinel today. The intended mapping per spec Step 4c → ADR 004 Storage Quota (429 Too Many Requests + code: "quota_exceeded" + Retry-After) is deferred to a plan-amendment once the sentinel is defined.
  • ErrInvalidMetadata sentinel (Step 9 reconciler) for DOCTYPE rejection (billion-laughs defense, asserted via errors.Is).
  • <release> conditional emit (Step 9): unconditional <latest> + <lastUpdated>; emit <release> only when a live non--SNAPSHOT version exists.

🧪 Test plan

  • Vale + markdownlint + lychee + gitlint pass (verified locally; all hooks green at commit).
  • Reviewer walks the AC matrix and confirms every per-step Acceptance bullet matches.
  • Reviewer verifies the DAG (Mermaid + Depends on:) is consistent with the Status table (12 rows: 1, 2, 3, 4, 5, 6, 7, 8a, 8b, 9, 10, 11).
  • Reviewer confirms the three "Spec ambiguities resolved in plan" are acceptable resolutions (package-level primary GET counter, archive-class downloads_count narrowing, S10 spec Step 3b stale relative to S06 surface).
  • Reviewer confirms direct internal/storage import (no Maven facade) — integration tests + dev binary consume internal/storage/stub (S06 Steps 2–4); real-impl swap is transparent through the interface.
  • Reviewer confirms Step 2's dependency on the blob_storage_attachments foundation MR + S06 Step 6 is acceptable (FK targets + shared CreateBlobStorageAttachment helper + MR-order gate for Step 8b / 9 INSERT-callsites).
  • Reviewer confirms Step 10's EnqueueTx[ReconcileArgs] binds correctly to S27 Phase 1's *sql.Tx API and the WithUnique(UniqueByArgs()) dedup mechanism.

🤖 Generated with Claude Code

Edited by Moaz Khalifa

Merge request reports