test(protocols): native-api value escaping round-trip + ssh no-PTY guard (JG-09/JG-15) by mobileskyfi · Pull Request #48 · tikoci/centrs

mobileskyfi · 2026-06-16T16:30:43Z

What

Stage-1 protocol-compliance work (JG-09), absorbing the JG-15 escaping
round-trip the fan-out flagged as the one genuinely untested transport behavior.
Turns local protocol-memory facts into GitHub-visible, reviewable assertions.

JG-15 — native-api attribute value round-trip

The RouterOS API protocol is length-prefix framed, not delimiter-escaped: a
value is a count of raw bytes, so =, spaces, CR/LF, NUL, and multibyte UTF-8
all survive verbatim with no escaping, and parseReply splits =name=value on
the second = so a value that itself contains = is read back whole. The
codec had framing tests but no end-to-end test for special-char values. New
describe("… (JG-15)") pins the full path
attributeWord → encodeSentence → SentenceReader → parseReply across 11 tricky
values + the value-less (=name= / =name) case.

execute/ssh no-PTY guard

SshExecClient.argv must never request a tty (-t, -tt, -T, RequestTTY)
— RouterOS grants no PTY and -tt hangs the session (CHR 7.23.1 grounded).
The terminal relay already guards this (terminal.test.ts); the per-command
execute client did not.

Docs

attributeWord doc comment now records the no-escaping contract the test pins.

Why no CHR

Test-only + one doc comment. The native-api codec is unchanged and the asserted
cells are already CHR-passed; this is additive regression coverage, so the
MATRIX is untouched.

Verification

bun run lint + bun run lint:ci + bun run test (701 pass / 0 fail / 26 skip)

bun run build all green.

🤖 Generated with Claude Code

Summary by CodeRabbit

Tests
- Added test coverage for native API attribute value round-trip preservation through the codec pipeline.
- Added test coverage for SSH command execution behavior validation.
Documentation
- Improved documentation for native API attribute handling behavior.

…Y (JG-09/JG-15) Promote the protocol-compliance facts the fan-out flagged as untested into GitHub-visible assertions, independent of any command/feature: - JG-15: native-api attribute *value* round-trip — the API protocol is length-prefix framed, not delimiter-escaped, so `=`, spaces, CR/LF, NUL, and multibyte UTF-8 survive verbatim, and parseReply splits `=name=value` on the second `=` so a value containing `=` is read back whole. Pins the full attributeWord → encodeSentence → SentenceReader → parseReply path; the codec was tested for framing but never for special-char values end-to-end. - execute/ssh no-PTY guard: SshExecClient.argv must never request a tty (`-t`, `-tt`, `-T`, RequestTTY) — RouterOS grants no PTY and `-tt` hangs it. The terminal relay already guards this (terminal.test.ts); execute did not. - attributeWord doc comment records the no-escaping contract the test pins. Test-only + doc comment; native-api codec unchanged; cells stay CHR-passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai · 2026-06-16T16:30:56Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 04d7c5f4-f225-4b9d-baed-ce76a98ccb04

📥 Commits

Reviewing files that changed from the base of the PR and between 70c0341 and 9a49c0a.

📒 Files selected for processing (3)

src/protocols/native-api.ts
test/unit/native-api.test.ts
test/unit/ssh.test.ts

📝 Walkthrough

Walkthrough

The PR expands the attributeWord JSDoc in src/protocols/native-api.ts to document the =name=value wire-format, value escaping, and parsing behavior. It adds a Bun test suite (native-api.test.ts) that round-trips attribute values through the full encode/decode pipeline for edge cases, and adds a unit test (ssh.test.ts) asserting SshExecClient never forces PTY allocation.

Changes

native-api attributeWord Documentation and Round-Trip Tests

Layer / File(s)	Summary
`attributeWord` JSDoc expansion and value round-trip tests `src/protocols/native-api.ts`, `test/unit/native-api.test.ts`	Replaces the brief `attributeWord` comment with a detailed description of the `=name=value` wire framing, no-escaping guarantee, and second-`=` split parsing. Backs this with the JG-15 test suite that encodes a `!re` sentence through `attributeWord → encodeSentence → SentenceReader → parseReply → readAttribute` and asserts exact value preservation for embedded `=`, expressions, spaces, CR/LF, tab/control bytes, NUL, multibyte UTF-8, multi-byte length-prefixed long values, whitespace, and empty/value-less attributes.

SSH No-PTY Assertion

Layer / File(s)	Summary
`SshExecClient` no-PTY unit test `test/unit/ssh.test.ts`	Adds a test verifying that the constructed SSH argv contains no `-t` or `-tt` flags and that `RequestTTY` does not carry `force` or `yes` values.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

tikoci/centrs#16: Both PRs touch SshExecClient command construction—the retrieved PR implements the SSH execute client and its argv/options handling, which is directly exercised by the new no-PTY test added here.

Poem

🐇 No -t for me, no PTY snare,
The attribute words, I've framed with care,
=name=value splits right on cue,
Round-trip tests pass—every byte came through,
Hop hop, the docs now explain the wire!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: testing native-api value escaping round-trip and SSH no-PTY guard, with reference to the issue IDs.
Description check	✅ Passed	The description is comprehensive and covers the main changes (JG-15 round-trip testing, SSH no-PTY guard, documentation updates) and verification, but the PR description template sections (Links, Change type, Notes) are not filled out.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

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

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/protocol-compliance-jg09

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

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

Copilot

Pull request overview

Adds regression coverage for two protocol-compliance behaviors: native RouterOS API attribute value round-trip (including tricky characters) and ensuring SSH execute never forces a PTY (which can hang RouterOS).

Changes:

Added a unit test asserting SshExecClient.argv does not request a PTY.
Added an end-to-end unit test covering native-api attribute value round-trips across encode/decode/parse.
Expanded attributeWord documentation to describe the “no escaping” contract.

Reviewed changes

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

File	Description
`test/unit/ssh.test.ts`	Adds a regression test around SSH execute argv PTY behavior.
`test/unit/native-api.test.ts`	Adds end-to-end round-trip tests for native-api attribute values with special characters.
`src/protocols/native-api.ts`	Documents the attribute value no-escaping behavior and ties it to the regression test.

+		const argv = client({ exitCode: 0, stdout: "", stderr: "" }).argv("/x");
+		expect(argv).not.toContain("-t");
+		expect(argv).not.toContain("-tt");
+		expect(argv).not.toContain("-T");
+		expect(argv.join(" ")).not.toContain("RequestTTY");


+	function roundTrip(name: string, value: string): string | undefined {
+		const words = ["!re", attributeWord(name, value)];
+		const reader = new SentenceReader();
+		const sentences = reader.push(encodeSentence(words));
+		expect(sentences).toHaveLength(1);
+		return readAttribute(parseReply(sentences[0] as string[]), name);
+	}


+ * The value needs **no escaping**: words are length-prefix framed on the wire
+ * ({@link encodeWord}), so any bytes — `=`, spaces, CR/LF, NUL, multibyte UTF-8 —
+ * pass through verbatim, and {@link parseReply} splits on the *second* `=` so a
+ * value that contains `=` is read back whole. Round-trip pinned in


…casts, UTF-8 wording - ssh no-PTY guard: only forbid *forcing* a PTY (`-t`/`-tt`/`RequestTTY=force|yes`); `-T` and `RequestTTY=no` *disable* a PTY and are valid hardening, so allow them. - native-api round-trip: destructure + guard instead of `as string[]` casts, so a decode regression to `undefined` fails loudly rather than being masked. - attributeWord doc + JG-15 test comment: "any UTF-8 content" (encodeWord UTF-8 encodes a JS string by byte length), not "any bytes" — avoids over-promising raw-binary safety while still covering `=`, spaces, CR/LF, NUL, multibyte. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings June 16, 2026 16:30

Copilot started reviewing on behalf of mobileskyfi June 16, 2026 16:31 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

mobileskyfi merged commit d0f60f0 into main Jun 16, 2026
9 checks passed

mobileskyfi deleted the feat/protocol-compliance-jg09 branch June 16, 2026 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(protocols): native-api value escaping round-trip + ssh no-PTY guard (JG-09/JG-15)#48

test(protocols): native-api value escaping round-trip + ssh no-PTY guard (JG-09/JG-15)#48
mobileskyfi merged 2 commits into
mainfrom
feat/protocol-compliance-jg09

mobileskyfi commented Jun 16, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 16, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mobileskyfi commented Jun 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

JG-15 — native-api attribute value round-trip

execute/ssh no-PTY guard

Docs

Why no CHR

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mobileskyfi commented Jun 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 16, 2026 •

edited

Loading