maint-79: grow verifier corpus from realized PR outcomes (#2819 move 2) - #2832
Conversation
Move 2 of the self-feeding verifier-model promotion system (#2819). The evaluation corpus was hand-built and never grew, so the approval benchmark stays perpetually under its 75-case minimum and no model is ever promoted — the evidence supply chain is the real chokepoint. This harvests new cases from outcomes the world already adjudicated by merging/reverting a PR, so the corpus grows with near-zero owner time. - tools/harvest_verifier_corpus.py: classify() labels a merged PR from its realized outcome — stable-for-N-days clean merge -> high-confidence clean-pass PASS; reverted-within-window -> regression-after-merge NON_PASS; resolved verifier follow-up -> follow-up-required NON_PASS. High-confidence cases auto-promote into the frozen corpus (version bumps +harvestN, dedup by repo+pr, per-category caps so clean-pass can't flood it). Ambiguous cases (too-recent, unresolved follow-up) route to an FYI-only staging file that auto-expires after staging_expiry_days, so no adjudication backlog can accumulate. fetch_records() targets PRs merged in a stability-aged date window (a newest-N fetch only returns too-recent merges that never promote). The semantic NON_PASS categories (stale-verifier-claim, review-thread-debt, missing-acceptance-criterion) cannot be labeled from outcomes and remain owner-sourced; the tool never fabricates them. - config/model_selection_policy.json: corpus_growth block (stability_days, staging_expiry_days, harvest_window_days, max_corpus_size, category_caps, source_repos = the 12-repo lane fleet). - config/model_eval_corpus_staging.json: FYI staging seed. - tests/tools/test_model_eval_pilot.py: the freeze test now digest-freezes only the owner-adjudicated SEED (provenance != harvested) and validates harvested cases structurally — so growth is allowed but silent tampering with the seed still fails CI. - maint-79: weekly + dispatch harvest that opens an auto-merging corpus-growth PR (the PR is the audit trail, never a gate); PR events run the unit tests only. Zero-Tim by design: high-confidence cases flow automatically; only genuinely ambiguous ones stage, FYI-only and auto-expiring. Promotion of MODELS is unchanged — a larger corpus just makes the existing gated benchmark reachable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a configurable verifier-corpus harvester that classifies merged PR outcomes, promotes stable cases, stages uncertain cases, and updates corpus files. It includes tests, scheduled/manual GitHub Actions automation, auto-merge PR creation, policy configuration, and worker-attempt metadata updates. ChangesVerifier corpus growth
Worker attempt metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Harvester
participant CorpusFiles
participant PullRequest
GitHubActions->>Harvester: run scheduled or manual harvest
Harvester->>CorpusFiles: classify records and update corpus/staging
GitHubActions->>PullRequest: create corpus-growth pull request
PullRequest-->>GitHubActions: return pull request number
GitHubActions->>PullRequest: enable squash auto-merge
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
GitHub Actions
Inactive
Workflow source neededPR #2832 needs either a linked GitHub issue or one valid non-issue Workflow Source before PR metadata automation can manage it safely. Please do one of:
Once a valid source is present, this warning will not be reposted. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if: steps.cpr.outputs.pull-request-number | ||
| env: | ||
| GH_TOKEN: ${{ secrets.OWNER_PR_PAT }} | ||
| run: gh pr merge "${{ steps.cpr.outputs.pull-request-number }}" --squash --auto |
| run: | | ||
| mode="" | ||
| if [ "${{ github.event_name }}" = "workflow_dispatch" ] && [ "${{ inputs.write }}" != "true" ]; then | ||
| mode="dry-run" | ||
| python -m tools.harvest_verifier_corpus | tee harvest.log | ||
| else | ||
| python -m tools.harvest_verifier_corpus --write | tee harvest.log | ||
| fi | ||
| { | ||
| echo '## Verifier corpus harvest' | ||
| echo '```' | ||
| cat harvest.log | ||
| echo '```' | ||
| } >> "$GITHUB_STEP_SUMMARY" |
| if: github.event_name == 'pull_request' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v7 |
| - uses: actions/checkout@v7 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: actions/setup-python@v6 |
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 | ||
| steps: | ||
| - uses: actions/checkout@v7 |
| - uses: actions/checkout@v7 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: actions/setup-python@v6 |
| - name: Open auto-merging corpus-growth PR | ||
| if: github.event_name == 'schedule' || (github.event_name == 'workflow_dispatch' && inputs.write) | ||
| id: cpr | ||
| uses: peter-evans/create-pull-request@v8 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f15c5cae46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| labels & {"verify:create-issue", "verifier-followup"} | ||
| ), | ||
| # Resolution of a follow-up needs semantic judgment; stay conservative | ||
| # (unresolved -> staged, never auto-labeled NON_PASS). | ||
| "followup_resolved": False, |
There was a problem hiding this comment.
In the scheduled live-data path, every detected follow-up is hard-coded as unresolved, while the only checked signals are current verify:create-issue/verifier-followup labels. The repository's follow-up workflows remove their trigger labels after processing, and verify:create-new-pr is not checked at all, so a completed verifier follow-up will normally be classified as clean-pass after 30 days; even if a label remains, it stays staged forever. Resolve follow-up status from durable workflow/issue evidence before assigning the expected verdict.
Useful? React with 👍 / 👎.
| if key in existing or len(cases) >= max_size: | ||
| continue |
There was a problem hiding this comment.
When a PR is first harvested just after the 30-day threshold, it can be added as a PASS; if it is reverted on a later weekly run, classify produces NON_PASS, but this repo/PR deduplication silently skips the updated outcome. The corpus therefore retains a known-false PASS indefinitely, so existing harvested cases need to be replaced or quarantined when their realized outcome changes.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,106 @@ | |||
| name: Maint 79 Verifier Corpus Harvest | |||
There was a problem hiding this comment.
This new workflow is absent from docs/ci/WORKFLOWS.md, docs/ci/WORKFLOW_SYSTEM.md, and EXPECTED_NAMES in tests/workflows/test_workflow_naming.py. The naming test asserts that every workflow has a display-name mapping, and the inventory test requires every workflow in both documents, so the repository's workflow validation will fail; scripts/check_docs_drift.py also reports this file as undocumented. Register the workflow in all three required surfaces.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
| for pr in reverts: | ||
| for match in _REVERT_REF.findall(f"{pr.get('title', '')} {pr.get('body', '')}"): | ||
| numbers.add(int(match)) |
There was a problem hiding this comment.
For any merged PR with revert in its title, this loop treats every #N in both title and body as a reverted PR. If the revert body also references an issue or unrelated PR for context, that unrelated candidate is assigned a high-confidence regression-after-merge verdict and permanently contaminates the evaluation corpus. Parse the canonical revert target or verify the reverted commit relationship instead of accepting all references.
Useful? React with 👍 / 👎.
Automated Status SummaryHead SHA: 874dc01
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
GitHub Actions
Inactive
GitHub Actions
Inactive
CI feedback on #2832: - Register maint-79 in EXPECTED_NAMES (tests/workflows/test_workflow_naming.py) and the WORKFLOWS.md / WORKFLOW_SYSTEM.md inventories, as required for any new workflow. - Remove the pull_request `test` job: it ran `python -m pytest` without installing pytest (No module named pytest). The harvester's unit tests already run in the standard python-ci suite, so the job was redundant. - Drop the unused `mode` shell variable (actionlint shellcheck SC2034). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/maint-79-verifier-corpus-harvest.yml:
- Around line 65-72: Remove the unused mode variable and its assignments from
the workflow step while preserving the existing conditional command selection
between dry-run and --write modes.
- Around line 23-27: Update the workflow path filters to include
config/model_eval_pilot.json and tests/tools/test_model_eval_pilot.py, then
adjust the test command at the existing test step to run both test modules:
test_harvest_verifier_corpus.py and test_model_eval_pilot.py. Preserve the
current harvest-related trigger paths and validation behavior.
In `@tests/tools/test_harvest_verifier_corpus.py`:
- Around line 141-156: Add coverage in the harvest verifier test around hv.main
to invoke the same arguments without --write, then assert both corpus and
staging files remain unchanged. Preserve the existing --write assertions for
promoted and staged cases, and verify the default dry-run behavior before any
persistence occurs.
In `@tools/harvest_verifier_corpus.py`:
- Around line 71-106: Update the follow-up classification behavior and module
documentation consistently: either implement resolution detection in
fetch_records so classify can produce high-confidence CATEGORY_FOLLOW_UP results
from live data, or revise the advertised auto-promotion documentation to state
that follow-up records are staging-only in production because followup_resolved
remains false. Preserve the existing --from-json fixture behavior.
- Around line 200-220: Update prune_staging so cases with missing or unparseable
harvested_at are conservatively expired rather than treating now as their
first_seen time. Use _parse_ts within the existing case loop and skip the case
when it returns no valid timestamp; preserve the current age-based expiry and
deduplication behavior for valid timestamps.
- Around line 321-402: Update _load so it returns the supplied default only when
the target file is missing, while propagating OSError and JSONDecodeError for
existing unreadable or malformed files. Ensure main’s corpus loading cannot
silently treat a corrupted existing corpus as empty before the --write path;
retain the empty default for genuinely absent corpus and staging files.
- Around line 226-260: Restrict _REVERT_REF and its use in _reverted_pr_numbers
to extract only the original PR number from GitHub’s default revert title
format, rather than every `#N` mention across title and body text. Match the
revert title structure and stop scanning the body for unrelated references;
preserve the resulting set[int] behavior for valid revert PRs.
- Around line 262-315: Update _gh_json to enforce a finite subprocess timeout
while preserving its existing non-shell invocation, and wrap each repo’s fetch
work in fetch_records—including _gh_json and _reverted_pr_numbers—in a
subprocess.SubprocessError handler. Log the failing repository and continue
processing remaining repos so one timeout or gh failure does not abort the
harvest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0be2fdd-b2f3-4a6e-869a-4b2b30d70036
📒 Files selected for processing (7)
.github/workflows/maint-79-verifier-corpus-harvest.ymlconfig/model_eval_corpus_staging.jsonconfig/model_selection_policy.jsonlangsmith-fleet-worker-attempt.jsontests/tools/test_harvest_verifier_corpus.pytests/tools/test_model_eval_pilot.pytools/harvest_verifier_corpus.py
| paths: | ||
| - tools/harvest_verifier_corpus.py | ||
| - tests/tools/test_harvest_verifier_corpus.py | ||
| - config/model_selection_policy.json | ||
| - config/model_eval_corpus_staging.json |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Run pilot-corpus validation for harvest PRs.
config/model_eval_pilot.json and tests/tools/test_model_eval_pilot.py are excluded from the trigger, while Line 48 runs only the harvester tests. A corpus-only update can therefore bypass the frozen-seed and harvested-case invariants before auto-merge. Add both paths and run both test modules.
Proposed fix
pull_request:
paths:
- tools/harvest_verifier_corpus.py
- tests/tools/test_harvest_verifier_corpus.py
+ - tests/tools/test_model_eval_pilot.py
- config/model_selection_policy.json
- config/model_eval_corpus_staging.json
+ - config/model_eval_pilot.json
...
- name: Unit tests
- run: python -m pytest tests/tools/test_harvest_verifier_corpus.py -q
+ run: python -m pytest tests/tools/test_harvest_verifier_corpus.py tests/tools/test_model_eval_pilot.py -qAs per path instructions, “Flag new or changed behavior with no accompanying test.”
Also applies to: 47-48
🤖 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 @.github/workflows/maint-79-verifier-corpus-harvest.yml around lines 23 - 27,
Update the workflow path filters to include config/model_eval_pilot.json and
tests/tools/test_model_eval_pilot.py, then adjust the test command at the
existing test step to run both test modules: test_harvest_verifier_corpus.py and
test_model_eval_pilot.py. Preserve the current harvest-related trigger paths and
validation behavior.
Source: Path instructions
| argv = [ | ||
| "--policy", | ||
| str(pol_p), | ||
| "--corpus", | ||
| str(cor_p), | ||
| "--staging", | ||
| str(stg_p), | ||
| "--from-json", | ||
| str(rec_p), | ||
| "--write", | ||
| ] | ||
| assert hv.main(argv) == 0 | ||
| grown = json.loads(cor_p.read_text()) | ||
| assert [c["pr"] for c in grown["cases"]] == [1] # only the stable merge promoted | ||
| staged = json.loads(stg_p.read_text()) | ||
| assert [c["pr"] for c in staged["cases"]] == [2] # recent merge staged |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test the default dry-run path.
This test always passes --write; it never verifies that the default invocation leaves corpus and staging files unchanged. Add a no---write invocation and assert no persistence occurs.
As per path instructions, “Flag new or changed behavior with no accompanying test.”
🤖 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/tools/test_harvest_verifier_corpus.py` around lines 141 - 156, Add
coverage in the harvest verifier test around hv.main to invoke the same
arguments without --write, then assert both corpus and staging files remain
unchanged. Preserve the existing --write assertions for promoted and staged
cases, and verify the default dry-run behavior before any persistence occurs.
Source: Path instructions
| def classify( | ||
| record: dict[str, Any], *, now: datetime, stability_days: int | ||
| ) -> dict[str, Any] | None: | ||
| """Label a single PR record from its realized outcome. | ||
|
|
||
| Returns ``{expected_verdict, category, confidence}`` or ``None`` when the PR | ||
| carries no usable verifier signal (e.g. never merged). | ||
| """ | ||
| if not record.get("merged"): | ||
| return None | ||
| merged_at = _parse_ts(record.get("merged_at")) | ||
| if merged_at is None: | ||
| return None | ||
|
|
||
| if record.get("reverted"): | ||
| return { | ||
| "expected_verdict": "NON_PASS", | ||
| "category": CATEGORY_REGRESSION, | ||
| "confidence": "high", | ||
| } | ||
|
|
||
| if record.get("verifier_followup"): | ||
| confidence = "high" if record.get("followup_resolved") else "low" | ||
| return { | ||
| "expected_verdict": "NON_PASS", | ||
| "category": CATEGORY_FOLLOW_UP, | ||
| "confidence": confidence, | ||
| } | ||
|
|
||
| age_days = (now - merged_at).total_seconds() / 86400.0 | ||
| confidence = "high" if age_days >= stability_days else "low" | ||
| return { | ||
| "expected_verdict": "PASS", | ||
| "category": CATEGORY_CLEAN_PASS, | ||
| "confidence": confidence, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
High-confidence follow-up-required path is unreachable from live data.
classify() only returns confidence: "high" for the follow-up category when record["followup_resolved"] is true (Line 93), but fetch_records() always sets "followup_resolved": False (Line 312), with a comment explaining resolution needs semantic judgment. In practice this means the third auto-promotion mechanism the module docstring advertises (lines 17-18: "resolved verifier-driven follow-up → high-confidence") never fires outside of --from-json test fixtures — real follow-up cases will always be staged and eventually auto-expire, never promoted. Worth either implementing real resolution detection or updating the docstring/note to reflect that this category is currently staging-only in production.
🤖 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 `@tools/harvest_verifier_corpus.py` around lines 71 - 106, Update the follow-up
classification behavior and module documentation consistently: either implement
resolution detection in fetch_records so classify can produce high-confidence
CATEGORY_FOLLOW_UP results from live data, or revise the advertised
auto-promotion documentation to state that follow-up records are staging-only in
production because followup_resolved remains false. Preserve the existing
--from-json fixture behavior.
| def prune_staging( | ||
| staging: dict[str, Any], stage_new: list[dict[str, Any]], *, now: datetime, expiry_days: int | ||
| ) -> dict[str, Any]: | ||
| """Merge new staging cases and drop any older than ``expiry_days`` (auto-expiry).""" | ||
| kept: list[dict[str, Any]] = [] | ||
| seen: set[tuple[Any, Any]] = set() | ||
| for case in list(staging.get("cases", [])) + stage_new: | ||
| key = (case.get("repo"), case.get("pr")) | ||
| if key in seen: | ||
| continue | ||
| first_seen = _parse_ts(case.get("harvested_at")) or now | ||
| if (now - first_seen).total_seconds() / 86400.0 > expiry_days: | ||
| continue | ||
| seen.add(key) | ||
| kept.append(case) | ||
| return { | ||
| "schema": staging.get("schema", "verifier-corpus-staging/v1"), | ||
| "note": "FYI-only. Auto-expiring candidate cases pending stability or adjudication; " | ||
| "nothing here gates anything.", | ||
| "cases": kept, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Malformed/missing harvested_at is treated as "just harvested" instead of expired.
first_seen = _parse_ts(case.get("harvested_at")) or now (Line 210) falls back to now when the timestamp is missing or unparseable, which makes (now - first_seen) evaluate to ~0 — the opposite of what the auto-expiry safety net intends. A corrupted/missing harvested_at should be treated conservatively (i.e., expire it), not perpetually renewed as "fresh," since the whole design relies on this file self-cleaning without human action.
🛠️ Proposed fix
- first_seen = _parse_ts(case.get("harvested_at")) or now
- if (now - first_seen).total_seconds() / 86400.0 > expiry_days:
+ first_seen = _parse_ts(case.get("harvested_at"))
+ if first_seen is None or (now - first_seen).total_seconds() / 86400.0 > expiry_days:
continue📝 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.
| def prune_staging( | |
| staging: dict[str, Any], stage_new: list[dict[str, Any]], *, now: datetime, expiry_days: int | |
| ) -> dict[str, Any]: | |
| """Merge new staging cases and drop any older than ``expiry_days`` (auto-expiry).""" | |
| kept: list[dict[str, Any]] = [] | |
| seen: set[tuple[Any, Any]] = set() | |
| for case in list(staging.get("cases", [])) + stage_new: | |
| key = (case.get("repo"), case.get("pr")) | |
| if key in seen: | |
| continue | |
| first_seen = _parse_ts(case.get("harvested_at")) or now | |
| if (now - first_seen).total_seconds() / 86400.0 > expiry_days: | |
| continue | |
| seen.add(key) | |
| kept.append(case) | |
| return { | |
| "schema": staging.get("schema", "verifier-corpus-staging/v1"), | |
| "note": "FYI-only. Auto-expiring candidate cases pending stability or adjudication; " | |
| "nothing here gates anything.", | |
| "cases": kept, | |
| } | |
| def prune_staging( | |
| staging: dict[str, Any], stage_new: list[dict[str, Any]], *, now: datetime, expiry_days: int | |
| ) -> dict[str, Any]: | |
| """Merge new staging cases and drop any older than ``expiry_days`` (auto-expiry).""" | |
| kept: list[dict[str, Any]] = [] | |
| seen: set[tuple[Any, Any]] = set() | |
| for case in list(staging.get("cases", [])) + stage_new: | |
| key = (case.get("repo"), case.get("pr")) | |
| if key in seen: | |
| continue | |
| first_seen = _parse_ts(case.get("harvested_at")) | |
| if first_seen is None or (now - first_seen).total_seconds() / 86400.0 > expiry_days: | |
| continue | |
| seen.add(key) | |
| kept.append(case) | |
| return { | |
| "schema": staging.get("schema", "verifier-corpus-staging/v1"), | |
| "note": "FYI-only. Auto-expiring candidate cases pending stability or adjudication; " | |
| "nothing here gates anything.", | |
| "cases": kept, | |
| } |
🤖 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 `@tools/harvest_verifier_corpus.py` around lines 200 - 220, Update
prune_staging so cases with missing or unparseable harvested_at are
conservatively expired rather than treating now as their first_seen time. Use
_parse_ts within the existing case loop and skip the case when it returns no
valid timestamp; preserve the current age-based expiry and deduplication
behavior for valid timestamps.
| def _gh_json(args: list[str]) -> Any: | ||
| result = subprocess.run(["gh", *args], check=True, capture_output=True, text=True) # noqa: S607 | ||
| return json.loads(result.stdout or "null") | ||
|
|
||
|
|
||
| _REVERT_REF = re.compile(r"#(\d+)") | ||
|
|
||
|
|
||
| def _reverted_pr_numbers(repo: str) -> set[int]: # pragma: no cover - integration | ||
| """PR numbers referenced by any recent revert PR/commit in the repo.""" | ||
| reverts = ( | ||
| _gh_json( | ||
| [ | ||
| "pr", | ||
| "list", | ||
| "--repo", | ||
| repo, | ||
| "--state", | ||
| "merged", | ||
| "--search", | ||
| "revert in:title", | ||
| "--limit", | ||
| "100", | ||
| "--json", | ||
| "title,body", | ||
| ] | ||
| ) | ||
| or [] | ||
| ) | ||
| numbers: set[int] = set() | ||
| for pr in reverts: | ||
| for match in _REVERT_REF.findall(f"{pr.get('title', '')} {pr.get('body', '')}"): | ||
| numbers.add(int(match)) | ||
| return numbers | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Revert-reference regex over-matches, risking mislabeled regression-after-merge cases.
_REVERT_REF = re.compile(r"#(\d+)") (Line 231) is applied to the full title+body text of every revert-titled PR (Line 257). Any unrelated #N mention in that text (e.g. "fixes #456", "follow-up to #789") gets added to the reverted-PR set, so an unrelated, perfectly healthy merged PR could be auto-labeled high-confidence regression-after-merge and land in the frozen corpus untouched by human review. Since this harvester's whole premise is trustworthy auto-labeled outcomes, a loose match here directly threatens corpus quality.
(Note: the static-analysis "XPath injection" hint on this regex is a false positive — this is a plain Python re.findall, not XPath.)
🛠️ Proposed fix — anchor to GitHub's default revert title format
-_REVERT_REF = re.compile(r"#(\d+)")
+_REVERT_REF = re.compile(r'^Revert ".*"\s*\(#(\d+)\)\s*$')
...
- for pr in reverts:
- for match in _REVERT_REF.findall(f"{pr.get('title', '')} {pr.get('body', '')}"):
- numbers.add(int(match))
+ for pr in reverts:
+ match = _REVERT_REF.match(pr.get("title", "") or "")
+ if match:
+ numbers.add(int(match.group(1)))📝 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.
| def _gh_json(args: list[str]) -> Any: | |
| result = subprocess.run(["gh", *args], check=True, capture_output=True, text=True) # noqa: S607 | |
| return json.loads(result.stdout or "null") | |
| _REVERT_REF = re.compile(r"#(\d+)") | |
| def _reverted_pr_numbers(repo: str) -> set[int]: # pragma: no cover - integration | |
| """PR numbers referenced by any recent revert PR/commit in the repo.""" | |
| reverts = ( | |
| _gh_json( | |
| [ | |
| "pr", | |
| "list", | |
| "--repo", | |
| repo, | |
| "--state", | |
| "merged", | |
| "--search", | |
| "revert in:title", | |
| "--limit", | |
| "100", | |
| "--json", | |
| "title,body", | |
| ] | |
| ) | |
| or [] | |
| ) | |
| numbers: set[int] = set() | |
| for pr in reverts: | |
| for match in _REVERT_REF.findall(f"{pr.get('title', '')} {pr.get('body', '')}"): | |
| numbers.add(int(match)) | |
| return numbers | |
| def _gh_json(args: list[str]) -> Any: | |
| result = subprocess.run(["gh", *args], check=True, capture_output=True, text=True) # noqa: S607 | |
| return json.loads(result.stdout or "null") | |
| _REVERT_REF = re.compile(r'^Revert ".*"\s*\(#(\d+)\)\s*$') | |
| def _reverted_pr_numbers(repo: str) -> set[int]: # pragma: no cover - integration | |
| """PR numbers referenced by any recent revert PR/commit in the repo.""" | |
| reverts = ( | |
| _gh_json( | |
| [ | |
| "pr", | |
| "list", | |
| "--repo", | |
| repo, | |
| "--state", | |
| "merged", | |
| "--search", | |
| "revert in:title", | |
| "--limit", | |
| "100", | |
| "--json", | |
| "title,body", | |
| ] | |
| ) | |
| or [] | |
| ) | |
| numbers: set[int] = set() | |
| for pr in reverts: | |
| match = _REVERT_REF.match(pr.get("title", "") or "") | |
| if match: | |
| numbers.add(int(match.group(1))) | |
| return numbers |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 226-226: Command coming from incoming request
Context: subprocess.run(["gh", *args], check=True, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[warning] 256-256: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: _REVERT_REF.findall(f"{pr.get('title', '')} {pr.get('body', '')}")
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
🤖 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 `@tools/harvest_verifier_corpus.py` around lines 226 - 260, Restrict
_REVERT_REF and its use in _reverted_pr_numbers to extract only the original PR
number from GitHub’s default revert title format, rather than every `#N` mention
across title and body text. Match the revert title structure and stop scanning
the body for unrelated references; preserve the resulting set[int] behavior for
valid revert PRs.
| def fetch_records( | ||
| repos: list[str], *, per_repo: int, stability_days: int, harvest_window_days: int | ||
| ) -> list[dict[str, Any]]: # pragma: no cover - integration | ||
| """Fetch PRs that crossed the stability line recently, with revert/follow-up signals. | ||
|
|
||
| Targets PRs merged in ``[now - stability_days - harvest_window, now - stability_days]`` | ||
| so every candidate is already past the stability window (a newest-N fetch only | ||
| returns too-recent merges that can never promote — the live-data failure mode | ||
| this window fixes). | ||
| """ | ||
| now = datetime.now(UTC) | ||
| end = (now - timedelta(days=stability_days)).date().isoformat() | ||
| start = (now - timedelta(days=stability_days + harvest_window_days)).date().isoformat() | ||
| records: list[dict[str, Any]] = [] | ||
| for repo in repos: | ||
| merged = ( | ||
| _gh_json( | ||
| [ | ||
| "pr", | ||
| "list", | ||
| "--repo", | ||
| repo, | ||
| "--state", | ||
| "merged", | ||
| "--search", | ||
| f"merged:{start}..{end}", | ||
| "--limit", | ||
| str(per_repo), | ||
| "--json", | ||
| "number,title,mergedAt,body,labels", | ||
| ] | ||
| ) | ||
| or [] | ||
| ) | ||
| reverted = _reverted_pr_numbers(repo) | ||
| for pr in merged: | ||
| number = pr.get("number") | ||
| labels = {lb.get("name") for lb in pr.get("labels", []) if isinstance(lb, dict)} | ||
| records.append( | ||
| { | ||
| "repo": repo, | ||
| "pr": number, | ||
| "merged": True, | ||
| "merged_at": pr.get("mergedAt"), | ||
| "reverted": number in reverted, | ||
| "verifier_followup": bool( | ||
| labels & {"verify:create-issue", "verifier-followup"} | ||
| ), | ||
| # Resolution of a follow-up needs semantic judgment; stay conservative | ||
| # (unresolved -> staged, never auto-labeled NON_PASS). | ||
| "followup_resolved": False, | ||
| } | ||
| ) | ||
| return records |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
gh subprocess calls lack timeouts and per-repo failure isolation.
_gh_json (Line 227) has no timeout=, so a hung gh call (auth prompt, network stall) blocks the whole harvest run. Additionally, check=True with no try/except around each repo's _gh_json/_reverted_pr_numbers call in fetch_records's loop (lines 276-315) means one flaky/renamed/inaccessible repo in source_repos aborts the entire scheduled harvest, losing candidate signal from every other repo for that run.
(The static-analysis "OS command injection" hint on _gh_json is a false positive here — no shell=True is used and repo values come from the static policy config, not external request input.)
🛠️ Suggested direction
-def _gh_json(args: list[str]) -> Any:
- result = subprocess.run(["gh", *args], check=True, capture_output=True, text=True) # noqa: S607
+def _gh_json(args: list[str], *, timeout: int = 60) -> Any:
+ result = subprocess.run(
+ ["gh", *args], check=True, capture_output=True, text=True, timeout=timeout # noqa: S607
+ )
return json.loads(result.stdout or "null")And wrap the per-repo body in fetch_records with try/except subprocess.SubprocessError to skip and log a failing repo rather than aborting the whole run.
🤖 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 `@tools/harvest_verifier_corpus.py` around lines 262 - 315, Update _gh_json to
enforce a finite subprocess timeout while preserving its existing non-shell
invocation, and wrap each repo’s fetch work in fetch_records—including _gh_json
and _reverted_pr_numbers—in a subprocess.SubprocessError handler. Log the
failing repository and continue processing remaining repos so one timeout or gh
failure does not abort the harvest.
| def _load(path: Path, default: Any) -> Any: | ||
| try: | ||
| return json.loads(path.read_text(encoding="utf-8")) | ||
| except (OSError, json.JSONDecodeError): | ||
| return default | ||
|
|
||
|
|
||
| def _growth_config(policy: dict[str, Any], profile: str) -> dict[str, Any]: | ||
| prof = policy.get("profiles", {}).get(profile, {}) | ||
| cfg = dict(prof.get("corpus_growth", {})) | ||
| cfg.setdefault("enabled", False) | ||
| cfg.setdefault("stability_days", 30) | ||
| cfg.setdefault("staging_expiry_days", 60) | ||
| cfg.setdefault("harvest_window_days", 60) | ||
| cfg.setdefault("max_corpus_size", 150) | ||
| cfg.setdefault( | ||
| "category_caps", | ||
| {CATEGORY_CLEAN_PASS: 40, CATEGORY_REGRESSION: 20, CATEGORY_FOLLOW_UP: 20}, | ||
| ) | ||
| cfg.setdefault("source_repos", []) | ||
| return cfg | ||
|
|
||
|
|
||
| def main(argv: list[str] | None = None) -> int: | ||
| parser = argparse.ArgumentParser( | ||
| description="Grow the verifier corpus from realized PR outcomes." | ||
| ) | ||
| parser.add_argument("--corpus", type=Path, default=DEFAULT_CORPUS_PATH) | ||
| parser.add_argument("--staging", type=Path, default=DEFAULT_STAGING_PATH) | ||
| parser.add_argument("--policy", type=Path, default=DEFAULT_POLICY_PATH) | ||
| parser.add_argument("--profile", default=DEFAULT_PROFILE) | ||
| parser.add_argument("--from-json", type=Path, help="Read pre-fetched PR records instead of gh.") | ||
| parser.add_argument("--per-repo", type=int, default=50) | ||
| parser.add_argument( | ||
| "--write", action="store_true", help="Persist corpus + staging (default: dry-run)." | ||
| ) | ||
| args = parser.parse_args(argv) | ||
|
|
||
| policy = _load(args.policy, {}) | ||
| cfg = _growth_config(policy, args.profile) | ||
| if not cfg["enabled"]: | ||
| print("corpus_growth is disabled in the policy; nothing to do.") | ||
| return 0 | ||
|
|
||
| now = datetime.now(UTC) | ||
| if args.from_json: | ||
| records = _load(args.from_json, []) | ||
| else: # pragma: no cover - integration path | ||
| records = fetch_records( | ||
| list(cfg["source_repos"]), | ||
| per_repo=args.per_repo, | ||
| stability_days=int(cfg["stability_days"]), | ||
| harvest_window_days=int(cfg["harvest_window_days"]), | ||
| ) | ||
|
|
||
| promote, stage = partition(records, now=now, stability_days=int(cfg["stability_days"])) | ||
| corpus = _load(args.corpus, {"cases": []}) | ||
| grown, added = grow_corpus( | ||
| corpus, | ||
| promote, | ||
| max_size=int(cfg["max_corpus_size"]), | ||
| category_caps=dict(cfg.get("category_caps") or {}), | ||
| ) | ||
| staging = _load(args.staging, {"cases": []}) | ||
| new_staging = prune_staging( | ||
| staging, stage, now=now, expiry_days=int(cfg["staging_expiry_days"]) | ||
| ) | ||
|
|
||
| print( | ||
| f"harvest: {len(records)} records → {len(added)} promoted " | ||
| f"(corpus {len(corpus.get('cases', []))}→{len(grown.get('cases', []))}), " | ||
| f"{len(new_staging['cases'])} staged (FYI, auto-expiring)." | ||
| ) | ||
| for case in added: | ||
| print(f" + {case['case_id']} {case['expected_verdict']} ({case['category']})") | ||
|
|
||
| if args.write: | ||
| if added: | ||
| args.corpus.write_text(json.dumps(grown, indent=2) + "\n", encoding="utf-8") | ||
| args.staging.write_text(json.dumps(new_staging, indent=2) + "\n", encoding="utf-8") | ||
| print(f"wrote corpus={args.corpus} staging={args.staging}") | ||
| return 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Silent load failure can wipe the frozen corpus on --write.
_load() (lines 321-325) catches both OSError and json.JSONDecodeError and returns the caller-supplied default. In main(), corpus = _load(args.corpus, {"cases": []}) (Line 377) uses that same fallback for the existing frozen pilot corpus. If the corpus file is ever unreadable or transiently corrupted (bad encoding, partial write from a previous run, wrong path), this silently proceeds as if the corpus were empty, and — with --write — args.corpus.write_text(json.dumps(grown, ...)) (Line 399) then overwrites the real file with only the newly-harvested cases, permanently destroying the human-adjudicated, versioned seed corpus this tool explicitly says must stay "frozen, versioned, and trustworthy" (module docstring, lines 6-7). The same swallow applies to args.staging (Line 384), though the FYI/auto-expiring nature there makes that loss far less severe.
As per path instructions, "Flag new or changed behavior with no accompanying test, silently swallowed exceptions, and unguarded NaN/None propagation" for **/*.py files — this is exactly such a swallowed exception feeding a destructive write path.
🛠️ Proposed fix — only default on missing file, not on corrupt JSON
def _load(path: Path, default: Any) -> Any:
- try:
- return json.loads(path.read_text(encoding="utf-8"))
- except (OSError, json.JSONDecodeError):
- return default
+ try:
+ text = path.read_text(encoding="utf-8")
+ except FileNotFoundError:
+ return default
+ return json.loads(text)This preserves "file doesn't exist yet" as a legitimate empty-default case while letting a corrupted/unreadable existing corpus abort the run loudly instead of being silently overwritten.
📝 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.
| def _load(path: Path, default: Any) -> Any: | |
| try: | |
| return json.loads(path.read_text(encoding="utf-8")) | |
| except (OSError, json.JSONDecodeError): | |
| return default | |
| def _growth_config(policy: dict[str, Any], profile: str) -> dict[str, Any]: | |
| prof = policy.get("profiles", {}).get(profile, {}) | |
| cfg = dict(prof.get("corpus_growth", {})) | |
| cfg.setdefault("enabled", False) | |
| cfg.setdefault("stability_days", 30) | |
| cfg.setdefault("staging_expiry_days", 60) | |
| cfg.setdefault("harvest_window_days", 60) | |
| cfg.setdefault("max_corpus_size", 150) | |
| cfg.setdefault( | |
| "category_caps", | |
| {CATEGORY_CLEAN_PASS: 40, CATEGORY_REGRESSION: 20, CATEGORY_FOLLOW_UP: 20}, | |
| ) | |
| cfg.setdefault("source_repos", []) | |
| return cfg | |
| def main(argv: list[str] | None = None) -> int: | |
| parser = argparse.ArgumentParser( | |
| description="Grow the verifier corpus from realized PR outcomes." | |
| ) | |
| parser.add_argument("--corpus", type=Path, default=DEFAULT_CORPUS_PATH) | |
| parser.add_argument("--staging", type=Path, default=DEFAULT_STAGING_PATH) | |
| parser.add_argument("--policy", type=Path, default=DEFAULT_POLICY_PATH) | |
| parser.add_argument("--profile", default=DEFAULT_PROFILE) | |
| parser.add_argument("--from-json", type=Path, help="Read pre-fetched PR records instead of gh.") | |
| parser.add_argument("--per-repo", type=int, default=50) | |
| parser.add_argument( | |
| "--write", action="store_true", help="Persist corpus + staging (default: dry-run)." | |
| ) | |
| args = parser.parse_args(argv) | |
| policy = _load(args.policy, {}) | |
| cfg = _growth_config(policy, args.profile) | |
| if not cfg["enabled"]: | |
| print("corpus_growth is disabled in the policy; nothing to do.") | |
| return 0 | |
| now = datetime.now(UTC) | |
| if args.from_json: | |
| records = _load(args.from_json, []) | |
| else: # pragma: no cover - integration path | |
| records = fetch_records( | |
| list(cfg["source_repos"]), | |
| per_repo=args.per_repo, | |
| stability_days=int(cfg["stability_days"]), | |
| harvest_window_days=int(cfg["harvest_window_days"]), | |
| ) | |
| promote, stage = partition(records, now=now, stability_days=int(cfg["stability_days"])) | |
| corpus = _load(args.corpus, {"cases": []}) | |
| grown, added = grow_corpus( | |
| corpus, | |
| promote, | |
| max_size=int(cfg["max_corpus_size"]), | |
| category_caps=dict(cfg.get("category_caps") or {}), | |
| ) | |
| staging = _load(args.staging, {"cases": []}) | |
| new_staging = prune_staging( | |
| staging, stage, now=now, expiry_days=int(cfg["staging_expiry_days"]) | |
| ) | |
| print( | |
| f"harvest: {len(records)} records → {len(added)} promoted " | |
| f"(corpus {len(corpus.get('cases', []))}→{len(grown.get('cases', []))}), " | |
| f"{len(new_staging['cases'])} staged (FYI, auto-expiring)." | |
| ) | |
| for case in added: | |
| print(f" + {case['case_id']} {case['expected_verdict']} ({case['category']})") | |
| if args.write: | |
| if added: | |
| args.corpus.write_text(json.dumps(grown, indent=2) + "\n", encoding="utf-8") | |
| args.staging.write_text(json.dumps(new_staging, indent=2) + "\n", encoding="utf-8") | |
| print(f"wrote corpus={args.corpus} staging={args.staging}") | |
| return 0 | |
| def _load(path: Path, default: Any) -> Any: | |
| try: | |
| text = path.read_text(encoding="utf-8") | |
| except FileNotFoundError: | |
| return default | |
| return json.loads(text) |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 398-398: use jsonify instead of json.dumps for JSON output
Context: json.dumps(grown, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 399-399: use jsonify instead of json.dumps for JSON output
Context: json.dumps(new_staging, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@tools/harvest_verifier_corpus.py` around lines 321 - 402, Update _load so it
returns the supplied default only when the target file is missing, while
propagating OSError and JSONDecodeError for existing unreadable or malformed
files. Ensure main’s corpus loading cannot silently treat a corrupted existing
corpus as empty before the --write path; retain the empty default for genuinely
absent corpus and staging files.
Source: Path instructions
2 participants
Part of #2819 (self-feeding verifier-model promotion) — move 2 of 3. Builds on move 1 (#2831).
Problem
The evaluation corpus (
config/model_eval_pilot.json) was hand-built and never grows, so the approval benchmark stays perpetually under its 75-case minimum and no model is ever promoted. The evidence supply chain — not the ranking math — is the real chokepoint.Change
Harvest new corpus cases from outcomes the world has already adjudicated by merging or reverting a PR:
tools/harvest_verifier_corpus.py—classify()labels a merged PR from its realized outcome:clean-pass(PASS)regression-after-merge(NON_PASS)follow-up-required(NON_PASS)High-confidence cases auto-promote into the frozen corpus (version →
+harvestN, dedup by repo+pr, per-category caps so easy-to-sourceclean-passcan't flood it and starve category balance). Ambiguous cases (too-recent to be stable, unresolved follow-up) route to an FYI-only staging file that auto-expires — no adjudication backlog can accumulate.fetch_records()targets a stability-aged merged-date window (a newest-N fetch only returns too-recent merges that can never promote — a live-data failure mode I hit and fixed).config/model_selection_policy.json—corpus_growthblock (stability/expiry/window days, max size, category caps, the 12-repo lane fleet as sources).tests/tools/test_model_eval_pilot.py— the freeze test now digest-freezes only the owner-adjudicated seed (provenance != harvested) and validates harvested cases structurally, so growth is allowed but silent tampering with the seed still reddens CI.maint-79— weekly + dispatch harvest that opens an auto-merging corpus-growth PR (the PR is the audit trail, never a gate).Design guarantees
stale-verifier-claim,review-thread-debt,missing-acceptance-criterion) cannot be labeled from outcomes and are never machine-added; the original 30 seed cases stay digest-frozen.new_catalog_models_auto_promote=falseand human approval still hold. That gate is move 3.Verification (local, CI-pinned)
fetch_records+partition+grow_corpusagainst realstranske/Manager-DatabasePRs — 30→51 cases, seed digest still frozen, onlyclean-passharvested, ids unique. The grown corpus passes the freeze test, confirming the future auto-PR merges cleanly.Summary by CodeRabbit
New Features
Tests