btest: fix bidirectional TCP server tx accounting + gate the client cell on real CHR (JG-31)

[mobileskyfi]

Links

• Linked spec ID: JG-31 (June-gloom closeout plan, Track A steps 1–4)
• Linked work item: docs/MATRIX.md → Peer measurement (btest), btest / client cell

Change type

• Behavior change — btest session TX accounting fix (direction=both TCP server)
• Docs/spec-only change — MATRIX, README, examples, CHANGELOG reconciled
• Tooling/workflow change — integration harness gains an extraPorts host→guest forward option (test-only)
• Security-sensitive change — none (the btest listener security posture is unchanged)

Notes

• Validation run: bun run lint, bun run lint:ci, bun run test (724 pass / 0 fail), bun run build — all green. bun run test:integration for both btest suites — green on real CHR 7.23.1 (server cell + new client cell).
• RouterOS/protocol assumptions: /tool/bandwidth-server enabled + authenticate set via exec; authenticate=yes verifies the client against the router user DB (a dedicated btestuser/btestpass group=full). RouterOS ≥ 6.43 btest auth is EC-SRP5 (same curve as mac-telnet MTWEI). The direct client gate is TCP only over a host→guest tcp:2000 forward; UDP client→server stays loopback/transitive (needs SLIRP reverse-NAT), as does connection-count > 1 fan-out.

---

Closes the last open product-grid cell: btest / client advances coded → CHR-passed, and fixes a bidirectional-TCP server accounting bug found while building the repro. This is Track A (JG-31) of the June-gloom closeout plan, steps 1–4.

What changed

1. Fix — direction=both TCP server tx was never accounted (src/protocols/btest-session.ts)
A both TCP session reported totalTxBytes=0 / txAvgBps=0 on the server side despite transmitting the client's receive half (hundreds of MB): the server's bulk-TX loop (tcpTxLoop with sendStatus=true) flushed only its rx into each interval and never its own tx. The server-both role is the sole sendStatus=true caller, so swapping + recording tx alongside rx fixes the total with no double-count (TX-only senders use sendStatus=false and are accounted by reportOnlyLoop).

RED→GREEN loopback grounding:

• test/unit/btest-session.test.ts — a TCP-both data-engine test (server totalTxBytes > 0).
• test/unit/btest-command.test.ts — an orchestrator-level back-to-back + both-direction anchor (two sequential sessions on one server both account tx+rx; the second is not poisoned by the first's teardown).

Note: the plan also listed a "TCP back-to-back failure" symptom. It does not reproduce in current code (the earlier hang/drain fixes in #50/#52 resolved it), so it's captured as a passing anchor guard rather than a faked RED test.

2. Gate — direct centrs client → CHR /tool/bandwidth-server (test/integration/btest-client.test.ts, new)
The previously-deferred direct client→server gate. The CHR boots with a host→guest tcp:2000 forward (new extraPorts option on test/integration/chr.ts, surfacing as chr.ports.btest); the centrs client dials 127.0.0.1:<host port>. Verified on CHR 7.23.1:

[unauth]   ok=true  rx=621631  auth=none       stop=duration-elapsed
[ec-srp5]  ok=true  rx=420963  auth=ec-srp5    user=btestuser
[bad-pass] ok=false transport/auth-failed

This validates the half the server-cell test cannot: centrs's EC-SRP5 client proof verified by RouterOS's own server verifier. Both btest integration suites were run together — the server cell is unaffected by the both fix.

TCP only — UDP client→server needs the server's datagrams to traverse SLIRP back to the host (reverse-NAT), which this forward does not provide, so it stays loopback/transitive (as does connection-count > 1 fan-out).

3. Docs reconciled — docs/MATRIX.md (btest / client → CHR-passed, priority item 12), commands/btest/{README,examples}.md (grounding caveat now honest about the direct client gate), CHANGELOG.md.

Review feedback (all dispositioned)

• Copilot ×2 (README + test header): extraPorts examples now include proto: "tcp" — fixed in a5d4ed4.
• CodeRabbit ×1 (btest-command.test.ts): explicit expect(server.ok).toBe(true) before the type guard — fixed in a5d4ed4.
• CodeRabbit Description-check: PR body now follows the repo template (Links / Change type / Notes) — this edit.

Deferred (tracked, not in this PR)

• Step 5 — promoting the development channel into the qa push/schedule gate. Depends on Track B's qa active-set / must-pass policy, which isn't built yet (tracked under the closeout plan, Track B).
• UDP client→CHR-server (SLIRP reverse-NAT) and TCP multi-connection fan-out remain loopback/transitive (tracked in commands/btest/README.md → Open questions / Out of scope).

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 47e6fc27-ee5e-4d05-8128-b3051e61f5b8

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

📝 Walkthrough

Walkthrough

Fixes missing server-side TX byte accounting in tcpTxLoop for direction=both sessions by calling counters.swapTx() during the status window. Adds unit regression tests for bidirectional sessions, introduces a CHR integration test for the btest client with a new extraPorts port-forward harness, and updates documentation to reflect btest/client as CHR-passed.

Changes

btest client CHR-passed and TX accounting fix

Layer / File(s)	Summary
tcpTxLoop TX accounting fix and unit tests src/protocols/btest-session.ts, test/unit/btest-session.test.ts, test/unit/btest-command.test.ts	In tcpTxLoop, when sendStatus is true the code now calls counters.swapTx() and records/emits interval metrics as (tx, rx) instead of (0, rx), fixing zero server TX totals for direction=both. Two new unit tests assert server.totalTxBytes > 0 and non-zero txAvgBps across two sequential bidirectional sessions.
CHR port-forward harness and btest client integration test test/integration/chr.ts, test/integration/btest-client.test.ts	Adds ChrPortForward interface and extraPorts to StartOptions for arbitrary host→guest TCP/UDP mappings at CHR boot. The new integration test forwards tcp:2000 to CHR's /tool/bandwidth-server and runs btestClient in unauthenticated, EC-SRP5, and wrong-password modes, asserting authKind, throughput, user identity, and transport/auth-failed error codes; records evidence with example IDs [6, 8]; cleans up via finally.
Documentation updates CHANGELOG.md, docs/MATRIX.md, commands/btest/README.md, commands/btest/examples.md	Updates all docs to reflect btest/client as CHR-passed: MATRIX.md client row gains EC-SRP5/wrong-pass evidence; README validation policy enumerates proven vs. not-yet-gated paths; examples.md gating narrative covers examples 6 and 8; CHANGELOG records both the CHR-passed milestone and the TX accounting fix.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• tikoci/centrs#12: Introduced the TCP TX/status machinery and session-state orchestration in src/protocols/btest-session.ts that this PR's tcpTxLoop TX accounting fix builds directly on.

Poem

🐇 Hop hop, the bytes now flow both ways,
TX was hiding in a loopback haze!
A CHR boots up, port two-thousand in sight,
EC-SRP5 shakes hands — the proof is right.
Wrong password? Rejected with flair!
The rabbit counts packets with meticulous care. 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the two main changes: a bidirectional TCP server accounting fix and advancing the btest client cell to CHR-passed status, with the issue reference (JG-31).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description follows the repository template structure with all required sections: Links, Change type, and Notes. It provides clear details about the fix, testing, and validation.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg-31-btest-client-chr

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[Copilot]

Pull request overview

Fixes btest server-side accounting for bidirectional TCP sessions and adds CHR-backed integration coverage that directly validates centrs’s btest client against RouterOS /tool/bandwidth-server, advancing the last open btest / client product-grid cell to CHR-passed.

Changes:

• Fix direction=both TCP server TX accounting by recording swapped TX bytes during the server’s status-emitting TX loop.
• Add unit coverage for TCP both accounting (including back-to-back sessions) and add a new CHR integration test for centrs client → CHR bandwidth-server over host→guest port forwarding.
• Update MATRIX/docs/changelog to reflect the btest / client cell as CHR-passed and reconcile grounding notes.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/protocols/btest-session.ts	Records TX bytes for server-side TCP both sessions during status intervals so totals/averages are non-zero.
test/unit/btest-session.test.ts	Adds loopback regression test ensuring both sides record TX+RX for TCP both.
test/unit/btest-command.test.ts	Adds orchestrator-level regression test for back-to-back TCP both sessions with correct accounting.
test/integration/chr.ts	Extends CHR harness options with extraPorts port-forward mappings.
test/integration/btest-client.test.ts	New CHR integration suite: centrs btest client → real RouterOS bandwidth-server (unauth + EC-SRP5 + wrong-pass reject).
docs/MATRIX.md	Advances btest / client to CHR-passed and updates grounding narrative.
commands/btest/README.md	Reconciles status/grounding documentation and references the new client CHR gate.
commands/btest/examples.md	Notes that examples 6 and 8 are now also asserted directly against CHR bandwidth-server.
CHANGELOG.md	Documents the new CHR gate and the TCP both server TX accounting fix.

[mobileskyfi]

@coderabbitai please review now that limit is reset

[coderabbitai]

@mobileskyfi Sure! Kicking off the review now.

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/unit/btest-command.test.ts`:
- Around line 180-183: The test currently includes a guard statement that
returns early if server.ok is false, but there is no explicit assertion for
server.ok being true. This allows the test to pass silently even when server
orchestration fails. Add an explicit expect assertion for server.ok to be true
(similar to the existing assertions for first.ok and second.ok on lines 180-181)
before or alongside these assertions, so that the test fails rather than
silently passes when server initialization fails.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0fca93ae-ce13-4730-85f0-e0de8cd57618

📥 Commits

Reviewing files that changed from the base of the PR and between fce2e7f and e1fece5.

📒 Files selected for processing (9)

• CHANGELOG.md
• commands/btest/README.md
• commands/btest/examples.md
• docs/MATRIX.md
• src/protocols/btest-session.ts
• test/integration/btest-client.test.ts
• test/integration/chr.ts
• test/unit/btest-command.test.ts
• test/unit/btest-session.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Assert server success explicitly to avoid false-positive test passes.

Line 182 currently returns when server.ok is false, so the test can pass even if server orchestration failed. Add an explicit assertion for server.ok before the type guard.

Suggested fix

 		expect(first.ok).toBe(true);
 		expect(second.ok).toBe(true);
-		if (!first.ok || !second.ok || !server.ok) return;
+		expect(server.ok).toBe(true);
+		if (!first.ok || !second.ok || !server.ok) return;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	expect(first.ok).toBe(true);
	expect(second.ok).toBe(true);
	if (!first.ok || !second.ok || !server.ok) return;
	expect(first.data.totalTxBytes).toBeGreaterThan(0);
	expect(first.ok).toBe(true);
	expect(second.ok).toBe(true);
	expect(server.ok).toBe(true);
	if (!first.ok || !second.ok || !server.ok) return;
	expect(first.data.totalTxBytes).toBeGreaterThan(0);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/unit/btest-command.test.ts` around lines 180 - 183, The test currently
includes a guard statement that returns early if server.ok is false, but there
is no explicit assertion for server.ok being true. This allows the test to pass
silently even when server orchestration fails. Add an explicit expect assertion
for server.ok to be true (similar to the existing assertions for first.ok and
second.ok on lines 180-181) before or alongside these assertions, so that the
test fails rather than silently passes when server initialization fails.

[mobileskyfi]

Addressed the review feedback in a5d4ed4:

• Copilot (README + btest-client.test.ts header): both extraPorts forward examples now include proto: "tcp", matching the ChrPortForward shape and the actual call.
• CodeRabbit (btest-command.test.ts): added an explicit expect(server.ok).toBe(true) before the type guard so the back-to-back test fails rather than silently passing if server orchestration fails.

All doc-comment/assertion only — no transport code touched, so the CHR-green evidence stands. lint / lint:ci / unit tests pass.

[mobileskyfi]

Full review-item disposition

Every flagged item now has a disposition — nothing dropped:

Item	Source	Disposition
extraPorts example missing proto (README)	Copilot	Fixed (a5d4ed4)
extraPorts example missing proto (test header)	Copilot	Fixed (a5d4ed4)
Assert server.ok explicitly in back-to-back test	CodeRabbit	Fixed (a5d4ed4)
PR body omits template (Links/Change type/Notes)	CodeRabbit Description-check	Fixed (body updated)
Unit tests (windows-latest) red	CI check	Tracked → #69 (see below)

On the Windows CI check

While reviewing, the Unit tests (windows-latest) job is red. I root-caused it rather than wave it off:

• It is pre-existing on main (already failure on base fce2e7fa) and non-gating by design — ci.yaml sets continue-on-error for windows-latest ("informational until centrs is verified Windows-clean"). The overall CI conclusion is success, and main has no required checks, so btest: fix bidirectional TCP server tx accounting + gate the client cell on real CHR (JG-31) #68 is not blocked.
• Root cause is environmental: bind ENOTSUP 127.0.0.1 (UDP loopback unsupported on some Windows runners). It fails the pre-existing MNDP loopback tests and error-pages.test.ts; this PR's UDP both loopback test joins that set for the same reason — not a regression.
• Tracked in Windows unit tier: UDP-loopback tests fail with bind ENOTSUP 127.0.0.1 (centrs not Windows-clean) #69 with concrete fixes (skip UDP-loopback unit tests on ENOTSUP, since the CHR integration tests already cover that path).

One correction

I initially added the final-interval flush (00cab18) thinking it fixed that Windows flake; it does not (the cause is bind ENOTSUP, not tail accounting). I kept the change on its own merits — it's a real fix for under-reporting the final partial interval, CHR-verified green — and corrected the commit message + CHANGELOG to say so honestly.