# Architecture Review Report

**Branch**: feature-190-wave-3b-frequency-analysis-api -> main
**Date**: 2026-06-02
**PR**: #266

## Issues in Your Changes (BLOCKING)

(none)

## Issues in Code You Touched (Should Fix)

(none)

## Pre-existing Issues (Not Blocking)

### MEDIUM

**Duplicate type definitions across crates** - `rskim-research/src/ast_types.rs:11-21` and `rskim-search/src/ast_index/ngram.rs:35,61,124`
**Confidence**: 85%
- Problem: `NodeKindId`, `AstBigram`, and `AstTrigram` are now defined in two independent crates. The research crate uses raw type aliases (`type AstBigram = u32`), while the search crate introduces proper newtypes (`struct AstBigram(u32)`). The encoding formulas are identical but maintained separately -- changes to one will not propagate to the other.
- Impact: Divergence risk over time. If the encoding scheme changes in either crate, the other becomes silently incompatible. The research crate generates the `ast_weights.rs` tables consumed by the search crate, so encoding consistency is a correctness requirement.
- Fix: In a future PR, consider extracting shared AST n-gram types into a shared crate (e.g. `rskim-ast-types`) or migrating the research crate to re-use the search crate's newtypes. This is non-blocking because: (1) no cross-crate dependency exists today, (2) the encoding formulas are tested independently in both crates, and (3) the codegen pipeline already validates encoding consistency.

## Suggestions (Lower Confidence)

- **`vocab_lookup` assumes sorted vocabulary** - `ngram.rs:190-193` (Confidence: 70%) -- `vocab_lookup` uses `binary_search` on `NODE_KIND_VOCABULARY`, which requires the array to be sorted. The array IS sorted (verified: auto-generated by `ast-codegen` in alphabetical order), but there is no compile-time or startup assertion guarding this invariant. If `ast-codegen` ever changes sort order, `vocab_lookup` would silently return wrong results. A `debug_assert!` in a const or init block would harden this.

- **`DEFAULT_AST_WEIGHT` cross-reference is a doc comment, not a code constraint** - `ngram.rs:44-46` (Confidence: 65%) -- The doc comment says `DEFAULT_AST_WEIGHT` "Matches `crate::weights::DEFAULT_WEIGHT`" but this is enforced only by convention, not a `const_assert!` or test. The ngram_tests do assert the value is `1.0`, but do not cross-check against `weights::DEFAULT_WEIGHT`. A single test asserting `DEFAULT_AST_WEIGHT == weights::DEFAULT_WEIGHT` would prevent silent drift.

- **`#[allow(dead_code)]` on `from_raw` methods** - `ngram.rs:98,161` (Confidence: 62%) -- Both `AstBigram::from_raw` and `AstTrigram::from_raw` are marked `pub(crate)` but also `#[allow(dead_code)]`, suggesting they have no current callers. If these are intended for future use (e.g. iterating weight tables), the allow is fine as a temporary measure. If they remain uncalled after the next wave, consider removing them to keep the API surface minimal.

## Summary

| Category | CRITICAL | HIGH | MEDIUM | LOW |
|----------|----------|------|--------|-----|
| Blocking | 0 | 0 | 0 | - |
| Should Fix | - | 0 | 0 | - |
| Pre-existing | - | - | 1 | 0 |

**Architecture Score**: 9/10
**Recommendation**: APPROVED

## Rationale

This PR adds well-structured AST n-gram newtypes that follow established codebase patterns closely. Key architectural strengths:

1. **Mirrors the lexical `Ngram` pattern** (applies FEATURE_KNOWLEDGE): The `AstBigram` and `AstTrigram` newtypes in `ngram.rs` follow the exact same structural pattern as `Ngram` in `src/ngram.rs` -- `#[repr(transparent)]` newtype with `encode`/`decode`/`key`/`from_raw` methods, `Display` impl, and `#[path = "...tests.rs"] mod tests`. This consistency makes the codebase predictable and discoverable.

2. **Clean module boundaries**: The `ngram` sub-module is properly encapsulated within `ast_index/`, re-exported through `ast_index/mod.rs` and `lib.rs`. Dependencies flow inward only: `ngram.rs` depends on `ast_weights` (data) and `rskim_core::Language` (types), with no reverse dependencies or circular imports.

3. **Vocabulary helpers are pure functions with no state**: `vocab_lookup`, `vocab_resolve`, and `vocab_len` are stateless thin wrappers over the compile-time `NODE_KIND_VOCABULARY` table. No runtime initialization, no caching, no allocation. This matches the crate's "pure core types" architecture principle documented in `lib.rs`.

4. **Weight lookup gracefully handles non-tree-sitter languages**: `ast_bigram_idf` and `ast_trigram_idf` fall back to `DEFAULT_AST_WEIGHT` for JSON/YAML/TOML, consistent with the Strategy Pattern documented in CLAUDE.md where non-tree-sitter languages return default/empty results rather than errors (applies ADR-001 -- the non-tree-sitter handling is explicit, not deferred).

5. **`#[repr(transparent)]` ensures zero-cost abstraction**: The newtypes add type safety at compile time with no runtime overhead, matching the project's "zero-cost abstractions" design constraint.

6. **Test coverage is thorough**: 400 lines of tests cover roundtrip encoding at boundaries, Display formatting for all edge cases (known/unknown/sentinel IDs), vocabulary lookup/resolve inverses, weight table consistency, and non-tree-sitter language fallback behavior.

The single pre-existing MEDIUM finding (duplicate type definitions across crates) is informational and does not affect this PR's correctness. The research crate and search crate are intentionally decoupled, and the encoding formulas are independently tested in both (avoids PF-002 -- surfacing this finding rather than silently deferring it).
