Skip to content

fix(admin): repair test_model_group_mutations_refresh_audit_events - #1029

Closed
seonghobae wants to merge 2 commits into
mainfrom
fix/admin-contract-missing-json-import
Closed

fix(admin): repair test_model_group_mutations_refresh_audit_events#1029
seonghobae wants to merge 2 commits into
mainfrom
fix/admin-contract-missing-json-import

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Root cause

tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events fails on current main (confirmed at 212ff437, unmodified) with:

NameError: name 'json' is not defined

The test was added by commit 212ff437 ("fix(admin): refresh audit after model-group mutations"). That commit's own message notes hosted checks were queue-saturated at merge time ("CodeQL startup_failure has no analysis result and the executable contract test is included") — consistent with what this PR found: the new test never actually ran to green before merge. It carries four stacked defects, all confined to this one test function:

  1. Missing imports. The test uses json.dumps, shutil.which("node"), and subprocess.run without importing json, shutil, or subprocess at all — hence the reported NameError (the first of the three to be hit).
  2. Wrong source_between() end markers. The end markers used to slice refreshModelGroups, refreshAuditEvents, and saveModelGroup out of ADMIN_HTML pointed at the next-next function's name instead of the function immediately following in admin.py. E.g. saveModelGroup's extraction was bounded by ' async function deleteModelGroup', but deleteModelGroup is defined ~550 lines later — so the slice swallowed all the intervening admin-console source, which threw ReferenceError: Cannot access 'els' before initialization when eval'd.
  3. eval() of a bare function declaration has no completion value. Once (2) was fixed, const saveModelGroup = eval("async function saveModelGroup(...) {...}") evaluated to undefined (a FunctionDeclaration statement doesn't produce a value), so calling it threw TypeError: saveModelGroup is not a function. source_between() now wraps its return value in parens so eval sees a function expression.
  4. showModelGroupRefreshWarning was never extracted/bound. refreshModelGroupViews calls it on any refresh failure, but the harness never defined it, so the audit/groups-refresh-failure scenarios threw ReferenceError: showModelGroupRefreshWarning is not defined. It's now extracted and bound alongside the other functions.

contextual_orchestrator/admin.py itself is untouched — this is a test-only fix.

Fix

All changes are in tests/test_admin_contract.py:

  • Added import json, import shutil, import subprocess (ordered per this repo's convention: plain stdlib imports alphabetically before the pathlib/sys path-bootstrap pair — matches the pattern in tests/test_assistant_tool_calls_null_noop_http_honesty.py and siblings).
  • Corrected the source_between() marker pairs for refreshModelGroups, refreshAuditEvents, and saveModelGroup to bound on the function that actually follows each in admin.py.
  • Added a showModelGroupRefreshWarning extraction/binding.
  • source_between() now parenthesizes its returned source so each eval() yields a real function value.

Test verification

$ python -m pytest tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events -q
1 passed

$ python -m pytest tests/test_admin_contract.py -q
3 passed

$ python tests/test_admin_contract.py   # the file's own __main__ entrypoint
ok

No other test file imports/exercises this specific extraction harness (several other files import contextual_orchestrator.admin for unrelated ADMIN_HTML string assertions, unaffected by this change). A broader pytest tests -q run (excluding two files that fail to collect on main for unrelated, pre-existing reasons — tests/fuzz/test_fuzz_properties.py needs hypothesis, tests/test_psychometric_routing.py needs numpy, neither installed via requirements.lock) was kicked off for extra confidence; see follow-up comment if anything else in that run needs attention.

Commits

Introducing commit: 212ff437 (fix(admin): refresh audit after model-group mutations).


🤖 Generated with Claude Code

https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4


Generated by Claude Code

Commit 212ff43 ("fix(admin): refresh audit after model-group
mutations") added this contract test but never actually got it to
pass before merge (its own message notes hosted checks were
queue-saturated at merge time). It carries four stacked defects,
all confined to this one test function:

1. Missing `import json`, `import shutil`, `import subprocess` — the
   test uses `json.dumps`, `shutil.which("node")`, and
   `subprocess.run` without importing any of the three, so every
   run failed immediately with `NameError: name 'json' is not
   defined`.
2. `source_between()` end markers for `refreshModelGroups`,
   `refreshAuditEvents`, and `saveModelGroup` pointed at the wrong
   next function name, so those extractions swallowed unrelated
   admin.py source between the real end of the target function and
   the (much later) marker text — e.g. saveModelGroup's extraction
   ran 550+ lines past its own closing brace into deleteModelGroup,
   producing `ReferenceError: Cannot access 'els' before
   initialization` when eval'd.
3. `eval()` of a bare `async function foo() {...}` declaration
   string has no completion value in this Node module context (it
   evaluates to `undefined`), so every `const x = eval(source)`
   silently produced `undefined` instead of a callable — only
   surfaced once (2) was fixed. `source_between()` now wraps its
   return value in parens so eval sees a function *expression*.
4. `showModelGroupRefreshWarning`, called from inside
   `refreshModelGroupViews`, was never extracted/defined in the
   harness, so any refresh-failure scenario threw
   `ReferenceError: showModelGroupRefreshWarning is not defined`.
   It is now extracted and bound alongside the other functions.

Verified: `test_model_group_mutations_refresh_audit_events` and the
rest of `tests/test_admin_contract.py` now pass under
`python -m pytest`, and the file's `python tests/test_admin_contract.py`
direct-run entrypoint also passes.

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

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

Copy link
Copy Markdown
Contributor Author

Broader-suite confidence check, as promised in the PR description:

python -m pytest tests -q --ignore=tests/fuzz/test_fuzz_properties.py --ignore=tests/test_psychometric_routing.py

(the two ignored files fail to collect on unmodified main for unrelated reasons — hypothesis/numpy aren't in requirements.lock)

Result on this branch: 2 failed, 3324 passed, 2 skipped.

Both failures are pre-existing and unrelated to this change (this branch only touches tests/test_admin_contract.py):

  • tests/test_spend_analytics.py::test_exact_output_without_prompt_usage_is_explicitly_unavailable — reproduces identically on unmodified main (assert 'tokenizer' == 'mixed').
  • tests/test_provider_embedding_batch_backend.py::test_unknown_tokenizer_uses_authoritative_provider_usage — flaky/order-dependent; failed once in the full-suite run but passes in isolation on both main and this branch.

Neither touches contextual_orchestrator/admin.py or the admin contract test harness. Flagging for awareness, not fixing here to keep this PR narrow — happy to file a follow-up if useful.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Exact-head fleet verification for 054918462d8cd69bcda018c4353933011b20ca1e against protected main@212ff437dc297613289dba2e6064ade9942e07d8:

  • Diff remains test-only: tests/test_admin_contract.py, +10/-4; production contextual_orchestrator/admin.py is untouched.
  • No submitted reviews and no inline review threads are currently present.
  • Exact Tests run 33656559073 has two required jobs, both still pre-checkout (runner_id=0, steps=[]).
  • Exact CodeQL PR 33656560886 terminated startup_failure with zero jobs.
  • The same control-plane failure class has been handed to canonical .github#712 with RED/GREEN acceptance; no leaf no-op retrigger or gate weakening is justified.

Keep this PR unmerged until the unchanged exact head receives terminal required evidence and independent current-head review under the live protected-branch rules. Predecessor/local test evidence is diagnostic only and does not transfer as merge evidence.

…ssing-json-import

# Conflicts:
#	tests/test_admin_contract.py

Copy link
Copy Markdown
Contributor Author

CodeQL failure: infrastructure, not a defect

Investigated CodeQL analysis job 100336445153 (run 33656559190) directly via its logs. The analysis itself ran to completion (45 Python security queries loaded and evaluated, SARIF generated and uploaded) — it did not fail on a finding in this PR's 10-line, single-file diff. The failure is at SARIF processing:

##[error]Code Scanning could not process the submitted SARIF file:
CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled
CodeQL job status was configuration error.

That's a repository-level GitHub code-scanning configuration conflict (advanced/workflow-based CodeQL vs. GitHub's "default setup" both enabled), not a code security defect — a different concrete symptom than the startup_failure/total_count: 0 pattern this PR's own body cites from .github#712, but the same class of org CodeQL-infrastructure issue, not something introduced by this diff. No push was made for it.

Merge conflict: real, and the PR is now superseded — resolved anyway

Fresh git fetch origin main confirmed mergeable_state: dirty was genuine (not stale): main had moved to f4e5fc67 (#1027) and, since this PR was opened, PR #1035 landed the same fix directly on main at 39a4348e (2026-09-03T02:22:27Z) — independently of #1032/#1033.

Compared #1035's landed diff against this PR's diff line-by-line: both cover the identical 4 stacked defects (missing json/shutil/subprocess imports, corrected source_between() end markers for refreshModelGroups/refreshAuditEvents/saveModelGroup, the showModelGroupRefreshWarning extraction, and the eval()-parenthesization fix). #1035's version additionally documents source_between() with a proper docstring where this PR had only an inline comment — a strict superset, not just an equivalent.

Resolved with a normal merge commit (git merge --no-edit origin/main, no rebase/force-push): kept main's import ordering and its docstring, dropping this PR's now-redundant duplicate import lines and comment. Result: git diff origin/main -- tests/test_admin_contract.py is empty — this PR's version of the file is now byte-identical to what's already on main. Pushed as 229aaa17 (fast-forward, 054918462d..229aaa17).

Given the fix is already on main, this PR adds nothing beyond it at this point — noting the redundancy rather than trying to force additional distinguishing changes into it.

Verification

$ python -m pytest tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events -q
1 passed in 0.88s

$ python -m pytest tests/test_admin_contract.py -q
3 passed in 0.44s

$ python tests/test_admin_contract.py
ok

$ python -m interrogate -c pyproject.toml .
RESULT: PASSED (minimum: 100.0%, actual: 100.0%)

tests/ is excluded from the interrogate gate (pyproject.toml [tool.interrogate] exclude = ["tests"]), confirmed rather than assumed — this file was never in scope for that gate either way.

No sign of other recent activity on this branch (last prior commit 2026-09-02T16:41:43Z; current time 2026-09-03T05:2x UTC).


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Fresh closure proof: protected main is now c594b6828ad99018157613fdc31b68922e8d01d2; PR head is 229aaa17113676a971542ed215ff9dbfcac77105. GitHub reports changed_files=0, additions=0, deletions=0, and compare(main, head) is diverged only by history (ahead_by=2, behind_by=1) with files=[]. Therefore no effective semantic/test/fixture delta remains to integrate. This is the permitted no-valid-delta close case, not a queue-reduction close and not a claim that predecessor checks transfer.

@seonghobae seonghobae closed this Sep 3, 2026
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