src/fromager/commands/graph.py (2)

1075-1100: Consider making --json and --constraints mutually exclusive.

If both flags are specified, --constraints silently takes precedence. This could surprise users.

Option: Add a click mutex constraint

`@click.option`(
    "--json",
    "as_json",
    is_flag=True,
    default=False,
    help="Output results as JSON.",
)
`@click.option`(
    "--constraints",
    "as_constraints",
    is_flag=True,
    default=False,
    help="Output collapsible pins in constraints.txt format.",
)

You could add validation at the start of check():

if as_json and as_constraints:
    raise click.UsageError("--json and --constraints are mutually exclusive")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/fromager/commands/graph.py` around lines 1075 - 1100, The check() command
accepts both --json (as_json) and --constraints (as_constraints) but currently
lets --constraints silently win; add an explicit mutual-exclusion check at the
start of the check() function to detect if both as_json and as_constraints are
True and raise a click.UsageError (with a clear message like "--json and
--constraints are mutually exclusive"). This change should be implemented inside
the check() function body that receives wkctx, graph_file, as_json,
as_constraints so any callers get an immediate, user-facing error instead of
surprising behavior.

---

1113-1129: Avoid reading the graph file twice.

The graph JSON is loaded at line 1115, then re-read at line 1127 via from_file. Use from_dict(graph_dict) instead.

Proposed fix

     if not wf_issues:
-        graph = DependencyGraph.from_file(graph_file)
+        graph = DependencyGraph.from_dict(graph_dict)
         entries = _classify_conflicts(graph)
         n_nodes = len(graph) - 1  # exclude root

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/fromager/commands/graph.py` around lines 1113 - 1129, The code currently
loads graph_file into graph_dict and later re-reads the file via
DependencyGraph.from_file; replace that second read with
DependencyGraph.from_dict(graph_dict) to avoid double I/O: after verifying
wf_issues and acyclicity (using _check_well_formed and _check_acyclicity), call
DependencyGraph.from_dict(graph_dict) (not from_file) to build the graph used by
_classify_conflicts and to compute n_nodes, keeping the existing logic for
entries = _classify_conflicts(graph) and n_nodes = len(graph) - 1.

tests/test_graph_check.py (1)

347-360: Consider adding assertion for build_efficiency field.

The JSON output includes build_efficiency with total_wheels and extra_builds per the implementation, but no test verifies this field. Minor gap.

assert "build_efficiency" in data
assert "total_wheels" in data["build_efficiency"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_graph_check.py` around lines 347 - 360, The test_clean_graph_json
test is missing assertions for the build_efficiency section of the output;
update the test (in test_clean_graph_json) to assert that "build_efficiency"
exists in the parsed data dict and that it contains the expected keys (e.g.,
"total_wheels" and "extra_builds") and optionally that those values are
present/not-None (use the local variable data to check keys and simple
truthiness or type). Ensure assertions reference
data["build_efficiency"]["total_wheels"] and
data["build_efficiency"]["extra_builds"] so the test will fail if those fields
are absent.

src/fromager/commands/graph.py (1)

1086-1114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Recursive DFS may hit Python's recursion limit on deep dependency chains.

dfs() recurses per edge traversed; depth equals the longest dependency chain. Fromager targets deep AI/ML dependency trees, so a sufficiently long chain could raise RecursionError on what's meant to be a fast, reliable pre-rebuild gate. Consider converting to an explicit stack-based DFS.

♻️ Iterative alternative (sketch)

def dfs(start: str) -> None:
    stack = [(start, iter(graph_dict.get(start, {}).get("edges", [])))]
    color[start] = in_stack
    path.append(start)
    while stack:
        key, edges = stack[-1]
        advanced = False
        for edge in edges:
            tgt = edge["key"]
            if tgt == key:
                continue
            if tgt not in color:
                continue
            if color[tgt] == in_stack:
                cycle_start = path.index(tgt)
                cycles.append(f"CYCLE: {' -> '.join(path[cycle_start:] + [tgt])}")
            elif color[tgt] == unvisited:
                color[tgt] = in_stack
                path.append(tgt)
                stack.append((tgt, iter(graph_dict.get(tgt, {}).get("edges", []))))
                advanced = True
                break
        if not advanced:
            path.pop()
            color[key] = done
            stack.pop()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/fromager/commands/graph.py` around lines 1086 - 1114, The cycle-detection
DFS in the graph-building logic uses recursion and can overflow on long
dependency chains. Replace the recursive dfs() used in the cycle detection block
with an explicit stack-based traversal, preserving the existing
color/path/cycles behavior while avoiding RecursionError; update the
implementation around dfs, path, and the graph_dict edge walk accordingly.

tests/test_graph_check.py (1)

332-575: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

No test for missing/invalid graph file.

The CLI test class covers structural and conflict scenarios well, but nothing exercises graph check <nonexistent-file> or a malformed-JSON file — the path where open()/json.load() currently produce a raw traceback (see companion comment in src/fromager/commands/graph.py). Once that's fixed with ClickPath(exists=True)/JSON error handling, a test asserting a clean click.UsageError/ClickException would lock in the behavior.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_graph_check.py` around lines 332 - 575, Add coverage in
TestCheckCLI for invalid input paths: exercise graph check with a nonexistent
file and with a malformed JSON file. The current tests cover only valid graphs,
so add cases that invoke fromager with a bad path or bad JSON and assert a clean
ClickException/UsageError instead of a traceback. Use the existing helpers
_write_graph, cli_runner.invoke, and the graph check command invocation to keep
the new tests aligned with the other TestCheckCLI scenarios.

Source: Path instructions

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes