feat(explain): promote Q16 write-shape tristate to production (phase 0.5, #186) - #195
feat(explain): promote Q16 write-shape tristate to production (phase 0.5, #186)#195mobileskyfi wants to merge 5 commits into
Conversation
…0.5, #186) Promotes the ratified `containsWrite` tristate from the throwaway lab probe `.scratch/explain-lab-q16-write.ts` into `src/explain/write.ts`, on top of the Q1 segmenter (#189), the Q2-Q4 path resolver (#191), and the Q6 verb/menu boundary (#193). Only the ratified `failclosed` navigation arm ships; the two arms that priced it stayed in the lab. All 35 oracle-checked corners are promoted as frozen fixtures, plus branch anchors for the ratified rollup rules, the frozen vocabularies, the Q14 defect floor, and the pre-registered hard-0 threshold (zero false negatives on statically obvious writes). Two fail-closed corrections fall out of composing with the production resolvers rather than the lab's: - a statement the path resolver refused (Q14 structural defect) is not descended, so a write may hide in a body that was never walked: it now emits a blocking `defect` occurrence instead of being read as control flow; - a bare-word head arrives with V4 ambiguity and no decided verb, because production `menuNavPath` requires a leading `/` (Q4's CHR correction that bare-word navigation is not valid RouterOS). Clearing it would be a false negative on a curated write verb, so it abstains -- Q14's rule (b). The lab's declared `overconfident` residual (a trailing bare path clears instead of abstaining) is carried as measured and pinned by a KNOWN-LIMIT test rather than fixed by guess; narrowing it needs a corpus re-score. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds offline RouterOS write inference with curated vocabularies, occurrence classification, structural-failure handling, and fail-closed tristate results. Exports the API, documents vocabulary, and adds fixture-driven classification and robustness coverage. ChangesWrite analysis
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Input
participant occurrences
participant resolveStatements
participant resolveDocument
participant containsWrite
Input->>occurrences: provide RouterOS text
occurrences->>resolveStatements: resolve statements
occurrences->>resolveDocument: resolve brackets and document notes
resolveStatements-->>occurrences: classified statements
resolveDocument-->>occurrences: classified bracket content and notes
occurrences-->>containsWrite: return occurrences and notes
containsWrite-->>Input: return write analysis
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
Promotes the Q16 “write-shape” tristate inference into the production src/explain surface, including a new containsWrite() analysis API plus fixtures/tests that pin the ratified fail-closed rules.
Changes:
- Added
src/explain/write.tsimplementingcontainsWrite()/occurrences()and the frozen vocabularies used for classification. - Exported the new explain/write API from
src/index.ts(withoccurrencesrenamed toexplainOccurrencesat the library root). - Added Q16 fixtures and unit tests to lock in the tristate behavior and safety thresholds; updated glossary terms used by the new tests/docs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/explain/write.ts |
New production implementation of Q16 write-shape tristate inference and rollup. |
src/index.ts |
Re-exports the new explain/write APIs and types from the library root. |
test/fixtures/explain/write.json |
Adds Q16 fixture corpus (corners + branch anchors) consumed by unit tests. |
test/unit/explain-write.test.ts |
Adds comprehensive unit coverage for Q16 invariants, thresholds, and export surface. |
GLOSSARY.txt |
Adds new project vocabulary tokens referenced by the promoted explain/write work. |
![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 `@src/explain/write.ts`:
- Around line 291-312: The navigable-statement flow reparses identical text
multiple times. Compute describeStatement(t) once in collect and update
isCommandShaped, isConfirmedNav, classifyStatement, and classifyUnconfirmedNav
to accept and reuse that parsed result, preserving their existing classification
behavior.
- Around line 195-204: Anchor the bracket-variable pattern in DYNAMIC_FORMS so
isDynamicForm only treats a statement or bracket-inner text beginning with [$…]
as dynamic, rather than matching argument values mid-string. Preserve the
existing dynamic detection for head-position variable/function calls, and add a
regression case for containsWrite("/ip route add gateway=[$gw]") expecting a
true verdict with one write.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 2c417e30-107c-447f-8239-ec122f14d119
📒 Files selected for processing (5)
GLOSSARY.txtsrc/explain/write.tssrc/index.tstest/fixtures/explain/write.jsontest/unit/explain-write.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CHR smoke (stable)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Runbun run lint && bun run test && bun run buildbefore finishing code changes.
Never commit secrets, real router credentials, private CDB/Dude databases, or sensitive packet captures.
Do not make generated output the hand-edited source of truth.
Do not add new documentation for status, plans, or roadmaps; update the constitution, matrix, or relevant per-command files instead.
PreferrosettaMCP tools for RouterOS facts before using web search.
Write code or tests only after reading the constitution, matrix, and relevant command specification; do not write additional prose during implementation.
Files:
GLOSSARY.txtsrc/index.tstest/fixtures/explain/write.jsontest/unit/explain-write.test.tssrc/explain/write.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.{ts,tsx}: Use Bun-native TypeScript and Web APIs where possible
Errors must be actionable for humans and agents, with next-step guidance when a dependency, protocol, credential, or validation source is missing
src/**/*.{ts,tsx}: Use Bun-first TypeScript workflows for source code and run the required lint, test, and build verification commands before finishing changes.
Do not silently fall back to another protocol when the caller pins--via.
Treat credential handling, RouterOS write execution, proxy listeners, and local discovery data as security-sensitive surfaces; followSECURITY.mdand the security-quality instructions.
Do not disable validation to make a test pass; validation is part of the product.
Files:
src/index.tssrc/explain/write.ts
test/fixtures/**/*
📄 CodeRabbit inference engine (test/AGENTS.md)
Keep fixtures under
test/fixtures/with clear source/provenance notes.
Files:
test/fixtures/explain/write.json
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests passing alone do not establish completion; transport- or RouterOS-touching tests require
bun run test:integration.
Files:
test/unit/explain-write.test.ts
🔇 Additional comments (5)
src/explain/write.ts (1)
1-186: LGTM! Vocabularies,classifyVerb, and thecontainsWriterollup are internally consistent — cross-checkedWRITE_VERBS/READ_VERBS/ROOT_WRITE/ROOT_DYNAMIC/ROOT_CMDSsizes/contents and the rollup rules against the accompanying fixtures/tests and found no discrepancies.Also applies to: 213-269, 469-531
GLOSSARY.txt (1)
530-561: LGTM!test/unit/explain-write.test.ts (1)
1-420: LGTM! Assertions align with the implementation's documented rollup/classification rules.src/index.ts (1)
346-361: LGTM!test/fixtures/explain/write.json (1)
1-317: LGTM! Fixture provenance note satisfies thetest/fixtures/**/*requirement. As per path instructions, "Keep fixtures undertest/fixtures/with clear source/provenance notes."Source: Path instructions
…p NUL bytes - Anchor the `[$…]` dynamic-form regex (Copilot + CodeRabbit, both correct). Unanchored it fired on any `[$…]` anywhere in the line, so a fully-known write was masked as `dynamic`: `/ip route add gateway=[$gw]` returned `unknown`/0 writes instead of `true`/1. A dynamic argument VALUE does not make a curated write verb uncertain (rule 1), and the inner `$gw` still abstains on its own through the bracket walk. Three regression tests added. - Replace the raw NUL/SOH bytes in the adversarial fixture list with escapes (Copilot). They were real -- `file(1)` classified the test as `data` -- and survived a first grep check that used the wrong matcher. - Parse each statement once and thread the result through the boundary rules instead of re-deriving it three or four times per statement (CodeRabbit). - Reword the occurrences JSDoc: order carries no meaning, but the list is not a set and repeats are significant, so it must not be deduped (Copilot). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mobileskyfi
left a comment
mobileskyfi
left a comment
There was a problem hiding this comment.
Independent review pass completed at 4f447b5.
I fixed three clear Q16 composition defects in that commit:
- Stored-script detection now reads the leading token run. The previous raw substring probe masked a definite write when an argument string contained
/system script run, while also dropping the valid mixed spelling/system/script run backupas clean navigation. - Non-structural path-resolution outcomes (
[find], expression/string heads, variable calls) no longer become fabricateddefectoccurrences; each now follows the existingdynamic/no-verbcontract. - Bracketed bare paths retain the Q6
ambiguousfloor, so:put [/system/reboot]cannot clear as read-only.
Focused coverage is 90/90. The normal pre-push gate passed: lint + CI lint, bun test (1,738 pass / 0 fail / 35 skipped), and build.
One P1 remains inline: the explicitly pinned top-level /system/reboot false negative conflicts with the constitution and the hard-zero Q16 safety threshold. I left it unpatched because changing the navigation arm requires the stated corpus re-score; that is major work, not a local predicate tweak.
…n leaks Addresses the open P1 on #195 and reviews 4f447b5. Codex's three fixes are correct and kept: the run-based stored-script probe (a raw substring probe masked writes whose ARGUMENT contained the text, and missed mixed separator spellings), the narrowed Q14 defect branch, and the bracket `ambiguous` verdict that made brackets consistent with statements. Three changes on top. 1. P1 — a dangling bare path now abstains. `isConfirmedNav` decides at statement scale, so a lone or trailing `/system/reboot` was dropped as navigation and cleared the document to `false`: a false negative on a statically obvious write, reproducing inside the tristate the exact failure the tristate was ratified to prevent. Position is the schema-free confirmation — navigating and then ending the script is a no-op, while a no-argument command is meaningful. Measured over the frozen 911-script corpus this moves ZERO documents, so the ratified navigation arm's abstention is unchanged (44.0% dev / 45.2% holdout). The fuller rule (abstain unless a later RELATIVE statement consumes the context) was also measured and is declared as a priced residual, not taken blind: +1.4pp dev / +1.9pp holdout across 14 documents, all 14 genuine directories. 2. Narrowing the defect branch to a "structural defect:" prefix failed OPEN for two of pathresolve's four statement refusal reasons. `runTokens` stops at the first non-bare segment, so `/ip/$menu/remove 0` and `/ip/$m/add address=1.2.3.4` yield an EMPTY run and are refused with "no leading path token" — the same reason a harmless `"a string"` gets — and both returned `false`. Now a `/`-led statement whose path could not be read abstains, and the refusal-reason check is an ALLOW list so an unrecognized or reworded reason blocks instead of clearing. 3. Corpus accounting for 2: 4 documents move `false` -> `unknown`, all pasted CLI transcripts (`/interface/lte> monitor lte1,lte2`), where the previous answer was a confident "no write" about unparseable text. The KNOWN LIMIT test is replaced by real assertions plus the declared residual. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reviewing b9dec39 found its two statement-side fixes had an exact twin in the bracket walk that was never closed. Both guards were applied to statements only, so the same text abstained or cleared depending on whether it sat inside brackets: /ip/$menu/remove 0 -> unknown (ambiguous) :put [/ip/$menu/remove 0] -> FALSE (no-verb) A confident "no write" about a statement that plainly names a curated write verb is a false negative on a statically obvious write — the one outcome Q16's hard-0 threshold forbids, reproduced inside the tristate for the second time. Finding 2 says fail-closed is enforced where input is DISCARDED, and a bracket discards it just as thoroughly as a statement does. Two changes, mirroring the statement branch exactly. 1. The Q14 defect floor now runs over bracket resolutions. Brackets carry their own refusal vocabulary, so this is not redundant with the statement check: `variable path segment` and `looks like a pasted CLI prompt, not a substitution` are reachable here and neither is on the ALLOW list, so both block instead of clearing. 2. The bracket `ambiguous` test becomes `isPathShapedButUnread` rather than a bare `run.length > 0`. Independently load-bearing: `[/ip/"foo"/remove]` and `[/ip/[find]/remove]` yield an EMPTY run with NO refusal reason, so rule 1 does not see them and only the path-shaped guard catches them. Priced over the frozen 911-script corpus with the same harness as b9dec39: +0.8pp abstention on both splits (44.0 -> 44.8 dev, 45.2 -> 46.0 holdout). All 7 moved documents go `false` -> `unknown`; ZERO move off `true`, so no precision is lost on decided writes. All 7 are pasted CLI transcripts whose `[user@device]` prompt parses as a bracket — and two of them (`/ip/socks/set enabled=yes`, `/system identity set name=…`) hide a real write behind that prompt, so those were genuine false negatives, not over-abstention. The 35 frozen oracle corners are unaffected. Five regression tests added, including the corpus transcript shape and an anchor on the bracket refusal vocabulary, which differs from the statement one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed the current state of this branch, including the two review-response commits ( The bracket walk was missing both fail-closed guards
A confident "no write" about a statement that plainly names a curated write verb is exactly what the Q16 hard-0 threshold forbids. Finding 2 says fail-closed is enforced where input is discarded — a bracket discards it just as thoroughly as a statement does. Two changes, mirroring the statement branch:
Priced on the frozen corpusSame harness and discipline as
+0.8pp on both splits, 7 documents moved. All 7 go All 7 are pasted CLI transcripts where the
The statement walk reads those lines as starting with The 35 frozen oracle corners are unaffected. Five regression tests added, including the corpus transcript shape and an anchor on the bracket refusal vocabulary (which differs from the statement one, so drift in either fails closed independently). Not merging — leaving this for review. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 32 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@GLOSSARY.txt`:
- Around line 548-561: Add the missing toip6 entry to the ROOT_CMDS dictionary
block in GLOSSARY.txt, alongside the existing command names, so the glossary
matches the ROOT_CMDS declaration in src/explain/write.ts.
In `@src/explain/write.ts`:
- Around line 365-375: Update collect to compute the last non-empty statement
index once before its loop, then pass that value to isDanglingBarePath at the
existing call site. Change isDanglingBarePath to compare the current index with
the precomputed lastNonEmpty value and remove its per-statement tail scan,
preserving the existing described.run empty check.
In `@test/unit/explain-write.test.ts`:
- Around line 366-375: Update the “the root tables are exactly the measured
sets” test to assert the sorted contents of ROOT_CMDS, not only its size.
Preserve the existing ROOT_WRITE and ROOT_DYNAMIC assertions, and enumerate all
56 expected command names so vocabulary substitutions are detected.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 692c386f-0af6-4178-8c01-96376a12860b
📒 Files selected for processing (5)
GLOSSARY.txtsrc/explain/write.tssrc/index.tstest/fixtures/explain/write.jsontest/unit/explain-write.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Runbun run lint && bun run test && bun run buildbefore finishing code changes.
Never commit secrets, real router credentials, private CDB/Dude databases, or sensitive packet captures.
Do not make generated output the hand-edited source of truth.
Do not add new documentation for status, plans, or roadmaps; update the constitution, matrix, or relevant per-command files instead.
PreferrosettaMCP tools for RouterOS facts before using web search.
Write code or tests only after reading the constitution, matrix, and relevant command specification; do not write additional prose during implementation.
Files:
GLOSSARY.txtsrc/index.tstest/fixtures/explain/write.jsontest/unit/explain-write.test.tssrc/explain/write.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.{ts,tsx}: Use Bun-native TypeScript and Web APIs where possible
Errors must be actionable for humans and agents, with next-step guidance when a dependency, protocol, credential, or validation source is missing
src/**/*.{ts,tsx}: Use Bun-first TypeScript workflows for source code and run the required lint, test, and build verification commands before finishing changes.
Do not silently fall back to another protocol when the caller pins--via.
Treat credential handling, RouterOS write execution, proxy listeners, and local discovery data as security-sensitive surfaces; followSECURITY.mdand the security-quality instructions.
Do not disable validation to make a test pass; validation is part of the product.
Files:
src/index.tssrc/explain/write.ts
test/fixtures/**/*
📄 CodeRabbit inference engine (test/AGENTS.md)
Keep fixtures under
test/fixtures/with clear source/provenance notes.
Files:
test/fixtures/explain/write.json
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests passing alone do not establish completion; transport- or RouterOS-touching tests require
bun run test:integration.
Files:
test/unit/explain-write.test.ts
🔇 Additional comments (8)
src/explain/write.ts (4)
86-200: LGTM!
229-238: LGTM!
494-601: LGTM!
641-668: LGTM!src/index.ts (1)
346-361: LGTM!test/unit/explain-write.test.ts (2)
390-562: LGTM!
564-655: LGTM!test/fixtures/explain/write.json (1)
1-330: LGTM!
| failclosed | ||
| notadirective | ||
| jobname | ||
| rndnum | ||
| rndstr | ||
| tobool | ||
| tocrlf | ||
| toid | ||
| toip | ||
| tolf | ||
| tonsec | ||
| tonum | ||
| totime | ||
| lte |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -H -t f 'cspell*' -x cat {}
rg -n 'toip6' --hiddenRepository: tikoci/centrs
Length of output: 1525
🏁 Script executed:
#!/bin/bash
set -e
printf '--- candidate files ---\n'
fd -H -t f 'cspell|write|GLOSSARY|composer|package|bun' -x sh -c 'echo "FILE:$0"; wc -l "$0"' {}
printf '\n--- toip6 occurrences ---\n'
rg -n -C 2 'toip6|ROOT_CMDS|toid|toip|tolf|tonsec' . --hidden || true
printf '\n--- cspell config relevant sections ---\n'
if [ -f cspell.json ]; then jq '.dictionaryDefinitions, .dictionaries, .ignoreWords, .ignorePaths' cspell.json; fi
if [ -f package.json ]; then jq '.scripts' package.json; fiRepository: tikoci/centrs
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -H -t f 'cspell|write|GLOSSARY|composer|package|bun' -x sh -c 'echo "FILE:$0"; wc -l "$0"' {}
printf '%s\n' ''
printf '%s\n' '--- toip6 occurrences ---'
rg -n -C 2 'toip6|ROOT_CMDS|toid|toip|tolf|tonsec' . --hidden || true
printf '%s\n' ''
printf '%s\n' '--- cspell config relevant sections ---'
if [ -f cspell.json ]; then
jq '.dictionaryDefinitions, .dictionaries, .ignoreWords, .ignorePaths' cspell.json || true
fi
if [ -f package.json ]; then
jq '.scripts' package.json || true
fiRepository: tikoci/centrs
Length of output: 8095
Add toip6 to the ROOT_CMDS glossary.
src/explain/write.ts now declares toip6 in ROOT_CMDS, but GLOSSARY.txt only lists the previous ROOT_CMDS entries. CSpell uses GLOSSARY.txt as project-words, so add toip6 to the ROOT_CMDS dictionary block to keep bun run lint:ci from failing.
🤖 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 `@GLOSSARY.txt` around lines 548 - 561, Add the missing toip6 entry to the
ROOT_CMDS dictionary block in GLOSSARY.txt, alongside the existing command
names, so the glossary matches the ROOT_CMDS declaration in
src/explain/write.ts.
| function isDanglingBarePath( | ||
| described: Described, | ||
| statements: readonly StatementResolution[], | ||
| index: number, | ||
| ): boolean { | ||
| if (described.run.length === 0) return false; | ||
| for (let j = index + 1; j < statements.length; j++) | ||
| if ((statements[j] as StatementResolution).text.trim().length > 0) | ||
| return false; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
isDanglingBarePath rescans the tail per statement — O(n²) on nav-heavy input.
The trailing-blank scan is recomputed for every confirmed-nav statement. Compute the index of the last non-empty statement once in collect and compare.
♻️ Proposed refactor
function isDanglingBarePath(
described: Described,
- statements: readonly StatementResolution[],
+ lastNonEmpty: number,
index: number,
): boolean {
if (described.run.length === 0) return false;
- for (let j = index + 1; j < statements.length; j++)
- if ((statements[j] as StatementResolution).text.trim().length > 0)
- return false;
- return true;
+ return index >= lastNonEmpty;
}In collect, derive lastNonEmpty once before the loop and pass it at Line 543.
🤖 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 `@src/explain/write.ts` around lines 365 - 375, Update collect to compute the
last non-empty statement index once before its loop, then pass that value to
isDanglingBarePath at the existing call site. Change isDanglingBarePath to
compare the current index with the precomputed lastNonEmpty value and remove its
per-statement tail scan, preserving the existing described.run empty check.
| test("the root tables are exactly the measured sets", () => { | ||
| expect([...ROOT_WRITE].sort()).toEqual([ | ||
| "import", | ||
| "password", | ||
| "redo", | ||
| "undo", | ||
| ]); | ||
| expect([...ROOT_DYNAMIC].sort()).toEqual(["execute", "parse"]); | ||
| expect(ROOT_CMDS.size).toBe(56); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Pin ROOT_CMDS contents, not just its size.
expect(ROOT_CMDS.size).toBe(56) passes if a name is swapped rather than added/removed — weaker than the sibling tables, which assert sorted contents. The stated purpose is freezing the vocabulary against tuning.
♻️ Proposed change
- expect(ROOT_CMDS.size).toBe(56);
+ expect([...ROOT_CMDS].sort()).toEqual([
+ "beep", "break", "continue", "convert", "delay", "deserialize", "do",
+ "error", "execute", "exit", "export", "find", "for", "foreach",
+ "global", "grep", "if", "import", "jobname", "len", "local", "lock",
+ "nothing", "onerror", "parse", "password", "pick", "ping", "put",
+ "quit", "range", "recursive-print", "redo", "resolve", "retry",
+ "return", "rndnum", "rndstr", "serialize", "set", "time", "timestamp",
+ "toarray", "tobool", "tocrlf", "toid", "toip", "toip6", "tolf",
+ "tonsec", "tonum", "tostr", "totime", "typeof", "undo", "while",
+ ]);📝 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.
| test("the root tables are exactly the measured sets", () => { | |
| expect([...ROOT_WRITE].sort()).toEqual([ | |
| "import", | |
| "password", | |
| "redo", | |
| "undo", | |
| ]); | |
| expect([...ROOT_DYNAMIC].sort()).toEqual(["execute", "parse"]); | |
| expect(ROOT_CMDS.size).toBe(56); | |
| }); | |
| test("the root tables are exactly the measured sets", () => { | |
| expect([...ROOT_WRITE].sort()).toEqual([ | |
| "import", | |
| "password", | |
| "redo", | |
| "undo", | |
| ]); | |
| expect([...ROOT_DYNAMIC].sort()).toEqual(["execute", "parse"]); | |
| expect([...ROOT_CMDS].sort()).toEqual([ | |
| "beep", "break", "continue", "convert", "delay", "deserialize", "do", | |
| "error", "execute", "exit", "export", "find", "for", "foreach", | |
| "global", "grep", "if", "import", "jobname", "len", "local", "lock", | |
| "nothing", "onerror", "parse", "password", "pick", "ping", "put", | |
| "quit", "range", "recursive-print", "redo", "resolve", "retry", | |
| "return", "rndnum", "rndstr", "serialize", "set", "time", "timestamp", | |
| "toarray", "tobool", "tocrlf", "toid", "toip", "toip6", "tolf", | |
| "tonsec", "tonum", "tostr", "totime", "typeof", "undo", "while", | |
| ]); | |
| }); |
🤖 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/explain-write.test.ts` around lines 366 - 375, Update the “the root
tables are exactly the measured sets” test to assert the sorted contents of
ROOT_CMDS, not only its size. Preserve the existing ROOT_WRITE and ROOT_DYNAMIC
assertions, and enumerate all 56 expected command names so vocabulary
substitutions are detected.
Pull Request
Links
Change type
Notes
bun run lint·bun run lint:ci·bun test(1727 pass / 0 fail) ·bun run build— all green.explainstaysdesignedin the MATRIX (no live protocol cell).What
The next phase-0.5 vertical slice (#186): promotes the ratified write-shape tristate into
src/explain/write.ts. This is the first of the two ratified amendments to reach production, and it consumes the Q6 boundary that landed in #193 — Q16 composes with Q6 rather than needing a schema of its own.src/explain/write.ts(Q16 —structure.containsWrite)containsWrite(text)→{ verdict: "true" | "false" | "unknown", writes, blockers, occurrences, notes }, plusoccurrences(text)andclassifyVerb(verb, directive)and the five frozen vocabularies.failclosednavigation arm is promoted. The two arms that priced it (q4nav/q6split) stayed in the lab, along with the corpus scorer, the vocabulary recon, and the staleness probe.canonicalizeExecuteCommand'smode/writeShapedis untouched;containsWriteis a distinct explain-only inference with basisheuristic, exactly as the spec requires.Why the tristate, restated at the code: no schema artifact carries mutation semantics (
inspect.jsontags nodespath|dir|cmd|arg; restraml's method assignment is rule-derived and circular to read back), so the table is curated — and while a 17-name table covers 93.1% of cmd nodes on 7.24beta1 and 93.8% on 7.9.2 (0.07% churn per minor version), 56 of the 225 uncovered verb names are write-shaped. A boolean would have to call/disk format-drivea non-write. The gap is standing, not staleness.The four ratified rollup rules ship verbatim: a proven write wins outright; with no proven write any unclassifiable occurrence yields
unknown, neverfalse; statements that cannot be confirmed as navigation emit an occurrence rather than being dropped (finding 2 — fail-closed has to be enforced where input is discarded, not where it is classified); andunknownis reported with its blockers.Two fail-closed corrections from composing with the production resolvers
Both are places where the production Q3/Q4 resolver is stricter than the lab's, so a faithful port would have left a hole. Neither invents a rule — each applies a floor the spec already ratifies.
resolveStatementsrefuses a structurally malformed statement and does not descend into it, so a write may hide in a body that was never walked. Reading it as harmless control flow would clear the document (:if [) do={ /ip route add … }→false). It now emits a blockingdefectoccurrence, and a document-level segmenter/over-depth note abstains on its own.ambiguous. ProductionmenuNavPathrequires a leading/, because Q4's CHR round confirmed bare-word navigation is not valid RouterOS. So a bare-word head (reset-countersunder an/interfacecontext) never reaches the unconfirmed-nav fallback and instead arrives with V4 ambiguity and no decided verb. Clearing it would be a false negative on a curated write verb — the one thing the hard threshold forbids — so an undecided boundary over a non-empty run abstains. That is Q14's rule (b) enforced at the rollup.Net effect on the measured numbers: strictly more abstention, never less. Both moves trade precision for the pre-registered zero-false-negative requirement, which is the direction the tristate exists to allow.
Left as measured, declared not fixed
A slash-led bare path with no hyphenated segment reads as confirmed navigation and is dropped — right for
/ip/firewall/filter(a directory), wrong for/system/reboot(a no-argument write). The two are the same text; only a schema separates them. This is the lab's declaredoverconfidentbin — 3 of 352 holdout captures, reported separately and deliberately excluded from the precision denominator because the IL oracle cannot decide those cells either. It is pinned by an explicitKNOWN LIMITtest so the residual is visible rather than latent; narrowingisConfirmedNavneeds a corpus re-score, not a guess.Tests
Fixtures + anchor tests (79 total, all green):
.scratch/explain-lab-q16-corners.ts— direct writes (incl. the four bare-word-operand shapes finding 1 named), other mutating verbs, nav-then-write, nested writes atdo=/else=/:onerror in=depth, the four inert cases that must not count (string, comment, literalsource={…}, write stored in a variable), six dynamic cases that must beunknown, root write vs root read (:setis a variable,/ip/route setis a device write), and three long-tail verbs that must abstain.false— each asserted over every fixture, which is strictly stronger than matching per-case verdicts.set/import/undo/export/findmean different things at root and at a menu, rollup consistency as a property over adversarial input, order independence, determinism, never-throws, bounded depth, and the public export surface.Left behind (measurement rig, not product)
The two non-
failclosednav arms, the corpus scorer, the vocabulary reconnaissance, the staleness/time-travel probe, and thecontainsWriteBooleanshape-comparison helper.🤖 Generated with Claude Code
Review round (
a17b0c6) — all 5 threads addressed and resolvedCopilot and CodeRabbit independently found the same real bug, and both were right:
[$…]dynamic-form probe (both bots). It fired on any[$…]anywhere in the line, so a fully-known write was masked:/ip route add gateway=[$gw]returnedunknown/0 writes instead oftrue/1. Now anchored to/^\s*\[\s*\$/— a dynamic argument value does not make a curated write verb uncertain (rule 1), and an inner$gwstill abstains on its own via the bracket walk. Three regression tests added.file(1)classified the test asdata, and my first check used a matcher that missed them. Replaced with explicit escapes.collectnow parses each statement once and threads the result through all four boundary rules instead of re-deriving it 3–4× per statement. Behavior unchanged; the frozen fixtures and property tests cover it.writescounts multiplicity), so the list must not be deduped.Re-validated:
bun run lint·bun run lint:ci·bun test(1730 pass / 0 fail) ·bun run build— all green; CI all green including CHR smoke.Summary by CodeRabbit
New Features
Tests