Rules Review

altium-cli Rules Review

Audit Rust source files against the project rules below. For each violation found, report:

1. Rule violated (by name from the checklist)
2. File and line (e.g., crates/altium-format/src/schlib.rs:123)
3. What's wrong (concrete description)
4. How to fix (specific suggestion)

At the end, give a summary: total violations found, grouped by severity (CRITICAL / WARNING).

Review Checklist

CRITICAL Rules (must never be violated)

R1: Fail Fast — No Silent Skipping

The parser must never silently skip data it doesn't understand. If a stream, record, or field exists in the file, the parser MUST either fully parse it or return an error. Look for:

• Any form of skip_known, ignore_remaining, or _ => {} catch-all match arms that discard data
• Marking entries as consumed in TrackedCfbDocument without actually reading and parsing their contents
• Deferring parsing to "future milestones" by suppressing errors
• Circumventing assert_all_consumed() in any way
• .ok(), .unwrap_or_default(), or let _ = on parse results that silently drop errors

R2: No Raw Primitive Types

Never use raw primitive integers where domain types exist in altium-format-types. Look for:

• u8 where PcbObjectId should be used
• i32 where SchRecordType, Coord, or other typed values should be used
• Raw integer literals for constants (e.g., 0xD0 instead of INSTRUCTION_BINARY, 0x00FF_FFFF instead of BLOCK_SIZE_MASK)
• Any struct field using a primitive integer type when a domain type exists or should be created
• Note: Rust String is correct for decoded text — Altium's own C# code uses plain .NET string. The encoding is a property of the serialization context, not the type.

R2b: Strict Encoding at Parse Boundaries

All byte-to-string decoding MUST use strict (non-lossy) functions. Look for:

• from_utf8_lossy() — ALWAYS a violation; use from_utf8() with error propagation or Windows-1252 decode
• Discarded had_errors flag on encoding_rs::UTF_16LE.decode() calls — MUST be checked
• Note: encoding_rs::WINDOWS_1252.decode() maps all 256 byte values and cannot error, so discarding its flags is safe

R3: No Silent Error Dropping

Everything fallible MUST return Result<T, AltiumFormatError> (in altium-format) or Result<T, SpecError> (in autopcb-spec). Look for:

• .unwrap() in non-test code
• .expect() in library code (acceptable in tests and CLI)
• Swallowing errors with if let Ok(x) = ... without handling the error case
• Using Option where Result is appropriate for fallible operations
• Missing ? propagation where errors should bubble up

R4: Crate Privacy — No Implementation Leaks

altium-format implementation details must stay private to the crate. Look for:

• pub visibility on internal parsing structs/functions that shouldn't be exposed
• autopcb-spec or altium-cli directly accessing altium-format internals
• Types or functions that should be pub(crate) but are pub

R5: No Unconsumed Data Suppression

Do NOT mark streams, records, or fields as "consumed" without actually parsing them. Look for:

• Calls to consume/mark-consumed methods without preceding parse logic
• Empty implementations that just mark things as handled
• TODOs or comments like "parse later" with suppressed errors

WARNING Rules (should be followed, deviations need justification)

R6: Error Type Correctness

Each crate uses its own error type:

• altium-format → AltiumFormatError
• autopcb-spec → SpecError
• altium-cli → anyhow Look for error types used in the wrong crate.

R7: Domain Types in altium-format-types

New domain types belong in altium-format-types, not inline in other crates. Look for:

• New enums or type aliases defined in altium-format that represent Altium concepts
• Constants defined outside of altium-format-types/src/constants/

R8: Constants from FileFormatConsts

All file format constants should come from altium_format_types::constants::* (mirroring FileFormatConsts.cs). Look for:

• Hard-coded magic numbers that correspond to known Altium constants
• String literals for stream names, record type names, or parameter keys that should be constants

R9: Dependency Direction

The dependency graph must be strictly:

altium-format-types → altium-format-derive → altium-format → autopcb-spec → altium-cli

Look for reverse dependencies or circular imports.

R10: Result Over Panic

Library code (everything except altium-cli) should return Result instead of panicking. Look for:

• panic!(), unreachable!() (when the case IS reachable), todo!() in non-test code
• assert!() in production code paths (acceptable in tests)

How to Review

1. Determine scope

If the user specifies a scope, use it. Otherwise, audit all Rust source files across the workspace:

crates/altium-format/src/**/*.rs
crates/altium-format-derive/src/**/*.rs
crates/altium-format-types/src/**/*.rs
crates/autopcb-spec/src/**/*.rs
crates/altium-cli/src/**/*.rs

Acceptable scope specifiers from the user:

• A crate name: e.g., "altium-format" → audit all .rs files in that crate
• A file or glob: e.g., "schlib.rs" or "src/param_*.rs"
• "changes" or "diff": audit only git changes (git diff HEAD)
• A commit or PR: audit that specific changeset
• No argument: audit the entire workspace

2. Scan systematically

For each file in scope, use Grep and Read to check for violations. Efficient search patterns:

3. Use context to reduce false positives

• .unwrap() and .expect() in #[cfg(test)] modules or test files are fine
• assert! in test code is fine
• pub on items that are part of the crate's intended public API is fine — focus on items that look like internal parsing machinery
• Some _ => arms legitimately handle a finite set — check if data is being discarded
• Raw types in proc macro code (altium-format-derive) may be acceptable since it generates code

4. Report findings

Order by severity (CRITICAL first, then WARNING), then by file path. Use this format:

## Findings

### CRITICAL

**R2: No Raw Primitive Types** — `crates/altium-format/src/schlib.rs:45`
Field `component_count: u32` uses raw primitive. Should use a domain type or at minimum document why a raw type is necessary here.
**Fix**: Define an appropriate type in `altium-format-types` or use an existing one.

### WARNING

**R8: Constants from FileFormatConsts** — `crates/altium-format/src/pcblib.rs:112`
Hard-coded string `"Board6"` should use a constant from `altium_format_types::constants`.
**Fix**: Add `pub const BOARD6: &str = "Board6";` to the appropriate constants module.

## Summary

| Severity | Count |
|----------|-------|
| CRITICAL | 1 |
| WARNING  | 1 |
| **Total** | **2** |

5. If no violations found

Explicitly state: "No rule violations found in the reviewed scope." and list which files were checked.