Skip to content

fix(train-inspect): info invented a model's architecture from its FILE SIZE — refuse instead of fabricating - #2529

Open
noahgift wants to merge 4 commits into
mainfrom
fix/inspect-no-fabrication
Open

fix(train-inspect): info invented a model's architecture from its FILE SIZE — refuse instead of fabricating#2529
noahgift wants to merge 4 commits into
mainfrom
fix/inspect-no-fabrication

Conversation

@noahgift

Copy link
Copy Markdown
Contributor

The code said so itself:

// For real implementation, would parse the actual file
// Here we return simulated data based on file size
let estimated_params = estimate_params_from_size(metadata.len(), &format);
let tensors = generate_mock_tensors(estimated_params);

It synthesised a tensor list from the file's size, then ran architecture detection over the invented shapes. Reproduced independently before changing anything — 5 KB of /dev/urandom renamed .safetensors:

Format           SafeTensors
Architecture     llama
Hidden Dimension 768
Layers           1
Vocab Size       256
Tensors          9
rc=0

A real one-tensor safetensors file gets the same nine tensors, because the answer never depended on the contents. This crate is published to crates.io, so that reached users as an "inspection".

Worth naming what it defeated: architecture.rs carries an N-05 hardening that derives hidden-dim from tensors rather than hardcoding 4096. It does derive honestly — from tensors fabricated one call earlier. The hardening sat one layer above the lie.

Fix

Return an error naming what it cannot do, pointing at the tools that actually read the file:

rc=1  Unsupported model format: SafeTensors: `inspect` cannot parse model files.
      It previously synthesised a tensor list from the file SIZE and reported that
      as the model's architecture… Use `apr inspect` or `apr tensors`, which read
      the file. Tracked in #2519.

Refusing is strictly better than fabricating. Whether this binary should exist at all is a separate question tracked in #2519 — this doesn't prejudge it.

Cleaned up rather than left behind: the two fabrication helpers are now #[cfg(test)] (retained only for the unit tests asserting their arithmetic, so no production path can call them); the ArchitectureDetector import is dropped; the metadata read is kept as _metadata with a comment, because it still surfaces a real permission/IO error — reporting the size was never the problem, inferring architecture from it was. Zero warnings in the crate.

Falsifier

test asserts
garbage_bytes_are_not_reported_as_a_model non-model bytes must not succeed
two_different_files_do_not_get_the_same_invented_answer the sharp one — two different files of equal size must not yield identical architecture and tensor count, which is exactly what size-derived answers do
a_missing_file_fails_for_its_own_reason non-vacuity

Mutation, done properly

My first attempt hand-restored the old code and did not compile (missing field total_params), so it proved nothing — the same standard I'd apply to any mutation that fails to turn RED. Redone with the real original from git:

$ git show HEAD:...inspect.rs > inspect.rs && cargo test --test falsify_...
garbage_bytes_are_not_reported_as_a_model               FAILED
two_different_files_do_not_get_the_same_invented_answer FAILED
a_missing_file_fails_for_its_own_reason                 ok

Two RED and the non-vacuity companion green — the discrimination that matters: the tests target the fabrication, not a function that refuses everything.

Verification

cargo test -p aprender-train-inspect 66 + 3 passed, 0 failed
cargo clippy --all-targets 0 errors
cargo fmt --check rc=0

--no-verify per #2526. Two sibling findings from the same audit remain in #2519aprender-train-bench (a sweep that never trains, returning a baked-in temperature = 4.00) and aprender-train-shell (fetch does-not-exist/totally-fake-7b exits 0 reporting 7.0B, Layers: 32). Same class, not touched here.

Refs #2519

…ILE SIZE — refuse instead of fabricating

crates/aprender-train-inspect/src/inspect.rs said so itself:

    // For real implementation, would parse the actual file
    // Here we return simulated data based on file size
    let estimated_params = estimate_params_from_size(metadata.len(), &format);
    let tensors = generate_mock_tensors(estimated_params);

It synthesised a tensor list from the file's SIZE, then ran architecture
detection over the invented shapes. Reproduced independently before changing
anything -- 5 KB of /dev/urandom renamed `.safetensors`:

    Format           SafeTensors
    Architecture     llama
    Hidden Dimension 768
    Layers           1
    Vocab Size       256
    Tensors          9
    rc=0

A real one-tensor safetensors file gets the SAME nine tensors, because the answer
never depended on the contents. **This crate is published to crates.io**, so that
output reached users as an "inspection".

Worth naming what it defeated: architecture.rs carries an N-05 hardening that
derives hidden-dim from tensors rather than hardcoding 4096. It derives honestly
-- from tensors fabricated one call earlier. The hardening sat one layer above
the lie.

FIX: return an error naming what it cannot do, and pointing at the tools that
actually read the file (`apr inspect`, `apr tensors`).

    rc=1  "Unsupported model format: SafeTensors: `inspect` cannot parse model
           files. It previously synthesised a tensor list from the file SIZE ..."

Refusing is strictly better than fabricating. Whether this binary should exist at
all is a separate question tracked in #2519; this does not prejudge it.

Also cleaned up rather than left behind:
  * the two fabrication helpers are now #[cfg(test)] -- retained only for the
    unit tests that assert their arithmetic, so no production path can call them
  * ArchitectureDetector import dropped (nothing detects from invented shapes)
  * the metadata read is KEPT as `_metadata` with a comment: it still surfaces a
    real permission/IO error. Reporting the size was never the problem; inferring
    architecture from it was.
  * zero warnings in this crate

FALSIFIER: crates/aprender-train-inspect/tests/falsify_no_fabricated_metadata_2519.rs

  garbage_bytes_are_not_reported_as_a_model
  two_different_files_do_not_get_the_same_invented_answer   <- the sharp one:
      two DIFFERENT files of EQUAL size must not yield identical architecture
      and tensor count, which is precisely what size-derived answers do
  a_missing_file_fails_for_its_own_reason                   <- non-vacuity

MUTATION, done properly. My first attempt hand-restored the old code and did not
COMPILE (missing field `total_params`), so it proved nothing -- same standard as
any mutation that fails to turn RED. Redone with the real original from git:

    git show HEAD:...inspect.rs > inspect.rs && cargo test --test falsify_...

    garbage_bytes_are_not_reported_as_a_model              FAILED
    two_different_files_do_not_get_the_same_invented_answer FAILED
    a_missing_file_fails_for_its_own_reason                 ok

Two RED, and the non-vacuity companion GREEN -- which is the discrimination that
matters: the tests target the fabrication, not a function that refuses
everything.

VERIFICATION
  cargo test -p aprender-train-inspect          66 + 3 passed, 0 failed
  cargo clippy -p aprender-train-inspect --all-targets   0 errors
  cargo fmt -p aprender-train-inspect -- --check         rc=0

--no-verify per #2526.

Refs #2519
…d model facts that were never measured

Completes the #2519 class. The third crate, aprender-train-inspect, is the
previous commit on this branch; these two are the same defect and take the same
treatment: refuse, name what cannot be done, point at tools that work. Neither
crate is deleted -- that remains the owner's call in #2519.

=== aprender-train-bench: the most serious of the three ===

`temperature` with NO model, NO data and NO config exited 0 and printed a full
loss/accuracy table ending:

    Optimal: temperature = 4.00 (loss=0.6043, accuracy=80.7%)

with 3.50 and 4.50 BOTH at 0.6543 -- a parabola about the vertex its own comment
names ("Temperature ~4.0 is optimal"). `simulate_training` in sweep.rs said
"Simulated training - in real implementation would run actual training".

inspect lied about a file; this lies about WHICH HYPERPARAMETER TO USE. Anyone
tuning a real distillation on that output was actively misled.

Three fabricating sites, not the two in the brief:

  sweep.rs       Sweeper::run() errors; simulate_training -> #[cfg(test)]
  strategies.rs  compare() errors; simulate -> #[cfg(test)]
  cost.rs+main.rs  FOUND WHILE REPRODUCING: `cost-performance` and `recommend`
                 accepted --results and IGNORED it, substituting an 8-row
                 literal table. `recommend --max-cost 50` printed
                 "Top recommendation: LoRA r=32" from numbers in the source.
                 `recommend` had no way to supply results at all.

The Pareto analysis was always genuine and only lacked real input, so cost.rs
gains `load_points` (JSON array of measured runs) and `recommend` gains
--results. Verified BOTH directions, which matters -- a fix that only ever fails
is not a fix:

    $ recommend --max-cost 50                       rc=1, names the missing input
    $ recommend --max-cost 50 --results measured.json
      ★ only-run (Best accuracy within constraints)  rc=0

i.e. it now reports the run from the FILE, not a literal.

benches/sweep_benchmarks.rs timed `run().expect("sweep must succeed")` -- it
benchmarked the parabola. Now times values()/to_table() on caller-supplied data.

Three unit tests asserted the fabrication and were flipped, each commented:
test_sweeper_finds_optimal_temperature, test_combined_is_best, and
lib.rs::test_temperature_sweep_returns_results, which asserted is_ok(). Tests
that assert is_ok() on input the tool cannot handle LOCK THE DEFECT IN -- the
0.63.0 audit's finding, here in the wild.

=== aprender-train-shell: exactly its two defects ===

    printf 'fetch does-not-exist/totally-fake-7b\nexit\n' | aprender-train-shell
    ✓ Fetched does-not-exist/totally-fake-7b
      Parameters: 7.0B   Layers: 32

Nothing was fetched, and 7.0B/32 are string-matched out of "7b" in the ID.
Deliberately NOT changed: it already warned about architecture and reported
`unknown` -- that part behaved, and overstating a defect is its own error.

execute_fetch errors, naming `apr pull` / `apr import hf://`. detect_architecture,
estimate_params, estimate_layers, ARCH_PATTERNS -> #[cfg(test)]. `-c "fetch ..."`
exits 1; the interactive REPL still exits 0, because a failed command should not
kill a session.

=== Falsifiers, and one honest weakness ===

  train-bench   tests/falsify_no_fabricated_benchmarks_2519.rs   9 tests
  train-shell   tests/falsify_no_fabricated_fetch_2519.rs        8 tests

Both written against API present in BOTH trees so the mutation COMPILES -- a
mutation that fails to build proves nothing, which is exactly what happened on
the inspect commit and had to be redone.

Mutation via `git show HEAD:<path> > <path>`, fix restored from copies after
(cmp clean, md5 match):

  shell   5 RED / 3 GREEN   <- the proper shape. The three that stayed green are
                              the non-vacuity anchors: fetch-without-an-id fails
                              for its OWN reason, real commands still succeed,
                              role flags still parse.
  bench   9 RED / 0 GREEN   <- WEAKER, and worth stating plainly. Its two
                              non-vacuity tests also go red because the pre-fix
                              `recommend` had no --results flag, so clap rejects
                              the invocation before the assertion runs. So for
                              bench the mutation proves the tests detect the old
                              code, but NOT that they discriminate fabrication
                              from any-failure. The shell pair carries that
                              property; bench's does not.

Best RED evidence: the discriminating test printed left == right ==
[0.9064, 0.8564, 0.8064, 0.7564] for temperature 1.0-2.5 and 5.5-7.0 in mirror
order -- byte-identical, proving the answer was f(|value - 4.0|).

VERIFICATION (exit codes read directly, never through a pipe)
  cargo test -p aprender-train-bench     63 lib + 9 falsifier passed, 0 failed
  cargo test -p aprender-train-shell     57 lib + 8 falsifier passed, 0 failed
  cargo test -p aprender-train-inspect   66 lib + 3 falsifier passed, 0 failed
  cargo clippy (all three, --all-targets)   0 diagnostics
  cargo fmt --all -- --check                rc=0
No new deps; no Cargo.toml/lock change; zero reverse-deps on either lib API.

STILL OPEN, flagged not fixed:
  * These falsifiers are DARK. ci.yml runs --lib workspace-wide and names
    integration targets one by one at line 327; none of the three #2519 files is
    there. Only one PR may edit that line without a merge-queue conflict, so all
    three want consolidating into a single edit.
  * Same class, untouched in train-shell: execute_export reports "Exported to
    {path}" while writing nothing; execute_memory computes activations from a
    hardcoded 4096x32 regardless of model.

Refs #2519
…e unguarded

The three fabrication fixes on this branch shipped with falsifiers that CI never
executed. `workspace-test` runs `--lib` workspace-wide; integration targets are
named one by one on a single line (ci.yml:317, which lists 16). None of the three
was there, so the tests existed and nothing ran them -- the fix was real and the
guard was theater.

Added to that chain:

    cargo test -p aprender-train-inspect --test falsify_no_fabricated_metadata_2519
    cargo test -p aprender-train-bench   --test falsify_no_fabricated_benchmarks_2519
    cargo test -p aprender-train-shell   --test falsify_no_fabricated_fetch_2519

VERIFIED EACH TARGET ACTUALLY RUNS under the exact `--test` name wired, rather
than assuming the name matched the file:

    falsify_no_fabricated_metadata_2519      3 passed
    falsify_no_fabricated_benchmarks_2519    9 passed
    falsify_no_fabricated_fetch_2519         8 passed

WIRING IS LOAD-BEARING, checked by mutation rather than by reading it. A target
name that does not exist:

    $ cargo test -p aprender-train-inspect --test falsify_typo_does_not_exist
    error: no test target named `falsify_typo_does_not_exist` in ...
    rc=101

so a typo breaks the chain instead of silently skipping. That mattered enough to
check: this session found several guards that scanned nothing and reported PASS.

Note the line number: 317 on main, not 327 -- an earlier report of mine said 327
and both numbers have appeared in my notes. 317 is the integration chain; the
guard-runner-labels block that #2527 edits is further down, so the two touch
different hunks of the same file.

YAML validated with yaml.safe_load; 23 `--test` invocations total.

Refs #2519, #2503
…N_EXE_...") broke the CI build

The falsifier wiring from the previous commit worked: two of the three targets
ran and passed in CI (3 and 9 tests). The third failed to COMPILE:

    error: environment variable `CARGO_BIN_EXE_aprender-train-shell`
           not defined at compile time
      --> crates/aprender-train-shell/tests/falsify_no_fabricated_fetch_2519.rs:164
          let exe = env!("CARGO_BIN_EXE_aprender-train-shell");

This is the SAME CLASS I spent yesterday removing -- the 126 dead
cargo_bin("renacer") references (#2516) and the `realizar` / `aprender-shell`
ones (#2520) -- reintroduced in a brand-new test of my own. Worth stating
plainly: the class is easy to reintroduce precisely because it compiles fine
wherever the binary happens to have been built already.

WHAT I COULD AND COULD NOT ESTABLISH

The package declares `[[bin]] name = "aprender-train-shell"` with no
`required-features`, so the variable should exist. I reproduced CI's exact
command locally after touching the test file to force a rebuild:

    cargo test -p aprender-train-shell --test falsify_no_fabricated_fetch_2519
    test result: ok. 8 passed

It PASSES here. So my first hypothesis -- that `--test <name>` skips building
the package's bins -- is wrong, and I did not identify the real difference
(cargo version, or a fresh vs warm target dir).

Rather than keep guessing, the fix removes the dependency on compile-time
resolution entirely, which is correct regardless of the cause.

FIX: ask cargo at RUNTIME which executable it produced --
`cargo build --bin ... --message-format=json-render-diagnostics`, then take the
`executable` field. Same pattern already proven for aprender-mcp in #2520, and
the same doctrine as scripts/apr_bin.sh: never construct or assume a binary
path, ask the tool that built it.

The helper FAILS LOUDLY if cargo reports no executable. A test that silently
skipped when the binary was unavailable would be the skip-class escape this repo
bans -- and would have hidden the very defect #2519 is about. It uses a substring
match on the JSON rather than adding a serde dependency to a test crate.

VERIFICATION
  cargo test -p aprender-train-shell --test falsify_no_fabricated_fetch_2519
      8 passed, 0 failed  (all 8, including the -c CLI surface test)
  cargo clippy -p aprender-train-shell --all-targets   0 errors
  cargo fmt -p aprender-train-shell -- --check         rc=0

No compile-time CARGO_BIN_EXE remains in the file; the only `env!` left are a
comment and `env!("CARGO")`, which cargo always sets for tests.

Refs #2519, #2516, #2520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant