fix(llm): harden structured submits for 0.5.5 - #20
Conversation
🧞 Codegenie ReviewReviewed 149 of 160 hunks (11 skipped, notably Highest-value items: (1) after the switch from Lower-severity items are diagnostic-fidelity gaps: Since these are behavior changes with mixed intent evidence, several items ask for confirmation of the intended contract rather than asserting a regression. Open follow-ups worth author attention: the runtime shape of pi-ai's CoverageReviewed 149/160 hunks.
|
pkieltyka
changed the title
pkieltyka
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧞 Codegenie Review
Reviewed 134/152 hunks.
Incomplete work: skipped 18.
Coverage disclosure:
- pnpm-lock.yaml: lockfile
Summary-only findings:
-
⚪ Low: Divergence test conflates both mismatch predicates; final-message substitution clause is uncovered (
tests/final-tool-arguments.test.ts:87)
The single divergence case makes both equality predicates fail simultaneously, so neither is independently exercised:it("rejects event/final value divergence including non-suffix final replacement", async () => { const final = message(call("submit-1", SUBMIT, { complete: true })); const result = await consumeFinalToolArguments( sequence(final, ['{"complete":false}'], { endArguments: { complete: true } }), SUBMIT ); expect(result.content[0]).toMatchObject({ argumentParse: { state: "event_final_mismatch" } }); });
The guard has two independent clauses:
if (!isDeepStrictEqual(value, capture.endCall.arguments) || !isDeepStrictEqual(value, finalCall.arguments)) { return { state: "event_final_mismatch" }; } return { state, value };
and the
sequence()helper defaultsendCall.argumentsto the final message block's arguments in every other case:const endCall = call( options.endId ?? finalCall.id, options.endName ?? finalCall.name, options.endArguments ?? finalCall.arguments );
so no case ever has the parsed delta agree with
endCall.argumentswhile the final block differs.Impact: deleting or inverting either clause — in particular
!isDeepStrictEqual(value, finalCall.arguments), which detects a final message whose tool-call arguments diverge from the verified stream capture — leaves every test in the file green; such a stream would be classifiedstrictinstead ofevent_final_mismatch, dropping the mismatch signal consumed bysrc/telemetry/telemetry-recorder.tsand verifier classification. The regression is bounded to classification/telemetry fidelity becausefinalizeMessagereturnsparse.valuefrom the stream capture rather than the final block's arguments.Suggested fix: split into two cases so each predicate is exercised alone.
// 1) end-event divergence only sequence(message(call("submit-1", SUBMIT, { complete: false })), ['{"complete":false}'], { endArguments: { complete: true } }); // expect state "event_final_mismatch" // 2) final-message substitution only (delta === endArguments, final block differs) sequence(message(call("submit-1", SUBMIT, { complete: true })), ['{"complete":false}'], { endArguments: { complete: false } }); // expect argumentParse.state "event_final_mismatch" and no `arguments` property on the block
-
⚪ Low: Explicit classification precedence can bypass the empty-submit repair-discard guard (
src/pipeline/verifier.ts:981)
The new explicit-classification early return inclassifyVerifierSchemaInvalidruns before the structural empty-submit check:function classifyVerifierSchemaInvalid(input: LlmSchemaRepairInput | string): VerifierSchemaInvalidKind { if (typeof input !== "string") { const explicit = explicitVerifierFailureClassification(input.classification); if (explicit !== undefined) { return explicit; } } if (typeof input !== "string" && isEmptySubmitObject(input.submitCalls[0]?.arguments)) { return "empty_submit_object"; }
The helper returns a defined value for six runner classifications (
length_stopped,final_arguments_partial,final_arguments_invalid,event_capture_missing,event_final_mismatch,revise_without_revision_payload), and the post-repair discard keys strictly on the empty-submit kind:if (repairAttempt !== undefined) { runtimeStats.repairSucceeded += 1; if (repairAttempt.classification === "empty_submit_object") { telemetry.event({ ... message: "verification_empty_submit_repair_discarded" ... }); return incompleteSubmittedVerdict("schema_invalid_after_repair: empty_submit_object"); } }
Impact: when a repair input carries one of those explicit classifications and its captured submit arguments are an empty object, the recorded kind is the explicit one, so the discard does not fire and the model's repaired verdict is returned rather than downgraded to
incompleteSubmittedVerdict("schema_invalid_after_repair: empty_submit_object"). Theverification_empty_submit_repair_discardedtelemetry signal is also lost for that case. Scope is bounded: such verdicts still pass schema validation and the non-repairisEmptySubmitObject(result)check, so a fully content-free verdict is still rejected. Co-occurrence frequency inpi-runnerwas not measured.The precedence change is deliberate; please confirm the intended interaction with the empty-submit discard.
Suggested fix: evaluate
isEmptySubmitObject(input.submitCalls[0]?.arguments)before the explicit-classification return, or keep the explicit kind and record anemptySubmitPayloadflag onVerifierRepairAttemptthat the discard also consumes.Suggested test: call
classifyVerifierSchemaInvalidwith{ classification: "final_arguments_partial", submitCalls: [{ id: "1", arguments: {} }], extraToolNames: [] }and assert the verifier still downgrades the repaired verdict to an incomplete empty-submit outcome. -
⚪ Low: Degraded-planning test's third assertion is a tautology; clean-vs-incomplete publication split stays unpinned (
tests/pipeline-phase5.test.ts:6645)
The third assertion in the new degraded-planning rendering test targets the test's own fixture rather than the rendered output:expect(markdown).toContain("**Degraded run: planner fallback.**"); expect(markdown.indexOf("**Degraded run: planner fallback.**")).toBeLessThan(markdown.indexOf("Review completed.")); expect(coverage.partial).toBe(false);
coverage.partialwas set tofalseby the fixture a few lines above, so the assertion cannot fail and constrains nothing aboutmarkdown. The banner assertion does coverrenderCoverageTrustBanner's mutually exclusive branches, but the No Findings section is rendered independently:function renderNoFindings(result: ReviewResult): string { if (!result.noFindings) { return ""; } if (result.coverage.partial) { return ( "## ⚠️ Review Incomplete\n\n" + "Completed review work produced no credible verified findings, but incomplete coverage or verification prevents a clean conclusion." ); } return "## ✅ No Findings\n\nNo credible findings were found. Everything looks good."; }
Impact: for the fixture (
partial: false,degradedPlanning: true,noFindings: true) nothing pins that the run is published as a clean## ✅ No Findingsreview. IfrenderNoFindings(or a later section) emitted incomplete language for a degraded-but-complete run, all three assertions would still pass. Truthful publication of degraded-but-complete runs is a stated goal of this change, so the missing guard matters even though no production code is wrong today. The sibling partial test added in the same hunk already pins both halves:expect(markdown).toContain("**Partial review:** 1 hunk did not complete review."); expect(markdown).toContain("## ⚠️ Review Incomplete"); expect(markdown).not.toContain("Everything looks good");
Suggested fix: drop the fixture assertion and mirror the sibling test's style:
expect(markdown).toContain("**Degraded run: planner fallback.**"); expect(markdown.indexOf("**Degraded run: planner fallback.**")).toBeLessThan(markdown.indexOf("Review completed.")); expect(markdown).not.toContain("**Review incomplete.**"); expect(markdown).not.toContain("## ⚠️ Review Incomplete"); expect(markdown).not.toContain("**Partial review:**"); expect(markdown).toContain("## ✅ No Findings");
🙋 Needs human attention:
- Can any producer of LlmCallRecord.finalArgumentState / finalArgumentErrorKind write a value outside the declared unions (e.g. a provider-supplied string coerced with
as), which would make finalArgumentStates[key] undefined and turn the counter into NaN in the run summary? - Does the implemented production adapter actually fail closed when the public event representation does not deep-equal Pi's final submit value, as this architecture line now specifies?
- Is coverage.partial guaranteed to be set whenever coverage.reasons contains composer fallback/pre-trim reasons, so the new message ('incomplete coverage or verification') is truthful rather than asserted when coverage is actually complete?
- Should finalArgumentStates/finalArgumentErrorKinds be gated on providerCallCount > 0 like retryAttempts and toolChoiceDowngradedCalls, so cache-hit replays (records restored with a stored finalArgumentState) do not inflate the per-model state histogram relative to providerCalls?
- Does buildStructuredSubmitFailureDiagnostic's classification normalizer accept "missing_submit", or does it silently fall back to "schema_invalid" and misreport the failure cause?
- Additional unresolved notes suppressed: 4
— codegenie v0.5.4 (ae1bb70243) · View Workflow Job
| if (!message.content.some(isInvalidToolCall)) { | ||
| messages.push(message as unknown as ConversationMessage); |
There was a problem hiding this comment.
The new guard drops the whole assistant turn whenever the message contains any invalidToolCall block, but the valid repository tool calls in that same message are still executed and their toolResult messages are appended, so the next provider call in the pass carries tool results with no originating assistant toolCall.
const toolCalls = toolCallsExcept(message, submitTool.name);
if (!message.content.some(isInvalidToolCall)) {
messages.push(message as unknown as ConversationMessage);
}isInvalidToolCall is not scoped to the submit tool:
function isInvalidToolCall(block: unknown): block is PiInvalidToolCall {
return Boolean(block && typeof block === "object" && (block as { type?: unknown }).type === "invalidToolCall");
}and toolCallsExcept only returns real toolCall blocks, so a mixed message still yields a non-empty toolCalls list that is executed:
if (toolCalls.length > 0 && !forceFinalize) {
investigationRounds += 1;
const toolResults: ConversationMessage[] = [];
for (const toolCall of toolCalls) {
...
}
messages.push(...toolResults);Impact: for responses that mix one unparseable tool call with valid ones, the transcript sent on the very next request is internally inconsistent (orphaned toolResult entries). Depending on the provider this is either rejected outright or the assistant turn and its reasoning are lost, turning a recoverable single-tool parse hiccup into a failed or degraded pass. In the base code the assistant message was always pushed, so results always had their originating turn.
Plan 112 establishes a fail-closed trust boundary for final structured-submit arguments; this guard is broader than the submit block it protects, so the contract for non-submit tool calls changes. Please confirm whether dropping assistant turns that own executed repository tool calls is intended.
Suggested fix: scope the suppression to the submit block — sanitize the message (strip the invalid submit block and push the remainder), or skip the push only when the message has no valid repository tool calls. If the turn must be dropped, also skip executing its tool calls and pushing their toolResults.
Suggested test: stub a provider response containing one valid repository toolCall plus one invalidToolCall block, then assert the messages passed to the next provider call include an assistant turn whose toolCall id matches every pushed toolResult.toolCallId.
| return consumeFinalToolArguments(stream, submitToolName); | ||
| } | ||
| return models.complete( | ||
| const stream = models.stream( |
There was a problem hiding this comment.
All four adapter paths were switched from models.complete/completeSimple (which reject on provider failure) to models.stream/streamSimple plus consumeFinalToolArguments:
const stream = models.stream(
model.raw as Model<Api>,
context as Context,
mapProviderOptions(model.raw as Model<Api>, completeOptions)
);
return consumeFinalToolArguments(stream, submitToolName);In the consumer, an error event is stored as the terminal value instead of being thrown:
} else if (event.type === "done") {
terminal = event.message as PiAssistantMessage;
} else if (event.type === "error") {
terminal = event.error as PiAssistantMessage;
}
...
if (terminal === undefined) {
throw new Error("Pi stream ended without a terminal event");
}
return finalizeMessage(terminal, submitToolName, captures);finalizeMessage immediately dereferences message.content.map.
Impact: if the Pi stream reports provider/transport failures as error events rather than by rejecting the iterator, then either (a) an Error-shaped value produces an opaque TypeError: Cannot read properties of undefined (reading 'map'), discarding the provider error identity used by the runner's classification and MAX_PROVIDER_ATTEMPTS retry/backoff, or (b) a message-shaped value resolves as if the model replied, so repair/finalize runs against a non-response. Either way the existing retryable-provider-failure path no longer applies and diagnostics are misleading.
Plan 112 specifies deriving final arguments from Pi's public event stream with fail-closed handling but does not state that provider error events should become terminal messages — please confirm the intended contract. Whether @earendil-works/pi-ai emits error events in addition to rejecting the iterator could not be verified here (typings unreadable in this workspace).
Suggested fix: treat error events as failures in consumeFinalToolArguments:
if (event.type === "error") {
throw event.error instanceof Error ? event.error : new Error(String(event.error));
}At minimum, validate the terminal value is message-shaped before calling finalizeMessage.
Suggested test: feed consumeFinalToolArguments a stream yielding only { type: "error", error: new Error("upstream 503") } and assert the promise rejects with that error (message preserved), not with a TypeError and not resolving with a pseudo-message.
|
|
||
| function submitCallHasFindings(toolCall: PiToolCall): boolean { | ||
| function submitCallHasFindings(toolCall: PiSubmitCall): boolean { | ||
| if (!isTrustedSubmitCall(toolCall)) { |
There was a problem hiding this comment.
submitCallHasFindings now returns false for any submit call whose parse provenance is untrusted, regardless of whether it carries findings:
function submitCallHasFindings(toolCall: PiSubmitCall): boolean {
if (!isTrustedSubmitCall(toolCall)) {
return false;
}
const findings = toolCall.arguments.findings;
return Array.isArray(findings) && findings.length > 0;
}Trust is provenance-only:
function hasTrustedArgumentParse(call: PiToolCall): boolean {
return call.argumentParse?.state === "strict" || call.argumentParse?.state === "repaired";
}and this is the single place candidateDrafted is armed:
candidateDrafted = candidateDrafted || submitCalls.some(submitCallHasFindings);The Stage 7 anti-downgrade guard depends on it:
if (request.stage === 7 && schemaRepairUsed) {
if (candidateDrafted && !submitCallHasFindings(submitCall)) {
const error = "Stage 7 candidate schema repair returned no findings; codegenie will not silently downgrade malformed findings to no-findings.";
... throw new CodegenieError("llm_schema_invalid", error, { recoverable: true, ...Impact: a submit whose argumentParse.state is neither strict nor repaired but whose findings array is non-empty no longer arms candidateDrafted. The runner discards the payload and schedules a repair turn; if that repair returns empty findings, the guard does not fire and the run is published as a no-findings review instead of raising recoverable llm_schema_invalid. Forced-finalize prompts also advertise finalizeTarget no_findings instead of candidate_or_unknown. In the base code (toolCallsNamed returned PiToolCall[], no trust predicate) such a findings-bearing degraded submit did arm candidateDrafted, so this is a base-vs-head contract change on exactly the path the guard was added for.
Plan 112's fail-closed trust boundary explains gating acceptance on provenance, but not the loss of candidate tracking — please confirm the intended behavior for callers/spec.
Suggested fix: split the concerns. Keep isTrustedSubmitCall for acceptance decisions, and arm candidateDrafted from a best-effort predicate (e.g. submitCallLooksCandidateLike(call) that safely reads call.arguments?.findings even for untrusted or recovered parses), so the anti-downgrade guard and finalizeTarget stay conservative.
Suggested test: first turn returns a stage-7 submit whose argumentParse.state is not strict/repaired but with findings.length > 0; repair turn returns a trusted submit with findings: []. Assert the runner throws recoverable llm_schema_invalid and that the forced-finalize prompt/telemetry uses candidate_or_unknown.
| : "unsafe_candidate_like_payload"; | ||
| const stage7CompactRepair = input.request.stage === 7 && | ||
| input.replaceConversationOverride === true && | ||
| isStage7SchemaInvalidKind(input.repairClassification); |
There was a problem hiding this comment.
The stage-7 compact repair prompt is now gated on the classification being a Stage7SchemaInvalidKind, but the conversation-replacement decision was left unchanged:
const stage7CompactRepair = input.request.stage === 7 &&
input.replaceConversationOverride === true &&
isStage7SchemaInvalidKind(input.repairClassification);
const content = stage7CompactRepair
? stage7CompactSchemaRepairPrompt(input.submitToolName, error, stage7Classification, repairInput)
: input.request.schemaRepair?.buildPrompt?.(repairInput) ??
defaultSchemaRepairPrompt(input.request, input.submitToolName, error);
const replaceConversation = input.replaceConversationOverride ?? (input.request.schemaRepair?.replaceConversation === true);if (replaceConversation) {
input.messages.splice(0, input.messages.length, repairMessage);
} else {
input.messages.push(repairMessage);
}The helper returns false for undefined and for every classification outside its closed set:
export function isStage7SchemaInvalidKind(value: LlmSubmitFailureClassification | undefined): value is Stage7SchemaInvalidKind {
return value !== undefined && STAGE7_SCHEMA_INVALID_KINDS.has(value);
}and a reachable caller forwards a semantic classification together with replaceConversationOverride: true:
const repairClassification = recovery.repairClassification ?? semanticClassification;
scheduleModelRepair({
...
...(repairClassification !== undefined ? { repairClassification } : {}),
...(recovery.replaceConversationOverride === true ? { replaceConversationOverride: true } : {}),
cause
});Impact: for a stage-7 repair with the override set and a non-schema classification (e.g. a semantic kind from SubmitSemanticValidationError, or none at all), the generic buildPrompt/defaultSchemaRepairPrompt content is produced while input.messages is still spliced down to that single message — the original prompt, invalid payload, and schema guidance are all removed even though this prompt was not designed to be self-contained. The single stage-7 repair attempt is therefore likely to fail, and the stage7_schema_compact_repair_scheduled telemetry that would explain it is skipped. Base always used the compact self-contained prompt on this path, falling back to unsafe_candidate_like_payload.
This is a deliberate edit whose interaction with the unchanged replaceConversation derivation needs author confirmation.
Suggested fix: either restore the previous fallback (use the compact prompt with stage7Classification whenever stage === 7 && replaceConversationOverride === true), or make replacement follow the prompt choice — splice only when stage7CompactRepair is true and otherwise append.
Suggested test: schedule a stage-7 repair with replaceConversationOverride: true and a non-Stage7SchemaInvalidKind classification (e.g. revise_without_revision_payload) and assert either that the scheduled message retains payload/schema context or that prior conversation messages are not removed.
| } catch (error) { | ||
| const code = error instanceof CodegenieError ? error.code : error instanceof Error ? error.name : "unknown_error"; | ||
| await controller.finalizeFailure(code); | ||
| const code = actionErrorCode(error); |
There was a problem hiding this comment.
The extracted helper drops the error.name fallback, so every non-CodegenieError failure is published as unknown_error:
function actionErrorCode(error: unknown): CodegenieErrorCode | "unknown_error" {
return error instanceof CodegenieError ? error.code : "unknown_error";
}Base was error instanceof CodegenieError ? error.code : error instanceof Error ? error.name : "unknown_error". The new diagnostic does not compensate, because a plain TypeError/AbortError has no context.structuredSubmitFailure:
export function structuredSubmitFailureDiagnosticFromError(
error: unknown
): StructuredSubmitFailureDiagnostic | undefined {
if (!(error instanceof Error)) return undefined;
const context = (error as { context?: unknown }).context;
if (!isRecord(context)) return undefined;
const diagnostic = context.structuredSubmitFailure;
return isStructuredSubmitFailureDiagnostic(diagnostic) ? sanitizePublicDiagnostic(diagnostic) : undefined;Impact: the status comment body, the review_failed action record code, the failure JSON/markdown errorCode, and the new github-action: review failed — ${code} log line all show unknown_error where they previously carried the concrete error.name, so separate non-Codegenie failure classes are no longer distinguishable during triage. No typed contract forced this: finalizeFailure(errorCode: string, ...) in src/github-action/status-comment.ts still accepts an arbitrary string. The narrowing was also not applied uniformly — the sibling catch keeps the old expression:
const code = error instanceof CodegenieError ? error.code : error instanceof Error ? error.name : "unknown_error";
emitActionRecord(runResult.runDir, eventName, authorized, "terminal_post_failed", controller.stats(), env, write, code);so the two failure paths now publish different error-code vocabularies.
Suggested fix: either restore the fallback inside the helper (widening the return type to include string) if publishing the error name is acceptable, or, if suppressing attacker-influenced error.name is the intent, apply actionErrorCode to the terminal_post_failed catch as well so both paths share one vocabulary.
Suggested test: in tests/github-action.test.ts, make runReview reject with new TypeError("boom") and assert the error code appearing in the review_failed action record, the failure JSON errorCode, and the status comment body; cover the terminal_post_failed path with the same assertion.
| expect(summary.finalArgumentStates).toMatchObject({ partial: 1, repaired: 1, strict: 0 }); | ||
| expect(summary.finalArgumentErrorKinds).toMatchObject({ unterminated: 1, invalid_syntax: 0 }); | ||
| expect(summary.finalArgumentOutcomes).toEqual({ recovered: 1, terminal_invalid: 0, not_dispatched: 0 }); | ||
| const serialized = readRunFiles(attached.runDir); |
There was a problem hiding this comment.
The two leakage assertions added in the new telemetry test cannot fail, because the sentinel strings are never recorded:
const serialized = readRunFiles(attached.runDir);
expect(serialized).not.toContain("raw-event-repository-secret");
expect(serialized).not.toContain("Unexpected token from parser");A repo-wide search finds each string only in these assertions. Every value the test feeds the recorder is a bounded enum, id, or hash:
run.recorder.recordModelCall({ ...base, callId: "mc-1", ..., finalArgumentState: "partial", finalArgumentErrorKind: "unterminated", finalArgumentCorrelationId: "mc-1:submit" });
run.recorder.event({ stage: 7, level: "info", message: "final_argument_repair_outcome", data: { correlationId: "mc-1:submit", outcome: "recovered" } });Impact: the "no raw final-argument text / no secret in run artifacts" boundary these lines appear to guard is not exercised. If a later change starts persisting raw submit-argument text or raw parser error output (e.g. via LlmCallRecord.errorMessage or a richer final_argument_repair_outcome payload) into on-disk run artifacts, the test still passes. The surrounding assertions on finalArgumentStates/ErrorKinds/Outcomes aggregation remain valid coverage, and no production code is defective, so the cost is confined to these two lines.
The existing secret test in the same file shows the correct pattern — push the sentinel through the recorder and assert a positive redaction marker:
const allRunText = readRunFiles(attached.runDir);
expect(allRunText).not.toContain("super-secret-token");
expect(allRunText).toContain("[redacted:secret]");Suggested fix: make the ingress real, e.g.
run.recorder.recordModelCall({
...base,
callId: "mc-3",
status: "schema_invalid",
errorMessage: "Unexpected token from parser near raw-event-repository-secret",
finalArgumentState: "partial"
});then keep both not.toContain assertions and add a positive check (redaction marker present, or an exact key-set assertion on the final-argument fields written to model-calls.jsonl) so unexpected raw fields fail the test.
| expect(raw).toContain("CODEGENIE_FAILURE_PATH:"); | ||
| expect(raw).toContain("codegenie-failure.json"); | ||
| expect(raw).toContain("if: ${{ always()"); |
There was a problem hiding this comment.
The new coverage for the failure-artifact wiring matches raw file text rather than the parsed action, so the three assertions are mutually independent:
expect(raw).toContain("CODEGENIE_FAILURE_PATH:");
expect(raw).toContain("codegenie-failure.json");
expect(raw).toContain("if: ${{ always()");In action.yml, codegenie-failure.json appears twice — in the run step env and in the upload step's path list:
env:
CODEGENIE_REPORT_PATH: ${{ runner.temp }}/codegenie-report.md
CODEGENIE_FAILURE_PATH: ${{ runner.temp }}/codegenie-failure.json
...
- name: Upload review report
if: ${{ always() && inputs.preflight-only != 'true' }}
uses: actions/upload-artifact@v7
with:
name: codegenie-report
path: |
${{ runner.temp }}/codegenie-report.md
${{ runner.temp }}/codegenie-failure.json
if-no-files-found: ignoreImpact: deleting the failure file from the always()-guarded upload step — the exact line this PR adds — still satisfies all three assertions via the env occurrence, and the if: ${{ always() substring is never bound to that step, so removing its guard also passes while any other step keeps always(). These are the only assertions guarding this newly introduced contract (base has no matches for either string), so the failure diagnostics artifact could stop being published with no test failing. Scope is limited to loss of a CI diagnostics artifact, not review correctness or security.
Suggested fix: assert against the already-parsed action instead of raw — locate the upload step in action.runs.steps (by name/uses), assert its if contains always(), and assert its with.path includes the filename derived from the run step's env.CODEGENIE_FAILURE_PATH. The test must fail if the failure file is dropped from the path list or the guard is removed from that step alone.
There was a problem hiding this comment.
🧞 Codegenie Review
Reviewed 137/153 hunks.
Incomplete work: skipped 16.
Coverage disclosure:
- pnpm-lock.yaml: lockfile
Summary-only findings:
-
⚪ Low: Terminal llm_schema_invalid error drops the truncated validation
errorcontext key and the original cause (src/llm/pi-runner.ts:2496)
The terminal post-repair throw now carries only the structured diagnostic, dropping the truncated validationerrorcontext key and the originalcause:const structuredSubmitFailure = buildStructuredSubmitFailureDiagnostic({ stage: input.request.stage, role: roleForStage(input.request.stage), submitTool: input.submitToolName, submitSchemaVersion: SCHEMA_VERSIONS[submitToolNameForStage(input.request.stage)], attempt: "repair", classification: input.repairClassification ?? "schema_invalid", schema: input.request.schema, ...(input.cause instanceof Error ? { validationMessage: input.cause.message } : {}) }); throw new CodegenieError("llm_schema_invalid", "model submit payload failed schema validation after repair", { recoverable: input.request.schemaRepair?.failAfterRepair === true ? false : true, context: { structuredSubmitFailure } });
Base attached both:
context: { submitTool: input.submitToolName, error }, cause: input.cause
The removed key has a live consumer in
src/pipeline/verifier.ts:function verifierErrorSummary(error: unknown): string { if (isCodegenieError(error)) { return error.context?.error !== undefined ? String(error.context.error) : error.message; } return error instanceof Error ? error.message : String(error); }
whose output feeds classification and the recorded reason:
const errorSummary = sanitizeVerifierSchemaError(verifierErrorSummary(error)); const fallbackAttempt = repairAttempt ?? recordVerifierSchemaInvalid(candidate, errorSummary, classifyVerifierSchemaInvalid(errorSummary), telemetry, runtimeStats);
Impact: every repair-stage schema-invalid summary now collapses to the constant
"model submit payload failed schema validation after repair", soclassifyVerifierSchemaInvalidandverifierOutcomeReasonreceive the same string for all failures. Separately,validationMessageis only populated wheninput.cause instanceof Error, so a non-Errororundefinedcause yields an emptyissuesarray in the diagnostic and the failure detail is lost entirely. Control flow is unchanged and no invalid data is published; the diagnostic still exposes stage, role,submitTool, and classification.The move to
context.structuredSubmitFailurelooks deliberate (the same pattern is applied to the primary non-repair failure path), so this is a contract change rather than an assumed bug — please confirm whether losing the legacyerror/causedetail is intended, and update the consumer accordingly.Suggested fix: keep the structured diagnostic and also retain the legacy detail, plus a non-
Errorfallback forvalidationMessage:context: { structuredSubmitFailure, submitTool: input.submitToolName, error }, cause: input.cause // ... validationMessage: input.cause instanceof Error ? input.cause.message : input.cause !== undefined ? String(input.cause) : undefined
If the legacy keys are intentionally withheld, change
verifierErrorSummaryto readstructuredSubmitFailure(classification/issues) so the stage-9 summary stays informative.Suggested test: drive
queueSchemaRepairwithschemaRepairUsed=trueand both anErrorand a non-Errorcause, asserting the thrownCodegenieErrorstill yields a distinguishing summary fromverifierErrorSummary(or a non-emptystructuredSubmitFailure.issues) in both cases.
🙋 Needs human attention:
-
Does the degraded banner for complete planner-fallback reviews actually render prominently (renderCoverageTrustBanner) as the new spec line 210 asserts?
-
Does the new failure-artifact upload step in action.yml actually use
if: ${{ always() }}and write to CODEGENIE_FAILURE_PATH=codegenie-failure.json, and is any step-level guard asserted (the test only greps the raw YAML text, so anif: ${{ always() }}on an unrelated step would satisfy it)? -
Does the action actually write ${{ runner.temp }}/codegenie-failure.json (same filename/path) on failure paths, and is the upload step reached (e.g. if: always()) when the run fails?
-
Does the codegenie-failure.json artifact contain any model payload or repo-content excerpts that would be exposed to anyone with read access to workflow artifacts?
-
Is the banner content duplicated with renderCoverageSummaryLines/coverageDisclosureLines output (e.g. "Planning was degraded and deterministic fallbacks were used.") in the posted review body?
-
Additional unresolved notes suppressed: 7
Inline findings included in the review body: -
🔵 Medium: recordModelCall eagerly calls submitToolNameForStage(), which throws for stages without a submit schema (
src/llm/pi-runner.ts:2954)
recordModelCallnow evaluates the submit-tool lookup as a plain, unguarded argument:const finalArguments = finalArgumentTelemetry(message, request.stage, submitToolNameForStage(request.stage), meta.callId);
That helper throws for any stage without a submit schema:
export function submitToolNameForStage(stage: ReviewStage): keyof typeof SCHEMA_VERSIONS { switch (stage) { case 5: return "submit_plan"; case 7: return "submit_review"; case 8: return "submit_system_review"; case 9: return "submit_verdict"; case 10: return "submit_composition"; default: throw new Error(`stage ${stage} does not have a submit schema`); } }
export type ReviewStage = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11;
All three call sites pass
requestthrough without stage filtering, andrecordModelCallcontains no stage guard ortry/catch:// src/llm/pi-runner.ts 1079: recordModelCall(opts, request, model, cachedResponse.message, { 1213: recordModelCall(opts, request, model, message, { 1287: recordModelCall(opts, request, model, message, definedRecord({
Impact: for a structured request on stages 1, 2, 3, 4, 6, or 11 the throw propagates out of a purely observational telemetry path (which also runs on the cached-response and provider-failure paths), aborting the enclosing provider-call flow and losing both the model-call record and the completed provider result. Previously
recordModelCallhad no dependency on the submit-tool mapping and recorded calls for all stages;finalArgumentTelemetryalready returns{}when no matching tool call is present, so a missing submit tool does not need an exception. Reachability from a live non-submit-stage caller is unconfirmed (the search for such construction sites was cut short), so today this is a latent crash rather than a demonstrated failure.Suggested fix: resolve the lookup defensively, or move it inside
finalArgumentTelemetryand return{}for stages without a submit schema (e.g. add a non-throwingsubmitToolNameForStageOrUndefinedinsrc/llm/schemas.ts):const submitTool = STAGES_WITH_SUBMIT.has(request.stage) ? submitToolNameForStage(request.stage) : undefined; const finalArguments = submitTool === undefined ? {} : finalArgumentTelemetry(message, request.stage, submitTool, meta.callId);
Suggested test: invoke
recordModelCall(or the runner path reaching it) with a request whose stage is1,2,6, or11, and asserttelemetry.recordModelCallis still called with a record omitting thefinalArgument*fields rather than throwing.
— codegenie v0.5.4 (ae1bb70243) · View Workflow Job
| delete capture.endCall; | ||
| } | ||
| captures.clear(); | ||
| hooks.onBuffersCleared?.(0); |
There was a problem hiding this comment.
The onBuffersCleared hook always receives the literal 0, so nothing observable depends on the clearing loop actually running:
} finally {
for (const capture of captures.values()) {
capture.text = "";
delete capture.endCall;
}
captures.clear();
hooks.onBuffersCleared?.(0);
}Both clearing tests assert only that value:
expect(cleared).toHaveBeenCalledWith(0);Impact: the documented guarantee that "argument fragments ... are cleared before it returns or throws" — the confidentiality property of this trust boundary, since captured submit payloads may hold sensitive repository content — has no test that can fail. Removing capture.text = "" / captures.clear(), or making the map longer-lived, leaves preserves terminal error semantics ... and clears buffers and throws a bounded error and clears buffers ... green while retained model argument text goes undetected. The hook's own type declares onBuffersCleared?(remainingChars: number), implying a derived measurement rather than a constant.
Suggested fix: derive the reported value from the actual post-clear state, e.g. accumulate capture.text.length after clearing, or expose the map size / aggregate length so the hook value is computed rather than hard-coded.
Suggested test: feed non-trivial deltas, then assert the derived count is 0 only because clearing ran, so removing the clearing loop makes the assertion fail.
| if (Buffer.byteLength(markdown, "utf8") <= FAILURE_MARKDOWN_MAX_BYTES) { | ||
| return `${markdown.trimEnd()}\n`; | ||
| } | ||
| return `${Buffer.from(markdown, "utf8").subarray(0, FAILURE_MARKDOWN_MAX_BYTES - 64).toString("utf8").trimEnd()}\n\n[Failure report truncated.]\n`; |
There was a problem hiding this comment.
fitFailureMarkdown slices the raw byte buffer at a fixed offset without aligning to a UTF-8 code-point boundary:
return `${Buffer.from(markdown, "utf8").subarray(0, FAILURE_MARKDOWN_MAX_BYTES - 64).toString("utf8").trimEnd()}\n\n[Failure report truncated.]\n`;The result is published to both the report file and the GitHub step summary:
const markdown = fitFailureMarkdown([
"# 🧞 Codegenie Review Failed", /* ... */ renderStructuredSubmitFailure(input.diagnostic) /* ... */
]);
writeFailureFile(input.env.CODEGENIE_REPORT_PATH, markdown);
appendFileSync(stepSummary, `${sanitizeGitHubCommentBody(markdown).trimEnd()}\n`);Impact: when the cut offset lands inside a multi-byte sequence (the header already contains the 4-byte 🧞 emoji, and diagnostic text can include non-ASCII paths or rule names), Buffer#toString("utf8") substitutes U+FFFD, so the operator's primary failure surface ends in mojibake instead of clean truncated Markdown. Cosmetic, but this path only runs when a review has already failed and diagnosability is the point of the change.
Suggested fix: truncate at a code-point boundary — use a StringDecoder/TextDecoder with stream semantics, walk back from the cut while (bytes[i] & 0xc0) === 0x80, or truncate on the string with a byte-length check loop.
Suggested test: call fitFailureMarkdown (or publishFailureFiles) with a diagnostic whose rendered text exceeds 4 KiB and places multi-byte characters across byte 4032, and assert the returned markdown contains no \uFFFD.
There was a problem hiding this comment.
🧞 Codegenie Review
Reviewed 139 of 154 hunks (15 skipped, pnpm-lock.yaml lockfile only); coverage was complete and non-degraded. Nine verified findings, all in the new LLM final-argument trust boundary, telemetry aggregation, GitHub Action failure surface, and the accompanying tests.
Highest-value items to address before merge:
consumeFinalToolArgumentsturns providererrorstream events into resolved messages, so transport/provider failures no longer rejectcomplete()and get misclassified as model indiscipline (src/llm/final-tool-arguments.ts,src/llm/pi-runner.ts).- The stage-7 repair path can now replace the whole conversation while using a non-self-contained prompt, spending the single allowed repair turn without packet context (
src/llm/pi-runner.ts). - Assistant messages containing an
invalidToolCallpart are dropped even when they also carry valid repository tool calls, leavingtoolResultentries with no matching assistant turn (src/llm/pi-runner.ts).
Lower-severity items: unknown_error collapse in the Action entrypoint diverging from the sibling catch block, dropped context.error/cause on the terminal llm_schema_invalid throw degrading verifier summaries, final-argument counters including cache-hit replays, and three test-strength gaps (vacuous payload-leak assertions, raw-text action.yml assertions, and pre-stamped strict provenance hiding the removeProvenanceLessSubmitArguments guard).
Several of these are contract changes with mixed intent signals; each notes the specific confirmation needed from the author.
Reviewed 139/154 hunks.
Incomplete work: skipped 15.
Coverage disclosure:
- pnpm-lock.yaml: lockfile
Summary-only findings:
-
⚪ Low: llm_schema_invalid repair failure drops context.error and cause, degrading verifier failure summaries (
src/llm/pi-runner.ts:2496)
The terminal post-repairllm_schema_invalidthrow no longer carriescontext.errororcause, so verifier failure summaries lose the specific validation diagnostic.throw new CodegenieError("llm_schema_invalid", "model submit payload failed schema validation after repair", { recoverable: input.request.schemaRepair?.failAfterRepair === true ? false : true, - context: { submitTool: input.submitToolName, error }, - cause: input.cause + context: { structuredSubmitFailure } });
src/pipeline/verifier.tsreads exactly the removed key, and is reached for this code viaisSchemaInvalidError/verifierOutcomeReason:function isSchemaInvalidError(error: unknown): boolean { return isCodegenieError(error) && error.code === "llm_schema_invalid"; } function verifierErrorSummary(error: unknown): string { if (isCodegenieError(error)) { return error.context?.error !== undefined ? String(error.context.error) : error.message; } return error instanceof Error ? error.message : String(error); }
The replacement diagnostic preserves
submitToolbut has no free-form error field, and itsissuescome only fromvalidationMessage, which is populated only wheninput.cause instanceof Error(src/llm/schema-diagnostics.ts).Impact: verifier incomplete-reason text collapses to the constant
"model submit payload failed schema validation after repair", and the underlying cause is no longer reachable by cause-chain walkers; non-Errorcauses lose all detail. Operator-facing diagnostics only — no control flow, recoverability, or published data changes. The context reshaping is clearly intentional (consumed bystructuredSubmitFailureDiagnosticFromErrorin the Action entrypoint), so please confirm whether dropping theerrorkey andcausewas also intended.Suggested fix: keep both shapes, e.g.
context: { submitTool: input.submitToolName, error, structuredSubmitFailure }, cause: input.cause, or makeverifierErrorSummaryfall back tocontext.structuredSubmitFailure(classification/issues) whencontext.erroris absent.Suggested test: exercise the post-repair failure path with a non-
Errorcause and assert the verifier incomplete reason still contains the specific validation diagnostic rather than only the constant message. -
⚪ Low: assistant() pre-stamps strict provenance on submit_ toolCalls, leaving removeProvenanceLessSubmitArguments untested (
tests/phase4-llm.test.ts:5103)
Theassistant()fixture stamps{ state: "strict" }on everysubmit_*toolCall, so theremoveProvenanceLessSubmitArgumentsdowngrade branch is never exercised by this suite.const contentWithProvenance = content.map((block) => (block as { type?: unknown; name?: unknown }).type === "toolCall" && typeof (block as { name?: unknown }).name === "string" && String((block as { name: string }).name).startsWith("submit_") ? { ...block, argumentParse: { state: "strict" as const } } : block );
The guard only fires for a submit block of type
toolCallwhoseargumentParseis absent or untrusted (src/llm/pi-runner.ts:1700-1730):function removeProvenanceLessSubmitArguments(message: PiAssistantMessage, submitToolName: string): PiAssistantMessage { const content = message.content.map((block) => { if (!isToolCall(block) || block.name !== submitToolName || hasTrustedArgumentParse(block)) { return block; } return { type: "invalidToolCall", id: block.id, name: block.name, argumentParse: { state: "event_capture_missing" } } satisfies PiInvalidToolCall; }); return { ...message, content }; } function hasTrustedArgumentParse(call: PiToolCall): boolean { return call.argumentParse?.state === "strict" || call.argumentParse?.state === "repaired"; }
The new untrusted-provenance tests inject blocks that are already
invalidToolCall, entering the pipeline downstream of the downgrade:function invalidSubmitCall(id: string, name: string, argumentParse: PiUntrustedArgumentParse): PiInvalidToolCall { return { type: "invalidToolCall", id, name, argumentParse }; }
Impact: if the guard, its name/state predicate, or its call site were removed, a provenance-less submit
toolCallwould be accepted and published and this suite would still pass. Narrow gap — classification, repair routing,final_arguments_rejectedtelemetry, and fail-closed behavior for all five untrusted states are asserted. One open item: whetherscriptedAdapterroutes fixtures throughstreamForMessageand the real finalizer, which would re-derive provenance and make the pre-stamp inert.Suggested fix: stop stamping provenance inside
assistant()and applytrustedSubmitCall(...)at the fixture sites that need a trusted call.Suggested test: add a
scriptedAdaptercase whose primary message contains a raw{ type: "toolCall", name: "submit_review", arguments: {...} }block with noargumentParse, runrunStructured, and assertuntrustedSubmitCallscontains{ id, name: "submit_review", state: "event_capture_missing" }and that nothing is published from that turn.
🙋 Needs human attention:
- Should renderNoFindings suppress the "Everything looks good" no-findings blurb when coverage.degradedPlanning is true but partial is false, and should the new degraded-planning test assert that?
- Do the verifier reason schema/prompt enforce a 2,000-character target with a 4,000-character hard maximum, with telemetry on accepted target overflow, as newly claimed on line 623?
- Is there test coverage for the noFindings + degradedPlanning (non-partial) markdown output path?
- Does the schema/prompt for Stage-9 verifier reason actually allow up to 4,000 characters (hard buffer) with a 2,000-character target, matching this ledger entry's claim of preserved 2,100-2,984 character reasons?
- Does fitFailureMarkdown's byte-slice truncation (Buffer.subarray on utf8) risk emitting a lone replacement char / broken multi-byte sequence for the emoji-containing header path?
- Additional unresolved notes suppressed: 1
— codegenie v0.5.4 (ae1bb70243) · View Workflow Job
| if (record.finalArgumentState !== undefined) { | ||
| this.modelSummary.finalArgumentStates[record.finalArgumentState] += 1; |
There was a problem hiding this comment.
The new finalArgumentStates/finalArgumentErrorKinds counters also count cache-hit replays, so model-calls-summary.json can report more final-argument states than providerCalls.
if (record.finalArgumentState !== undefined) {
this.modelSummary.finalArgumentStates[record.finalArgumentState] += 1;
}
if (record.finalArgumentErrorKind !== undefined) {
this.modelSummary.finalArgumentErrorKinds[record.finalArgumentErrorKind] += 1;
}Sibling provider-attribute aggregates in the same method are gated on providerCallCount:
const providerCallCount = record.cacheStatus === "hit" ? 0 : 1;
this.modelSummary.providerCalls += providerCallCount;
this.modelSummary.retryAttempts += providerCallCount > 0 && record.attempt > 1 ? 1 : 0;
this.modelSummary.toolChoiceDowngradedCalls += providerCallCount > 0 && record.toolChoiceDowngraded === true ? 1 : 0;Cache replays reach this path with the fields populated: src/llm/pi-runner.ts records cached responses via recordModelCall(..., { cacheStatus: "hit", durationMs: 0, ... }), and recordModelCall derives the provenance fields from the (possibly cached) message:
const finalArguments = finalArgumentTelemetry(message, request.stage, submitToolNameForStage(request.stage), meta.callId);
...
finalArgumentState: state,
finalArgumentErrorKind: parse?.state === "partial" || parse?.state === "invalid" ? parse.errorKind : undefined,Impact: diagnostics-only skew in the aggregate this PR adds for the final-argument trust boundary — cached runs look like they experienced fresh provider-side argument states, overstating structured-submit failures in replay/eval runs. No production behavior depends on these counters. Note that sibling record-level counters (repairCalls, schemaInvalidCalls) are also ungated, so please confirm which convention these are meant to follow.
Suggested fix: if they describe provider-side behavior, gate both increments:
if (providerCallCount > 0 && record.finalArgumentState !== undefined) {
this.modelSummary.finalArgumentStates[record.finalArgumentState] += 1;
}
if (providerCallCount > 0 && record.finalArgumentErrorKind !== undefined) {
this.modelSummary.finalArgumentErrorKinds[record.finalArgumentErrorKind] += 1;
}Otherwise document in the summary schema that they are per-record (including cache hits).
Suggested test: in tests/telemetry.test.ts, record one cacheStatus: "hit" record with finalArgumentState: "partial" and finalArgumentErrorKind: "unterminated" and pin the intended contract — either both counters stay 0 while cache.hit === 1 and totalRecords === 1, or assert the per-record semantics explicitly.
There was a problem hiding this comment.
🧞 Codegenie Review
Reviewed 149 of 160 hunks (11 skipped, notably pnpm-lock.yaml); no coverage degradation. Five verified findings, all concentrated in the new final-argument trust boundary and its consumers in src/llm/pi-runner.ts and src/llm/final-tool-arguments.ts.
Highest-value items: (1) after the switch from models.complete/completeSimple to models.stream/streamSimple, a Pi error terminal event is cast to an assistant message instead of rejecting, so provider/transport failures lose their type and classification on the primary structured-submit path; (2) the new trust gate inside submitCallHasFindings prevents the candidateDrafted latch from being set for findings-bearing but untrusted submits, which disables the Stage 7 anti-downgrade guard and can let a repaired findings: [] be published as a legitimate no-findings review.
Lower-severity items are diagnostic-fidelity gaps: removeProvenanceLessSubmitArguments collapses every untrusted state to event_capture_missing (making the new length_stopped/partial/invalid/event_final_mismatch classification branches unreachable for submit calls), semantic and untrusted-argument rejections are recorded as schema_invalid/llm_schema_invalid telemetry, and the onBuffersCleared hook always reports a literal 0, making the buffer-clearing tests vacuous.
Since these are behavior changes with mixed intent evidence, several items ask for confirmation of the intended contract rather than asserting a regression. Open follow-ups worth author attention: the runtime shape of pi-ai's error event payload, whether tests/pipeline-phase5.test.ts still exercises untrusted submit provenance now that the shared toolCall helper stamps argumentParse: { state: "strict" }, and whether the stream-derived state actually gates acceptance or is telemetry-only.
Reviewed 149/160 hunks.
Incomplete work: skipped 11.
Coverage disclosure:
- pnpm-lock.yaml: lockfile
🙋 Needs human attention:
- Was SubmitVerificationVerdictSchema.reason.maxLength actually raised to 4,000 (VERIFIER_REASON_HARD_MAX_CHARS) and the Stage-9 prompt version bumped from p9.8 to p9.9, as required alongside this schema-version bump?
- Is there a test proving that a terminal Stage 5 llm_schema_invalid failure (now recoverable due to failAfterRepair: false) is caught by runPlanner/runChunkedPlanner and produces the deterministic default coverage fallback?
- Do all downstream consumers of classifyVerifierSchemaInvalid (repair-prompt selection, telemetry labels, retry/abort policy) handle the newly reachable FinalArgumentFailureClassification kinds (length_stopped, final_arguments_partial, final_arguments_invalid, event_capture_missing, event_final_mismatch) rather than falling through a default branch that skips repair?
- Is updateFinalArgumentOutcomeFromEvent idempotent/counting-safe given it is invoked for every event, not only final-argument events?
- Does any downstream consumer of VerificationVerdict.reason (composer/publisher/telemetry serialization) still assume a 2,000-character bound now that the verifier schema permits up to 4,000?
- Additional unresolved notes suppressed: 13
— codegenie v0.5.4 (ae1bb70243) · View Workflow Job
| type: "invalidToolCall", | ||
| id: block.id, | ||
| name: block.name, | ||
| argumentParse: { state: "event_capture_missing" } |
There was a problem hiding this comment.
removeProvenanceLessSubmitArguments hardcodes argumentParse to { state: "event_capture_missing" } and drops errorKind, so the four other untrusted-state classification branches added in this same diff are unreachable for submit calls.
function removeProvenanceLessSubmitArguments(message: PiAssistantMessage, submitToolName: string): PiAssistantMessage {
const content = message.content.map((block) => {
if (!isToolCall(block) || block.name !== submitToolName || hasTrustedArgumentParse(block)) {
return block;
}
return {
type: "invalidToolCall",
id: block.id,
name: block.name,
argumentParse: { state: "event_capture_missing" }
} satisfies PiInvalidToolCall;
});
return { ...message, content };
}Both the cached and live paths run this rewrite before any consumer sees the message:
// line 1043 (cached response)
message: removeProvenanceLessSubmitArguments(scrubbedCachedResponse.message, submitToolNameForStage(request.stage))
// line 1209 (live provider response)
const message = removeProvenanceLessSubmitArguments(scrubAssistantMessage(rawMessage), submitToolNameForStage(request.stage));The downstream readers therefore only ever observe one state:
function provenanceFailureClassification(call: PiSubmitCall): LlmSubmitFailureClassification {
const state = call.argumentParse?.state;
if (state === "length_stopped") return "length_stopped";
if (state === "partial") return "final_arguments_partial";
if (state === "invalid") return "final_arguments_invalid";
if (state === "event_final_mismatch") return "event_final_mismatch";
return "event_capture_missing";
}and untrustedRepairMetadata's conditional errorKind spread can never fire for submit calls, because errorKind was dropped by the rewrite. The collapsed classification is what gets recorded (line 439):
const classification = provenanceFailureClassification(submitCall);
recordRejectedFinalArguments(opts, request, submitTool.name, submitCall, classification, schemaRepairUsed, correlationId);Impact: triage cannot distinguish a length-truncated submit from a provider that never emitted argument events, and the schema-repair prompt loses the parse errorKind that would let it target the actual defect. The call is still correctly treated as untrusted and no invalid payload is published, so this is bounded to one telemetry field plus the optional errorKind hint in LlmSchemaRepairInput.untrustedSubmitCalls.
Suggested fix: preserve the incoming untrusted provenance instead of hardcoding it:
const parse = block.argumentParse;
const argumentParse: PiUntrustedArgumentParse =
parse && parse.state !== "strict" && parse.state !== "repaired"
? parse
: { state: "event_capture_missing" };
return { type: "invalidToolCall", id: block.id, name: block.name, argumentParse } satisfies PiInvalidToolCall;If the collapse is deliberate trust-boundary policy, remove or document the now-dead branches in provenanceFailureClassification and untrustedRepairMetadata so the code does not imply a fidelity it cannot deliver.
Suggested test: unit-test removeProvenanceLessSubmitArguments with a submit toolCall carrying { state: "length_stopped" } and another carrying { state: "invalid", errorKind: ... }; assert the resulting invalidToolCall retains those states and errorKind, and that provenanceFailureClassification returns "length_stopped" / "final_arguments_invalid". Add a cached-path case asserting the recorded classification for a length-stopped cached submit.
One secondary check remains open: whether pi-ai / consumeFinalToolArguments actually attaches length_stopped/partial/invalid states to message blocks before this function runs. If it never does, the added branches are merely defensive rather than lossy.
| } | ||
| try { | ||
| adapter.validateToolCall([toolSpec(submitTool)], submitCall); | ||
| validateSubmitCall(adapter, request, submitTool, submitCall); |
There was a problem hiding this comment.
schemaValidityForResponse now returns false for two non-schema conditions — semantic rejection and untrusted final arguments — and that boolean is mapped straight onto the schema_invalid telemetry status.
if (!isTrustedSubmitCall(submitCall)) {
return false;
}
try {
validateSubmitCall(adapter, request, submitTool, submitCall);
return true;
} catch {
return false;
}The helper throws for schema-conformant submits that fail the semantic validator (src/llm/pi-runner.ts:2363-2375):
function validateSubmitCall<T>(
adapter: PiAiAdapter,
request: LlmStructuredRequest<T>,
submitTool: ToolDefinition,
submitCall: PiToolCall
): T {
const validated = adapter.validateToolCall([toolSpec(submitTool)], submitCall) as T;
const semantic = request.validateSubmit?.(validated);
if (semantic !== undefined && !semantic.ok) {
throw new SubmitSemanticValidationError(semantic.classification);
}
return validated;
}and the consumer conflates all three causes (lines 1261, 1289-1290):
const schemaValid = schemaValidityForResponse(adapter, request, tools, kind, message);
// ...
const callStatus = schemaValid === false ? "schema_invalid" : "ok";
const callErrorCode = schemaValid === false ? "llm_schema_invalid" : undefined;schemaValid is a persisted telemetry field (src/telemetry/telemetry-recorder.ts:62), so a submit rejected by request.validateSubmit with e.g. revise_without_revision_payload, or one that fails only the new trust check, is recorded as a schema violation even though the schema was never violated.
Impact: debugging is pointed at schema definitions instead of the semantic validator or the argument-trust boundary, and 0.5.5 schema-violation failure-rate metrics overcount. No response data is dropped or wrongly published, and the related cache suppression via isCacheableProviderResponse is explicitly intended ("routes semantic revise_without_revision_payload through the real one-repair and cache-validity boundary") — the telemetry labeling is what is unaddressed. This changes what schemaValid means for downstream consumers; please confirm the intended classification.
Suggested fix: return a discriminated result (e.g. { schemaValid: boolean; semanticValid: boolean; trusted: boolean }), or catch SubmitSemanticValidationError and the trust-gate case separately, so cacheability can still be suppressed while telemetry records distinct statuses and error codes rather than schema_invalid/llm_schema_invalid.
Suggested test: a pi-runner case where the submit call satisfies the JSON schema but request.validateSubmit returns { ok: false, classification: "revise_without_revision_payload" }; assert the response is not cached and that the recorded model-call status and error code reflect a semantic rejection. Add a sibling case where hasTrustedArgumentParse is false.
1 participant
Summary
Implements Plans 111, 112, and 114 for Codegenie 0.5.5.
Plan 111: observed submit resilience
revise_without_revision_payloadthrough the real one-repair and cache-validity boundary.always().Plan 112: final-argument provenance
stream()/streamSimple()paths.toolcall_endand terminal values.arguments.Plan 114: context-preserving provenance retry
replaceConversationOverride: falsethrough both scheduler forwarding layers.actionErrorCode()vocabulary consistently for terminal-post failures.No provider-specific parsing, Pi fork/patch, broad JSON repair library, delimiter completion, second repair loop, or provider-specific fixture matrix was added.
Validation
Core implementation gates:
pnpm install --frozen-lockfilepnpm run checkpnpm test— 41 files, 839 tests passed for Plans 111/112pnpm buildgit diff --checkclaude-haiku-4-5) — strict schema-valid submit, no repairgpt-5.4-mini) — strict schema-valid submit, no repairLive no-cache evals:
49f4645brun 64 — pass after Plans 111/112; complete coverage and no unrecovered structured-submit failures.49f4645brun 66 — pass with Plan 114; a real Stage-9invalid_syntaxfinal was rejected, the repair was scheduled withreplaceConversation: false, the five-message trusted prefix was retained, the rejected turn was absent, and the strict retry recovered.0c4d5213run 74 — pass with Plan 114; 131/131 hunks reviewed, all required/optional/negative guards passed, seven inline findings, 245 model calls, $23.6898 cost, and one Stage-7 extra-property fault recovered deterministically without another provider call. All 94 observed final submits were strict, with zero partial/invalid/length-stopped/capture-missing/divergent finals.The live evals cover both sides of the trust boundary: run 66 exercises the context-preserving malformed-provenance retry, while run 74 confirms ordinary strict and deterministic schema-cleanup paths remain stable.
Plan status
IMPLEMENTED (dogfood pending). The code and owner-case validation are complete; the wider repeat suite and a live Action-failure observation remain external validation.IMPLEMENTED (measuring). The trust boundary is implemented; the post-land corpus remains intentionally open before any broader parser or upstream Pi proposal.BACKLOG (measurement gate not met). It records an intermittent stale-note contradiction and authorizes no production change yet.COMPLETE. Deterministic gates and live owner smokes are complete.Release and bookkeeping
tsxdevelopment dependency from 4.23.1 to 4.23.4.Small test-hygiene adjustment
Two filesystem-heavy pre-existing integration tests repeatedly exceeded Vitest's default timeout by only tens to hundreds of milliseconds during the full parallel suite while passing alone. Their per-test timeout is explicitly 15 seconds; production behavior is unchanged.