Skip to content

fix(tests): repair changed-scope drift and stale noema cancel-step test - #1877

Merged
seonghobae merged 5 commits into
mainfrom
fix/changed-scope-drift-and-noema-cancel-step
Sep 5, 2026
Merged

fix(tests): repair changed-scope drift and stale noema cancel-step test#1877
seonghobae merged 5 commits into
mainfrom
fix/changed-scope-drift-and-noema-cancel-step

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Summary

Fixes 2 pre-existing tests/ failures that independently block agent-review-runtime-quality-ci.yml's "Verify scheduler and contextual-orchestrator review-repair contracts" step — it runs pytest -q --cov=... --cov-fail-under=100 with no positional test-file arguments, so it implicitly collects and runs pytest's default full-tests/-directory discovery. Any failing test anywhere in tests/ fails this required check. Confirmed unrelated to .github#1875 (which fixes 3 separate stale hourly-cron tests) — these two test files are untouched by that PR's diff.

Bug 1 — test_gate_job_is_byte_identical_across_the_five_workflows_apart_from_if

strix.yml's changed-scope job had textually drifted from its byte-identical siblings in security-scan.yml/sast-semgrep.yml. Root cause: PR #1869 ("retire review scans when PRs return to draft") folded strix.yml's if: condition onto a multi-line >- block scalar to add converted_to_draft handling:

if: >-
  github.event_name != 'pull_request_target' ||
  (github.event.action != 'closed' && github.event.action != 'converted_to_draft')

The test's normalizer only strips lines that literally start with if: (by design — each workflow is allowed its own single-line if:), so the two continuation lines survived and broke the intended byte-for-byte parity of the rest of the job. Fix: collapsed it to one physical if: line, same expression, no semantic change:

if: github.event_name != 'pull_request_target' || (github.event.action != 'closed' && github.event.action != 'converted_to_draft')

Bug 2 — test_noema_close_cleanup_selects_only_the_closed_pr_across_shared_display_titles

This was a stale test, not a production regression. The same PR #1869 generalized noema-review.yml's cancel-closed-pr-runs job to also cover converted_to_draft (not just closed):

  • step renamed "...for the closed pull request""...for the inactive pull request"
  • env vars renamed CLOSED_PR_NUMBER/CLOSED_PR_HEAD_SHAINACTIVE_PR_NUMBER/INACTIVE_PR_HEAD_SHA, plus new PR_ACTION
  • added a live_target_matches live-PR re-verification (mirroring strix.yml's identical job) before every cancellation pass

tests/test_noema_review_gate.py's equivalent tests were updated for this at the time; this one test in test_noema_orchestrator_workflow_contract.py was missed, so it still looked up the old step name and passed the old env var, and the script's un-mocked live_target_matches gh lookup would have failed regardless once the step name matched.

Verified the protected invariant is intact, not broken: the PR #1507 "sibling Noema runs evade cancellation" fix (matching via GitHub's own pull_requests[] array, not just display_title) is unchanged in the current script — confirmed by reading the full cancel_runs() jq selector. No production regression; only the test needed updating. Updated it to the new step name/env vars and taught its fake gh to answer the new pulls/<number> live-state lookup.

Validation

  1. PYTHONPATH=. python3 -m pytest tests/test_docs_only_pr_runner_admission.py tests/test_noema_orchestrator_workflow_contract.py tests/test_required_workflow_queue_contract.py -v91 passed.
  2. Exact unscoped command from the CI step (--cov=scripts.ci.pr_review_conflict_scope --cov=scripts.ci.pr_review_autofix_context --cov=scripts.ci.zdr_policy --cov=scripts.ci.contextual_orchestrator_review_policy --cov-branch --cov-fail-under=100) — 100% coverage reached; only the pre-existing stale-cron failures (owned by .github#1875) and the known local-sandbox gh-missing limitation (test_strix_rerun_job_selection.py) remain (20 failed, down from 22 on a clean baseline; both targeted tests now pass).
  3. Full suite (coverage run -m pytest tests --ignore=tests/test_queue_cancellation_open_pr_revalidation.py --ignore=tests/test_queue_cancellation_revalidation.py -q): baseline on clean origin/main was 22 failed, 2830 passed; after this fix, 20 failed, 2832 passed — exactly the 2 targeted bugs fixed, zero new regressions.
  4. coverage report --include='scripts/ci/*' --show-missing and interrogate scripts/ci: both identical to the clean-baseline result (99% / 98.3%, from the two revalidation test files excluded above, which the harness doesn't mock gh/git for) — confirmed unaffected by this test-only/workflow-YAML-only change via a stash-and-compare.
  5. git diff --check clean; strix.yml re-parses as valid YAML; the edited test file re-parses as valid Python.
  6. Added a CHANGELOG.md entry under the first ## [Unreleased] heading.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX


Generated by Claude Code

Two pre-existing tests/ failures blocked the unscoped pytest discovery
run in agent-review-runtime-quality-ci.yml's review-repair contracts
step, unrelated to the hourly-cron fixes in #1875:

- strix.yml's changed-scope job if: had drifted onto a multi-line `>-`
  block scalar when PR #1869 added converted_to_draft handling, and the
  extra continuation lines broke byte-identical parity with
  security-scan.yml/sast-semgrep.yml's copies. Collapsed back to one
  physical if: line with the same expression.
- test_noema_close_cleanup_selects_only_the_closed_pr_across_shared_display_titles
  still targeted the pre-#1869 step name/env vars
  (CLOSED_PR_NUMBER) that PR #1869 renamed to "...for the inactive pull
  request" / INACTIVE_PR_NUMBER/INACTIVE_PR_HEAD_SHA/PR_ACTION when it
  generalized noema-review.yml's cleanup to also cover
  converted_to_draft and added a live_target_matches re-verification.
  tests/test_noema_review_gate.py's equivalent tests were already
  updated; this one was missed. Updated the step name/env vars and
  taught the fake gh to answer the new live-PR lookup -- the PR #1507
  pull_requests[] cancellation-scoping invariant it protects is
  unchanged and still correctly implemented in production.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 17 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 2467e4c8-bab1-48ef-9d49-98238f687c7a

📥 Commits

Reviewing files that changed from the base of the PR and between 1b65dbc and f374f5b.

📒 Files selected for processing (7)
  • .github/workflows/strix.yml
  • CHANGELOG.md
  • scripts/ci/test_strix_quick_gate.sh
  • tests/test_github_hourly_conflict_repair.py
  • tests/test_hourly_review_repair_callers.py
  • tests/test_noema_orchestrator_workflow_contract.py
  • tests/test_strix_rerun_job_selection.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as draft September 5, 2026 00:01
@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head dependency RCA at 183f9703991fff868559cf4847265ead1da7ff03: the Noema/Strix focused slice reaches 23 passing tests plus 17 subtests, but tests/test_strix_rerun_job_selection.py::test_dispatch_strix_reruns_scan_job_not_sibling_publisher still fails (stale_head vs rerun). That missing fixture correction is owned by unmerged #1871, so this PR is not independently GREEN against current main. I returned it to Draft rather than treating two repaired oracles as a complete current-main fix. Re-evaluate after #1871 merges or reconstruct this head on that protected-main generation; no failing test was excluded.

test_dispatch_strix_reruns_scan_job_not_sibling_publisher only mocked
rerun_actions_job, leaving dispatch_strix_evidence's live_dispatch_head_matches
call to invoke the real fetch_pr. In any environment with a real gh CLI on
PATH this hits the actual GitHub API for a synthetic PR that does not exist
there, returning a live/head mismatch ("stale_head") instead of the expected
"rerun"; without gh at all it fails even earlier with a missing executable.

Add monkeypatch.setattr(sched, "fetch_pr", lambda *_args: [pr]) so the
live-head re-read observes the same fixture pr as authoritative, consistent
with how every other GitHub call in this test path is already isolated.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
@seonghobae
seonghobae marked this pull request as ready for review September 5, 2026 00:14

Copy link
Copy Markdown
Contributor Author

Re-evaluated per the diagnosis above (stale_head vs rerun at 183f9703, attributed to the fixture correction in unmerged #1871).

Rather than pulling #1871 wholesale (~1000 unreviewed lines across 22 files, actively churning), I independently traced the exact call path: dispatch_strix_evidencelive_dispatch_head_matchesfetch_pr. The test mocked rerun_actions_job but not fetch_pr, so with a real gh on PATH the live-head re-read hits the actual GitHub API for a synthetic PR that doesn't exist there, returning a live/head mismatch ("stale_head") instead of "rerun" — confirmed this is the only remaining gh call in that code path (matching_actions_job_id, review_dispatch_admitted, and validate_github_repository are all pure/in-memory).

Cherry-picked only the one confirmed-correct fixture line — monkeypatch.setattr(sched, "fetch_pr", lambda *_args: [pr]) — added alongside the existing rerun_actions_job mock, nothing else from #1871's branch.

Verified:

  • tests/test_strix_rerun_job_selection.py now passes cleanly (no gh FileNotFoundError, no stale_head/rerun mismatch).
  • Full suite (coverage run -m pytest tests --ignore=tests/test_queue_cancellation_open_pr_revalidation.py --ignore=tests/test_queue_cancellation_revalidation.py -q): 19 failed / 2833 passed — one fewer failure than this branch's prior 20/2832, all 19 remaining failures pre-existing and unrelated (hourly-cron schedule drift, owned by .github#1875), confirmed via stash-and-compare.
  • coverage report --include='scripts/ci/*' and interrogate scripts/ci: identical to this branch's pre-existing baseline (99% / 98.3%) — test-only change, no production code touched.
  • git diff --check clean.
  • Appended a third-fix addendum to this PR's existing CHANGELOG.md bullet (not a new bullet).

One caveat for the reviewer: GitHub currently reports this PR's mergeable_state as dirtymain advanced independently via #1878, which added an unrelated ## [Unreleased] entry to the same CHANGELOG.md section, producing a textual conflict unrelated to this fix. Pre-existing, out of scope for this targeted change; flagging rather than silently resolving it myself.

Marking ready for review again — this is exactly the re-evaluation your comment called for.


Generated by Claude Code

…ft-and-noema-cancel-step

# Conflicts:
#	CHANGELOG.md

Copy link
Copy Markdown
Contributor Author

Merged origin/main into this branch to resolve the mergeable_state: "dirty" GitHub reported after #1878 ("remove organization queue sweep") landed on main and edited the same ## [Unreleased] CHANGELOG.md section this PR touches — a textual conflict, not a logic one (two unrelated bullets added near the same spot).

Resolution: kept the first ## [Unreleased] heading as the active section (per convention, this file has multiple historical ## [Unreleased] headings) and kept both bullets — this PR's own "Fix two pre-existing tests/ failures..." entry and #1878's three bullets (queue-sweep removal, Noema concurrency-group move, coalescer admission scoping) — merged cleanly with no content dropped.

Verified no other files conflicted (confirmed strix.yml/noema-review.yml/this PR's test files were untouched by #1878's diff, as expected).

Validation after the merge commit (13a71d25):

  • git diff --check: clean.
  • Full suite (coverage run -m pytest tests, excluding the two live-revalidation files per the standing exclusion): 19 failed / 2814 passed — the identical 19 pre-existing, environment/date-driven test_hourly_review_repair_callers.py / test_github_hourly_conflict_repair.py failures present on both the pre-merge PR head and clean origin/main (confirmed via a side-by-side worktree comparison); no new failures introduced by the merge or by #1878.
  • This PR's own targeted files (test_docs_only_pr_runner_admission.py, test_noema_orchestrator_workflow_contract.py, test_strix_rerun_job_selection.py): all 19 tests pass.

Pushed as a normal (non-force) merge commit to fix/changed-scope-drift-and-noema-cancel-step.


Generated by Claude Code

@seonghobae
seonghobae merged commit b5efbc2 into main Sep 5, 2026
3 of 17 checks passed
@seonghobae
seonghobae deleted the fix/changed-scope-drift-and-noema-cancel-step branch September 5, 2026 00:36
seonghobae pushed a commit that referenced this pull request Sep 5, 2026
Resolves the dirty mergeable_state caused by #1877 independently
re-authoring most of this PR's fixes on main.

- tests/test_hourly_review_repair_callers.py and
  tests/test_github_hourly_conflict_repair.py: dropped this branch's
  now-redundant duplicate hunks (same cron assertions fixed to the same
  values) in favor of main's versions verbatim, including main's
  test_central_repository_has_daily_self_caller rename.
- tests/test_pr_review_autofix_nvidia_nim_contract.py: kept the two
  assertions genuinely not covered anywhere on main -- the negative
  cron: "23 */2 * * *" check and the product-neutrality check that
  ContextualWisdomLab/clearfolio is never hard-coded into
  pr-review-fix-scheduler.yml -- layered onto main's already-fixed
  test_review_fix_caller_keeps_the_github_daily_recovery_slot (#1870).
- CHANGELOG.md: kept both this branch's and main's Unreleased bullets
  (additive convention), rewriting this branch's bullet to correctly
  attribute the now-shared-on-main fixes to #1870/#1877 and describe
  only the residual scope this PR still closes.
- docs/product-technical-gap-baseline.md: corrected the
  "opencode-review-dispatch.yml still requesting the starved floating
  image" entry's stale "not yet fixed" sub-note -- the core cron
  assertion was already fixed by #1870 itself, and the other two files
  were fixed on main via #1877. The entry now states this accurately
  and lists the two extra assertions as the actual remaining scope.

Validated: targeted 3-file suite (59 passed), full suite minus the two
excluded queue-cancellation-revalidation files (2833 passed, 1 skipped),
git diff --check clean. Final diff vs origin/main is limited to
CHANGELOG.md, docs/product-technical-gap-baseline.md, and the two new
assertions in tests/test_pr_review_autofix_nvidia_nim_contract.py.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
seonghobae added a commit that referenced this pull request Sep 5, 2026
#1883)

The admission-controller feature burst (#1859-#1869) shipped
review_admission_controller.py, pr_review_merge_scheduler_core.py's
SchedulerAdmissionGate, and (separately, pre-existing) a coverage gap
in audit_codeql_default_setup_rollout.py without full test coverage
or docstrings, breaking the required 100% coverage/docstring gate for
every PR in this repository regardless of that PR's own diff.

The original fix for this landed on .github#1871, which was later
closed in favor of narrower successors (#1877 for the stale schedule
oracles, #1879 for HTTP error response bodies) -- but the coverage and
docstring portion of #1871's delta was dropped in that narrowing and
never reached main. This PR recovers exactly that portion from
#1871's still-present branch (fix/hourly-review-repair-callers-cron-
format-drift) and completes it:

- review_admission_controller.py: 85% -> 100% coverage (new
  tests/test_review_admission_controller.py), 14 missing docstrings
  added across its WorkerBoundary/AdmissionRequest/RequestRecord/
  DispatchLease/ControllerState/DispatchPlan dataclasses and methods.
- audit_codeql_default_setup_rollout.py: 79% -> 100% coverage (new
  tests/test_codeql_default_setup_rollout.py), 2 missing docstrings
  added (parse_args, main).
- pr_review_merge_scheduler_core.py's SchedulerAdmissionGate: 3
  missing docstrings added (__init__ and its two nested closures,
  lease/reconcile_state).

Additionally closed pr_review_merge_scheduler_core.py's own separate,
longer-standing coverage gap (98% -> 100%, unrelated to the
admission-controller work) discovered while verifying this fix would
actually bring main to a green gate rather than a differently-shaped
99%: the durable admission gate's own bounded-budget/stale-head
branches across every dispatch call site (9 "admission_deferred"
checks across post_update_branch_followup/dispatch_draft_review_only/
inspect_pr, plus dispatch_strix_evidence's own two "admission_deferred"/
"stale_head" pairs), reconcile()'s live-head-moved and still-running
branches, rotating_pr_window's/dispatch_draft_review_only's/the
workflow-run classifier's/the empty-PR-close path's/main()'s own
--admission-state-path wiring's remaining gaps, and two untestable
package-import fallback lines marked `# pragma: no cover - package
import path` matching this file's established convention for that
exact pattern.

Full local triad: 2875 passed, 1 skipped; coverage 100%; interrogate
100%.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae added a commit that referenced this pull request Sep 5, 2026
…pline (#1909)

Two rules from mistakes this session actually made and corrected, per the
per-session lane split agreed with the other concurrent sessions (peer 3
took verification discipline in #1907; peer 2 has gate/merge mechanics;
host 1 has close-time diff comparison and noema concurrency; host 2 has
CI failure diagnosis).

- Narrowing a PR does not carry its delta. #1871 was closed in favor of
  #1877 plus #1879; both successors were green, but neither carried the
  coverage/docstring delta, leaving main's required 100% gate broken
  until #1883 recovered it. "Each piece works" and "the pieces together
  cover the original's scope" are different questions.
- Compare content, not ancestry. main mixes squash and merge commits
  (last 200: 153 single-parent, 47 two-parent, counted directly), so
  `git merge-base --is-ancestor` gives false negatives for squashed
  deltas and false positives for reverted merge-commit deltas.
- Never endorse a timeout or retry constant on a model-invocation path
  without reading docs/product-goal-directive.md section 8, which accepts
  more than two hours per model and states speed is not a core
  consideration. #1889/#1890/#1892 each capped a model step at 900s on
  real multi-hour-hang evidence and were all reverted (#1891, #1895).

Every PR number, the section-8 quotes, the parent-count distribution, and
the 100% gate values were verified against the repository directly. An
earlier draft of the timeout bullet cited a section number that does not
exist and attributed a sentence to that file which appears only in
#1891's PR body; both were caught by grepping rather than trusting the
summary that introduced them, and that failure is recorded in the text.

Full suite: 2883 passed, 1 skipped.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants