cli, engine: add systems format (.txt) support to CLI and expand tests

[bpowers]

Summary

• The CLI's format auto-detection defaulted .txt files to XMILE parsing, which silently failed when opening systems format models like hiring.txt. Added a Systems variant to InputFormat and auto-detect .txt extension.
• Expanded systems format integration tests: added table-driven compilation and diagnostic checks for all valid models, added simulation comparison tests for dest_cap_order/infinity/formula_deps/standalone_stocks.
• Fixed incorrect expected CSV for dest_cap_order (was never tested against actual simulation output).
• Added 3 new test models extracted from the Python systems package test suite: infinity.txt, formula_deps.txt, standalone_stocks.txt.

Test plan

• All 12 systems format simulation tests pass
• Table-driven compilation test covers all 10 valid models with zero diagnostics
• open_systems() production entry point test passes for all models
• CLI correctly auto-detects .txt as systems format
• Full pre-commit hook passes (Rust fmt/clippy/test, TS lint/build/test, pysimlin)

[claude]

Code Review

Changes reviewed:

• CLI: Added Systems input format variant with .txt auto-detection
• Tests: New simulation tests (dest_cap_order, infinity, formula_deps, standalone_stocks), table-driven compilation test, and open_systems() entry point test
• Fixed incorrect expected CSV for dest_cap_order
• 3 new test model files

Findings: None.

The CLI changes are minimal and correctly mirror the existing pattern for other formats. The test additions are well-structured — the ALL_VALID_MODELS list with table-driven compilation and entry-point tests is a good approach for catching regressions across all models. The two test helper functions (simulate_systems_file for CSV comparison, assert_systems_model_compiles for diagnostic-free compilation) use the appropriate DB paths (incremental vs non-incremental) for their respective purposes.

Overall correctness: Correct. The patch introduces no bugs and existing code/tests will not break.

[claude]

Code Review

Reviewed the full diff: CLI systems format support, visible_stocks filtering, print_filtered_tsv, test expansions, and CSV corrections.

Findings

No bugs found.

The logic is consistent with existing patterns:

• print_filtered_tsv correctly mirrors print_tsv_comparison's TIME_OFF/stop-time behavior
• visible_stocks correctly compares canonical idents (both Stock.ident and Ident<Canonical>.as_str() are canonicalized)
• Format auto-detection and dispatch are straightforward extensions of the existing pattern
• The dest_cap_order_output.csv correction aligns with the PR description (previously untested expected values)

Minor observation (non-blocking): resolve_input_format is called twice in the Simulate path (once at line 529, once inside open_model), but it's a pure function so this is just minor redundancy, not a correctness issue.

Overall Correctness

Correct. The patch is free of bugs and blocking issues. Existing code and tests will not break.

[claude]

Code Review

Reviewed the full diff: CLI systems format support, visible_stocks helper, filtered TSV output, and expanded integration tests.

Checked for issues:

• print_filtered_tsv stop-time check (row[0]) is consistent with existing print_tsv (curr[TIME_OFF] where TIME_OFF = 0)
• open_model return type threading of Result + Option<VisibleStocks> is correct
• ident_to_offset lookup correctly bridges Ident<Canonical>::as_str() keys with canon()-produced strings from visible_stocks
• Corrected dest_cap_order_output.csv is now validated by actual simulation

No bugs found. The patch is clean, well-tested, and correct.

Overall correctness: Correct ✓

[claude]

Code Review

Reviewed the full diff: CLI systems format auto-detection, filtered TSV output, visible_stocks(), and expanded test coverage.

No bugs found.

The new print_filtered_tsv and print_filtered_tsv_comparison functions follow the same patterns as the existing Results::print_tsv_comparison. The open_model return type change is properly propagated to all call sites. The LTM bypass logic (&& !ltm) correctly avoids filtering when LTM adds synthetic analysis variables. Test coverage is thorough — table-driven compilation, simulation comparison, visible_stocks unit tests, and open_systems entry point validation.

Verdict: Correct — no existing code or tests will break, and the new functionality is well-tested.

[claude]

Code Review

Reviewed the full diff: CLI systems format support, visible_stocks(), filtered TSV output, comparison output, new test models, and expanded integration tests.

No bugs found.

The implementation is straightforward and well-tested:

• visible_stocks() filtering logic correctly handles both bracket-syntax ([A]) and explicit A(inf) infinite stocks
• print_filtered_tsv and print_filtered_tsv_comparison correctly use canonical ident lookups and handle missing reference offsets gracefully
• The open_model return type change is propagated correctly to all call sites
• The LTM guard (&& !ltm) correctly preserves unfiltered output when LTM adds synthetic analysis variables
• The .csv extension detection for comma-delimited reference files is a sensible addition

Overall correctness: Correct. The patch is free of bugs and blocking issues. Existing code and tests will not break.

[claude]

Code Review

Reviewed the full diff: CLI systems format support (InputFormat::Systems, .txt auto-detection, filtered TSV output), visible_stocks() in the engine, expanded integration tests, and new test models.

No bugs found.

The implementation is clean and well-tested:

• open_model return type change is handled consistently across all subcommands.
• Filtered TSV output correctly handles missing reference columns (empty cells, not misaligned).
• visible_stocks filter logic correctly matches the documented Python systems behavior (bracket syntax vs explicit inf).
• The dest_cap_order_output.csv correction is validated by the new simulation test.
• CSV comma-delimiter detection in debug command is a reasonable addition.

Overall correctness: correct. The patch is free of bugs and blocking issues.

[claude]

Code Review

Reviewed the full diff: CLI systems format support, filtered TSV output, new test models, and expanded integration tests.

Findings

No bugs found. The implementation is clean and well-tested:

• open_model correctly threads the VisibleStocks through all code paths, with None for non-systems formats.
• print_filtered_tsv and print_filtered_tsv_comparison correctly handle the offset lookups — the former via string-keyed HashMap, the latter via typed Ident<Canonical> lookups. Both approaches are correct given Ident<Canonical>::as_str() returns the inner string.
• The visible_stocks function correctly filters on is_infinite and preserves declaration order.
• The dest_cap_order_output.csv correction aligns expected output with actual simulation results.
• Test coverage is thorough: table-driven compilation for all valid models, simulation comparison tests, visible_stocks unit tests covering infinite/non-infinite edge cases, and open_systems entry point validation.

Overall Correctness

Correct. The patch introduces no regressions and is free of blocking issues.

[claude]

Code Review

Reviewed the full diff for CLI systems format support and expanded tests.

Findings

No bugs found that meet the review threshold. The implementation is clean and well-tested.

Notes

• The open_model return type change to (DatamodelProject, Option<VisibleStocks>) is a reasonable way to thread the visible stocks through without changing the other format paths.
• print_filtered_tsv silently drops any visible stock whose canonical ident isn't found in the results offsets (line 260). This is fine for the current use case but could be confusing if a model has translation issues — the column would just vanish without warning. Not a bug, just a design choice worth noting.
• The test visible_stocks_includes_time_named_stock correctly documents that a user stock named "Time" would be deduplicated away by print_filtered_tsv (since it skips canonical_ident == "time"). The stock's simulation data exists in results but wouldn't appear in filtered output. This is an extreme edge case and the test explicitly acknowledges the consumer's responsibility to handle it.
• The dest_cap_order_output.csv correction looks consistent with the PR description (previously untested against actual simulation output).

Overall Correctness

Correct. The patch is free of bugs and blocking issues. Existing code and tests will not break. The new functionality is well-covered by the added tests.

[bpowers]

Review cycle summary

This PR went through 8 review iterations with both codex and Claude reviewers. The improvements made during the review cycle:

Output quality (iterations 2-3): The initial implementation exposed all internal translator scaffolding variables in CLI output. Codex flagged this as inconsistent with the Python systems package behavior. Added visible_stocks() to extract non-infinite stocks in declaration order, and print_filtered_tsv/print_filtered_tsv_comparison to produce clean output with original stock names. Later refined to use the parsed SystemsModel directly (preserving declaration order and original casing) rather than the translated project.

Edge case correctness (iterations 4-7): Fixed several edge cases identified by codex: the debug subcommand now detects .csv extension for comma-separated reference files; stocks named "time" are deduplicated against the synthetic time column to prevent duplicate headers; the time column uses hardcoded offset 0 rather than name-based lookup; and the A(inf) initial-value filter was corrected to match the Python systems behavior (only bracket-syntax [A] stocks are hidden, not explicit A(inf)).

Three pre-existing issues were discovered and filed during review: _max helper name collision in the translator, missing --rounds flag for systems format in the CLI, and reserved identifier validation for engine-implicit variable names (time, dt, etc.).

[codecov]

Codecov Report

❌ Patch coverage is 50.60241% with 82 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.24%. Comparing base (1d34f41) to head (660c1b3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simlin-cli/src/main.rs	0.00%	75 Missing ⚠️
src/simlin-engine/tests/simulate_systems.rs	91.95%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
- Coverage   81.31%   81.24%   -0.07%     
==========================================
  Files         172      172              
  Lines       47797    47951     +154     
==========================================
+ Hits        38865    38958      +93     
- Misses       8932     8993      +61     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bpowers]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 660c1b347c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep Time-named stocks in filtered systems output

Dropping any visible stock whose canonical ident is time removes a valid user-authored stock from CLI output instead of just disambiguating headers. The new tests explicitly allow this case (visible_stocks_includes_time_named_stock), but simulate now omits that stock entirely (e.g., Time(5) > B @ 1 only prints time and B), so users cannot inspect or compare a real model variable that affects dynamics. Please preserve the stock column (using its original display name) rather than skipping it.

Useful? React with 👍 / 👎.