docs(test): version-gate device expectations, and prove them on long-term (#297) - #301
Conversation
…term (#297) Records the decision #297 was waiting on, and writes it where a cold agent reads it before writing an integration test rather than only in the issue. Adopted: convention (option 1) plus a LOCAL, scoped long-term run in the done definition. Not a pre-merge CI leg — the cost estimate in the issue measures the wrong run. A full leg is ~18 minutes (measured: stable 17m, long-term 18m, testing 18m, development 16m), but what finds this drift is the TOUCHED file against the OLDEST device, and that is ~22s for one file, 73-120s for two: CENTRS_CHR_CHANNEL=long-term CENTRS_RUN_FAST_INTEGRATION=1 \ bun test test/integration/<touched>.test.ts Verified that command resolves long-term (-> 7.21.5) and passes, rather than documenting an unrun recipe. Every version-drift instance in #297 would have been caught by it before the PR was opened. Three surfaces, one canonical statement plus pointers: - test/AGENTS.md (test/CLAUDE.md symlinks to it, so both harnesses get it) — the rule, with a table mapping each existing contract to when it fits, and the two standing traps: gate on the running VERSION not the channel name, and never relax an assertion to make a version pass. - .github/instructions/done-definition.instructions.md — the long-term run as step 3 of the existing procedure, pointing at the rule. - test/integration/chr.ts — a section header over the contracts naming the two shapes (accepted set vs version boundary) and why they are not interchangeable, so the surface is discoverable from the code. Scope stated explicitly: this prevents VERSION DRIFT. A harness that mishandles normal teardown (#300's unhandled socket error when the relay peer exits) is a different failure that only shows up by running the thing, and saying so keeps the next reader from reading this as flake prevention too. The issue's "used exactly once" premise is already stale: routerOsAtLeast now has three integration call sites and VALIDATION_REJECT_CODES five, with PARSE_REJECTED/PARSE_REJECTED_HINT added in the same shape. The mechanism gets reached for once someone knows it exists — discovery was the gap. Fixes #297 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (5)test/**/AGENTS.md📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
**/AGENTS.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
{docs/**,.github/instructions/**,.github/**/*.yml,.github/**/*.yaml,**/*.{md,txt,dict}}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
.github/instructions/*.instructions.md📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe PR updates integration-test guidance. Device-output tests must run against the ChangesRouterOS integration guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized documentation change clarifies version-gated device expectations and the long-term test procedure; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 #297 by documenting (in the places a “cold” contributor/agent will read first) how to express RouterOS version-dependent integration expectations, and by adding a lightweight local verification step to catch long-term-only behavior splits before a PR is opened.
Changes:
- Adds a “device-behaviour contracts” header in
test/integration/chr.tsto make version-split expectation patterns discoverable at the declaration site. - Documents the “declare the split in
chr.ts” convention and provides guidance on choosing the right contract intest/AGENTS.md. - Updates the done-definition workflow to include a scoped
long-termrun of the touched integration test file when assertions read device output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/integration/chr.ts |
Adds a prominent section header framing where/how RouterOS version-split contracts should be declared. |
test/AGENTS.md |
Documents conventions for version-dependent assertions and when to run a local long-term CHR check. |
.github/instructions/done-definition.instructions.md |
Extends the “done definition” checklist with a scoped long-term run step for device-output assertions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Fixes #297.
Records the decision #297 was waiting on and writes it where a cold agent reads it before writing an integration test — its second acceptance criterion — rather than leaving it in an issue.
Adopted: convention (option 1) + a local, scoped long-term run in the done definition. Not a pre-merge CI leg.
Why not the CI gate
The cost estimate in the issue measures the wrong run. A full leg really is ~18 minutes — measured on the failing run: stable 17m, long-term 18m, testing 18m, development 16m. But what finds this drift is the touched file against the oldest device, and that is not expensive:
mcp.test.ts) on long-termmcp+execute)explain-values+explain-string-escape)So this doesn't need to be a gate at all. Every version-drift instance in #297 would have been caught by a ~20-second local run before the PR was opened.
I ran the documented command rather than documenting an unrun recipe — it resolves
long-term→ 7.21.5 and passes:Three surfaces, one canonical statement plus pointers
test/AGENTS.md— the rule.test/CLAUDE.mdis a symlink to it, so Claude and Copilot both pick it up from one edit. Includes a table mapping each existing contract to when it fits, and the two standing traps: gate on the running version, never the channel name (channels move); and never relax an assertion to make a version pass — explain-values integration tests hard-code ≥7.23 device behaviour (blocks the long-term gate) #296 kept a byte-exact offset on every version, just not the same byte..github/instructions/done-definition.instructions.md— the long-term run as step 3 of the existing procedure, pointing at the rule rather than restating it.test/integration/chr.ts— a section header over the contracts, naming the two shapes and why they are not interchangeable: an accepted set when several answers are all correct, a version boundary when the behaviour genuinely changed and the test must still assert exactly on each side.Scope, stated rather than implied
This prevents version drift. A harness that mishandles normal teardown — #300's unhandled socket error when the relay peer exits — is a different failure that only shows up by running the thing, and would not have been caught by any of this. Saying so in the doc keeps the next reader from treating it as flake prevention too.
One premise in the issue is already stale
"Used exactly once" no longer holds:
routerOsAtLeast()now has three integration call sites (transfer.test.ts:242,explain-values.test.ts:112and:560),VALIDATION_REJECT_CODEShas five assertion sites across four files, andPARSE_REJECTED/PARSE_REJECTED_HINTwere added in the same shape for a wording split where the obvious/error/matcher silently accepted nothing.The mechanism gets reached for once someone knows it exists. Discovery was the gap, which is why the fix is three pointers and no new machinery.
bun run lint:ci,bun run test(2854) andbun run buildall clean.Summary by CodeRabbit
Documentation
Tests