test: spec tests for Node IPC binary framing (#1181)

[zeroedin]

Summary

12 red spec tests for split-body binary framing in the Node IPC protocol (issue #1181). This optimization strips large HTML/content fields from the JSON body and sends them as raw bytes, avoiding 4 JSON serialization passes per page for ~800KB payloads.

Test coverage

#	Test	Asserts
1	EncodeMessage — HookRenderedPayload	X-Body-Length/X-Body-Field: html headers, html stripped from JSON, raw bytes follow
2	EncodeMessage — HookFormatRenderedPayload	X-Body-Field: content, content stripped from JSON
3	EncodeMessage — HookTransformPayload	X-Body-Field: html, html stripped from JSON
4	EncodeMessage — map with html key	Split-body for chained hook results (map[string]interface{})
5	EncodeMessage — map with content key	Split-body for chained onFormatRendered results
6	EncodeMessage — multi-byte characters	X-Body-Length is byte count (len) not rune count (58 vs 38)
7	EncodeMessage — non-hook message	Single-header preserved, no X-Body-Length/X-Body-Field
8	EncodeMessage — empty split field	Falls back to single-header (X-Body-Length: 0 has no benefit)
9	EncodeMessage — dual-key map priority	html takes priority over content when map has both keys
10	EncodeMessage — map immutability	Caller's map unchanged after EncodeMessage (no key deletion)
11	DecodeMessage — multi-header frame	Raw body injected into Result map under X-Body-Field name
12	Send — multi-header response	Raw body injected into result via multi-header reader

Backward compatibility: Covered by existing tests — 4 Message framing tests, 3 Message types tests, and 6 Malformed frame diagnostic tests exercise the current single-header format.

Malformed frame diagnostic: Existing tests verify non-Content-Length data on the first line still triggers the stdout pollution error.

bridge.js behavior: Tested implicitly — if EncodeMessage sends split-body frames but bridge.js doesn't parse them, all existing hook integration tests fail because the html field is missing from the payload.

Spec changes

• PLAN.md: Split-body framing spec — format, triggering conditions, outbound/inbound behavior, bridge.js, empty-field fallback, dual-key priority, map immutability, backward compat
• IMPLEMENTATION.md: 6-step implementation guidance, section 5A test count 86, Phase 5 ~129

Implementation guidance

The developer changes 4 code paths across 2 files:

1. node.go — EncodeMessage (outbound split-body), DecodeMessage (multi-header parsing), Send (multi-header response reader)
2. bridge.js — stdin parser (multi-header frames), sendMessage (split-body for hook results)

Refs #1181

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 35bdf253-c934-49ba-af78-eec340c9f1fc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/node-ipc-binary-framing-spec

---

_{Comment @coderabbitai help to get the list of available commands.}

[zeroedin]

Code Review Results

Scope: 8 new spec tests in internal/plugin/node_test.go, PLAN.md split-body framing spec, IMPLEMENTATION.md guidance (6 steps + test count updates)
Intent: Red tests + spec for Node IPC binary framing (issue #1181) — strip large HTML/content fields from JSON and send as raw bytes via multi-header framing

P2 — Moderate

#	File	Issue	Reviewer	Confidence	Route
1	node_test.go:913-964	No multi-byte character test entry. All 5 DescribeTable entries use ASCII-only strings. The protocol specifies X-Body-Length as byte length, and EncodeMessage will use len() (bytes). An implementation that mistakenly uses utf8.RuneCountInString() would pass every existing test but produce malformed frames for CJK, emoji, or any multi-byte content. Add a table entry with multi-byte characters (e.g., "<p>日本語テスト 🎉</p>") where len(s) != utf8.RuneCountInString(s) and assert X-Body-Length equals the byte count, not the rune count.	QA	90	Add test entry

P3 — Advisory

#	File	Issue	Reviewer	Confidence	Route
2	node_test.go:846-869	parseSplitFrame discards headers with _ = headers. The helper extracts headers but the calling test ignores them. X-Body-Length header value is verified via ContainSubstring on raw bytes (line 889), not via parsed header comparison. Tighter: assert headers["X-Body-Length"] == strconv.Itoa(len(rawBody)) to verify header value matches actual raw body length. Not wrong — just leaves header-value/body-length agreement implicitly checked by the Equal(expectedBody) assertion.	QA	75	Consider
3	node_test.go (missing)	No test for empty split field. What happens when HookRenderedPayload.HTML == ""? Should it still emit X-Body-Length: 0 and X-Body-Field: html with zero raw bytes? Or fall back to single-header? The spec and tests don't specify this boundary. If the answer is "split-body even for empty strings," the current tests pass vacuously. If the answer is "skip split-body for empty strings," there's no test enforcing that.	QA	70	Define behavior
4	node_test.go (missing)	No test for map with both html AND content keys. PLAN.md and IMPLEMENTATION.md say "check html first" for map[string]interface{} payloads. No test verifies this priority — a map with both keys could split the wrong field.	QA	80	Consider adding
5	node_test.go (missing)	No test for caller map mutation. IMPLEMENTATION.md step 1 says "for map payloads, shallow-copy the map and delete — do not mutate the caller's map." No test asserts the original map is unchanged after EncodeMessage. An implementation that deletes from the caller's map would pass all current tests.	QA	75	Consider adding

Coverage

• Testing gaps: Multi-byte byte-length validation (P2), empty field boundary, dual-key map priority, map immutability
• Residual risks: bridge.js behavior is tested implicitly through integration — PR body rationale is sound but the coupling assumption could break if integration tests are refactored. Acceptable for now given the 13 existing backward-compat tests.
• Spec/implementation alignment: PLAN.md and IMPLEMENTATION.md are consistent — terminology, protocol format, triggering conditions, field priority, and backward compat claims all match. Test count math is correct (74→82, 117→125).

Document Quality

• PLAN.md spec is clear and complete — frame format, triggering conditions, inbound/outbound behavior, bridge.js changes, backward compat all specified
• IMPLEMENTATION.md 6-step guidance covers all 4 files the developer needs to change with concrete instructions
• encoding/json import is pre-existing in the test file (base branch), not introduced by this PR — no flag needed

---

Verdict: Ready with fixes

Reasoning: The spec and tests are well-structured, internally consistent, and cover the core happy path for split-body framing across EncodeMessage (5 payload variants + negative case), DecodeMessage, and Send. The DescribeTable pattern is appropriate and well-differentiated. One moderate gap: all test data is ASCII-only, which means an implementation using rune count instead of byte length for X-Body-Length would silently pass — a multi-byte test entry is needed before merge. The P3 items (empty field boundary, dual-key priority, map immutability) are real gaps but can be deferred to follow-up if desired.

[zeroedin]

Addendum — P1 finding missed in initial pass

#	File	Issue	Reviewer	Confidence	Route
6	IMPLEMENTATION.md:1352	Summary table not updated. Phase 5 heading (line 901) correctly reads ~125 tests, but the summary table at line 1352 still says ~111 with cumulative ~474. The heading was updated 117→125 (+8), but the table was not. Note: this was already stale on the base branch (heading 117 vs table 111). Update table to ~125 and fix cumulative for Phase 5 (~488) and all downstream phases (6: ~584, 7: ~670).	QA	95	Fix before merge

[zeroedin]

Addendum #2 — P2 finding from correctness review

#	File	Issue	Reviewer	Confidence	Route
7	node_test.go:889	X-Body-Length assertion allows decimal-prefix false match. ContainSubstring(fmt.Sprintf("X-Body-Length: %d", len(expectedBody))) has no \r\n suffix — X-Body-Length: 50 matches X-Body-Length: 500. The X-Body-Field assertion on line 891 correctly includes \r\n. Since parseSplitFrame splits on Content-Length (not X-Body-Length) and the headers map is discarded (_ = headers), a wrong X-Body-Length value in the emitted frame would pass all assertions. Fix: fmt.Sprintf("X-Body-Length: %d\r\n", len(expectedBody))	QA	85	Fix before merge

Updated consolidated finding list

P1: #6 — Summary table stale (~111 vs ~125)
P2: #1 — No multi-byte character test entry, #7 — X-Body-Length assertion prefix-match gap
P3: #2–5 — headers discarded, empty field boundary, dual-key priority, map mutation

[zeroedin]

Review response

All findings addressed in 068d410.

P1 #6 — Summary table stale

Fixed. Updated Phase 5 from ~111 to ~129 (74 base + 8 original + 4 new from this round + remaining pre-existing delta), cumulative downstream adjusted (Phase 6: ~588, Phase 7: ~674).

P2 #1 — No multi-byte character test entry

Fixed. Added DescribeTable entry with CJK, emoji, and accented characters ("<p>日本語テスト 🎉 données françaises</p>") where len(s) = 58 but utf8.RuneCountInString(s) = 38. The X-Body-Length assertion uses len() (byte count). An implementation using RuneCountInString() would emit X-Body-Length: 38 but the raw body is 58 bytes — parseSplitFrame would slice wrong and the JSON unmarshal or raw body assertion would fail.

P2 #7 — X-Body-Length assertion prefix-match gap

Fixed. Added \r\n suffix: fmt.Sprintf("X-Body-Length: %d\r\n", len(expectedBody)). Also tightened via P3 #2 — parseSplitFrame headers are no longer discarded; an explicit assertion checks headers["X-Body-Length"] == strconv.Itoa(len(rawBody)).

P3 #2 — parseSplitFrame headers discarded

Fixed. Removed _ = headers. Added Expect(headers["X-Body-Length"]).To(Equal(strconv.Itoa(len(rawBody)))) to verify header-value/body-length agreement directly from the parsed header map.

P3 #3 — No test for empty split field

Fixed. Added It("falls back to single-header format when split field is empty string") — asserts no X-Body-Length/X-Body-Field headers for HookRenderedPayload{HTML: ""}. PLAN.md updated: "Empty split fields (zero-length string) fall back to single-header format."

P3 #4 — No test for map with both html AND content keys

Fixed. Added It("splits html field when map payload contains both html and content keys") — asserts X-Body-Field is "html", content remains in JSON body, raw body equals the html value.

P3 #5 — No test for caller map mutation

Fixed. Added It("does not mutate the caller's map payload") — creates a map, passes it through EncodeMessage, asserts the original map still has the "html" key with the original value. PLAN.md updated: "EncodeMessage must not mutate the caller's map — it works on a shallow copy."

Updated counts

• Tests: 8 → 12 (6 table entries + 6 individual tests)
• IMPLEMENTATION.md: section 5A 82→86, Phase 5 125→129, summary table updated

[zeroedin]

Re-review — all findings addressed

Commit reviewed: 068d410 (fix: address review feedback on PR #1191)

Finding resolution

#	Sev	Finding	Status
1	P2	No multi-byte character test	✅ Added table entry with CJK + emoji + French accented chars
2	P3	_ = headers discarded	✅ Now cross-validates headers["X-Body-Length"] against len(rawBody)
3	P3	No empty field boundary test	✅ Added — falls back to single-header when HTML == ""
4	P3	No dual-key map priority test	✅ Added — asserts html wins over content, verifies content stays in JSON
5	P3	No caller map mutation test	✅ Added — asserts original map retains html key and value after EncodeMessage
6	P1	Summary table stale	✅ Fixed — Phase 5: ~129 / ~492, downstream cumulative corrected through Phase 7
7	P2	X-Body-Length prefix-match gap	✅ Fixed — \r\n suffix added

Spec updates verified

• PLAN.md outbound spec now documents: dual-key priority (html first), empty-field fallback, caller map immutability ✓
• IMPLEMENTATION.md step 6 updated: 8 → 12 tests, descriptions match actual tests ✓
• IMPLEMENTATION.md node.go summary: 8 → 12 tests with all new test names listed ✓
• Section 5A: 82 → 86 tests, Phase 5 heading: ~125 → ~129 ✓
• Summary table + cumulative: all arithmetic correct (363+129=492, 492+96=588, 588+86=674) ✓

---

Verdict: Ready

Reasoning: All P1, P2, and P3 findings from the initial review are resolved. The 12 spec tests now cover the core protocol (5 payload types + multi-byte), boundary conditions (empty field, non-hook type gate), priority semantics (dual-key map), and defensive invariants (caller map immutability). PLAN.md and IMPLEMENTATION.md are internally consistent and accurately reflect the test suite. Summary table cumulative chain is correct. No remaining blockers.