fix(tests): correct test_model_group_mutations_refresh_audit_events - #1033
fix(tests): correct test_model_group_mutations_refresh_audit_events#1033seonghobae wants to merge 2 commits into
Conversation
This test (contextual-orchestrator#1026) was broken on main from the
moment it was added -- three separate bugs, all masked because the
test never actually ran to completion before this fix:
1. json/subprocess/shutil are used but never imported (NameError on
the first line that touches json.dumps).
2. source_between()'s end markers for two of the six extractions
didn't point at each function's actual next sibling in admin.py:
- saveModelGroup's end marker was 'async function
deleteModelGroup', but ~568 unrelated lines (renderTrace,
renderAccess, and several other panel-rendering functions) sit
between them, so the extraction swept in a stray
`els.agentSearch.addEventListener(...)` reference and threw
"Cannot access 'els' before initialization" (TDZ) once the
eval'd script ran far enough to reach it.
- refreshModelGroups had the identical shape of bug one function
earlier: its end marker pulled in refreshAuditEvents's entire
body too.
Fixed both to their real immediate next sibling.
3. Once the ranges were correct, a third, more fundamental bug
surfaced: eval() of a bare function *declaration* (not wrapped in
parentheses) returns undefined, not the callable -- every one of
these `const X = eval(...)` assignments was silently undefined
regardless of extraction correctness. Verified with a minimal
`node -e` repro before writing the fix. Wrapped each extracted
function body in parens in source_between() so eval() evaluates it
as an expression.
Also added showModelGroupRefreshWarning as a sixth extracted const --
it's called internally by refreshModelGroupViews but was never itself
extracted, so calling it threw ReferenceError even after fixes 1-3.
Verified incrementally: each fix surfaced the next real error in
sequence rather than a new symptom, confirming this is the actual
converging root cause chain, not a series of unrelated patches. Full
suite: 3333 passed, 1 skipped (pre-existing, unrelated). interrogate
100% (tests/ is excluded from the docstring gate, unaffected either
way).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
contextual-orchestrator#1026 (item 25's fix) shipped a matching contract test in the same PR, but the test never actually ran to completion on main -- three separate bugs, uncaught because nobody had run it since merge. Handed off by a peer session after hitting the failure three times and deselecting around it each time. Cloned contextual-orchestrator (previously deferred across several ticks citing "no local clone") and fixed it directly: ContextualWisdomLab/contextual-orchestrator#1033. Full details in the gap-baseline correction. The underlying admin.py fix was never in question -- only its own test's ability to prove that fix works was broken. Item 25 remains resolved; recorded the narrower lesson that "PR merged with tests included" and "those tests actually ran and passed" are different verification steps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough관리자 계약 테스트가 추출한 JavaScript 함수 표현식을 Changes관리자 계약 테스트
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Merge Risk: ⚪ Minimal · up to This change repairs an existing administrative contract test without changing production behavior, APIs, permissions, dependencies, or deployment settings. It is merge-ready after normal checks and review, with no actionable merge-blocking risk remaining. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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}
|
Independently re-verified (can't formally approve — same account as the PR author): re-derived every marker pair's adjacency directly against |
Non-force descendant integration of PR #1033 into the orchestrator/free routing repair lane. The production fix in #1032 is unchanged; this adds the executable Node-backed admin contract repair so the branch no longer carries the known pre-existing broken test that masked model-group audit-refresh behavior. Parents preserve both exact histories: #1032 ef971be and #1033 3ca8c94.
…al-extraction-bugs # Conflicts: # tests/test_admin_contract.py
|
Resolved the merge conflict per Autofix's standing authorization: merged After the merge, Net effect: this PR's own delta is now fully superseded/inherited by #1035, which already merged. Flagging rather than closing myself — Autofix's authorization here covers fixing/pushing, not close decisions. If nothing else is intended to land on this branch, it looks safe to close as fully covered by #1035. 🤖 Addressed by Claude Code |
|
Verified succession before closure: live base |
|
자동 정리: base 대비 실제 변경(diff)이 0건이라 이 PR을 닫습니다. 변경을 추가한 뒤 reopen하세요. |
1 participant
Summary
contextual-orchestrator#1026(212ff43, item 25 in the standing.githubbacklog) addedtest_model_group_mutations_refresh_audit_eventstotests/test_admin_contract.py, but the test was broken from the moment it was merged — three separate bugs, all masked because it never actually ran to completion before this fix. Found while investigating a handoff from a peer session that had hit this repeatedly and worked around it withdeselect.json/subprocess/shutilare used but never imported (NameErroron the firstjson.dumpscall).admin.py:saveModelGroup's end marker was'async function deleteModelGroup', but ~568 unrelated lines (renderTrace,renderAccess, and several other panel-rendering functions) sit between them, so the extraction swept in a strayels.agentSearch.addEventListener(...)reference and threwCannot access 'els' before initialization(TDZ) once the eval'd script reached it.refreshModelGroupshad the same shape of bug one function earlier: its end marker pulled inrefreshAuditEvents's entire body too.eval()of a bare function declaration (not wrapped in parens) returnsundefined, not the callable — everyconst X = eval(...)assignment was silentlyundefinedregardless of extraction correctness. Verified with a minimalnode -erepro before writing the fix.Also added
showModelGroupRefreshWarningas a sixth extractedconst— it's called internally byrefreshModelGroupViewsbut was never itself extracted, so calling it threwReferenceErroreven after fixes 1-3.Each fix surfaced the next real error in sequence rather than a new symptom, confirming this is the actual converging root cause chain rather than a series of unrelated patches.
Test plan
pytest tests/test_admin_contract.py -v— all 3 tests passpython -m pytest tests -q --ignore=tests/fuzz— 3333 passed, 1 skipped (pre-existing, unrelated)interrogate— 100% (tests/ excluded from the docstring gate, unaffected either way)🤖 Generated with Claude Code
Summary by CodeRabbit