Skip to content

fix: correct bugs found during deep codebase review #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jlevy wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: correct bugs found during deep codebase review #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58f57f8
fix: correct bugs found during deep codebase review
claude Feb 21, 2026
516b3dc
Merge branch 'main' into claude/explore-codebase-flows-qkB0X
claude Feb 23, 2026
284982f
Merge branch 'main' into claude/explore-codebase-flows-qkB0X
claude Feb 24, 2026
ee778f4
fix: pass executionId to LiveAgent in serial fill path
claude Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions packages/markform/src/engine/inspect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ function mapValidationToInspectReason(vi: ValidationIssue): IssueReason {
if (
vi.code === 'MULTI_SELECT_TOO_FEW' ||
vi.code === 'STRING_LIST_MIN_ITEMS' ||
vi.message.includes('at least')
msg.includes('at least')
) {
return 'min_items_not_met';
}
Expand Down Expand Up @@ -419,7 +419,6 @@ export function isCheckboxComplete(form: ParsedForm, fieldId: Id): boolean {
return true; // Non-checkbox fields are not blocking
}

const checkboxField = field;
const response = form.responsesByFieldId[fieldId];

// If no response or not answered, checkbox is not complete
Expand All @@ -433,13 +432,13 @@ export function isCheckboxComplete(form: ParsedForm, fieldId: Id): boolean {
}

const values = value.values;
const optionIds = checkboxField.options.map((o) => o.id);
const mode = checkboxField.checkboxMode;
const optionIds = field.options.map((o) => o.id);
const mode = field.checkboxMode;

if (mode === 'multi') {
// Multi mode: all options must be done or na (not todo, incomplete, or active)
// If minDone is set, at least that many must be done
const minDone = checkboxField.minDone;
const minDone = field.minDone;
if (minDone !== undefined) {
const doneCount = optionIds.filter((id) => values[id] === 'done').length;
return doneCount >= minDone;
Expand Down Expand Up @@ -488,8 +487,7 @@ export function findBlockingCheckpoint(form: ParsedForm): BlockingCheckpointResu
continue;
}

const checkboxField = field;
if (checkboxField.approvalMode === 'blocking' && !isCheckboxComplete(form, fieldId)) {
if (field.approvalMode === 'blocking' && !isCheckboxComplete(form, fieldId)) {
return { index: i, fieldId };
}
}
Expand Down
5 changes: 2 additions & 3 deletions packages/markform/src/engine/summaries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,10 @@ function isFieldSubmitted(field: Field, value: FieldValue | undefined): boolean
}
case 'checkboxes': {
const v = value as CheckboxesValue;
const checkboxField = field;
// For checkboxes, check if any option has been changed from default
const mode = checkboxField.checkboxMode ?? 'multi';
const mode = field.checkboxMode ?? 'multi';

for (const opt of checkboxField.options) {
for (const opt of field.options) {
const state = v.values[opt.id];
if (mode === 'explicit') {
// In explicit mode, "unfilled" is default, anything else is submitted
Expand Down
16 changes: 0 additions & 16 deletions packages/markform/src/harness/fillRecordCollector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,22 +571,6 @@ export class FillRecordCollector implements FillCallbacks {
);
}

private findTurnKeyForExecutionId(
executionId: string,
turnStartEvents: Map<string, TurnStartEvent>,
): string | undefined {
// Find the most recent turn for this executionId
let latestKey: string | undefined;
let latestTime = '';
for (const [key, event] of turnStartEvents) {
if (event.executionId === executionId && event.timestamp > latestTime) {
latestKey = key;
latestTime = event.timestamp;
}
}
return latestKey;
}

private normalizeInput(input: unknown): Record<string, unknown> {
if (input && typeof input === 'object' && !Array.isArray(input)) {
return input as Record<string, unknown>;
Expand Down
18 changes: 11 additions & 7 deletions packages/markform/src/harness/programmaticFill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,7 @@ export async function fillForm(options: FillOptions): Promise<FillResult> {
additionalTools: options.additionalTools,
callbacks: mergedCallbacks,
maxStepsPerTurn: options.maxStepsPerTurn,
executionId: serialExecutionId(0),
toolChoice: options.toolChoice,
signal: options.signal,
maxRetries: options.maxRetries,
Expand Down Expand Up @@ -639,10 +640,8 @@ export async function fillForm(options: FillOptions): Promise<FillResult> {
mergedCallbacks.onTurnStart({
turnNumber: turnCount + 1,
issuesCount: stepResult.issues.length,
// Default to order 0, serial execution for non-parallel fills
// Parallel harness will override these values
order: 0,
executionId: '0-serial',
executionId: serialExecutionId(0),

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Use one execution ID format for serial callbacks

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 👍 / 👎.

Comment thread
cursor[bot] marked this conversation as resolved.
});
} catch {
// Ignore callback errors
Expand Down Expand Up @@ -802,7 +801,7 @@ export async function fillForm(options: FillOptions): Promise<FillResult> {
patches,
rejectedPatches: stepResult.rejectedPatches ?? [],
coercionWarnings: stepResult.coercionWarnings,
executionId: '0-serial',
executionId: serialExecutionId(0),
});
} catch {
// Ignore callback errors
Expand Down Expand Up @@ -1166,7 +1165,7 @@ async function runMultiTurnForItems(
items: ExecutionPlanItem[],
targetRoles: string[],
maxPatchesPerTurn: number,
_maxIssuesPerTurn: number,
maxIssuesPerTurn: number,
maxTurnsTotal: number,
startTurn: number,
options: FillOptions,
Expand Down Expand Up @@ -1212,6 +1211,9 @@ async function runMultiTurnForItems(
return true;
});

// Cap issues shown to agent per turn (consistent with serial path)
scopedIssues = scopedIssues.slice(0, maxIssuesPerTurn);

if (scopedIssues.length === 0) break; // All items filled

// Fire turn callback
Expand Down Expand Up @@ -1291,9 +1293,11 @@ async function runMultiTurnForItems(

// Apply patches
let lastCoercionWarnings: PatchWarning[] | undefined;
let turnPatchesApplied = 0;
if (response.patches.length > 0) {
const applyResult = applyPatches(form, response.patches);
patchesApplied += applyResult.appliedPatches.length;
turnPatchesApplied = applyResult.appliedPatches.length;
patchesApplied += turnPatchesApplied;
previousRejections = applyResult.rejectedPatches;
lastCoercionWarnings = applyResult.warnings;
} else {
Expand All @@ -1309,7 +1313,7 @@ async function runMultiTurnForItems(
mergedCallbacks?.onTurnComplete?.({
turnNumber: startTurn + turnsUsed,
issuesShown: scopedIssues.length,
patchesApplied: response.patches.length,
patchesApplied: turnPatchesApplied,
requiredIssuesRemaining: requiredIssues.length,
isComplete: postInspect.isComplete,
stats: response.stats,
Expand Down