Repository navigation
Provide more details for supported units in resource validation rules - #13
Merged
Merged
Conversation
RyaliNvidia
approved these changes
Nov 17, 2025
vvnpn-nv
approved these changes
Nov 17, 2025
jiaenren
added a commit
that referenced
this pull request
May 4, 2026
The previous defaults under-weighted dependency centrality: lib/utils/ common.py (fan_in=222 — by far the most-imported module in OSMO) ranked #13, despite its 45% coverage being the floor on a 1266-line file (~696 uncovered lines). Two adjustments: - fan_in weight 1.5 -> 2.5: dependency centrality is the most durable signal of criticality. A hub stays a hub for years, while coverage gaps and recent churn shift week to week. Both fan_in and churn are still log-normalized in score_entry so the heaviest hub's contribution is bounded at the weight value itself. - uncovered_lines cap 300 -> 500: a 1266-line file with 700+ uncovered lines was being clipped to 300, throwing away signal that it has substantially more surface area than a 300-line file at the same coverage %. Dry-run effect: lib/utils/common.py rises #13 -> #8. The top 5 stay the same backend-foundation files (utils/job/*, lib/data/storage/ client.py, lib/utils/client.py) because their coverage gap is wider — the multiplicative score formula keeps "important AND under-covered" ahead of "very important but already half-covered." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jiaenren
added a commit
that referenced
this pull request
May 5, 2026
* testbot: rank coverage targets by criticality x coverage gap Pure-coverage ranking let trivial low-coverage files dominate the budget — PR #939 picked a one-off file at 0% over heavily-imported public APIs at 30%. Replace the single-pass selector with a two-stage pipeline. Stage 1 (criticality_scorer.py): combines Codecov coverage with three cheap static signals — Python AST + Go reverse imports for fan-in, six-month git churn, and a path-tier classification keyed off AGENTS.md. Scores files by score = criticality * (1 - coverage) * log(uncovered_lines) with log normalization on fan-in/churn so a single mega-hub doesn't swamp the rest. Outputs a top-20 JSON shortlist with a per-signal breakdown so reviewers can see why each file ranked where it did. Stage 2 (select_targets_agent.py + SELECT_TARGETS_PROMPT.md): a small read-only Claude subagent (Read,Glob,Grep, 30-turn cap) reads each candidate and picks the 1-3 files where unit tests would have the highest ROI. Rejects heavy I/O glue, long-running orchestration, and SDK-call delegators. Can return zero picks rather than burn the budget on low-ROI work. Final picks plus the LLM rationale flow into the existing TESTBOT_PROMPT.md / create_pr.py contract — no downstream changes needed. Dry-run on main shortlists src/lib/{rsync,data/storage,utils}, src/utils/{job,roles}, and src/service/core/{data,workflow} — exactly the kind of files we want testbot writing tests for. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: set PYTHONPATH=. so criticality_scorer can resolve src.* imports The Stage-1/Stage-2 scripts use namespace-package imports (`from src.scripts.testbot.coverage_targets import ...`) which need the repo root on sys.path. CI invokes them as plain scripts, where Python adds the script's directory — not the repo root — to sys.path. Mirror the existing `PYTHONPATH=src/scripts python -m testbot.create_pr` pattern but at the repo-root level so the existing import paths work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: drop fragile inline shortlist printer The Python f-string with escaped double quotes broke under YAML+shell quoting (`SyntaxError: unexpected character after line continuation`). criticality_scorer.py already prints the ranked shortlist via logger.info in its main(), so the inline pretty-printer was redundant. Replace with a `cat` of the JSON so the full per-signal breakdown is still one click away in the workflow log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: stream + diagnostics for Stage-2 target picker Mirror the foldable stream + diagnostics pattern the generate step uses so the target picker is debuggable in the workflow log: - Run npx with `--output-format stream-json --verbose` and tee the JSONL into a tempfile while echoing each line live. - Wrap the live stream in `::group::Claude Code stream — target picker` so the per-turn JSON doesn't dominate the default view. - After completion, parse the final `result` event in reverse and print exit_status + subtype + is_error + num_turns + duration_ms + duration_api_ms + total_cost_usd + truncated result text. - Wrap diagnostics in `::group::` + `::stop-commands::<token>` so untrusted result text can't inject `::warning::`, `::add-mask::`, or other workflow commands. - Use the parsed `result.result` string as the agent text fed into parse_agent_output, replacing the previous capture_output approach. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: resolve __init__.py re-exports + multi-name from-imports for fan-in OSMO uses three import patterns the naive AST scan missed, all of which caused fan_in=0 for genuinely heavily-imported files (lib/utils/client.py read 0 before this; it's actually 85): 1. `from src.utils.job import jobs, workflow, task` — record each imported name as a candidate submodule (not just the source package). 2. `from src.lib.data.storage import Client` — follow `__init__.py` re-exports (`from .client import Client`) so `Client` aliases to client.py. 3. `from src.lib.data import storage` — credit every file the storage/__init__.py imports from (Python runs __init__.py on package import, so the importer's fate is tied to all of them). Dedup resolved targets per importer so a file that imports both `Client` and `SingleObjectClient` adds 1 to client.py, not 2. Dry-run on main now shows lib/data/storage/client.py at rank 2 with fan_in=70 (was missing entirely before), lib/rsync/rsync.py at rank 3 with fan_in=5, and lib/data/dataset/manager.py at rank 11 with fan_in=5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: include uncovered_lines in criticality_scorer log output The shortlist log printed coverage % but not the absolute uncovered line count. Both matter — a 0%-covered 30-line file and a 0%-covered 800-line file have very different testing surfaces, and the score formula already weighs them differently. Surface the count next to the % so operators can see why two files at the same coverage rank differently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: bump fan_in weight 1.5->2.5 and uncovered cap 300->500 The previous defaults under-weighted dependency centrality: lib/utils/ common.py (fan_in=222 — by far the most-imported module in OSMO) ranked #13, despite its 45% coverage being the floor on a 1266-line file (~696 uncovered lines). Two adjustments: - fan_in weight 1.5 -> 2.5: dependency centrality is the most durable signal of criticality. A hub stays a hub for years, while coverage gaps and recent churn shift week to week. Both fan_in and churn are still log-normalized in score_entry so the heaviest hub's contribution is bounded at the weight value itself. - uncovered_lines cap 300 -> 500: a 1266-line file with 700+ uncovered lines was being clipped to 300, throwing away signal that it has substantially more surface area than a 300-line file at the same coverage %. Dry-run effect: lib/utils/common.py rises #13 -> #8. The top 5 stay the same backend-foundation files (utils/job/*, lib/data/storage/ client.py, lib/utils/client.py) because their coverage gap is wider — the multiplicative score formula keeps "important AND under-covered" ahead of "very important but already half-covered." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: simplify — consolidate Codecov parsing + markdown formatting Three reviewer agents flagged duplication across coverage_targets, criticality_scorer, and select_targets_agent. Fix the high-value items: * Extract `parse_coverage_entries()` in coverage_targets.py and reuse it from criticality_scorer.py — kills the 35-line copy of the Codecov line-coverage parsing loop. select_targets and rank_targets now layer their own ordering/filtering on the shared parser. * Extend `coverage_targets.format_targets()` with an optional empty_message and a `Why this file:` line (read off `target.reason`) so select_targets_agent.format_targets_markdown collapses to a one-line wrapper. * Add CODECOV_LINE_HIT/CODECOV_LINE_MISS constants — kills the last magic-number 0/1 branch in the parser. * Pre-compute `path -> rel_str` in build_python_fan_in so the inner loop doesn't re-call relative_to() per file (~220 redundant conversions on the OSMO repo). * Single-pass classification in build_go_fan_in: package_files and package_imports are populated in one loop instead of two `_go_package_for_file` calls per file. * Replace `assert proc.stdout is not None` with a runtime RuntimeError — assertions can be stripped under -O and shouldn't gate production behavior even when the path is unreachable. * Drop two narrating comments per repo style (MIN_LOC explainer and 1-level-relative rationale). Verified: all 17 testbot bazel targets still pass; dry-run on main yields identical top-5 scores and ranking. Net diff: 74 insertions, 106 deletions across the three files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: clarify scoring formula in README Replace the one-line score formula with a structured ASCII diagram + per-term table so each component's contribution and bound is obvious without reading the source. Adds a short rationale for the two non-obvious modeling choices (multiplicative score, log normalization on fan_in/churn). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: split scoring formula into three equations The combined ASCII diagram with bracketed labels was hard to parse — criticality and coverage_gap appeared on the same line joined by a mid-line `×`. Split into three separate equations so each piece reads left-to-right on its own. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: prune ignored subtrees + resolve re-exports to package __init__ Two issues from review, both verified against current code: 1. **Pruned filesystem walk.** _walk_files used rglob('*') which descended into every subtree before filtering — including src/ui/node_modules (~645MB), .git, and any .claude/worktrees/ copies of the repo a developer might have. Replace with a single os.walk-based generator that prunes ignored subtrees in place (PRUNE_DIR_NAMES at any depth, PRUNE_ROOTED_DIRS at the top). Both _walk_files and _walk_init_files now share this iterator. Real fix: my locally-developed branch had .claude/worktrees/ from a prior agent run, which the old rglob descended into. That inflated fan-in counts ~5x because every src/ file appeared twice in the importer set. CI (no .claude/) was always seeing the correct, lower numbers — this commit makes local runs match. 2. **Re-export to package __init__.py fallback.** The relative-import resolver in _re_export_targets always set target_file to `<module>.py`. For `from .backends import construct_storage_backend` in storage/__init__.py, this pointed at a phantom src/lib/data/storage/backends.py — but that file doesn't exist; only backends/__init__.py does. Try the .py target first, fall back to <package>/__init__.py if it exists. Phantom paths no longer leak into module_to_path or package_re_exports. Tests: 17/17 testbot bazel targets still pass. Two new tests cover the package-init fallback case and the phantom-path absence invariant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: harden Stage-2 pick handling — non-dict guard + max_targets cap Two findings from review, both verified: 1. **Non-dict picks.** parse_agent_output() validates only that `targets` is a list, not that each element is a dict. If the model ever emits a list of strings (e.g. `["src/lib/foo.py", ...]`) the loop `for pick in picks: pick.get("file_path")` would raise AttributeError mid-run. Add an isinstance(pick, dict) guard with the same warn-and-skip pattern used for missing file_path. 2. **max_targets ceiling.** The prompt says "at most {max_targets}" but if the agent returns more we currently merge all of them. After parse_agent_output, slice picks to args.max_targets and log a warning when truncation happens. The downstream pipeline now never sees more targets than configured. Tests: 17/17 testbot bazel targets pass. Added a non-dict-pick test with strings, ints, and None. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: drain Claude CLI stdout from a background thread so timeout fires Verified: the synchronous `for line in proc.stdout: ...` loop blocked until the agent closed its pipe, which meant `proc.wait(timeout=...)` ran *after* the process had already exited and could never raise TimeoutExpired during a hang. If the agent stalled without writing, the wrapper would hang forever despite the timeout argument. Spawn a daemon thread to drain stdout (echoing to sys.stdout and the JSONL log file), then call proc.wait(timeout=timeout_sec) from the main thread. On TimeoutExpired we kill the process, reap it with proc.wait(), and join the drain thread with a 2s ceiling so a stuck reader can't block teardown. On clean exit we still join the drain thread so the last lines flush before the function returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: harden _stream_npx — encoding, drain race, exit path, timeout test Code-review feedback: - **Drain/teardown race**: on the success path drop the 2.0s ceiling on drain.join() — the pipe closes when the process exits, the for-loop sees EOF, and the thread completes promptly. Bounding the join risked closing logf with the daemon thread mid-write, which could lose the final `result` event under exactly the conditions the timeout was meant to catch. On the timeout path keep the bounded join (process is dead, pipe is closed, thread should exit fast) and wrap the drain loop in a ValueError/OSError except so a teardown race can't crash the daemon. - **Encoding**: pass `encoding="utf-8", errors="replace"` to Popen. With `text=True` alone we were inheriting locale.getpreferredencoding — fine on the GitHub-hosted Ubuntu runner today, but a single non-UTF-8 byte from the agent would have killed the daemon thread silently. - **Diagnostics on unreachable Popen-no-stdout branch**: convert the `raise RuntimeError(...)` into `return 1` + log so the diagnostics block in invoke_claude still emits before exit. - **Timeout integration test**: TestStreamNpxTimeout runs `sleep 30` with timeout_sec=1 and asserts exit code 124. Pins the background-thread-drain fix — the earlier synchronous loop would have hung indefinitely. 17/17 testbot bazel targets pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * testbot: stop over-crediting package siblings on named-symbol imports Code-review #2: when an importer says `from src.lib.data.storage import StorageBackend`, the previous fan-in resolver was adding +1 to *every* file that storage/__init__.py re-exports from — even though the importer only ever references backends/common.py. The package only matters as a value when the importer actually binds it (via `import X` or `from X.parent import X`), not when X is the source of named symbols. Refactor `_python_imports` to return `(direct, package_use)`: - `direct` — every dotted name that *might* resolve to a single file. Source `X` from `from X import Y` plus the composed `X.Y`, plus the whole-name target of `import X.Y.Z`. - `package_use` — names the importer binds *as a value*. Populated by `import X.Y.Z` (top-level package binding) and the composed `X.Y` from `from X import Y` (Y could be a sub-package the importer accesses as `Y.something`). Crucially **not** the source `X` of `from X import Y, Z` — in that form X is never referenced as a value. `build_python_fan_in` now consumes both: direct hits credit single files; package re-exports expand only for `package_use` entries that didn't already resolve to a file. Test added: `test_named_import_does_not_overcredit_package_siblings` asserts client.py stays at 0 when only StorageBackend was imported. Top-10 dry-run on main is unchanged (the over-credit was distributed evenly across packages so removing it shifted everyone proportionally). The fix matters more as __init__.py re-exports grow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks done
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
The documentation does not clearly specify what units can be supported in the resource validation rules. This clearly specifies what units can be interpreted for memory and storage, and what can be supported for CPU.
Checklist