Add local mock user agent judge loop - #18
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13e38f73fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session_override=session_override, | ||
| template_override=template_override, | ||
| ) | ||
| adapter, config = _adapter_for_case(config, resolved_mode) |
There was a problem hiding this comment.
When a live case cannot construct its adapter (for example the backend is down, reviewer login returns an error, or the live case lacks both session_id and template_id), _adapter_for_case raises before entering the try that records RuntimeFailureArtifacts. That makes the whole gold-set evaluation abort without writing the intended validation-failed trace/summary/report for that case; include adapter construction in the same failure path as run_mock_user_episode.
Useful? React with 👍 / 👎.
| from tools.rpg_eval.narrative_mock_user import AgentLoopEvent, MockUserRuntimeError | ||
|
|
||
|
|
||
| class _StaticJudgeGateway: |
There was a problem hiding this comment.
Running python -m pytest -q now fails in test_only_author_fixtures_defines_fake_gateway_classes, because that guard scans every test file for ^class .*Gateway\b and only allows two author fixture files. This new _StaticJudgeGateway class name matches the forbidden pattern, so the full suite is blocked until this test double is renamed or moved into an allowed fixture path.
Useful? React with 👍 / 👎.
| statuses.append("fail") | ||
| if case.expected.leverage_usage_required and not result.episode_memory.played_leverage_cards: | ||
| statuses.append("warn") | ||
| return _combine_statuses(statuses) |
There was a problem hiding this comment.
For --llm-judge fake gold-set runs, a case with ending_required: true can still be reported as passing when the episode stops without an ending: deterministic_status never checks result.summary.ending_detected, and the fake judge/consistency path only derives failures from deterministic status, forbidden hits, or explicit LLM misses. That means a violated required stop-condition expectation is silently ignored in the CI-safe evaluation mode; add a deterministic failure/warning before combining statuses.
Useful? React with 👍 / 👎.
| turn_agent_events.append(self._repo.append_agent_event( | ||
| session_id, | ||
| ord_value=turn.narrator_message.ord, | ||
| event_type="agent_plan", | ||
| payload=turn.agent_plan, | ||
| )) |
There was a problem hiding this comment.
These trace writes run after the player and narrator messages have already been committed, but before turn_count is incremented. If any append_agent_event call fails (DB lock/disk error/malformed payload), the API returns an error with the new narrator beat already persisted and the session counter stale, so a retry advances from that hidden narrator instead of recovering the original turn; either write the turn and trace in one transaction or let trace persistence fail without aborting the user turn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c39251b61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| player_user_id=user.user_id, | ||
| include_agent_trace=agent_trace, |
There was a problem hiding this comment.
When agent_trace=true has just authorized a reviewer/admin, this still passes the reviewer's user.user_id into get_story_history, whose _load_session_for_player check requires the session owner to match exactly. An authorized reviewer using the new live eval path with --session against an existing anonymous or other-user session will therefore get session_forbidden instead of the gated trace; either bypass the owner check for authorized trace reads or load by the session's actual owner in that context.
Useful? React with 👍 / 👎.
| runtime_state_db_path: str = "artifacts/runtime_state.sqlite3" | ||
| default_actor_id: str = "local-dev" | ||
| agent_trace_reviewer_user_ids: str = "" | ||
| agent_trace_reviewer_usernames: str = "portfolio_reviewer" |
There was a problem hiding this comment.
With the default settings, username-only auth lets any visitor log in as portfolio_reviewer, and can_view_agent_trace grants access based only on that display name. For sessions that visitor starts, ?agent_trace=true can return AgentPlan payloads containing hidden NPC leverage/unused-leverage text, so the new reviewer/admin gate is not actually restricted unless operators remember to override this default.
Useful? React with 👍 / 👎.
| for turn in result.turns: | ||
| statuses.extend([turn.step_judge_status, turn.contract_judge_status]) | ||
| if len(result.turns) < case.expected.min_turns: | ||
| statuses.append("fail") | ||
| if result.trajectory_judge.status not in case.expected.trajectory_status_allowed: | ||
| statuses.append("fail") |
There was a problem hiding this comment.
When a gold case sets expected.required_stage_progression, --llm-judge fake can still report pass even if none of those stages appear, because deterministic_status only aggregates judge statuses/min turns/forbidden codes and never compares turn.agent_plan_summary.stage_phase against the required list. For example, a fixture run requiring climax passes with only hook/reversal stages and no expectation miss, so CI-safe evals can silently accept a missing progression requirement.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation already run
Boundary