Skip to content

fix: avoid unsupported ORT instructions by default - #78

Open
redxzeta wants to merge 2 commits into
intuit:mainfrom
redxzeta:t3code/fix-infigraph-sigill
Open

redxzeta wants to merge 2 commits into
intuit:mainfrom
redxzeta:t3code/fix-infigraph-sigill

Conversation

@redxzeta

@redxzeta redxzeta commented Sep 18, 2026 •

Copy link
Copy Markdown

Summary

  • Fixes SIGILL on older x86_64 CPUs (including Ivy Bridge) when invoking graph-only commands such as test-context.
  • Root cause: the CLI and MCP binaries enabled the kompress feature by default, which links the downloaded prebuilt ONNX Runtime archive. That archive contains newer x86 instructions and is loaded into every process, even when no ML compression is requested.
  • Keeps ML output compression available as an explicit opt-in feature while leaving graph indexing/query functionality baseline-compatible.

What changed

  • Make kompress opt-in for infigraph-cli and infigraph-mcp.
  • Document the explicit --features kompress build requirement and CPU compatibility tradeoff.
  • Preserve loopback-only MCP HTTP binding by default, with INFIGRAPH_MCP_BIND as the explicit exposure opt-in.
  • Add a process-level CLI regression test that indexes a temporary fixture and runs test-context without machine-specific paths or external indexes.

Test plan

  • cargo fmt --all -- --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --all
  • cargo build --release -p infigraph-cli -p infigraph-mcp
  • Re-run the Ivy Bridge/Forkara test-context reproducer with the existing index read-only.
  • Start a fresh infigraph-mcp process and verify generate_test_context returns an MCP error/result instead of terminating the transport.
  • Verify normal search, stats, callers, impact, and project-discovery paths remain usable.
  • Verify the HTTP server remains loopback-only unless INFIGRAPH_MCP_BIND is explicitly set.

Limitations

  • The prebuilt ONNX Runtime archive may still require newer x86 instructions when kompress is explicitly enabled. Users on older CPUs should omit that feature; extractive compression remains the fallback.
  • This change does not alter Forkara or existing graph/index data.

Notes

The fix is intentionally narrow: it removes an unconditional native ML dependency from graph-only invocations instead of disabling semantic search or changing the database/query engine.

@murari316

Copy link
Copy Markdown
Collaborator

Verified locally: core fix works correctly — infigraph --root <tmp> test-context --file src/calculator.py --limit 1 runs successfully and produces correct output (confirms the CLI no longer SIGILLs without kompress, and the loopback-bind fix in web/mod.rs is conflict-free against current main).

However, the new regression test itself has a bug: crates/infigraph-cli/tests/cli_parity.rs::test_context_runs_in_the_cli_process asserts stdout.contains("Test Context"), but the actual CLI output header is "Test Generation Context" — "Test Context" is not a substring of that (different word order), so the assertion fails even though the command's output is correct. This looks like a pre-existing output format (confirmed via git log -S, "Test Generation Context" predates this PR) that the test's string match just doesn't account for. Could you fix the assertion to check for "Test Generation Context" instead? Once that's fixed this looks ready to merge — everything else I checked (build, loopback-bind regression test, kompress-opt-in Cargo.toml changes) is clean.

@redxzeta

redxzeta commented Oct 3, 2026

Copy link
Copy Markdown
Author

Verified locally: core fix works correctly — infigraph --root <tmp> test-context --file src/calculator.py --limit 1 runs successfully and produces correct output (confirms the CLI no longer SIGILLs without kompress, and the loopback-bind fix in web/mod.rs is conflict-free against current main).

However, the new regression test itself has a bug: crates/infigraph-cli/tests/cli_parity.rs::test_context_runs_in_the_cli_process asserts stdout.contains("Test Context"), but the actual CLI output header is "Test Generation Context" — "Test Context" is not a substring of that (different word order), so the assertion fails even though the command's output is correct. This looks like a pre-existing output format (confirmed via git log -S, "Test Generation Context" predates this PR) that the test's string match just doesn't account for. Could you fix the assertion to check for "Test Generation Context" instead? Once that's fixed this looks ready to merge — everything else I checked (build, loopback-bind regression test, kompress-opt-in Cargo.toml changes) is clean.

done

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants