Skip to content

fix(admin): refresh audit events after model-group save/delete - #1026

Merged
seonghobae merged 8 commits into
mainfrom
fix/admin-model-group-audit-refresh
Sep 2, 2026
Merged

fix(admin): refresh audit events after model-group save/delete#1026
seonghobae merged 8 commits into
mainfrom
fix/admin-model-group-audit-refresh

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary (backlog item 25)

The admin console's Audit tab reads state.recent_audit_events, which is only exposed on /admin/state (no scoped audit endpoint exists). saveModelGroup() and the model-group delete handler only called refreshModelGroups() (/api/v1/model_groups), so a save/delete that produced a new server-side audit event left the shared Audit tab stale until an unrelated full page reload.

Note on provenance: the backlog item cited PR #1010 as a precedent for this fix pattern ("model-timeouts panel"). That PR is actually closed, not merged (rejected via no-heuristics RCA — its per-model timeout bounds were an unreviewed heuristic), and there is no timeout-related code in admin.py at all. The actual bug and fix here were independently re-derived from the current codebase; the closest real precedent is startSession()'s existing await load()-after-success pattern, which this fix deliberately does not copy wholesale (too heavy — it also re-triggers simulate() and every readiness/commercial endpoint for what should be a lightweight group edit).

Changes

  • New refreshAuditEvents() helper: re-fetches /admin/state and applies only the recent_audit_events slice, then calls renderAudit(). Scoped, not a full load().
  • Wired into saveModelGroup() (after refreshModelGroups()) and the model-group delete click handler's .then() chain.
  • Model-group feedback text (#modelGroupFeedback) now gets --green/--red on success/error, matching the console's existing status-color convention (also flagged as a low-priority related issue in the backlog item).
  • CHANGELOG.d/admin-model-group-audit-refresh.md fragment, matching this repo's existing convention.

Test plan

  • pytest tests/test_admin_contract.py tests/test_admin_spend_view.py — 6 passed
  • New test_model_group_mutations_refresh_audit_events extracts each handler's source text and asserts both the call-site wiring and that the new helper stays scoped to /admin/state (not simulate()/refreshReadiness())
  • Full suite: uv run --locked --extra api --extra db --extra queue --group dev python -m pytest -q3313 passed, 2 skipped, 0 failures

🤖 Generated with Claude Code


Devin Review

The admin console's Audit tab reads from `state.recent_audit_events`,
which is only populated by /admin/state -- there is no scoped audit
endpoint. saveModelGroup() and the model-group delete handler only
called refreshModelGroups() (which re-fetches /api/v1/model_groups),
so a save/delete that produced a new server-side audit event left the
shared Audit tab stale until an unrelated full page reload.

Adds a lightweight refreshAuditEvents() helper -- re-fetches
/admin/state and applies only the recent_audit_events slice -- rather
than reusing the heavier load() (which also re-triggers simulate() and
every readiness/commercial endpoint), and wires it into both the save
and delete handlers. Also gives the shared model-group feedback text a
success/error color, matching the console's existing --green/--red
status-color convention used elsewhere.

New tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events
locks in the fix by extracting each handler's source and asserting the
call sites and the scoped (not full-load) shape of the new helper.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 12 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: 645655c0-06ef-43d2-b57a-936dfb0eadd8

📥 Commits

Reviewing files that changed from the base of the PR and between 464da47 and beb79d4.

📒 Files selected for processing (3)
  • CHANGELOG.d/admin-model-group-audit-refresh.md
  • contextual_orchestrator/admin.py
  • tests/test_admin_contract.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.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae seonghobae added bug Something isn't working priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: bug Defect or incorrect behavior labels Sep 2, 2026 — with ChatGPT Codex Connector

Copy link
Copy Markdown
Contributor Author

Routine staleness triage: this PR's base was behind current main (mergeable_state: behind, no real conflict). Fast-forwarded by merging origin/main into this branch (clean merge, no conflicts) and pushed directly (no force-push, no new PR).

Verification before push:

  • python -m pip install --require-hashes -r requirements.lock && python -m pip install --no-deps -e .
  • pytest tests/test_admin_contract.py tests/test_admin_spend_view.py tests/test_rater_observation.py tests/test_rater_observation_criterion_binding.py tests/test_rater_observation_trusted_binding.py — 43 passed (this PR's own suite plus the files touched by the incoming merge diff)

Note for the reviewer/maintainer: this PR and #1011 (fix/model-groups-audit-staleness) both add a refreshAuditEvents()-style fix to contextual_orchestrator/admin.py independently — they look like duplicate/overlapping work on the same bug. Not resolving that overlap here (out of scope for this staleness pass), just flagging it.

No source changes beyond the merge commit itself; fresh exact-head CI evidence is still required before merge per this repo's governance.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Restore the last current-main-compatible admin console tree after the refresh-isolation follow-up accidentally deleted the rendering, loading, navigation, session, evaluation, and listener functions required at startup. Preserve the original scoped audit-refresh fix while reverting only the broken follow-up implementation and its obsolete/syntactically invalid test additions. Keep the remaining refresh-failure-isolation finding open for a behavioral RED-first repair.

Copy link
Copy Markdown
Contributor Author

Repair-first reconciliation applied after fresh review found a RED startup regression on the prior head.

  • prior head inspected: f99b6d20225608ed51cade9530262fece6e8108f
  • protected base: main@464da4715b495b5eaaa593eba3796e2d976ee0c9
  • new exact head: e7e11e20d926deaf8c9e1cb659230e02c3ba339f
  • ref move was fast-forward/non-force; history was not rewritten.

RCA: the refresh-isolation follow-up had accidentally deleted ~500 lines of the embedded admin console, including render/load/navigation/session/evaluation functions and event registrations. The current review correctly identified that applyI18n would then call a deleted function during startup. Its test delta also asserted obsolete pre-refactor wiring and had introduced a literal \\n collection error before the later syntax-only correction.

The repair commit deliberately restores the last current-main-compatible tree (697ee1e2c373fadb4428a3bf06edf4c20684974c) as a descendant of the broken head. Fresh base→head diff is narrow again: only CHANGELOG.d/admin-model-group-audit-refresh.md (+1), contextual_orchestrator/admin.py (+21/-2), and tests/test_admin_contract.py (+44). The 555-line deletion is gone, while the original scoped audit-refresh behavior remains.

This does not claim the PR is merge-ready. The earlier valid review finding remains: once the model-group mutation is confirmed, /admin/state HTTP/network/JSON refresh failure must not make the successful mutation appear failed. That needs a fresh behavioral RED-first repair covering non-2xx, network rejection, and malformed JSON, and the test must execute the handler behavior rather than merely search source strings. I attempted to convert the PR back to Draft, but the connected mutation surface was blocked before GitHub state changed; treat this exact head as Draft-required until that behavioral finding and fresh checks/review are GREEN.

devin-ai-integration[bot]

This comment was marked as resolved.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

Devin Review

Comment on lines +1215 to +1229
async function refreshModelGroupViews() {
let modelGroupsRefreshed = true;
try {
await refreshModelGroups();
} catch (error) {
modelGroupsRefreshed = false;
console.warn("Could not refresh model groups after mutation", error);
}
const auditRefreshed = await refreshAuditEvents();
if (!modelGroupsRefreshed) {
showModelGroupRefreshWarning(t("model_groups_refresh_warning"));
}
if (!auditRefreshed) {
showModelGroupRefreshWarning(t("audit_refresh_warning"));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Refresh failures remain isolated

refreshModelGroupViews attempts both refreshes independently. Either failure preserves the completed mutation message and adds the corresponding reload warning.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +1802 to +1804
const response = await fetch(endpoint, {method: "DELETE"});
const payload = await response.json();
if (!response.ok) throw new Error(payload.error?.message || "Could not delete model group");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Delete parsing matches endpoint contract

deleteModelGroup parses JSON before checking status. The endpoint returns JSON for successful deletions and structured failures, so valid responses remain handled.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@seonghobae
seonghobae merged commit 212ff43 into main Sep 2, 2026
23 of 26 checks passed
@seonghobae
seonghobae deleted the fix/admin-model-group-audit-refresh branch September 2, 2026 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants