Quality of life improvements for MFC toolchain #1118
+12,817
−1,021
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add validate command for pre-flight case validation without building - Add clean command implementation (was previously missing) - Show detailed CMake/compiler errors on build failure with formatted panels - Add build progress indicators (Configuring/Building/Installing status) - Display human-readable test names prominently in test output - Add real-time test failure feedback with helpful hints - Enhanced case validator error messages with suggestions - Add --debug-log flag for troubleshooting - Create troubleshooting documentation Co-Authored-By: Claude Opus 4.5 <[email protected]>
Test Summary: - Rich panel with formatted pass/fail/skip counts - Total test duration display - Failed test details with UUIDs and error types - Helpful "Next Steps" suggestions for failures Case Template Generator (./mfc.sh init): - Create new cases from built-in templates (1D/2D/3D_minimal) - Copy from any example with --template example:<name> - List available templates with --list - Well-documented case files with clear parameter sections Co-Authored-By: Claude Opus 4.5 <[email protected]>
…oarding - Add ./mfc.sh completion command to install shell tab-completion - Installs to ~/.local/share/mfc/completions/ (works across MFC clones) - Automatically configures ~/.bashrc or ~/.zshrc - Supports install/uninstall/status subcommands - Add enhanced help output with Rich formatting - Add contextual tips after build/test failures - Add interactive menu mode (./mfc.sh interactive) - Add welcome message for first-time users with getting started guide - Add bash and zsh completion scripts - Update README with toolchain features documentation Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove unused Text import from rich.text - Add pylint disable for too-many-locals in test() - Add pylint disable for too-many-arguments in _print_test_summary() - Rename failed_tests param to failed_test_list to avoid shadowing - Prefix unused skipped_cases param with underscore - Add pylint disable for global-statement in setup_debug_logging() Lint score: 10.00/10 Co-Authored-By: Claude Opus 4.5 <[email protected]>
1. Enhanced subcommand help (./mfc.sh build --help): - Shows Rich-formatted header with command description - Displays practical examples with explanations - Lists key options in easy-to-read format - Still shows full argparse options below 2. Topic-based help (./mfc.sh help <topic>): - gpu: GPU configuration, OpenACC/OpenMP, troubleshooting - clusters: HPC cluster setup (Phoenix, Frontier, etc.) - batch: Batch job submission options and examples - debugging: Debug logging, validation, common issues 3. Command aliases for faster typing: - b = build - r = run - t = test - v = validate - c = clean 4. Consistent --help behavior: - ./mfc.sh --help now shows enhanced help (same as ./mfc.sh) - Aliases shown in command table Updated bash/zsh completion scripts to support new commands. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The 'load' command must be sourced to set environment variables in the current shell. Running it directly (./mfc.sh load) now shows a clear error message with the correct usage: source ./mfc.sh load -c p -m g Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add Unicode progress bar during first-time pip package installation Shows "│████████░░░░░░░░░░░░│ 53% (37 pkgs, 1m23s)" in terminals Falls back to milestone updates when output is piped - Defer list_cases() call in args.py until test command is actually used Previously generated all ~200 test cases on every startup - Make pyrometheus and cantera imports lazy in run/input.py These heavy chemistry packages are now only imported when needed These changes fix the apparent "hang" on first run where pip install showed no progress, and significantly speed up startup time for non-test commands. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Progress bar now tracks three phases: resolving (0-60%), building wheels (60-80%), and installing (80-100%) - Add "Starting..." message in mfc.sh for immediate user feedback - Non-TTY mode now shows phase transitions (building, installing) - Better status messages showing current operation Co-Authored-By: Claude Opus 4.5 <[email protected]>
Display the currently processing package (collecting, downloading, or building) on a second line below the progress bar. The display updates in real-time as pip processes each package. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Use lazy imports in main.py to defer loading heavy modules - Remove heavy imports from args.py (run.run, build, test.cases) - Hardcode target names and template names to avoid import chain - Import test.cases only when running test command This reduces startup time from ~1.4s to ~1.0s by avoiding loading mako, pygments, and other heavy dependencies until they're needed. Co-Authored-By: Claude Opus 4.5 <[email protected]>
uv is a blazing fast Python package installer (10-100x faster than pip). When uv is detected, use it for package installation (~7 seconds vs several minutes with pip). Falls back to pip with progress bar when uv is not available. Users can install uv with: pip install uv Co-Authored-By: Claude Opus 4.5 <[email protected]>
Instead of requiring users to manually install uv, automatically bootstrap it in the venv using pip. This gives all users the speed benefit of uv (~23 seconds for fresh install vs several minutes with pip alone). The bootstrap process: 1. Create venv 2. Install uv via pip (~2-3 seconds) 3. Use uv to install all ~70 packages (~7 seconds) Falls back to pip with progress bar if uv installation fails. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The exact number of packages varies, so just say "Installing Python packages" without specifying a count. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Instead of hiding uv's output, show its native progress display when running in an interactive terminal. This lets users see what's happening during installation instead of appearing to hang. For non-interactive (CI) environments, output is still captured for logs. Co-Authored-By: Claude Opus 4.5 <[email protected]>
On HPC systems where cache and venv are on different filesystems, uv's hardlink attempts fail and fall back to copying. Setting UV_LINK_MODE=copy skips the failed hardlink attempts, reducing overhead and eliminating the warning message. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove early exit for 'init' command in mfc.sh that prevented help from showing - Fix _handle_enhanced_help to properly show both enhanced help and argparse help for all subcommands - Add subparser map to print argparse help for specific commands Now ./mfc.sh init --help (and other subcommands) properly show help. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Make build imports lazy in case.py and run/input.py to break circular import chains that were exposed by the lazy import optimizations in main.py and args.py. - case.py: lazy import build in get_inp() and get_fpp() - run/input.py: lazy import build in generate_inp(), validate_constraints(), and clean() Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Pass --help through to Python for bash-handled commands (clean, lint, format, load, spelling) so enhanced help is shown - Add missing commands to COMMANDS dict in user_guide.py: bench, completion, lint, format, spelling Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use argparse's built-in aliases= parameter instead of separate parsers for command aliases (b, r, t, v, c). This ensures aliases inherit all arguments from their parent commands. Previously, ./mfc.sh t would fail because the 't' parser didn't have test-specific arguments like --rdma-mpi. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Parse ninja's [X/Y] output to show compile progress - Display progress bar with: file count, percentage, current file, elapsed time - Falls back to spinner with elapsed time for non-TTY environments - Shows "(build took Xs)" for builds longer than 5 seconds In interactive terminals, users now see: Building simulation [████████░░░░] 42/156 • 0:02:34 m_rhs.fpp.f90 In non-interactive mode: Building simulation... (build took 234.5s) ✓ Built simulation Co-Authored-By: Claude Opus 4.5 <[email protected]>
The 'init' command was originally used to bootstrap the Python environment without running any command. This restores that behavior and renames the case template creation to 'new': - ./mfc.sh init -> Just bootstrap venv/environment, then exit - ./mfc.sh new -> Create a new case from a template Updated files: - mfc.sh: Restore original init behavior (exit after python.sh) - args.py: Rename init parser to new - main.py: Handle 'new' command instead of 'init' - user_guide.py: Update all references from init to new - init.py: Update usage messages to use 'new' Co-Authored-By: Claude Opus 4.5 <[email protected]>
When --verbose is used, bypass the progress bar and show raw cmake/compiler output. This applies to both configure and build steps. Previously, verbose mode would still capture all output in the progress bar, making the --verbose flag ineffective. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Replace black with autopep8 due to Python 3.12.5 memory safety issues - Fix bash completion to allow file path navigation after commands - Fix format.sh to accept custom path arguments - Fix pylint errors in build.py and main.py - Reformat all Python files with autopep8 for CI consistency Co-Authored-By: Claude Opus 4.5 <[email protected]>
Implement a declarative CLI schema that eliminates the need to manually update 8+ files when adding commands or options: - Add cli/schema.py with dataclass definitions for CLI structure - Add cli/commands.py as SINGLE SOURCE OF TRUTH for all commands - Add cli/argparse_gen.py to generate argparse parsers from schema - Add cli/completion_gen.py to generate bash/zsh completions - Add generate.py and ./mfc.sh generate command Modified files to use the schema: - args.py: Now uses generated parser from CLI schema - user_guide.py: Imports COMMANDS from cli/commands.py - Completion scripts are now auto-generated, not hand-edited Adding a new command now requires editing only cli/commands.py, then running ./mfc.sh generate to update completion scripts. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Extract helper functions to reduce complexity in completion_gen.py and argparse_gen.py. Add pylint disable for dataclasses that legitimately need many attributes. Code now scores 10.00/10 on pylint. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add cli/docs_gen.py to generate CLI reference markdown from schema - Update generate.py to also produce docs/cli-reference.md - Add generate --check step to lint-toolchain CI workflow - Generated documentation includes quick reference table, all commands with options, examples, and common options section Co-Authored-By: Claude Opus 4.5 <[email protected]>
This adds toolchain/mfc/params_tests/ with tools to capture and verify validation behavior before refactoring case_validator.py: - inventory.py: Export all 3300 parameters with types and stages - snapshot.py: Capture validation results from 134 example cases - coverage.py: Analyze 306 constraints across 56 check methods - runner.py: CLI for build/verify/summary commands Run `python -m mfc.params_tests.runner build` to capture baseline, then `python -m mfc.params_tests.runner verify` after changes to detect any unintended changes to validation behavior. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Extends the safety net with two new testing approaches: 1. negative_tests.py - Hand-crafted test cases that intentionally violate constraints (20 tests, 100% trigger rate) 2. mutation_tests.py - Automatically mutates valid example cases to test validator coverage (830 mutations, 82.8% catch rate) Mutation testing found real validator gaps: - t_step_save: No validation for 0 or negative values - mpp_lim/cyl_coord: Invalid strings like "X" not caught - x_domain%beg/end: Missing value not validated Run with: python -m mfc.params_tests.runner negative python -m mfc.params_tests.runner mutation Co-Authored-By: Claude Opus 4.5 <[email protected]>
Mutation testing found several gaps where the validator allowed invalid parameter values. This commit fixes them: 1. Add check_parameter_types() - validates Fortran logicals are 'T'/'F' - mpp_lim, cyl_coord, bubbles_euler, etc. now reject 'X', 'yes', etc. 2. Add x_domain validation - x_domain%beg/end required when m > 0 3. Fix t_step_save - now validates t_step_save > 0 4. Fix dt validation - now validates dt > 0 in all modes where dt is set 5. Improve time stepping validation - properly handles cfl_dt, cfl_adap_dt, and adap_dt modes Mutation test results improved from 82.8% to 99.9% catch rate. All 134 example cases still pass validation. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Implement a central parameter registry that serves as the single source of truth for MFC's ~3,300 case parameters. This eliminates manual synchronization between case_dicts.py, case_validator.py, and documentation. Key changes: - Create mfc/params/ package with schema definitions and registry - Refactor case_dicts.py from ~680 lines to ~115 lines (uses registry) - Add parameter definitions organized by domain (core, weno, patches, etc.) - Add code generators for case_dicts, validator, and documentation - Add physics validation for patches, acoustics, and bubbles The registry provides: - ParamDef dataclass for parameter metadata (name, type, stages, description) - ConstraintRule dataclass for validation constraints (26 defined) - Generators that produce case_dicts-compatible output - Documentation generator (~194K chars of markdown) - Comparison tool to verify registry matches original All tests pass: - Safety net: 134/134 cases unchanged - Mutation tests: 99.9% catch rate - JSON schema validation: working Co-Authored-By: Claude Opus 4.5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.typos.toml (1)
14-31:⚠️ Potential issue | 🟠 MajorRemove the duplicate
izkey to avoid invalid TOML.
izis defined twice in the same table (Line 14 and Line 31). TOML disallows duplicate keys and this can break the typos config parsing.✅ Suggested fix (keep a single `iz` entry)
@@ -iz = "iz" @@ iz = "iz"
🤖 Fix all issues with AI agents
In `@docs/documentation/case.md`:
- Around line 455-456: Replace the misspelled word "discritize" with
"discretize" in the sentence that reads 'Models and assumptions that are used to
formulate and discritize the governing equations...' in the documentation
(update the text to 'discretize' while preserving the rest of the sentence and
existing citations to Bryngelson et al. (2019) and Coralic (2015)).
- Line 960: The internal reference in the sentence about `hyper_cleaning` is
incorrect; replace `@ref references.md` with `@ref references` so the
cross-reference resolves consistently; update the line mentioning
`hyper_cleaning` (which references `mhd`, `div B`, and `riemann_solver = 4`) to
use `@ref references` instead of `@ref references.md`.
- Line 798: The sentence "Implementation of the parameters into the model follow
[Ando (2010)](`@ref` references)." has a subject/verb agreement error; change
"follow" to "follows" so it reads "Implementation of the parameters into the
model follows [Ando (2010)](`@ref` references)." Ensure only that verb is
corrected and punctuation remains unchanged.
🧹 Nitpick comments (1)
toolchain/mfc/user_guide.py (1)
676-748: LGTM!The interactive handlers are well-structured and consistently use
_run_mfc_commandfor safe command execution.Optional enhancement: Consider adding lightweight input validation for freeform fields (
name,template,path) to reject obviously suspicious patterns (e.g., containing.., null bytes, or shell-significant characters) as defense-in-depth. This is low priority given the current subprocess.run usage.,
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Contributor
|
CodeAnt AI Incremental review completed. |
- Add hyper_cleaning (LOG, COMMON) to parameter registry - Add hyper_cleaning_speed (REAL, SIMULATION) to parameter registry - Add hyper_cleaning_tau (REAL, SIMULATION) to parameter registry - Fix typo: "discritize" → "discretize" in case.md - Fix grammar: "follow" → "follows" in case.md - Fix broken Doxygen link: @ref references.md → @ref references - Remove duplicate iz entry in .typos.toml Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add @lru_cache to params_for_stage() to avoid repeated iteration over ~3300 parameters during module import - Clarify comment: variable_dt is for CFL-based modes (cfl_dt, cfl_adap_dt) that use t_stop/t_save, NOT adap_dt which uses t_step_* for termination Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@toolchain/mfc/case_validator.py`:
- Around line 1393-1395: num_fluids can be None (valid for model_eqns=1) which
later causes range(1, num_fluids + 1) to raise TypeError; update the assignment
for num_fluids (the variable retrieved via self.get('num_fluids', 1)) to coerce
None to an integer default (e.g., set num_fluids = self.get('num_fluids', 1) or
1 or explicitly if num_fluids is None: num_fluids = 1) and optionally ensure
it's an int so the loop using range(1, num_fluids + 1) in the validation logic
will not fail.
🧹 Nitpick comments (1)
toolchain/mfc/run/case_dicts.py (1)
33-42: The@lru_cacheon nested function provides no benefit here.The cache is local to each invocation of
_load_stage_dicts(), and within that single call, each(stage, include_common)pair is queried exactly once—resulting in zero cache hits. The docstring claim about caching is misleading.If caching across module reloads or repeated calls is intended, consider moving
params_for_stageto module level. Otherwise, remove the@lru_cacheand update the docstring.♻️ Option 1: Remove ineffective caching
def _load_stage_dicts(): """ Load parameter definitions from the central registry. - Returns dicts mapping parameter names to their ParamType. - Uses caching to avoid repeated iteration over ~3300 parameters. + Returns dicts mapping parameter names to their ParamType. """ - from functools import lru_cache from ..params import REGISTRY from ..params.schema import Stage - `@lru_cache`(maxsize=8) def params_for_stage(stage, include_common=True): """Get params for a stage as {name: ParamType} dict."""
Comment on lines
+1393
to
+1395
| num_patches = self.get('num_patches', 0) | ||
| num_fluids = self.get('num_fluids', 1) | ||
| bubbles_euler = self.get('bubbles_euler', 'F') == 'T' |
Contributor
There was a problem hiding this comment.
Handle num_fluids being None to prevent TypeError.
If num_fluids is explicitly None in the params (valid for model_eqns=1), the loop at line 1429 will raise TypeError: unsupported operand type(s) for +: 'NoneType' and 'int' when evaluating range(1, num_fluids + 1).
🐛 Proposed fix
def check_patch_physics(self): # pylint: disable=too-many-locals,too-many-branches
"""Checks physics constraints on patch initial conditions (pre-process).
...
"""
num_patches = self.get('num_patches', 0)
- num_fluids = self.get('num_fluids', 1)
+ num_fluids = self.get('num_fluids') or 1 # Default to 1 if None or 0
bubbles_euler = self.get('bubbles_euler', 'F') == 'T'
num_ibs = self.get('num_ibs', 0) or 0 # IBM (Immersed Boundary Method)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| num_patches = self.get('num_patches', 0) | |
| num_fluids = self.get('num_fluids', 1) | |
| bubbles_euler = self.get('bubbles_euler', 'F') == 'T' | |
| num_patches = self.get('num_patches', 0) | |
| num_fluids = self.get('num_fluids') or 1 # Default to 1 if None or 0 | |
| bubbles_euler = self.get('bubbles_euler', 'F') == 'T' |
🤖 Prompt for AI Agents
In `@toolchain/mfc/case_validator.py` around lines 1393 - 1395, num_fluids can be
None (valid for model_eqns=1) which later causes range(1, num_fluids + 1) to
raise TypeError; update the assignment for num_fluids (the variable retrieved
via self.get('num_fluids', 1)) to coerce None to an integer default (e.g., set
num_fluids = self.get('num_fluids', 1) or 1 or explicitly if num_fluids is None:
num_fluids = 1) and optionally ensure it's an int so the loop using range(1,
num_fluids + 1) in the validation logic will not fail.
- Add --feature flag to show params by physics feature (mhd, bubbles, weno, etc.) - Add -F/--features to list all available feature groups - Search descriptions by default for better discoverability - Add --names-only flag to search only parameter names - Add FEATURE_GROUPS mapping in descriptions.py for 16 physics features - Add descriptions for hyper_cleaning MHD parameters Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
toolchain/mfc/params/descriptions.py
Outdated
Comment on lines
784
to
801
| for pattern in patterns: | ||
| if pattern.endswith("%"): | ||
| # Prefix pattern: match anything starting with this | ||
| # Strip index from param name for comparison | ||
| base_name = re.sub(r"\(\d+\)", "", param_name) | ||
| if base_name.startswith(pattern[:-1]): | ||
| matches.append(param_name) | ||
| break | ||
| elif "%" in pattern: | ||
| # Partial pattern like "patch_icpp%Bx" | ||
| # Match patch_icpp(N)%Bx | ||
| base_name = re.sub(r"\(\d+\)", "", param_name) | ||
| if base_name == pattern or base_name.startswith(pattern): | ||
| matches.append(param_name) | ||
| break | ||
| else: | ||
| # Exact match | ||
| if param_name == pattern: |
Contributor
There was a problem hiding this comment.
✅ Suggestion: The feature-parameter matching logic only normalizes parameter names (stripping numeric indices in parentheses) for patterns containing '%', so feature entries that use plain base names like "chem_wrt_Y" or "alpha_wrt" will not match indexed parameters such as "chem_wrt_Y(1)" or "alpha_wrt(1)", causing those parameters to be omitted from the corresponding feature groups. Normalizing every parameter name once per iteration and comparing against both the raw and normalized name for exact-match patterns fixes this and makes the matching consistent. [logic error]
Severity Level: Major ⚠️
- ❌ params_cmd.py feature discovery omits indexed parameters
- ⚠️ CLI parameter search returns incomplete results
- ⚠️ Auto-completion may miss indexed parameters
Suggested change
| for pattern in patterns: | |
| if pattern.endswith("%"): | |
| # Prefix pattern: match anything starting with this | |
| # Strip index from param name for comparison | |
| base_name = re.sub(r"\(\d+\)", "", param_name) | |
| if base_name.startswith(pattern[:-1]): | |
| matches.append(param_name) | |
| break | |
| elif "%" in pattern: | |
| # Partial pattern like "patch_icpp%Bx" | |
| # Match patch_icpp(N)%Bx | |
| base_name = re.sub(r"\(\d+\)", "", param_name) | |
| if base_name == pattern or base_name.startswith(pattern): | |
| matches.append(param_name) | |
| break | |
| else: | |
| # Exact match | |
| if param_name == pattern: | |
| # Strip numeric indices in parentheses for consistent matching | |
| base_name = re.sub(r"\(\d+\)", "", param_name) | |
| for pattern in patterns: | |
| if pattern.endswith("%"): | |
| # Prefix pattern: match anything starting with this | |
| if base_name.startswith(pattern[:-1]): | |
| matches.append(param_name) | |
| break | |
| elif "%" in pattern: | |
| # Partial pattern like "patch_icpp%Bx" | |
| # Match patch_icpp(N)%Bx | |
| if base_name == pattern or base_name.startswith(pattern): | |
| matches.append(param_name) | |
| break | |
| else: | |
| # Exact match (also allow match against base name without indices) | |
| if param_name == pattern or base_name == pattern: |
Steps of Reproduction ✅
1. Open the repository Python REPL or run a small script that imports the function defined
at toolchain/mfc/params/descriptions.py:766 (function get_feature_params).
2. Call get_feature_params("chemistry", ["chem_wrt_Y(1)"]) from the REPL (this simulates a
registry containing an indexed parameter chem_wrt_Y(1)). Expectation: the feature group
"chemistry" (FEATURE_GROUPS entry) lists "chem_wrt_Y" but the function returns [] (missing
the indexed parameter).
3. Trace execution inside toolchain/mfc/params/descriptions.py between lines 766–805: the
loop over all_param_names runs, and for the pattern "chem_wrt_Y" the code reaches the
exact-match branch (the final else) which compares param_name == pattern (existing code)
and does not compare the normalized base name without "(1)". As a result, "chem_wrt_Y(1)"
is not considered a match and is omitted.
4. Verify corrected behavior by applying the improved code: re-run
get_feature_params("chemistry", ["chem_wrt_Y(1)"]) and observe the result contains
["chem_wrt_Y(1)"] because the improved code normalizes base_name once per loop and
compares base_name == pattern, causing the indexed parameter to be included.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/mfc/params/descriptions.py
**Line:** 784:801
**Comment:**
*Logic Error: The feature-parameter matching logic only normalizes parameter names (stripping numeric indices in parentheses) for patterns containing '%', so feature entries that use plain base names like "chem_wrt_Y" or "alpha_wrt" will not match indexed parameters such as "chem_wrt_Y(1)" or "alpha_wrt(1)", causing those parameters to be omitted from the corresponding feature groups. Normalizing every parameter name once per iteration and comparing against both the raw and normalized name for exact-match patterns fixes this and makes the matching consistent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+21
to
+23
| show_features = ARG("features") | ||
| feature_name = ARG("feature") | ||
| names_only = ARG("names_only") |
Contributor
There was a problem hiding this comment.
Suggestion: The new optional CLI flags ("features", "feature", and "names_only") are read via ARG without defaults, so if the argument registry does not define these keys (e.g., in older tests or non-CLI callers), calling params() will raise a KeyError instead of simply treating the options as disabled; they should default to False/None to avoid spurious crashes. [possible bug]
Severity Level: Major ⚠️
- ❌ params CLI command crashes with KeyError when flags missing.
- ⚠️ Direct Python callers importing params_cmd may raise KeyError.
- ⚠️ Batch scripts/tests invoking params() programmatically affected.
Suggested change
| show_features = ARG("features") | |
| feature_name = ARG("feature") | |
| names_only = ARG("names_only") | |
| show_features = ARG("features", False) | |
| feature_name = ARG("feature", None) | |
| names_only = ARG("names_only", False) |
Steps of Reproduction ✅
1. Import and call the params() function directly: run Python and execute:
from toolchain.mfc.params_cmd import params; params()
This executes the ARG lookups inside params() at toolchain/mfc/params_cmd.py lines
~18-30 (the ARG calls at lines 21-23).
2. Inside params(), the code calls ARG("features"), ARG("feature"), ARG("names_only")
(toolchain/mfc/params_cmd.py:21-23). ARG delegates to the global argument registry.
3. If gARG does not contain those keys, state.ARG raises KeyError per its implementation
at toolchain/mfc/state.py:85-93 ("raise KeyError(f\"{arg} is not an argument.\")").
4. Observe a KeyError traceback originating from state.ARG (toolchain/mfc/state.py:85-93)
when params() is invoked without those keys set. This reproduces the crash
deterministically — calling params() without the three keys present triggers the error.
(This is not hypothetical: ARG explicitly raises KeyError when the key is absent.)Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/mfc/params_cmd.py
**Line:** 21:23
**Comment:**
*Possible Bug: The new optional CLI flags ("features", "feature", and "names_only") are read via ARG without defaults, so if the argument registry does not define these keys (e.g., in older tests or non-CLI callers), calling params() will raise a KeyError instead of simply treating the options as disabled; they should default to False/None to avoid spurious crashes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI Incremental review completed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@toolchain/mfc/params/descriptions.py`:
- Around line 766-804: The partial-pattern branch in get_feature_params is too
permissive: when a pattern contains "%" but doesn't end with it (patterns
variable), the code currently allows base_name.startswith(pattern) which can
wrongly match longer names; change the elif "%" in pattern branch to only
compare the stripped base_name for exact equality to the pattern (using the same
re.sub(r"\(\d+\)", "", param_name) logic already used) and remove the startswith
check so that non-wildcard patterns only match exact names.
🧹 Nitpick comments (2)
toolchain/mfc/params_cmd.py (2)
15-16: Remove unusednoqadirective.The static analysis tool flagged that the
noqa: F401directive on line 16 is unnecessary since F401 is not enabled. The import ofdefinitionsappears intentional (likely for side effects), but the noqa comment serves no purpose.🔧 Suggested fix
from .params import REGISTRY - from .params import definitions # noqa: F401 pylint: disable=unused-import + from .params import definitions # pylint: disable=unused-import
167-179: Redundant conditional branches produce identical output.Lines 168-171 and 173-176 have identical branches - both produce the same range string format regardless of whether the indices are contiguous:
if idx1_vals == list(range(min(idx1_vals), max(idx1_vals) + 1)): range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") else: range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") # Same!Either remove the contiguity check or differentiate the output (e.g., show individual values for non-contiguous ranges).
🔧 Suggested fix (simplify)
if idx1_vals: - if idx1_vals == list(range(min(idx1_vals), max(idx1_vals) + 1)): - range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") - else: - range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") + range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") if idx2_vals: - if idx2_vals == list(range(min(idx2_vals), max(idx2_vals) + 1)): - range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}") - else: - range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}") + range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}") if idx3_vals: range_parts.append(f"K={min(idx3_vals)}..{max(idx3_vals)}")
toolchain/mfc/params/descriptions.py
Outdated
Comment on lines
766
to
804
| def get_feature_params(feature_name: str, all_param_names: list) -> list: | ||
| """ | ||
| Get all parameter names matching a feature group. | ||
|
|
||
| Args: | ||
| feature_name: Name of the feature group (e.g., "mhd") | ||
| all_param_names: List of all parameter names in registry | ||
|
|
||
| Returns: | ||
| List of matching parameter names | ||
| """ | ||
| if feature_name not in FEATURE_GROUPS: | ||
| return [] | ||
|
|
||
| patterns = FEATURE_GROUPS[feature_name]["params"] | ||
| matches = [] | ||
|
|
||
| for param_name in all_param_names: | ||
| for pattern in patterns: | ||
| if pattern.endswith("%"): | ||
| # Prefix pattern: match anything starting with this | ||
| # Strip index from param name for comparison | ||
| base_name = re.sub(r"\(\d+\)", "", param_name) | ||
| if base_name.startswith(pattern[:-1]): | ||
| matches.append(param_name) | ||
| break | ||
| elif "%" in pattern: | ||
| # Partial pattern like "patch_icpp%Bx" | ||
| # Match patch_icpp(N)%Bx | ||
| base_name = re.sub(r"\(\d+\)", "", param_name) | ||
| if base_name == pattern or base_name.startswith(pattern): | ||
| matches.append(param_name) | ||
| break | ||
| else: | ||
| # Exact match | ||
| if param_name == pattern: | ||
| matches.append(param_name) | ||
| break | ||
|
|
Contributor
There was a problem hiding this comment.
Potential issue with partial pattern matching logic.
The pattern matching for feature groups has potentially confusing behavior:
-
Line 789: For prefix patterns ending with
%, the code checksbase_name.startswith(pattern[:-1])which strips the%. For pattern"bub_pp%", this becomesstartswith("bub_pp")which is correct. -
Line 796: For partial patterns like
"patch_icpp%Bx", the code checks both exact match (base_name == pattern) ANDstartswith(pattern). Thestartswithcheck would incorrectly matchpatch_icpp%Bx_somethingif such a parameter existed.
Consider using exact match only for non-wildcard patterns:
🔧 Suggested fix
elif "%" in pattern:
# Partial pattern like "patch_icpp%Bx"
# Match patch_icpp(N)%Bx
base_name = re.sub(r"\(\d+\)", "", param_name)
- if base_name == pattern or base_name.startswith(pattern):
+ if base_name == pattern:
matches.append(param_name)
break🤖 Prompt for AI Agents
In `@toolchain/mfc/params/descriptions.py` around lines 766 - 804, The
partial-pattern branch in get_feature_params is too permissive: when a pattern
contains "%" but doesn't end with it (patterns variable), the code currently
allows base_name.startswith(pattern) which can wrongly match longer names;
change the elif "%" in pattern branch to only compare the stripped base_name for
exact equality to the pattern (using the same re.sub(r"\(\d+\)", "", param_name)
logic already used) and remove the startswith check so that non-wildcard
patterns only match exact names.
- Add tags field to ParamDef schema for feature-based grouping - Add get_params_by_tag() and get_all_tags() to registry - Register tags directly at parameter definition time (single source of truth) - Remove stage-based organization, make all parameters universal - Reorganize definitions.py by feature groups instead of stages - Simplify descriptions.py to only contain feature descriptions Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@toolchain/mfc/params_cmd.py`:
- Around line 15-16: Remove the redundant noqa directive from the side-effect
import of definitions in params_cmd.py: keep the import statement "from .params
import definitions" (so the module is still imported for side effects) but
delete the trailing "# noqa: F401" while retaining the "pylint:
disable=unused-import" if you want to silence pylint; ensure REGISTRY import and
the definitions import remain otherwise unchanged.
In `@toolchain/mfc/params/definitions.py`:
- Around line 205-206: The _load function triggers Pylint R0912 (too many
branches); to unblock CI either explicitly silence that specific check on the
function or refactor into smaller helpers: add a function-level disable for
R0912 to the _load definition (e.g., keep def _load(): but add the pylint
disable for R0912) or extract independent logical blocks from _load into named
helper functions (e.g., _load_section_x, _load_validations, _load_defaults) and
call them from _load so the original function's branch count drops below the
limit; update any imports/locals used by those helpers accordingly.
In `@toolchain/mfc/params/schema.py`:
- Around line 50-72: ParamDef currently triggers Pylint R0902 for too many
instance attributes; to unblock CI add a class-level pylint disable for R0902
immediately above the dataclass definition of ParamDef (i.e., annotate the
ParamDef declaration to suppress R0902) so the linter ignores this class, or
alternatively refactor by grouping metadata into a nested dataclass used by
ParamDef (e.g., move constraints, dependencies, tags into a ParamMetadata
dataclass and replace those fields with a single metadata: ParamMetadata field)
and update references to ParamDef accordingly.
Comment on lines
+15
to
+16
| from .params import REGISTRY | ||
| from .params import definitions # noqa: F401 pylint: disable=unused-import |
Contributor
There was a problem hiding this comment.
Remove unused noqa directive.
Ruff flags # noqa: F401 on Line 16 as unused (RUF100). Keep the side-effect import, but drop the redundant noqa.
🔧 Suggested fix
-from .params import definitions # noqa: F401 pylint: disable=unused-import
+from .params import definitions # pylint: disable=unused-import📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from .params import REGISTRY | |
| from .params import definitions # noqa: F401 pylint: disable=unused-import | |
| from .params import REGISTRY | |
| from .params import definitions # pylint: disable=unused-import |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 16-16: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In `@toolchain/mfc/params_cmd.py` around lines 15 - 16, Remove the redundant noqa
directive from the side-effect import of definitions in params_cmd.py: keep the
import statement "from .params import definitions" (so the module is still
imported for side effects) but delete the trailing "# noqa: F401" while
retaining the "pylint: disable=unused-import" if you want to silence pylint;
ensure REGISTRY import and the definitions import remain otherwise unchanged.
Comment on lines
+205
to
+206
| def _load(): # pylint: disable=too-many-locals,too-many-statements | ||
| """Load all parameter definitions.""" |
Contributor
There was a problem hiding this comment.
CI lint failure: _load exceeds branch limit.
Line 205 triggers Pylint R0912. Please silence it explicitly or split _load into smaller helpers to unblock CI.
🔧 Minimal fix to unblock CI
-def _load(): # pylint: disable=too-many-locals,too-many-statements
+def _load(): # pylint: disable=too-many-locals,too-many-statements,too-many-branches🧰 Tools
🪛 GitHub Actions: Lint Toolchain
[error] 205-205: Pylint: Too many branches (R0912) in module mfc.params.definitions.
🤖 Prompt for AI Agents
In `@toolchain/mfc/params/definitions.py` around lines 205 - 206, The _load
function triggers Pylint R0912 (too many branches); to unblock CI either
explicitly silence that specific check on the function or refactor into smaller
helpers: add a function-level disable for R0912 to the _load definition (e.g.,
keep def _load(): but add the pylint disable for R0912) or extract independent
logical blocks from _load into named helper functions (e.g., _load_section_x,
_load_validations, _load_defaults) and call them from _load so the original
function's branch count drops below the limit; update any imports/locals used by
those helpers accordingly.
The Stage categorization (COMMON, PRE_PROCESS, SIMULATION, POST_PROCESS) added complexity without benefit since users run all three codes anyway. - Remove Stage enum from schema.py - Remove stages field from ParamDef - Remove _by_stage tracking from registry - Simplify case_dicts.py to just export ALL params - Update all tests to remove stage references - Update inventory.py to use tags instead of stages Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@toolchain/mfc/params_cmd.py`:
- Around line 164-173: The current logic in params_cmd.py building range_parts
uses min..max for both contiguous and non-contiguous idx1_vals/idx2_vals, hiding
gaps; modify the branches that handle idx1_vals and idx2_vals so that when the
set is contiguous (idx1_vals == list(range(min(...), max(...)+1))) you keep the
"N=min..max" / "M=min..max" format, but when non-contiguous you emit an explicit
list (e.g., "N={v1,v2,...}" or comma-separated values) instead of min..max;
update the code around the idx1_vals/idx2_vals checks and the range_parts append
logic to generate the explicit list for non-contiguous cases so users see gaps.
In `@toolchain/mfc/run/case_dicts.py`:
- Around line 68-71: ARG may raise KeyError when "case_optimization" is missing;
change the check in the block using ARG, target_name, ALL and CASE_OPTIMIZATION
so it defaults to False when the argument is absent (e.g., call ARG with a
default or catch KeyError) and then return list(ALL.keys()) when case
optimization is not enabled or target_name != "simulation"; otherwise filter
ALL.keys() by excluding CASE_OPTIMIZATION as before.
Comment on lines
+164
to
+173
| if idx1_vals: | ||
| if idx1_vals == list(range(min(idx1_vals), max(idx1_vals) + 1)): | ||
| range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") | ||
| else: | ||
| range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") | ||
| if idx2_vals: | ||
| if idx2_vals == list(range(min(idx2_vals), max(idx2_vals) + 1)): | ||
| range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}") | ||
| else: | ||
| range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}") |
Contributor
There was a problem hiding this comment.
Non‑contiguous indices are shown as contiguous ranges.
The non‑contiguous branches emit the same min..max range as contiguous sets, which hides gaps and can mislead users. Consider rendering explicit lists (or a gap-aware range format).
🧩 Suggested fix
- if idx1_vals:
- if idx1_vals == list(range(min(idx1_vals), max(idx1_vals) + 1)):
- range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}")
- else:
- range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}")
- if idx2_vals:
- if idx2_vals == list(range(min(idx2_vals), max(idx2_vals) + 1)):
- range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}")
- else:
- range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}")
+ def _format_range(label, vals):
+ if not vals:
+ return None
+ if vals == list(range(min(vals), max(vals) + 1)):
+ return f"{label}={min(vals)}..{max(vals)}"
+ return f"{label}=" + ",".join(str(v) for v in vals)
+
+ if idx1_vals:
+ range_parts.append(_format_range("N", idx1_vals))
+ if idx2_vals:
+ range_parts.append(_format_range("M", idx2_vals))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if idx1_vals: | |
| if idx1_vals == list(range(min(idx1_vals), max(idx1_vals) + 1)): | |
| range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") | |
| else: | |
| range_parts.append(f"N={min(idx1_vals)}..{max(idx1_vals)}") | |
| if idx2_vals: | |
| if idx2_vals == list(range(min(idx2_vals), max(idx2_vals) + 1)): | |
| range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}") | |
| else: | |
| range_parts.append(f"M={min(idx2_vals)}..{max(idx2_vals)}") | |
| def _format_range(label, vals): | |
| if not vals: | |
| return None | |
| if vals == list(range(min(vals), max(vals) + 1)): | |
| return f"{label}={min(vals)}..{max(vals)}" | |
| return f"{label}=" + ",".join(str(v) for v in vals) | |
| if idx1_vals: | |
| range_parts.append(_format_range("N", idx1_vals)) | |
| if idx2_vals: | |
| range_parts.append(_format_range("M", idx2_vals)) |
🤖 Prompt for AI Agents
In `@toolchain/mfc/params_cmd.py` around lines 164 - 173, The current logic in
params_cmd.py building range_parts uses min..max for both contiguous and
non-contiguous idx1_vals/idx2_vals, hiding gaps; modify the branches that handle
idx1_vals and idx2_vals so that when the set is contiguous (idx1_vals ==
list(range(min(...), max(...)+1))) you keep the "N=min..max" / "M=min..max"
format, but when non-contiguous you emit an explicit list (e.g., "N={v1,v2,...}"
or comma-separated values) instead of min..max; update the code around the
idx1_vals/idx2_vals checks and the range_parts append logic to generate the
explicit list for non-contiguous cases so users see gaps.
Descriptions are now auto-generated at registration time, providing a single source of truth in definitions.py. - Add _auto_describe() function that parses param names - Add _PREFIX_DESCS for indexed family descriptions (patch_icpp, etc.) - Add _ATTR_DESCS for common attribute descriptions (geometry, vel, etc.) - Add _SIMPLE_DESCS for simple parameter descriptions (~80 params) - Update _r() to accept optional desc= override - Update get_description() to use ParamDef.description first DESCRIPTIONS dict and PATTERNS list in descriptions.py are kept as fallback but can be removed once all params have good auto-descriptions. Co-Authored-By: Claude Opus 4.5 <[email protected]>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="toolchain/mfc/params/definitions.py">
<violation number="1" location="toolchain/mfc/params/definitions.py:239">
P3: The attribute regex in `_auto_describe` only matches lowercase letters, so uppercase attributes like `Re(1)` never use the `_ATTR_DESCS` mapping and produce degraded descriptions. Expand the regex to include uppercase letters so parameter docs resolve to the intended descriptions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| prefix_desc = _PREFIX_DESCS.get(prefix, prefix.replace("_", " ")) | ||
|
|
||
| # Check for nested index: attr(M) or attr(M, K) | ||
| attr_match = re.match(r"([a-z_]+)\((\d+)(?:,\s*(\d+))?\)", attr) |
Contributor
There was a problem hiding this comment.
P3: The attribute regex in _auto_describe only matches lowercase letters, so uppercase attributes like Re(1) never use the _ATTR_DESCS mapping and produce degraded descriptions. Expand the regex to include uppercase letters so parameter docs resolve to the intended descriptions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At toolchain/mfc/params/definitions.py, line 239:
<comment>The attribute regex in `_auto_describe` only matches lowercase letters, so uppercase attributes like `Re(1)` never use the `_ATTR_DESCS` mapping and produce degraded descriptions. Expand the regex to include uppercase letters so parameter docs resolve to the intended descriptions.</comment>
<file context>
@@ -5,13 +5,282 @@
+ prefix_desc = _PREFIX_DESCS.get(prefix, prefix.replace("_", " "))
+
+ # Check for nested index: attr(M) or attr(M, K)
+ attr_match = re.match(r"([a-z_]+)\((\d+)(?:,\s*(\d+))?\)", attr)
+ if attr_match:
+ attr_base = attr_match.group(1)
</file context>
Suggested change
| attr_match = re.match(r"([a-z_]+)\((\d+)(?:,\s*(\d+))?\)", attr) | |
| attr_match = re.match(r"([A-Za-z_]+)\((\d+)(?:,\s*(\d+))?\)", attr) |
- Add descriptions for grid stretching, body forces, output fields - Remove duplicate mpp_lim entry - Change params command default limit from 25 to unlimited Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Tab completion: Add backward-scanning for multi-value arguments (nargs="+" or "*") so completing works after the first value e.g., ./mfc.sh b -t pre_process sim<TAB> now completes to simulation - Fortran namelist: Convert T/F to .true./.false. for compatibility with strict compilers (some Mac compilers reject bare T/F) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@toolchain/mfc/cli/completion_gen.py`:
- Around line 300-345: The zsh generator (_generate_zsh_command_args) omits
several mfc_config_flags present in the bash completions; update the
mfc_config_flags branch (the block guarded by if common_set.mfc_config_flags) to
include the missing paired flags --gcov/--no-gcov, --unified/--no-unified,
--single/--no-single, --mixed/--no-mixed, and --fastmath/--no-fastmath (use the
same pattern as existing --mpi/--no-mpi and --gpu/--no-gpu entries), or better
yet refactor to centralize the common flag definitions so both bash and zsh use
the same source (e.g., move the literal flag list into a shared
constant/function and reference it from _generate_zsh_command_args and the bash
generator) so completions remain in sync.
- Around line 159-177: The current _generate_bash_command_case builds only
top-level subcommand name completion and returns early, so option/positional
completions for the chosen subcommand never run; replace the early return when
cmd.subcommands is truthy with a nested case on ${COMP_WORDS[2]} that dispatches
to the selected subcommand's completion block. Concretely, keep the subcmd_names
pattern but instead of returning, append a line like ' case
"${COMP_WORDS[2]}" in' and for each subcommand in cmd.subcommands call the same
generator logic (e.g., reuse _generate_bash_command_case or inline its logic for
that subcommand) to produce its options/positionals, then close the nested case
and only after that append the ';;' for the parent pattern so option/positional
completion of the selected subcommand runs. Ensure variables referenced
(patterns, options, cmd.subcommands, subcmd_names) match existing names and
remove the early return.
In `@toolchain/mfc/params/definitions.py`:
- Around line 282-338: The function _auto_describe has too many return
statements causing Pylint R0911; to quickly fix CI either extract chunks into
small helpers (e.g., _describe_indexed_prefix, _describe_percent_attr,
_describe_suffix_indexed) and call them from _auto_describe to reduce returns,
or add an explicit Pylint disable for this function by placing a disable comment
for R0911 on the _auto_describe definition; update references to helper names
(_describe_indexed_prefix, _describe_percent_attr, _describe_suffix_indexed) if
you choose refactoring so logic and tests remain unchanged.
🧹 Nitpick comments (2)
toolchain/mfc/cli/completion_gen.py (1)
66-70: Make the Optional type explicit in the signature.
This avoids implicit Optional per PEP 484 and aligns with typing best practices.💡 Proposed typing fix
-from typing import List, Set +from typing import List, Set, Optional ... -def _bash_completion_for_type(comp_type: CompletionType, choices: List[str] = None) -> str: +def _bash_completion_for_type(comp_type: CompletionType, choices: Optional[List[str]] = None) -> str:Please confirm the project’s minimum Python version and typing policy to ensure this aligns with your standards.
toolchain/mfc/case.py (1)
87-117: Use PEP 604 union syntax and preserve exception context with chaining.The codebase already uses
str | Nonesyntax in gen_case_constraints_docs.py, confirming Python 3.10+ support. Additionally, exception chaining withfrom eis a pattern used throughout the codebase (run/input.py, common.py), yet it's missing in the current error handling. Apply both changes for consistency:🔧 Proposed changes
- def validate_params(self, origin_txt: str = None): + def validate_params(self, origin_txt: str | None = None): @@ - except fastjsonschema.JsonSchemaException as e: - if origin_txt: - raise common.MFCException(f"{origin_txt}: {e}") - raise common.MFCException(f"{e}") + except fastjsonschema.JsonSchemaException as e: + if origin_txt: + raise common.MFCException(f"{origin_txt}: {e}") from e + raise common.MFCException(f"{e}") from e
Comment on lines
+159
to
+177
| def _generate_bash_command_case(cmd: Command, schema: CLISchema) -> List[str]: | ||
| """Generate bash completion case for a single command.""" | ||
| lines = [] | ||
|
|
||
| # Include aliases in case pattern | ||
| patterns = [cmd.name] + cmd.aliases | ||
| lines.append(f' {"|".join(patterns)})') | ||
|
|
||
| options = _collect_all_options(cmd, schema) | ||
|
|
||
| # Handle subcommands (like packer pack, packer compare) | ||
| if cmd.subcommands: | ||
| lines.append(' if [[ ${COMP_CWORD} -eq 2 ]]; then') | ||
| subcmd_names = [sc.name for sc in cmd.subcommands] | ||
| lines.append(f' COMPREPLY=( $(compgen -W "{" ".join(subcmd_names)}" -- "${{cur}}") )') | ||
| lines.append(' return 0') | ||
| lines.append(' fi') | ||
| lines.append(' ;;') | ||
| return lines |
Contributor
There was a problem hiding this comment.
Subcommand completions stop after choosing the subcommand.
When cmd.subcommands is present, the generator only completes the subcommand name (COMP_CWORD==2) and then returns, so option/positional completions for the selected subcommand never trigger. That’s a functional gap for nested commands.
Consider generating a nested case on ${COMP_WORDS[2]} to dispatch to the selected subcommand’s options and positionals rather than returning early.
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 164-164: Consider [cmd.name, *cmd.aliases] instead of concatenation
Replace with [cmd.name, *cmd.aliases]
(RUF005)
🤖 Prompt for AI Agents
In `@toolchain/mfc/cli/completion_gen.py` around lines 159 - 177, The current
_generate_bash_command_case builds only top-level subcommand name completion and
returns early, so option/positional completions for the chosen subcommand never
run; replace the early return when cmd.subcommands is truthy with a nested case
on ${COMP_WORDS[2]} that dispatches to the selected subcommand's completion
block. Concretely, keep the subcmd_names pattern but instead of returning,
append a line like ' case "${COMP_WORDS[2]}" in' and for each
subcommand in cmd.subcommands call the same generator logic (e.g., reuse
_generate_bash_command_case or inline its logic for that subcommand) to produce
its options/positionals, then close the nested case and only after that append
the ';;' for the parent pattern so option/positional completion of the selected
subcommand runs. Ensure variables referenced (patterns, options,
cmd.subcommands, subcmd_names) match existing names and remove the early return.
Comment on lines
+282
to
+338
| def _auto_describe(name: str) -> str: | ||
| """Auto-generate description from parameter name.""" | ||
| # Check simple params first | ||
| if name in _SIMPLE_DESCS: | ||
| return _SIMPLE_DESCS[name] | ||
|
|
||
| # Handle indexed params: prefix(N)%attr or prefix(N)%attr(M) | ||
| match = re.match(r"([a-z_]+)\((\d+)\)%(.+)", name) | ||
| if match: | ||
| prefix, idx, attr = match.group(1), match.group(2), match.group(3) | ||
| prefix_desc = _PREFIX_DESCS.get(prefix, prefix.replace("_", " ")) | ||
|
|
||
| # Check for nested index: attr(M) or attr(M, K) | ||
| attr_match = re.match(r"([a-z_]+)\((\d+)(?:,\s*(\d+))?\)", attr) | ||
| if attr_match: | ||
| attr_base = attr_match.group(1) | ||
| idx2 = attr_match.group(2) | ||
| attr_desc = _ATTR_DESCS.get(attr_base, attr_base.replace("_", " ")) | ||
| return f"{attr_desc} {idx2} for {prefix_desc} {idx}" | ||
|
|
||
| attr_desc = _ATTR_DESCS.get(attr, attr.replace("_", " ")) | ||
| return f"{attr_desc} for {prefix_desc} {idx}" | ||
|
|
||
| # Handle bc_x%attr style (no index in prefix) | ||
| if "%" in name: | ||
| prefix, attr = name.split("%", 1) | ||
| # Check for indexed attr | ||
| attr_match = re.match(r"([a-z_]+)\((\d+)\)", attr) | ||
| if attr_match: | ||
| attr_base, idx = attr_match.group(1), attr_match.group(2) | ||
| attr_desc = _ATTR_DESCS.get(attr_base, attr_base.replace("_", " ")) | ||
| return f"{attr_desc} {idx} for {prefix.replace('_', ' ')}" | ||
|
|
||
| attr_desc = _ATTR_DESCS.get(attr, "") | ||
| if attr_desc: | ||
| return f"{attr_desc} for {prefix.replace('_', ' ')}" | ||
| # Fallback: just clean up the name | ||
| return f"{attr.replace('_', ' ').title()} for {prefix.replace('_', ' ')}" | ||
|
|
||
| # Handle suffix-indexed: name(N) or name(N, M) | ||
| match = re.match(r"([a-z_]+)\((\d+)(?:,\s*(\d+))?\)", name) | ||
| if match: | ||
| base, idx = match.group(1), match.group(2) | ||
| # Handle _wrt patterns | ||
| if base.endswith("_wrt"): | ||
| field = base[:-4].replace("_", " ") | ||
| return f"Write {field} for component {idx}" | ||
| return f"{base.replace('_', ' ').title()} {idx}" | ||
|
|
||
| # Fallback patterns | ||
| if name.endswith("_wrt"): | ||
| return f"Write {name[:-4].replace('_', ' ')}" | ||
| if name.startswith("num_"): | ||
| return f"Number of {name[4:].replace('_', ' ')}" | ||
|
|
||
| # Last resort: clean up the name | ||
| return name.replace("_", " ").replace("%", " ") |
Contributor
There was a problem hiding this comment.
Fix CI: Pylint R0911 (too many return statements) in _auto_describe.
Line 282 triggers a CI-blocking lint error. Either refactor into helper functions or explicitly silence the check at the function definition.
🔧 Minimal fix to unblock CI
-def _auto_describe(name: str) -> str:
+def _auto_describe(name: str) -> str: # pylint: disable=too-many-return-statements🧰 Tools
🪛 GitHub Actions: Lint Toolchain
[error] 282-282: Pylint: Too many return statements (R0911).
🤖 Prompt for AI Agents
In `@toolchain/mfc/params/definitions.py` around lines 282 - 338, The function
_auto_describe has too many return statements causing Pylint R0911; to quickly
fix CI either extract chunks into small helpers (e.g., _describe_indexed_prefix,
_describe_percent_attr, _describe_suffix_indexed) and call them from
_auto_describe to reduce returns, or add an explicit Pylint disable for this
function by placing a disable comment for R0911 on the _auto_describe
definition; update references to helper names (_describe_indexed_prefix,
_describe_percent_attr, _describe_suffix_indexed) if you choose refactoring so
logic and tests remain unchanged.
Use ARG("case_optimization", dflt=False) so the function works
in test environments where CLI args aren't initialized.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
The T/F format is correct - the namelist issue was due to stale binaries, not the format of logical values. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="toolchain/mfc/case.py">
<violation number="1">
P1: Boolean parameters are no longer converted to Fortran logical literals. Emitting `T`/`F` directly makes the generated .inp invalid for Fortran logicals. Restore the `.true.`/`.false.` conversion for string booleans before falling back to the generic handling.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -1,12 +1,17 @@ | |||
| import re, json, math, copy, dataclasses, fastjsonschema | |||
| # pylint: disable=import-outside-toplevel | |||
Contributor
There was a problem hiding this comment.
P1: Boolean parameters are no longer converted to Fortran logical literals. Emitting T/F directly makes the generated .inp invalid for Fortran logicals. Restore the .true./.false. conversion for string booleans before falling back to the generic handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At toolchain/mfc/case.py, line 65:
<comment>Boolean parameters are no longer converted to Fortran logical literals. Emitting `T`/`F` directly makes the generated .inp invalid for Fortran logicals. Restore the `.true.`/`.false.` conversion for string booleans before falling back to the generic handling.</comment>
<file context>
@@ -62,12 +62,7 @@ def get_inp(self, _target) -> str:
- elif val == "F":
- dict_str += f"{key} = .false.\n"
- elif not isinstance(val, str) or len(val) == 1:
+ if not isinstance(val, str) or len(val) == 1:
dict_str += f"{key} = {val}\n"
else:
</file context>
Added --gcov, --unified, --single, --mixed, --fastmath and their --no-* variants to zsh completion for consistency with bash. Co-Authored-By: Claude Opus 4.5 <[email protected]>
When we removed the Stage system, all params were written to all targets. But some params (like run_time_info) only exist in simulation's Fortran namelist, not pre_process. This caused "Invalid line in namelist" errors. Added _SIMULATION_ONLY and _POST_PROCESS_ONLY sets to filter params that should not be written to other targets' .inp files. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@toolchain/mfc/cli/completion_gen.py`:
- Around line 275-292: The _zsh_completion_for_arg function omits
CompletionType.FILES_YAML handling; add a branch for CompletionType.FILES_YAML
(similar to the FILES_PY/FILES_PACK cases) that returns f'{label}:_files -g
"*.yaml"' (and possibly "*.yml" if desired) so argument completions support YAML
file globs; update the condition checks in _zsh_completion_for_arg to include
CompletionType.FILES_YAML.
- Around line 256-272: The _zsh_completion_for_positional function is missing
handling for CompletionType.FILES_YAML, so positional args that expect YAML
files get no zsh completion; update _zsh_completion_for_positional to include an
elif branch for CompletionType.FILES_YAML (similar to FILES_PY/FILES_PACK) that
sets completion to a file glob for YAML (e.g. ':_files -g "*.yml" -g "*.yaml"'),
ensure help_text processing remains the same and reference
CompletionType.FILES_YAML and the _zsh_completion_for_positional function when
making the change.
🧹 Nitpick comments (3)
toolchain/mfc/cli/completion_gen.py (2)
66-70: Use explicitOptionalor union syntax for nullable parameter.The
choicesparameter defaults toNonebut the type hint doesn't reflect this. PEP 484 prohibits implicitOptional.♻️ Suggested fix
-def _bash_completion_for_type(comp_type: CompletionType, choices: List[str] = None) -> str: +def _bash_completion_for_type(comp_type: CompletionType, choices: List[str] | None = None) -> str:
395-404: Minor style improvement for list construction.Similar to line 164, line 397 can use unpacking syntax per Ruff's suggestion.
♻️ Suggested fix
- all_names = [cmd.name] + cmd.aliases + all_names = [cmd.name, *cmd.aliases]toolchain/mfc/case.py (1)
89-95: Preserve schema error context with exception chaining.
Useraise ... from eto keep the original traceback for debugging.🔧 Proposed change
- except fastjsonschema.JsonSchemaException as e: - if origin_txt: - raise common.MFCException(f"{origin_txt}: {e}") - raise common.MFCException(f"{e}") + except fastjsonschema.JsonSchemaException as e: + if origin_txt: + raise common.MFCException(f"{origin_txt}: {e}") from e + raise common.MFCException(f"{e}") from e
Comment on lines
+256
to
+272
| def _zsh_completion_for_positional(pos, index: int) -> str: | ||
| """Generate zsh completion spec for a positional argument.""" | ||
| completion = "" | ||
| if pos.completion.type == CompletionType.FILES_PY: | ||
| completion = ':_files -g "*.py"' | ||
| elif pos.completion.type == CompletionType.FILES_PACK: | ||
| completion = ':_files -g "*.pack"' | ||
| elif pos.completion.type == CompletionType.CHOICES: | ||
| choices = pos.completion.choices or pos.choices or [] | ||
| completion = f':({" ".join(choices)})' | ||
| elif pos.completion.type == CompletionType.DIRECTORIES: | ||
| completion = ':_files -/' | ||
| elif pos.completion.type == CompletionType.FILES: | ||
| completion = ':_files' | ||
|
|
||
| help_text = pos.help.replace("'", "").replace("[", "").replace("]", "")[:120] | ||
| return f"'{index}:{help_text}{completion}'" |
Contributor
There was a problem hiding this comment.
FILES_YAML completion type not handled for positional arguments.
The function handles FILES_PY, FILES_PACK, FILES, DIRECTORIES, and CHOICES, but FILES_YAML (present in _BASH_COMPLETION_MAP) is missing. If YAML file positionals exist in the schema, they won't get proper zsh completion.
🐛 Suggested fix
elif pos.completion.type == CompletionType.FILES:
completion = ':_files'
+ elif pos.completion.type == CompletionType.FILES_YAML:
+ completion = ':_files -g "*.y*ml"'
help_text = pos.help.replace("'", "").replace("[", "").replace("]", "")[:120]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _zsh_completion_for_positional(pos, index: int) -> str: | |
| """Generate zsh completion spec for a positional argument.""" | |
| completion = "" | |
| if pos.completion.type == CompletionType.FILES_PY: | |
| completion = ':_files -g "*.py"' | |
| elif pos.completion.type == CompletionType.FILES_PACK: | |
| completion = ':_files -g "*.pack"' | |
| elif pos.completion.type == CompletionType.CHOICES: | |
| choices = pos.completion.choices or pos.choices or [] | |
| completion = f':({" ".join(choices)})' | |
| elif pos.completion.type == CompletionType.DIRECTORIES: | |
| completion = ':_files -/' | |
| elif pos.completion.type == CompletionType.FILES: | |
| completion = ':_files' | |
| help_text = pos.help.replace("'", "").replace("[", "").replace("]", "")[:120] | |
| return f"'{index}:{help_text}{completion}'" | |
| def _zsh_completion_for_positional(pos, index: int) -> str: | |
| """Generate zsh completion spec for a positional argument.""" | |
| completion = "" | |
| if pos.completion.type == CompletionType.FILES_PY: | |
| completion = ':_files -g "*.py"' | |
| elif pos.completion.type == CompletionType.FILES_PACK: | |
| completion = ':_files -g "*.pack"' | |
| elif pos.completion.type == CompletionType.CHOICES: | |
| choices = pos.completion.choices or pos.choices or [] | |
| completion = f':({" ".join(choices)})' | |
| elif pos.completion.type == CompletionType.DIRECTORIES: | |
| completion = ':_files -/' | |
| elif pos.completion.type == CompletionType.FILES: | |
| completion = ':_files' | |
| elif pos.completion.type == CompletionType.FILES_YAML: | |
| completion = ':_files -g "*.y*ml"' | |
| help_text = pos.help.replace("'", "").replace("[", "").replace("]", "")[:120] | |
| return f"'{index}:{help_text}{completion}'" |
🤖 Prompt for AI Agents
In `@toolchain/mfc/cli/completion_gen.py` around lines 256 - 272, The
_zsh_completion_for_positional function is missing handling for
CompletionType.FILES_YAML, so positional args that expect YAML files get no zsh
completion; update _zsh_completion_for_positional to include an elif branch for
CompletionType.FILES_YAML (similar to FILES_PY/FILES_PACK) that sets completion
to a file glob for YAML (e.g. ':_files -g "*.yml" -g "*.yaml"'), ensure
help_text processing remains the same and reference CompletionType.FILES_YAML
and the _zsh_completion_for_positional function when making the change.
Comment on lines
+275
to
+292
| def _zsh_completion_for_arg(arg) -> str: | ||
| """Generate zsh completion suffix for an argument.""" | ||
| # For multi-value args (nargs="+" or "*"), add a label before the choices | ||
| is_multivalue = arg.nargs in ("+", "*") | ||
| label = ":value" if is_multivalue else "" | ||
|
|
||
| if arg.completion.type == CompletionType.CHOICES: | ||
| choices = arg.completion.choices or arg.choices or [] | ||
| return f'{label}:({" ".join(str(c) for c in choices)})' | ||
| if arg.completion.type == CompletionType.FILES_PY: | ||
| return f'{label}:_files -g "*.py"' | ||
| if arg.completion.type == CompletionType.FILES_PACK: | ||
| return f'{label}:_files -g "*.pack"' | ||
| if arg.completion.type == CompletionType.FILES: | ||
| return f'{label}:_files' | ||
| if arg.completion.type == CompletionType.DIRECTORIES: | ||
| return f'{label}:_files -/' | ||
| return "" |
Contributor
There was a problem hiding this comment.
FILES_YAML completion type also missing for argument completions.
Same gap as in _zsh_completion_for_positional - FILES_YAML is not handled here.
🐛 Suggested fix
if arg.completion.type == CompletionType.DIRECTORIES:
return f'{label}:_files -/'
+ if arg.completion.type == CompletionType.FILES_YAML:
+ return f'{label}:_files -g "*.y*ml"'
return ""🤖 Prompt for AI Agents
In `@toolchain/mfc/cli/completion_gen.py` around lines 275 - 292, The
_zsh_completion_for_arg function omits CompletionType.FILES_YAML handling; add a
branch for CompletionType.FILES_YAML (similar to the FILES_PY/FILES_PACK cases)
that returns f'{label}:_files -g "*.yaml"' (and possibly "*.yml" if desired) so
argument completions support YAML file globs; update the condition checks in
_zsh_completion_for_arg to include CompletionType.FILES_YAML.
Split exclusions into three categories: - _SIMULATION_ONLY: params only in simulation (not pre or post) - _SIM_AND_POST: params in simulation AND post_process (not pre) - _POST_PROCESS_ONLY: params only in post_process pre_process now correctly excludes all non-pre_process params. post_process excludes only simulation-only params. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Added __ensure_generated_files() check at startup that compares mtimes of source files (commands.py, completion_gen.py, definitions.py) against generated files (mfc.bash, mfc-case-schema.json). If sources are newer, automatically regenerates the files with a brief message. This means users no longer need to manually run ./mfc.sh generate after pulling changes that modify CLI commands or parameters. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
PR Type
Enhancement, Tests, Documentation
Description
Major Quality of Life Improvements for MFC Toolchain
Centralized Parameter System: Consolidated ~3,300 MFC case parameters into unified registry with descriptions, constraints, and dependencies (
params/definitions.py,params/descriptions.py)Enhanced CLI Architecture: Refactored argument parsing with declarative schema-based command definitions, auto-generated argparse parsers, and shell completion support (
cli/schema.py,cli/argparse_gen.py,cli/completion_gen.py)Improved User Experience: Added interactive user guide with topic-based help system, contextual tips, and first-time user onboarding (
user_guide.py,completion.py)Case Template System: New case template generator with built-in minimal templates and example-based case creation (
init.py)Parameter Discovery: New CLI command for searching and exploring parameters with filtering and statistics (
params_cmd.py)Enhanced Validation: Comprehensive physics constraint validation for bubbles, patches, time stepping, and acoustic sources with improved error reporting (
case_validator.py)Build System Improvements: Added progress bars, streaming output, and detailed error reporting with contextual hints (
build.py)Test Infrastructure: Comprehensive parameter validation test suite including negative tests, mutation testing, snapshot regression testing, and coverage analysis (
params_tests/module)Documentation Generation: Auto-generated CLI reference and parameter documentation from schema definitions (
cli/docs_gen.py,params/generators/docs_gen.py)Code Quality: Fixed formatting issues across example cases and benchmarks (PEP 8 compliance for imports and comments)
Diagram Walkthrough
File Walkthrough
15 files
descriptions.py
Centralized parameter descriptions and documentation systemtoolchain/mfc/params/descriptions.py
case parameters
patch_icpp(N)%geometry)parameters
case_dicts.py
Migrate parameter definitions to central registrytoolchain/mfc/run/case_dicts.py
of hardcoded dictionaries
loading via
_load_from_registry()CASE_OPTIMIZATIONto derive from registry metadata instead ofhardcoded list
ParamTypeenum and utility functionscommands.py
Unified CLI command definitions and schematoolchain/mfc/cli/commands.py
etc.) with full argument specifications
completions, and documentation
case_validator.py
Enhanced case validation with physics constraintstoolchain/mfc/case_validator.py
check_parameter_types()to validate logical parameters are 'T'or 'F'
check_bubbles_euler()with validation for bubble physicsparameters (R0ref, p0ref, viscosities, surface tension)
check_patch_physics()to validate initial condition constraints(positive pressure, valid volume fractions, positive dimensions)
check_time_stepping()to handle multiple time stepping modes(CFL-based, adaptive, fixed)
check_acoustic_source()with physics validation (positivefrequency, wavelength, gaussian parameters)
_is_numeric()helper and_format_errors()for better errorreporting with hints
definitions.py
Consolidated parameter definitions module with constraint anddependency supporttoolchain/mfc/params/definitions.py
compact module using loops instead of a definitions/ directory
dependencies (requires/recommends relationships)
_load()function that registers all parameters acrossCOMMON, PRE_PROCESS, SIMULATION, and POST_PROCESS stages
acoustic sources, probes, and other multi-instance configurations
user_guide.py
New user guide with enhanced help and interactive onboarding systemtoolchain/mfc/user_guide.py
with Rich formatting
setup, batch jobs, and debugging
failures, and run failures
interactive menu-driven interface
init.py
New case template generator with built-in and example-based templatestoolchain/mfc/init.py
built-in or example templates
1D_minimal,2D_minimal, and3D_minimalwith documented parameterscreate_case()function to generate new cases from templatesor copy from examples directory
list_templates()to display available templates andget_available_templates()for programmatic accessargs.py
Refactored argument parsing with centralized CLI schema and enhancedhelptoolchain/mfc/args.py
centralized CLI schema instead of manual argparse setup
print_help(),print_command_help(),and topic-based help system integration
_handle_enhanced_help()to provide rich formatting beforeargparse help
users
build.py
Enhanced build system with progress bars and improved error reportingtoolchain/mfc/build.py
output parsing using regex patterns
_run_build_with_progress()function supporting streamingmode (-v) and interactive progress bars
_show_build_error()to display detailed error information fromcaptured subprocess output
(✓/✗), timing information, and contextual error tips
-vv+ (full compiler output)
completion_gen.py
New shell completion generator for bash and zsh from CLI schematoolchain/mfc/cli/completion_gen.py
schema
generate_bash_completion()andgenerate_zsh_completion()functions for auto-generated completions
file types (Python, YAML, pack files)
argument sets in completion generation
argparse_gen.py
New argparse generator from declarative CLI schematoolchain/mfc/cli/argparse_gen.py
definitions
generate_parser()to convert schema into completeArgumentParser with subcommands and argument groups
dynamically from dataclass fields
exclusive groups, and nested subcommands
params_cmd.py
Parameter search and discovery CLI commandtoolchain/mfc/params_cmd.py
MFC case parameters
indexed parameters
configurable output formats
alternative suggestions
test.py
Enhanced test reporting and failure feedbacktoolchain/mfc/test/test.py
test names prominently
visual panel formatting
helpful hints
failed_teststracking and_print_test_summary()function fordetailed failure reporting
completion.py
Shell completion installation and managementtoolchain/mfc/completion.py
~/.local/share/mfc/completions/schema.py
CLI schema dataclass definitionstoolchain/mfc/cli/schema.py
truth)
Argument,Positional,Command,CommonArgumentSet, andCLISchemadataclassesArgActionandCompletionTypewith completionconfiguration
generation
7 files
case.py
Fix comment formatting in example caseexamples/2D_shearlayer/case.py
#'to# 'for consistency with Pythonstyle guidelines
case.py
Fix import statement formattingexamples/1D_bubblescreen/case.py
case.py
Code formatting improvement for import statementsexamples/nD_perfect_reactor/case.py
import json, argparseinto separatelines for PEP 8 compliance
case.py
Code formatting and style improvementsbenchmarks/5eq_rk3_weno3_hllc/case.py
##to single#for consistencycase.py
Comment style normalizationexamples/2D_axisym_shockwatercavity/case.py
##to single#for consistencycase.py
Comment style normalizationexamples/2D_ibm_steady_shock/case.py
##to single#for consistencyexport.py
Import statement formattingexamples/scaling/export.py
6 files
negative_tests.py
Negative test case generator for validator constraintstoolchain/mfc/params_tests/negative_tests.py
violate validator constraints
BASE_CASEwith valid parameters andConstraintTestdataclassfor test specification
generate_constraint_tests()covering simulation domain,model equations, time stepping, WENO, boundary conditions, and
acoustic source constraints
run_constraint_tests()andprint_test_report()for validationand reporting
mutation_tests.py
Mutation testing for validator coverage analysistoolchain/mfc/params_tests/mutation_tests.py
parameters to verify validator coverage
MUTATIONSdictionary with invalid values for ~40 parametersacross numeric, boolean, physics, and numerics categories
tracking
by parameter
snapshot.py
Validation snapshot tool for regression testingtoolchain/mfc/params_tests/snapshot.py
cases for regression testing
CaseSnapshotandValidationResultdataclasses for storingvalidation state
and save/load snapshots to JSON
across refactoring
coverage.py
Constraint coverage analysis tooltoolchain/mfc/params_tests/coverage.py
case_validator.pyAST
self.prohibit()calls and maps them to validation stagesrunner.py
Test safety net runner and orchestratortoolchain/mfc/params_tests/runner.py
test suite
build_safety_net()to capture parameter inventory,validation snapshots, and constraint coverage
verify_safety_net()to detect unintended validation behaviorchanges
generation commands
inventory.py
Parameter inventory export and analysistoolchain/mfc/params_tests/inventory.py
stages
export_parameter_inventory()categorizing parameters bystage and type
tracking
metadata
2 files
docs_gen.py
CLI documentation generator from schematoolchain/mfc/cli/docs_gen.py
definitions
examples, and subcommands
docs_gen.py
Parameter documentation generatortoolchain/mfc/params/generators/docs_gen.py
parameters
constraints
grouping for large families
instructions
74 files
CodeAnt-AI Description
Centralize parameters, add human-readable parameter descriptions, and unify CLI commands
What Changed
Impact
✅ Clearer parameter descriptions for case authors✅ Easier pre-flight case validation (validate command + registry schema)✅ Consistent CLI experience and reliable shell tab-completion💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests & CI