Unify public taxonomy on runtime 12 and retire canonical 14 claims#116
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes failure classes in ChangesFailure Taxonomy Migration
Sequence Diagram(s)sequenceDiagram
participant ContractsIndex as packages/contracts/src/index.ts (FAILURE_CLASSES)
participant SyncScript as scripts/failure-taxonomy.mjs
participant RuntimeJSON as docs/oss/failure-taxonomy.runtime.json
participant DocsMarkdown as docs/oss/FAILURE-TAXONOMY-12.md
SyncScript->>ContractsIndex: read FAILURE_CLASSES text
ContractsIndex-->>SyncScript: returned class identifiers
SyncScript->>SyncScript: validate non-empty & uniqueness
SyncScript->>RuntimeJSON: generate runtime JSON (schema, count, classes)
SyncScript->>DocsMarkdown: generate markdown taxonomy table
RuntimeJSON-->>SyncScript: parity/write confirmation
DocsMarkdown-->>SyncScript: parity/write confirmation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the failure taxonomy from a legacy 14-class taxonomy to a canonical 12-class runtime taxonomy derived dynamically from @martin/contracts. It introduces a synchronization script, updates documentation, and adds automated tests to prevent taxonomy drift. The review feedback suggests making the taxonomy generator script more robust by normalizing line endings to prevent Windows-specific test failures, enhancing the parser to handle comments and single quotes, and dynamically generating the markdown header count instead of hardcoding it.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const existing = await readFile(targetPath, "utf8").catch(() => ""); | ||
| if (existing !== expectedContents) { | ||
| throw new Error(`Taxonomy artifact drift detected: ${path.relative(ROOT_DIR, targetPath)}`); | ||
| } |
There was a problem hiding this comment.
On Windows systems, Git may automatically convert LF line endings to CRLF. Since the generated taxonomy artifacts are constructed with LF (\n), a strict equality check (existing !== expectedContents) will fail on Windows even if the content is semantically identical. Normalizing line endings by replacing \r\n with \n before comparison prevents false-positive test/check failures on Windows.
| const existing = await readFile(targetPath, "utf8").catch(() => ""); | |
| if (existing !== expectedContents) { | |
| throw new Error(`Taxonomy artifact drift detected: ${path.relative(ROOT_DIR, targetPath)}`); | |
| } | |
| const existing = await readFile(targetPath, "utf8").catch(() => ""); | |
| if (existing.replace(/\\r\\n/g, "\\n") !== expectedContents.replace(/\\r\\n/g, "\\n")) { | |
| throw new Error("Taxonomy artifact drift detected: " + path.relative(ROOT_DIR, targetPath)); | |
| } |
| const body = match[1]; | ||
| const classes = [...body.matchAll(/"([a-z_]+)"/g)].map((entry) => entry[1]); |
There was a problem hiding this comment.
The current parser strictly expects double quotes around class names and does not handle comments inside the FAILURE_CLASSES array. If a developer uses single quotes (e.g., due to an auto-formatter or personal preference) or adds comments inside the array, the parser will fail or extract incorrect values. Stripping comments and supporting both single and double quotes makes the parser significantly more robust.
const body = match[1];
const cleanBody = body.replace(/\\/\\/.*|\\/\\*[\\s\\S]*?\\*\\/g, "");
const classes = [...cleanBody.matchAll(/['"]([a-z_]+)['"]/g)].map((entry) => entry[1]);| .map((classId) => `| \`${classId}\` | ${CLASS_DESCRIPTIONS[classId] ?? "Runtime failure classification."} |`) | ||
| .join("\n"); | ||
| return [ | ||
| "# Failure Taxonomy (12 Runtime Classes)", |
There was a problem hiding this comment.
The header in the generated markdown is hardcoded to 12 Runtime Classes. If the number of failure classes in @martin/contracts changes in the future, the generated file's header will become outdated and inconsistent with the actual number of rows. Using classes.length dynamically ensures the header always reflects the true count.
| "# Failure Taxonomy (12 Runtime Classes)", | |
| "# Failure Taxonomy (" + classes.length + " Runtime Classes)", |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (2)
docs/agent-failure-atlas.md (1)
5-5: ⚡ Quick winConsider breaking this into separate sentences for clarity.
The sentence conveys three distinct pieces of information (canonical taxonomy, legacy aliases, and atlas purpose) in one complex structure. For developers encountering this for the first time, consider:
-The public canonical subset is the runtime 12-class taxonomy documented in [./oss/FAILURE-TAXONOMY-12.md](./oss/FAILURE-TAXONOMY-12.md). Legacy operational labels are replay-only aliases in [./oss/FAILURE-TAXONOMY-ALIASES.md](./oss/FAILURE-TAXONOMY-ALIASES.md). Use this atlas for the extended catalog. +The public canonical taxonomy is the runtime 12-class set documented in [./oss/FAILURE-TAXONOMY-12.md](./oss/FAILURE-TAXONOMY-12.md). Legacy operational labels are replay-only aliases documented in [./oss/FAILURE-TAXONOMY-ALIASES.md](./oss/FAILURE-TAXONOMY-ALIASES.md). + +This atlas provides an extended failure-mode catalog beyond the canonical 12 runtime classes.This separates the canonical taxonomy reference from the atlas's scope/purpose and clarifies that "extended catalog" means modes beyond the public 12 classes.
🤖 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 `@docs/agent-failure-atlas.md` at line 5, Split the long sentence into two or three clear sentences: state that the public canonical subset is the runtime 12-class taxonomy and link to FAILURE-TAXONOMY-12.md in the first sentence; in a second sentence note that legacy operational labels are replay-only aliases and link to FAILURE-TAXONOMY-ALIASES.md; add a short third sentence (or clause) clarifying that "atlas" refers to the extended catalog covering modes beyond the public 12 classes (i.e., explain "extended catalog" explicitly).Source: Coding guidelines
docs/oss/FAILURE-TAXONOMY-ALIASES.md (1)
1-24: ⚡ Quick winConsider consolidating duplicate mapping tables to reduce maintenance burden.
Both
FAILURE-TAXONOMY-14.mdandFAILURE-TAXONOMY-ALIASES.mdcontain identical legacy-to-canonical mapping tables (lines 9-24). If the mapping evolves, both files must be updated in lockstep. Consider:
- Consolidate: Keep the mapping in one file and have the other reference it, or
- Add cross-reference: Add a note like "See the mapping in FAILURE-TAXONOMY-14.md" if you want to keep distinct narrative framing.
The current approach serves different audiences well (historical users vs. replay implementers), but the DRY principle suggests centralizing the source of truth for the mapping data itself.
🤖 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 `@docs/oss/FAILURE-TAXONOMY-ALIASES.md` around lines 1 - 24, The duplicate legacy-to-canonical mapping table appears in both FAILURE-TAXONOMY-ALIASES.md and FAILURE-TAXONOMY-14.md; remove the redundant table from one file and point it to the single source of truth instead—e.g., move the full mapping table into FAILURE-TAXONOMY-14.md (or a new central file like FAILURE-TAXONOMY-ALIASES-SOURCE.md) and replace the table in FAILURE-TAXONOMY-ALIASES.md with a short cross-reference sentence such as "See mapping in FAILURE-TAXONOMY-14.md" so future edits only change the canonical mapping once; ensure the table header ("Legacy label | Canonical runtime class") and all legacy keys (policy_input_invalid, allow_path_traversal_rejected, etc.) are preserved in the chosen canonical file and update any README/links if necessary.
🤖 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 `@scripts/failure-taxonomy.mjs`:
- Around line 56-72: In toCanonicalMarkdown, do not use the fallback string for
missing descriptions; instead ensure the code throws an error when
CLASS_DESCRIPTIONS[classId] is undefined so missing descriptions surface at
build time. Locate the map over classes in toCanonicalMarkdown and replace the
fallback expression (the "Runtime failure classification." default) with logic
that throws a descriptive Error referencing classId (or, if you implement the
suggested validation elsewhere, simply use CLASS_DESCRIPTIONS[classId] without a
default). This change should reference the CLASS_DESCRIPTIONS lookup and the
classId variable in the map so missing entries fail fast.
- Around line 90-95: Add a validation step at the start of
syncFailureTaxonomyArtifacts to ensure CLASS_DESCRIPTIONS and FAILURE_CLASSES
are exact mirrors: call loadFailureClassesFromContracts() or otherwise read
FAILURE_CLASSES, compute the set of class keys, compare against
Object.keys(CLASS_DESCRIPTIONS), and throw a clear error if any class is missing
a description or if CLASS_DESCRIPTIONS contains extra keys; include the
mismatched keys in the error message so CI fails early and authors must update
CLASS_DESCRIPTIONS when they add/remove entries.
---
Nitpick comments:
In `@docs/agent-failure-atlas.md`:
- Line 5: Split the long sentence into two or three clear sentences: state that
the public canonical subset is the runtime 12-class taxonomy and link to
FAILURE-TAXONOMY-12.md in the first sentence; in a second sentence note that
legacy operational labels are replay-only aliases and link to
FAILURE-TAXONOMY-ALIASES.md; add a short third sentence (or clause) clarifying
that "atlas" refers to the extended catalog covering modes beyond the public 12
classes (i.e., explain "extended catalog" explicitly).
In `@docs/oss/FAILURE-TAXONOMY-ALIASES.md`:
- Around line 1-24: The duplicate legacy-to-canonical mapping table appears in
both FAILURE-TAXONOMY-ALIASES.md and FAILURE-TAXONOMY-14.md; remove the
redundant table from one file and point it to the single source of truth
instead—e.g., move the full mapping table into FAILURE-TAXONOMY-14.md (or a new
central file like FAILURE-TAXONOMY-ALIASES-SOURCE.md) and replace the table in
FAILURE-TAXONOMY-ALIASES.md with a short cross-reference sentence such as "See
mapping in FAILURE-TAXONOMY-14.md" so future edits only change the canonical
mapping once; ensure the table header ("Legacy label | Canonical runtime class")
and all legacy keys (policy_input_invalid, allow_path_traversal_rejected, etc.)
are preserved in the chosen canonical file and update any README/links if
necessary.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a8713e64-e52b-4fe4-9750-bdccd08c6334
📒 Files selected for processing (13)
README.mddocs/agent-failure-atlas.mddocs/oss/FAILURE-TAXONOMY-12.mddocs/oss/FAILURE-TAXONOMY-14.mddocs/oss/FAILURE-TAXONOMY-ALIASES.mddocs/oss/README.mddocs/oss/WEBSITE-TAXONOMY-SYNC.mddocs/oss/failure-taxonomy.runtime.jsonpackage.jsonpackages/contracts/src/index.tsscripts/failure-taxonomy.mjsscripts/tests/failure-taxonomy-sync.test.mjsscripts/tests/readme-public-surface.test.mjs
| function toCanonicalMarkdown(classes) { | ||
| const rows = classes | ||
| .map((classId) => `| \`${classId}\` | ${CLASS_DESCRIPTIONS[classId] ?? "Runtime failure classification."} |`) | ||
| .join("\n"); | ||
| return [ | ||
| "# Failure Taxonomy (12 Runtime Classes)", | ||
| "", | ||
| "<!-- Generated by scripts/failure-taxonomy.mjs. Do not hand-edit class rows. -->", | ||
| "", | ||
| "This is the canonical MartinLoop taxonomy. The class list is generated from `FAILURE_CLASSES` in `@martin/contracts`.", | ||
| "", | ||
| "| Runtime class | Meaning |", | ||
| "| --- | --- |", | ||
| rows, | ||
| "" | ||
| ].join("\n"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace fallback description with an error.
Line 58 uses a fallback description ("Runtime failure classification.") when a class is missing from CLASS_DESCRIPTIONS. This could mask missing descriptions. Instead, throw an error to force developers to add proper descriptions for all classes.
Note: If you accept the validation suggestion in lines 90-95, this fallback will never be reached, so you can simplify line 58 to just use CLASS_DESCRIPTIONS[classId] directly.
♻️ Alternative: throw error instead of using fallback
function toCanonicalMarkdown(classes) {
const rows = classes
- .map((classId) => `| \`${classId}\` | ${CLASS_DESCRIPTIONS[classId] ?? "Runtime failure classification."} |`)
+ .map((classId) => {
+ const description = CLASS_DESCRIPTIONS[classId];
+ if (!description) {
+ throw new Error(`Missing description for failure class: ${classId}`);
+ }
+ return `| \`${classId}\` | ${description} |`;
+ })
.join("\n");🤖 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 `@scripts/failure-taxonomy.mjs` around lines 56 - 72, In toCanonicalMarkdown,
do not use the fallback string for missing descriptions; instead ensure the code
throws an error when CLASS_DESCRIPTIONS[classId] is undefined so missing
descriptions surface at build time. Locate the map over classes in
toCanonicalMarkdown and replace the fallback expression (the "Runtime failure
classification." default) with logic that throws a descriptive Error referencing
classId (or, if you implement the suggested validation elsewhere, simply use
CLASS_DESCRIPTIONS[classId] without a default). This change should reference the
CLASS_DESCRIPTIONS lookup and the classId variable in the map so missing entries
fail fast.
| export async function syncFailureTaxonomyArtifacts({ write = false } = {}) { | ||
| const classes = await loadFailureClassesFromContracts(); | ||
| await ensureMatchesOrWrite(CANONICAL_JSON_PATH, toCanonicalJson(classes), write); | ||
| await ensureMatchesOrWrite(CANONICAL_MARKDOWN_PATH, toCanonicalMarkdown(classes), write); | ||
| return classes; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add validation to ensure CLASS_DESCRIPTIONS and FAILURE_CLASSES stay in sync.
The script doesn't validate that every class in FAILURE_CLASSES has a corresponding description in CLASS_DESCRIPTIONS, or that CLASS_DESCRIPTIONS doesn't contain extra entries. This could lead to incomplete documentation if a developer adds a class but forgets to add its description (the fallback on line 58 would silently use a generic description).
Add validation at the start of this function to catch missing or extra descriptions early.
🛡️ Proposed validation to add
export async function syncFailureTaxonomyArtifacts({ write = false } = {}) {
const classes = await loadFailureClassesFromContracts();
+
+ // Validate CLASS_DESCRIPTIONS
+ const descriptionKeys = Object.keys(CLASS_DESCRIPTIONS);
+ const missingDescriptions = classes.filter(c => !CLASS_DESCRIPTIONS[c]);
+ const extraDescriptions = descriptionKeys.filter(k => !classes.includes(k));
+
+ if (missingDescriptions.length > 0) {
+ throw new Error(`Missing descriptions for failure classes: ${missingDescriptions.join(', ')}`);
+ }
+
+ if (extraDescriptions.length > 0) {
+ throw new Error(`Extra descriptions for non-existent failure classes: ${extraDescriptions.join(', ')}`);
+ }
+
await ensureMatchesOrWrite(CANONICAL_JSON_PATH, toCanonicalJson(classes), write);
await ensureMatchesOrWrite(CANONICAL_MARKDOWN_PATH, toCanonicalMarkdown(classes), write);
return classes;
}🤖 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 `@scripts/failure-taxonomy.mjs` around lines 90 - 95, Add a validation step at
the start of syncFailureTaxonomyArtifacts to ensure CLASS_DESCRIPTIONS and
FAILURE_CLASSES are exact mirrors: call loadFailureClassesFromContracts() or
otherwise read FAILURE_CLASSES, compute the set of class keys, compare against
Object.keys(CLASS_DESCRIPTIONS), and throw a clear error if any class is missing
a description or if CLASS_DESCRIPTIONS contains extra keys; include the
mismatched keys in the error message so CI fails early and authors must update
CLASS_DESCRIPTIONS when they add/remove entries.
Resolves conflicts between audit-fixes-v2 (F1/F4-F8) and main's independently-landed PR #88 hardening plus PR #116 taxonomy unification, which touched nearly the same files (context-integrity, grounding, leash, claude-cli, run-store/cost-provenance, and the corresponding tests). Resolution highlights: - context-integrity.ts: kept main's stricter identity-redefinition regex - grounding.ts: kept main's `??`-based MARTIN_GROUNDING_DIR resolution - leash.ts: combined main's Windows destructive-pattern additions with main's stricter rmInvocation regex - claude-cli.ts: took main's hasAuthoritativeCost/extractUsage and streaming-usage-inspector wholesale; combined main's stricter secret patterns with audit-fixes-v2's generic catch-all redaction pattern - run-store.ts / cli/index.ts: unified on main's CostProvenance API, added costProvenanceLabel to the tokenWasteReceipt - Combined test isolation helpers (MARTIN_RUNS_DIR/MARTIN_GROUNDING_DIR/ MARTIN_INTEGRITY_KEY_DIR) across operator-commands, corpus-intelligence, cli-integration, and runtime tests so the F6 receipt-integrity isolation fix also covers main's newer tests - Adjusted a cost-provenance test assertion to match main's "provenance: provider-settled actual" label text - Fixed an additional pre-existing MARTIN_INTEGRITY_KEY_DIR isolation gap in operator-commands.test.ts's "reports unsigned for ad-hoc --file loads" test, found during validation Verified: pnpm build, lint, test (414 vitest + 58 node:test, 0 failures), oss:validate, and public:smoke all pass; ~/.martin/grounding stayed empty throughout. # Conflicts: # packages/adapters/src/claude-cli.ts # packages/cli/src/index.ts # packages/cli/src/run-store.ts # packages/cli/tests/cli-integration.test.ts # packages/cli/tests/corpus-intelligence.test.ts # packages/cli/tests/operator-commands.test.ts # packages/core/src/context-integrity.ts # packages/core/src/grounding.ts # packages/core/src/index.ts # packages/core/src/leash.ts # packages/core/tests/context-integrity.test.ts # packages/core/tests/grounding.test.ts # packages/core/tests/runtime.test.ts
1 participant
Summary\n- switch public canonical taxonomy language from 14 labels to runtime 12 classes\n- keep historical 14-label page as legacy replay mapping only\n- add generated taxonomy artifacts + sync/check script\n- add parity tests and update README taxonomy assertions\n\n## Verification\n- node ./scripts/failure-taxonomy.mjs --check\n- node --test ./scripts/tests/failure-taxonomy-sync.test.mjs ./scripts/tests/readme-public-surface.test.mjs ./scripts/tests/readme-cta-guard.test.mjs ./scripts/tests/public-copy-scan.test.mjs\n\n## Governed proof\n- loop_vtu4tynk (dossier/verify/share complete)
Summary by CodeRabbit
Documentation
Tests