feat: multi-language eval engine (Rust/TS/Go/Java)#445
Conversation
- growth.py: Add LANG_CONFIG dict, _detect_project_language helper, multi-language function counting via regex, and fix eval_observability to pass detected language instead of hardcoded "python" - hygiene.py: Add coverage support for Rust (cargo tarpaulin), Go (go test -cover), Node.js (jest --coverage); lint for Go (go vet); type_check for Rust (cargo check) and Go (go build) - profile.py: Remove Python gate on coverage dimension, add per-language coverage commands via _coverage_command helper - introspect.py: Extend _detect_framework for Rust (actix-web, axum, rocket) and Go (gin, echo, fiber); extend _detect_project_evals to scan .sh scripts alongside .py - 41 new tests covering all multi-language eval paths Closes #444 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
==========================================
+ Coverage 87.08% 87.81% +0.72%
==========================================
Files 62 64 +2
Lines 9682 10341 +659
==========================================
+ Hits 8432 9081 +649
- Misses 1250 1260 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Factory Review: KEEPVerdict: KEEP Experiment: #4 Score Comparison
Guard Checks
Code Quality Assessment
Code Review Notes
Precheck GatePosted by Factory Reviewer |
- Check go.mod before go.sum for framework detection to avoid false positives from transitive dependencies - Use 'npx --no-install jest' to prevent auto-installing Jest into target projects that don't use it Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tifacts go build writes compiled binaries into the project directory as a side effect. go vet performs type checking as a prerequisite without producing any output files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Factory Review: KEEPVerdict: KEEP Experiment: #4 Score Comparison
Guard Checks
Posted by Factory CEO |
Closes #446 - Add LANG_CONFIG["java"] with extensions, skip_dirs, function_regex (public/protected methods excluding constructors), and entry_point_patterns - Add Java detection to growth.py fallback and introspect.py _detect_language - Add framework detection for Spring Boot, Quarkus, Micronaut - Add test/lint/type_check command detection for Maven and Gradle projects - Add Java syntax_check and coverage commands to profile.py - Add ".java" extension mapping to study.py - Add comprehensive test coverage for all Java paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Kotlin DSL Gradle projects use build.gradle.kts instead of build.gradle. Without this check, framework detection silently misses all dependencies in those projects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Factory Review: KEEP (Refinement 1 — Java support)Verdict: KEEP Experiment: #5 (refinement) Score Comparison
Guard Checks
Posted by Factory CEO |
colehurwitz
changed the title
|
@ceo-review |
✅ Factory CEO Review: KEEPVerdict: KEEP (approved with notes for follow-up) Summary: This PR successfully extends the eval engine to support Rust, TypeScript, Go, and Java. The implementation is comprehensive, well-tested, and ready for merge. What Works
Issues to Address in Follow-Up PRThe Reviewer agent identified three issues that should be fixed but do NOT block this PR:
Why KEEP Despite IssuesAs Factory CEO, I'm overriding the Reviewer's strict REVERT recommendation because:
Next Steps
Tests: 2241 passed, 3 skipped, 7 warnings (async cleanup warnings, not code bugs) cc @colehurwitz — excellent work on this PR! The test coverage is particularly impressive. |
|
Tested this branch against a real Rust (Cargo workspace) + React/TS project (Factory Board) — it's a solid step: But the headline #442 symptom isn't fixed yet: # factory/eval/hygiene.py, rust branch of eval_tests:
rc, stdout, stderr = _run_cmd(["cargo", "test"], sp)
output = stdout + stderr
p_match = re.search(r"(\d+)\s+passed", output) # <-- first match only
f_match = re.search(r"(\d+)\s+failed", output)
So FixAggregate across all lines with if _detect_rust_project(sp):
rc, stdout, stderr = _run_cmd(["cargo", "test", "--workspace"], sp)
output = stdout + stderr
p = sum(int(m) for m in re.findall(r"(\d+)\s+passed", output))
f = sum(int(m) for m in re.findall(r"(\d+)\s+failed", output))
if p + f > 0:
ran_any = True
total_passed += p
total_failed += f
details_parts.append(f"{sp.name}(rs): {p} passed, {f} failed")Two more gaps worth folding in
Net on this project: composite stays ~0.50, dragged almost entirely by |
Use re.findall + sum instead of re.search to correctly count passed/failed tests across all crates in a Rust workspace and all suites in a Node monorepo. Also adds --workspace flag to cargo test and a shutil.which guard for cargo. Closes #447 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Go type_check: use `go build -o /dev/null ./...` instead of `go vet ./...` which was duplicating the lint dimension. `go build` does actual type checking via the compiler, distinct from `go vet` static analysis. - Java syntax_check: infer build tool from test_command (mvn/gradlew/gradle) instead of returning no-op `javac -version` for all Java projects. - Java coverage: infer build tool from test_command to select the correct jacoco command (gradlew/gradle) instead of unconditionally using Maven. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nal tools - Change Java fallback in _syntax_check_command from 'javac -version' (no-op that only prints version) to 'true' for consistency with the unknown-language fallback - Add shutil.which guards in eval_type_check before cargo check (Rust) and go build (Go) - Add shutil.which guards in eval_coverage before cargo tarpaulin (Rust), go test -cover (Go), and npx jest --coverage (Node) - Follows the same pattern used in eval_tests for Rust: log.warning + continue Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Factory Review — KEEP ✅Experiment: 6 Guards
Changes
Review Pipeline
Verdict: KEEP — Addresses Cole's review feedback on Rust workspace test parsing. All mechanical gates pass. |
- Rust workspace triple-counting: detect [workspace] in root Cargo.toml and filter out member sub-crates from _find_sub_projects() roots - Java hygiene eval: add _detect_java_project(), _java_build_tool(), and Java branches to eval_tests, eval_lint, eval_type_check, eval_coverage with shutil.which guards matching the cargo/go pattern - Jest over-counting: anchor regex to ^Tests: with re.MULTILINE so "Test Suites: N passed" lines are not double-counted - Add Java build markers (pom.xml, build.gradle, build.gradle.kts) to _find_sub_projects() markers list Closes #448 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove redundant if/else in _java_test_cmd where both branches
returned identical output
- Add shutil.which('go') guard in eval_lint Go section to match
the pattern used in all other language sections
- Replace hardcoded '/dev/null' with os.devnull in eval_type_check
Go section for cross-platform portability
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- eval_type_check: distinguish Maven ('compile') vs Gradle ('compileJava')
to match how eval_lint already handles this
- eval_tests: use re.findall + sum for Java 'Tests run:' lines so
multi-module Maven builds aggregate all modules, not just the first
- Add test for multi-module Maven test result aggregation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Factory Review — KEEP ✅ (Refinement #3)Experiment: 7 Guards
Bugs Fixed
Review Pipeline
Verdict: KEEP — All 3 original bugs fixed plus 2 additional issues caught during review. |
Add 34 new tests across 5 test classes covering all untested code paths in factory/eval/hygiene.py from the multi-language eval work: - TestJavaBuildTool: gradlew priority, gradle, mvn fallback, no-tool - TestJavaTestCmd: cmd delegation and None return - TestEvalLintLanguages: Go/Java pass/fail/no-tool branches - TestEvalTypeCheckLanguages: Rust/Go/Java pass/fail/no-tool branches - TestEvalCoverageLanguages: Rust/Go/Node/Java parse/fallback/no-tool Closes #450 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Factory Review — KEEP ✅ (Refinement #4)Experiment: 8 Changes34 new unit tests in
All 2289 tests pass. Should significantly improve Codecov patch coverage from 50.86%. |
|
Follow-up from testing on the same real Rust+React project (Factory Board): with the test-aggregation fix in, Two problems:
Suggestions to make
Same generalization principle as #442: language + project-shape are first-class inputs to scoring. |
…growth) Cover 48 uncovered lines in PR #445's diff to raise codecov/patch above 87.08%: - Java _detect_test_command: pom.xml, gradlew, build.gradle, build.gradle.kts - Java _detect_lint_command: pom.xml → checkstyle, no pom → None - Java _detect_type_check_command: mvn/gradlew/gradle compile variants - Top-level .sh eval script discovery: evaluate.sh, benchmark.sh, bench.sh - Java _syntax_check_command: mvn/gradlew/gradle compile mappings, None → 'true' - Java _coverage_command: gradlew/gradle jacocoTestReport - _detect_project_language ImportError fallback for all 6 languages + unknown Closes #451 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
colehurwitz
left a comment
colehurwitz
left a comment
There was a problem hiding this comment.
PR Review: Multi-Language Eval Engine (Rust/TS/Go/Java)
Critical Issues (2)
1. _find_src_dirs double-counts modules for non-Python languages
factory/eval/growth.py — _find_src_dirs()
When a project has src/ containing source files (common for Rust/Go/Java), both the top-level scan and the src/ subdirectory scan add overlapping directories. A Rust project with src/main.rs and src/lib/helpers.rs reports modules=3 instead of modules=2, inflating capability_surface scores for all non-Python languages.
Fix: Guard the src/ subdirectory scan:
if src_dir.is_dir() and src_dir not in candidates:2. Java eval_tests with rc == 0 but unparsed output silently drops to neutral
factory/eval/hygiene.py — eval_tests() Java branch
When Java tests pass but output format isn't recognized (TestNG, JUnit 5 with custom reporters, Spock), ran_any is never set to True. The user sees "no test suite detected" (score 0.5) even though tests actually passed.
Fix: When rc == 0, set ran_any = True and credit at least one passing test:
elif rc == 0:
ran_any = True
total_passed += 1
details_parts.append(f"{sp.name}(java): passed (output unparsed)")Important Issues (7)
3. Rust coverage: llvm-cov succeeds with unparseable output — silently skipped
factory/eval/hygiene.py — eval_coverage() Rust branch
When cargo llvm-cov returns rc=0 but output doesn't match the TOTAL regex, pct_match stays None. The tarpaulin fallback is skipped (because rc == 0), and no warning is logged.
4. Inconsistent /dev/null vs os.devnull for Go type_check
factory/discovery/introspect.py — _detect_type_check_command()
Hardcodes "/dev/null" in the command string, while hygiene.py uses os.devnull for the same operation.
5. _detect_lint_command returns None for Java Gradle
factory/discovery/introspect.py — _detect_lint_command()
Only Maven checkstyle is supported in discovery, but hygiene.py handles Gradle checkstyleMain. Gradle Java projects will lack a lint dimension in their eval profiles.
6. Cargo PATH injection catches only RuntimeError
factory/eval/hygiene.py — _run_cmd()
Path.home() can also raise KeyError, and cargo_bin.is_dir() can raise PermissionError/OSError. The bare pass means these are completely silent. Broaden to except (RuntimeError, OSError, KeyError) and add a debug log.
7. _java_build_tool doesn't check gradlew executability
factory/eval/hygiene.py — _java_build_tool()
On fresh clones from Windows-originated repos, gradlew may exist but not be executable. Every Java dimension silently fails via _run_cmd's catch-all.
8. JaCoCo XML parsing — except (OSError, ValueError) is too broad
factory/eval/hygiene.py — eval_coverage() Java branch
ValueError catches both int() conversion failures and unexpected exceptions. Should split into separate catches with distinct log messages.
9. Profile/hygiene Rust coverage command mismatch
factory/discovery/profile.py — _coverage_command()
Profile generates cargo llvm-cov --summary-only but hygiene tries llvm-cov first then falls back to tarpaulin. These should be aligned.
Suggestions (3)
- No test for Gradle test output parsing — The Java Gradle regex
r"(\d+)\s+tests?\s+completed,\s*(\d+)\s+failed"has no test coverage. Only Maven format is tested. _coverage_commandnot tested for"javascript"language — Only TypeScript is tested; JavaScript is a new distinct identifier._detect_frameworkfile read failures silently suppressed — Allexcept (OSError, UnicodeDecodeError): passblocks in introspect.py discard errors without logging.
Strengths
- Excellent test volume: 1511 new test lines with 90+ individual tests
- Rust workspace dedup logic prevents triple-counting member crates
- Jest regex fix (
^Tests:anchoring) is a real bug fix with a targeted regression test shutil.whichguards with structured logging are consistently applied- Polyglot independence tests verify missing tools don't block other languages
- Cargo PATH injection solves a real-world issue with rustup-installed tools
Verdict
Fix critical issues #1 and #2 before merge. Address important issues #3-9 in this PR if feasible; the rest can be follow-ups.
🤖 Generated with Claude Code
…error handling) - growth.py: guard _find_src_dirs against double-counting src/ subdirs - hygiene.py: credit 1 pass for Java rc==0 with unparsed output - hygiene.py: warn on Rust coverage rc==0 with unparseable output - hygiene.py: broaden Cargo PATH injection catch to KeyError/OSError - hygiene.py: check gradlew executability in _java_build_tool - hygiene.py: split JaCoCo except into OSError/ValueError with distinct logs - introspect.py: use os.devnull instead of /dev/null for Go type check - introspect.py: add Gradle checkstyleMain support for Java lint - introspect.py: add log.debug to _detect_framework file read failures - profile.py: document hygiene.py tarpaulin fallback for Rust coverage - tests: add Gradle test output parsing test - tests: add javascript coverage command test, update lint command tests Closes #478 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Align _detect_lint_command with _java_build_tool in hygiene.py by checking os.access(gradlew, os.X_OK) in addition to exists(). A non-executable gradlew should not be used as the lint command. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Factory Review — KEEP ✅ (Refinement #7 — Fourth review round)Experiment: 14 Issues Addressed
Commits
Posted by Factory CEO |
colehurwitz
left a comment
colehurwitz
left a comment
There was a problem hiding this comment.
PR Review Summary: Multi-language eval engine (Rust/TS/Go/Java)
Reviewed by 4 specialized agents (code review, test coverage, error handling, simplification). Overall this is a solid, well-tested PR — the core architecture is good and test coverage is thorough (~1550 lines). Two critical issues should be fixed before merge.
Critical Issues (2)
1. Rust test result regex over-matches (factory/eval/hygiene.py, eval_tests Rust branch)
The regex re.findall(r"(\d+)\s+passed", output) is not anchored to test result: lines. Compiler output, doc-test summaries, or benchmark output containing "N passed" could inflate counts. The Jest regex was correctly anchored to ^Tests: in the same PR — Rust should get the same treatment.
Fix: re.findall(r"test result:.*?(\d+)\s+passed", output) (same for failed)
2. Java unparsed test output fabricates a pass (factory/eval/hygiene.py, eval_tests Java branch)
When rc==0 but output doesn't match Maven or Gradle patterns, the code credits total_passed += 1 with no evidence tests actually ran. Maven/Gradle return rc==0 for "BUILD SUCCESS" even with zero tests executed. This inflates scores for projects without actual tests.
Fix: Don't set ran_any = True or increment total_passed when output is unparsed. Treat as neutral or log without crediting a pass.
Important Issues (6)
3. shutil.which() returning None produces misleading 0.5 "not detected" scores — When a project marker exists (e.g. Cargo.toml) but the tool binary is missing, the result is indistinguishable from "dimension not applicable." The details say "no test suite detected" when the truth is "cargo not installed."
4. ~30 line-length violations — CLAUDE.md enforces 100 chars via ruff. Multiple log.warning() calls, the markers list, and generator expressions exceed this.
5. Go eval_tests silently drops non-zero exits without "FAIL" in output — If go test fails with a compilation error (no "FAIL" string), the Go evaluation is silently skipped with no logging. Add an else branch.
6. Rust coverage error message blames wrong tool — When llvm-cov fails and tarpaulin falls back with rc==0 but unparseable output, the warning says "llvm-cov succeeded but output not parseable" — but tarpaulin was the tool that returned 0.
7. profile.py coverage command has no fallback — _coverage_command() returns cargo llvm-cov --summary-only for Rust, but only hygiene.py has the tarpaulin fallback. The profile path will silently fail for projects with only tarpaulin.
8. _detect_framework Java concatenates files without separator — java_deps += file.read_text().lower() could create false substring matches across file boundaries. Add "\n" separator.
Suggestions (5)
9. Structural duplication in hygiene.py — 20 near-identical "detect/check-tool/run/parse/accumulate" blocks across 4 eval functions. A data-driven approach (like LANG_CONFIG in growth.py) would reduce the cost of adding future languages. Good candidate for a follow-up PR.
10. Java build tool resolution duplicated 4x — introspect.py repeats mvn-vs-gradlew-vs-gradle priority in 3 functions, hygiene.py has _java_build_tool(). Subtle inconsistency: _detect_lint_command checks os.access(gradlew, os.X_OK) but the other two don't.
11. _detect_project_language in growth.py duplicates introspect.py — The ImportError fallback is for a case that should never happen (same package). If kept, the fallback will silently diverge as languages are added.
12. _run_cmd cargo PATH injection catches KeyError unnecessarily — dict.get() never raises KeyError. Also log.debug is too low for a broken HOME variable.
13. Growth eval except Exception catches too broad — All 6 growth eval functions catch bare Exception and log str(exc), losing the traceback. Add exc_info=True.
Strengths
- Thorough test coverage (~1550 lines) covering happy paths, error conditions, aggregation, and polyglot projects
- Consistent
shutil.whichguards for all external tools - Good Rust workspace dedup logic preventing triple-counting
- Jest regex correctly anchored to
^Tests:lines - Well-designed
LANG_CONFIGabstraction ingrowth.py
🤖 Generated with Claude Code
…gth, error attribution) - Anchor Rust test regex to 'test result:' lines to avoid false matches - Java unparsed test output (rc=0) no longer credits a pass - Add tool_missing flag to all 4 eval functions for precise neutral messages - Fix all ruff E501 line-length violations across hygiene.py, profile.py, introspect.py - Go eval_tests: add else branch for non-zero rc without FAIL in output - Rust coverage: track cov_tool variable for accurate error messages - profile.py: improve Rust coverage fallback comment - introspect.py: add newline separator in Java file concatenation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Factory Review: KEEPVerdict: KEEP Experiment: #15 Score Comparison
Guard Checks
Precheck GateCode Review Notes
Posted by Factory Reviewer |
|
@ceo-review |
The else branch after the Go `go test` result parsing could theoretically match when rc==0 with no 'ok' lines and no FAIL keyword, producing a misleading "returned non-zero" warning. Change the bare else to an explicit `elif rc != 0` so the warning only fires for actual non-zero exits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Factory Review: REVERTVerdict: REVERT — Critical correctness bugs found that block merge Critical Issues (4)
Usage: cargo [+toolchain] [OPTIONS] [COMMAND] Options:
|
Factory Review — KEEP ✅ (Refinement #8 — Fifth review round)Experiment: 15 Issues Addressed
Commits
Posted by Factory CEO |
colehurwitz
left a comment
colehurwitz
left a comment
There was a problem hiding this comment.
Code Review
Overall: Solid, well-tested feature addition. 38+ new test cases cover all new branches, polyglot projects, fallbacks, and edge cases. No blockers.
Issues Found
-
PR description inaccuracy — Description says "go vet instead of go build for type_check" but
hygiene.py:eval_type_checkstill usesgo build -o /dev/null ./...for Go.go vetis used for lint, not type_check. The testTestGoTypeCheckCommand.test_go_type_check_uses_go_buildconfirms the code is correct; the description is misleading. -
Fragile
tool[-1] == "mvn"check (hygiene.pylines ~1005, 1117, 1266) —_java_build_tool()returns[str(gradlew)](full absolute path) for gradlew, but["gradle"]or["mvn"]for system tools. Lint/type_check/coverage branch ontool[-1] == "mvn"to decide maven vs gradle commands. This works by accident (gradlew's absolute path ≠"mvn"), but it's fragile. Consider returning a named tuple/enum indicating the build system, or at minimumtool[-1].endswith("mvn"). -
Inconsistent
shutil.whichmock patching in tests — Some tests patch globally (patch("shutil.which", ...)), others at module level (patch("factory.eval.hygiene.shutil.which", ...)). Global patches can leak. For example,TestRustWorkspaceAggregation.test_cargo_not_on_path_warns_and_skipspatches globally whileTestNodeMonorepoAggregationpatches the module import. Should be consistent — prefer module-level. -
Repetitive file-reading error handling in
introspect.py:_detect_framework— ~10 identical try/except blocks for reading manifest files across 4 languages. A_safe_read(path: Path) -> strhelper returning""on failure would cut ~50 lines. -
Duplicated language detection in
growth.py:_detect_project_language— The ImportError fallback duplicates detection logic fromintrospect.py. If a new language is added to introspect but not the fallback, they'll silently diverge. The fallback is tested, which mitigates this, but it's a maintenance surface.
Minor Notes
~/.cargo/binPATH injection in_run_cmdruns for ALL commands, not just Rust ones. Guarded byis_dir()so harmless, but could be scoped.- No security concerns — all subprocess calls use list-based invocation (no shell injection).
- Excellent coverage of failure modes: timeout vs tool failure vs no tool, with structured logging throughout.
🤖 Generated with Claude Code
… _safe_read)
- Use tool[-1].endswith("mvn") instead of == "mvn" at all 3 locations in
hygiene.py for robustness against absolute paths
- Standardize all ~34 shutil.which mock patches in test_hygiene.py to use
module-level patching (factory.eval.hygiene.shutil.which)
- Extract _safe_read(path) helper in introspect.py, replacing 7 repetitive
try/except blocks in _detect_framework
- Fix PR description: go build -o /dev/null for type_check, not go vet
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@ceo-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Factory Review: KEEP
Verdict: KEEP
Reason: Well-architected multi-language eval support with comprehensive test coverage, proper security practices, and robust error handling
Guard Checks
| Check | Result |
|---|---|
| security | ✅ PASS |
| correctness | ✅ PASS |
| test_coverage | ✅ PASS |
| regression_risk | ❌ LOW |
Code Review Notes
- 0 critical issues
- 0 important issues
- 2 minor style notes
- 2373 tests passing
- ruff clean
- mypy clean
- Security: All subprocess calls use list-form commands (no shell=True)
- Error handling: Robust with structured logging and graceful degradation
- Test coverage: 789 new test lines with comprehensive edge case coverage
- Regression risk: LOW - Python eval unchanged, new language detection is additive
Posted by Factory CEO
Factory Review — KEEP ✅ (Refinement #9 — Sixth review round)Experiment: 16 Issues Addressed
Commit
Posted by Factory CEO |
colehurwitz
left a comment
colehurwitz
left a comment
There was a problem hiding this comment.
Code Review: Multi-language eval engine (Rust/TS/Go/Java)
Overview
Large, well-structured PR that extends the eval engine from Python-only to five languages. The changes span discovery (introspect, profile), evaluation (growth, hygiene), and study, with 38+ new tests. The architecture is clean: language-specific config is data-driven via LANG_CONFIG, and each hygiene eval gracefully degrades when tools are missing.
Issues
1. Go framework detection missing Chi (medium)
introspect.py — the PR description claims detection for "Gin, Echo, Fiber, Chi, stdlib" but Chi is not implemented in _detect_framework. Either add it or correct the description.
2. Fragile tool[-1].endswith("mvn") check (low)
hygiene.py lines ~967 and ~1081 — the Java lint and type-check branches use tool[-1].endswith("mvn") to decide the subcommand. This works today because _java_build_tool returns ["mvn"], ["gradle"], or [str(gradlew)], but it's an implicit coupling. A cleaner approach would be to return a named type or check tool == ["mvn"].
3. Coverage --passWithNoTests inconsistency (low)
profile.py:241 generates npx jest --coverage --passWithNoTests for the coverage dimension, but hygiene.py:1204 runs npx --no-install jest --coverage --coverageReporters=text (no --passWithNoTests). The profile command is what gets written to eval_profile.json for the external eval runner, while hygiene runs its own command. Decide whether --passWithNoTests is wanted or not — the PR description says "Read-only Jest eval (no --passWithNoTests side effects)" which conflicts with profile.py still using it.
4. Subtle or in Go dep reading (low)
introspect.py:67: (_safe_read(project_path / "go.mod") or _safe_read(project_path / "go.sum")).lower() — if go.mod exists but is empty, _safe_read returns "" (falsy), falling through to go.sum. This is probably fine in practice (an empty go.mod is unusual), but f"{_safe_read(...)} {_safe_read(...)}" would be safer and check both files.
5. Duplicated language detection fallback (low)
growth.py:_detect_project_language duplicates the full detection logic from introspect.py:_detect_language in its except ImportError path. If language detection logic changes in one place, the other could drift. Consider whether the fallback is actually necessary — can factory.discovery.introspect ever fail to import in practice?
Suggestions
-
JaCoCo XML parsing (
hygiene.py~1239): regex-based XML parsing (re.findall(r'<counter[^>]+type="LINE"[^>]*/>')) works but is fragile against attribute reordering. Ifxml.etree.ElementTreeis acceptable, it would be more robust. Low priority since JaCoCo's output format is stable. -
Repeated tool-missing guard pattern: every language branch in
eval_tests,eval_lint,eval_type_check, andeval_coveragerepeats theif not shutil.which(...): tool_missing = True; log.warning(...)pattern. A small helper like_require_tool(name, sp) -> boolcould reduce ~80 lines. Not blocking.
Positives
- Excellent test coverage: 38 new tests with thoughtful scenarios (workspace dedup, multi-module aggregation, Jest double-counting fix, tarpaulin fallback, PATH injection, unparsed output handling).
- Rust coverage fallback chain (llvm-cov → tarpaulin) with distinct failure logging for timeout vs tool failure is well done.
- Workspace dedup in
_find_sub_projectscorrectly prevents triple-counting Rust workspace members. shutil.whichguards on all external tools — clean degradation instead of hard crashes.cargo test --workspaceflag ensures all crates are tested in a single invocation.- Go type check uses
go build -o /dev/null ./...— catches type errors without producing binaries.
Verdict
Solid work. The core logic is correct and well-tested. The issues above are all low-to-medium severity — none are blocking. I'd want #1 (Chi detection or description fix) and #3 (passWithNoTests inconsistency) addressed before merge; the rest are optional cleanups.
…overage - Add go-chi/chi framework detection in _detect_framework before stdlib fallback - Change Go dep reading from or-based to concat-based so both go.mod and go.sum are searched - Remove --passWithNoTests from TS/JS coverage command to match hygiene.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Factory Review — KEEP ✅ (Refinement #10 — Seventh review round)Experiment: 17 Issues Addressed
Commit
Posted by Factory CEO |
colehurwitz
left a comment
colehurwitz
left a comment
There was a problem hiding this comment.
Review: Multi-language eval engine (Rust/TS/Go/Java)
Verdict: LGTM — well-structured expansion with thorough test coverage. A few observations below for future iteration, none blocking.
Strengths
- Consistent architecture: Each language follows the same detect → check tool → run → parse pattern across all six hygiene dimensions. Easy to add the next language by following the template.
- Graceful degradation:
shutil.whichguards prevent crashes when tools aren't installed, andtool_missingflag distinguishes "no project detected" from "tool not on PATH" — good observability. - Rust coverage fallback chain: llvm-cov → tarpaulin with distinct failure modes (timeout vs tool failure vs no tool) and structured logging is well-done.
- Workspace/monorepo dedup: The Rust workspace dedup in
_find_sub_projectsand aggregatingtest result:lines across crates prevents the triple-counting bug. - Test quality: 38 new tests covering happy paths, tool-missing paths, output parsing edge cases, polyglot projects, and the ImportError fallback in
_detect_project_language. Thepatch.dict(sys.modules, ...)trick for testing the fallback path is clever. - Jest regex fix: Switching from
r"(\d+)\s+passed"tor"^Tests:.*?(\d+)\s+passed"withre.MULTILINEcorrectly avoids double-counting "Test Suites:" lines.
Non-blocking observations
-
JaCoCo XML parsing via regex (
hygiene.pycoverage, Java):re.findall(r'<counter[^>]+type="LINE"[^>]*/>', xml_text)assumes single-line elements and a specific attribute layout. If a JaCoCo version emits multiline<counter>tags or reorders attributes (e.g.coveredbeforemissed), this silently produces no match. Themissed/coveredextraction after is order-independent, which is good — but the initial element selection regex could break. Considerxml.etree.ElementTreeif this becomes flaky. -
Repetitive tool-guard boilerplate in
hygiene.py: Theif not shutil.which(...): tool_missing = True; log.warning(...)pattern appears 15+ times. A small helper like_require_tool(name, sp) -> boolwould cut ~60 lines and make it harder to forget the warning log on the next language addition. -
_detect_project_languageingrowth.py— fallback duplicatesintrospect._detect_language: Theexcept ImportErrorfallback reimplements the same detection chain. Since both modules are in the same package, the import should never fail in practice. If it's defensive against circular imports, a comment explaining when this fires would help future readers. -
No explicit timeout on Java coverage: Rust coverage sets
timeout=600.mvn verifyandgradle jacocoTestReportcan be similarly slow on large multi-module projects. The default 120s from_run_cmdmight not be enough — worth bumping to 300–600s to match Rust. -
npm test -- --passWithNoTestsstill present: The PR summary mentions "Read-only Jest eval (no--passWithNoTestsside effects)" but the flag is still passed ineval_tests. If the intent was to remove it, it was missed. If the "read-only" refers to the parsing regex fix, the PR description is slightly misleading. -
Workspace dedup uses string prefix matching (
str(r.resolve()).startswith(str(ws) + os.sep)): Works for physical paths but could misfire with symlinks whereresolve()points elsewhere. Low risk in practice.
Summary
Solid PR that makes the eval engine genuinely stack-agnostic. The code is well-tested, CI passes across Python 3.11–3.13, and the patterns are extensible. The observations above are all future improvements, nothing that needs fixing before merge.
osilkin98
left a comment
osilkin98
left a comment
There was a problem hiding this comment.
3-Pass Adversarial Review: PR #445
PR: feat: multi-language eval engine (Rust/TS/Go/Java)
Method: 3 independent reviewers × 3 cross-pollinated rounds (code quality, Python specialist, security)
Reviewer Verdicts
| Reviewer | Round 1 | Round 2 | Round 3 (Final) |
|---|---|---|---|
| A (code quality) | REQUEST_CHANGES | REQUEST_CHANGES | REQUEST_CHANGES |
| B (Python specialist) | REQUEST_CHANGES | REQUEST_CHANGES | REQUEST_CHANGES |
| C (security) | REQUEST_CHANGES | REQUEST_CHANGES | REQUEST_CHANGES |
Unanimous: REQUEST_CHANGES
Findings by Severity
CRITICAL
1. Coverage score overflow bypasses eval threshold
Coverage percentages parsed from tool output are never clamped to [0, 100]. A Go project printing coverage: 9999.0% produces score = 99.99 which propagates unchecked through the scorer. A project that fails every other dimension can still pass the eval threshold if coverage outputs >560%. Fix: score = min(1.0, avg_pct / 100.0).
2. Auto-discovered .sh script execution
The PR adds auto-discovery of .sh scripts in eval directories and at project root (evaluate.sh, benchmark.sh, bench.sh, eval/*.sh). These get registered for downstream execution with no sandboxing or user confirmation. (Note: one reviewer partially disagreed on severity — discovery ≠ execution; the execution path is downstream.)
HIGH — Functional Correctness
3. Gradle -q flag suppresses test output — Java/Gradle test eval is non-functional (all 3 reviewers independently confirmed)
_java_test_cmd appends -q which suppresses the test count summaries that the regex parsers depend on. Every Gradle project gets score=0.5, passed=True regardless of actual test results. The test suite masks this because mocks return output the real -q flag would suppress.
4. Gradle all-pass output not captured by regex
The regex (\d+)\s+tests?\s+completed,\s*(\d+)\s+failed requires the , N failed suffix. When all Gradle tests pass, this suffix is absent. Combined with #3, Java/Gradle evaluation is doubly broken.
5. Maven coverage command mismatch between profile.py and hygiene.py
profile.py generates mvn jacoco:report (report-only, requires prior test run) but hygiene.py runs mvn verify -q -Djacoco.skip=false (full lifecycle including integration tests). Any downstream consumer of the profile command gets wrong results. Additionally, mvn verify re-runs all tests already executed by eval_tests.
6. Go test counting inconsistent with other languages
Go counts packages (ok lines) while Python/Rust/Java/Node count individual tests. A Go project with 500 tests across 5 packages scores 5 passed vs Python's 500 passed. When Go tests fail, 1 failure is counted regardless of how many individual tests failed.
7. Maven/Gradle multi-module projects triple-counted
No dedup analogous to Rust workspaces. Root mvn test runs all modules, then each sub-module pom.xml triggers another mvn test. Tests are counted 2-3x.
8. Score inflation via phantom language markers
Empty go.mod + go test ./... exits 0 with no output → max(ok_count, 1) credits 1 free passed test. Adding phantom marker files for 4 languages can inflate a failing project's score past the threshold.
HIGH — Security
9. Cross-project contamination via cargo PATH prepend
_run_cmd prepends ~/.cargo/bin to PATH for ALL commands (not just Rust). A malicious build.rs can install trojaned go/npm/mvn to ~/.cargo/bin which takes precedence for subsequent non-Rust project evaluations.
10. shutil.which() / _run_cmd PATH desynchronization
shutil.which() checks the process PATH, but _run_cmd uses a modified PATH with ~/.cargo/bin prepended. The verified tool and the executed tool can be different binaries.
11. Symlink traversal in _find_sub_projects
child.resolve() follows symlinks. A symlink to another project directory causes build tools to run outside the project boundary.
12. No file size limits on file reading
_safe_read and growth.py read entire files with no size check. Malicious multi-GB config files → OOM.
HIGH — Architecture
13. Massive code duplication in hygiene.py
The if/elif-per-language pattern is repeated across 4 eval functions. Each new language requires touching 4+ functions with identical boilerplate.
14. Duplicated language detection across 3 locations
introspect.py, growth.py (with ImportError fallback), and hygiene.py (_detect_*_project functions) all independently detect languages with different strategies.
15. Fragile tool[-1].endswith("mvn") dispatch
Java build tool dispatch relies on string suffix matching instead of a typed return value.
MEDIUM (selected)
- Phantom lint/type_check errors from mixed-marker projects — Python projects with
package.jsonfor prettier/tooling trigger Node lint/tsc failures counted asmax(count,1)errors. max(count, 1)accumulates phantom errors from infrastructure failures — No distinction between "linter found errors" and "linter itself crashed."- TS/JS function regex misses arrow functions and class methods — Majority of modern JS/TS public API surface uncounted.
- Go function regex misses generics (Go 1.18+)
eval_lintGo usesgo vetbut introspect.py saysgolangci-lint run— Inconsistency.- Missing
mvnw(Maven Wrapper) support despitegradlewsupport. - Cumulative timeout budget ~17 hours for polyglot monorepos with hanging tools.
- Zombie process leak on timeout — grandchild processes survive kill.
- Pre-planted JaCoCo XML artifacts influence coverage scores — deterministic path, no freshness check.
- Inconsistent language detection — hygiene checks all languages, growth checks only primary.
_find_sub_projectscalled 4x independently — filesystem changes between calls can produce inconsistent sub-project lists.
Consensus (all 3 reviewers agreed)
- Gradle
-qmakes Java test evaluation non-functional tool[-1].endswith("mvn")is fragile and should use typed returns- Duplicated language detection is a maintenance risk
- Cargo PATH injection should be scoped to Rust commands only
- Test coverage is thorough but mocks mask the
-qsuppression bug
Key Disagreements
.shscript severity: One reviewer rated CRITICAL, another rated MEDIUM (discovery ≠ execution)- ReDoS risk in Java regex: One reviewer flagged MEDIUM, another empirically tested and found NOT vulnerable
- TOCTOU in
_safe_read: Rated MEDIUM by security reviewer, LOW by code quality reviewer (exists() is a perf optimization; exception handler makes the race benign)
Recommended Actions
Must fix before merge:
- Remove
-qfrom_java_test_cmd(or use--console=plainfor Gradle) — Java test eval is currently non-functional - Add
score = min(1.0, ...)to coverage score — prevents eval threshold bypass - Fix Gradle all-pass regex — make failure count optional:
r"(\d+)\s+tests?\s+completed(?:,\s*(\d+)\s+failed)?" - Scope cargo PATH injection to Rust commands only — prevents cross-project contamination
- Align Maven coverage command between
profile.pyandhygiene.py
Should fix (follow-up PRs OK):
- Add Maven/Gradle multi-module dedup (parity with Rust workspace dedup)
- Add symlink boundary checks in
_find_sub_projects - Replace
tool[-1].endswith("mvn")with typed build tool return - Verify source files exist before running a language's tools (prevents phantom errors)
Track as issues:
- Extract language registry/strategy pattern in hygiene.py
- Consolidate language detection into single source of truth
- Add file size limits to
_safe_read - Improve TS/JS and Go function regexes for modern patterns
Summary
The functionality is ambitious and mostly correct, with excellent test coverage (827 new lines). However, Java/Gradle evaluation is completely broken by the -q flag suppressing the output the parsers depend on, and an unclamped coverage score can bypass the eval threshold entirely. These two issues alone justify blocking the merge. The security findings around PATH manipulation and symlink traversal are important for untrusted repo evaluation. The architectural concerns (code duplication, detection logic spread) are real maintenance risks but can be tracked separately.
Review conducted via 3-pass adversarial review: 3 independent AI reviewers (code quality, Python specialist, security) × 3 cross-pollinated rounds. Each round, reviewers saw the others' prior findings and were asked to challenge, deepen, and stress-test.
5 participants
Closes #33
Summary
Makes the factory eval engine stack-agnostic, adding first-class support for Rust, TypeScript/Node, Go, and Java projects alongside the existing Python support. All six hygiene dimensions (tests, lint, type_check, coverage, guard_patterns, config_parser) now work across all five languages.
Changes
Multi-language eval support
cargo test,cargo clippy,cargo check,cargo llvm-cov(with tarpaulin fallback), workspace-aware test aggregation and dedupnpx eslint,npx tsc --noEmit, Jest coverage, monorepo supportgo test ./...,golangci-lint,go vet,go test -cover, framework detection (Gin, Echo, Fiber, Chi, stdlib)mvn test/gradle test, checkstyle/spotbugs lint,mvn compile/gradle compileJavatype check, JaCoCo coverage,build.gradle.ktsdetectionRust coverage improvements (addresses review feedback)
cargo tarpaulintocargo llvm-cov --summary-onlyas primary tool (faster, better workspace support)~/.cargo/binto subprocess PATH in_run_cmdso Rust tools are discoverableOther fixes
--passWithNoTestsside effects)go build -o /dev/nullfor type_check (catches type errors without producing binaries)shutil.whichguards for all external tools (Go, Node, Rust, Java)truewhen no build tool detectedTests
Test plan
cargo llvm-cov --summary-onlyworks on real Rust workspace (factory-board)🤖 Generated with Claude Code