fix: ground string header tab claim and clarify chr wording contracts (#276, #304) - #307
fix: ground string header tab claim and clarify chr wording contracts (#276, #304)#307mobileskyfi wants to merge 1 commit into
Conversation
…#276, #304) - src/core/routeros-string.ts: replace ungrounded 'expands each tab to four spaces while reading a string literal' with defensive wording that escaping preserves the caller's bytes. Cite RouterOS 7.20 changelog nuance: version-gated, width-configurable via tab-width, scoped to script editor not proven as string-literal expansion. - test/integration/chr.ts: split accepted-set grouping into ACCEPTED SET vs WORDING CONTRACT vs VERSION BOUNDARY per test/AGENTS.md and #304. Keeps escaping correct, no behavior change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used📓 Path-based instructions (4)**1. Declare the split in `test/integration/chr.ts`, never inline.**📄 CodeRabbit inference engine (test/AGENTS.md) Files:
Transport or RouterOS-touching code is not done until `bun run test:integration` passes.📄 CodeRabbit inference engine (AGENTS.md) Files:
- Put long-running, RouterOS-backed, or platform-specific tests under `test/integration/` and wire them through QA or lab workflows. "Platform-specific" includes process-level tests that spawn the real `src/cli.ts` through `cli-process.ts` ...📄 CodeRabbit inference engine (test/AGENTS.md) Files:
Use Bun-native TypeScript and Web APIs where possible📄 CodeRabbit inference engine (src/AGENTS.md) Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThe pull request updates two documentation comments. It clarifies the RouterOS string escaping rationale and distinguishes accepted sets, wording contracts, and version boundaries in device-behaviour tests. ChangesRouterOS string escaping documentation
Device-behaviour contract terminology
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This PR clarifies string-escaping documentation and separates integration-test contract descriptions without changing runtime behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains both changes, links them to issues Full details: Linked Issues checkExplanation The changes satisfy Full details: Out of Scope Changes checkExplanation The pull request changes only documentation comments in src/core/routeros-string.ts and test/integration/chr.ts. Both changes directly support linked issues ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses two documentation/wording nits to better ground RouterOS behavior claims and clarify the integration test “device-behaviour contracts” taxonomy.
Changes:
- Updates
routerOsStringLiteralheader comment to remove an ungrounded “tabs expand to four spaces” claim and replace it with version-gated, defensive wording referencing the 7.20 changelog nuance. - Refines
test/integration/chr.tscommentary to separate “accepted set”, “wording contract”, and “version boundary” concepts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/core/routeros-string.ts |
Rewords the header comment to avoid asserting unproven device behavior while keeping the rationale for escaping intact. |
test/integration/chr.ts |
Clarifies the taxonomy of device-dependent expectation patterns used in CHR-backed integration tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -117,11 +117,9 @@ export function exampleIds(count: number): number[] { | |||
| // answer at a call site; see `test/AGENTS.md` → "Device-dependent expectations". | |||
| // | |||
| // Two shapes, and they are not interchangeable: | |||
Closes #276, closes #304
routeros-string: the "tabs expand to four spaces" header claim is ungrounded, and the 7.20 changelog says something narrower and version-gated #276 — src/core/routeros-string.ts:7: Replace ungrounded 'RouterOS expands each tab to four spaces while reading a string literal' with defensive wording that escaping preserves the caller's bytes. Cite RouterOS 7.20 changelog nuance: version-gated (absent before 7.20), width-configurable via
tab-widthin/console/settings, and scoped to the script editor not proven as string-literal expansion. No CHR available in this environment, so per issue fallback: state only that escaping preserves bytes and cite changelog nuance. Escaping itself unchanged and verified.explain/tokens: clean up chr.ts wording — accepted-set vs wording-contract (#297 follow-up) #304 — test/integration/chr.ts:119-124: Split the 'ACCEPTED SET' grouping into three distinct shapes per
test/AGENTS.md:VALIDATION_REJECT_CODESPARSE_REJECTED,PARSE_REJECTED_HINT*_SINCEviarouterOsAtLeast(...)Applied proposed comment verbatim from explain/tokens: clean up chr.ts wording — accepted-set vs wording-contract (#297 follow-up) #304.
Verification:
bun run lint✔,bun run test✔ (40 integration skipped without CHR),bun run build✔,bun run lint:ci✔Single PR for both nits as requested.
Summary by CodeRabbit