feat(npm): streampub streaming attachment integration (part 6/8)

📦 What this MR does

Part 6/8 of the stack splitting the streampub streaming publish-envelope library (S11 Step 13). See Part 1/8 (!432) for the stack overview and the byte-identity contract against the reference, !420 (closed).

  • Targets 10io/npm-local-step-13-part5of8 (Part 5/8). GitLab retargets to main as the parent merges.
  • Depends on Part 5/8 (set as a merge-request dependency).

This is the streaming integration — it wires the leaf primitives into the walk, completing the single-pass design. After this, only the checked-in fixture corpus and the fuzz target remain (Part 8/8).

  • pipeline.go (new) — pipeAttachment owns the io.Pipe lifecycle: base64 decode → multi-hash + size-limit → bounded buffer → inspector goroutine, with the drain-on-error / CloseWithError contract so neither side blocks until the request read deadline. resolveAttachment combines the producer and inspector outcomes into one typed result.
  • attachment.go — the data field is now streamed (streamData) rather than skipped: it splices the decoder's UnreadBuffer ahead of the reader, hands the base64 body to the pipeline, then consumes the envelope remainder with consumeToEnvelopeEnd — a string-aware tail scanner that enforces MaxEnvelopeDepth inline, so a depth bomb after _attachments cannot bypass the tokenizer's guards. This replaces the Part 4/8 structural stub.
  • inspector.go (new) — runInspector gzip-decompresses, walks the tar, captures package/package.json (bounded by MaxPackageJSONSize), validates entry paths (rejecting absolute, .., and backslash traversal), and always drains the pipe.
  • walker.go — gains the src reader and the done flag the streaming hand-off needs.
  • error.go — the coherence / tarball / package-json codes and sentinels, referenced for the first time here.
  • Deletes primitives_test.go — the Part 5/8 throwaway. The primitives are now exercised black-box through Process.

🧪 Tests

  • TestProcess_HappyPath — the full result: multi-hash digests (SHA-256/1/512) over the decoded tarball, captured package/package.json, tarball size, and the envelope identity fields.
  • TarballSizeInvalid, Base64Corruption, DecoderBoundaryClean, TarBackslashTraversalRejected, ErrorDoesNotLeakEntryName, TrailingFieldBoundBypass (the AC-44 envelope-tail depth bound via the streaming tail scanner).

The fixture-corpus coherence cases (TestProcess_CoherenceFailures) and the fuzz target land with the checked-in corpus in Part 8/8.

  • golangci-lint (repo-pinned v2.12) → 0 issues
  • GOEXPERIMENT=jsonv2 go test -race -count=1 ./internal/format/npm/streampub/... → pass (17 tests)
  • Diff is 466 net lines (633 added / 167 removed — the removed lines are the deleted throwaway test and the Part 4/8 attachment stub now superseded by streamData). Within the 500-LOC reviewability guideline; the production half is one interlocking mechanism (the single-pass hand-off), reviewed as a unit.

🚩 Deviations from the reference (!420 (closed))

With this part, attachment.go, walker.go, token.go, versions.go, name.go, base64reader.go, buffer.go are byte-identical to the reference. The remaining deviations are the previously-documented reviewed changes, the stale-comment fixes, plus two review fixes to pipeline.go/inspector.go here:

  • error.go / streampub.go — the ProcessError API + six-cap validate (Parts 1/2).
  • sink.go — the sizeLimitWriter byte-count fix (Part 5).
  • pipeline.gopipeAttachment now closes the pipe writer via a deferred CloseWithError on the named return, per the S11 coherence-sink lifecycle contract (the reference closes inline only; a producer panic could leave the inspector blocked). The inline close is retained before the join so the inspector unblocks on the normal path (a defer-only form would deadlock).
  • inspector.goinspectTarball skips a non-regular tar entry at package/package.json rather than capturing it as the manifest, per docs/dev/go-secure-coding.md ("skip non-regular files"). Path-traversal validation still runs for every entry.
  • Stale comment fixes (this part): TestProcess_TarBackslashTraversalRejected, TestProcess_ErrorDoesNotLeakEntryName, and TestProcess_TrailingFieldBoundBypass carried test-author-phase "this test FAILS today" notes (and ErrorDoesNotLeakEntryName described the pre-Part-1 Error() format that embedded the cause). The implementations now satisfy these tests, so the comments are corrected to describe current behavior.
  • Added test coverage (6c6e17c): TestProcessError_LeakPreventionContract asserts both halves of the *ProcessError model — Error() returns only the spec code, and the wrapped cause stays reachable via Unwrap. This takes error.go Unwrap from 0% → 100%; it was unexercised because errors.As in the helpers matches *ProcessError at the first level without unwrapping past it. The reference leaves Unwrap at 0% too; this is extra coverage in the permanent streampub_test.go, not a functional change.

Security / spec-compliance deviations (deep review, 2ff91d6). Two gaps that exist in the reference itself; both run on untrusted input once streampub is wired into the publish handler (a later step), so closed before that:

  • Decompression-bomb cap (inspector.go / streampub.go): the reference caps only the compressed .tgz (MaxTarballSize) and the manifest body, leaving the decompressed tar walk unbounded — a go-secure-coding.md violation (a small payload can inflate ~1000:1). Adds a dedicated MaxUncompressedTarballSize Limits cap, wraps the gzip stream in io.LimitReader, and fails closed in validate(); exceeding it is manifest_coherence_failed. Open item: the production default for this cap needs config/spec-author sign-off at the publish-handler wiring step (this part only adds the injected bound + enforcement).
  • Inspector panic recovery (inspector.go): S11 Step 3 requires "the reader goroutine MUST NOT panic", but the reference has no recover() — a panic in the off-request inspector goroutine (beyond LabKit's PanicRecoveryMiddleware) would crash the process. runInspector now recovers into a coherence failure, still draining and sending once on done. Follow-up (09f4555): the recover path wraps a distinct errInspectorPanic sentinel (not errCoherence) so errors.Is separates a recovered panic from routine corruption (client code unchanged: manifest_coherence_failed); the panic metric/export is deferred to the wiring step.
  • Control-byte tar-path rejection (inspector.go, c43746c): validateEntryPath now rejects NUL and ASCII control bytes (go-secure-coding.md), the same path-poisoning class as the existing backslash guard. The reference lacks this. NUL can't round-trip through archive/tar's writer, so it's tested white-box against validateEntryPath; a surviving control byte is tested black-box through Process.
  • Docstring scope (streampub.go): the Process cancellation claim is scoped to the tokenizing phase; the streaming phase's backstop is the request read deadline (S11 Step 3 forbids a ctx.Done() side goroutine). Comment-only.
  • Tests: TestProcess_UncompressedTarballTooLarge, TestRunInspector_RecoversPanic (white-box, panicking reader), and TestProcess_ManifestTooLarge (AC 41 package_json_too_large, inline — the reference covers it only via the Part 8/8 fixture corpus).
Edited by David Fernandez

Merge request reports

Loading
Loading