fix(tests): repair test_model_group_mutations_refresh_audit_events - #1035
Merged
Conversation
The regression test PR #1010's item-25 fix added for the model-groups audit-refresh bug has been silently non-functional since it merged (212ff43) -- it never actually ran the behavior it claims to verify. Found while running contextual-orchestrator's full suite for an unrelated change and seeing it fail on a plain NameError. Four independent bugs, in the order they surfaced once each prior one was fixed: 1. `json`, `shutil`, and `subprocess` are used (Node subprocess harness, JSON-encoding extracted JS source) but were never imported -- an immediate NameError before the test's own logic ever ran. 2. `source_between(start, end)` extracts one admin.py JS function's source by searching for the *next* occurrence of `end` after `start`. Three of the five call sites named a marker several functions further down the file instead of the function's own true next neighbor (e.g. `saveModelGroup`'s end marker was `deleteModelGroup`, ~550 lines and several unrelated functions later, silently swallowing all of it into one bogus "function body"). Retargeted each to its actual immediate successor in admin.py's source order. 3. Even with correct boundaries, `eval("async function foo() {...}")` evaluates a FunctionDeclaration statement, whose completion value is `undefined` per the ECMAScript spec -- not the function itself. Every `const x = eval(...)` in this test was silently binding `undefined`. `source_between` now wraps its extracted source in parentheses, forcing function*-expression* parsing, so `eval` returns the callable. 4. `refreshModelGroupViews`'s real source calls `showModelGroupRefreshWarning(...)`, which the test harness never defined at all (only referenced it as an end marker) -- a ReferenceError once the functions above started actually running. Added the missing extraction+eval for it, matching the pattern used for its four siblings. With all four fixed, the test now genuinely exercises admin.py's real JS save/delete/refresh-audit flow end-to-end via a Node subprocess and passes for the right reason, not by accident. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
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. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🔍 Node dependency remains implicit
The test invokes node, but the main test job neither installs nor versions it. Runner-provided tooling therefore controls this contract test's reproducibility.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+189
to
199
| return "(" + ADMIN_HTML[start_index:end_index].strip() + ")" | ||
|
|
||
| node_script = "\n".join( | ||
| [ | ||
| 'import assert from "node:assert/strict";', | ||
| f"const refreshModelGroups = eval({json.dumps(source_between('async function refreshModelGroups()', ' function showModelGroupRefreshWarning'))});", | ||
| f"const refreshAuditEvents = eval({json.dumps(source_between('async function refreshAuditEvents()', ' async function refreshModelGroupViews'))});", | ||
| f"const refreshModelGroups = eval({json.dumps(source_between('async function refreshModelGroups()', ' async function refreshAuditEvents'))});", | ||
| f"const refreshAuditEvents = eval({json.dumps(source_between('async function refreshAuditEvents()', ' function showModelGroupRefreshWarning'))});", | ||
| f"const showModelGroupRefreshWarning = eval({json.dumps(source_between('function showModelGroupRefreshWarning(message)', ' async function refreshModelGroupViews'))});", | ||
| f"const refreshModelGroupViews = eval({json.dumps(source_between('async function refreshModelGroupViews()', ' async function saveModelGroup'))});", | ||
| f"const saveModelGroup = eval({json.dumps(source_between('async function saveModelGroup(event)', ' async function deleteModelGroup'))});", | ||
| f"const saveModelGroup = eval({json.dumps(source_between('async function saveModelGroup(event)', ' function renderTrace(result)'))});", | ||
| f"const deleteModelGroup = eval({json.dumps(source_between('async function deleteModelGroup(groupName)', ' els.modelGroups.addEventListener'))});", |
Contributor
There was a problem hiding this comment.
📝 Info: Extraction boundaries preserve isolation
Each source_between range ends at the next declaration. Parentheses make the extracted declaration evaluate to a callable without capturing unrelated functions.
Was this helpful? React with 👍 or 👎 to provide feedback.
This was referenced Sep 3, 2026
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
Both sides landed the same repair to ``test_model_group_mutations_refresh_audit_events`` (#1035 on main, 3ca8c94 here), so the only conflicts were two presentational collisions inside that one file: * the ``json``/``shutil``/``subprocess`` imports both sides added, in two different orders -- kept main's ordering, same import set; * the explanation of why ``source_between`` parenthesizes the extracted source (this branch wrote a ``#`` comment, main wrote the same reasoning as the nested function's docstring) -- kept main's docstring, dropping the now-redundant comment. No orchestrator.py conflict: main's two new commits (SearXNG web_search, the test repair) do not touch the ``_orchestrated_provider_completion``, budget, or embedding-cache code this branch has been fixing. Full suite after the merge: 3379 passed, 1 skipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
pushed a commit
that referenced
this pull request
Sep 3, 2026
Bring in main's already-landed fix for the pre-existing tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events NameError (missing 'import json', commit 39a4348 / #1035) that PR #1025 was still behind on. PR #1025's own diff (CHANGELOG.d fragment + tests/test_provider_embedding_batch_backend.py) is untouched by this merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
This was referenced Sep 3, 2026
seonghobae
pushed a commit
that referenced
this pull request
Sep 3, 2026
…ndidate Merged current main to pick up main's test_admin_contract.py `import json` fix (PR #1035) that this PR's stale base predated -- clean, no conflicts. Hosted CI's "Full unit and contract suite" (run 33692781067) then showed two of this PR's own new tests failing with an extra `worker_only` call in client.calls: test_http_auto_preflight_accepts_worker_only_pin_when_free_model_ always_routes and test_coordinator_auto_route_only_pin_succeeds_for_free_model (tests/test_candidate_routing_controls.py). Neither failed in any earlier local round because this sandbox's blocked fast-mlsirm GitHub-archive download always short-circuits _model_judge_verification to its fail-closed return before a judge is ever selected -- masking a real, pre-existing (present unchanged at merge-base 212ff43, predates #983) selection bug that only a hosted run with fast-mlsirm actually importable can exercise. Root cause: _ranked_agents deliberately still returns role-ineligible members (appended after every eligible one, per its own docstring), so a caller wanting only role-eligible candidates must re-apply `role not in agent.provider_exclusions` itself, exactly as _plan_generated/_parse_workflow_plan already do. _model_judge_verification's judge-selection next(...) was missing that filter, so with a single-candidate pool excluded from "verifier" (#983's own new orchestrator/free worker-only provable-route fixture), it picked that ineligible agent as judge anyway instead of failing closed --an extra, unrequested live call. _invoke's own failover path already enforces this same exclusion for a *backup* judge (test_fast_mlsirm_judge_failover_honors_verifier_exclusions); this closes the identical gap for the *primary* selection. Fix: add `if "verifier" not in agent.provider_exclusions` to the judge-selection generator in _model_judge_verification. Verification: - RED-before/GREEN-after: new regression test_model_judge_never_selects_a_verifier_excluded_sole_candidate (tests/test_model_judge.py) fails on the pre-fix code (records the excluded worker_only agent as judge) and passes after (next(...) raises StopIteration, caught by the existing fail-closed handler, judge never constructed). - test_model_judge.py + test_candidate_routing_controls.py + test_candidate_routing_no_heuristic_limits.py + test_api_contract.py + test_admin_contract.py: 98 passed. - Full local suite (Python 3.12, matching CI's `uv run` toolchain): 3441 passed, 2 pre-existing sandbox-only failures unrelated to this change and already documented in docs/product-technical-gap-baseline.md's 2026-09-02 entry (fast_mlsirm unavailable; test_spend_analytics's local-tokenizer artifact -- same missing-fast-mlsirm mechanism). - interrogate on orchestrator.py: 100%. - git diff --check: clean. CHANGELOG.md and docs/product-technical-gap-baseline.md updated with a dated entry per this PR's established practice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
This was referenced Sep 3, 2026
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
1 participant
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.
Summary
The item-25 audit-refresh fix (#1010, merged
212ff437) shipped with a regression test that was silently non-functional from the moment it merged — it never actually ran the behavior it claims to verify. Found while running the full test suite for an unrelated change (feat/web-search-mcp-a2a-foundation) and seeingtest_model_group_mutations_refresh_audit_eventsfail on a plainNameError.Four independent, stacked bugs surfaced in order, each hidden behind the previous one:
json,shutil,subprocessare used (a Node subprocess harness that evals extracted JS source) but never imported — an immediateNameErrorbefore any of the test's own logic ran.source_between(start, end)helper extracts oneadmin.pyJS function's source by searching for the next occurrence ofendafterstart. Three of five call sites named a marker several functions further down the file instead of the function's true immediate neighbor — e.g.saveModelGroup's end marker wasdeleteModelGroup, ~550 lines and several unrelated functions later, silently swallowing all of it into one bogus "function body." Retargeted each to its actual next function inadmin.py's source order.eval()of a bare function declaration returnsundefined. Per the ECMAScript spec, evaluating aFunctionDeclarationstatement's completion value isundefined, not the function — confirmed empirically (see commit message). Everyconst x = eval(...)in this test was silently bindingundefined, and nothing failed until something tried to call one of them.source_betweennow wraps its extracted source in parentheses to force function-expression parsing, soevalactually returns the callable.refreshModelGroupViews's real source callsshowModelGroupRefreshWarning(...), which the harness only ever used as an end marker, never extracted or defined — aReferenceErroronce the functions above started genuinely executing. Added its extraction, matching its four siblings.With all four fixed, the test now genuinely exercises
admin.py's real save/delete/refresh-audit JS flow end-to-end via a Node subprocess, and passes for the right reason.Test plan
uv run --locked --extra api --extra db --extra queue --group dev python -m pytest tests/test_admin_contract.py -q— 3 passed (previously: 1 failed on NameError before any of these bugs could even be reached).ruff checkon the touched file: fewer findings after this diff than before (10 → 3, all pre-existing style nitpicks unrelated to this fix, confirmed viagit stash).🤖 Generated with Claude Code