fix: correct bugs found during deep codebase review#166
Conversation
- inspect.ts: Fix case-sensitive string match on lowercased variable (`vi.message.includes` → `msg.includes`) in mapValidationToInspectReason, which could miss capitalized "At least" validation messages - programmaticFill.ts: Fix patchesApplied in parallel path onTurnComplete callback reporting attempted count instead of actual applied count - programmaticFill.ts: Use structured executionId format (`eid:serial:o0`) in serial fillForm path instead of ad-hoc `'0-serial'` string - programmaticFill.ts: Apply maxIssuesPerTurn cap in parallel runMultiTurnForItems (was accepted but ignored via `_` prefix) - fillRecordCollector.ts: Remove dead findTurnKeyForExecutionId method - summaries.ts, inspect.ts: Remove pointless `checkboxField = field` aliases (TypeScript already narrows via case guards) Co-Authored-By: Claude <noreply@anthropic.com> https://claude.ai/code/session_01DRb2BYXTsYqsEfZUoU4v52
Co-Authored-By: Claude <noreply@anthropic.com>
![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: 516b3dc7b1
ℹ️ 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".
| // Parallel harness will override these values | ||
| order: 0, | ||
| executionId: '0-serial', | ||
| executionId: serialExecutionId(0), |
There was a problem hiding this comment.
This serial-path callback now emits executionId: serialExecutionId(0) (eid:serial:o0), but the LiveAgent is still created without an executionId, so its onLlmCall*/onTool* events keep the default 0-serial. FillRecordCollector.buildTimeline() correlates turn, tool, and LLM events by matching execution ID, so this mismatch causes serial runs (with a live agent) to lose per-turn tool/tokens attribution and produces corrupted timeline metrics for fill records. Please pass the same serial execution ID into createLiveAgent (or keep the turn callbacks on the legacy ID) so event correlation remains intact.
Useful? React with 👍 / 👎.
Coverage Report for packages/markform
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 22
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Co-Authored-By: Claude <noreply@anthropic.com>
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Secrets |  |
Feb 25, 2026 12:56a.m. | Review ↗ |
The serial fillForm path used serialExecutionId(0) in onTurnStart and onTurnComplete callbacks but didn't pass it to createLiveAgent, which defaulted to '0-serial'. This mismatch caused FillRecordCollector's buildTimeline() to fail to correlate LLM/tool events with their turns, resulting in zero tokens and missing tool call records in serial runs. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
Summary
inspect.tswherevi.messagewas used instead of the already-assignedmsgvariablecheckboxFieldvariable aliases ininspect.tsandsummaries.ts— the narrowedfieldtype already has the correct propertiesfindTurnKeyForExecutionIdmethod fromfillRecordCollector.tsprogrammaticFill.tsbugs: useserialExecutionId()helper instead of hardcoded strings, wire up previously-ignoredmaxIssuesPerTurnparameter in parallel path, and report actual applied patch count (not attempted) inonTurnCompleteserialExecutionId(0)tocreateLiveAgentso LLM/tool events correlate with turn events inFillRecordCollector.buildTimeline()Co-Authored-By: Claude noreply@anthropic.com
Note
Medium Risk
Touches core fill-loop telemetry and parallel execution behavior; bugs are fixes but could change callback/event correlation and turn-by-turn agent input in production.
Overview
Fixes several correctness issues across inspection and the programmatic fill harness.
In
inspect.ts/summaries.ts, tightens validation-reason mapping (uses the normalizedmsgconsistently) and removes redundantcheckboxFieldaliases while preserving checkbox completion/submission logic.In
programmaticFill.ts, makes execution IDs consistent by usingserialExecutionId()everywhere (includingcreateLiveAgent), wiresmaxIssuesPerTurninto the parallel multi-turn path, and reports actual applied patches (not attempted) inonTurnComplete; also removes deadfindTurnKeyForExecutionIdcode fromfillRecordCollector.ts.Written by Cursor Bugbot for commit ee778f4. This will update automatically on new commits. Configure here.