Repository navigation
feat: complete Tiny Stories research runtime - #25
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: 010eb40721
ℹ️ 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".
| export function isRpgEvaluationBundle(value: unknown): value is RpgEvaluationBundle { | ||
| if (!value || typeof value !== "object") return false | ||
| const candidate = value as Partial<RpgEvaluationBundle> | ||
| return candidate.schema_version === "rpg_evaluation_bundle.v1" | ||
| && typeof candidate.run_id === "string" | ||
| && typeof candidate.system_label === "string" | ||
| && Boolean(candidate.scenario && typeof candidate.scenario === "object") | ||
| && Array.isArray(candidate.turns) | ||
| && candidate.turns.length > 0 |
There was a problem hiding this comment.
An imported object with the expected root fields and a nonempty turns array passes this guard even when its turns or memory snapshots are malformed. For example, a bundle containing "turns": [{}] is accepted, then the next render throws while accessing turn.memory.superseded_facts, outside the import handler's try block; because there is no error boundary, the public lab becomes blank instead of showing the invalid-file message. Perform structural validation of the complete portable contract before calling chooseBundle.
Useful? React with 👍 / 👎.
| ) -> RpgMemorySnapshotV1: | ||
| """Reduce portable events into a bounded, auditable memory snapshot.""" | ||
|
|
||
| ordered = sorted(events, key=lambda event: (event.turn_index, event.event_id)) |
There was a problem hiding this comment.
When two events for the same fact occur in one turn, ordering them by the arbitrary event_id can reverse their semantics. For example, an assertion with ID z followed by a correction with ID a is processed correction-first, leaving the obsolete asserted value active and marking the correction superseded. Preserve input order within each turn_index, or add and sort by an explicit sequence field, so same-turn corrections are reduced chronologically.
Useful? React with 👍 / 👎.
| normalized_options = tuple(" ".join(option.casefold().split()) for option in turn.options) | ||
| if len(set(normalized_options)) < 2: | ||
| agency_gaps.append(f"Turn {turn.turn_index}: fewer than two distinct options") | ||
| option_signatures.add(normalized_options) |
There was a problem hiding this comment.
If consecutive turns contain the same choices in a different display order, their tuples differ and the evaluator awards full choice diversity even though the available action set did not change. This inflates experimental results for runtimes that reorder repeated choices; sort the normalized options when constructing the signature, and mirror that canonicalization in the frontend evaluator.
Useful? React with 👍 / 👎.
| visible_text = " ".join([turn.player_action, turn.world_response, *turn.options]) | ||
| if TECHNICAL_LEAK_RE.search(visible_text): | ||
| leakage.append(f"Turn {turn.turn_index}: technical wording appeared in player-facing text") |
There was a problem hiding this comment.
The run trace renders state-delta labels, clue unlocks, and opportunity unlocks as player-visible text, but the boundary check scans only the action, response, and options. A technical string such as raw JSON received in a clue or delta label is therefore displayed in the lab while boundary_hygiene still scores 100. Include all fields rendered by RunTrace in the leakage scan in both evaluator implementations.
Useful? React with 👍 / 👎.
| options: list[str] = Field(default_factory=list, min_length=1, max_length=6) | ||
| state_deltas: list[RpgStateDeltaV1] = Field(default_factory=list, max_length=8) | ||
| clue_unlocks: list[str] = Field(default_factory=list, max_length=4) | ||
| opportunity_unlocks: list[str] = Field(default_factory=list, max_length=4) |
There was a problem hiding this comment.
These list fields constrain only their lengths, not the strings they contain, so the CLI accepts values such as options=["Act", ""] and clue_unlocks=[""]. The evaluator then counts two distinct actions and a typed consequence, awarding full agency and consequence scores even though the UI presents only one usable choice and an empty change. Constrain and strip each list item before evaluation.
Useful? React with 👍 / 👎.
| for index, value in enumerate(context.cast_or_factions): | ||
| add("fact_asserted", key=f"cast_{index + 1}", value=value, namespace="cast") | ||
| for index, value in enumerate(context.constraints): | ||
| add("fact_asserted", key=f"constraint_{index + 1}", value=value, namespace="boundary") |
There was a problem hiding this comment.
StoryGuideCompressedContext.confirmed_facts is the collection of current truths carried by the Story Guide, but the projection adds scene fields, cast, constraints, and superseded facts while never adding this collection. A confirmed detail that is not duplicated in one of those specialized fields therefore disappears from the portable snapshot and cannot inform downstream evaluation. Emit these facts into an active namespace during projection.
Useful? React with 👍 / 👎.
| "objective": context.scene_summary, | ||
| "active_facts": active, | ||
| "superseded_facts": [*snapshot.superseded_facts, *superseded][-32:], |
There was a problem hiding this comment.
The rejected facts are initially reduced as active facts and incorporated into episodic_summary; this copy then moves them into superseded_facts but leaves that summary unchanged. With a minimal correction such as superseded player_role: Backup dancer, the returned snapshot can have no active old role while its summary still presents player_role: Backup dancer as current. Recompute the summary after reclassification or exclude the temporary superseded namespace when it is first built.
Useful? React with 👍 / 👎.
| if turn.objective_progress + 0.001 < previous_progress: | ||
| progress_regressions += 1 | ||
| elif turn.objective_progress > previous_progress + 0.05: | ||
| progress_advances += 1 | ||
| previous_progress = max(previous_progress, turn.objective_progress) |
There was a problem hiding this comment.
Updating previous_progress with max turns it into a high-water mark, yet the resulting value is scored and reported as the final observed progress. For a run whose progress goes from 90% to 10%, the report says the final progress is 90% and retains the large completion component despite recording a regression. Track the immediately preceding value for regression detection and final evidence separately from any peak-progress statistic, and mirror the fix in the frontend.
Useful? React with 👍 / 👎.
| schema_version: Literal["rpg_evaluation_bundle.v1"] = "rpg_evaluation_bundle.v1" | ||
| run_id: str = Field(min_length=1, max_length=120) | ||
| system_label: str = Field(min_length=1, max_length=120) | ||
| locale: Literal["en", "zh", "mixed"] = "en" | ||
| scenario: RpgEvaluationScenarioV1 | ||
| turns: list[RpgTurnObservationV1] = Field(default_factory=list, min_length=1, max_length=80) |
There was a problem hiding this comment.
The bundle contract does not verify that each turn's memory snapshot belongs to the bundle's run_id or matches that turn's turn_index. An adapter can accidentally attach a snapshot from another run or turn and still receive a valid report, while the UI displays that unrelated memory as evidence for the current trajectory. Add cross-model validation for both identity fields before accepting or evaluating a bundle.
Useful? React with 👍 / 👎.
| lookup = (event.namespace, key) | ||
| fact_id = _fact_id(event.namespace, key, value, event.turn_index) | ||
| previous = active.get(lookup) |
There was a problem hiding this comment.
The evaluator treats namespace/key identity case-insensitively, but the reducer uses the original casing for its lookup. If an adapter asserts Player_Role and later corrects player_role, the reducer retains both as active instead of superseding the first, after which the evaluator reports a duplicate conflict. Normalize both namespace and key for the lookup while retaining their desired display spelling.
Useful? React with 👍 / 👎.
lishehao
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee9a1f8fb3
ℹ️ 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".
| for key, value in ( | ||
| ("premise", story_brief.premise_summary), | ||
| ("player_role", story_brief.player_role or ""), | ||
| ("tone", story_brief.genre_tone), | ||
| ("story_kernel", story_brief.story_kernel), |
There was a problem hiding this comment.
When the generated Story Brief paraphrases a user-confirmed role, tone, or boundary, these events are added after the Story Guide events with the same namespace/key, so reduce_memory_events marks the user-confirmed value as superseded and keeps the generated paraphrase active. This also disagrees with _build_turn_story_contract, where the Story Guide context intentionally overrides the brief, causing the exported audit memory to report different current truth from the truth used during play; apply the brief first or otherwise give the Story Guide values precedence.
Useful? React with 👍 / 👎.
| const options = turn.options.map((option) => option.trim().toLowerCase()) | ||
| if (!turn.terminal && new Set(options).size < 2) agencyGaps.push(`Turn ${turn.turn_index}: fewer than two distinct options`) | ||
| if (!turn.terminal && options.length > 0) optionSignatures.add(options.join("|")) |
There was a problem hiding this comment.
For imported bundles, the browser only trims leading/trailing whitespace, while the backend collapses all whitespace with " ".join(option.casefold().split()). Thus choices such as "Ask Han" and "Ask Han" are considered distinct here but identical by the authoritative evaluator, changing both agency and diversity scores depending on whether the same bundle is imported or submitted to the API. Normalize internal whitespace identically in the browser evaluator.
Useful? React with 👍 / 👎.
| if context.planner_job: | ||
| add("world_consequence", value=f"Planner focus: {context.planner_job}", namespace="planner") |
There was a problem hiding this comment.
When a persisted Story Guide context has a planner_job and the entry survives the recency budget, this converts the internal routing directive into a world consequence prefixed with Planner focus:. The Memory view renders recent_events directly, while the boundary evaluator does not scan them, so an exported bundle can visibly expose orchestration internals and still receive a passing hygiene score. Omit planner metadata from the portable projection rather than presenting it as an in-world event.
Useful? React with 👍 / 👎.
| system_label: str = Field(min_length=1, max_length=120) | ||
| locale: Literal["en", "zh", "mixed"] = "en" | ||
| scenario: RpgEvaluationScenarioV1 | ||
| turns: list[RpgTurnObservationV1] = Field(default_factory=list, min_length=1, max_length=80) |
There was a problem hiding this comment.
A bundle with valid but out-of-order or duplicate turn_index values is accepted because this field only constrains the list length. Both evaluators then process the array as supplied, so an adapter that emits turns in the order [2, 1] receives a false progress regression and the lab renders the trajectory backwards despite the explicit indices. Validate strictly increasing unique indices at the bundle boundary or sort them consistently before scoring and rendering.
Useful? React with 👍 / 👎.
| client = LiveHarnessClient(base_url, timeout=timeout) | ||
| failures = [ | ||
| failure | ||
| for failure in payload.get("failures") or [] | ||
| if str(failure.get("stage") or "") != "/research/rpg-evaluations" | ||
| ] |
There was a problem hiding this comment.
When a completed run originally failed while fetching its evaluation bundle, during a schema/artifact check, or during telemetry collection, the stored failure's stage is not exactly /research/rpg-evaluations, so this filter retains it. The resume path can subsequently recompute all of that evidence successfully but still writes status: fail because the stale failure remains in failures, defeating the advertised post-run recovery for those failure modes. Remove failures for every stage that the resume path reruns, or rebuild that portion of the failure list from the fresh checks.
Useful? React with 👍 / 👎.
| for turn in turns: | ||
| progress_bases.add(turn.progress_basis) | ||
| active_keys: set[tuple[str, str]] = set() | ||
| superseded_ids = {fact.fact_id for fact in turn.memory.superseded_facts} |
There was a problem hiding this comment.
An imported fact placed only in superseded_facts can retain the model's default status="active", yet the evaluator only validates facts in active_facts. Such a snapshot receives full memory-continuity credit while the Memory view presents an explicitly active fact as superseded, making malformed correction histories look valid. Require every entry in superseded_facts to have superseded status, and mirror the check in the browser evaluator.
Useful? React with 👍 / 👎.
| .getNarrativeStory(sessionId, canRequestAgentTrace ? { agentTrace: true } : undefined) | ||
| .then(async (response) => { | ||
| if (cancelled) return | ||
| if (response.template.language) setLang(response.template.language) |
There was a problem hiding this comment.
When an English-UI user opens a Chinese story, this calls the global setLang, which also writes to localStorage, so the story's narration locale permanently changes the user's interface preference and leaves subsequent English-only surfaces such as Portfolio in a mixed-language state. Template language describes generated story prose, while Create already tracks it separately from the UI language; use a scoped display locale for Play and Replay instead of persisting it as the user's UI choice.
Useful? React with 👍 / 👎.
1 participant
Summary
Evidence
Claim boundary
The deterministic evaluator is an inspectable reliability rubric, not a calibrated academic quality metric. The static Vercel lab is not presented as Live LLM proof.