test(explain): cover catalog drift reporting - #284
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR extracts catalog drift parsing and reporting into a reusable module, integrates it with catalog generation and CI summaries, adds regression tests, updates the QA fallback message, and corrects unknown-attribute validation documentation. ChangesCatalog drift reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CatalogGenerator
participant DriftReporter
participant GitHubSummary
participant QAFallback
CatalogGenerator->>DriftReporter: Parse committed catalog
CatalogGenerator->>DriftReporter: Build report from committed and fresh rows
DriftReporter->>GitHubSummary: Append structured report
QAFallback->>GitHubSummary: Check for report sentinel
QAFallback-->>CatalogGenerator: Avoid duplicate fallback report
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 extracts explain-catalog drift parsing/rendering and GitHub Actions summary publication into a dedicated module so it can be unit-tested directly, and then hardens the drift report and related documentation.
Changes:
- Introduces
scripts/explain-catalog-drift.tswithparseCommittedCatalog,buildDriftReport, and best-effortappendDriftReportToSummary. - Adds comprehensive unit coverage for drift parsing/report formatting and Actions summary behavior (including LF/CRLF and malformed/duplicate fallback).
- Updates the
validation/unknown-attributeerror details page to match the actual context shapes emitted by retrieve, inspect-gated validation, and console:parse, and tweaks QA fallback wording.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
test/unit/explain-catalog-drift.test.ts |
Adds regression coverage for committed-catalog parsing, drift report rendering, sentinel behavior, and Actions summary append semantics. |
scripts/gen-explain-catalog.ts |
Replaces embedded drift/report logic with calls into the new drift module. |
scripts/explain-catalog-drift.ts |
New extracted module implementing committed-catalog parsing, drift report generation, and best-effort summary publication. |
docs/errors/validation/unknown-attribute.md |
Corrects/clarifies documented context keys and version-dependent behavior for validation/unknown-attribute. |
.github/workflows/qa.yaml |
Aligns the fallback message wording when no structured drift report is present and keeps sentinel dedupe behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/errors/validation/unknown-attribute.md`:
- Around line 37-40: Update the validation guidance so `/console/inspect`
applies only to retrieve and inspect-gated API/execute checks; add separate
:parse guidance using context.parameter with the RouterOS path parameters, and
direct readers to console errors’ command and detail fields instead of inspect
attributes.
In `@scripts/explain-catalog-drift.ts`:
- Around line 22-44: Update the row validation around the CatalogRow parsing to
reject any input where parts.length exceeds 6, returning the existing null
sentinel before constructing the row. Preserve parsing of the six supported
columns and add a regression case covering a seventh column.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: e82ab363-f833-4db5-9e84-02f187cd5603
📒 Files selected for processing (5)
.github/workflows/qa.yamldocs/errors/validation/unknown-attribute.mdscripts/explain-catalog-drift.tsscripts/gen-explain-catalog.tstest/unit/explain-catalog-drift.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: CHR smoke (stable)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (2)
{docs/**,.github/instructions/**,.github/**/*.yml,.github/**/*.yaml,**/*.{md,txt,dict}}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun run lint:ciwhen changing documentation, instructions, security configuration, spelling dictionaries, or workflow files.
Files:
.github/workflows/qa.yamldocs/errors/validation/unknown-attribute.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Transport or RouterOS-touching code is not done untilbun run test:integrationpasses.
Do not silently fall back to another protocol when the caller pinned--via.
Do not make generated output the hand-edited source of truth.
Do not disable validation to make a test pass; validation is part of the product.
Files:
test/unit/explain-catalog-drift.test.tsscripts/gen-explain-catalog.tsscripts/explain-catalog-drift.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 224
File: commands/explain/examples.md:11-16
Timestamp: 2026-08-06T14:17:50.227Z
Learning: For the phase-1 offline explain API, offline behavior is tested through unit and fixture tests rather than CHR-backed integration tests. In `test/unit/explain-envelope.test.ts`, the CLI-spelling gate-versus-analysis behavior described by `commands/explain/examples.md` example `1b` and the resolved-menu behavior described by example `18b` have focused unit coverage. The CLI `explain` command surface is deferred and is not implemented as `src/cli/explain.ts` in this PR.
📚 Learning: 2026-08-11T03:49:52.355Z
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 265
File: .github/workflows/ci.yaml:171-172
Timestamp: 2026-08-11T03:49:52.355Z
Learning: In this repository's GitHub Actions workflows, SHA-pin `oven-sh/setup-bun` and `actions/setup-node` references and include version comments. Other first-party `actions/*` references may continue using floating major-version tags according to the repository's current convention. Do not request SHA pinning for those first-party actions unless a repository-wide maintainer policy and corresponding Dependabot update policy have been established.
Applied to files:
.github/workflows/qa.yaml
📚 Learning: 2026-08-06T14:17:50.227Z
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 224
File: commands/explain/examples.md:11-16
Timestamp: 2026-08-06T14:17:50.227Z
Learning: For the phase-1 offline explain API, offline behavior is tested through unit and fixture tests rather than CHR-backed integration tests. In `test/unit/explain-envelope.test.ts`, the CLI-spelling gate-versus-analysis behavior described by `commands/explain/examples.md` example `1b` and the resolved-menu behavior described by example `18b` have focused unit coverage. The CLI `explain` command surface is deferred and is not implemented as `src/cli/explain.ts` in this PR.
Applied to files:
test/unit/explain-catalog-drift.test.tsscripts/gen-explain-catalog.ts
📚 Learning: 2026-08-05T03:19:54.280Z
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 209
File: test/unit/explain-menus.test.ts:146-152
Timestamp: 2026-08-05T03:19:54.280Z
Learning: In `test/unit/explain-write.test.ts`, keep separate test anchors for `isDanglingBarePath` and menu-table rejection. Known menu paths such as `/ip/firewall/filter` and `/log` reach `isDanglingBarePath` at document end, while `/system/reboot` is rejected earlier by `isConfirmedNav` because `isMenuPath` returns false. Do not combine these inputs in one test when asserting coverage of either mechanism.
Applied to files:
test/unit/explain-catalog-drift.test.tsscripts/explain-catalog-drift.ts
📚 Learning: 2026-08-05T20:06:21.299Z
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 217
File: src/explain/symbols.ts:0-0
Timestamp: 2026-08-05T20:06:21.299Z
Learning: In `src/explain/symbols.ts`, comparative corpus measurements that explain a semantic modeling trade-off are rule provenance and follow the established F1/F2 documentation convention. Treat run-specific bookkeeping, fuzz counts, scratch-script paths, method pointers, and per-version missed-tail details as status data that should not remain in source comments.
Applied to files:
test/unit/explain-catalog-drift.test.tsscripts/gen-explain-catalog.tsscripts/explain-catalog-drift.ts
📚 Learning: 2026-08-06T05:38:01.223Z
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 222
File: src/explain/defects.ts:1-70
Timestamp: 2026-08-06T05:38:01.223Z
Learning: In the TypeScript `src/explain` modules, references to phase-0 lab questions and issue numbers are an established documentation idiom when they explain the technical basis for a current contract. Keep these rationale references when they support implementation behavior. Remove dated decisions, delivery status, and future roadmap text from implementation and test comments.
Applied to files:
test/unit/explain-catalog-drift.test.tsscripts/gen-explain-catalog.ts
📚 Learning: 2026-06-16T10:51:07.659Z
Learnt from: CR
Repo: tikoci/centrs PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-06-16T10:51:07.659Z
Learning: Unit tests are anchor tests for local behavior and exported contracts.
Applied to files:
test/unit/explain-catalog-drift.test.ts
📚 Learning: 2026-08-06T21:57:06.372Z
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 226
File: test/integration/cli-smoke.test.ts:333-347
Timestamp: 2026-08-06T21:57:06.372Z
Learning: For the `centrs explain` CLI, ambient stdin is read only when no positional argument can provide the offline input. When a positional input is present and stdin is supplied, the explain result includes the `usage/stdin-ignored` warning. In `test/integration/cli-smoke.test.ts`, no-input coverage must test both child stdin shapes: no `stdin` option (`/dev/null`) and `stdin: ""` (an empty pipe).
Applied to files:
test/unit/explain-catalog-drift.test.ts
📚 Learning: 2026-08-11T17:46:27.539Z
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 273
File: test/fixtures/explain/corpus-partition.json:4-4
Timestamp: 2026-08-11T17:46:27.539Z
Learning: For durable fixtures in this repository, do not cite in-flight `.scratch/` files as provenance. In `test/fixtures/explain/corpus-partition.json`, the committed groups are the durable artifact, and the `frozen` no-regeneration statement replaces the former `.scratch/explain-lab-partition.ts` generator citation.
Applied to files:
test/unit/explain-catalog-drift.test.ts
🔇 Additional comments (5)
docs/errors/validation/unknown-attribute.md (3)
3-4: LGTM!Also applies to: 6-17, 19-25
18-18: 🗄️ Data Integrity & IntegrationVerify the
validationSourcecontext key.The supplied integration snippets use
meta.validation.source, while this table documentscontext.validationSource. Confirm thatsrc/api.ts,src/execute.ts, and console:parseemiterror.context.validationSource. If they emit onlymeta.validation.source, document the actual field instead.
3-40: 📐 Maintainability & Code QualityRun the required documentation lint.
Because this file is under
docs/**, runbun run lint:cibefore merge.As per coding guidelines, documentation changes require
bun run lint:ci.Source: Coding guidelines
scripts/explain-catalog-drift.ts (1)
12-14: 🎯 Functional CorrectnessRun required integration validation.
These TypeScript changes parse and publish the RouterOS CLI Reference catalog. Run
bun run test:integrationand provide a passing result before merge.
scripts/explain-catalog-drift.ts#L12-L14: include committed-catalog parsing and drift rendering in the passing integration result.scripts/gen-explain-catalog.ts#L37-L41: include shared drift-helper use in the passing integration result.scripts/gen-explain-catalog.ts#L177-L177: include GitHub Actions summary publication flow in the passing integration result.As per coding guidelines,
Transport or RouterOS-touching code is not done until bun run test:integration passes.Source: Coding guidelines
.github/workflows/qa.yaml (1)
168-168: 📐 Maintainability & Code QualityRun CI lint for this workflow update.
Run
bun run lint:cibefore merge. No provided result verifies this workflow change passes required linting.As per coding guidelines,
Run bun run lint:ci when changing documentation, instructions, security configuration, spelling dictionaries, or workflow files.Source: Coding guidelines
mobileskyfi
left a comment
mobileskyfi
left a comment
There was a problem hiding this comment.
muse (via mobileskyfi) — independent double-check pass on #281 follow-up (PR #284):
Checked:
- Checked out
agent/281-catalog-drift-review-followup@956034d, ranbun run lint(clean),bun run test(2626 pass, 40 skip, 5/5 drift tests green),bun run build(316 modules, 67ms),bun run lint:cifrom clean worktree (passes; local.agents/skills7 markdownlint errors are pre-existing, noted in PR description). - Probed
parseCommittedCatalogLF vs CRLF (both 2 rows), 7-column rejection (parts.length>6→ null, sentinel fallback), duplicate detection (out.has(path)→ null), blank-line table separation (lines[kindIdx-1]===""), sentinel at top, andappendDriftReportToSummaryappend-without-truncation (earlier step\npreserved). - Verified
buildDriftReportacceptance criteria: row counts, published add/remove withcap=40,both→publishedemphasis, gate/kind changes, sentinel fallback whencommitted===null, and QA workflowgrep -qF "<!-- explain-catalog-drift -->"dedup. - Compared
docs/errors/validation/unknown-attribute.mdtable against actual emits:src/retrieve.ts:233(parameter/requestedAttributes/availableAttributes/path),src/api.ts:988,1033+src/execute.ts:742(add validationSource/verb),src/protocols/mac-telnet-console.ts:674(command/detail/position) — doc now correctly scopes each key and notesretrievedoes not emitvalidationSourceand older RouterOS may surfacevalidation/syntax.
No blocking issues. Two tiny nits for next polish (out-of-scope per #281, but would silence future copilot nits):
scripts/explain-catalog-drift.ts:22—parts.length>6correctly rejects 7th column, but an empty path"|menu|both"would still create a row withpath=""(falsy not undefined). Considerif (!path) return nullif you want to treat empty path as malformed — current 1,119 rows have no empty path, so not urgent..github/workflows/qa.yaml:165fallback now says “No structured drift report was produced.” — matches summary-based design (no file); matches generator’sconsole.error(report)as source of truth. Good.
Also: cleaned lingering git rebase --continue (PIDs 21300/21298) left from earlier rebase onto 6fc34d2 — git status now clean on main.
LGTM — hardening is complete and review-surface is now directly testable. Post-merge separates (#282 CRLF fixture, #283 version-dependent, rosetta#137) remain correctly out-of-scope.
Summary
validation/unknown-attributedetails page against the actual retrieve, inspect-gated, and console-only context shapesReview context
This is the independent-review follow-up to #279 and #280. It hardens their reporting/docs changes without reopening #237 or #275.
The distinct post-merge failures remain separate:
The current live catalog refresh still aborts loudly on the first stale alias, as designed; this PR does not guess at the upstream migration.
Validation
bun test test/unit/explain-catalog.test.ts test/unit/explain-catalog-drift.test.ts test/unit/retrieve.test.tsbun run lintbun run testbun run buildbun run lint:cifrom a clean tracked-only worktreeThe ordinary local
lint:ciinvocation sees an ignored local.agents/skills/tikoci-centrs/SKILL.mdand reports its pre-existing markdownlint errors. The exact committed snapshot passeslint:ciin a clean worktree.Closes #281.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation