diff --git a/.github/workflows/codegenie-review.yml b/.github/workflows/codegenie-review.yml index 6676e69..030cca6 100644 --- a/.github/workflows/codegenie-review.yml +++ b/.github/workflows/codegenie-review.yml @@ -63,6 +63,7 @@ jobs: GH_TOKEN: ${{ github.token }} LLM_API_KEY: ${{ secrets.LLM_API_KEY }} CODEGENIE_REPORT_PATH: ${{ runner.temp }}/codegenie-report.md + CODEGENIE_FAILURE_PATH: ${{ runner.temp }}/codegenie-failure.json # Bootstrap guard: the trusted-base checkout builds the BASE branch's # codegenie, so a PR whose base predates the github-action adapter # cannot run it — skip cleanly instead of failing. Once the adapter is @@ -79,5 +80,7 @@ jobs: uses: actions/upload-artifact@v7 with: name: codegenie-report - path: ${{ runner.temp }}/codegenie-report.md + path: | + ${{ runner.temp }}/codegenie-report.md + ${{ runner.temp }}/codegenie-failure.json if-no-files-found: ignore diff --git a/README.md b/README.md index 7b6e0a6..70a956e 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,7 @@ jobs: with: ref: ${{ github.event.pull_request.base.sha }} # trusted base; PR head is fetched as review data fetch-depth: 0 - - uses: 0xPolygon/codegenie@v0.5.4 + - uses: 0xPolygon/codegenie@v0.5.5 with: model: "anthropic/claude-opus-5:high" llm-api-key: ${{ secrets.LLM_API_KEY }} diff --git a/action.yml b/action.yml index a8e5f78..e0939f2 100644 --- a/action.yml +++ b/action.yml @@ -77,6 +77,7 @@ runs: env: GH_TOKEN: ${{ inputs.github-token }} CODEGENIE_REPORT_PATH: ${{ runner.temp }}/codegenie-report.md + CODEGENIE_FAILURE_PATH: ${{ runner.temp }}/codegenie-failure.json INPUT_TRIGGER_PHRASE: ${{ inputs.trigger-phrase }} INPUT_ON_PULL_REQUEST: ${{ inputs.on-pull-request }} INPUT_ALLOWED_ASSOCIATIONS: ${{ inputs.allowed-associations }} @@ -117,5 +118,7 @@ runs: uses: actions/upload-artifact@v7 with: name: codegenie-report - path: ${{ runner.temp }}/codegenie-report.md + path: | + ${{ runner.temp }}/codegenie-report.md + ${{ runner.temp }}/codegenie-failure.json if-no-files-found: ignore diff --git a/examples/workflows/codegenie-review-comment.yml b/examples/workflows/codegenie-review-comment.yml index 81ca342..9160f4b 100644 --- a/examples/workflows/codegenie-review-comment.yml +++ b/examples/workflows/codegenie-review-comment.yml @@ -34,7 +34,7 @@ jobs: with: fetch-depth: 0 - - uses: 0xPolygon/codegenie@v0.5.4 + - uses: 0xPolygon/codegenie@v0.5.5 with: model: "anthropic/claude-opus-5:high" llm-api-key: ${{ secrets.LLM_API_KEY }} diff --git a/examples/workflows/codegenie-review-pr.yml b/examples/workflows/codegenie-review-pr.yml index d6c6486..eca1051 100644 --- a/examples/workflows/codegenie-review-pr.yml +++ b/examples/workflows/codegenie-review-pr.yml @@ -34,7 +34,7 @@ jobs: ref: ${{ github.event.pull_request.base.sha }} fetch-depth: 0 - - uses: 0xPolygon/codegenie@v0.5.4 + - uses: 0xPolygon/codegenie@v0.5.5 with: model: "anthropic/claude-opus-5:high" llm-api-key: ${{ secrets.LLM_API_KEY }} diff --git a/package.json b/package.json index fc8a531..f95cedf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@0xsequence/codegenie", - "version": "0.5.4", + "version": "0.5.5", "description": "High-signal AI code review agent", "type": "module", "bin": { @@ -46,7 +46,7 @@ "tree-sitter-rust": "0.24.0", "tree-sitter-solidity": "1.2.13", "tree-sitter-typescript": "0.23.2", - "tsx": "4.23.1", + "tsx": "4.23.4", "typescript": "7.0.2", "vitest": "4.1.10" } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f4756c9..109cea1 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -61,14 +61,14 @@ importers: specifier: 0.23.2 version: 0.23.2 tsx: - specifier: 4.23.1 - version: 4.23.1 + specifier: 4.23.4 + version: 4.23.4 typescript: specifier: 7.0.2 version: 7.0.2 vitest: specifier: 4.1.10 - version: 4.1.10(@opentelemetry/api@1.9.0)(@types/node@26.1.2)(vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.1)(yaml@2.9.0)) + version: 4.1.10(@opentelemetry/api@1.9.0)(@types/node@26.1.2)(vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.4)(yaml@2.9.0)) packages: @@ -1179,8 +1179,8 @@ packages: tslib@2.8.1: resolution: {integrity: sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==} - tsx@4.23.1: - resolution: {integrity: sha512-GQHnkIfxyx1wYCOS/wonik5MVRZU9hi1TEZmzGZSCJB1y9YgoZ8H6itNE/u4suE+yLmOzuE4E5S4TZ/ZX2wcWQ==} + tsx@4.23.4: + resolution: {integrity: sha512-ZiUQ8oT/KzN51mJUWPqARYqwFLFJZtGZipRkw1ynHMr9vy3eU77m5yfF3Gzm6meEg/beW+lUu3fHYgskTN2oVQ==} engines: {node: '>=18.0.0'} hasBin: true @@ -1942,13 +1942,13 @@ snapshots: chai: 6.2.2 tinyrainbow: 3.1.1 - '@vitest/mocker@4.1.10(vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.1)(yaml@2.9.0))': + '@vitest/mocker@4.1.10(vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.4)(yaml@2.9.0))': dependencies: '@vitest/spy': 4.1.10 estree-walker: 3.0.3 magic-string: 0.30.21 optionalDependencies: - vite: 8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.1)(yaml@2.9.0) + vite: 8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.4)(yaml@2.9.0) '@vitest/pretty-format@4.1.10': dependencies: @@ -2375,7 +2375,7 @@ snapshots: tslib@2.8.1: {} - tsx@4.23.1: + tsx@4.23.4: dependencies: esbuild: 0.28.1 optionalDependencies: @@ -2410,7 +2410,7 @@ snapshots: unicorn-magic@0.3.0: {} - vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.1)(yaml@2.9.0): + vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.4)(yaml@2.9.0): dependencies: lightningcss: 1.32.0 picomatch: 4.0.5 @@ -2421,13 +2421,13 @@ snapshots: '@types/node': 26.1.2 esbuild: 0.28.1 fsevents: 2.3.3 - tsx: 4.23.1 + tsx: 4.23.4 yaml: 2.9.0 - vitest@4.1.10(@opentelemetry/api@1.9.0)(@types/node@26.1.2)(vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.1)(yaml@2.9.0)): + vitest@4.1.10(@opentelemetry/api@1.9.0)(@types/node@26.1.2)(vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.4)(yaml@2.9.0)): dependencies: '@vitest/expect': 4.1.10 - '@vitest/mocker': 4.1.10(vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.1)(yaml@2.9.0)) + '@vitest/mocker': 4.1.10(vite@8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.4)(yaml@2.9.0)) '@vitest/pretty-format': 4.1.10 '@vitest/runner': 4.1.10 '@vitest/snapshot': 4.1.10 @@ -2444,7 +2444,7 @@ snapshots: tinyexec: 1.2.4 tinyglobby: 0.2.17 tinyrainbow: 3.1.1 - vite: 8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.1)(yaml@2.9.0) + vite: 8.0.16(@types/node@26.1.2)(esbuild@0.28.1)(tsx@4.23.4)(yaml@2.9.0) why-is-node-running: 2.3.0 optionalDependencies: '@opentelemetry/api': 1.9.0 diff --git a/specs/plans/111-issue-111-observed-structured-submit-resilience.md b/specs/plans/111-issue-111-observed-structured-submit-resilience.md new file mode 100644 index 0000000..dec1311 --- /dev/null +++ b/specs/plans/111-issue-111-observed-structured-submit-resilience.md @@ -0,0 +1,595 @@ +# Issue 111: Fix Observed Structured-Submit Failures and Preserve Safe Diagnostics + +Status: IMPLEMENTED (dogfood pending) +Planned from: GitHub Action run `30998651040` / job `92281793676`, +trails-api eval `49f4645b` runs 61-62, and `0c4d5213` run 69, +2026-08-05 +Planned at: commit `ae1bb70` (branch `llm-repair`; tree-identical to the +original planning SHA `07434ba` on the retired `plans` branch) +Recommended priority: immediate. This plan fixes the observed production +failure and measured Stage-9 friction without waiting for an upstream Pi +release or building a speculative general JSON-repair layer. + +> **Executor instructions**: Follow this plan step by step. Read the entire +> plan before changing code. Run every verification command and confirm its +> expected result before continuing. If a STOP condition occurs, stop and +> report; do not add permissive parsing or weaken a submit schema. Update this +> plan's row in `specs/plans/README.md` when complete. +> +> **Drift check (run first)**: +> `git diff --stat ae1bb70..HEAD -- src/llm/llm-runner.ts src/llm/pi-runner.ts src/llm/schema-diagnostics.ts src/llm/schemas.ts src/pipeline/planner.ts src/pipeline/verifier.ts src/pipeline/composer.ts src/skills/prompt-builder.ts src/output/markdown-renderer.ts src/util/coverage-summary.ts src/github-action/entrypoint.ts src/github-action/render.ts action.yml .github/workflows/codegenie-review.yml tests/phase4-llm.test.ts tests/pipeline-phase5.test.ts tests/github-action.test.ts tests/shared-utils.test.ts specs/project/architecture.md specs/project/components/skills_llm_telemetry.md specs/project/components/review_pipeline.md specs/project/components/repository_and_github.md` +> If an in-scope path changed, reconcile every Current state statement against +> live code. STOP if the one-model-repair scheduler, planner fallback, +> verifier incomplete behavior, coverage aggregation, or Action status-comment +> lifecycle changed semantically. + +## Execution metadata + +- **Priority**: P1 +- **Effort**: M +- **Risk**: MED +- **Depends on**: + `specs/plans/106-issue-106-verifier-revision-payload-contract.md` + (COMPLETE). The shared Plan-95 submit/retry seam is already implemented at + the planned SHA; no pending Plan-95 work blocks this plan. +- **Category**: bug / resilience / diagnostics +- **Planned at**: commit `ae1bb70`, 2026-08-05 + +## Why this matters + +The GitHub Action failed after a Stage-5 planner submit and its one repair +were both schema-invalid. Stage 5 set `failAfterRepair: true`, so the runner +made the error fatal before the planner's documented deterministic fallback +could run. The Action then uploaded no report or diagnostic artifact, leaving +only a generic `llm_schema_invalid` line and no safe indication of which +schema rule failed. + +The successful owner evals exposed a separate measured cost. Four otherwise +complete Stage-9 verdicts carried `reason` strings of 2,100-2,984 characters +against a 2,000-character schema maximum. All four repairs succeeded, but +every repair was an avoidable paid call and another stochastic failure point. +Run 57 also established that the provider-safe flat verdict schema can accept +`revise` without `finalFinding` or `revisedAnchor`; that semantic omission +currently becomes incomplete without using the available repair attempt. + +This plan fixes those observed paths and makes degraded/incomplete output +truthful. It does not implement a general final-JSON parser. Final argument +provenance and loss-suspected input are handled separately by Issue 112 so +this production fix is not blocked on an upstream Pi release. + +## Current state + +- `src/pipeline/planner.ts:319-336` sets `failAfterRepair: true` even though + `runPlanner` and `runChunkedPlanner` already catch recoverable failures and + build deterministic default coverage. The project architecture explicitly + promises planner degrade-and-disclose behavior. +- `src/llm/pi-runner.ts:259-275` contains Plan 95's single model-repair + scheduler. `queueSchemaRepair` enforces the one-repair budget; this plan must + use it rather than creating another retry loop. +- `src/llm/pi-runner.ts:2275-2279` throws the terminal schema error with + `recoverable` controlled by `failAfterRepair`. Its current context includes + a truncated validator message which can itself contain the model's complete + received arguments; that string is not safe for a public artifact. +- Pi's validator formats safe path/rule lines before a blank-line delimiter + and then appends `Received arguments:` plus the raw payload. Safe diagnostics + must be reconstructed through an allowlist and must never retain the raw + suffix or arbitrary validator prose. +- `src/llm/schemas.ts:250-268` caps verifier `reason` at 2,000 characters; + `SCHEMA_VERSIONS.submit_verdict` is 3. The Stage-9 prompt version is `p9.8`. +- `src/pipeline/verifier.ts:689-727` canonicalizes keep-with-payload and turns + empty revise into an incomplete result only after `runStructured` returns. + `classifyVerifierSchemaInvalid` already recognizes + `revise_without_revision_payload`, but schema-valid empty revise cannot + currently enter the real repair scheduler. +- `src/llm/pi-runner.ts:2841-2867` computes cache schema validity from submit + discipline plus TypeBox validation. A new semantic validation hook must also + participate there so an empty revise is not cached as valid. +- `src/output/markdown-renderer.ts:4-18` renders coverage, but appends + “No credible findings were found. Everything looks good.” whenever + `noFindings` is true, including partial reviews. +- `src/util/coverage-summary.ts` already owns complete/partial/degraded + coverage wording. `RunCoverageStatus` is the existing single source of + truth and must remain so. +- `src/github-action/entrypoint.ts:194-209` finalizes a generic failure comment + and rethrows. It writes no report/failure files in that branch. +- `action.yml` and `.github/workflows/codegenie-review.yml` upload only + `${runner.temp}/codegenie-report.md`; the file does not exist on early + review failure. +- Telemetry is disabled by default. Failure diagnostics therefore must use + runner-temp paths and cannot depend on a telemetry run directory. + +## Trust and publication policy + +Content-stage invalidity remains binary: never use known-invalid model data. +The stage may continue only from independently trusted work or an existing +deterministic fallback. + +| Terminal condition after the one repair | Trusted state that remains | Required disposition | +| --- | --- | --- | +| Stage-5 planner invalid | Parsed diff/hunk inventory and deterministic default coverage | Discard every planner field; continue with `degradedPlanning: true`. | +| Stage-7 packet invalid | Successful packet results; failed packet hunks are known gaps | Discard the failed packet; mark its hunks incomplete/partial. | +| Stage-9 verdict invalid or semantic empty revise | Other completed verdicts; this candidate is unverified | Suppress the candidate and increment verification-incomplete coverage. | +| Stage-10 composition invalid | Already verified findings | Use existing deterministic composition; do not drop verified findings. | +| Diff/base/repository identity, authentication, provider-wide availability, or another non-isolatable foundation is untrusted | No trustworthy review foundation | Fail the run and leave the scrubbed failure artifact. | + +Run-level publication follows existing coverage truth: + +- complete and normal: publish normally; +- complete after planner fallback: publish with a prominent degraded banner; +- partial with verified findings: publish only those findings with a prominent + incomplete banner and the existing coverage disclosure; +- partial with no findings: publish an explicit “Review Incomplete” conclusion + and never say “Everything looks good” or otherwise grant a clean bill of + health; +- foundational failure: do not render a normal review. + +Do not add a numeric reviewed-hunk threshold, a second coverage ledger, or a +new partial-run exit-code policy here. A percentage would not express hunk +criticality and is not supported by current evidence. The repository's +documented contract is that partial reviews exit 0 and say they are partial; +changing that product policy requires a separate measured plan. + +## Commands you will need + +| Purpose | Command | Expected on success | +| --- | --- | --- | +| Install | `pnpm install --frozen-lockfile` | exit 0 | +| Runner/schema tests | `pnpm exec vitest run tests/phase4-llm.test.ts tests/shared-utils.test.ts` | all selected tests pass | +| Pipeline/output tests | `pnpm exec vitest run tests/pipeline-phase5.test.ts` | all selected tests pass | +| Action tests | `pnpm exec vitest run tests/github-action.test.ts` | all selected tests pass | +| Checks | `pnpm run check` | exit 0 | +| Full tests | `pnpm test` | all tests pass | +| Build | `pnpm build` | exit 0 | +| Diff hygiene | `git diff --check` | no output | + +## Scope + +**In scope**: + +- `src/llm/schemas.ts`, `src/skills/prompt-builder.ts`, + `src/pipeline/verifier.ts` — verifier reason target/buffer and semantic + empty-revise repair. +- `src/llm/llm-runner.ts`, `src/llm/pi-runner.ts`, and one small new + `src/llm/schema-diagnostics.ts` — pure semantic validation hook plus a + safe-by-construction schema-failure identity. +- `src/pipeline/planner.ts` — restore documented deterministic fallback after + terminal planner schema failure. +- `src/util/coverage-summary.ts`, `src/output/markdown-renderer.ts`, and + `src/pipeline/composer.ts` only as needed for one shared prominent + degraded/incomplete banner and truthful partial no-findings output. +- `src/github-action/entrypoint.ts`, `src/github-action/render.ts`, `action.yml`, + `.github/workflows/codegenie-review.yml` — always-available scrubbed failure + JSON/Markdown and bounded public failure identity. +- Focused tests in `tests/phase4-llm.test.ts`, + `tests/pipeline-phase5.test.ts`, `tests/github-action.test.ts`, and the + existing why-ledger guard in `tests/shared-utils.test.ts`. +- Contract updates in `specs/project/architecture.md`, + `specs/project/components/skills_llm_telemetry.md`, + `specs/project/components/review_pipeline.md`, and + `specs/project/components/repository_and_github.md`. + +**Out of scope**: + +- Any Pi dependency change, final raw-argument parser, `partial-json` change, + JSONC scanner, `jsonrepair`, JSON5, or new syntax-repair allowlist. +- More than one model repair, provider-specific Codegenie parsing, raw model + argument persistence, or a second LLM runtime. +- Deterministic truncation of verifier reasons. Reasons from 2,001 through + 4,000 are preserved unchanged; reasons above 4,000 use model repair. +- Duplicate-submit policy changes, generic structured repair-prompt redesign, + broad recovery telemetry taxonomy, or model-call-cache version changes + unrelated to semantic empty-revise validity. +- New candidate publication/confidence policy, Stage-7 cleanup behavior, + composer fallback behavior, eval expectation changes, numeric publication + thresholds, or partial-run exit-code changes. + +## Git workflow + +- Branch: `fix/observed-structured-submit-resilience` +- Keep reason-buffer/semantic repair, planner degradation/output truth, and + Action diagnostics in reviewable logical commits. +- Suggested commit subject: + `fix(llm): degrade observed submit failures safely` +- Do not push or open a PR unless the operator asks. + +## Steps + +### Step 1: Give Stage-9 reason text a soft target and hard buffer + +1. Define one pair of shared constants, preferably beside the verifier schema: + `VERIFIER_REASON_TARGET_CHARS = 2_000` and + `VERIFIER_REASON_HARD_MAX_CHARS = 4_000`. If importing them from the schema + creates a cycle, use one tiny limits module. Do not repeat the numeric + literals across schema, prompt, and verifier code. +2. Keep 2,000 as the model-facing target in the primary and stateless repair + prompts. Say the reason should be concise and at most 2,000 characters. +3. Set `SubmitVerificationVerdictSchema.reason.maxLength` to 4,000. Bump + `SCHEMA_VERSIONS.submit_verdict` from 3 to 4, Stage 9 from `p9.8` to `p9.9`, + and update the why ledger. +4. Accept reasons from 2,001 through 4,000 unchanged and without repair. Emit + one bounded `verification_reason_target_exceeded` event containing only + candidate id, actual length, target, hard maximum, and whether the accepted + submit followed a model repair. +5. A reason of 4,001 or more remains schema-invalid and consumes the existing + single model repair. Do not add a deterministic truncator. + +Pin exact historical lengths 2,327, 2,984, 2,102, and 2,285 as primary-call +regressions. Also pin 2,000/2,001/4,000/4,001 boundaries and verify the +submitted reason text is byte-identical after acceptance. + +**Verify**: +`pnpm exec vitest run tests/phase4-llm.test.ts tests/pipeline-phase5.test.ts tests/shared-utils.test.ts` +passes; schema/prompt versions are 4/`p9.9`. + +### Step 2: Route semantic empty revise through the real one-repair scheduler + +Add one optional, pure post-schema validation hook to +`LlmStructuredRequest`, preferably +`validateSubmit?(value: T): LlmSubmitSemanticValidation`. Use a discriminated +result such as: + +```ts +type LlmSubmitSemanticValidation = + | { ok: true } + | { + ok: false; + classification: LlmSubmitSemanticFailureKind; + }; + +type LlmSubmitSemanticFailureKind = "revise_without_revision_payload"; +``` + +The hook receives only the adapter-validated submit value. It cannot call a +provider, mutate the value, or synthesize fields. + +1. Invoke the hook after every successful submit-schema validation, including + the primary response, a value returned by `tryRecoverInvalidSubmit`, and the + model-repair response. Prefer one internal + schema-then-semantics helper so deterministic recovery cannot return early + with a semantically invalid value. A semantic failure after deterministic + recovery declines that recovery and proceeds to the same one model repair. + A semantic failure after model repair follows the stage's existing terminal + disposition; never dispatch a second repair. +2. Carry its enum-bounded classification through `LlmSchemaRepairInput` and + the existing scheduler. Define one central allowlist type for safe terminal + classifications, including the existing Stage-7 kinds and an `unknown` + fallback; never publish an arbitrary hook/classifier string. Remove the + scheduler's internal Stage-7-only cast if necessary, but preserve the + Stage-7 classifier union and every existing Stage-7 event value at the + Stage-7 boundary. +3. Include the hook in `schemaValidityForResponse` so a schema-valid but + semantically invalid submit is not cached as valid. +4. In `runVerifierStructured`, reject only `verdict === "revise"` with neither + `finalFinding` nor `revisedAnchor`, using classification + `revise_without_revision_payload`. Either field alone is a valid revision. + Keep-with-payload remains Plan 106's canonicalized revise and is not a + semantic failure. +5. Preserve the current incomplete result and telemetry when repair still + fails or cannot dispatch because budget is exhausted. + +Test live runner behavior, not only direct prompt helpers: primary empty revise +dispatches exactly one stateless repair; a deterministic normalizer returning +an empty revise is not accepted and uses that same repair; a valid repaired +revision succeeds; another empty revise becomes incomplete; cache eligibility +is false for the primary invalid response; keep-with-payload behavior remains +unchanged. + +**Verify**: +`pnpm exec vitest run tests/phase4-llm.test.ts tests/pipeline-phase5.test.ts` +passes. + +### Step 3: Make terminal planner schema failure degrade, not abort + +In `src/pipeline/planner.ts`, set planner post-repair schema failure to +recoverable (`failAfterRepair: false`, or remove the override if false is the +runner default). Only terminal planner `llm_schema_invalid` after the existing +one repair may use this fallback. Do not catch authentication errors, +permission failures, provider exhaustion/availability failures, configuration +or programming errors, or another fatal class. + +Pin the complete path: + +- invalid primary planner submit -> exactly one repair; +- invalid repaired submit -> `llm_schema_invalid` with `recoverable: true`; +- `runPlanner` discards every submitted planner field, writes the existing + deterministic default `review-plan.json`, and returns + `degradedPlanning: true`; +- Stage 6 and later stages consume only deterministic default coverage; +- chunked planning falls back only the failed chunk and preserves successful + chunks; +- auth/provider-wide/fatal-class failures remain fatal; +- no second repair is dispatched. + +Add an Action-level fixture equivalent to the observed incident: two invalid +planner submits result in a completed degraded review, not a failed job. Do +not encode the GitHub run id or repository-specific payload. + +**Verify**: +`pnpm exec vitest run tests/pipeline-phase5.test.ts tests/github-action.test.ts` +passes. + +### Step 4: Make degraded and incomplete reviews prominent and truthful + +Use `RunCoverageStatus` as the only coverage/trust ledger. + +1. Add one small shared renderer in `src/util/coverage-summary.ts` for the + prominent trust banner: + - partial takes precedence and says the review is incomplete; when planning + was also degraded, include the planner-fallback fact in the same banner; + - otherwise degraded planning says the planner failed and deterministic + default coverage was used, and recommends rerunning; + - normal complete runs return no banner. +2. Render the banner immediately below the Codegenie heading and before the + model-authored summary in both the full Markdown/status comment and the + GitHub review body. Keep detailed counts/reasons in the existing coverage + section; do not duplicate them in the banner. +3. Split the unconditional `noFindings` section: + - complete: preserve today's heading and sentence byte-for-byte; + - partial: use “Review Incomplete” and say that completed work produced no + credible verified findings, but incomplete coverage/verification prevents + a clean conclusion. Never include “Everything looks good.” +4. Keep independently verified findings publishable on partial runs with the + same prominent incomplete banner. Do not demote or suppress them because an + unrelated packet/candidate failed. +5. Planner fallback alone does not set `coverage.partial` when all downstream + work completes. Zero-work filtered/empty diffs remain complete reviews of + nothing. + +Add regressions for normal complete, planner-degraded complete, partial with a +finding, partial no-findings, all Stage-7 packets failed, Stage-9 incomplete +with no published finding, combined degraded+partial, and foundational Action +failure. Assert banner placement before the summary and that no partial path +contains an approval-equivalent sentence. + +**Verify**: +`pnpm exec vitest run tests/pipeline-phase5.test.ts tests/github-action.test.ts` +passes. + +### Step 5: Preserve a safe schema-failure identity and always write failure artifacts + +Create one pure `src/llm/schema-diagnostics.ts` helper and one bounded type for +failure identity. Suggested shape: + +```ts +type StructuredSubmitFailureDiagnostic = { + schemaVersion: 1; + stage: ReviewStage; + role: LlmRole; + submitTool: string; + submitSchemaVersion: number; + attempt: "primary" | "repair"; + classification: StructuredSubmitFailureClassification; + issues: Array<{ + path: string; + rule: "required" | "additionalProperties" | "type" | + "minLength" | "maxLength" | "minItems" | "maxItems" | + "enum" | "const" | "semantic" | "schema"; + expectedLimit?: number; + }>; +}; +``` + +Define the diagnostic classification as an explicit union, not `string`, for +example the generic safe kinds (`schema_invalid`, `missing_submit`, +`multiple_submits`, `revise_without_revision_payload`, `unknown`) plus the +existing `Stage7SchemaInvalidKind` union. Map anything outside that set to +`unknown` at the runner boundary and cap serialized classification labels at +64 characters defensively. + +Names may match local style, but these safety properties are mandatory: + +1. Build the record at the runner boundary while stage, role, tool, schema + version, attempt, and validation failure are available. +2. For Pi's current thrown validator format, discard everything beginning at + its blank-line `Received arguments:` delimiter before inspecting the safe + prefix. Accept only bounded path/rule patterns from an allowlist; normalize + an unknown/malformed path to `root` and an unknown rule to `schema`. +3. Derive a safe property-name allowlist by walking the request's static + TypeBox schema, including union/object branches and array items. A retained + path may contain only names from that schema-derived set plus numeric array + indexes and is capped at 200 characters; regex shape alone is insufficient. + If any segment cannot be proven schema-owned, normalize the whole path to + `root`. Store at most 12 issues. Store numeric schema limits only when an + allowlisted rule exposes them. +4. Never store raw field values, arbitrary validation prose, prompts, model + output, arguments, repository snippets, diffs, tool results, stack traces, + or hashes that could be used as a surrogate payload. +5. Missing/extra submit discipline and semantic empty revise use the same + record with bounded classifications; they do not invent schema issues. +6. Replace the terminal schema error's raw validation-message context with + this safe diagnostic. Do not attach the original Pi validation exception as + `cause`: it contains the complete `Received arguments` payload. If a cause + is required for internal error chaining, replace it with a new value-free + bounded error. Repair prompts may continue receiving the original bounded + error before the terminal throw; public errors/artifacts may not retain it. + +Then make the Action failure path always leave evidence: + +1. Add `CODEGENIE_FAILURE_PATH`, set to + `${runner.temp}/codegenie-failure.json` in both Action surfaces. +2. In the Action catch path, project the thrown error through an explicit + allowlist. Write a schema-versioned JSON record containing Action lane/PR, + run URL/id when available, error code, and the structured diagnostic when + present. Never serialize arbitrary `error.context` or `error.message`. +3. Cap the JSON artifact at 16 KiB. Write a separate Markdown report capped at + 4 KiB to `CODEGENIE_REPORT_PATH` on failure. It may show error code, + stage/tool/attempt/classification, and safe issue path/rule/limit fields, + then point to the JSON artifact. +4. Pass only the same bounded identity to the status comment and CI log. A + useful form is “Stage 5 `submit_plan` repair remained schema-invalid + (`coverage: required`)”; do not print the validator exception. +5. Upload report and failure JSON with `if: always()`. Success behavior remains + unchanged; the failure JSON may be absent on success without warning. +6. Any file-write problem must not replace the original review error. + +Tests must use the real default `runDir: ""` path. Seed a fake secret and a +repository-like snippet in received arguments and exception text; assert that +neither appears in JSON, Markdown, lifecycle log, status comment, or step +output. Also test malformed validator prose, more than 12 issues, unsafe paths, +unknown errors without structured diagnostics, successful reviews, artifact +size caps, and unwritable artifact paths. Recursively inspect the thrown +error's context/cause, JSON serialization, stack/log rendering, and artifacts +to prove the seeded values are absent. + +**Verify**: +`pnpm exec vitest run tests/phase4-llm.test.ts tests/github-action.test.ts` +passes. + +### Step 6: Document the contracts and run full validation + +Update the architecture/component specs with these exact invariants: + +- observed Stage-9 verbosity uses a 2,000 target and 4,000 hard schema limit; +- semantic validation shares the one model-repair budget and cache-validity + decision; +- planner terminal schema failure discards model data and uses deterministic + fallback; +- complete, degraded, partial, and failed output states are visibly distinct; +- partial reviews never grant a clean bill of health; +- public failure diagnostics are safe-by-construction allowlisted fields and + exist even when telemetry is disabled; +- final raw-argument parse provenance is deferred to Issue 112 and is not + approximated by serializing Pi's already-parsed arguments. + +Lift the complete “Trust and publication policy” matrix from this plan into +`specs/project/architecture.md`, preserving the distinction between terminal +condition, independently trusted state, and required disposition. Supporting +component specs may summarize the rows relevant to their component, but the +architecture document must retain the full lookup table rather than replacing +it with general degrade-or-fail prose. + +Run every command in Commands you will need. Inspect `git status --short` and +the complete diff. The final implementation should contain one semantic hook, +one schema-diagnostic helper, one trust-banner renderer, the existing one +repair scheduler, and the existing coverage ledger. + +**Verify**: `pnpm run check`, `pnpm test`, `pnpm build`, and +`git diff --check` all succeed. Only Scope paths plus the plan status row are +changed. + +### Step 7: Run provider and owner validation + +1. Re-run the PR-19 Action scenario (or an equivalent configured-provider PR) + and prove a twice-invalid planner submission completes through deterministic + degraded planning with a prominent header. +2. Force a separate safe fixture failure and download the Action artifact. + Confirm the report and JSON identify stage/tool/classification without raw + arguments or repository content. +3. Run no-cache owner evals for `49f4645b`, `0c4d5213`, and `relay-wc` under + `/home/peter/Dev/0xPolygon/codegenie-private-evals/trails-api`. +4. Compare against the accepted baselines: + - the exact four 2.1k-3.0k verifier reasons do not schedule repairs; + - `revise_without_revision_payload` receives at most one real repair and + remains incomplete if unrepaired; + - required findings, `should_not_find` guards, rendered-note guards, and + publication behavior remain unchanged; + - no raw failure payload appears in logs or artifacts. + +Record run ids and summarized schema-repair counts in this plan before marking +it COMPLETE. Do not weaken eval expectations to pass the gate. Revert or +correct the responsible slice if any fatal class is swallowed, partial output +contains approval wording, a second repair occurs, or an owner eval regresses. + +**Verify**: Action/provider smoke and all three owner cases satisfy every +listed invariant. + +### Implementation evidence (2026-08-05) + +- Focused runner/schema, pipeline/output, verifier, telemetry, and Action + regressions pass. `pnpm install --frozen-lockfile`, `pnpm run check`, + `pnpm build`, and `git diff --check` pass. +- Boundary tests pin the 2,000/2,001/4,000/4,001 reason lengths and all four + historical 2,102/2,285/2,327/2,984 lengths. Values through 4,000 are + preserved unchanged; 4,001 still enters the existing one repair. +- Integration tests exercise semantic empty-revise on primary, repair, + deterministic-recovery, and cache-validity paths; twice-invalid planner + fallback; degraded/partial banners; truthful partial no-findings; and + telemetry-disabled Action failure files with seeded secret/repository text, + malformed diagnostics, issue caps, unknown errors, and unwritable paths. +- The direct Anthropic Messages and OpenAI Codex Responses configured-provider + smokes each accepted a strict schema-valid submit without model repair. +- Owner eval `49f4645b` run 63 passed 1/1 required expectations with complete + coverage and no verification/composition loss. Its Stage-5 primary submit + was schema-invalid and recovered through exactly one repair; the required + finding remained published. No raw event value crossed the new bounded + provenance telemetry. +- Owner eval `0c4d5213` run 71 passed all 4/4 required expectations, the + required candidate expectation, and the `should_not_find` guard, with six + published findings, complete coverage, and no stage loss. A 2,031-character + verifier reason was accepted unchanged on the primary call and emitted only + `verification_reason_target_exceeded`. Its three invalid submits all + recovered through the existing bounded paths: one Stage-7 schema repair, + one Stage-9 final-argument repair, and one deterministic Stage-10 recovery. + +The remaining dogfood gate is recording the `relay-wc` owner suite and a live +Action-path observation. The implementation is complete, but this plan stays +`IMPLEMENTED (dogfood pending)` until that external validation record is +closed rather than claiming `COMPLETE` early. + +## Test plan summary + +- `tests/phase4-llm.test.ts`: semantic hook on primary/repair/cache paths, + reason schema boundaries, bounded safe diagnostic extraction. +- `tests/pipeline-phase5.test.ts`: historical reason lengths, empty-revise + repair, planner fallback, prominent output banners, partial no-findings. +- `tests/github-action.test.ts`: twice-invalid planner success, no-telemetry + failure artifacts, safe public diagnostics, secret/repository-text absence. +- `tests/shared-utils.test.ts`: prompt version/why-ledger consistency. +- Full check/test/build plus configured-provider and owner-eval gates. + +## Done criteria + +- [x] The four historical 2,100-2,984-character reasons pass on the primary + call unchanged; 2,001-4,000 emits target-exceeded telemetry and 4,001+ + uses exactly one model repair. +- [x] Schema-valid empty revise enters the real one-repair path, is not cached + as valid, and remains incomplete if repair cannot produce a payload. +- [x] Twice-invalid planner output is discarded and deterministic planning + continues; auth/provider-wide/fatal-class failures remain fatal. +- [x] Complete degraded and partial reviews show a prominent pre-summary + banner; partial no-findings never says “Everything looks good.” +- [x] One safe schema-diagnostic helper records only allowlisted bounded + metadata and terminal schema errors no longer expose raw validation + arguments through their context. +- [x] Action failure always leaves scrubbed Markdown (at most 4 KiB) and JSON + (at most 16 KiB), including with telemetry disabled and no run attachment. +- [x] Existing one-repair, stage-local degradation, cache, finding publication, + and partial-exit-code contracts remain intact. +- [x] `pnpm run check`, `pnpm test`, `pnpm build`, and `git diff --check` pass. +- [ ] Configured-provider and all three owner-eval gates pass without + expectation weakening. +- [x] Architecture/component specs and the README status row are updated. + +## STOP conditions + +Stop and report; do not improvise if any occurs: + +- The current runner no longer has exactly one model-repair scheduler or the + semantic hook would require a second provider call path. +- Making empty revise repairable requires restoring a provider-rejected root + union or synthesizing a revision payload. +- Planner fallback would consume any field from the invalid planner response + rather than only deterministic diff/hunk inputs. +- Safe failure diagnostics require serializing arbitrary error context, raw + arguments, arbitrary validator messages, prompts, diffs, repository text, + tool results, or hashes of those values. +- Pi's validator format no longer has a provable safe delimiter before raw + arguments. Fall back to pathless classification; do not parse or persist an + uncertain message. +- Prominent disclosure requires a second completeness calculation rather than + `RunCoverageStatus`. +- The provider rejects the flat Stage-9 schema after the reason limit changes, + a cross-case eval regresses, or focused tests fail twice after a reasonable + correction. + +## Maintenance notes + +- The 2,000-character reason value is a prompt target; 4,000 is the hard + schema maximum. Revisit either only with measured accepted-length data. +- Safe diagnostics are intentionally less detailed than raw validator errors. + Add a new field only when it can be proven value-free and bounded. +- Degraded and partial are independent axes: deterministic planner fallback + can still achieve full hunk coverage, while a normal planner can still be + followed by partial packet/verification work. +- Do not grow this plan into final JSON parsing. Issue 112 owns that boundary + and its evidence gate. diff --git a/specs/plans/112-issue-112-final-tool-argument-provenance.md b/specs/plans/112-issue-112-final-tool-argument-provenance.md new file mode 100644 index 0000000..82fe38d --- /dev/null +++ b/specs/plans/112-issue-112-final-tool-argument-provenance.md @@ -0,0 +1,709 @@ +# Issue 112: Enforce Final Structured-Submit Provenance from Pi Events + +Status: IMPLEMENTED (measuring) +Planned from: audit of `@earendil-works/pi-ai` 0.83.0 final tool-call parsing +and its public `AssistantMessageEventStream`, 2026-08-05 +Planned at: commit `ae1bb70` (branch `llm-repair`; tree-identical to the +original planning SHA `07434ba` on the retired `plans` branch) +Recommended priority: after Issue 111. This establishes a fail-closed final +argument boundary using Pi's existing public stream, then measures whether any +larger loss-aware syntax repair or upstream Pi change is justified. + +> **Executor instructions**: Follow this plan step by step. Read the entire +> plan before changing code. This is a Codegenie-side implementation against +> Pi's public event API; it does not require a Pi fork, patch, PR, or release. +> Do not implement any item in “Conditional future design.” Mark the plan +> `IMPLEMENTED (measuring)` after the implementation gate passes, +> then collect the corpus in normal use and mark it `COMPLETE` only after +> Step 5 records the human-reviewed outcome. Stop on any STOP condition rather +> than accepting missing/divergent event provenance or adding provider-specific +> wire parsing. Update the README row at both status transitions. +> +> **Drift check (run first)**: +> `git diff --stat ae1bb70..HEAD -- src/llm/llm-runner.ts src/llm/pi-runner.ts src/llm/final-tool-arguments.ts src/llm/model-call-cache.ts src/llm/schema-diagnostics.ts src/telemetry/telemetry-recorder.ts src/telemetry/run-artifacts.ts tests/final-tool-arguments.test.ts tests/phase4-llm.test.ts tests/telemetry.test.ts specs/project/architecture.md specs/project/components/skills_llm_telemetry.md specs/project/components/review_pipeline.md` +> Reconcile changed paths against Current state. STOP if Issue 111 has not +> landed, Pi's public stream/event shape no longer matches Current state, or +> the one-repair/cache boundaries changed semantically. + +## Execution metadata + +- **Priority**: P2 +- **Effort**: M (one event-capture helper, the production adapter switch, + runner/cache/telemetry wiring, generic event-shape tests, and two live + provider smokes; the post-land corpus is time rather than implementation) +- **Risk**: HIGH (provider boundary and cache trust, mitigated by strict parse, + equality, fail-closed routing, and no provider wire parsing) +- **Depends on**: + `specs/plans/111-issue-111-observed-structured-submit-resilience.md` +- **Category**: correctness / observability +- **Planned at**: commit `ae1bb70`, 2026-08-05 + +## Why this matters + +Pi 0.83.0 uses `parseStreamingJson` for streaming previews and finalized tool +calls. After strict JSON and Pi's narrow string repair fail, that function uses +`partial-json`, which intentionally returns a usable object from incomplete +input. A schema-valid partial prefix can therefore look complete after Pi +deletes its internal scratch buffer. + +Codegenie currently calls `Models.complete()` / `completeSimple()` and receives +only that finalized object. Pi's public implementation, however, defines both +completion methods as `stream(...).result()`. The same public stream emits +`toolcall_start`, string `toolcall_delta`, `toolcall_end`, and terminal +`done`/`error` events. Codegenie can consume that stream, retain argument +fragments only in memory for the duration of the call, strictly parse the +assembled representation, and compare it with Pi's final arguments before any +submit schema/semantic validation. + +No confirmed Codegenie incident has been attributed to partial final parsing. +That evidence level supports a small permanent fail-closed boundary plus +measurement, not a general self-repair parser or an externally coordinated Pi +change. Untrusted final values use the existing one model repair for submit +calls and are never accepted merely because Pi's partial object passes TypeBox. + +## Research decision + +| Candidate | Evidence | Decision in this plan | +| --- | --- | --- | +| Pi public `stream()` / `streamSimple()` | `Models.complete()` is exactly `stream(...).result()` and the public event union exposes tool-call framing and string deltas. | Use this Codegenie-side boundary first; preserve the same provider request/options and terminal result semantics. | +| Strict parse plus deep equality | Proves the complete event representation parses as one object and is semantically identical to Pi's finalized arguments. | Required for acceptance. Use Codegenie's parsed value as authoritative after equality succeeds. | +| Pi public `repairJson` | Exported from the package root; escapes raw control characters and invalid backslashes without completing delimiters or values. | Use only after strict parse fails, then run strict `JSON.parse` again and require equality. Record one bounded `pi_narrow_string_repair` kind. | +| Pi `parseStreamingJson` / `partial-json` | Intentionally yields partial values for presentation and is used internally by finalizers. | Never call or accept its output as final provenance in Codegenie. Pi may continue using it for preview state. | +| Provider-adapter fixture matrix | Pi adapters do not all construct deltas identically, but the Codegenie boundary has runtime parse/equality checks. | Do not duplicate every Pi adapter. Test generic event shapes plus the two configured provider smokes; add a targeted adapter fixture only after telemetry or a smoke exposes a real gap. | +| Upstream Pi change | Native provenance remains the ideal shared end state, but no incident or event-fidelity failure currently requires external coordination. | Deferred. Use measurements to support a later additive upstream proposal; delete the app-side accumulator only after equivalent native behavior is released and verified. | +| `jsonrepair`, JSONC, JSON5, streaming completion parsers | Can add closing delimiters or otherwise make truncated data look complete. | Do not add. They can mask semantic loss. | + +The ownership boundary for this plan is: + +- Pi owns provider wire formats, authentication, normalized public events, and + its streaming preview objects. +- Codegenie's production Pi adapter owns ephemeral event accumulation, strict + final parsing, equality against Pi's final value, and the local trusted / + non-executable submit-call distinction. +- The runner owns submit discipline, TypeBox, stage semantics, the one repair, + caching, and stage-local disposition. +- Neither layer persists the assembled event text. + +## Important event-contract limits + +`toolcall_delta.delta` is a normalized public representation, not universally +raw wire text: + +- Anthropic Messages, Bedrock, and ordinary OpenAI argument streams forward + JSON fragments. +- Google/Vertex receive structured argument objects and emit + `JSON.stringify(arguments)` as one delta. +- Mistral forwards a string when supplied and otherwise stringifies a + structured argument object. +- OpenAI Responses can replace its internal accumulated buffer with a final + `arguments` value; when that value is not a suffix, the replacement is not + emitted as another public delta. +- Grammar/custom-tool paths may emit a synthesized JSON representation of the + provider's string input. + +Therefore the accepted state means “the complete public event representation +strictly parses and equals Pi's final arguments,” not “Codegenie observed raw +provider bytes.” Missing or divergent events fail closed. A normal provider +that emits a non-suffix final replacement may incur one repair until Pi exposes +that replacement publicly; telemetry will make that concrete. + +Pi's normalized `message.stopReason === "length"` remains an independent +message-level rejection and takes precedence. Do not reproduce mappings from +provider-specific `rawStopReason` strings in Codegenie. Pi 0.83.0's Google APIs +can overwrite a mapped length result with `toolUse` when a tool call exists; +this known upstream stop-reason limitation is not repaired by interpreting raw +Google values here. The strict/equality boundary still protects malformed +argument syntax, but this plan claims length-stop enforcement only when Pi's +public normalized stop reason is `length`. If a required provider smoke exposes +incorrect normalized length behavior, STOP and report the narrow Pi issue. + +## Current state + +- `package.json` depends on `@earendil-works/pi-ai ^0.83.0`; no dependency + change is required by this plan. +- Pi's public `Models.stream()` / `streamSimple()` apply the same auth and + provider options as completion. `complete()` and `completeSimple()` only + await the corresponding stream's `result()`. +- Pi's public event union exposes `toolcall_start`, `toolcall_delta` with a + string `delta`, `toolcall_end`, and terminal `done`/`error` events. +- Pi publicly exports `repairJson`; Codegenie need not copy or fork its narrow + repair algorithm. +- `src/llm/pi-runner.ts:createRealPiAiAdapter` currently delegates to + `models.complete` / `completeSimple`. The rest of the runner sees one + `Promise`, so event consumption can remain encapsulated + inside the production adapter. +- `src/llm/llm-runner.ts:PiToolCall` currently has mandatory `arguments` and no + provenance or non-executable variant. +- `src/llm/pi-runner.ts` already owns submit selection, repository-tool + execution, Plan 95's one model-repair scheduler, cache eligibility, and + Issue 111's stage-local terminal behavior. +- `schemaValidityForResponse` currently considers submit discipline plus + schema/semantic validity; final provenance must become an earlier validity + and cache gate. +- `MODEL_CALL_CACHE_SCHEMA_VERSION` is 1. Historical cache entries contain no + event provenance and cannot be trusted after this boundary lands. +- Pi's validation layer separately clones and coerces argument values before + TypeBox validation. That semantic-transformation channel is distinct from + final event completeness and remains out of scope. + +## Commands you will need + +| Purpose | Command | Expected on success | +| --- | --- | --- | +| Install | `pnpm install --frozen-lockfile` | exit 0 | +| Event helper tests | `pnpm exec vitest run tests/final-tool-arguments.test.ts` | all selected tests pass | +| Runner tests | `pnpm exec vitest run tests/phase4-llm.test.ts` | all selected tests pass | +| Telemetry tests | `pnpm exec vitest run tests/telemetry.test.ts` | all selected tests pass | +| Checks | `pnpm run check` | exit 0 | +| Full tests | `pnpm test` | all tests pass | +| Build | `pnpm build` | exit 0 | +| Diff hygiene | `git diff --check` | no output | + +## Scope + +**In scope**: + +- One new `src/llm/final-tool-arguments.ts` helper for public-event + accumulation, strict/narrow parsing, deep equality, bounded provenance, and + ephemeral-buffer cleanup. +- `src/llm/llm-runner.ts` and `src/llm/pi-runner.ts` — local trusted/untrusted + call types, production stream consumption, fail-closed submit routing, + and existing one-repair integration. +- `src/llm/model-call-cache.ts` — cache schema bump so provenance-less entries + cannot bypass the boundary. +- Existing telemetry/model-call/run-artifact types only as needed for bounded + state counts and repair outcomes. +- Issue 111's `src/llm/schema-diagnostics.ts` closed classification allowlist, + only to admit this plan's five bounded provenance failure classifications. +- New focused `tests/final-tool-arguments.test.ts`, existing + `tests/phase4-llm.test.ts`, `tests/telemetry.test.ts`, and architecture / + LLM-telemetry / review-pipeline specs. +- Two configured-provider smokes: direct Anthropic Messages and one + OpenAI-compatible Responses provider. +- Owner measurement and the written evidence-gate result in this plan. + +**Out of scope**: + +- Any Pi source/dependency change, upstream PR, fork, `pnpm patch`, workspace + link, tarball, or provider-wire interception. +- Enforcing final provenance on read-only repository investigation tools. They + have different provider-history and retry semantics, no incident evidence, + and do not directly become the published review. The event helper may observe + their framing transiently, but this plan transforms/gates only named stage + submit calls and preserves today's repository-tool loop unchanged. +- A fixture or live-test matrix for every Pi provider/model. Add another + targeted fixture only when a required smoke or bounded production telemetry + demonstrates a distinct event-contract failure. +- `jsonc-parser`, `jsonrepair`, JSON5, missing comma/colon/trailing-comma/comment + recovery, delimiter completion, scalar completion, or any new syntax repair. +- Reading private Pi scratch fields such as `partialJson` / `partialArgs`, + serializing Pi's final object as a substitute for event capture, or parsing + provider-specific `rawStopReason` values. +- Duplicate-submit policy changes, generic repair-prompt redesign, new stage + normalization, or a second provider/model repair. +- Any change to Pi's TypeBox/JSON-schema value coercion or value-bearing + coercion telemetry. +- Synthesizing/deleting findings, evidence, paths, anchors, lines, enum values, + verdicts, revision payloads, coverage entries, strings, or array members. +- Raw accumulated argument text, parser messages, prompts, model output, + repository snippets, diffs, tool results, validation values, or hashes of + those values in telemetry, caches, artifacts, errors, or logs. +- Implementing the conditional future design in this plan. + +## Git workflow + +- Branch: `fix/final-tool-argument-provenance`. +- Keep event capture/types, runner/cache enforcement, and telemetry/docs in + reviewable logical commits. +- Suggested commit subject: + `fix(llm): verify final arguments from Pi events` +- Do not modify or publish Pi, push, or open PRs unless the operator asks. + +## Steps + +### Step 1: Build one loss-aware public-event accumulator + +Create `src/llm/final-tool-arguments.ts`. It must depend only on Pi's public +event/types plus public `repairJson`, Node's deep strict equality, and small +local types. Do not import a provider adapter or private Pi path. + +Define a bounded result equivalent to: + +```ts +type FinalArgumentParse = + | { state: "strict"; value: Record } + | { + state: "repaired"; + value: Record; + repairs: ["pi_narrow_string_repair"]; + } + | { state: "length_stopped" } + | { state: "partial"; errorKind: "unexpected_end" | "unterminated" } + | { state: "invalid"; errorKind: "invalid_syntax" | "non_object_root" } + | { state: "event_capture_missing" } + | { state: "event_final_mismatch" }; +``` + +Exact names may follow repository style. States and error kinds must remain +closed unions with no raw parser text or data values. + +Implement these invariants: + +1. Track each call by public `contentIndex` between `toolcall_start` and + `toolcall_end`; retain only concatenated `toolcall_delta.delta` strings and + bounded framing state. Reconcile the completed capture with the end event's + id/name and the terminal message's call at the same content index. A missing + start, delta representation, end, or terminal call is + `event_capture_missing`; ambiguous index/id/name reuse is never guessed. + Produce provenance only for the adapter-supplied stage submit-tool name. + Other tool buffers may be held until their end event identifies the name, + but must then be discarded without parsing or changing the call. +2. Consume through the terminal `done` or `error` event and return the exact + final assistant message semantics that `.result()` would return. If the + terminal message has normalized `stopReason: "length"`, mark every captured + stage submit `length_stopped` before parsing. +3. Otherwise run strict `JSON.parse` on the accumulated representation. Do not + call `parseStreamingJson` or `partial-json`. +4. If strict parsing fails, call Pi's public `repairJson` once. Only if the + returned string differs, run strict `JSON.parse` once more. A successful + second parse is `repaired/pi_narrow_string_repair`; do not reimplement or + expand Pi's algorithm. +5. Require exactly one non-null, non-array object root. Scalars and arrays are + `invalid/non_object_root`. +6. Deep-compare the strict/repaired object with the final + `toolcall_end.toolCall.arguments`. Equality is semantic and key-order + independent. Divergence is `event_final_mismatch`; never choose either + object heuristically. When equal, use Codegenie's strict/repaired parsed + object as the authoritative arguments. +7. Normalize only clear unexpected-end/unterminated `SyntaxError` classes to + `partial`; everything else is `invalid/invalid_syntax`. Parser messages may + be inspected transiently for that bounded classification but never returned, + logged, persisted, or placed in an error context. False-invalid is safe and + may inform a later classifier plan; false-accept is not. +8. Clear all accumulated strings in `finally` after terminal conversion, + including error/abort paths. Do not add raw text to the returned message. + +Add `tests/final-tool-arguments.test.ts` with synthetic public event sequences, +not provider-adapter replicas. Cover: + +- fragmented strict object and one-delta canonical object; +- key-order-different but deeply equal final arguments; +- both narrow repair inputs through public `repairJson`; +- unterminated string/object/array, malformed complete syntax, empty input, + scalar, and array root; +- missing start/delta/end, duplicate/ambiguous framing, id/name/index mismatch, + and final semantic mismatch; +- a non-suffix final replacement represented by divergent end arguments; +- normalized length stop with otherwise valid JSON; +- terminal error/abort parity and buffer cleanup; +- seeded credential/repository-like values absent from every returned + classification and thrown error. + +These are event-shape tests. Do not add Anthropic/OpenAI/Google/Mistral fixture +copies here. + +**Verify**: +`pnpm exec vitest run tests/final-tool-arguments.test.ts` passes. + +### Step 2: Consume Pi streams inside the production adapter without changing call semantics + +Keep `PiAiAdapter.complete(...) => Promise` as the runner's +single completion boundary. Change only `createRealPiAiAdapter` internally: + +1. Extend the adapter-local completion input with an explicit + `submitToolName`, supplied by `completeWithCache` from the current stage. + Destructure it before constructing Pi's public `Context`/options; it is + Codegenie routing metadata and must never be sent to a provider. Pass it to + Step 1 so only that named call is transformed. +2. Extend `RealPiAiAdapterDeps.models` to expose `stream` and `streamSimple`. + Forced-tool-choice calls use `models.stream`; ordinary simple calls use + `models.streamSimple`, with the same prepared model, auth, mapped options, + signal, headers callback, tool choice, reasoning, session id, cache + retention, and `maxRetries: 0` currently passed to completion. +3. Replace injected `complete` / `completeSimple` test seams with public-stream + equivalents, or add a small stream fixture factory. Do not retain a + production path that returns provenance-less completion messages. +4. Pass the selected stream to Step 1's helper and return its locally typed + final message. The runner, retry loop, provider-limit wrapper, timeout, + accounting, and response-header capture remain outside the adapter exactly + where they are today. +5. Preserve `.result()` terminal semantics: the `done` event returns its + `message`; the `error` event returns its `error` message so the existing + `providerFailureFromMessage` logic performs the same retry classification. + Do not turn normal error events into a new thrown-error class inside the + adapter. +6. Do not persist or emit intermediate stream events. This is an integrity + wrapper, not a user-facing streaming feature. + +Add integration tests proving that, for the same synthetic stream: + +- the adapter returns the same final role/content/provider/model/usage/ + stopReason/error fields as `.result()` except for bounded local call + provenance and authoritative equal arguments; +- forced and simple branches choose `stream` and `streamSimple` respectively; +- only the explicit submit-tool name is transformed; repository calls remain + byte-for-byte unchanged and the private name is absent from provider input; +- request options, auth preparation, response callback, abort signal, and tool + choice are unchanged; +- provider error, abort, timeout, retry, usage, and budget behavior remain + byte-for-byte equivalent at the runner boundary; +- no extra provider request is created by event consumption. + +**Verify**: +`pnpm exec vitest run tests/final-tool-arguments.test.ts tests/phase4-llm.test.ts` +passes. + +### Step 3: Make untrusted submit arguments non-executable and reuse existing recovery + +In `src/llm/llm-runner.ts`, represent the adapter's output as a discriminated +local union: + +```ts +type PiTrustedSubmitCall = { + type: "toolCall"; + id: string; + name: string; + arguments: Record; + argumentParse: { state: "strict" | "repaired"; repairs?: string[] }; +}; + +type PiUntrustedSubmitCall = { + type: "invalidToolCall"; + id: string; + name: string; + argumentParse: Exclude; +}; +``` + +The untrusted submit variant has no `arguments`. Never manufacture `{}`. The +exact types may be factored differently, but an untrusted stage submit must be +unrepresentable as an executable/validatable call inside Codegenie. Ordinary +repository tool calls retain their current Pi type and behavior. + +Wire the runner with these rules: + +1. Existing submit-call discipline counts both trusted and untrusted named + calls by id/name, so an invalid selected submit cannot masquerade as a + missing submit. Preserve Stage 5/10 exact-cardinality and Stage 7-9 existing + first-submit/drop behavior. +2. A trusted submit proceeds to the unchanged TypeBox and Issue 111 semantic + gates. Provenance never substitutes for either validation. +3. An untrusted primary submit enters Plan 95's existing single model-repair + scheduler with one bounded classification: + `length_stopped`, `final_arguments_partial`, `final_arguments_invalid`, + `event_capture_missing`, or `event_final_mismatch`. Repair metadata may + carry id/name/state but no arguments. Do not invoke a deterministic submit + normalizer on an untrusted call. +4. An untrusted repaired submit follows Issue 111's existing stage-local + terminal disposition. Never dispatch a second repair. +5. Do not append an assistant message containing `invalidToolCall` to the Pi + provider conversation. Route an untrusted submit through the existing + one-repair seam using only bounded id/name/state metadata and the submit + schema. Issue 114 supersedes the original replace-conversation behavior: + discard the invalid assistant payload and event text, retain the independently + constructed conversation that preceded it, append the bounded repair + instruction, and perform one clean re-execution. Pi therefore never has to + serialize a Codegenie-local content variant or rejected arguments. +6. Apply the same submit provenance gate inside + `schemaValidityForResponse`. A selected untrusted/provenance-less submit is + invalid and not cacheable. +7. Bump `MODEL_CALL_CACHE_SCHEMA_VERSION`. Historical messages without + provenance become cache misses. Cached trusted messages retain only the + bounded provenance plus the already-persisted validated argument object; + untrusted messages are never written. +8. Update test helpers/fake adapters to return explicit strict provenance for + valid tool calls. Missing provenance is rejected in production and tests; + do not add a permissive compatibility capability or accepted `unknown`. + +Precedence is explicit: + +- normalized message `stopReason: "length"` wins first and makes every named + stage submit in that message untrusted; +- otherwise existing submit discipline/selection runs unchanged; +- provenance gates only the call selected by that existing policy; +- TypeBox and Issue 111 semantics run only after trusted provenance; +- repair remains capped at one. + +Tests must cover every state on primary and repaired submits, schema failure +after strict provenance, semantic failure after strict provenance, +context-preserving repair history that contains neither the invalid local call +nor its arguments, +every stage-local terminal disposition, no cache write for untrusted responses, +old-cache miss, unchanged repository-tool behavior, and preservation of +duplicate-submit behavior. + +**Verify**: `pnpm exec vitest run tests/phase4-llm.test.ts` passes. + +### Step 4: Measure bounded outcomes without storing event text + +Extend existing model-call telemetry rather than creating a parallel raw log. + +1. Record one bounded final-argument state for the submit selected by Step 3: + `strict`, `repaired`, `partial`, `invalid`, `length_stopped`, + `event_capture_missing`, or `event_final_mismatch`. A response with no named + submit remains existing `missing_submit`, not capture-missing. +2. For `repaired`, record only `pi_narrow_string_repair`; for partial/invalid, + record only bounded error kind. Include stage, role, submit tool, provider, + model-call attempt kind, and one bounded correlation id. +3. Add summary counters for states, error kinds, and outcomes. Strict calls need + no warning event; emit a warning only when Codegenie rejects a selected + submit value. +4. When the one submit repair resolves, emit one bounded outcome referencing + the original correlation id: `recovered`, `terminal_invalid`, or + `not_dispatched`. Do not mutate the earlier model-call record. +5. Preserve all existing schema-recovery counters and meanings. Event + provenance is an earlier trust dimension, not schema validity. +6. Never persist event text, final parser messages, argument values beyond the + existing trusted cached call, or hashes of those values. Seed secret-like + and repository-like strings in tests and scan events, model-call summaries, + errors, debug artifacts, and run artifacts for absence. + +Telemetry is also the trigger for extra targeted adapter work: if a provider +not in the two live smokes records `event_capture_missing` or +`event_final_mismatch`, reproduce only that public event shape in one focused +regression. Do not pre-build a provider matrix. + +**Verify**: +`pnpm exec vitest run tests/final-tool-arguments.test.ts tests/phase4-llm.test.ts tests/telemetry.test.ts` +passes. + +### Step 5: Document, run the two provider smokes, and measure post-land + +Document these invariants: + +- Pi may use partial parsing for streaming presentation, but Codegenie accepts + a final call only after strict/narrow parse plus deep equality over the + complete public event representation; +- public event representations may be raw fragments or provider-neutral + canonical JSON; Codegenie never parses provider wire formats; +- untrusted submits are non-executable, use one context-preserving clean + re-execution after discarding the invalid assistant response, and are never + cached; +- the rare clean re-execution can resend more trusted input context; existing + per-call token telemetry measures that cost without a new mechanism; +- normalized length stop wins, but Codegenie does not reinterpret + provider-specific raw stop reasons; +- no accumulated event text crosses into telemetry, cache, artifacts, or logs; +- broader syntax repair and upstream migration remain measurement-gated. + +Run full Codegenie checks. Then run only these configured-provider smokes: + +1. Direct Anthropic Messages. +2. One OpenAI-compatible Responses provider used by the project. + +For each, a normal structured submit must produce complete framing, +`strict` or current narrow `repaired` provenance, deep equality, unchanged +usage/stop reason, and zero extra calls or model-repair request. Synthetic stream tests—not live fault +requests—cover missing/mismatched/partial/length cases. + +Do not add live or copied fixtures for Google, Mistral, Bedrock, OpenAI +Completions, or every provider wrapper at this gate. The runtime boundary is +fail-closed; add one only after evidence identifies a distinct gap. + +The implementation gate is: + +- all generic event-shape and stream/completion parity tests pass; +- both live normal-provider smokes are trusted without a model-repair request + (Pi's bounded `pi_narrow_string_repair` provenance remains acceptable); +- zero accepted untrusted or provenance-less submits; +- zero extra provider calls on trusted finals; +- zero raw/value-bearing telemetry or artifacts. + +If either required normal smoke lacks complete event data or produces an +event/final mismatch, STOP rather than adding a provider exception or accepting +Pi's final object. Report the exact bounded state and evaluate a narrow +upstream event-contract fix. + +Once this gate passes, land the implementation and update the README row to +`IMPLEMENTED (measuring)`. The following corpus is post-land and must not hold +the implementation PR open: + +- at least 2,500 non-cache-hit primary finalized structured submits (the + terminal submit selected from the original stage request, before model + repair; tool continuations, missing-submit responses, and repair responses + are excluded); +- at least 20 completed runs spanning `49f4645b`, `0c4d5213`, and `relay-wc`; +- at least two provider API families and at least 500 qualifying submits from + each family; +- synthetic fixtures and model-repair responses do not count. + +Record totals by provenance state, repair kind, stage, provider API family, +model-repair outcome, and terminal disposition in this plan. Record no payload +examples. If real usage cannot satisfy the provider mix, leave the plan +`IMPLEMENTED (measuring)` and request an explicit plan revision; do not silently +lower or waive the denominator. + +Apply the broader-repair evidence gate after human review. Open a new numbered +classifier-and-remeasurement plan only when complete `invalid` syntax, +excluding `partial`, `length_stopped`, capture-missing, and final-mismatch, +meets either trigger: + +1. At least 10 occurrences and at least 0.25% of qualifying primary final + submits, with one bounded error class occurring at least five times; or +2. The same bounded complete-syntax class causes at least three terminal + degradations after the existing model repair. + +Current narrow repairs do not trigger a larger parser. Capture missing/mismatch +is an event-contract issue, not a JSON-repair opportunity. A broad +`invalid_syntax` count can authorize only a classifier-and-remeasurement plan, +not repair implementation. + +If neither syntax trigger fires, record “full loss-aware repair parser not +needed.” If one fires, open only the bounded follow-up described below. In +either case, record the human-reviewed result and mark Issue 112 `COMPLETE`; +the new numbered plan owns any follow-up. + +Separately, summarize event-capture missing/mismatch and terminal impact. These +counts may support an additive upstream Pi provenance proposal. An upstream PR +is not an automatic requirement and must not be bundled with a syntax-repair +plan. + +**Verify the implementation gate**: `pnpm run check`, `pnpm test`, +`pnpm build`, both provider smokes, and `git diff --check` pass. + +**Verify measurement closure**: the corpus satisfies every denominator rule, +contains no synthetic/repair submits, records a human-reviewed gate outcome, +and contains no payload examples. + +### Implementation evidence (2026-08-05) + +- The generic public-event, runner/cache, telemetry, and stage-disposition + regressions pass. `pnpm install --frozen-lockfile`, `pnpm run check`, + `pnpm build`, and `git diff --check` also pass. +- A direct Anthropic Messages smoke with `claude-haiku-4-5` completed one + provider call as `strict`, with a schema-valid submit, unchanged terminal + behavior, and no model repair. +- An OpenAI Codex Responses smoke with `gpt-5.4-mini` completed one provider + call as `strict`, with a schema-valid submit, unchanged terminal behavior, + and no model repair. +- Owner validation `49f4645b` run 63 passed its required finding and complete- + coverage gates. It recorded 11 strict selected submits and two complete + `invalid_syntax` Stage-9 submits; both invalid values were non-executable, + each used the existing single repair, and both recovered. No + capture-missing, mismatch, partial, length-stopped, terminal-invalid, or + not-dispatched outcome occurred. This pre-land validation is evidence for + the boundary but does not count toward the post-land corpus denominator. +- Owner validation `0c4d5213` run 71 passed all required, candidate, negative, + completeness, and budget gates. It recorded 96 strict selected submits and + one complete `invalid_syntax` Stage-9 submit. The invalid call retained the + explicit `final_arguments_invalid` classification through verifier + adjudication, used one repair, and recovered. No capture-missing, + mismatch, partial, length-stopped, terminal-invalid, or not-dispatched + outcome occurred. This pre-land validation also does not count toward the + post-land corpus. +- No provider-specific fixture, provider-wire parser, Pi patch/fork, broad + syntax repair, or second repair loop was added. + +The implementation gate is closed. The post-land corpus and human-reviewed +classifier gate remain intentionally open, so the status is +`IMPLEMENTED (measuring)` rather than `COMPLETE`. + +## Conditional future design — not authorized by this plan + +If and only if the classifier/remeasurement follow-up identifies a refined +complete-input syntax subtype that independently passes the gate, a later +repair plan must remain limited to that evidenced class: + +- operate in the isolated earliest-captured-text helper, never in stage code; +- prove one complete root with closed strings/numbers and balanced delimiters + before any tolerant transformation; +- use an operation ledger and deep-equality assertions proving no semantic + leaf or array member was added, removed, truncated, or guessed; +- rerun event/final equality, TypeBox, and stage semantics after repair; +- use the existing one model repair for missing fields, wrong types/enums, + truncation, divergence, and ambiguous transformations; +- never accept `partial-json`, delimiter auto-closing, scalar completion, or + capture-missing/final-mismatch as repairable JSON; +- kill/revert the future implementation if it accepts partial/length input, + mutates data, regresses an owner eval, or removes fewer than half of the + targeted repair calls in its validation corpus. + +An upstream Pi migration is a separate possible success path. If Pi later +releases native bounded final provenance, first run A/B fixtures proving its +states and accepted values are at least as strict as this helper. Then delete +the Codegenie accumulator in the same migration; do not retain two authorities. + +## Test plan summary + +- `tests/final-tool-arguments.test.ts`: generic public-event framing, strict / + narrow parsing, equality, mismatch/missing, length, error parity, cleanup, + and payload absence. +- `tests/phase4-llm.test.ts`: production stream selection/parity, trusted and + non-executable submit routing, one context-preserving repair, cache protection, + unchanged repository-tool behavior, and stage-local terminal behavior. +- `tests/telemetry.test.ts`: bounded states/outcomes and strict absence of event + text/values. +- Live gates: one normal Anthropic Messages submit and one normal + OpenAI-compatible Responses submit; no full provider matrix. +- Full Codegenie checks/tests/build plus owner measurement corpus. + +## Done criteria + +- [x] The production adapter consumes Pi's public stream while preserving the + existing completion request/options/result/error semantics. +- [x] Final accepted submit arguments come only from strict JSON or Pi's public + narrow repair, require one object root, and deep-equal Pi's final + arguments. +- [x] Missing/divergent/partial/invalid/normalized-length-stopped calls are + represented as local submit calls without `arguments` and cannot enter + submit validation. +- [x] Untrusted submits consume at most the existing one repair and then use + Issue 111's stage-local disposition; invalid assistant data is not sent + back to Pi in repair history. +- [x] Untrusted/provenance-less responses are never cached and the cache schema + version is bumped. +- [x] Telemetry records only bounded states/counts/outcomes and no accumulated + text, parser messages, values, or value hashes. +- [x] Generic event-shape/parity tests and the two required provider smokes + pass; no every-provider fixture matrix was added. +- [x] Full checks/tests/build pass and the README row moves to + `IMPLEMENTED (measuring)` without waiting for the corpus. +- [ ] Post-land, the minimum corpus and human-reviewed gate outcome are + recorded; the README row then moves to `COMPLETE`. +- [x] No Pi change, broad parser, provider-wire parser, duplicate-submit + change, second model repair, or generic repair-prompt redesign landed. +- [x] Specs and README status are updated. + +## STOP conditions + +Stop and report; do not improvise if any occurs: + +- Pi's public `complete` methods are no longer equivalent to consuming the + corresponding stream result, or required public event framing/deltas are + unavailable. +- A required normal Anthropic/OpenAI Responses smoke yields missing/divergent + event capture, extra provider calls, changed usage/error/timeout behavior, or + an incorrectly normalized length stop. +- Trustworthy capture requires private Pi scratch fields, provider wire + formats, provider-specific raw stop-reason mappings, or serializing Pi's + final object as the event source. +- Pi's public `repairJson` is unavailable or expands beyond the two narrow + string repairs. +- An untrusted submit would need an `arguments` placeholder, could reach + TypeBox, could re-enter Pi conversation history, or could enter the + model-call cache. +- Rejecting an untrusted submit cannot use the existing one-repair and + stage-local disposition paths. +- Event accumulation would need persistence, logging, hashing, or inclusion in + error context to work. +- A provider-specific exception or broad adapter fixture matrix appears + necessary without bounded smoke/telemetry evidence. +- Provider/owner gates regress, or focused tests fail twice after a reasonable + correction. + +## Maintenance notes + +- `strict` means strict parsing of the complete public event representation + plus equality with Pi's final value; it does not claim raw-wire capture. +- Runtime equality makes event-contract drift fail closed. A new provider + fixture is justified only after a smoke or telemetry identifies a distinct + missing/mismatch shape. +- Review Pi dependency upgrades for changes to event framing, `repairJson`, + completion-vs-stream equivalence, and stop-reason normalization. +- Pi's Google stop-reason overwrite and pre-validation primitive coercion are + separate semantic trust channels. Do not quietly absorb either into this + plan. +- Safe narrow-repair counts measure existing behavior, not evidence for a + broader parser. +- Keep the corpus denominator/provider mix with the recorded result so later + measurements remain comparable. diff --git a/specs/plans/113-issue-113-direct-reject-note-reconciliation.md b/specs/plans/113-issue-113-direct-reject-note-reconciliation.md new file mode 100644 index 0000000..1af5b6a --- /dev/null +++ b/specs/plans/113-issue-113-direct-reject-note-reconciliation.md @@ -0,0 +1,284 @@ +# Issue 113: Measure Direct-Reject Note Contradictions Before Adding Suppression + +Status: BACKLOG (measurement gate not met) +Planned from: trails-api eval `0c4d5213`, runs 72-73, 2026-08-05 +Planned at: commit `803bf6f` (branch `llm-repair`) +Recommended disposition: ship 0.5.5 without this change. Reopen design only +after the evidence gate below is met. + +> **Executor instructions**: This is a measurement and decision plan, not +> authorization to change human-attention suppression. Read the entire plan +> and evaluate the entry gate first. If it is not met, record any newly +> inspected evidence, leave production code unchanged, and STOP. If it is met, +> write or amend a separate implementation plan that satisfies every trust +> boundary below; do not implement the discarded fuzzy direct-reject design. +> +> **Drift check (run before re-evaluating the design)**: +> `git diff --stat 803bf6f..HEAD -- src/pipeline/human-attention.ts src/pipeline/lens-runner.ts src/pipeline/verifier.ts src/llm/schemas.ts tests/human-attention-adjudication.test.ts tests/verifier.test.ts specs/project/components/review_pipeline.md specs/plans/README.md` +> Re-read the live code if any path changed. STOP and retire or replace this +> plan if Stage 7 now retains exact candidate-to-hint provenance, verifier +> verdict semantics changed, or another plan already owns this gap. + +## Execution metadata + +- **Current priority**: BACKLOG; no implementation work authorized +- **Priority if the evidence gate fires**: P2 +- **Effort**: S for evidence review; implementation intentionally unestimated + until a safe data contract is selected +- **Risk**: HIGH for a premature fix because the behavior removes user-visible + questions and the current signals cannot distinguish all unresolved rejects +- **Depends on**: + `specs/plans/75-issue-75-human-attention-output-discipline.md` Step 1 and + `specs/plans/110-issue-110-publication-aware-note-adjudication.md` +- **Category**: measurement / output correctness / noise reduction +- **Planned at**: commit `803bf6f`, 2026-08-05 + +## Why this matters + +Run 72 exposed one real output contradiction. Stage 7 emitted both a direct +candidate and a singleton same-packet question asking whether +`TokenCharge`/`AmountOut` could be zero. Stage 9 proved those values strictly +positive on every valid path and rejected the candidate with high +false-positive risk. Because the verdict also said +`requiredEvidencePresent: false`, the normal verification-resolution index did +not consume it, and the stale question reached the rendered review. + +Run 73 is the adjacent negative control. A fresh model call emitted the same +candidate and sibling note from the same packet and reached the same substantive +rejection, but this time returned `requiredEvidencePresent: true`. The existing +`stage9_verified_predicate` path suppressed the note correctly. The common path +therefore works; the observed miss is intermittent. + +Removing a stale question is useful, but removing a genuinely unresolved +question is worse. The initially proposed shortcut—treating any complete +`reject + falsePositiveRisk: high + requiredEvidencePresent: false` as +conclusive—does not satisfy that trust boundary. + +## Why the original implementation design is unsafe + +The following are verified current-state constraints. A future plan must not +reason around them: + +1. **The verdict tuple is ambiguous.** + `buildVerifierSchemaRepairPrompt()` explicitly instructs the model that when + verification cannot be completed it should reject with + `requiredEvidencePresent=false` and `falsePositiveRisk=high`. The same tuple + can therefore mean either “the predicate was disproved” or “decisive + evidence was unavailable.” `verificationIncomplete` does not disambiguate + this reliably: it is a runner-normalized state, not a model-authored + machine-readable claim-resolution field. +2. **Packet membership is not exact sibling provenance.** + `poolEnsemblePassResults()` unions findings, hints, and uncertainties from + multiple Stage-7 passes into one `PacketReviewResult`. Candidates retain an + `ensemblePass`; hints currently do not. Finding and note membership in the + same pooled packet cannot prove they originated in the same model response. +3. **The normal semantic matcher is not an ownership relation.** + `attentionGroupResolvedByVerification()` intentionally uses permissive + file/symbol/term overlap for evidence-backed resolution. Reusing those + thresholds for a new subtractive path can match a different predicate in + the same file or call chain. +4. **Per-group uniqueness is insufficient.** One candidate can be the sole + fuzzy match for two singleton groups. Checking “exactly one candidate per + group” alone could suppress both notes; safe association requires + bidirectional one-to-one identity or explicit provenance. + +Consequently, verifier prose parsing, packet equality, or existing fuzzy +thresholds are not acceptable substitutes for a trustworthy relation. + +## Standing trust invariant + +> A direct Stage-7 note remains visible unless Codegenie has both (a) +> machine-readable proof that verification resolved/refuted its predicate +> rather than failing to obtain evidence and (b) an exact, unambiguous +> candidate-to-note relation. When either fact is unavailable, keep the note. + +This preserves the existing asymmetry: mild stale-note noise is preferable to +silently deleting an unresolved review question. + +## Evidence ledger + +Keep this table current when a suspected recurrence is reviewed. Do not add a +row from aggregate counts alone; inspect the candidate, verdict, raw/grouped +note, suppression record, and rendered output. + +| Evidence | Real model execution | Predicate outcome | `requiredEvidencePresent` | Note outcome | Classification | +| --- | --- | --- | --- | --- | --- | +| `0c4d5213` run 72, candidate `8aef05e7-f1` | yes, no-cache | verifier directly refuted the zero-amount predicate from available source | false | singleton note rendered | confirmed stale contradiction; positive incident 1 | +| `0c4d5213` run 73, candidate `8aef05e7-f1` | yes, no-cache | same predicate directly refuted | true | suppressed by existing `stage9_verified_predicate` path | negative control; not an incident | + +For every new row, record separately whether: + +- the candidate was direct Stage 7 or promoted; +- the packet had one pass or pooled multiple passes; +- verification completed or was runner-incomplete; +- the verifier refuted the predicate from inspected evidence, declared it + unconfirmed/unavailable, or was ambiguous; +- the note group was singleton or merged; +- the note reached the composer prompt and/or final output; and +- an existing evidence-backed, promoted-provenance, finding-coverage, or + publication-fallback path should already have handled it. + +## Measurement entry gate + +Do not authorize an implementation plan until either gate A or gate B is met: + +- **Gate A — repeated eval evidence**: at least three confirmed stale + contradictions from distinct real no-cache model executions, spanning at + least two distinct predicates or owner cases. +- **Gate B — production evidence**: at least one retained, user-visible + production incident plus one independent confirmed recurrence in an eval or + production run. + +Run 72 counts as one Gate-A incident. Run 73 does not count. Synthetic fixtures, +cache replays, identical copied artifacts, and rejects whose reason says the +decisive helper/evidence was unavailable do not count. + +Existing artifacts are sufficient for the current gate. Do not add production +instrumentation solely to measure this issue unless a suspected incident +cannot be classified from `verification.json`, `human-attention-notes.json`, +packet results, and the rendered review. If artifacts are insufficient, write +a small observability-only plan first; do not combine instrumentation and +suppression. + +If neither gate fires, leave this plan in BACKLOG. The absence of recurrence is +a valid result and should retire the proposed mechanism rather than invite +weaker thresholds. + +## Measurement procedure + +### Step 1: Identify a suspected contradiction + +Start from a rendered human-attention note, then trace backward through: + +1. `human-attention-notes.json` for the raw note, group membership, packet id, + output/suppression disposition, and match facts; +2. `verification.json` for candidates from the same bounded scope; +3. the Stage-7 packet result for direct/promoted origin and `passesRun`; and +4. the verifier reason and repository evidence needed to classify the + predicate as refuted, unresolved, or ambiguous. + +Do not infer an incident merely because a high-risk reject and a same-file note +coexist. + +### Step 2: Apply the incident test + +Count the case only when all of these hold: + +- a real direct candidate was rejected by completed verification; +- the verifier's inspected evidence actually refutes the candidate predicate; +- a singleton note states that same predicate; +- the note survives into the composer or rendered output; +- the failure is attributable to the current resolution contract rather than + invalid paths, grouping, publication fallback, or verification skipping; and +- a human reviewer agrees the output is self-contradictory. + +Classify evidence-unavailable, multi-member, multi-predicate, or cross-pass +cases separately. They are not authorization to suppress. + +### Step 3: Record the decision + +Append the evidence row and state whether Gate A or B is met. If not met, STOP +without source changes. If met, create a separate implementation plan using +the design requirements below and link it from this file and the plans index. + +## Requirements for any future implementation plan + +The evidence gate authorizes design work, not the discarded implementation. +A replacement plan must provide all of the following: + +1. **Machine-readable predicate disposition.** Introduce or derive a bounded + structured state that distinguishes at least `refuted` from `unresolved`. + An unavailable helper, missing decisive evidence, incomplete verification, + or ambiguous result must be `unresolved`. Do not parse verifier prose to + manufacture this state. +2. **Exact origin or explicit association.** Preserve enough Stage-7 + generation/pass provenance to prove which candidate and note are related. + Same pooled `packetId` is insufficient. Prefer an explicit local relation; + if exact provenance is unavailable, keep the note and STOP. +3. **Bidirectional uniqueness.** The note must map to exactly one qualifying + candidate, and that candidate must map to exactly one eligible note. One + candidate matching two notes, or two candidates matching one note, keeps + every ambiguous note. +4. **Candidate-only identity.** The verifier may return both a structured + disposition and a prose reason, but the prose must neither be parsed into + that disposition nor create candidate-to-note identity. No new text + threshold, embedding, LLM adjudication call, language-specific rule, or + run-id rule may be introduced without separately measured evidence. +5. **Narrow output scope.** Only singleton, valid-scope direct notes may be + eligible. Existing promoted-note exact provenance, evidence-backed normal + resolution, final-finding coverage, Issue-110 publication fallback, note + caps, and wording remain unchanged. +6. **Auditable removal.** Artifacts and bounded telemetry must identify the + suppression source, structured predicate disposition, exact association, + and uniqueness facts without storing unbounded verifier/note prose. + +The replacement plan must include adversarial tests proving that all of these +remain visible: + +- `reject + requiredEvidencePresent:false + falsePositiveRisk:high` where the + decisive helper or evidence could not be inspected; +- one candidate associated with two singleton notes; +- two candidates associated with one singleton note; +- same packet/file/symbol but a different predicate; +- candidate and note pooled from different ensemble passes; +- a merged/multi-member group; +- incomplete verification and medium/low-risk rejects; and +- missing/invalid origin or file scope. + +It must also include a truly direct, provenance-free run-73-shaped control: +`requiredEvidencePresent:true` continues through +`stage9_verified_predicate`, suppresses through the existing path, and emits no +new direct-adjudication source. + +## Scope + +**In scope while the gate is closed**: + +- Inspect retained eval/production artifacts. +- Update only the evidence ledger, status, and disposition in this plan and + `specs/plans/README.md`. + +**Out of scope while the gate is closed**: + +- Any production source, schema, prompt, matcher, telemetry, or test change. +- Parsing verdict prose. +- Reinterpreting `requiredEvidencePresent`, `falsePositiveRisk`, or + `verificationIncomplete` without a new explicit contract. +- Reusing normal fuzzy similarity as a direct-note deletion rule. +- Holding the 0.5.5 release. + +## Done criteria for this measurement plan + +- [ ] Every candidate incident is recorded with inspected artifacts and a + refuted/unresolved/ambiguous classification. +- [ ] Run 72 remains the first positive incident and run 73 remains a negative + control unless new primary artifacts prove otherwise. +- [ ] Gate A or Gate B is evaluated explicitly. +- [ ] If the gate is not met, no production or test files are changed. +- [ ] If the gate is met, a separate implementation plan satisfies every + future-design requirement and adversarial test above before coding begins. + +## STOP conditions + +Stop and leave behavior unchanged if: + +- neither measurement gate is met; +- the only proposed discriminator is the current verdict tuple or reason text; +- exact candidate-to-note/pass association cannot be represented; +- a solution relies on same-packet fuzzy similarity alone; +- a solution can suppress an evidence-unavailable or ambiguous predicate; +- a solution changes existing promoted, evidence-backed, finding-coverage, or + publication-fallback behavior; or +- the issue disappears after a schema/provenance change elsewhere. + +## Maintenance notes + +This plan intentionally records a real but low-impact inconsistency without +turning one stochastic event into production suppression. Its success outcome +may be deletion: if the issue does not recur, mark it closed/not planned. + +If Stage 7 later gains exact candidate-to-hint provenance or Stage 9 gains a +machine-readable refuted/unresolved distinction for another justified reason, +re-evaluate 113 against those contracts. Reuse them only if they eliminate the +ambiguity described here; do not preserve a separate semantic bridge merely +because this plan once proposed one. diff --git a/specs/plans/114-issue-114-context-preserving-provenance-retry.md b/specs/plans/114-issue-114-context-preserving-provenance-retry.md new file mode 100644 index 0000000..28cff45 --- /dev/null +++ b/specs/plans/114-issue-114-context-preserving-provenance-retry.md @@ -0,0 +1,548 @@ +# Issue 114: Re-execute Untrusted Structured Submits From Trusted Context + +Status: COMPLETE +Planned from: Codegenie self-review GitHub Actions run `31021166634` +(Codegenie v0.5.4 reviewing `ae1bb70..803bf6f`) and cold source/spec +validation at current branch HEAD, 2026-08-05 +Planned at: commit `fdeb767` (branch `llm-repair`) +Recommended priority: before the 0.5.5 release. This plan fixes one +release-blocking continuation defect in Issue 112, then closes a bounded set +of already-confirmed one-line/test-contract gaps in the same branch. It does +not add a new mechanism, telemetry field, parser, or stage-specific matrix. + +> **Executor instructions**: Read this plan completely before changing code. +> Implement the smallest generic correction in the existing one-repair seam: +> reject and discard the untrusted assistant response exactly as today, retain +> the structurally valid conversation that preceded it, append the existing +> bounded stage repair instruction, and make the one forced-submit retry. Do +> not inspect, infer from, salvage, log, hash, cache, or resend rejected final +> arguments. Run every verification command. If a STOP condition occurs, stop +> and report rather than broadening the fix. Update this plan and its row in +> `specs/plans/README.md` when complete. +> +> **Drift check (run first)**: +> `git diff --stat fdeb767..HEAD -- package.json pnpm-lock.yaml src/llm/pi-runner.ts src/llm/final-tool-arguments.ts src/pipeline/planner.ts src/pipeline/verifier.ts src/pipeline/composer.ts src/output/markdown-renderer.ts src/telemetry/run-artifacts.ts src/github-action/entrypoint.ts action.yml tests/phase4-llm.test.ts tests/final-tool-arguments.test.ts tests/telemetry.test.ts tests/github-action.test.ts tests/pipeline-phase5.test.ts specs/plans/112-issue-112-final-tool-argument-provenance.md specs/project/architecture.md specs/project/components/skills_llm_telemetry.md specs/plans/README.md` +> Reconcile every Current state statement if a listed path changed. The +> planner/verifier/composer and Pi dependency files are read-only drift inputs, +> not authorized modification targets. STOP if the one-repair scheduler, +> non-executable call representation, Pi 0.83.0 public event/error contract, +> or stage-local terminal policies changed semantically. + +## Execution metadata + +- **Priority**: P1 +- **Effort**: S-M +- **Risk**: MED (one shared runner continuation changes, with a narrow + parameterized regression and unchanged provider/schema/cache boundaries) +- **Depends on**: + `specs/plans/111-issue-111-observed-structured-submit-resilience.md` and + `specs/plans/112-issue-112-final-tool-argument-provenance.md`, both already + implemented on this branch +- **Category**: correctness / trust-boundary follow-up / telemetry / tests +- **Planned at**: commit `fdeb767`, 2026-08-05 + +## Why this matters + +Issue 112 correctly turns a partial, invalid, length-stopped, capture-missing, +or event-divergent named submit into a local `invalidToolCall` without +`arguments`. The runner does not append that assistant response to Pi history, +does not validate or execute the submit, and does not cache the response. These +are the required fail-closed guarantees and must remain unchanged. + +The retry currently discards independently trusted information too. The +untrusted path forces `replaceConversationOverride: true`, so +`queueSchemaRepair()` replaces the original stage request and any earlier valid +tool history with a repair message. A Stage-7 retry then sees a submit schema +but not the packet or diff it must review. It can return schema-valid +`findings: []` without doing an informed review, allowing known response loss +to look like a clean packet result. Planner, verifier, and composer provenance +failures have the same stage-generic problem. + +The fix is a clean stage re-execution, not JSON repair. The invalid assistant +turn remains absent, while the trusted pre-response conversation remains. The +existing bounded repair message is appended and the existing one forced-submit +retry runs. If that retry fails, Issue 111's current planner fallback, packet +incompleteness, verifier suppression, or composer fallback remains the final +authority. + +The same self-review confirmed two tiny consistency defects and four weak +regression assertions. They do not justify separate architecture or separate +plans: cache replays inflate the aggregate provenance histogram, the Action's +terminal-post path bypasses its bounded error-code helper, and existing tests +do not structurally pin failure-artifact upload, both event/final equality +clauses, telemetry provenance shape, or complete-degraded rendering. Fixing +those now is reasonable because each change is local, deterministic, and adds +no behavior policy. New telemetry fields and planner/verifier/composer test +matrices remain unnecessary. + +## Current state + +### The trusted prefix already exists + +`src/llm/pi-runner.ts:267-269` initializes the provider conversation with the +stage request: + +```ts +const messages: ConversationMessage[] = [ + { role: "user", content: request.prompt, timestamp: 0 } +]; +``` + +Successful investigation rounds append structurally valid assistant tool calls +and deterministic tool results. At `src/llm/pi-runner.ts:404-410`, the runner +does not append an assistant message containing a local `invalidToolCall`. +Therefore `messages` still contains the valid pre-response conversation when a +provenance-invalid submit is rejected. + +At `src/llm/pi-runner.ts:435-449`, the untrusted submit path schedules repair +with `replaceConversationOverride: true` and immediately `continue`s. That +`continue` happens before repository tools from the same assistant response +could execute, so mixed invalid-submit/tool turns currently produce no orphaned +`toolResult`. Preserve that ordering. + +### Explicit false is currently dropped + +The local scheduler accepts `replaceConversationOverride?: boolean`, but two +forwarding sites preserve only literal `true`: + +- `src/llm/pi-runner.ts:302` forwards the local scheduler input with + `repair.replaceConversationOverride === true`. +- `src/llm/pi-runner.ts:539` forwards recovery guidance with + `recovery.replaceConversationOverride === true`. + +At `src/llm/pi-runner.ts:2519`, `queueSchemaRepair()` already implements the +required tri-state behavior: + +```ts +const replaceConversation = input.replaceConversationOverride ?? + (input.request.schemaRepair?.replaceConversation === true); +``` + +An explicit `false` must reach this expression. Merely deleting the untrusted +path's current `true` is insufficient because planner, verifier, composer, and +some tests configure ordinary schema repair with `replaceConversation: true`. + +### Existing repair prompts remain the authority + +Do not add another prompt builder or mode. Keep the current selection order: + +1. trusted Stage-7 compact schema repair when its current classification and + replacement conditions hold; +2. the stage's existing `schemaRepair.buildPrompt`, when provided; otherwise +3. `defaultSchemaRepairPrompt()`. + +Those stage builders remain meaningful with `repairInput.submitCalls: []`: + +- `src/pipeline/planner.ts:675-750` uses the independently trusted planner + dossier/hunk inventory; only its invalid-submission list is empty. +- `src/pipeline/verifier.ts:945-980` uses the independently trusted candidate + projection and explicit provenance classification. Its wrapper also owns + verifier repair-attempt telemetry. +- `src/pipeline/composer.ts:546-590` uses the independently trusted verified + finding groups and known ids. +- Production Stage 7 supplies no custom builder, so it uses the bounded default + repair instruction appended after the original packet prompt/tool history. + +The stage-specific prompts can repeat some trusted context after this change. +That is acceptable on the rare provenance-repair path and is safer than +creating a second prompt policy. Existing per-call token telemetry can reveal +material cost if it ever becomes frequent. + +### The current test encodes the defect + +`tests/phase4-llm.test.ts:2049-2107` covers all five untrusted states but calls +the path “stateless repair” and asserts that the second provider request does +not contain the original Stage-7 prompt. It also already captures the +`schema_repair_scheduled` event, whose bounded data includes +`replaceConversation`. This is the primary regression to reverse. + +### Confirmed mechanical hardening + +- `src/telemetry/run-artifacts.ts:845-869` already assigns cache hits + `providerCallCount = 0`, but increments `finalArgumentStates` and + `finalArgumentErrorKinds` outside that guard. Raw cache-hit records should + remain; only provider-call aggregates should exclude them. +- `src/github-action/entrypoint.ts:438-440` intentionally maps arbitrary + non-`CodegenieError` failures to `unknown_error`. The main failure path uses + it; `src/github-action/entrypoint.ts:229-233` still publishes `Error.name` for + terminal-post failures. +- `tests/telemetry.test.ts:720-779` asserts that two strings are absent even + though neither enters the system under test. Parsed record/summary shape is + the correct assertion surface; raw-delta removal is already exercised in + `tests/final-tool-arguments.test.ts`. +- `tests/github-action.test.ts:1137-1143` searches three independent raw YAML + substrings instead of binding the failure path and `always()` guard to the + parsed upload step. +- `tests/final-tool-arguments.test.ts:84-91` makes both deep-equality clauses + fail in one fixture, so neither clause is independently protected. +- `tests/pipeline-phase5.test.ts:6621-6646` asserts a fixture field that the + test itself set instead of asserting complete-degraded output language. + +## Required behavior + +All requirements are load-bearing: + +1. **Reject bad data unchanged.** The untrusted selected submit has no + `arguments`; its assistant response and accumulated event text never enter + provider history, cache, validation, telemetry, logs, or artifacts. +2. **Preserve the prior conversation.** The retry receives the original stage + prompt plus any earlier valid assistant tool calls and deterministic tool + results. It never receives the rejected assistant turn. +3. **Append existing bounded guidance.** The current stage-specific/default + builder receives the same bounded classification and untrusted-call + id/name/state metadata as today, with `submitCalls: []`. No rejected + argument value is synthesized or supplied. +4. **Keep one forced-submit retry.** Reuse `queueSchemaRepair`; do not add a + loop, provider retry, repository-tool round, or special Stage-7 second pass. +5. **Discard a mixed invalid turn atomically.** If the same response contains + valid repository calls and an untrusted named submit, execute none of those + repository calls and push no results. Schedule only the submit retry. +6. **Keep trusted repairs unchanged.** Trusted schema-invalid Stage-7 compact + repair, planner/verifier/composer deterministic recovery, semantic + validation, and terminal stage policies retain their current replacement / + fallback behavior. +7. **Keep candidate tracking trusted.** Do not read or infer from discarded + arguments to arm `candidateDrafted`. A context-complete successful redo may + legitimately return no findings. +8. **No version or provider work.** Do not change schemas, prompt template + versions, cache schema/version, Pi, provider adapters, or parsing tolerance. +9. **Provider-call aggregates exclude cache replay.** Keep raw cache-hit model + records and cache counts, but do not count their final-argument state/error + in provider-call histograms. +10. **Public Action identity stays bounded.** Both failure paths use the same + `actionErrorCode()` allowlist; arbitrary `Error.name` is not restored. +11. **Test hardening changes no production policy.** Bind tests to the parsed + Action step, each equality predicate, persisted bounded provenance shape, + and rendered review language. Do not alter correct production behavior to + satisfy a test. + +## Scope + +**Production source in scope**: + +- `src/llm/pi-runner.ts` +- `src/telemetry/run-artifacts.ts` +- `src/github-action/entrypoint.ts` + +**Test source in scope**: + +- `tests/phase4-llm.test.ts` +- `tests/final-tool-arguments.test.ts` +- `tests/telemetry.test.ts` +- `tests/github-action.test.ts` +- `tests/pipeline-phase5.test.ts` + +**Contract/status documentation in scope**: + +- `specs/plans/112-issue-112-final-tool-argument-provenance.md` +- `specs/plans/114-issue-114-context-preserving-provenance-retry.md` +- `specs/plans/README.md` +- `specs/project/architecture.md` +- `specs/project/components/skills_llm_telemetry.md` + +**Read-only drift inputs**: + +- `package.json` and `pnpm-lock.yaml` +- `src/pipeline/planner.ts` +- `src/pipeline/verifier.ts` +- `src/pipeline/composer.ts` +- `src/llm/final-tool-arguments.ts` +- `src/output/markdown-renderer.ts` + +**Explicitly out of scope**: + +- `src/llm/final-tool-arguments.ts`: strict/narrow parsing, deep equality, + Pi terminal `error` handling, and the argument-less invalid-call shape are + already correct. +- `src/llm/stage7-submit-repair.ts`: trusted schema-invalid compact repair is + separate and remains correct. +- `src/pipeline/verifier.ts`: do not alter `candidateDrafted`, verdict policy, + or empty-submit classification. +- `action.yml`: current failure-artifact wiring is correct. It is a read-only + test input; do not edit it to make the structural assertion pass. +- `src/output/markdown-renderer.ts`: current behavior is correct. Strengthen + its test without changing production. +- New telemetry for the deferred question of how often a provenance-lost + Stage-7 retry later returns no findings. Existing + `final_argument_repair_outcome` and `packet_review_no_findings` events share + worker/packet identity and can be analyzed if evidence later justifies a + publication-policy plan. +- Plan 113, tolerant JSON repair, provider-specific handling, Pi changes, + retry-budget changes, selection, verification, or publication policy. + +## Commands you will need + +| Purpose | Command | Expected on success | +| --- | --- | --- | +| Focused runner test | `pnpm exec vitest run tests/phase4-llm.test.ts` | exit 0; all tests pass | +| Focused hardening tests | `pnpm exec vitest run tests/final-tool-arguments.test.ts tests/telemetry.test.ts tests/github-action.test.ts tests/pipeline-phase5.test.ts` | exit 0; all tests pass | +| Type and workflow checks | `pnpm run check` | exit 0; no TypeScript or workflow errors | +| Full tests | `pnpm test` | exit 0; all tests pass | +| Build | `pnpm build` | exit 0 | +| Patch hygiene | `git diff --check` | exit 0; no output | +| Owner regression smoke | `pnpm dev eval --eval-dir /home/peter/Dev/0xPolygon/codegenie-private-evals/trails-api/49f4645b --no-cache` | required expectation holds and coverage/publication remains truthful | + +The focused baseline at plan time was green. The changed provenance-context +assertion may fail before the production edit and must pass afterward. + +## Implementation steps + +### Step 1: Preserve an explicit append override through the repair seam + +In `src/llm/pi-runner.ts`: + +1. At both boolean forwarding sites, include + `replaceConversationOverride` whenever it is `!== undefined`, preserving + both `true` and `false`. +2. In the untrusted selected-submit branch, pass + `replaceConversationOverride: false`. +3. Do not otherwise change `queueSchemaRepair()`. Its existing nullish + fallback will now choose append mode for this path. +4. Do not push or sanitize the invalid assistant response. Preserve the + existing immediate `continue` before repository execution. +5. Keep the prompt-selection order, `forceFinalize`, one-repair accounting, + correlation/outcome telemetry, cache rejection, and terminal error handling + unchanged. + +This must be a small runner diff. If implementation appears to require a new +request type, retry abstraction, prompt builder, state flag, or stage branch, +STOP and re-read the existing optional boolean seam before proceeding. + +### Step 2: Replace the defective assertion with direct boundary regressions + +Update `tests/phase4-llm.test.ts`: + +1. Revise the existing parameterized test for all five untrusted states: + `length_stopped`, `partial`, `invalid`, `event_capture_missing`, and + `event_final_mismatch`. +2. Configure `schemaRepair.replaceConversation: true` in the fixture so the + test proves explicit `false` overrides a stage's normal replacement policy. +3. Assert the second provider context: + - contains the original stage prompt; + - contains the bounded repair builder output/classification; + - contains neither the local `invalidToolCall` nor rejected argument data. +4. Assert exactly two provider calls, one repair, no cache write for the + rejected primary, the existing bounded repair outcome, and + `schema_repair_scheduled.data.replaceConversation === false`. +5. Add one mixed-turn negative control: the primary response contains a valid + repository call plus an untrusted selected submit. Assert that the + repository executor is not called, no `toolResult` appears in the retry + context, the invalid assistant turn is absent, and the original trusted + request remains. +6. Preserve the existing tests in which the repair is also untrusted, + dispatch is budget-blocked, a provenance-less cache entry is rejected, and + normal trusted submits remain byte-for-byte unaffected. + +The custom builder used by the parameterized fixture is sufficient to prove +that append mode still invokes the stage builder with bounded metadata. Do not +add separate planner/verifier/composer test matrices: their builder selection +does not change, their source inputs are independently trusted, and existing +tests already cover their prompt-specific behavior. + +**Verify**: + +`pnpm exec vitest run tests/phase4-llm.test.ts` + +Expected: exit 0; every provenance state keeps trusted context, no rejected +assistant data returns, the mixed turn executes no tool, and existing terminal +and cache protections pass. + +### Step 3: Apply the confirmed mechanical hardening + +Make only these bounded changes: + +1. In `RunTelemetryImpl.updateModelSummary()` in + `src/telemetry/run-artifacts.ts`, increment `finalArgumentStates` and + `finalArgumentErrorKinds` only when `providerCallCount > 0`. Preserve the + raw cache-hit record, `totalRecords`, cache counters, and all existing + token/cost/finalize semantics. In `tests/telemetry.test.ts`, add a cache-hit + provenance record and prove it remains in `model-calls.jsonl` but not the + aggregate histograms. Replace the never-seeded string assertions with + assertions over parsed bounded provenance fields. Do not add a telemetry + field. +2. In the `terminal_post_failed` catch in + `src/github-action/entrypoint.ts`, call `actionErrorCode(error)` just like + the main failure path. Extend the Action test seam with a plain `TypeError` + terminal-update failure and assert the action record uses `unknown_error` + without publishing its message/name. Preserve the existing typed + `github_post_failed` case. +3. In the parsed `action.yml` test, find the run step and upload-artifact step. + Read the exact `CODEGENIE_FAILURE_PATH` value from the run step, assert that + value occurs in the upload step's `with.path`, and assert that same upload + step's `if` contains `always()`. Remove the three unrelated substring + assertions. Do not edit `action.yml`. +4. Split the event/final mismatch fixture into end-event-only divergence and + terminal-message-only divergence. Each must yield an argument-less + `invalidToolCall` with `event_final_mismatch`. Do not change + `src/llm/final-tool-arguments.ts`. +5. Replace the degraded-planning fixture assertion with rendered output + assertions: the degraded banner precedes the summary, + `## ✅ No Findings` appears, and partial/incomplete wording does not. Keep + the sibling partial test that forbids approval-equivalent clean language. + Do not change the renderer. + +Do not add new measurement assertions linking packet publication, any new +telemetry field, or separate planner/verifier/composer matrices. Those are not +needed to close the confirmed gaps above. + +**Verify**: + +`pnpm exec vitest run tests/final-tool-arguments.test.ts tests/telemetry.test.ts tests/github-action.test.ts tests/pipeline-phase5.test.ts` + +Expected: exit 0; cache aggregates exclude hits, public failure identity is +bounded consistently, and every strengthened test is bound to its real +contract. + +### Step 4: Correct the standing trust-boundary documentation + +Update only the stale contract language: + +- In `specs/plans/112-issue-112-final-tool-argument-provenance.md`, supersede + the statement that an untrusted submit uses schema-only stateless / + replace-conversation repair. State that the invalid assistant response and + its arguments are discarded while the independently constructed prior + conversation is retained for one clean re-execution. Link Issue 114 and keep + Issue 112 `IMPLEMENTED (measuring)` until its existing corpus closes. +- Make the same narrow correction in `specs/project/architecture.md` and + `specs/project/components/skills_llm_telemetry.md`. Preserve the rule that an + untrusted assistant response never enters provider history or cache. +- Note that the rare repair can consume more input tokens because trusted + context is resent. Existing per-call telemetry is sufficient; add no cost + mechanism or field. +- Document that final-argument state/error aggregates count provider calls and + exclude cache hits; the Plan-112 corpus still filters primary non-cache-hit + records explicitly. +- Mark Plan 114 `COMPLETE` in this file and `specs/plans/README.md` only after + Step 5 passes. + +Do not revise unrelated Plan-112 measurement, parser, cache, or provider text. + +**Verify**: + +`rg -n "stateless model repair|always replace conversation history|schema-only" specs/plans/112-issue-112-final-tool-argument-provenance.md specs/project/architecture.md specs/project/components/skills_llm_telemetry.md` + +Expected: no standing claim that provenance-invalid retries discard the +trusted request context. Historical wording may remain only when immediately +identified as superseded by Issue 114. + +### Step 5: Run release validation + +Run, in order: + +1. `pnpm run check` +2. `pnpm test` +3. `pnpm build` +4. `git diff --check` +5. `pnpm dev eval --eval-dir /home/peter/Dev/0xPolygon/codegenie-private-evals/trails-api/49f4645b --no-cache` + +Inspect the owner smoke rather than relying only on its score: + +- the required finding remains present; +- coverage/publication is complete or explicitly partial; +- ordinary strict submits do not schedule new repairs; +- no extra provider call or repository-tool round was introduced; and +- any naturally occurring provenance repair uses one context-preserving retry + and then the existing stage-local disposition. + +The deterministic tests, not stochastic occurrence in the owner smoke, prove +the malformed-submit path. The owner smoke guards broader review quality. If +credentials or the private eval repository are unavailable, leave Plan 114 +`PENDING (owner smoke required)` after local checks and report the external +gate. + +### Implementation evidence (2026-08-05) + +- The runner change preserves explicit `false` through both repair-forwarding + layers and selects append mode only for an untrusted final submit. The + invalid assistant turn remains discarded before repository-tool execution, + validation, history, and cache. +- The five focused files pass 451 tests. The parameterized runner regression + covers every untrusted provenance state with a stage-level replacement + policy enabled, and the mixed-turn negative control proves that a repository + call beside an invalid submit is neither executed nor orphaned. +- Cache-hit provenance remains in `model-calls.jsonl` and cache counts while + provider-call state/error histograms exclude it. Both Action terminal paths + use the bounded error-code helper, and the Action YAML, divergence, bounded + telemetry shape, and degraded-rendering contracts have structural tests. +- `pnpm run check`, the full 843-test suite, `pnpm build`, and + `git diff --check` pass. +- Owner eval `49f4645b` run 65 passed 1/1 required expectations with one inline + finding, complete coverage, zero budget overruns, and zero loss before + candidate generation, verification, or composition. It recorded 12 strict + final submits, zero schema-invalid calls, and zero repair calls, so no + malformed submit occurred naturally. Relative to run 64 it used 28 provider + calls instead of 38 and 61 tool calls instead of 69; deterministic tests, + not this stochastic delta, prove the repaired path. + +## Done criteria + +- [x] An untrusted selected submit remains argument-less, non-executable, + uncached, and absent from provider history. +- [x] Its one retry retains the valid pre-response conversation and appends + only the existing bounded stage repair instruction. +- [x] Explicit `replaceConversationOverride: false` survives both forwarding + layers and overrides stage-level `replaceConversation: true`. +- [x] A mixed invalid-submit/repository-tool response executes no repository + call and produces no orphaned result. +- [x] All five provenance-invalid states, repaired-submit failure, cache + rejection, and normal trusted submit paths pass focused tests. +- [x] Trusted Stage-7 compact repair, stage builders, candidate tracking, + retry count, schemas, prompt/cache versions, provider behavior, and + terminal policies remain unchanged. +- [x] Cache-hit provenance remains in raw records/cache counts but is excluded + from aggregate final-argument state/error histograms. +- [x] Both Action failure paths use the bounded error-code vocabulary. +- [x] Action upload, divergence, telemetry-shape, and degraded-rendering tests + assert their real contracts without changing correct production code. +- [x] Plan 112 and standing architecture docs describe context-preserving clean + re-execution without weakening the invalid-data boundary. +- [x] `pnpm run check`, `pnpm test`, `pnpm build`, and `git diff --check` pass. +- [x] One no-cache `49f4645b` owner smoke passes its required and coverage + gates, or Plan 114 remains explicitly pending that smoke. + +## STOP conditions + +Stop and report; do not improvise if any occurs: + +- Retaining task context would require appending the invalid assistant turn, + reconstructing its arguments, accessing accumulated event text, or + serializing a local `invalidToolCall` back into Pi history. +- The conversation before the rejected response is already structurally + inconsistent, such as containing an orphaned tool result. +- Explicit false cannot reach `queueSchemaRepair()` without changing trusted + Stage-7 compact replacement or another existing schema-repair path. +- A stage builder attempts to read rejected arguments rather than using its + independently trusted dossier/candidate/groups and bounded provenance + metadata. +- Mechanical hardening appears to require a new telemetry field, schema, + parser, prompt, or production change to `action.yml`, + `final-tool-arguments.ts`, or the Markdown renderer. +- A provider requires echoing the invalid assistant response. Do not add a + provider-specific exception. +- A proposed fix reads untrusted arguments to arm `candidateDrafted`, accepts + partial/length-stopped data, adds tolerant JSON repair, or adds another model + call. +- Pi's installed public terminal `error` event is no longer message-shaped. + Treat that as a separate dependency investigation. +- Focused/full tests or the owner smoke regress review quality, coverage truth, + or stage-local failure behavior. + +## Maintenance notes + +- “Trusted conversation” means structurally valid history independently + constructed before the malformed response. Reviewed repository content is + still attacker-controlled and remains governed by existing prompt fencing + and repository-tool containment. +- This is a redo of lost stage work, not repair of rejected JSON. Future code + and documentation should call it “clean re-execution” or + “context-preserving retry.” +- A successful informed Stage-7 redo may legitimately return no findings. Do + not reintroduce an untrusted candidate-like flag. If production evidence + later suggests a stricter publication policy, existing repair-outcome and + packet-result telemetry can support a separate measured plan. +- Context-preserving retries deliberately cost more input tokens than the + schema-only replacement they supersede. Investigate only if existing + telemetry shows material frequency or cost. +- New publication measurement fields and separate planner/verifier/composer + matrices remain deliberately excluded. The included hardening is limited to + confirmed local behavior/contract gaps with direct deterministic tests. diff --git a/specs/plans/README.md b/specs/plans/README.md index 6edca87..a7117e2 100644 --- a/specs/plans/README.md +++ b/specs/plans/README.md @@ -98,7 +98,7 @@ This directory tracks implementation plans for confirmed improvements. Status va | 92 | IMPLEMENTED (measuring) | Issue 92: Planner Coverage Calibration and the Adaptive Second Pass | | 93 | PENDING | Issue 93: Delete the Dead Ripgrep Fast Path (D9) | | 94 | PENDING | Issue 94: One Shared Similarity/Util Module | -| 95 | PENDING | Issue 95: One Shared Submit/Salvage Layer + Prompt "Why" Ledger | +| 95 | COMPLETE | Issue 95: One Shared Submit/Salvage Layer + Prompt "Why" Ledger | | 96 | PENDING | Issue 96: Fixed Stage-6 Symbol-Context Budget | | 97 | IMPLEMENTED (dogfood pending) | Issue 97: GitHub Action Integration (comment-triggered PR review + live status comment) | | 98 | COMPLETE | Issue 98: Language Support — Rust, Python, Solidity, JavaScript (tree-sitter adapters + bundled skills) | @@ -113,6 +113,10 @@ This directory tracks implementation plans for confirmed improvements. Status va | 108 | COMPLETE | Issue 108: Add a Verifier Severity Rubric and Revision Telemetry | | 109 | COMPLETE | Issue 109: Restore Summary-Only Publication for Verified Low-Confidence Deltas | | 110 | COMPLETE | Issue 110: Make Note Fallback Publication-Aware and Score Rendered Notes | +| 111 | IMPLEMENTED (dogfood pending) | Issue 111: Fix Observed Structured-Submit Failures and Preserve Safe Diagnostics | +| 112 | IMPLEMENTED (measuring) | Issue 112: Enforce Final Structured-Submit Provenance from Pi Events | +| 113 | BACKLOG (measurement gate not met) | Issue 113: Measure Direct-Reject Note Contradictions Before Adding Suppression | +| 114 | COMPLETE | Issue 114: Re-execute Untrusted Structured Submits From Trusted Context | ## Recommended order for 106-110 @@ -120,3 +124,40 @@ Land the terminal visibility chain first: **106 -> 109 -> 110**. Plans 107 and 108 are lower-risk supporting improvements; both follow 106 and may land after the visibility chain. Plan 108 must rebase its Stage-9 prompt version onto the landed 106 version. + +## Recommended order for 111-112 + +Implement **111 first**: it contains the observed production/eval fixes and has +no Pi release dependency. Implement **112 second**: it consumes Pi's public +event stream inside Codegenie's existing adapter, strictly verifies final +arguments, and measures whether any broader lossless syntax repair or upstream +Pi change deserves a new plan. Its implementation gate uses generic event-shape +tests plus only the existing Anthropic/OpenAI Responses smokes; do not create an +every-provider fixture matrix without evidence. After that gate, move its row +to `IMPLEMENTED (measuring)`. The 2,500-submit corpus is post-land, not a reason +to hold the PR open; move the row to `COMPLETE` after the corpus and +human-reviewed outcome. Do not implement conditional parser work unless Plan +112's written evidence gate fires. + +## Recommended disposition for 113 + +Do not hold 0.5.5 for Plan 113. Run 72 is the sole confirmed stale-note +incident; run 73 is a negative control in which the same candidate/note shape +was correctly reconciled by the existing evidence-backed path. The current +high-risk-reject tuple is also ambiguous with evidence-unavailable rejects, so +same-packet fuzzy suppression is not authorized. Reopen design only after the +plan's repeated-evidence or production-evidence gate fires; until then, leave +113 in BACKLOG and make no source changes. + +## Recommended order for 114 + +Implement **114 before releasing 0.5.5**. Plans 111 and 112 remain its landed +dependencies: 111 owns stage-local terminal disposition and bounded Action +diagnostics; 112 owns strict final-argument provenance and the non-executable +untrusted-call representation. Plan 114 does not weaken either contract. It +corrects the continuation after an untrusted submit by retaining the trusted +pre-response conversation for the existing one retry. It also includes the +already-confirmed cache-aggregate and bounded Action identity fixes plus four +direct contract-test corrections; each is local and adds no new mechanism. +New telemetry fields, stage-specific test matrices, and Plan 113 remain +independent and must not be pulled into the 0.5.5 release. diff --git a/specs/project/architecture.md b/specs/project/architecture.md index 77af8fb..6ee66a5 100644 --- a/specs/project/architecture.md +++ b/specs/project/architecture.md @@ -1206,7 +1206,7 @@ Responsibilities: - Record per-call telemetry: model, provider, duration, token usage, prompt hash, output hash, and schema validation result. - Enforce `llm.maxConcurrentCalls`. -Structured output strategy: every structured stage call uses a forced submit tool. The stage's output schema is exposed as a tool (e.g. `submit_plan`, `submit_review`, `submit_verdict`, `submit_composition`); the model finishes by calling it; the adapter validates the tool arguments against the schema. This composes with read-only repository tools attached to the same call. There is no reliance on a provider response_format. +Structured output strategy: every structured stage call uses a forced submit tool. The stage's output schema is exposed as a tool (e.g. `submit_plan`, `submit_review`, `submit_verdict`, `submit_composition`); the model finishes by calling it; the adapter validates the tool arguments against the schema. Before validation, the production adapter consumes Pi's public stream and accepts the named submit arguments only when the complete public event representation parses strictly (or through Pi's narrow string repair) and deep-equals Pi's final value. This composes with read-only repository tools attached to the same call. There is no reliance on a provider response_format. Schema system: LLM input/output schemas are authored in TypeBox (pi-ai's native schema system), with static types derived via `Static`. zod remains for config validation only. @@ -1376,6 +1376,18 @@ Failure and budget handling: - Provider 429 and transient 5xx responses get up to 3 retries with exponential backoff; retries count against budgets. - The run-level coverage status is owned by the orchestrator, which aggregates plan-time coverage, runtime failures, budget stops, and verification incompleteness into the final coverage summary (run-level, not only `ReviewPlan.partialReview`): +Content-stage invalidity is binary: known-invalid model data is never used. A stage continues only from independently trusted work or an existing deterministic fallback. + +| Terminal condition after the one repair | Trusted state that remains | Required disposition | +| --- | --- | --- | +| Stage-5 planner invalid | Parsed diff/hunk inventory and deterministic default coverage | Discard every planner field; continue with `degradedPlanning: true`. | +| Stage-7 packet invalid | Successful packet results; failed packet hunks are known gaps | Discard the failed packet; mark its hunks incomplete/partial. | +| Stage-9 verdict invalid or semantic empty revise | Other completed verdicts; this candidate is unverified | Suppress the candidate and increment verification-incomplete coverage. | +| Stage-10 composition invalid | Already verified findings | Use deterministic composition; do not drop verified findings. | +| Diff/base/repository identity, authentication, provider-wide availability, or another non-isolatable foundation is untrusted | No trustworthy review foundation | Fail the run and leave the scrubbed failure artifact. | + +Publication is equally explicit: normal complete reviews publish normally; complete reviews using planner fallback carry a prominent degraded banner; partial reviews carry a prominent incomplete banner and publish only trusted findings; partial reviews with no findings say `Review Incomplete`, never `Everything looks good`; foundational failures never render a normal review. + ```ts type RunCoverageStatus = { totalHunks: number @@ -1892,6 +1904,10 @@ Prompt construction: untrusted content must be structurally delimited in prompts Output channel control: everything posted to GitHub passes deterministic sanitization (see Output And GitHub Publishing). Telemetry/debug artifacts contain untrusted content by design and are local-only. +Final submit boundary: Pi stream fragments are ephemeral. Codegenie does not parse provider wire formats, accept Pi's partial streaming parser as final provenance, or persist accumulated argument text/parser messages/value hashes. An untrusted named submit is represented locally without `arguments`; its assistant response never enters provider conversation history or cache. The runner retains the independently constructed conversation that preceded that response, appends bounded repair guidance, and may perform one forced-submit clean re-execution. This rare retry can resend more trusted input context, and its cost is measured by existing per-call telemetry. A normalized `stopReason: "length"` wins; provider-specific raw stop reasons are not reinterpreted. + +Public schema-failure diagnostics are safe-by-construction projections: stage, role, submit tool/schema version, attempt, a closed bounded classification, and schema-owned path/rule/limit fields only. Raw validator payloads, arbitrary error context, repository text, prompts, arguments, and original validator causes never cross into Action comments, logs, or failure artifacts. + Repository tools path containment (single chokepoint in the RepositoryTools layer): all paths are canonicalized as repository-relative tree paths; absolute paths and `..` traversal are rejected with a typed error (`path_outside_repo`); git-plumbing reads are inherently contained to repository object paths; refs are harness-resolved only (model-facing source selectors expose `head`/`base`), and harness-side ref values are validated against `git check-ref-format` rules and rejected if option-like (leading `-`). Config trust partitioning: the per-key config-source table in CLI And Config is normative. Repo `codegenie.toml` may set only the repo-settable safe keys listed there, including the positive-minute `review.maxTime` run bound; every other key takes effect only with user-level opt-in — a CLI flag, `~/.codegenie/settings.json`, or the user-scoped config file `~/.codegenie/config.toml` (all under `CODEGENIE_HOME`) — and repo-config values for user-scope keys are ignored with a warning. Repo-config-relative paths are constrained to the repo root. diff --git a/specs/project/components/repository_and_github.md b/specs/project/components/repository_and_github.md index 83fc8cd..248509f 100644 --- a/specs/project/components/repository_and_github.md +++ b/specs/project/components/repository_and_github.md @@ -664,6 +664,8 @@ Every attempt, demotion, and the final status is recorded in `RunPostingRecord` The **status comment** is one issue comment per PR carrying the `` marker: created (or reclaimed from a prior run) before the review starts, edited in place at stage boundaries (throttled, default minimum 10s between edits; after 3 consecutive edit failures the run continues headless), and terminally edited into the sanitized markdown report — capped to GitHub's 65,536-character comment limit with a truncation disclosure — or a short failure state. It is the explicitly-mutable exception to the never-mutate rule above. Reclaim requires the marker and an exact case-insensitive author match against identity resolved as `bot-login` → `/user` → `github-actions[bot]`; there is no `[bot]`-suffix fallback, so another app's marker is never adopted. Progress coalescing never polls an unresolved PATCH, and terminal states are absorbing. Bodies pass the comment sanitizer first, marker appended after, same ordering as finding markers. Lifecycle body-size fields measure UTF-8 bytes over the complete attempted payload including that marker, consistently before and after capping. The step-summary copy is sanitized as another GitHub-rendered surface; the downloadable report retains canonical secret-scrubbed Markdown. +On failure, the adapter always attempts to write the capped report (`CODEGENIE_REPORT_PATH`, 4 KiB) and a schema-versioned JSON identity (`CODEGENIE_FAILURE_PATH`, 16 KiB), even with telemetry disabled; both are uploaded under `if: always()`. These public surfaces use only allowlisted stage/tool/attempt/classification and schema-owned issue path/rule/limit fields, never arbitrary error messages/context or Pi's received arguments. Artifact-write failure never replaces the original review error. + **Viewer identity:** Actions installation tokens have no `/user` context, so `gh api user` fails there. The adapter resolves its identity in strict order — explicit `bot-login` action input (custom GitHub Apps) → `/user` lookup (PATs) → `github-actions[bot]` — and that exact login (case-insensitive) gates status-comment reclaim; suffix heuristics are forbidden so another app's marker comment is never adopted. Only the known installation-token `/user` limitation falls back; unrelated auth/infrastructure errors surface. The resolved login is injected via `CODEGENIE_GITHUB_LOGIN`, which `loadViewerLogin` uses only when `gh api user` fails; the author read back from a just-created status comment overrides the resolved login as ground truth. Duplicate detection is otherwise unchanged. The adapter's terminal comment carries the full `renderMarkdownReview` report (stdout in posting mode is the posting summary and goes to the CI log). Telemetry stays off by default; bounded decision/lifecycle records always reach the CI log, and `github-action.json` is persisted best-effort only when an attached telemetry run directory exists. ## Dependencies diff --git a/specs/project/components/review_pipeline.md b/specs/project/components/review_pipeline.md index 54853aa..f624c9a 100644 --- a/specs/project/components/review_pipeline.md +++ b/specs/project/components/review_pipeline.md @@ -207,6 +207,8 @@ interface WorkerRunner { | Soft budget exhaustion | Degradation ladder; partial disclosure; exit 0 | | 2x runtime budget | Fatal `timeout`; best-effort telemetry flush | +These dispositions never consume fields from the invalid response. Complete planner-fallback reviews render a prominent degraded banner. Partial output renders a prominent incomplete banner; if nothing publishable remains it concludes `Review Incomplete` and never uses approval-equivalent wording such as `Everything looks good.` + ## Internal Design ### Stage Sequencing And Data Flow @@ -618,9 +620,9 @@ Verdict handling (`VerificationVerdict` per candidate): - `revise` → the provider-safe flat submit schema keeps both compatibility payloads optional, then runtime semantic validation requires a non-empty structured change: either `finalFinding` or `revisedAnchor`. `finalFinding` must preserve the candidate id (contract); a `revisedAnchor` is accepted only if it validates against a changed diff line, otherwise the original validated anchor is preserved; a real-but-unanchorable issue may revise through a complete `finalFinding` and proceed as summary-only. Severity/confidence/wording/fix/test narrowing is accepted as submitted; lineage is preserved. An empty revise is recorded as incomplete with `verification_semantic_invalid`, never as a verifier rejection. - Verdicts referencing unknown candidate ids are discarded with telemetry. -Failure rules: authentication or provider-wide failures fail the run or mark the review incomplete (fatal per the global policy). Individual schema/parse failures get the one repair attempt. The stateless repair prompt carries a bounded, untrusted-data-fenced projection of candidate semantics and evidence; repository-tool results from the discarded response are explicitly not represented. An empty primary `{}` is classified as `empty_submit_object`; even a schema-valid repair is discarded and persisted incomplete because it cannot preserve the primary adjudication state. Empty revise remains a runtime semantic failure (`revise_without_revision_payload`). `verification_primary_submit_accepted` is emitted only for a non-empty, semantically complete, unrepaired primary verdict and carries submit schema version 3, so provider smoke acceptance cannot be inferred from HTTP acceptance or repair success alone. Candidates still unverified are marked `verificationIncomplete: true`, suppressed from publication by default, and counted into `RunCoverageStatus.verificationIncompleteCount`. When `review.verify === false` (explicit configuration only), gates 1-6 still run, the LLM verifier is skipped, gate-surviving candidates pass through as the verified set, and the coverage summary discloses that verification was skipped. +Failure rules: authentication or provider-wide failures fail the run or mark the review incomplete (fatal per the global policy). Individual schema/parse failures get the one repair attempt. The stateless repair prompt carries a bounded, untrusted-data-fenced projection of candidate semantics and evidence; repository-tool results from the discarded response are explicitly not represented. An empty primary `{}` is classified as `empty_submit_object`; even a schema-valid repair is discarded and persisted incomplete because it cannot preserve the primary adjudication state. Empty revise is a post-schema semantic failure (`revise_without_revision_payload`) and shares the same one-repair/cache-validity path. `verification_primary_submit_accepted` is emitted only for a non-empty, semantically complete, unrepaired primary verdict and carries submit schema version 4, so provider smoke acceptance cannot be inferred from HTTP acceptance or repair success alone. Verifier reasons target 2,000 characters in prompts and accept up to the 4,000-character hard maximum unchanged; accepted target overflow is telemetry-visible. Candidates still unverified are marked `verificationIncomplete: true`, suppressed from publication by default, and counted into `RunCoverageStatus.verificationIncompleteCount`. When `review.verify === false` (explicit configuration only), gates 1-6 still run, the LLM verifier is skipped, gate-surviving candidates pass through as the verified set, and the coverage summary discloses that verification was skipped. -The Stage-9 `p9.8` prompt makes confidence follow decisive verified evidence rather than inherited generation confidence or pressure from a secondary lookup. A confirmed low-confidence promoted predicate returns a complete calibrated `finalFinding`; unresolved secondary checks do not by themselves hold confidence low, while speculative reachability, ambiguous intent, or weak path matching still do. Its severity rubric is impact-based: low is bounded or localized, medium is material but limited, high is broad or serious user/system impact, and critical is catastrophic impact or a security-boundary compromise. Severity measures magnitude and reach rather than the mere existence of a technical invariant violation; a verifier changing severity by more than one level must quantify the concrete impact bound in the revised verification text. +The Stage-9 `p9.9` prompt makes confidence follow decisive verified evidence rather than inherited generation confidence or pressure from a secondary lookup. A confirmed low-confidence promoted predicate returns a complete calibrated `finalFinding`; unresolved secondary checks do not by themselves hold confidence low, while speculative reachability, ambiguous intent, or weak path matching still do. Its severity rubric is impact-based: low is bounded or localized, medium is material but limited, high is broad or serious user/system impact, and critical is catastrophic impact or a security-boundary compromise. Severity measures magnitude and reach rather than the mere existence of a technical invariant violation; a verifier changing severity by more than one level must quantify the concrete impact bound in the revised verification text. Every complete verifier `finalFinding` revision passively records `severityRevision: { original, submitted, applied, deltaLevels }` on the persisted verdict. `deltaLevels` is signed from the input candidate to the submitted severity, while `applied` reflects the existing behavior-change policy; the audit record never changes severity. The bounded `verification_severity_revision` event carries the same fields plus candidate id, category, and behavior change, at info level for increases of two or more levels and debug otherwise. Future deterministic calibration policy requires a fresh plan based on at least 20 runs spanning the `49f4645b`, `0c4d5213`, and `relay-wc` eval cases, and only after either three unquantified multi-level increases or one operator-confirmed severe user-facing calibration failure. diff --git a/specs/project/components/skills_llm_telemetry.md b/specs/project/components/skills_llm_telemetry.md index 8fb6634..4bebfb2 100644 --- a/specs/project/components/skills_llm_telemetry.md +++ b/specs/project/components/skills_llm_telemetry.md @@ -17,7 +17,7 @@ This component is responsible for: - The lens registry: lens existence derived from loaded skills (`lens exists iff at least one loaded skill declares it`), `enabledByDefault` conflict resolution, `--lens` validation with an available-lens error listing, config precedence for the effective enabled-lens set, and one-line lens summaries for the planner dossier. - The prompt builder: the four stage prompt templates (stages 5, 7, 9, 10), the deterministic dossier renderer `renderDossier` (also called by the pipeline's dossier compaction for size estimation), per-stage skill projection maps with the 4000-char per-skill and 12000-char total caps, telemetry-recorded truncation, and the untrusted-content fencing required by Trust Boundaries. - The provider/auth command layer: `codegenie provider ...`, `~/.codegenie/` path resolution, Pi provider/model registry access, login/logout/auth status, model listing, user-level default provider/model/depth/reasoning settings, and credential registration with the redaction layer. -- The `LlmRunner` implementation (`PiRunner`): forced submit-tool structured outputs per stage (`submit_plan`, `submit_review`, `submit_verdict`, `submit_composition`), TypeBox schema authoring with `Static<>` type derivation, the codegenie-owned agent loop driving pi-ai `complete()` + `validateToolCall`, `ToolBudget` enforcement, `AbortController` timeouts chained to the run-wide abort, one schema-repair retry, single run-wide model resolution from the resolved `llm` config (`llm.model ?? Pi/provider default`, already merged from CLI/environment/user settings by the config loader), reasoning-effort resolution from the resolved `llm.reasoning` with the built-in `high` default, `llm.maxConcurrentCalls` enforcement, 429/transient-5xx exponential backoff with budget accounting, and per-call telemetry — including one law `ToolCallRecord` emitted to the recorder for every tool-loop call (executed, budget-rejected, or containment-rejected), stamped with `initiator: "model"` and the issuing `modelCallId`. +- The `LlmRunner` implementation (`PiRunner`): forced submit-tool structured outputs per stage (`submit_plan`, `submit_review`, `submit_verdict`, `submit_composition`), TypeBox schema authoring with `Static<>` type derivation, the codegenie-owned agent loop driving Pi public streams + final submit provenance + `validateToolCall`, `ToolBudget` enforcement, `AbortController` timeouts chained to the run-wide abort, one schema-repair retry, single run-wide model resolution from the resolved `llm` config (`llm.model ?? Pi/provider default`, already merged from CLI/environment/user settings by the config loader), reasoning-effort resolution from the resolved `llm.reasoning` with the built-in `high` default, `llm.maxConcurrentCalls` enforcement, 429/transient-5xx exponential backoff with budget accounting, and per-call telemetry — including one law `ToolCallRecord` emitted to the recorder for every tool-loop call (executed, budget-rejected, or containment-rejected), stamped with `initiator: "model"` and the issuing `modelCallId`. - The delegated `ToolDefinition` type and the factory that wraps `RepositoryTools` methods as model-facing tool definitions, including rendering tool rejections as model-visible errors. - The local model-call cache: normalized-request key derivation, per-provider-call caching with conversation-prefix keying, cache-schema-version validation on read, refusal of repo-tracked cache directories, 14-day / 500MB eviction at run start, and hit/miss/write telemetry. - The logger and telemetry recorder: `run.log` and `events.jsonl` writing, level filtering, stderr mirroring, stage `0` pre-pipeline event buffering, and monotonic event ids. @@ -676,12 +676,12 @@ Stage-to-submit-tool mapping (the tool name the model must call; the schema trav Schema authoring rules: - Every schema sets `additionalProperties: false` so hallucinated fields fail validation rather than passing silently. -- String fields carry `maxLength` guards (titles 200, bodies 10000, evidence 4000) so a runaway model cannot bloat artifacts; over-length submissions are schema-invalid and go through the repair path. +- String fields carry `maxLength` guards (titles 200, bodies 10000, evidence 4000) so a runaway model cannot bloat artifacts; over-length submissions are schema-invalid and go through the repair path. Stage 9's verifier reason has a model-facing 2,000-character target and a 4,000-character hard maximum; accepted text inside that buffer is preserved unchanged and target overflow is counted. - `SCHEMA_VERSIONS: Record` exports a version per schema; versions feed the cache key and bump on any schema change. ### The Agent Loop -`PiRunner.runStructured` owns the agent loop. Per `architecture.md`, the agent loop is implemented inside the pi-runner, behind `LlmRunner.runStructured`: pi-ai's own `agentLoop` is not used; the runner drives `complete()` + `validateToolCall` per step. The pipeline's worker runner schedules workers and supplies the budget through `LlmStructuredRequest`; it does not run the loop itself (`components/review_pipeline.md`). +`PiRunner.runStructured` owns the agent loop. Per `architecture.md`, the agent loop is implemented inside the pi-runner, behind `LlmRunner.runStructured`: pi-ai's own `agentLoop` is not used. The production adapter consumes `Models.stream()` / `streamSimple()` while preserving the runner's single `Promise` seam, and the runner then drives final provenance + `validateToolCall` per step. The pipeline's worker runner schedules workers and supplies the budget through `LlmStructuredRequest`; it does not run the loop itself (`components/review_pipeline.md`). Setup per request: @@ -694,14 +694,14 @@ Setup per request: Loop, repeated until terminal: 1. Budget checkpoint: call `hooks.checkpoint(request.stage)` before every provider call. On `"exhausted"`, enter finalization (below) instead of a normal completion. -2. Provider call: acquire the `llm.maxConcurrentCalls` semaphore, check the cache (key over the full conversation prefix; see Cache Key Derivation), and on miss call pi-ai `complete()` with messages, tools, model, and the abort signal. Tool choice is `"auto"` while investigation is permitted; for stages with no repository tools (5, 10) and for all finalization calls it forces the submit tool. Release the semaphore when the call settles; report `hooks.onUsage` and `telemetry.recordModelCall` for every attempt. +2. Provider call: acquire the `llm.maxConcurrentCalls` semaphore, check the cache (key over the full conversation prefix; see Cache Key Derivation), and on miss consume Pi's public stream with the same messages, tools, model, and abort signal that completion used. Tool choice is `"auto"` while investigation is permitted; for stages with no repository tools (5, 10) and for all finalization calls it forces the submit tool. The adapter accumulates only the named stage-submit deltas, requires complete framing, strict JSON or Pi's narrow string repair, one object root, and deep equality with Pi's final value; normalized length stops and missing/divergent/partial/invalid captures become non-executable local calls without `arguments`. Release the semaphore when the call settles; report `hooks.onUsage` and `telemetry.recordModelCall` for every attempt. 3. Response handling, in order: - - Submit-tool call present: validate its arguments against `request.schema` via pi-ai `validateToolCall`. Valid → resolve `runStructured` with the typed payload; any other tool calls in the same response are ignored with a telemetry note (`submit_with_extra_tools`). Invalid → schema repair (below). + - Submit-tool call present: count it under the existing stage discipline. A trusted call proceeds through pi-ai `validateToolCall` and the optional pure semantic validator. An untrusted/provenance-less call cannot be executed or normalized: its entire assistant response is discarded, the independently constructed prior conversation is retained, and the same one-repair scheduler appends bounded state guidance for a clean forced-submit re-execution. Valid schema and semantics resolve `runStructured`; any other tool calls in the same response are ignored with a telemetry note (`submit_with_extra_tools`). - Repository tool calls present (no submit): if `roundsUsed >= maxInvestigationRounds`, skip execution and enter finalization with a budget notice. Otherwise increment `roundsUsed` and execute the requested tool calls sequentially in response order. For each: if `toolCallsUsed >= maxToolCalls` or `resultCharsUsed >= maxResultChars`, append a budget-exhausted tool error result (a `ToolCallRecord` with `status: "rejected"`) instead of executing; else validate arguments (`validateToolCall` against `parameters`; invalid arguments render as `isError` results without execution), invoke `definition.execute(args, signal)`, cap the appended text at the remaining `maxResultChars`, wrap it in untrusted fencing with a one-line framing prefix, and append it as the tool-result message. Update counters; every requested call — executed, budget-rejected, or containment-rejected — emits one law `ToolCallRecord` via `recordToolCall` (stamped `initiator: "model"`, the issuing step's `modelCallId`, and normalized args, with `ToolResultMeta` passthrough) plus the per-call debug trace. Then continue the loop. - Plain text with no tool calls: append a one-line nudge message (versioned constant) instructing the model to call the submit tool or a repository tool. At most one nudge per request; a second text-only response enters finalization. 4. Finalization (entered on tool-call budget exhaustion, round exhaustion, ledger-checkpoint exhaustion, or the post-nudge text response): append a versioned finalize message — investigation is over; call the submit tool now using only evidence already gathered — and make one completion with tool choice forced to the submit tool. A valid submit resolves the request. An invalid submit gets the one schema repair. Anything else rejects `llm_schema_invalid`. -Schema repair (one attempt per request, all stages): append a repair message containing the submit tool's validation errors (TypeBox error paths and messages, truncated to 2000 chars) and an instruction to call the submit tool again with corrected arguments only, then make one completion with tool choice forced to the submit tool. Valid → resolve. Invalid → reject `llm_schema_invalid` (recoverable; the worker layer's re-dispatch policy is pipeline-owned). +Schema repair (one attempt per request, all stages): append or replace with a repair message containing bounded validation identity and an instruction to call the submit tool again with corrected arguments only, then make one call with tool choice forced to the submit tool. Schema and semantic failures share this budget and cache-validity gate. For untrusted final-argument calls, discard the invalid assistant response, expose no arguments, retain the valid prior conversation, and append the existing bounded repair message. This context-preserving retry can consume more input tokens than replacement mode on the rare failure path; existing per-call telemetry measures it. Valid → resolve. Invalid → reject `llm_schema_invalid` (recoverable; the worker layer's stage-local disposition is pipeline-owned). Terminal public errors retain only the allowlisted structured diagnostic; Pi's value-bearing validator exception is never attached as cause/context. Loop invariants: @@ -739,12 +739,13 @@ Every provider call attempt and every cache hit produces one `LlmCallRecord` app - ids from `request.telemetryContext` — `workerId`, `packetId`, `candidateId` — are copied onto the record and onto every telemetry event the call emits, fulfilling the worker-traceability requirements. - Token usage comes from pi-ai's reported usage. `costUSD` is recorded when pi-ai or the provider reports cost or sufficient pricing metadata; when unavailable, `costUSD` is omitted and the call is disclosed in `cost-profile.json`'s unknown-cost call count. Cost is observability only in v1 — there is no cost budget, so unknown-cost calls interact with no budget (cost-based run budgets are deferred; see architecture.md Future Considerations). - Cache hits record `cacheStatus: "hit"`, the stored usage for visibility, `durationMs` of the lookup, and report no `hooks.onUsage` (no provider spend: cached replays consume no token or model-call budget and incur no provider cost; they are development/eval conveniences, not provider work). +- Named submit responses record one bounded `finalArgumentState`, optional closed error/repair kind, submit tool, and local correlation id. Warnings are emitted only for rejected values; repair outcomes are `recovered`, `terminal_invalid`, or `not_dispatched`. No event fragment, parser message, argument value, or value-derived hash is stored. -Aggregates maintained in memory and written at `finalize`, keyed by numeric stage (`role` is a derived display label, not an aggregation key): per-stage call counts, token sums, cost sums, retry counts, schema-repair counts, cache hit/miss/write counts (→ `model-calls-summary.json`); total/known/unknown cost, per-stage token/cost breakdown (→ `cost-profile.json`). +Aggregates maintained in memory and written at `finalize`, keyed by numeric stage (`role` is a derived display label, not an aggregation key): per-stage call counts, token sums, cost sums, retry counts, schema-repair counts, cache hit/miss/write counts, and final-argument state/error histograms (→ `model-calls-summary.json`); total/known/unknown cost, per-stage token/cost breakdown (→ `cost-profile.json`). Final-argument state/error histograms count provider calls only and exclude cache-hit replays; raw cache-hit records and cache counters remain available. ### Model-Call Cache -The cache is per provider call, never per task: in tool-using stages each model→tool→model step caches individually, keyed on the full conversation prefix, so a changed tool result invalidates only the steps after it. Whole-task results are never cached. +The cache is per provider call, never per task: in tool-using stages each model→tool→model step caches individually, keyed on the full conversation prefix, so a changed tool result invalidates only the steps after it. Whole-task results are never cached. The cache schema is version 2: historical provenance-less entries miss; only trusted submit calls retain bounded provenance alongside their already-validated arguments, and untrusted responses are never written. #### Cache Key Derivation diff --git a/src/github-action/entrypoint.ts b/src/github-action/entrypoint.ts index c6fffc0..d997066 100644 --- a/src/github-action/entrypoint.ts +++ b/src/github-action/entrypoint.ts @@ -5,7 +5,11 @@ import { getPiApiKeyEnvVarName } from "../provider/pi-ai-models.js"; import { renderMarkdownReview } from "../output/markdown-renderer.js"; import { sanitizeGitHubCommentBody, scrubGitHubSecrets } from "../github/comment-sanitizer.js"; import type { ReviewResult, TelemetryEvent } from "../types.js"; -import { CodegenieError, isCodegenieError } from "../util/errors.js"; +import { CodegenieError, isCodegenieError, type CodegenieErrorCode } from "../util/errors.js"; +import { + structuredSubmitFailureDiagnosticFromError, + type StructuredSubmitFailureDiagnostic +} from "../llm/schema-diagnostics.js"; import { DEFAULT_ALLOWED_ASSOCIATIONS, DEFAULT_TRIGGER_PHRASE, @@ -15,6 +19,7 @@ import { } from "./event-gate.js"; import { createIssueCommentClient, type IssueCommentClient } from "./issue-comments.js"; import { createStatusCommentController } from "./status-comment.js"; +import { renderStructuredSubmitFailure } from "./render.js"; type ProgressEvent = Omit; @@ -202,9 +207,18 @@ export async function executeGitHubActionCommand( writeOutput: write }); } catch (error) { - const code = error instanceof CodegenieError ? error.code : error instanceof Error ? error.name : "unknown_error"; - await controller.finalizeFailure(code); + const code = actionErrorCode(error); + const diagnostic = structuredSubmitFailureDiagnosticFromError(error); + publishFailureFiles({ + errorCode: code, + decision: authorized, + env, + ...(diagnostic !== undefined ? { diagnostic } : {}), + ...(runUrl !== undefined ? { runUrl } : {}) + }); + await controller.finalizeFailure(code, diagnostic); emitActionRecord(attachment?.runDir, eventName, authorized, "review_failed", controller.stats(), env, write, code); + write(`github-action: review failed — ${diagnostic !== undefined ? renderStructuredSubmitFailure(diagnostic) : code}\n`); throw error; } @@ -214,7 +228,7 @@ export async function executeGitHubActionCommand( try { await controller.finalizeSuccess(runResult.reportMarkdown); } catch (error) { - const code = error instanceof CodegenieError ? error.code : error instanceof Error ? error.name : "unknown_error"; + const code = actionErrorCode(error); emitActionRecord(runResult.runDir, eventName, authorized, "terminal_post_failed", controller.stats(), env, write, code); throw error; } @@ -408,6 +422,93 @@ function publishReportFiles(report: string, env: NodeJS.ProcessEnv): void { } } +type ActionFailureRecord = { + schemaVersion: 1; + lane: AuthorizedDecision["lane"]; + prNumber: number; + errorCode: CodegenieErrorCode | "unknown_error"; + runUrl?: string; + runId?: string; + structuredSubmitFailure?: StructuredSubmitFailureDiagnostic; +}; + +const FAILURE_JSON_MAX_BYTES = 16 * 1024; +const FAILURE_MARKDOWN_MAX_BYTES = 4 * 1024; + +function actionErrorCode(error: unknown): CodegenieErrorCode | "unknown_error" { + return error instanceof CodegenieError ? error.code : "unknown_error"; +} + +function publishFailureFiles(input: { + errorCode: CodegenieErrorCode | "unknown_error"; + diagnostic?: StructuredSubmitFailureDiagnostic; + decision: AuthorizedDecision; + runUrl?: string; + env: NodeJS.ProcessEnv; +}): void { + const runId = input.env.GITHUB_RUN_ID; + const record: ActionFailureRecord = { + schemaVersion: 1, + lane: input.decision.lane, + prNumber: input.decision.prNumber, + errorCode: input.errorCode, + ...(input.runUrl !== undefined ? { runUrl: input.runUrl } : {}), + ...(runId !== undefined && /^\d+$/u.test(runId) ? { runId } : {}), + ...(input.diagnostic !== undefined ? { structuredSubmitFailure: input.diagnostic } : {}) + }; + const json = fitFailureJson(record); + const markdown = fitFailureMarkdown([ + "# 🧞 Codegenie Review Failed", + "", + `Error code: \`${input.errorCode}\``, + ...(input.diagnostic !== undefined ? ["", renderStructuredSubmitFailure(input.diagnostic)] : []), + ...(input.runUrl !== undefined ? ["", `See the [workflow job](${input.runUrl}) and the failure JSON artifact.`] : []) + ].join("\n")); + writeFailureFile(input.env.CODEGENIE_FAILURE_PATH, json); + writeFailureFile(input.env.CODEGENIE_REPORT_PATH, markdown); + const stepSummary = input.env.GITHUB_STEP_SUMMARY; + if (stepSummary !== undefined && stepSummary !== "") { + try { + appendFileSync(stepSummary, `${sanitizeGitHubCommentBody(markdown).trimEnd()}\n`); + } catch { + // The original review failure remains authoritative. + } + } +} + +function fitFailureJson(record: ActionFailureRecord): string { + let candidate = record; + let serialized = `${JSON.stringify(candidate, null, 2)}\n`; + if (Buffer.byteLength(serialized, "utf8") <= FAILURE_JSON_MAX_BYTES) { + return serialized; + } + candidate = { ...record, ...(record.structuredSubmitFailure !== undefined + ? { structuredSubmitFailure: { ...record.structuredSubmitFailure, issues: [] } } + : {}) }; + serialized = `${JSON.stringify(candidate, null, 2)}\n`; + return Buffer.byteLength(serialized, "utf8") <= FAILURE_JSON_MAX_BYTES + ? serialized + : `${JSON.stringify({ schemaVersion: 1, lane: record.lane, prNumber: record.prNumber, errorCode: record.errorCode }, null, 2)}\n`; +} + +function fitFailureMarkdown(markdown: string): string { + 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`; +} + +function writeFailureFile(filePath: string | undefined, contents: string): void { + if (filePath === undefined || filePath === "") { + return; + } + try { + writeFileSync(filePath, contents); + } catch { + // The original review failure remains authoritative. + } +} + type DecisionRecord = | { eventName: string; run: false; reason: string } | { diff --git a/src/github-action/render.ts b/src/github-action/render.ts index 7d4cc88..e53be82 100644 --- a/src/github-action/render.ts +++ b/src/github-action/render.ts @@ -1,4 +1,5 @@ import { STAGES, type ReviewStageNumber } from "../review-stages.js"; +import type { StructuredSubmitFailureDiagnostic } from "../llm/schema-diagnostics.js"; export type StageState = "pending" | "active" | "done"; @@ -67,14 +68,25 @@ export function renderProgressBody(checklist: StageChecklist, runUrl: string | u ].join("\n"); } -export function renderFailureBody(errorCode: string, runUrl: string | undefined): string { +export function renderFailureBody( + errorCode: string, + runUrl: string | undefined, + diagnostic?: StructuredSubmitFailureDiagnostic +): string { return [ `**🧞 Codegenie** review failed (\`${errorCode}\`).`, + ...(diagnostic !== undefined ? ["", renderStructuredSubmitFailure(diagnostic)] : []), "", ...renderRunLinkFooter(runUrl) ].join("\n"); } +export function renderStructuredSubmitFailure(diagnostic: StructuredSubmitFailureDiagnostic): string { + const issue = diagnostic.issues[0]; + const issueText = issue !== undefined ? ` (\`${issue.path}\`: ${issue.rule})` : ""; + return `Stage ${diagnostic.stage} \`${diagnostic.submitTool}\` ${diagnostic.attempt} remained invalid (${diagnostic.classification})${issueText}.`; +} + export type CappedTerminalBody = { body: string; bodyBeforeCap: string; diff --git a/src/github-action/status-comment.ts b/src/github-action/status-comment.ts index 44fc2a5..e229455 100644 --- a/src/github-action/status-comment.ts +++ b/src/github-action/status-comment.ts @@ -1,5 +1,6 @@ import { Buffer } from "node:buffer"; import type { TelemetryEvent } from "../types.js"; +import type { StructuredSubmitFailureDiagnostic } from "../llm/schema-diagnostics.js"; import { sanitizeGitHubCommentBody } from "../github/comment-sanitizer.js"; import { CodegenieError } from "../util/errors.js"; import type { IssueComment, IssueCommentClient } from "./issue-comments.js"; @@ -30,7 +31,7 @@ export type StatusCommentController = { claim(): Promise<{ commentId: number; author: string }>; onTelemetryEvent(event: ProgressEvent): void; finalizeSuccess(reportMarkdown: string): Promise; - finalizeFailure(errorCode: string): Promise; + finalizeFailure(errorCode: string, diagnostic?: StructuredSubmitFailureDiagnostic): Promise; settle(): Promise; stats(): StatusCommentStats; }; @@ -220,13 +221,13 @@ export function createStatusCommentController(options: StatusCommentOptions): St stats.editCount += 1; } - async function finalizeFailure(errorCode: string): Promise { + async function finalizeFailure(errorCode: string, diagnostic?: StructuredSubmitFailureDiagnostic): Promise { if (commentId === undefined) { return false; } terminal = true; await settle(); - const body = appendStatusCommentMarker(renderFailureBody(errorCode, options.runUrl)); + const body = appendStatusCommentMarker(renderFailureBody(errorCode, options.runUrl, diagnostic)); stats.terminalState = "failure"; const bodyBytes = Buffer.byteLength(body, "utf8"); stats.finalBodyBytes = bodyBytes; diff --git a/src/llm/final-tool-arguments.ts b/src/llm/final-tool-arguments.ts new file mode 100644 index 0000000..be0e54a --- /dev/null +++ b/src/llm/final-tool-arguments.ts @@ -0,0 +1,224 @@ +import { repairJson, type AssistantMessageEvent } from "@earendil-works/pi-ai"; +import { isDeepStrictEqual } from "node:util"; +import type { + PiAssistantMessage, + PiInvalidToolCall, + PiToolCall, + PiTrustedArgumentParse, + PiUntrustedArgumentParse +} from "./llm-runner.js"; + +type PublicAssistantEventStream = AsyncIterable; + +type Capture = { + started: boolean; + ended: boolean; + ambiguous: boolean; + sawDelta: boolean; + text: string; + endCall?: PiToolCall; +}; + +export type FinalToolArgumentTestHooks = { + onBuffersCleared?(remainingChars: number): void; +}; + +/** + * Consume Pi's public stream and establish final-argument provenance for one + * named stage submit tool. Argument fragments remain local to this call and + * are cleared before it returns or throws. + */ +export async function consumeFinalToolArguments( + stream: PublicAssistantEventStream, + submitToolName: string, + hooks: FinalToolArgumentTestHooks = {} +): Promise { + const captures = new Map(); + let terminal: PiAssistantMessage | undefined; + + try { + for await (const event of stream) { + if (event.type === "toolcall_start") { + const existing = captures.get(event.contentIndex); + if (existing !== undefined) { + existing.ambiguous = true; + } else { + captures.set(event.contentIndex, emptyCapture(true)); + } + } else if (event.type === "toolcall_delta") { + const capture = captures.get(event.contentIndex) ?? emptyCapture(false); + if (!captures.has(event.contentIndex)) { + captures.set(event.contentIndex, capture); + } + if (capture.ended) { + capture.ambiguous = true; + } + capture.sawDelta = true; + capture.text += event.delta; + } else if (event.type === "toolcall_end") { + const capture = captures.get(event.contentIndex) ?? emptyCapture(false); + if (!captures.has(event.contentIndex)) { + captures.set(event.contentIndex, capture); + } + if (capture.ended) { + capture.ambiguous = true; + } + capture.ended = true; + capture.endCall = event.toolCall as PiToolCall; + } 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); + } finally { + for (const capture of captures.values()) { + capture.text = ""; + delete capture.endCall; + } + captures.clear(); + hooks.onBuffersCleared?.(0); + } +} + +function emptyCapture(started: boolean): Capture { + return { started, ended: false, ambiguous: false, sawDelta: false, text: "" }; +} + +function finalizeMessage( + message: PiAssistantMessage, + submitToolName: string, + captures: ReadonlyMap +): PiAssistantMessage { + const content = message.content.map((block, contentIndex) => { + if (!isPiToolCall(block) || block.name !== submitToolName) { + return block; + } + const parse = message.stopReason === "length" + ? { state: "length_stopped" } as const + : parseCapturedArguments(captures.get(contentIndex), block); + if (parse.state === "strict" || parse.state === "repaired") { + const argumentParse: PiTrustedArgumentParse = parse.state === "strict" + ? { state: "strict" } + : { state: "repaired", repairs: ["pi_narrow_string_repair"] }; + return { ...block, arguments: parse.value, argumentParse } satisfies PiToolCall; + } + return { + type: "invalidToolCall", + id: block.id, + name: block.name, + argumentParse: parse + } satisfies PiInvalidToolCall; + }); + return { ...message, content }; +} + +type ParsedCapture = + | { state: "strict"; value: Record } + | { state: "repaired"; value: Record } + | PiUntrustedArgumentParse; + +function parseCapturedArguments(capture: Capture | undefined, finalCall: PiToolCall): ParsedCapture { + if ( + capture === undefined || + !capture.started || + !capture.sawDelta || + !capture.ended || + capture.ambiguous || + capture.endCall === undefined || + capture.endCall.id !== finalCall.id || + capture.endCall.name !== finalCall.name + ) { + return { state: "event_capture_missing" }; + } + + let value: unknown; + let state: "strict" | "repaired" = "strict"; + try { + value = JSON.parse(capture.text); + } catch (strictCause) { + let repaired: string; + try { + repaired = repairJson(capture.text); + } catch { + return classifySyntaxFailure(strictCause, capture.text); + } + if (repaired === capture.text) { + return classifySyntaxFailure(strictCause, capture.text); + } + try { + value = JSON.parse(repaired); + state = "repaired"; + } catch { + return classifySyntaxFailure(strictCause, capture.text); + } + } + + if (!isRecord(value)) { + return { state: "invalid", errorKind: "non_object_root" }; + } + if (!isDeepStrictEqual(value, capture.endCall.arguments) || !isDeepStrictEqual(value, finalCall.arguments)) { + return { state: "event_final_mismatch" }; + } + return { state, value }; +} + +function classifySyntaxFailure(cause: unknown, text: string): PiUntrustedArgumentParse { + const message = cause instanceof SyntaxError ? cause.message : ""; + const completeness = jsonDelimiterCompleteness(text); + if (/unterminated/iu.test(message) || completeness === "unterminated_string") { + return { state: "partial", errorKind: "unterminated" }; + } + if (/unexpected end|end of json/iu.test(message) || completeness === "open_delimiter" || text.trim() === "") { + return { state: "partial", errorKind: "unexpected_end" }; + } + return { state: "invalid", errorKind: "invalid_syntax" }; +} + +function jsonDelimiterCompleteness(text: string): "balanced" | "open_delimiter" | "unterminated_string" { + const stack: string[] = []; + let inString = false; + let escaped = false; + for (const char of text) { + if (inString) { + if (escaped) { + escaped = false; + } else if (char === "\\") { + escaped = true; + } else if (char === '"') { + inString = false; + } + continue; + } + if (char === '"') { + inString = true; + } else if (char === "{" || char === "[") { + stack.push(char); + } else if (char === "}" || char === "]") { + stack.pop(); + } + } + if (inString) { + return "unterminated_string"; + } + return stack.length > 0 ? "open_delimiter" : "balanced"; +} + +function isPiToolCall(value: unknown): value is PiToolCall { + return Boolean( + value && + typeof value === "object" && + (value as { type?: unknown }).type === "toolCall" && + typeof (value as { id?: unknown }).id === "string" && + typeof (value as { name?: unknown }).name === "string" + ); +} + +function isRecord(value: unknown): value is Record { + return Boolean(value && typeof value === "object" && !Array.isArray(value)); +} diff --git a/src/llm/llm-runner.ts b/src/llm/llm-runner.ts index fb8ff15..5683c23 100644 --- a/src/llm/llm-runner.ts +++ b/src/llm/llm-runner.ts @@ -99,6 +99,7 @@ export type LlmStructuredRequest = { packetId?: string; candidateId?: string; }; + validateSubmit?(value: T): LlmSubmitSemanticValidation; schemaRepair?: { replaceConversation?: boolean; failAfterRepair?: boolean; @@ -115,12 +116,44 @@ export interface LlmRunner { runStructured(request: LlmStructuredRequest): Promise; } +export type LlmSubmitFailureClassification = + | "schema_invalid" + | "missing_submit" + | "multiple_submits" + | "revise_without_revision_payload" + | "length_stopped" + | "final_arguments_partial" + | "final_arguments_invalid" + | "event_capture_missing" + | "event_final_mismatch" + | "xml_parameter_bleed" + | "extra_finding_properties" + | "extra_top_level_properties" + | "missing_required_finding_fields" + | "invalid_enum_value" + | "string_too_long" + | "empty_no_findings_missing_fields" + | "unsafe_candidate_like_payload" + | "invalid_tool_arguments" + | "unknown"; + +export type LlmSubmitSemanticValidation = + | { ok: true } + | { ok: false; classification: LlmSubmitFailureClassification }; + export type LlmSchemaRepairInput = { stage: ReviewStage; submitTool: string; error: string; submitCalls: Array<{ id: string; arguments: Record }>; + untrustedSubmitCalls?: Array<{ + id: string; + name: string; + state: PiUntrustedArgumentParse["state"]; + errorKind?: "unexpected_end" | "unterminated" | "invalid_syntax" | "non_object_root"; + }>; extraToolNames: string[]; + classification?: LlmSubmitFailureClassification; }; export type LlmSchemaInvalidSubmitRecoveryInput = LlmSchemaRepairInput & { @@ -145,7 +178,7 @@ export type LlmInvalidSubmitRecovery = { recoveredCallId?: string; onRecovered?(recoveredCallId: string): void; onRejected?(error: string): void; - repairClassification?: string; + repairClassification?: LlmSubmitFailureClassification; replaceConversationOverride?: boolean; }; @@ -172,8 +205,30 @@ export type PiToolCall = { id: string; name: string; arguments: Record; + /** Present only on stage submit calls finalized by Codegenie's stream boundary. */ + argumentParse?: PiTrustedArgumentParse; }; +export type PiTrustedArgumentParse = + | { state: "strict" } + | { state: "repaired"; repairs: ["pi_narrow_string_repair"] }; + +export type PiUntrustedArgumentParse = + | { state: "length_stopped" } + | { state: "partial"; errorKind: "unexpected_end" | "unterminated" } + | { state: "invalid"; errorKind: "invalid_syntax" | "non_object_root" } + | { state: "event_capture_missing" } + | { state: "event_final_mismatch" }; + +export type PiInvalidToolCall = { + type: "invalidToolCall"; + id: string; + name: string; + argumentParse: PiUntrustedArgumentParse; +}; + +export type PiSubmitCall = PiToolCall | PiInvalidToolCall; + export type PiTextContent = { type: "text"; text: string; @@ -181,7 +236,7 @@ export type PiTextContent = { export type PiAssistantMessage = { role: "assistant"; - content: Array>; + content: Array>; provider: string; model: string; usage?: { @@ -216,7 +271,7 @@ export interface PiAiAdapter { complete( model: PiModelRef, context: { messages: unknown[]; tools: Array<{ name: string; description: string; parameters: TSchema }> }, - options: Record + options: Record & { submitToolName: string } ): Promise; validateToolCall(tools: Array<{ name: string; description: string; parameters: TSchema }>, toolCall: PiToolCall): unknown; } diff --git a/src/llm/model-call-cache.ts b/src/llm/model-call-cache.ts index 34b5806..f1ed950 100644 --- a/src/llm/model-call-cache.ts +++ b/src/llm/model-call-cache.ts @@ -10,7 +10,7 @@ import type { Logger, ReviewStage } from "../types.js"; import type { TelemetryRecorder } from "../telemetry/telemetry-recorder.js"; import type { ModelCallCache, PiAssistantMessage, StoredProviderResponse } from "./llm-runner.js"; -export const MODEL_CALL_CACHE_SCHEMA_VERSION = 1; +export const MODEL_CALL_CACHE_SCHEMA_VERSION = 2; type CreateModelCallCacheOptions = { dir: string; diff --git a/src/llm/pi-runner.ts b/src/llm/pi-runner.ts index 11bfc57..e20d9aa 100644 --- a/src/llm/pi-runner.ts +++ b/src/llm/pi-runner.ts @@ -1,6 +1,7 @@ import { validateToolCall, type Api, + type AssistantMessageEventStream, type Context, type Model, type ModelAuth, @@ -35,11 +36,14 @@ import { type LlmSchemaRepairInput, type LlmRunner, type LlmStructuredRequest, + type LlmSubmitFailureClassification, type LlmToolResultSummary, type ModelCallCacheMissReason, type PiAiAdapter, type PiAssistantMessage, type PiModelRef, + type PiInvalidToolCall, + type PiSubmitCall, type PiToolCall, type StoredProviderResponse, type ToolDefinition, @@ -50,8 +54,11 @@ import { } from "./llm-runner.js"; import { MODEL_CALL_CACHE_SCHEMA_VERSION, buildModelCallCacheKey } from "./model-call-cache.js"; import { SCHEMA_VERSIONS, submitToolNameForStage } from "./schemas.js"; +import { buildStructuredSubmitFailureDiagnostic } from "./schema-diagnostics.js"; +import { consumeFinalToolArguments } from "./final-tool-arguments.js"; import { classifyStage7SchemaInvalid, + isStage7SchemaInvalidKind, stage7CompactSchemaRepairPrompt, type Stage7SchemaInvalidKind, type Stage7SubmitRepairDecision @@ -59,6 +66,16 @@ import { type ConversationMessage = Record; +class SubmitSemanticValidationError extends Error { + readonly classification: LlmSubmitFailureClassification; + + constructor(classification: LlmSubmitFailureClassification) { + super(`Submit semantic validation failed: ${classification}`); + this.name = "SubmitSemanticValidationError"; + this.classification = classification; + } +} + type ProviderCallResult = | { source: "cache"; message: PiAssistantMessage; callId: string } | { source: "provider"; message: PiAssistantMessage; callId: string }; @@ -174,15 +191,15 @@ const MAX_DEBUG_ARTIFACT_CHARS = 1_500_000; const RECORDED_PROVIDER_FAILURE = Symbol("recordedProviderFailure"); type RealPiAiAdapterDeps = { - models?: Pick; - complete?: PiCompleteFunction; - completeSimple?: PiCompleteSimpleFunction; + models?: Pick; + stream?: PiStreamFunction; + streamSimple?: PiStreamSimpleFunction; getOAuthApiKey?: GetOAuthApiKey; authStorage?: PiAuthStorage; }; -type PiCompleteFunction = (model: Model, context: Context, options?: ProviderStreamOptions) => Promise; -type PiCompleteSimpleFunction = (model: Model, context: Context, options?: SimpleStreamOptions) => Promise; +type PiStreamFunction = (model: Model, context: Context, options?: ProviderStreamOptions) => AssistantMessageEventStream; +type PiStreamSimpleFunction = (model: Model, context: Context, options?: SimpleStreamOptions) => AssistantMessageEventStream; type OAuthApiKeyResult = { newCredentials: OAuthCredentials; apiKey: string; @@ -255,6 +272,7 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { let resultCharsUsed = 0; const sourceExtensionState: ToolBudgetExtensionState = { toolCallsUsed: 0, resultCharsUsed: 0 }; let schemaRepairUsed = false; + let pendingFinalArgumentRecovery: { correlationId: string } | undefined; let finalizeNudgeUsed = false; // Plan 95: the single model-repair scheduler. Every schema-repair retry // (discipline errors and schema-invalid submits alike) books the one @@ -263,30 +281,55 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { // attempt is already spent. const scheduleModelRepair = (repair: { submitToolName: string; - submitCalls: PiToolCall[]; + submitCalls: PiSubmitCall[]; extraToolNames: string[]; error: string; - repairClassification?: string; + repairClassification?: LlmSubmitFailureClassification; replaceConversationOverride?: boolean; cause?: unknown; }): void => { - queueSchemaRepair({ - opts, - request, - messages, - submitToolName: repair.submitToolName, - submitCalls: repair.submitCalls, - extraToolNames: repair.extraToolNames, - error: repair.error, - schemaRepairUsed, - ...(repair.repairClassification !== undefined ? { repairClassification: repair.repairClassification as Stage7SchemaInvalidKind } : {}), - ...(repair.replaceConversationOverride === true ? { replaceConversationOverride: true } : {}), - ...(repair.cause !== undefined ? { cause: repair.cause } : {}) - }); + try { + queueSchemaRepair({ + opts, + request, + messages, + submitToolName: repair.submitToolName, + submitCalls: repair.submitCalls, + extraToolNames: repair.extraToolNames, + error: repair.error, + schemaRepairUsed, + ...(repair.repairClassification !== undefined ? { repairClassification: repair.repairClassification } : {}), + ...(repair.replaceConversationOverride !== undefined + ? { replaceConversationOverride: repair.replaceConversationOverride } + : {}), + ...(repair.cause !== undefined ? { cause: repair.cause } : {}) + }); + } catch (cause) { + recordFinalArgumentRepairOutcome(schemaRepairUsed ? "terminal_invalid" : "not_dispatched"); + throw cause; + } schemaRepairUsed = true; forceFinalize = true; budgetForceFinalize = false; }; + const recordFinalArgumentRepairOutcome = (outcome: "recovered" | "terminal_invalid" | "not_dispatched"): void => { + if (pendingFinalArgumentRecovery === undefined) { + return; + } + opts.telemetry.event(definedRecord({ + stage: request.stage, + level: outcome === "recovered" ? "info" : "warn", + message: "final_argument_repair_outcome", + workerId: request.telemetryContext?.workerId, + packetId: request.telemetryContext?.packetId, + data: definedRecord({ + correlationId: pendingFinalArgumentRecovery.correlationId, + outcome, + candidateId: request.telemetryContext?.candidateId + }) + }) as Parameters[0]); + pendingFinalArgumentRecovery = undefined; + }; let finalizeSubmitRetryUsed = false; let forceFinalize = false; let budgetForceFinalize = false; @@ -357,20 +400,22 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { }); continue; } + recordFinalArgumentRepairOutcome("not_dispatched"); throw cause; } const message = providerResult.message; - messages.push(message as unknown as ConversationMessage); - const candidateDraftedBeforeSubmit = candidateDrafted; const submitCalls = toolCallsNamed(message, submitTool.name); const submitCall = submitCalls[0]; const toolCalls = toolCallsExcept(message, submitTool.name); + if (!message.content.some(isInvalidToolCall)) { + messages.push(message as unknown as ConversationMessage); + } recordExtraSubmitDropped(opts, request, submitTool.name, submitCalls); candidateDrafted = candidateDrafted || submitCalls.some(submitCallHasFindings); const submitDisciplineError = submitResponseDisciplineError(request, submitTool.name, submitCalls); if (submitDisciplineError !== undefined) { - if (request.stage === 5) { + if (request.stage === 5 && submitCalls.every(isTrustedSubmitCall)) { request.schemaRepair?.recoverInvalidSubmit?.(schemaRepairInput({ request, submitToolName: submitTool.name, @@ -384,13 +429,29 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { submitToolName: submitTool.name, submitCalls, extraToolNames: toolCalls.map((toolCall) => toolCall.name), - error: submitDisciplineError + error: submitDisciplineError, + repairClassification: submitCalls.length === 0 ? "missing_submit" : "multiple_submits" }); continue; } if (submitCall) { + if (!isTrustedSubmitCall(submitCall)) { + const classification = provenanceFailureClassification(submitCall); + const correlationId = `${providerResult.callId}:submit`; + recordRejectedFinalArguments(opts, request, submitTool.name, submitCall, classification, schemaRepairUsed, correlationId); + pendingFinalArgumentRecovery ??= { correlationId }; + scheduleModelRepair({ + submitToolName: submitTool.name, + submitCalls, + extraToolNames: toolCalls.map((toolCall) => toolCall.name), + error: `The ${submitTool.name} final arguments were not trusted: ${classification}.`, + repairClassification: classification, + replaceConversationOverride: false + }); + continue; + } try { - const validated = adapter.validateToolCall([toolSpec(submitTool)], submitCall); + const validated = validateSubmitCall(adapter, request, submitTool, submitCall); 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."; @@ -435,9 +496,17 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { }) }) as Parameters[0]); } + if (schemaRepairUsed) { + recordFinalArgumentRepairOutcome("recovered"); + } return validated as T; } catch (cause) { - const submitError = `The ${submitTool.name} arguments were schema-invalid: ${truncateDiagnostic(cause instanceof Error ? cause.message : String(cause))}`; + const semanticClassification = cause instanceof SubmitSemanticValidationError + ? cause.classification + : undefined; + const submitError = semanticClassification !== undefined + ? `The ${submitTool.name} arguments were semantically invalid: ${semanticClassification}` + : `The ${submitTool.name} arguments were schema-invalid: ${truncateDiagnostic(cause instanceof Error ? cause.message : String(cause))}`; const repairInput: LlmSchemaInvalidSubmitRecoveryInput = { ...schemaRepairInput({ request, @@ -445,7 +514,8 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { error: submitError, submitCalls, extraToolNames: toolCalls.map((toolCall) => toolCall.name), - schemaRepairUsed + schemaRepairUsed, + ...(semanticClassification !== undefined ? { classification: semanticClassification } : {}) }), candidateDrafted: candidateDraftedBeforeSubmit, fullError: cause instanceof Error ? cause.message : String(cause) @@ -461,13 +531,16 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { if (recovery.validated !== undefined) { return recovery.validated as T; } + const repairClassification = recovery.repairClassification ?? semanticClassification; scheduleModelRepair({ submitToolName: submitTool.name, submitCalls, extraToolNames: toolCalls.map((toolCall) => toolCall.name), error: submitError, - ...(recovery.repairClassification !== undefined ? { repairClassification: recovery.repairClassification } : {}), - ...(recovery.replaceConversationOverride === true ? { replaceConversationOverride: true } : {}), + ...(repairClassification !== undefined ? { repairClassification } : {}), + ...(recovery.replaceConversationOverride !== undefined + ? { replaceConversationOverride: recovery.replaceConversationOverride } + : {}), cause }); continue; @@ -485,9 +558,18 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { }); continue; } + const structuredSubmitFailure = buildStructuredSubmitFailureDiagnostic({ + stage: request.stage, + role: roleForStage(request.stage), + submitTool: submitTool.name, + submitSchemaVersion: SCHEMA_VERSIONS[submitToolNameForStage(request.stage)], + attempt: schemaRepairUsed ? "repair" : "primary", + classification: "missing_submit", + schema: request.schema + }); throw new CodegenieError("llm_schema_invalid", `model did not call ${submitTool.name} during ${kind}`, { recoverable: true, - context: { submitTool: submitTool.name, kind, unexpectedTools: toolCalls.map((toolCall) => toolCall.name) } + context: { structuredSubmitFailure } }); } @@ -645,28 +727,37 @@ export function createRealPiAiAdapter(deps: RealPiAiAdapterDeps = {}): PiAiAdapt return { resolveModel: ({ provider, model }) => resolveRealModel(provider, model, authStorage, models), complete: async (model, context, options) => { - const completeOptions = { ...options } as SimpleStreamOptions & Record; + const { submitToolName, ...providerOptions } = options; + const completeOptions = { ...providerOptions } as SimpleStreamOptions & Record; if (isForcedToolChoice(completeOptions.toolChoice)) { - if (deps.complete !== undefined) { + if (deps.stream !== undefined) { const prepared = await prepareInjectedCompletion(model, completeOptions, deps, models); - return deps.complete( + const stream = deps.stream( prepared.model, context as Context, mapProviderOptions(prepared.model, prepared.options) - ) as Promise; + ); + return consumeFinalToolArguments(stream, submitToolName); } - return models.complete( + const stream = models.stream( model.raw as Model, context as Context, mapProviderOptions(model.raw as Model, completeOptions) - ) as Promise; + ); + return consumeFinalToolArguments(stream, submitToolName); } delete completeOptions.forceSubmitToolChoice; - if (deps.completeSimple !== undefined) { + if (deps.streamSimple !== undefined) { const prepared = await prepareInjectedCompletion(model, completeOptions, deps, models); - return deps.completeSimple(prepared.model, context as Context, prepared.options) as Promise; + return consumeFinalToolArguments( + deps.streamSimple(prepared.model, context as Context, prepared.options), + submitToolName + ); } - return models.completeSimple(model.raw as Model, context as Context, completeOptions) as Promise; + return consumeFinalToolArguments( + models.streamSimple(model.raw as Model, context as Context, completeOptions), + submitToolName + ); }, validateToolCall: (tools, toolCall) => validateToolCall(tools as Tool[], toolCall as ToolCall) }; @@ -783,7 +874,10 @@ function recordProviderPromptCacheStrategy( }) as Parameters[0]); } -function submitCallHasFindings(toolCall: PiToolCall): boolean { +function submitCallHasFindings(toolCall: PiSubmitCall): boolean { + if (!isTrustedSubmitCall(toolCall)) { + return false; + } const findings = toolCall.arguments.findings; return Array.isArray(findings) && findings.length > 0; } @@ -943,7 +1037,14 @@ async function completeWithCache(input: { if (opts.cache) { const cached = await opts.cache.get(cacheKey, request.stage); if (cached.status === "hit") { - const cachedResponse = scrubStoredProviderResponse(cached.response); + const scrubbedCachedResponse = scrubStoredProviderResponse(cached.response); + const cachedResponse = { + ...scrubbedCachedResponse, + message: removeProvenanceLessSubmitArguments( + scrubbedCachedResponse.message, + submitToolNameForStage(request.stage) + ) + }; const cachedFailure = providerFailureFromMessage(cachedResponse.message, false); const cachedSchemaValid = schemaValidityForResponse(adapter, request, tools, kind, cachedResponse.message); if (cachedFailure) { @@ -1075,6 +1176,7 @@ async function completeWithCache(input: { ? {} : { reasoning: opts.llmConfig.reasoning ?? "high" }), forceSubmitToolChoice: opts.llmConfig.forceSubmitToolChoice !== false, + submitToolName: submitToolNameForStage(request.stage), toolChoice, sessionId: providerPromptCache.sessionId, cacheRetention: providerPromptCache.cacheRetention, @@ -1104,7 +1206,10 @@ async function completeWithCache(input: { taskTimedOut ); }); - const message = scrubAssistantMessage(rawMessage); + const message = removeProvenanceLessSubmitArguments( + scrubAssistantMessage(rawMessage), + submitToolNameForStage(request.stage) + ); const durationMs = Date.now() - startedAt; const providerFailure = providerFailureFromMessage(message, taskTimedOut()); if (providerFailure) { @@ -1576,8 +1681,8 @@ function withToolChoicePayload( }; } -function toolCallsNamed(message: PiAssistantMessage, name: string): PiToolCall[] { - return message.content.filter((block): block is PiToolCall => isToolCall(block) && block.name === name); +function toolCallsNamed(message: PiAssistantMessage, name: string): PiSubmitCall[] { + return message.content.filter((block): block is PiSubmitCall => isSubmitCall(block) && block.name === name); } function toolCallsExcept(message: PiAssistantMessage, excludedName: string): PiToolCall[] { @@ -1588,6 +1693,58 @@ function isToolCall(block: unknown): block is PiToolCall { return Boolean(block && typeof block === "object" && (block as { type?: unknown }).type === "toolCall"); } +function isInvalidToolCall(block: unknown): block is PiInvalidToolCall { + return Boolean(block && typeof block === "object" && (block as { type?: unknown }).type === "invalidToolCall"); +} + +function isSubmitCall(block: unknown): block is PiSubmitCall { + return isToolCall(block) || isInvalidToolCall(block); +} + +function isTrustedSubmitCall(call: PiSubmitCall): call is PiToolCall { + return isToolCall(call) && hasTrustedArgumentParse(call); +} + +function hasTrustedArgumentParse(call: PiToolCall): boolean { + return call.argumentParse?.state === "strict" || call.argumentParse?.state === "repaired"; +} + +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 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"; +} + +function untrustedRepairMetadata(calls: PiSubmitCall[]): NonNullable | undefined { + const metadata = calls.filter((call) => !isTrustedSubmitCall(call)).map((call) => ({ + id: call.id, + name: call.name, + state: call.argumentParse?.state ?? "event_capture_missing", + ...((call.argumentParse?.state === "partial" || call.argumentParse?.state === "invalid") + ? { errorKind: call.argumentParse.errorKind } + : {}) + })); + return metadata.length > 0 ? metadata : undefined; +} + async function executeToolCall( adapter: PiAiAdapter, tools: ToolDefinition[], @@ -2108,7 +2265,7 @@ function recordExtraSubmitDropped( opts: CreateRunnerOptions, request: LlmStructuredRequest, submitTool: string, - submitCalls: PiToolCall[] + submitCalls: PiSubmitCall[] ): void { if (submitCalls.length <= 1) { return; @@ -2129,6 +2286,35 @@ function recordExtraSubmitDropped( }) as Parameters[0]); } +function recordRejectedFinalArguments( + opts: CreateRunnerOptions, + request: LlmStructuredRequest, + submitTool: string, + call: PiSubmitCall, + classification: LlmSubmitFailureClassification, + schemaRepairUsed: boolean, + correlationId: string +): void { + opts.telemetry.event(definedRecord({ + stage: request.stage, + level: "warn", + message: "final_arguments_rejected", + workerId: request.telemetryContext?.workerId, + packetId: request.telemetryContext?.packetId, + data: definedRecord({ + submitTool, + correlationId, + state: call.argumentParse?.state ?? "event_capture_missing", + errorKind: call.argumentParse?.state === "partial" || call.argumentParse?.state === "invalid" + ? call.argumentParse.errorKind + : undefined, + classification, + schemaRepairUsed, + candidateId: request.telemetryContext?.candidateId + }) + }) as Parameters[0]); +} + function recordFinalizeMissingSubmitRetry( opts: CreateRunnerOptions, request: LlmStructuredRequest, @@ -2156,20 +2342,38 @@ function schemaRepairInput(input: { request: LlmStructuredRequest; submitToolName: string; error: string; - submitCalls: PiToolCall[]; + submitCalls: PiSubmitCall[]; extraToolNames: string[]; schemaRepairUsed: boolean; + classification?: LlmSubmitFailureClassification; }): LlmSchemaInvalidSubmitRecoveryInput { + const untrustedSubmitCalls = untrustedRepairMetadata(input.submitCalls); return { stage: input.request.stage, submitTool: input.submitToolName, error: truncateDiagnostic(input.error), - submitCalls: input.submitCalls.map((call) => ({ id: call.id, arguments: call.arguments })), + submitCalls: input.submitCalls.filter(isTrustedSubmitCall).map((call) => ({ id: call.id, arguments: call.arguments })), + ...(untrustedSubmitCalls !== undefined ? { untrustedSubmitCalls } : {}), extraToolNames: input.extraToolNames, - schemaRepairUsed: input.schemaRepairUsed + schemaRepairUsed: input.schemaRepairUsed, + ...(input.classification !== undefined ? { classification: input.classification } : {}) }; } +function validateSubmitCall( + adapter: PiAiAdapter, + request: LlmStructuredRequest, + 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; +} + function tryRecoverInvalidSubmit(input: { opts: CreateRunnerOptions; adapter: PiAiAdapter; @@ -2177,13 +2381,13 @@ function tryRecoverInvalidSubmit(input: { submitTool: ToolDefinition; repairInput: LlmSchemaInvalidSubmitRecoveryInput; cause: unknown; -}): { validated?: unknown; repairClassification?: string; replaceConversationOverride?: boolean } { +}): { validated?: unknown; repairClassification?: LlmSubmitFailureClassification; replaceConversationOverride?: boolean } { const result = input.request.schemaRepair?.recoverInvalidSubmit?.(input.repairInput); if (result === undefined) { return {}; } const recovery: LlmInvalidSubmitRecovery = isBrandedRecovery(result) ? result : { kind: "recovery", arguments: result }; - const hints: { validated?: unknown; repairClassification?: string; replaceConversationOverride?: boolean } = { + const hints: { validated?: unknown; repairClassification?: LlmSubmitFailureClassification; replaceConversationOverride?: boolean } = { ...(recovery.repairClassification !== undefined ? { repairClassification: recovery.repairClassification } : {}), ...(recovery.replaceConversationOverride !== undefined ? { replaceConversationOverride: recovery.replaceConversationOverride } : {}) }; @@ -2192,7 +2396,7 @@ function tryRecoverInvalidSubmit(input: { } const recoveredCallId = recovery.recoveredCallId ?? `${input.repairInput.submitTool}-recovered`; try { - const validated = input.adapter.validateToolCall([toolSpec(input.submitTool)], { + const validated = validateSubmitCall(input.adapter, input.request, input.submitTool, { type: "toolCall", id: recoveredCallId, name: input.repairInput.submitTool, @@ -2217,6 +2421,9 @@ function tryRecoverInvalidSubmit(input: { } return { ...hints, validated }; } catch (recoveryCause) { + const semanticClassification = recoveryCause instanceof SubmitSemanticValidationError + ? recoveryCause.classification + : undefined; const recoveryError = truncateDiagnostic(recoveryCause instanceof Error ? recoveryCause.message : String(recoveryCause)); if (recovery.onRejected !== undefined) { recovery.onRejected(recoveryError); @@ -2236,7 +2443,10 @@ function tryRecoverInvalidSubmit(input: { }) }) as Parameters[0]); } - return hints; + return { + ...hints, + ...(semanticClassification !== undefined ? { repairClassification: semanticClassification } : {}) + }; } } @@ -2249,17 +2459,20 @@ function queueSchemaRepair(input: { request: LlmStructuredRequest; messages: ConversationMessage[]; submitToolName: string; - submitCalls: PiToolCall[]; + submitCalls: PiSubmitCall[]; extraToolNames: string[]; error: string; schemaRepairUsed: boolean; - repairClassification?: Stage7SchemaInvalidKind; + repairClassification?: LlmSubmitFailureClassification; replaceConversationOverride?: boolean; cause?: unknown; }): void { const error = truncateDiagnostic(input.error); if (input.schemaRepairUsed) { if (input.request.stage === 7) { + const classification = isStage7SchemaInvalidKind(input.repairClassification) + ? input.repairClassification + : classifyStage7SchemaInvalid(input.error, input.submitCalls.filter(isTrustedSubmitCall)); recordStage7SchemaRepairEvent({ opts: input.opts, request: input.request, @@ -2267,27 +2480,44 @@ function queueSchemaRepair(input: { message: "stage7_schema_repair_failed", data: { submitTool: input.submitToolName, - classification: input.repairClassification ?? classifyStage7SchemaInvalid(input.error, input.submitCalls), + classification, error } }); } + 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: { submitTool: input.submitToolName, error }, - cause: input.cause + context: { structuredSubmitFailure } }); } + const untrustedSubmitCalls = untrustedRepairMetadata(input.submitCalls); const repairInput: LlmSchemaRepairInput = { stage: input.request.stage, submitTool: input.submitToolName, error, - submitCalls: input.submitCalls.map((call) => ({ id: call.id, arguments: call.arguments })), - extraToolNames: input.extraToolNames + submitCalls: input.submitCalls.filter(isTrustedSubmitCall).map((call) => ({ id: call.id, arguments: call.arguments })), + ...(untrustedSubmitCalls !== undefined ? { untrustedSubmitCalls } : {}), + extraToolNames: input.extraToolNames, + ...(input.repairClassification !== undefined ? { classification: input.repairClassification } : {}) }; - const stage7CompactRepair = input.request.stage === 7 && input.replaceConversationOverride === true; + const stage7Classification = isStage7SchemaInvalidKind(input.repairClassification) + ? input.repairClassification + : "unsafe_candidate_like_payload"; + const stage7CompactRepair = input.request.stage === 7 && + input.replaceConversationOverride === true && + isStage7SchemaInvalidKind(input.repairClassification); const content = stage7CompactRepair - ? stage7CompactSchemaRepairPrompt(input.submitToolName, error, input.repairClassification ?? "unsafe_candidate_like_payload", repairInput) + ? 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); @@ -2328,7 +2558,7 @@ function queueSchemaRepair(input: { submitTool: input.submitToolName, invalidSubmitCallCount: input.submitCalls.length, extraToolNames: input.extraToolNames, - classification: input.repairClassification ?? "unsafe_candidate_like_payload", + classification: stage7Classification, repairPromptChars: content.length, replaceConversation, candidateId: input.request.telemetryContext?.candidateId, @@ -2725,6 +2955,7 @@ function recordModelCall( ): void { const outputText = stableJson(message.content); const usage = normalizeUsage(meta.usage ?? message.usage); + const finalArguments = finalArgumentTelemetry(message, request.stage, submitToolNameForStage(request.stage), meta.callId); const record = definedRecord({ callId: meta.callId, ...meta.protocol, @@ -2760,6 +2991,7 @@ function recordModelCall( durationMs: meta.durationMs, cacheStatus: meta.cacheStatus, schemaValid: meta.schemaValid, + ...finalArguments, stopReason: stopReason(message), status: meta.status ?? "ok", errorCode: meta.errorCode, @@ -2775,6 +3007,37 @@ function recordModelCall( writeModelCallResponseDebug(opts, request, `${meta.callId}.response`, record, { response: message, usage: usageDebug }); } +function finalArgumentTelemetry( + message: PiAssistantMessage, + stage: ReviewStage, + submitTool: string, + callId: string +): Pick< + import("../telemetry/telemetry-recorder.js").LlmCallRecord, + "submitTool" | "finalArgumentState" | "finalArgumentErrorKind" | "finalArgumentRepairKind" | "finalArgumentCorrelationId" +> | Record { + const calls = toolCallsNamed(message, submitTool); + if ((stage === 5 || stage === 10) && calls.length !== 1) { + return {}; + } + const call = calls[0]; + if (call === undefined) { + return {}; + } + const parse = call.argumentParse; + const state = parse?.state ?? "event_capture_missing"; + return definedRecord({ + submitTool, + finalArgumentState: state, + finalArgumentErrorKind: parse?.state === "partial" || parse?.state === "invalid" ? parse.errorKind : undefined, + finalArgumentRepairKind: state === "repaired" ? "pi_narrow_string_repair" : undefined, + finalArgumentCorrelationId: `${callId}:submit` + }) as Pick< + import("../telemetry/telemetry-recorder.js").LlmCallRecord, + "submitTool" | "finalArgumentState" | "finalArgumentErrorKind" | "finalArgumentRepairKind" | "finalArgumentCorrelationId" + >; +} + function recordErroredModelCall( opts: CreateRunnerOptions, request: LlmStructuredRequest, @@ -2858,8 +3121,11 @@ function schemaValidityForResponse( if (!submitCall) { return kind === "finalize" || kind === "repair" ? false : undefined; } + if (!isTrustedSubmitCall(submitCall)) { + return false; + } try { - adapter.validateToolCall([toolSpec(submitTool)], submitCall); + validateSubmitCall(adapter, request, submitTool, submitCall); return true; } catch { return false; @@ -2869,7 +3135,7 @@ function schemaValidityForResponse( function submitResponseDisciplineError( request: LlmStructuredRequest, submitToolName: string, - submitCalls: PiToolCall[] + submitCalls: PiSubmitCall[] ): string | undefined { if (request.stage !== 5 && request.stage !== 10) { return undefined; diff --git a/src/llm/schema-diagnostics.ts b/src/llm/schema-diagnostics.ts new file mode 100644 index 0000000..63b168e --- /dev/null +++ b/src/llm/schema-diagnostics.ts @@ -0,0 +1,287 @@ +import type { TSchema } from "@earendil-works/pi-ai"; +import type { ReviewStage } from "../types.js"; +import type { LlmRole, LlmSubmitFailureClassification } from "./llm-runner.js"; +import { + SubmitCompositionSchema, + SubmitPacketReviewSchema, + SubmitPlanSchema, + SubmitSystemReviewSchema, + SubmitVerificationVerdictSchema +} from "./schemas.js"; + +export type StructuredSubmitFailureRule = + | "required" + | "additionalProperties" + | "type" + | "minLength" + | "maxLength" + | "minItems" + | "maxItems" + | "enum" + | "const" + | "semantic" + | "schema"; + +export type StructuredSubmitFailureDiagnostic = { + schemaVersion: 1; + stage: ReviewStage; + role: LlmRole; + submitTool: string; + submitSchemaVersion: number; + attempt: "primary" | "repair"; + classification: LlmSubmitFailureClassification; + issues: Array<{ + path: string; + rule: StructuredSubmitFailureRule; + expectedLimit?: number; + }>; +}; + +const RECEIVED_ARGUMENTS_DELIMITER = "\n\nReceived arguments:\n"; +const MAX_ISSUES = 12; +const MAX_PATH_CHARS = 200; +const MAX_LABEL_CHARS = 64; + +const CLASSIFICATIONS: ReadonlySet = new Set([ + "schema_invalid", + "missing_submit", + "multiple_submits", + "revise_without_revision_payload", + "length_stopped", + "final_arguments_partial", + "final_arguments_invalid", + "event_capture_missing", + "event_final_mismatch", + "xml_parameter_bleed", + "extra_finding_properties", + "extra_top_level_properties", + "missing_required_finding_fields", + "invalid_enum_value", + "string_too_long", + "empty_no_findings_missing_fields", + "unsafe_candidate_like_payload", + "invalid_tool_arguments", + "unknown" +]); +const RULES: ReadonlySet = new Set([ + "required", + "additionalProperties", + "type", + "minLength", + "maxLength", + "minItems", + "maxItems", + "enum", + "const", + "semantic", + "schema" +]); +const PUBLIC_SUBMITS: Readonly> = { + submit_plan: { schema: SubmitPlanSchema, stage: 5, role: "planner" }, + submit_review: { schema: SubmitPacketReviewSchema, stage: 7, role: "packetReview" }, + submit_system_review: { schema: SubmitSystemReviewSchema, stage: 8, role: "systemReview" }, + submit_verdict: { schema: SubmitVerificationVerdictSchema, stage: 9, role: "verifier" }, + submit_composition: { schema: SubmitCompositionSchema, stage: 10, role: "composer" } +}; +const ROLES: ReadonlySet = new Set(["planner", "packetReview", "systemReview", "verifier", "composer"]); + +export function normalizeStructuredSubmitFailureClassification( + value: string | undefined +): LlmSubmitFailureClassification { + if (value === undefined || value.length > MAX_LABEL_CHARS) { + return "unknown"; + } + return CLASSIFICATIONS.has(value as LlmSubmitFailureClassification) + ? value as LlmSubmitFailureClassification + : "unknown"; +} + +export function buildStructuredSubmitFailureDiagnostic(input: { + stage: ReviewStage; + role: LlmRole; + submitTool: string; + submitSchemaVersion: number; + attempt: "primary" | "repair"; + classification?: string; + schema: TSchema; + validationMessage?: string; +}): StructuredSubmitFailureDiagnostic { + const classification = normalizeStructuredSubmitFailureClassification(input.classification ?? "schema_invalid"); + return { + schemaVersion: 1, + stage: input.stage, + role: input.role, + submitTool: input.submitTool.slice(0, MAX_LABEL_CHARS), + submitSchemaVersion: input.submitSchemaVersion, + attempt: input.attempt, + classification, + issues: classification === "revise_without_revision_payload" + ? [{ path: "root", rule: "semantic" }] + : parseSafeValidationIssues(input.validationMessage, input.schema) + }; +} + +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; +} + +function sanitizePublicDiagnostic( + diagnostic: StructuredSubmitFailureDiagnostic +): StructuredSubmitFailureDiagnostic | undefined { + const identity = PUBLIC_SUBMITS[diagnostic.submitTool]; + if ( + identity === undefined || + !ROLES.has(diagnostic.role) || + diagnostic.stage !== identity.stage || + diagnostic.role !== identity.role || + !Number.isSafeInteger(diagnostic.submitSchemaVersion) || + diagnostic.submitSchemaVersion < 0 || + diagnostic.submitSchemaVersion > 1_000_000 + ) { + return undefined; + } + const allowedProperties = collectSchemaPropertyNames(identity.schema); + return { + schemaVersion: 1, + stage: diagnostic.stage, + role: diagnostic.role, + submitTool: diagnostic.submitTool, + submitSchemaVersion: diagnostic.submitSchemaVersion, + attempt: diagnostic.attempt, + classification: normalizeStructuredSubmitFailureClassification(diagnostic.classification), + issues: diagnostic.issues.slice(0, MAX_ISSUES).map((issue) => { + const rule = RULES.has(issue.rule) ? issue.rule : "schema"; + const expectedLimit = issue.expectedLimit; + return { + path: normalizeSchemaPath(issue.path, allowedProperties), + rule, + ...(expectedLimit !== undefined && Number.isSafeInteger(expectedLimit) && expectedLimit >= 0 && expectedLimit <= 1_000_000_000 + ? { expectedLimit } + : {}) + }; + }) + }; +} + +export function isStructuredSubmitFailureDiagnostic(value: unknown): value is StructuredSubmitFailureDiagnostic { + if (!isRecord(value) || value.schemaVersion !== 1 || !Array.isArray(value.issues)) { + return false; + } + return typeof value.stage === "number" && + typeof value.role === "string" && + typeof value.submitTool === "string" && + typeof value.submitSchemaVersion === "number" && + (value.attempt === "primary" || value.attempt === "repair") && + typeof value.classification === "string" && + value.issues.every((issue) => isRecord(issue) && typeof issue.path === "string" && typeof issue.rule === "string"); +} + +function parseSafeValidationIssues(message: string | undefined, schema: TSchema): StructuredSubmitFailureDiagnostic["issues"] { + if (message === undefined) { + return []; + } + const delimiter = message.indexOf(RECEIVED_ARGUMENTS_DELIMITER); + if (delimiter < 0) { + return []; + } + const safePrefix = message.slice(0, delimiter); + const allowedProperties = collectSchemaPropertyNames(schema); + const issues: StructuredSubmitFailureDiagnostic["issues"] = []; + for (const line of safePrefix.split(/\r?\n/u)) { + const match = /^\s*-\s+([^:]{1,400}):\s+(.{1,500})$/u.exec(line); + if (match === null) { + continue; + } + const path = normalizeSchemaPath(match[1] ?? "root", allowedProperties); + const parsed = classifyRule(match[2] ?? ""); + issues.push({ path, rule: parsed.rule, ...(parsed.expectedLimit !== undefined ? { expectedLimit: parsed.expectedLimit } : {}) }); + if (issues.length >= MAX_ISSUES) { + break; + } + } + return issues; +} + +function collectSchemaPropertyNames(schema: TSchema): Set { + const names = new Set(); + const seen = new Set(); + const visit = (node: unknown): void => { + if (!isRecord(node) || seen.has(node)) { + return; + } + seen.add(node); + if (isRecord(node.properties)) { + for (const [name, child] of Object.entries(node.properties)) { + names.add(name); + visit(child); + } + } + if (Array.isArray(node.items)) { + node.items.forEach(visit); + } else { + visit(node.items); + } + for (const branch of [node.anyOf, node.oneOf, node.allOf]) { + if (Array.isArray(branch)) { + branch.forEach(visit); + } + } + }; + visit(schema); + return names; +} + +function normalizeSchemaPath(path: string, allowedProperties: ReadonlySet): string { + const normalized = path.trim().replace(/^\//u, "").replaceAll("/", ".") || "root"; + if (normalized.length > MAX_PATH_CHARS) { + return "root"; + } + const segments = normalized.split(".").filter(Boolean); + if (segments.length === 0 || segments.some((segment) => segment !== "root" && !/^\d+$/u.test(segment) && !allowedProperties.has(segment))) { + return "root"; + } + return normalized; +} + +function classifyRule(message: string): { rule: StructuredSubmitFailureRule; expectedLimit?: number } { + const lower = message.toLowerCase(); + const expectedLimit = boundedNumber(message); + if (/required propert|is required/u.test(lower)) return { rule: "required" }; + if (/unexpected propert|additional propert/u.test(lower)) return { rule: "additionalProperties" }; + if (/string length.*(?:greater|minimum|minlength)|(?:at least|fewer than).*characters/u.test(lower)) return withLimit("minLength", expectedLimit); + if (/string length.*(?:less|maximum|maxlength)|(?:more than|at most).*characters|too long/u.test(lower)) return withLimit("maxLength", expectedLimit); + if (/array length.*(?:greater|minimum|minitems)|(?:at least|fewer than).*items/u.test(lower)) return withLimit("minItems", expectedLimit); + if (/array length.*(?:less|maximum|maxitems)|(?:more than|at most).*items/u.test(lower)) return withLimit("maxItems", expectedLimit); + if (/literal|const/u.test(lower)) return { rule: "const" }; + if (/union|enum|allowed value|one of/u.test(lower)) return { rule: "enum" }; + if (/expected/u.test(lower)) return { rule: "type" }; + return { rule: "schema" }; +} + +function withLimit( + rule: Extract, + expectedLimit: number | undefined +): { rule: StructuredSubmitFailureRule; expectedLimit?: number } { + return { rule, ...(expectedLimit !== undefined ? { expectedLimit } : {}) }; +} + +function boundedNumber(message: string): number | undefined { + const values = [...message.matchAll(/\b(\d{1,9})\b/gu)] + .map((match) => Number(match[1])) + .filter((value) => Number.isSafeInteger(value) && value >= 0); + return values.length > 0 ? values[values.length - 1] : undefined; +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} diff --git a/src/llm/schemas.ts b/src/llm/schemas.ts index a979b4b..9044ecd 100644 --- a/src/llm/schemas.ts +++ b/src/llm/schemas.ts @@ -247,8 +247,11 @@ export const SubmitSystemReviewSchema = Type.Object( { additionalProperties: false } ); +export const VERIFIER_REASON_TARGET_CHARS = 2_000; +export const VERIFIER_REASON_HARD_MAX_CHARS = 4_000; + const VerificationVerdictSharedProperties = { - reason: Type.String({ minLength: 1, maxLength: 2000 }), + reason: Type.String({ minLength: 1, maxLength: VERIFIER_REASON_HARD_MAX_CHARS }), requiredEvidencePresent: Type.Boolean(), falsePositiveRisk: Type.Union([Type.Literal("low"), Type.Literal("medium"), Type.Literal("high")]), behaviorChange: Type.Optional(BehaviorChangeAssessmentSchema), @@ -296,7 +299,7 @@ export const SCHEMA_VERSIONS = { submit_plan: 5, submit_review: 4, submit_system_review: 1, - submit_verdict: 3, + submit_verdict: 4, submit_composition: 1 } as const; diff --git a/src/llm/stage7-submit-repair.ts b/src/llm/stage7-submit-repair.ts index bcb9469..2a04af3 100644 --- a/src/llm/stage7-submit-repair.ts +++ b/src/llm/stage7-submit-repair.ts @@ -6,6 +6,7 @@ import type { LlmSchemaInvalidSubmitRecoveryInput, LlmSchemaRepairInput, LlmStructuredRequest, + LlmSubmitFailureClassification, PiToolCall } from "./llm-runner.js"; import type { TelemetryRecorder } from "../telemetry/telemetry-recorder.js"; @@ -22,7 +23,7 @@ export type Stage7SubmitRepairDecision = { truncatedNoFindingReason?: boolean; }; -export type Stage7SchemaInvalidKind = +export type Stage7SchemaInvalidKind = Extract; + +const STAGE7_SCHEMA_INVALID_KINDS: ReadonlySet = new Set([ + "xml_parameter_bleed", + "extra_finding_properties", + "extra_top_level_properties", + "missing_required_finding_fields", + "invalid_enum_value", + "string_too_long", + "empty_no_findings_missing_fields", + "unsafe_candidate_like_payload", + "invalid_tool_arguments" +]); + +export function isStage7SchemaInvalidKind(value: LlmSubmitFailureClassification | undefined): value is Stage7SchemaInvalidKind { + return value !== undefined && STAGE7_SCHEMA_INVALID_KINDS.has(value); +} type Stage7CandidateCleanup = | { status: "not_applicable" } diff --git a/src/output/markdown-renderer.ts b/src/output/markdown-renderer.ts index d8533ad..a342591 100644 --- a/src/output/markdown-renderer.ts +++ b/src/output/markdown-renderer.ts @@ -1,11 +1,12 @@ import type { BudgetLimitEvent, BudgetSummary, FinalFinding, ReviewResult, ReviewRunStats, RunCoverageStatus, RunPostingRecord } from "../types.js"; -import { renderBudgetStopNotice, renderCoverageSummaryLines } from "../util/coverage-summary.js"; +import { renderBudgetStopNotice, renderCoverageSummaryLines, renderCoverageTrustBanner } from "../util/coverage-summary.js"; import { inlineCode, severityBadge } from "../util/markdown.js"; export function renderMarkdownReview(result: ReviewResult): string { const sections = [ "# 🧞 Codegenie Review", "", + renderCoverageTrustBanner(result.coverage), renderBudgetStopNotice(result.coverage), result.summary.trim() || "Review completed.", "", @@ -14,12 +15,25 @@ export function renderMarkdownReview(result: ReviewResult): string { renderFindings("Summary-Only Findings", result.summaryOnlyFindings, result.runStats?.git), renderNeedsHumanAttention(result), renderStats(result.runStats, result.budgetSummary, result.posting), - result.noFindings ? "## ✅ No Findings\n\nNo credible findings were found. Everything looks good." : "" + renderNoFindings(result) ].filter((section) => section.trim().length > 0); return `${sections.join("\n\n")}\n`; } +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."; +} + function renderCoverage(coverage: RunCoverageStatus): string { return ["## Coverage", "", ...renderCoverageSummaryLines(coverage)].join("\n"); } diff --git a/src/pipeline/composer.ts b/src/pipeline/composer.ts index 6b3b52d..eb9e791 100644 --- a/src/pipeline/composer.ts +++ b/src/pipeline/composer.ts @@ -20,7 +20,7 @@ import type { UnifiedDiff, VerificationVerdict } from "../types.js"; -import { coverageDisclosureLines, renderCoverageSummaryLines } from "../util/coverage-summary.js"; +import { coverageDisclosureLines, renderCoverageSummaryLines, renderCoverageTrustBanner } from "../util/coverage-summary.js"; import { codeBlock, fenceLanguageForPath, inlineCode, severityBadge } from "../util/markdown.js"; import { sha256Hex } from "../util/hashing.js"; import { isCompositionTestPath, isDocsPath } from "../util/path-roles.js"; @@ -225,7 +225,9 @@ export async function dedupeRankAndComposeReview( telemetry ); const summary = publishableCount === 0 - ? fallbackSummary(0) + ? coverage.partial + ? "Review incomplete: completed work produced no credible verified findings, but incomplete coverage or verification prevents a clean conclusion." + : fallbackSummary(0) : fallbackUsed || compositionDegraded || isNoFindingsSummary(composition.summary) || summaryCountConflicts(composition.summary, publishableCount) ? fallbackSummary(publishableCount) : composition.summary || fallbackSummary(publishableCount); @@ -1586,7 +1588,15 @@ function renderReviewBody( coverage: RunCoverageStatus, omittedNoteCount = 0 ): string { - const lines = ["### 🧞 Codegenie Review", "", summary || "Review completed.", "", ...renderCoverageSummaryLines(coverage).slice(0, 2)]; + const trustBanner = renderCoverageTrustBanner(coverage); + const lines = [ + "### 🧞 Codegenie Review", + "", + ...(trustBanner.length > 0 ? [trustBanner, ""] : []), + summary || "Review completed.", + "", + ...renderCoverageSummaryLines(coverage).slice(0, 2) + ]; const coverageDisclosures = coverageDisclosureLines(coverage); if (coverageDisclosures.length > 0) { lines.push("", "**Coverage disclosure:**", ...coverageDisclosures); diff --git a/src/pipeline/planner.ts b/src/pipeline/planner.ts index 3720358..583119c 100644 --- a/src/pipeline/planner.ts +++ b/src/pipeline/planner.ts @@ -324,7 +324,7 @@ async function runPlannerCall( timeoutMs: config.review.perPassTimeoutMs, schemaRepair: { replaceConversation: true, - failAfterRepair: true, + failAfterRepair: false, recoverInvalidSubmit: (input) => recoverPlannerInvalidSubmit(input, telemetry, recovery), buildPrompt: (input) => buildPlannerSchemaRepairPrompt(dossier, opts.lenses, input) } diff --git a/src/pipeline/verifier.ts b/src/pipeline/verifier.ts index a0ffa2f..7017cca 100644 --- a/src/pipeline/verifier.ts +++ b/src/pipeline/verifier.ts @@ -1,6 +1,12 @@ import { buildRepositoryToolDefinitions } from "../llm/tool-definitions.js"; -import type { LlmRunner, LlmSchemaRepairInput } from "../llm/llm-runner.js"; -import { SCHEMA_VERSIONS, SubmitVerificationVerdictSchema, type SubmitVerificationVerdict } from "../llm/schemas.js"; +import type { LlmRunner, LlmSchemaRepairInput, LlmSubmitFailureClassification } from "../llm/llm-runner.js"; +import { + SCHEMA_VERSIONS, + SubmitVerificationVerdictSchema, + VERIFIER_REASON_HARD_MAX_CHARS, + VERIFIER_REASON_TARGET_CHARS, + type SubmitVerificationVerdict +} from "../llm/schemas.js"; import { skillsCompatibleWithLanguage, type LensRegistry } from "../skills/lens-registry.js"; import { fenceUntrusted, stableJson, type PromptBuilder } from "../skills/prompt-builder.js"; import type { TelemetryRecorder } from "../telemetry/telemetry-recorder.js"; @@ -117,6 +123,15 @@ type VerificationRuntimeStats = { repairFailed: number; }; +type FinalArgumentFailureClassification = Extract< + LlmSubmitFailureClassification, + | "length_stopped" + | "final_arguments_partial" + | "final_arguments_invalid" + | "event_capture_missing" + | "event_final_mismatch" +>; + type VerifierSchemaInvalidKind = | "xml_parameter_bleed" | "empty_submit_object" @@ -124,6 +139,7 @@ type VerifierSchemaInvalidKind = | "missing_submit_tool" | "invalid_tool_arguments" | "extra_tool_calls" + | FinalArgumentFailureClassification | "unknown"; type VerifierRepairAttempt = { @@ -769,6 +785,9 @@ async function runVerifierStructured( toolBudget: scaleToolBudget(VERIFIER_TOOL_BUDGET, config.review.budgetBoost), timeoutMs: config.review.perPassTimeoutMs, telemetryContext: { workerId, candidateId: candidate.id, packetId: candidate.producedBy.packetId }, + validateSubmit: (value) => value.verdict === "revise" && value.finalFinding === undefined && value.revisedAnchor === undefined + ? { ok: false, classification: "revise_without_revision_payload" } + : { ok: true }, schemaRepair: { replaceConversation: true, failAfterRepair: false, @@ -778,6 +797,21 @@ async function runVerifierStructured( } } }); + if (result.reason.length > VERIFIER_REASON_TARGET_CHARS) { + telemetry.event({ + stage: 9, + level: "info", + message: "verification_reason_target_exceeded", + file: candidate.path, + data: { + candidateId: candidate.id, + actualLength: result.reason.length, + target: VERIFIER_REASON_TARGET_CHARS, + hardMaximum: VERIFIER_REASON_HARD_MAX_CHARS, + followedModelRepair: repairAttempt !== undefined + } + }); + } if (repairAttempt !== undefined) { runtimeStats.repairSucceeded += 1; if (repairAttempt.classification === "empty_submit_object") { @@ -936,12 +970,19 @@ function buildVerifierSchemaRepairPrompt( "- Judge only the bounded candidate evidence above. It preserves the candidate claim, not repository-tool results from the discarded response.", "- keep only if the candidate is proven by concrete evidence.", "- revise only when the same issue is real but the evidence, wording, or anchor needs correction; include finalFinding or revisedAnchor.", + `- Keep reason concise and at most ${VERIFIER_REASON_TARGET_CHARS.toLocaleString("en-US")} characters.`, "- reject when required evidence is missing, the claim is speculative, or false-positive risk is high.", "- If rejecting because verification cannot be completed, set requiredEvidencePresent=false and falsePositiveRisk=high." ].join("\n"); } 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"; } @@ -980,6 +1021,22 @@ function classifyVerifierSchemaInvalid(input: LlmSchemaRepairInput | string): Ve return "unknown"; } +function explicitVerifierFailureClassification( + classification: LlmSubmitFailureClassification | undefined +): FinalArgumentFailureClassification | "revise_without_revision_payload" | undefined { + switch (classification) { + case "length_stopped": + case "final_arguments_partial": + case "final_arguments_invalid": + case "event_capture_missing": + case "event_final_mismatch": + case "revise_without_revision_payload": + return classification; + default: + return undefined; + } +} + function verifierRepairCandidateProjection(candidate: CandidateFinding): Record { const relatedCode = (candidate.evidence.relatedCode ?? []).slice(0, 3).map((entry) => ({ path: boundedVerifierRepairText(entry.path, 500), diff --git a/src/skills/prompt-builder.ts b/src/skills/prompt-builder.ts index 5d68afc..874c4df 100644 --- a/src/skills/prompt-builder.ts +++ b/src/skills/prompt-builder.ts @@ -11,6 +11,7 @@ import type { Skill, SkillSectionName } from "./skill-loader.js"; import type { LensDescriptor, LensRegistry } from "./lens-registry.js"; import type { TelemetryRecorder } from "../telemetry/telemetry-recorder.js"; import { prettyStableJson as stableJson } from "../util/json.js"; +import { VERIFIER_REASON_TARGET_CHARS } from "../llm/schemas.js"; export { stableJson }; @@ -73,7 +74,7 @@ export const PROMPT_TEMPLATE_VERSIONS: Record<5 | 7 | 8 | 9 | 10, string> = { 5: "p5.6", 7: "p7.10", 8: "p8.2", - 9: "p9.8", + 9: "p9.9", 10: "p10.2" }; @@ -122,6 +123,7 @@ export const PROMPT_TEMPLATE_WHY_LEDGER: Record<5 | 7 | 8 | 9 | 10, PromptLedger { surface: "testing candidate guidance", reason: "Keeps real test-boundary regressions while rejecting generic add-more-tests comments.", evidence: "Plan 92 E1 escalator and Plan 75 suppression" }, { surface: "conditional skill guidance block", reason: "An empty authoritative provenance list must not leave a provider-facing label that implies verifier guidance was supplied.", evidence: "Plan 101 exact skill provenance" }, { surface: "bounded verifier repair candidate evidence", reason: "Stateless replacement repair needs the candidate's claim and evidence without replaying contaminated output or discarded repository-tool state.", evidence: "Private eval 49f4645b runs 58-60; run 60 evidence-starved empty-submit repairs" }, + { surface: "verifier reason target", reason: "Keeps normal verdicts concise while the schema retains measured headroom for complete evidence-backed explanations.", evidence: "Owner evals observed four valid Stage-9 reasons between 2,100 and 2,984 characters" }, { surface: "strict submit_verdict closeout", reason: "Verifier model repair is still live and successful, so the structured closeout remains load-bearing.", evidence: "Plan 95 census: 3 Stage-9 schema repairs, all recovered" } ], 10: [ @@ -319,6 +321,7 @@ export function createPromptBuilder(_registry: LensRegistry, options: ProjectSki "Same-PR tests that assert new behavior prove the behavior changed; they do not by themselves prove the behavior is safe or intended. If intent signals are refactor-like or behavior-preserving without explicit behavior-change intent, compare base versus head behavior and keep or revise material semantic regressions that can break callers. If intent signals are mixed, frame the issue as intentional_needs_confirmation unless evidence proves accidental regression. Reject accidental-regression framing when PR text/spec clearly requires the behavior change and caller impact is covered.", "Examples of refactor-like or behavior-preserving intent include refactor, cleanup, consolidation, behavior-preserving, no behavior change, and equivalent behavior.", "When revising or keeping a behavior-change finding, preserve or set behaviorChange and intentEvidence. Do not use accidental-regression framing without behavior-preserving/refactor evidence and a concrete caller-visible regression.", + `Keep the verdict reason concise and at most ${VERIFIER_REASON_TARGET_CHARS.toLocaleString("en-US")} characters.`, skillGuidance, ...blocks, "Finish by calling submit_verdict with schema-valid arguments. Do not answer in plain text." diff --git a/src/telemetry/run-artifacts.ts b/src/telemetry/run-artifacts.ts index 6b4ecf3..65b7f21 100644 --- a/src/telemetry/run-artifacts.ts +++ b/src/telemetry/run-artifacts.ts @@ -94,6 +94,9 @@ const CANONICAL_ARTIFACT_PATHS: ReadonlySet = new Set(Object.values(ARTI type CacheCounts = Record<"hit" | "miss" | "disabled" | "write", number>; type ModelStatusCounts = Record; +type FinalArgumentStateCounts = Record, number>; +type FinalArgumentErrorKindCounts = Record, number>; +type FinalArgumentOutcomeCounts = Record<"recovered" | "terminal_invalid" | "not_dispatched", number>; type ProviderPromptCacheSummary = { readTokens: number; writeTokens: number; @@ -404,6 +407,9 @@ class RunTelemetryImpl { retryAttempts: 0, repairCalls: 0, schemaInvalidCalls: 0, + finalArgumentStates: emptyFinalArgumentStateCounts(), + finalArgumentErrorKinds: emptyFinalArgumentErrorKindCounts(), + finalArgumentOutcomes: { recovered: 0, terminal_invalid: 0, not_dispatched: 0 } as FinalArgumentOutcomeCounts, toolChoiceDowngradedCalls: 0, finalize: emptyModelFinalizeSummary(), byStage: {} as Record @@ -617,6 +623,7 @@ class RunTelemetryImpl { this.updateContextPressureFromEvent(capped); this.updateSchemaRecoveryFromEvent(capped); this.updateStage7SchemaRepairSummaryFromEvent(capped); + this.updateFinalArgumentOutcomeFromEvent(capped); this.mirrorTelemetryEventToRunLog(capped); if (this.runDirectory) { this.appendJsonl("events.jsonl", capped); @@ -854,6 +861,12 @@ class RunTelemetryImpl { this.modelSummary.retryAttempts += providerCallCount > 0 && record.attempt > 1 ? 1 : 0; this.modelSummary.repairCalls += record.kind === "repair" ? 1 : 0; this.modelSummary.schemaInvalidCalls += record.status === "schema_invalid" ? 1 : 0; + if (providerCallCount > 0 && record.finalArgumentState !== undefined) { + this.modelSummary.finalArgumentStates[record.finalArgumentState] += 1; + } + if (providerCallCount > 0 && record.finalArgumentErrorKind !== undefined) { + this.modelSummary.finalArgumentErrorKinds[record.finalArgumentErrorKind] += 1; + } this.modelSummary.toolChoiceDowngradedCalls += providerCallCount > 0 && record.toolChoiceDowngraded === true ? 1 : 0; this.updateSchemaRecoveryFromModelCall(record); this.updateStage7SchemaRepairSummaryFromModelCall(record); @@ -1096,6 +1109,16 @@ class RunTelemetryImpl { } } + private updateFinalArgumentOutcomeFromEvent(event: TelemetryEvent): void { + if (event.message !== "final_argument_repair_outcome" || event.data === undefined) { + return; + } + const outcome = event.data.outcome; + if (outcome === "recovered" || outcome === "terminal_invalid" || outcome === "not_dispatched") { + this.modelSummary.finalArgumentOutcomes[outcome] += 1; + } + } + private runTotals(): unknown { return { events: this.eventSeq, @@ -1488,6 +1511,27 @@ function emptyModelStatusCounts(): ModelStatusCounts { }; } +function emptyFinalArgumentStateCounts(): FinalArgumentStateCounts { + return { + strict: 0, + repaired: 0, + partial: 0, + invalid: 0, + length_stopped: 0, + event_capture_missing: 0, + event_final_mismatch: 0 + }; +} + +function emptyFinalArgumentErrorKindCounts(): FinalArgumentErrorKindCounts { + return { + unexpected_end: 0, + unterminated: 0, + invalid_syntax: 0, + non_object_root: 0 + }; +} + function emptyModelStageSummary(): ModelStageSummary { return { recordCount: 0, diff --git a/src/telemetry/telemetry-recorder.ts b/src/telemetry/telemetry-recorder.ts index 7037073..dbe38a1 100644 --- a/src/telemetry/telemetry-recorder.ts +++ b/src/telemetry/telemetry-recorder.ts @@ -3,6 +3,15 @@ import type { CodegenieErrorCode } from "../util/errors.js"; export type LlmRole = "planner" | "packetReview" | "systemReview" | "verifier" | "composer"; +export type LlmFinalArgumentState = + | "strict" + | "repaired" + | "partial" + | "invalid" + | "length_stopped" + | "event_capture_missing" + | "event_final_mismatch"; + export type LlmCallRecord = { callId: string; runId: string; @@ -51,6 +60,11 @@ export type LlmCallRecord = { durationMs: number; cacheStatus: "hit" | "miss" | "disabled" | "write"; schemaValid?: boolean; + submitTool?: string; + finalArgumentState?: LlmFinalArgumentState; + finalArgumentErrorKind?: "unexpected_end" | "unterminated" | "invalid_syntax" | "non_object_root"; + finalArgumentRepairKind?: "pi_narrow_string_repair"; + finalArgumentCorrelationId?: string; stopReason: "submit" | "tool_calls" | "text" | "error"; status: "ok" | "schema_invalid" | "transient_error" | "auth_error" | "timeout" | "aborted"; errorCode?: CodegenieErrorCode; diff --git a/src/util/coverage-summary.ts b/src/util/coverage-summary.ts index 83ac66b..655a091 100644 --- a/src/util/coverage-summary.ts +++ b/src/util/coverage-summary.ts @@ -2,6 +2,27 @@ import type { RunCoverageStatus } from "../types.js"; import { isDisclosableCoverageReason } from "./coverage-reasons.js"; import { inlineCode } from "./markdown.js"; +export function renderCoverageTrustBanner(coverage: RunCoverageStatus): string { + if (coverage.partial) { + const planning = coverage.degradedPlanning + ? " Planning also fell back to deterministic default coverage." + : ""; + return ( + "> [!WARNING]\n" + + `> **Review incomplete.** Some review or verification work did not complete.${planning} ` + + "Treat the results below as partial and consider rerunning." + ); + } + if (coverage.degradedPlanning) { + return ( + "> [!WARNING]\n" + + "> **Degraded run: planner fallback.** The planner failed, so deterministic default coverage was used. " + + "Downstream review completed, but consider rerunning." + ); + } + return ""; +} + export function renderCoverageSummaryLines(coverage: RunCoverageStatus): string[] { const lines = [coverageHeadline(coverage)]; if (coverage.partial) { diff --git a/tests/final-tool-arguments.test.ts b/tests/final-tool-arguments.test.ts new file mode 100644 index 0000000..82ce506 --- /dev/null +++ b/tests/final-tool-arguments.test.ts @@ -0,0 +1,215 @@ +import { + createAssistantMessageEventStream, + type AssistantMessageEvent, + type AssistantMessageEventStream +} from "@earendil-works/pi-ai"; +import { describe, expect, it, vi } from "vitest"; +import { consumeFinalToolArguments } from "../src/llm/final-tool-arguments.js"; +import type { PiAssistantMessage, PiInvalidToolCall, PiToolCall } from "../src/llm/llm-runner.js"; + +const SUBMIT = "submit_review"; + +describe("final tool argument provenance", () => { + it.each([ + ["fragmented", ['{"find', 'ings":[]}']], + ["canonical", ['{"findings":[]}']] + ])("accepts a %s strict object", async (_label, deltas) => { + const final = message(call("submit-1", SUBMIT, { findings: [] })); + const result = await consumeFinalToolArguments(sequence(final, deltas), SUBMIT); + expect(result).toEqual({ + ...final, + content: [{ ...call("submit-1", SUBMIT, { findings: [] }), argumentParse: { state: "strict" } }] + }); + }); + + it("accepts key-order differences after semantic deep equality", async () => { + const final = message(call("submit-1", SUBMIT, { a: 1, b: 2 })); + const result = await consumeFinalToolArguments(sequence(final, ['{"b":2,"a":1}']), SUBMIT); + expect(result.content[0]).toMatchObject({ + arguments: { b: 2, a: 1 }, + argumentParse: { state: "strict" } + }); + }); + + it.each([ + ["raw control", '{"reason":"line\nbreak"}', "line\nbreak"], + ["invalid backslash", '{"reason":"C:\\temp\\q"}', "C:\temp\\q"] + ])("accepts Pi's narrow %s repair and records only its kind", async (_label, delta, expected) => { + const final = message(call("submit-1", SUBMIT, { reason: expected })); + const result = await consumeFinalToolArguments(sequence(final, [delta]), SUBMIT); + expect(result.content[0]).toMatchObject({ + arguments: { reason: expected }, + argumentParse: { state: "repaired", repairs: ["pi_narrow_string_repair"] } + }); + }); + + it.each([ + ["unterminated string", '{"reason":"secret', { state: "partial", errorKind: "unterminated" }], + ["unterminated object", '{"reason":"ok"', { state: "partial", errorKind: "unexpected_end" }], + ["unterminated array", '{"items":[1,2', { state: "partial", errorKind: "unexpected_end" }], + ["malformed complete", '{"a":}', { state: "invalid", errorKind: "invalid_syntax" }], + ["empty", "", { state: "partial", errorKind: "unexpected_end" }], + ["scalar", "7", { state: "invalid", errorKind: "non_object_root" }], + ["array", "[]", { state: "invalid", errorKind: "non_object_root" }] + ])("rejects %s without returning captured data", async (_label, delta, expected) => { + const secret = "repository-secret-value"; + const final = message(call("submit-1", SUBMIT, { secret })); + const result = await consumeFinalToolArguments(sequence(final, [delta]), SUBMIT); + expect(result.content[0]).toEqual({ + type: "invalidToolCall", + id: "submit-1", + name: SUBMIT, + argumentParse: expected + }); + expect(JSON.stringify(result.content[0])).not.toContain(secret); + expect(result.content[0]).not.toHaveProperty("arguments"); + }); + + it.each([ + ["missing start", { omitStart: true }], + ["missing delta", { omitDelta: true }], + ["missing end", { omitEnd: true }], + ["duplicate framing", { duplicateStart: true }], + ["id mismatch", { endId: "other" }], + ["name mismatch", { endName: "other" }] + ])("rejects %s as capture missing", async (_label, options) => { + const final = message(call("submit-1", SUBMIT, { ok: true })); + const result = await consumeFinalToolArguments(sequence(final, ['{"ok":true}'], options), SUBMIT); + expect(result.content[0]).toMatchObject({ + type: "invalidToolCall", + argumentParse: { state: "event_capture_missing" } + }); + }); + + it("rejects toolcall-end arguments that diverge from the captured event value", async () => { + const final = message(call("submit-1", SUBMIT, { complete: false })); + const result = await consumeFinalToolArguments( + sequence(final, ['{"complete":false}'], { endArguments: { complete: true } }), + SUBMIT + ); + expect(result.content[0]).toMatchObject({ argumentParse: { state: "event_final_mismatch" } }); + expect(result.content[0]).not.toHaveProperty("arguments"); + }); + + it("rejects final-message substitution when event and toolcall-end arguments agree", async () => { + const final = message(call("submit-1", SUBMIT, { complete: true })); + const result = await consumeFinalToolArguments( + sequence(final, ['{"complete":false}'], { endArguments: { complete: false } }), + SUBMIT + ); + expect(result.content[0]).toMatchObject({ argumentParse: { state: "event_final_mismatch" } }); + expect(result.content[0]).not.toHaveProperty("arguments"); + }); + + it("gives normalized length stop precedence over valid JSON", async () => { + const final = { ...message(call("submit-1", SUBMIT, { ok: true })), stopReason: "length" }; + const result = await consumeFinalToolArguments(sequence(final, ['{"ok":true}']), SUBMIT); + expect(result.content[0]).toEqual({ + type: "invalidToolCall", + id: "submit-1", + name: SUBMIT, + argumentParse: { state: "length_stopped" } + }); + }); + + it("preserves terminal error semantics, leaves repository calls unchanged, and clears buffers", async () => { + const cleared = vi.fn(); + const repository = call("read-1", "read_range", { path: "src/secret.ts" }); + const final = { + ...message(repository, call("submit-1", SUBMIT, { ok: true })), + stopReason: "error", + errorMessage: "provider error" + }; + const result = await consumeFinalToolArguments( + sequence(final, ['{"ok":true}'], { contentIndex: 1 }), + SUBMIT, + { onBuffersCleared: cleared } + ); + expect(result.stopReason).toBe("error"); + expect(result.errorMessage).toBe("provider error"); + expect(result.content[0]).toEqual(repository); + expect(result.content[1]).toMatchObject({ argumentParse: { state: "strict" } }); + expect(cleared).toHaveBeenCalledWith(0); + }); + + it("throws a bounded error and clears buffers when the stream has no terminal event", async () => { + const cleared = vi.fn(); + const stream = createAssistantMessageEventStream(); + const final = message(call("submit-1", SUBMIT, { secret: "never expose me" })); + push(stream, { type: "toolcall_start", contentIndex: 0, partial: final as never }); + push(stream, { type: "toolcall_delta", contentIndex: 0, delta: '{"secret":"never expose me"}', partial: final as never }); + stream.end(); + await expect(consumeFinalToolArguments(stream, SUBMIT, { onBuffersCleared: cleared })) + .rejects.toThrow("Pi stream ended without a terminal event"); + expect(cleared).toHaveBeenCalledWith(0); + }); +}); + +function call(id: string, name: string, args: Record): PiToolCall { + return { type: "toolCall", id, name, arguments: args }; +} + +function message(...content: PiToolCall[]): PiAssistantMessage { + return { + role: "assistant", + content, + provider: "fake", + model: "fake-model", + usage: { input: 3, output: 2, totalTokens: 5 }, + stopReason: "toolUse", + timestamp: 1 + }; +} + +function sequence( + final: PiAssistantMessage, + deltas: string[], + options: { + contentIndex?: number; + omitStart?: boolean; + omitDelta?: boolean; + omitEnd?: boolean; + duplicateStart?: boolean; + endId?: string; + endName?: string; + endArguments?: Record; + } = {} +): AssistantMessageEventStream { + const stream = createAssistantMessageEventStream(); + const contentIndex = options.contentIndex ?? 0; + const finalCall = final.content[contentIndex] as PiToolCall; + const endCall = call( + options.endId ?? finalCall.id, + options.endName ?? finalCall.name, + options.endArguments ?? finalCall.arguments + ); + push(stream, { type: "start", partial: final as never }); + if (!options.omitStart) { + push(stream, { type: "toolcall_start", contentIndex, partial: final as never }); + if (options.duplicateStart) { + push(stream, { type: "toolcall_start", contentIndex, partial: final as never }); + } + } + if (!options.omitDelta) { + for (const delta of deltas) { + push(stream, { type: "toolcall_delta", contentIndex, delta, partial: final as never }); + } + } + if (!options.omitEnd) { + push(stream, { type: "toolcall_end", contentIndex, toolCall: endCall as never, partial: final as never }); + } + if (final.stopReason === "error" || final.stopReason === "aborted") { + push(stream, { type: "error", reason: final.stopReason, error: final as never }); + } else { + push(stream, { + type: "done", + reason: final.stopReason === "length" ? "length" : final.stopReason === "toolUse" ? "toolUse" : "stop", + message: final as never + }); + } + return stream; +} + +function push(stream: AssistantMessageEventStream, event: AssistantMessageEvent): void { + stream.push(event); +} diff --git a/tests/github-action.test.ts b/tests/github-action.test.ts index 8500665..79a38f4 100644 --- a/tests/github-action.test.ts +++ b/tests/github-action.test.ts @@ -156,6 +156,7 @@ function createFakeComments(opts: { existing?: IssueComment[]; permission?: string; failUpdates?: boolean; + failUpdateError?: unknown; viewerLogin?: string; } = {}): { client: IssueCommentClient; calls: FakeCommentCall[] } { const calls: FakeCommentCall[] = []; @@ -172,6 +173,9 @@ function createFakeComments(opts: { }, async updateComment(commentId, body) { calls.push({ kind: "update", commentId, body }); + if (opts.failUpdateError !== undefined) { + throw opts.failUpdateError; + } if (opts.failUpdates === true) { throw new CodegenieError("github_post_failed", "boom"); } @@ -724,6 +728,110 @@ describe("github-action entrypoint", () => { expect(output).toContain('"outcome":"review_failed"'); }); + it("always writes scrubbed failure artifacts and bounded schema identity without telemetry", async () => { + const fake = createFakeComments(); + const reportPath = path.join(scratch, "schema-failure-report.md"); + const failurePath = path.join(scratch, "schema-failure.json"); + const summaryPath = path.join(scratch, "schema-failure-summary.md"); + const secret = "sk-action-secret-must-not-surface"; + const repositoryText = "PRIVATE_REPOSITORY_SNIPPET_MUST_NOT_SURFACE"; + let output = ""; + const error = new CodegenieError( + "llm_schema_invalid", + `validator payload ${secret} ${repositoryText}`, + { + context: { + structuredSubmitFailure: { + schemaVersion: 1, + stage: 5, + role: "planner", + submitTool: "submit_plan", + submitSchemaVersion: 5, + attempt: "repair", + classification: "schema_invalid", + issues: [ + { path: "coverage", rule: "required" }, + { path: `${repositoryText}.private`, rule: "attacker-controlled-rule", expectedLimit: -7 }, + ...Array.from({ length: 20 }, (_, index) => ({ path: `unsafe-${index}`, rule: "type" })) + ] + }, + unsafe: `${secret} ${repositoryText}` + } + } + ); + + await expect(executeGitHubActionCommand([], { + env: actionEnv(pullRequestPayload(), "pull_request", { + CODEGENIE_REPORT_PATH: reportPath, + CODEGENIE_FAILURE_PATH: failurePath, + GITHUB_STEP_SUMMARY: summaryPath + }), + issueComments: fake.client, + minEditIntervalMs: 0, + writeOutput: (text) => { + output += text; + }, + runReview: async () => { + throw error; + } + })).rejects.toBe(error); + + const artifactSurfaces = [ + readFileSync(reportPath, "utf8"), + readFileSync(failurePath, "utf8"), + readFileSync(summaryPath, "utf8"), + output, + (fake.calls.at(-1) as { body: string }).body + ]; + for (const surface of artifactSurfaces) { + expect(surface).not.toContain(secret); + expect(surface).not.toContain(repositoryText); + } + expect(JSON.parse(artifactSurfaces[1] ?? "{}")).toMatchObject({ + schemaVersion: 1, + lane: "pull_request", + prNumber: 7, + errorCode: "llm_schema_invalid", + structuredSubmitFailure: { + stage: 5, + submitTool: "submit_plan", + attempt: "repair", + classification: "schema_invalid", + issues: expect.arrayContaining([ + { path: "coverage", rule: "required" }, + { path: "root", rule: "schema" } + ]) + } + }); + expect((JSON.parse(artifactSurfaces[1] ?? "{}") as { structuredSubmitFailure: { issues: unknown[] } }) + .structuredSubmitFailure.issues).toHaveLength(12); + expect(Buffer.byteLength(artifactSurfaces[0] ?? "", "utf8")).toBeLessThanOrEqual(4 * 1024); + expect(Buffer.byteLength(artifactSurfaces[1] ?? "", "utf8")).toBeLessThanOrEqual(16 * 1024); + }); + + it("keeps unknown failures bounded and preserves the original error when artifact paths are unwritable", async () => { + const fake = createFakeComments(); + const secret = "UNKNOWN_FAILURE_PRIVATE_REPOSITORY_TEXT"; + const original = new Error(`unexpected ${secret}`); + await expect(executeGitHubActionCommand([], { + env: actionEnv(pullRequestPayload(), "pull_request", { + CODEGENIE_REPORT_PATH: scratch, + CODEGENIE_FAILURE_PATH: scratch, + GITHUB_STEP_SUMMARY: scratch + }), + issueComments: fake.client, + minEditIntervalMs: 0, + writeOutput: (text) => { + expect(text).not.toContain(secret); + }, + runReview: async () => { + throw original; + } + })).rejects.toBe(original); + expect((fake.calls.at(-1) as { body: string }).body).toContain("`unknown_error`"); + expect((fake.calls.at(-1) as { body: string }).body).not.toContain(secret); + }); + it("disables inline posting when post-inline-comments is false", async () => { const fake = createFakeComments(); let reviewArgv: string[] = []; @@ -776,6 +884,31 @@ describe("github-action entrypoint", () => { }); }); + it("uses the bounded unknown_error code when a terminal edit throws an untyped error", async () => { + const secret = "terminal-update-private-message"; + const terminalError = new TypeError(secret); + const fake = createFakeComments({ failUpdateError: terminalError }); + const runDir = mkdtempSync(path.join(scratch, "post-unknown-failed-run-")); + const output: string[] = []; + + await expect( + executeGitHubActionCommand([], { + env: actionEnv(issueCommentPayload(), "issue_comment"), + issueComments: fake.client, + writeOutput: (text) => output.push(text), + runReview: async () => ({ runId: "r1", runDir, reportMarkdown: "# fallback report" }) + }) + ).rejects.toBe(terminalError); + + expect(JSON.parse(readFileSync(path.join(runDir, "github-action.json"), "utf8"))).toMatchObject({ + outcome: "terminal_post_failed", + errorCode: "unknown_error" + }); + expect(output.join("\n")).toContain('"errorCode":"unknown_error"'); + expect(output.join("\n")).not.toContain(secret); + expect(output.join("\n")).not.toContain("TypeError"); + }); + it("resolves identity via viewer login (PATs) or the bot-login input, and reclaims only exact matches", async () => { // PAT: /user resolves → own prior comment reclaimed const pat = createFakeComments({ @@ -991,6 +1124,7 @@ describe("issue comment client", () => { type WorkflowStep = { env?: Record; id?: string; + if?: string; name?: string; uses?: string; run?: string; @@ -1032,6 +1166,12 @@ describe("GitHub Action and workflow contracts", () => { const runStep = action.runs.steps.find((step) => step.id === "run"); expect(runStep?.run).toContain('args+=(--bot-login "$INPUT_BOT_LOGIN")'); expect(runStep?.run).toContain('args+=(--preflight-only "$INPUT_PREFLIGHT_ONLY")'); + const failurePath = runStep?.env?.CODEGENIE_FAILURE_PATH; + expect(failurePath).toBe("${{ runner.temp }}/codegenie-failure.json"); + const uploadStep = action.runs.steps.find((step) => step.name === "Upload review report"); + expect(uploadStep?.uses).toBe("actions/upload-artifact@v7"); + expect(uploadStep?.if).toContain("always()"); + expect(uploadStep?.with?.path).toContain(failurePath); }); it("keeps authorization in the binary and one newest-event-wins concurrency policy", () => { diff --git a/tests/phase4-llm.test.ts b/tests/phase4-llm.test.ts index c40ca87..690927d 100644 --- a/tests/phase4-llm.test.ts +++ b/tests/phase4-llm.test.ts @@ -2,14 +2,16 @@ import { execFileSync } from "node:child_process"; import { existsSync, mkdtempSync, mkdirSync, readFileSync, readdirSync, utimesSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; -import { Type, validateToolCall } from "@earendil-works/pi-ai"; +import { createAssistantMessageEventStream, Type, validateToolCall } from "@earendil-works/pi-ai"; import { describe, expect, it, vi } from "vitest"; import { __piRunnerTestHooks, createPiRunner, createRealPiAiAdapter } from "../src/llm/pi-runner.js"; import type { LlmCallUsage, PiAiAdapter, PiAssistantMessage, + PiInvalidToolCall, PiToolCall, + PiUntrustedArgumentParse, StoredProviderResponse } from "../src/llm/llm-runner.js"; import { @@ -25,6 +27,8 @@ import { type SubmitPacketReview, SubmitPacketReviewSchema, SubmitPlanSchema, + SubmitSystemReviewSchema, + type SubmitVerificationVerdict, SubmitVerificationVerdictSchema, submitToolNameForStage } from "../src/llm/schemas.js"; @@ -38,10 +42,14 @@ import type { ToolDefinition } from "../src/llm/llm-runner.js"; import type { PiAuthStorage, ProviderAuthEntry } from "../src/provider/provider-services.js"; import { CodegenieError } from "../src/util/errors.js"; import { scaleToolBudget } from "../src/util/budget.js"; +import { + buildStructuredSubmitFailureDiagnostic, + structuredSubmitFailureDiagnosticFromError +} from "../src/llm/schema-diagnostics.js"; type RealPiAiAdapterDepsForTest = NonNullable[0]>; -type PiCompleteForTest = NonNullable; -type PiCompleteSimpleForTest = NonNullable; +type PiStreamForTest = NonNullable; +type PiStreamSimpleForTest = NonNullable; describe("Phase 4 schemas and repository tool definitions", () => { it("redacts shared object references without mistaking them for cycles", () => { @@ -63,10 +71,11 @@ describe("Phase 4 schemas and repository tool definitions", () => { it("rejects hallucinated fields and exposes stage submit tool names", () => { expect(submitToolNameForStage(5)).toBe("submit_plan"); expect(submitToolNameForStage(7)).toBe("submit_review"); + expect(submitToolNameForStage(8)).toBe("submit_system_review"); expect(submitToolNameForStage(9)).toBe("submit_verdict"); expect(submitToolNameForStage(10)).toBe("submit_composition"); expect(SCHEMA_VERSIONS.submit_plan).toBe(5); - expect(SCHEMA_VERSIONS.submit_verdict).toBe(3); + expect(SCHEMA_VERSIONS.submit_verdict).toBe(4); const valid = { diffUnderstanding: { declaredIntent: "Small change", inferredBehavior: "The diff makes a small change." }, @@ -99,6 +108,113 @@ describe("Phase 4 schemas and repository tool definitions", () => { ).toThrow(); }); + it("keeps a 2,000-character verifier target with a 4,000-character hard schema buffer", () => { + const tool = { name: "submit_verdict", description: "submit", parameters: SubmitVerificationVerdictSchema }; + const verdict = (length: number): PiToolCall => ({ + type: "toolCall", + id: `verdict-${length}`, + name: "submit_verdict", + arguments: { + verdict: "reject", + reason: "r".repeat(length), + requiredEvidencePresent: false, + falsePositiveRisk: "high" + } + }); + for (const length of [2_000, 2_001, 2_102, 2_285, 2_327, 2_984, 4_000]) { + expect(() => validateToolCall([tool], verdict(length))).not.toThrow(); + } + expect(() => validateToolCall([tool], verdict(4_001))).toThrow(); + }); + + it("extracts only schema-owned paths and rules before Pi's received-arguments delimiter", () => { + const secret = "sk-seeded-secret-that-must-never-surface"; + let validationMessage = ""; + try { + validateToolCall( + [{ name: "submit_verdict", description: "submit", parameters: SubmitVerificationVerdictSchema }], + { + type: "toolCall", + id: "bad-verdict", + name: "submit_verdict", + arguments: { + verdict: "reject", + reason: `${"x".repeat(4_001)}${secret}`, + requiredEvidencePresent: false, + falsePositiveRisk: "high" + } + } + ); + } catch (error) { + validationMessage = error instanceof Error ? error.message : String(error); + } + const diagnostic = buildStructuredSubmitFailureDiagnostic({ + stage: 9, + role: "verifier", + submitTool: "submit_verdict", + submitSchemaVersion: 4, + attempt: "repair", + classification: "schema_invalid", + schema: SubmitVerificationVerdictSchema, + validationMessage + }); + expect(diagnostic.issues).toEqual(expect.arrayContaining([ + expect.objectContaining({ path: "reason", rule: "maxLength" }) + ])); + expect(JSON.stringify(diagnostic)).not.toContain(secret); + + const malformed = buildStructuredSubmitFailureDiagnostic({ + stage: 9, + role: "verifier", + submitTool: "submit_verdict", + submitSchemaVersion: 4, + attempt: "repair", + schema: SubmitVerificationVerdictSchema, + validationMessage: ` - attacker.path: required property\n${secret}` + }); + expect(malformed.issues).toEqual([]); + expect(JSON.stringify(malformed)).not.toContain(secret); + + const manyIssues = buildStructuredSubmitFailureDiagnostic({ + stage: 9, + role: "verifier", + submitTool: "submit_verdict", + submitSchemaVersion: 4, + attempt: "repair", + classification: "final_arguments_invalid", + schema: SubmitVerificationVerdictSchema, + validationMessage: `${[ + " - attacker.path: arbitrary prose", + ...Array.from({ length: 20 }, () => " - reason: must not have more than 4000 characters") + ].join("\n")}\n\nReceived arguments:\n${secret}` + }); + expect(manyIssues.classification).toBe("final_arguments_invalid"); + expect(manyIssues.issues).toHaveLength(12); + expect(manyIssues.issues[0]).toEqual({ path: "root", rule: "schema" }); + expect(JSON.stringify(manyIssues)).not.toContain(secret); + }); + + it("extracts Stage 8 diagnostics and rejects mismatched stage/role identities", () => { + const diagnostic = buildStructuredSubmitFailureDiagnostic({ + stage: 8, + role: "systemReview", + submitTool: "submit_system_review", + submitSchemaVersion: 1, + attempt: "repair", + schema: SubmitSystemReviewSchema, + validationMessage: " - findings: required property\n\nReceived arguments:\n{}" + }); + const wrapped = (structuredSubmitFailure: unknown) => new CodegenieError( + "llm_schema_invalid", + "bounded terminal failure", + { context: { structuredSubmitFailure } } + ); + + expect(structuredSubmitFailureDiagnosticFromError(wrapped(diagnostic))).toEqual(diagnostic); + expect(structuredSubmitFailureDiagnosticFromError(wrapped({ ...diagnostic, role: "planner" }))).toBeUndefined(); + expect(structuredSubmitFailureDiagnosticFromError(wrapped({ ...diagnostic, stage: 7 }))).toBeUndefined(); + }); + it("exposes model-facing submit schemas without pipeline-owned fields", () => { const review = { findings: [], followUpHints: [], uncertainties: [] }; const reviewTool = { name: "submit_review", description: "submit", parameters: SubmitPacketReviewSchema }; @@ -1545,7 +1661,7 @@ describe("Phase 4 Pi runner and model-call cache", () => { }); expect(telemetry.modelCalls.map((call) => call.cacheStatus)).toEqual(["miss", "write"]); expect(cache.put).toHaveBeenCalledTimes(1); - expect(cache.put.mock.calls[0]?.[1].message.content).toEqual([validSubmitReviewCall("submit-repair")]); + expect(cache.put.mock.calls[0]?.[1].message.content).toEqual([trustedSubmitCall(validSubmitReviewCall("submit-repair"))]); }); it("requires planner responses to submit exactly one plan and repairs with replacement context", async () => { @@ -1605,7 +1721,7 @@ describe("Phase 4 Pi runner and model-call cache", () => { expect(adapter.contexts[1]).toContain("compact planner repair"); expect(adapter.contexts[1]).not.toContain(originalPromptMarker); expect(cache.put).toHaveBeenCalledTimes(1); - expect(cache.put.mock.calls[0]?.[1].message.content).toEqual([validSubmitPlanCall("submit-plan-repaired")]); + expect(cache.put.mock.calls[0]?.[1].message.content).toEqual([trustedSubmitCall(validSubmitPlanCall("submit-plan-repaired"))]); }); it("repairs planner responses that omit submit_plan without generic finalization nudges", async () => { @@ -1685,6 +1801,47 @@ describe("Phase 4 Pi runner and model-call cache", () => { expect(adapter.complete).toHaveBeenCalledTimes(2); expect(telemetry.modelCalls.map((call) => call.status)).toEqual(["schema_invalid", "schema_invalid"]); + expect(telemetry.modelCalls.map((call) => call.finalArgumentState)).toEqual([undefined, undefined]); + }); + + it("keeps twice-invalid planner schema failure recoverable for deterministic fallback", async () => { + const adapter = scriptedAdapter([ + assistant([validSubmitPlanCall("submit-plan-a"), validSubmitPlanCall("submit-plan-b")]), + assistant([validSubmitPlanCall("submit-plan-c"), validSubmitPlanCall("submit-plan-d")]) + ]); + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: fakeTelemetry().recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } + }); + + await expect(runner.runStructured({ + stage: 5, + prompt: "planner", + schema: SubmitPlanSchema, + templateVersion: "test-template", + timeoutMs: 1_000, + schemaRepair: { + replaceConversation: true, + failAfterRepair: false, + buildPrompt: () => "compact planner repair" + } + })).rejects.toMatchObject({ + code: "llm_schema_invalid", + recoverable: true, + context: { + structuredSubmitFailure: expect.objectContaining({ + stage: 5, + submitTool: "submit_plan", + attempt: "repair", + classification: "multiple_submits" + }) + } + }); + expect(adapter.complete).toHaveBeenCalledTimes(2); }); it("repairs verifier schema-invalid submits with replacement context and submit-only tools", async () => { @@ -1753,6 +1910,395 @@ describe("Phase 4 Pi runner and model-call cache", () => { expect(telemetry.modelCalls.map((call) => call.status)).toEqual(["schema_invalid", "ok"]); }); + it("routes semantic empty-revise submits through the existing single repair and cache gate", async () => { + const telemetry = fakeTelemetry(); + const cache = { + get: vi.fn(async () => ({ status: "miss" as const, reason: "not_found" as const })), + put: vi.fn(async () => ({ status: "write" as const })) + }; + const emptyRevise = (id: string): PiToolCall => ({ + type: "toolCall", + id, + name: "submit_verdict", + arguments: { + verdict: "revise", + reason: "The issue needs a correction.", + requiredEvidencePresent: true, + falsePositiveRisk: "low" + } + }); + const adapter = scriptedAdapter([ + assistant([emptyRevise("empty-primary")]), + assistant([validSubmitVerdictCall("valid-repair")]) + ]); + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: telemetry.recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + cache, + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } + }); + + await expect(runner.runStructured({ + stage: 9, + prompt: "verify", + schema: SubmitVerificationVerdictSchema, + templateVersion: "test-template", + timeoutMs: 1_000, + validateSubmit: (value) => value.verdict === "revise" && value.finalFinding === undefined && value.revisedAnchor === undefined + ? { ok: false, classification: "revise_without_revision_payload" } + : { ok: true }, + schemaRepair: { + replaceConversation: true, + failAfterRepair: false, + buildPrompt: (input) => { + expect(input.classification).toBe("revise_without_revision_payload"); + return "repair the empty revision with a payload"; + } + } + })).resolves.toMatchObject({ verdict: "reject" }); + + expect(adapter.complete).toHaveBeenCalledTimes(2); + expect(telemetry.modelCalls.map((call) => call.schemaValid)).toEqual([false, true]); + expect(cache.put).toHaveBeenCalledTimes(1); + const putCalls = cache.put.mock.calls as unknown as Array<[unknown, StoredProviderResponse]>; + const cachedResponse = putCalls[0]?.[1]; + expect(cachedResponse?.message.content).toEqual([trustedSubmitCall(validSubmitVerdictCall("valid-repair"))]); + }); + + it("rejects a semantic-invalid deterministic recovery and exposes only bounded terminal identity", async () => { + const secret = "sk-semantic-payload-must-not-survive"; + const emptyRevise = (id: string): PiToolCall => ({ + type: "toolCall", + id, + name: "submit_verdict", + arguments: { + verdict: "revise", + reason: `The issue needs a correction. ${secret}`, + requiredEvidencePresent: true, + falsePositiveRisk: "low" + } + }); + const adapter = scriptedAdapter([ + assistant([{ + type: "toolCall", + id: "schema-invalid-primary", + name: "submit_verdict", + arguments: {} + }]), + assistant([emptyRevise("empty-repair")]) + ]); + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: fakeTelemetry().recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } + }); + + let caught: unknown; + try { + await runner.runStructured({ + stage: 9, + prompt: "verify", + schema: SubmitVerificationVerdictSchema, + templateVersion: "test-template", + timeoutMs: 1_000, + validateSubmit: (value) => value.verdict === "revise" && value.finalFinding === undefined && value.revisedAnchor === undefined + ? { ok: false, classification: "revise_without_revision_payload" } + : { ok: true }, + schemaRepair: { + replaceConversation: true, + failAfterRepair: false, + recoverInvalidSubmit: () => ({ + verdict: "revise", + reason: `Recovered without a payload ${secret}`, + requiredEvidencePresent: true, + falsePositiveRisk: "low" + }), + buildPrompt: (input) => { + expect(input.classification).toBe("revise_without_revision_payload"); + return "repair the semantic omission"; + } + } + }); + } catch (error) { + caught = error; + } + + expect(caught).toMatchObject({ + code: "llm_schema_invalid", + recoverable: true, + context: { + structuredSubmitFailure: expect.objectContaining({ + classification: "revise_without_revision_payload", + attempt: "repair", + issues: [{ path: "root", rule: "semantic" }] + }) + } + }); + expect(JSON.stringify(caught)).not.toContain(secret); + expect((caught as Error & { cause?: unknown }).cause).toBeUndefined(); + expect(adapter.complete).toHaveBeenCalledTimes(2); + }); + + it.each([ + { state: "length_stopped" } as const, + { state: "partial", errorKind: "unexpected_end" } as const, + { state: "invalid", errorKind: "invalid_syntax" } as const, + { state: "event_capture_missing" } as const, + { state: "event_final_mismatch" } as const + ])("routes primary $state final arguments through one context-preserving repair", async (argumentParse) => { + const telemetry = fakeTelemetry(); + const cache = { + get: vi.fn(async () => ({ status: "miss" as const, reason: "not_found" as const })), + put: vi.fn(async () => ({ status: "write" as const })) + }; + const invalid = invalidSubmitCall("untrusted-primary", "submit_review", argumentParse); + const primary = { ...assistant([invalid]), stopReason: "toolUse" }; + const adapter = scriptedAdapter([primary, assistant([validSubmitReviewCall("trusted-repair")])]); + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: telemetry.recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + cache, + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } + }); + + await expect(runner.runStructured({ + stage: 7, + prompt: "review ORIGINAL_STAGE7_PACKET_CONTEXT", + schema: SubmitPacketReviewSchema, + templateVersion: "test-template", + timeoutMs: 1_000, + schemaRepair: { + replaceConversation: true, + failAfterRepair: false, + buildPrompt: (input) => { + expect(input.submitCalls).toEqual([]); + expect(input.untrustedSubmitCalls).toEqual([expect.objectContaining({ + id: "untrusted-primary", + name: "submit_review", + state: argumentParse.state + })]); + return `retry bounded state ${argumentParse.state}`; + } + } + })).resolves.toMatchObject({ findings: [] }); + + expect(adapter.complete).toHaveBeenCalledTimes(2); + expect(adapter.contexts[1]).toContain(`retry bounded state ${argumentParse.state}`); + expect(adapter.contexts[1]).toContain("ORIGINAL_STAGE7_PACKET_CONTEXT"); + expect(adapter.contexts[1]).not.toContain("invalidToolCall"); + expect(cache.put).toHaveBeenCalledTimes(1); + expect(telemetry.modelCalls.map((call) => call.finalArgumentState)).toEqual([argumentParse.state, "strict"]); + expect(telemetry.events).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: "final_arguments_rejected", + data: expect.objectContaining({ state: argumentParse.state, schemaRepairUsed: false }) + }), + expect.objectContaining({ + message: "schema_repair_scheduled", + data: expect.objectContaining({ replaceConversation: false }) + }), + expect.objectContaining({ + message: "final_argument_repair_outcome", + data: expect.objectContaining({ outcome: "recovered" }) + }) + ])); + }); + + it("does not execute or orphan a valid repository tool beside an invalid submit", async () => { + const telemetry = fakeTelemetry(); + const execute = vi.fn(async () => ({ + text: "must not execute", + meta: { backend: "text" as const, precision: "exact" as const, degraded: false } + })); + const readRange: ToolDefinition = { + name: "read_range", + description: "read range", + parameters: Type.Object({ path: Type.String() }, { additionalProperties: false }), + execute + }; + const adapter = scriptedAdapter([ + assistant([ + { type: "toolCall", id: "read-beside-invalid-submit", name: "read_range", arguments: { path: "src/a.ts" } }, + invalidSubmitCall("invalid-submit", "submit_review", { state: "event_capture_missing" }) + ]), + assistant([validSubmitReviewCall("trusted-repair")]) + ]); + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: telemetry.recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } + }); + + await expect(runner.runStructured({ + stage: 7, + prompt: "review ORIGINAL_MIXED_PACKET_CONTEXT", + schema: SubmitPacketReviewSchema, + templateVersion: "test-template", + tools: [readRange], + toolBudget: { maxToolCalls: 2, maxInvestigationRounds: 2, maxResultChars: 1_000 }, + timeoutMs: 1_000, + schemaRepair: { + replaceConversation: true, + failAfterRepair: false, + buildPrompt: () => "retry invalid submit only" + } + })).resolves.toMatchObject({ findings: [] }); + + expect(execute).not.toHaveBeenCalled(); + expect(telemetry.toolCalls).toEqual([]); + expect(adapter.contexts[1]).toContain("ORIGINAL_MIXED_PACKET_CONTEXT"); + expect(adapter.contexts[1]).toContain("retry invalid submit only"); + expect(adapter.contexts[1]).not.toContain("read-beside-invalid-submit"); + expect(adapter.contexts[1]).not.toContain("toolResult"); + expect(telemetry.events).toContainEqual(expect.objectContaining({ + message: "schema_repair_scheduled", + data: expect.objectContaining({ replaceConversation: false, extraToolNames: ["read_range"] }) + })); + }); + + it("records not_dispatched when budget exhaustion prevents the final-argument repair call", async () => { + const telemetry = fakeTelemetry(); + const adapter = scriptedAdapter([ + { ...assistant([invalidSubmitCall("invalid-primary", "submit_review", { state: "invalid", errorKind: "invalid_syntax" })]), stopReason: "toolUse" } + ]); + let checkpoints = 0; + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: telemetry.recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + hooks: { + checkpoint: () => ++checkpoints === 1 ? "ok" : "exhausted", + onUsage: vi.fn() + } + }); + + await expect(runner.runStructured({ + stage: 7, + prompt: "review", + schema: SubmitPacketReviewSchema, + templateVersion: "test-template", + timeoutMs: 1_000, + schemaRepair: { replaceConversation: true, failAfterRepair: false, buildPrompt: () => "retry safely" } + })).rejects.toMatchObject({ + code: "llm_call_failed", + context: expect.objectContaining({ reason: "budget_exhausted" }) + }); + + expect(adapter.complete).toHaveBeenCalledTimes(1); + expect(telemetry.events).toContainEqual(expect.objectContaining({ + message: "final_argument_repair_outcome", + data: expect.objectContaining({ outcome: "not_dispatched" }) + })); + }); + + it.each([ + { state: "length_stopped" } as const, + { state: "partial", errorKind: "unterminated" } as const, + { state: "invalid", errorKind: "non_object_root" } as const, + { state: "event_capture_missing" } as const, + { state: "event_final_mismatch" } as const + ])("fails closed when repaired submit remains $state", async (argumentParse) => { + const telemetry = fakeTelemetry(); + const cache = { + get: vi.fn(async () => ({ status: "miss" as const, reason: "not_found" as const })), + put: vi.fn(async () => ({ status: "write" as const })) + }; + const adapter = scriptedAdapter([ + { ...assistant([invalidSubmitCall("invalid-primary", "submit_review", argumentParse)]), stopReason: "toolUse" }, + { ...assistant([invalidSubmitCall("invalid-repair", "submit_review", argumentParse)]), stopReason: "toolUse" } + ]); + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: telemetry.recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + cache, + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } + }); + + await expect(runner.runStructured({ + stage: 7, + prompt: "review", + schema: SubmitPacketReviewSchema, + templateVersion: "test-template", + timeoutMs: 1_000, + schemaRepair: { replaceConversation: true, failAfterRepair: false, buildPrompt: () => "retry safely" } + })).rejects.toMatchObject({ + code: "llm_schema_invalid", + recoverable: true, + context: { + structuredSubmitFailure: expect.objectContaining({ + attempt: "repair", + classification: provenanceClassification(argumentParse) + }) + } + }); + + expect(cache.put).not.toHaveBeenCalled(); + expect(telemetry.modelCalls.map((call) => call.finalArgumentState)).toEqual([argumentParse.state, argumentParse.state]); + expect(telemetry.events).toContainEqual(expect.objectContaining({ + message: "final_argument_repair_outcome", + data: expect.objectContaining({ outcome: "terminal_invalid" }) + })); + }); + + it("removes provenance-less submit arguments before telemetry, repair history, and cache", async () => { + const secret = "sk-provenance-less-argument-secret"; + const telemetry = fakeTelemetry(); + const provenanceLess = assistant([validSubmitReviewCall("missing-provenance")]); + provenanceLess.content = [{ + type: "toolCall", + id: "missing-provenance", + name: "submit_review", + arguments: { findings: [], followUpHints: [], uncertainties: [], secret } + }]; + const adapter = scriptedAdapter([provenanceLess, assistant([validSubmitReviewCall("trusted-repair")])]); + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: telemetry.recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } + }); + + await expect(runner.runStructured({ + stage: 7, + prompt: "review", + schema: SubmitPacketReviewSchema, + templateVersion: "test-template", + timeoutMs: 1_000, + schemaRepair: { replaceConversation: true, failAfterRepair: false, buildPrompt: () => "bounded retry" } + })).resolves.toMatchObject({ findings: [] }); + + const publicRecords = JSON.stringify({ + modelCalls: telemetry.modelCalls, + events: telemetry.events, + debugWrites: telemetry.debugWrites, + contexts: adapter.contexts + }); + expect(publicRecords).not.toContain(secret); + expect(telemetry.modelCalls[0]).toMatchObject({ + finalArgumentState: "event_capture_missing", + schemaValid: false + }); + }); + it("lets stages recover invalid submit arguments before model schema repair", async () => { const telemetry = fakeTelemetry(); const adapter = scriptedAdapter([ @@ -1810,6 +2356,64 @@ describe("Phase 4 Pi runner and model-call cache", () => { ])); }); + it("preserves an explicit append override returned by invalid-submit recovery", async () => { + const telemetry = fakeTelemetry(); + const adapter = scriptedAdapter([ + assistant([{ + type: "toolCall", + id: "submit-composition-invalid", + name: "submit_composition", + arguments: { summary: "missing composed findings" } + }]), + assistant([{ + type: "toolCall", + id: "submit-composition-repaired", + name: "submit_composition", + arguments: { + summary: "Recovered composition.", + composedFindings: [{ findingIds: ["finding-1"], finalBody: "Recovered body.", publication: "inline" }] + } + }]) + ]); + const runner = createPiRunner({ + llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: telemetry.recorder, + logger: fakeLogger(), + runSignal: new AbortController().signal, + adapter, + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } + }); + + await expect(runner.runStructured({ + stage: 10, + prompt: "ORIGINAL_RECOVERY_CONTEXT compose", + schema: SubmitCompositionSchema, + templateVersion: "test-template", + timeoutMs: 1_000, + schemaRepair: { + replaceConversation: true, + buildPrompt: () => "retry composition with complete fields", + recoverInvalidSubmit: () => ({ + kind: "recovery", + replaceConversationOverride: false + }) + } + })).resolves.toMatchObject({ + summary: "Recovered composition.", + composedFindings: [{ findingIds: ["finding-1"] }] + }); + + expect(adapter.complete).toHaveBeenCalledTimes(2); + expect(adapter.contexts[1]).toContain("ORIGINAL_RECOVERY_CONTEXT"); + expect(adapter.contexts[1]).toContain("retry composition with complete fields"); + expect(telemetry.modelCalls.map((call) => call.status)).toEqual(["schema_invalid", "ok"]); + expect(telemetry.events).toContainEqual(expect.objectContaining({ + stage: 10, + message: "schema_repair_scheduled", + data: expect.objectContaining({ replaceConversation: false }) + })); + }); + it("repairs composer responses that omit submit_composition with replacement context", async () => { const telemetry = fakeTelemetry(); const adapter = scriptedAdapter([ @@ -1936,7 +2540,7 @@ describe("Phase 4 Pi runner and model-call cache", () => { }); expect(adapter.complete).toHaveBeenCalledTimes(2); expect(cache.put).toHaveBeenCalledTimes(1); - expect(cache.put.mock.calls[0]?.[1].message.content).toEqual([validSubmitReviewCall("must-not-run")]); + expect(cache.put.mock.calls[0]?.[1].message.content).toEqual([trustedSubmitCall(validSubmitReviewCall("must-not-run"))]); }); it("includes tool budget in model-call cache keys", async () => { @@ -3812,7 +4416,13 @@ describe("Phase 4 Pi runner and model-call cache", () => { ).rejects.toMatchObject({ code: "llm_schema_invalid", recoverable: true, - context: { submitTool: "submit_review", kind: "finalize" } + context: { + structuredSubmitFailure: expect.objectContaining({ + submitTool: "submit_review", + classification: "missing_submit", + attempt: "primary" + }) + } }); expect(adapter.complete).toHaveBeenCalledTimes(3); }); @@ -4239,18 +4849,18 @@ describe("Phase 4 Pi runner and model-call cache", () => { } }); - it("uses Pi completeSimple so resolved reasoning is provider-mapped", async () => { + it("uses Pi streamSimple so resolved reasoning is provider-mapped", async () => { const optionsSeen: Record[] = []; - const completeSimple = (async (_model, _context, options) => { + const streamSimple = ((_model, _context, options) => { optionsSeen.push(options as Record); - return assistant([validSubmitReviewCall("submit-simple-reasoning")]); - }) as PiCompleteSimpleForTest; - const adapter = createRealPiAiAdapter({ completeSimple }); + return streamForMessage(assistant([validSubmitReviewCall("submit-simple-reasoning")])); + }) as PiStreamSimpleForTest; + const adapter = createRealPiAiAdapter({ streamSimple }); await adapter.complete( { provider: "fake", id: "fake-model", raw: { id: "fake-model" }, apiKey: "fake-api-key" }, { messages: [], tools: [] }, - { reasoning: "xhigh", maxRetries: 0 } + { reasoning: "xhigh", maxRetries: 0, submitToolName: "submit_review" } ); expect(optionsSeen).toEqual([ @@ -4259,16 +4869,17 @@ describe("Phase 4 Pi runner and model-call cache", () => { reasoning: "xhigh" }) ]); + expect(optionsSeen[0]).not.toHaveProperty("submitToolName"); }); it("maps forced submit calls to raw Pi provider reasoning and tool choice options", async () => { const rawOptionsSeen: Record[] = []; - const complete = (async (_model, _context, options) => { + const stream = ((_model, _context, options) => { rawOptionsSeen.push(options as Record); - return assistant([validSubmitReviewCall("submit-raw-forced")]); - }) as PiCompleteForTest; - const completeSimple = vi.fn(async () => assistant([validSubmitReviewCall("must-not-use-simple")])) as unknown as PiCompleteSimpleForTest; - const adapter = createRealPiAiAdapter({ complete, completeSimple }); + return streamForMessage(assistant([validSubmitReviewCall("submit-raw-forced")])); + }) as PiStreamForTest; + const streamSimple = vi.fn(() => streamForMessage(assistant([validSubmitReviewCall("must-not-use-simple")]))) as unknown as PiStreamSimpleForTest; + const adapter = createRealPiAiAdapter({ stream, streamSimple }); await adapter.complete( { @@ -4278,10 +4889,10 @@ describe("Phase 4 Pi runner and model-call cache", () => { apiKey: "fake-api-key" }, { messages: [], tools: [] }, - { reasoning: "high", toolChoice: { type: "tool", name: "submit_review" }, maxRetries: 0 } + { reasoning: "high", toolChoice: { type: "tool", name: "submit_review" }, maxRetries: 0, submitToolName: "submit_review" } ); - expect(completeSimple).not.toHaveBeenCalled(); + expect(streamSimple).not.toHaveBeenCalled(); expect(rawOptionsSeen).toEqual([ expect.objectContaining({ apiKey: "fake-api-key", @@ -4294,12 +4905,12 @@ describe("Phase 4 Pi runner and model-call cache", () => { it("disables Anthropic thinking and applies real forced tool choice for submit turns (plan 86 step 3)", async () => { const rawOptionsSeen: Record[] = []; - const complete = (async (_model, _context, options) => { + const stream = ((_model, _context, options) => { rawOptionsSeen.push(options as Record); - return assistant([validSubmitReviewCall("submit-anthropic-forced")]); - }) as PiCompleteForTest; - const completeSimple = vi.fn(async () => assistant([validSubmitReviewCall("must-not-use-simple")])) as unknown as PiCompleteSimpleForTest; - const adapter = createRealPiAiAdapter({ complete, completeSimple }); + return streamForMessage(assistant([validSubmitReviewCall("submit-anthropic-forced")])); + }) as PiStreamForTest; + const streamSimple = vi.fn(() => streamForMessage(assistant([validSubmitReviewCall("must-not-use-simple")]))) as unknown as PiStreamSimpleForTest; + const adapter = createRealPiAiAdapter({ stream, streamSimple }); await adapter.complete( { @@ -4309,10 +4920,10 @@ describe("Phase 4 Pi runner and model-call cache", () => { apiKey: "fake-api-key" }, { messages: [], tools: [] }, - { reasoning: "high", toolChoice: { type: "tool", name: "submit_review" }, maxRetries: 0 } + { reasoning: "high", toolChoice: { type: "tool", name: "submit_review" }, maxRetries: 0, submitToolName: "submit_review" } ); - expect(completeSimple).not.toHaveBeenCalled(); + expect(streamSimple).not.toHaveBeenCalled(); expect(rawOptionsSeen).toEqual([ expect.objectContaining({ apiKey: "fake-api-key", @@ -4327,11 +4938,11 @@ describe("Phase 4 Pi runner and model-call cache", () => { it("keeps the legacy Anthropic downgrade when forceSubmitToolChoice is disabled", async () => { const rawOptionsSeen: Record[] = []; - const complete = (async (_model, _context, options) => { + const stream = ((_model, _context, options) => { rawOptionsSeen.push(options as Record); - return assistant([validSubmitReviewCall("submit-anthropic-downgraded")]); - }) as PiCompleteForTest; - const adapter = createRealPiAiAdapter({ complete }); + return streamForMessage(assistant([validSubmitReviewCall("submit-anthropic-downgraded")])); + }) as PiStreamForTest; + const adapter = createRealPiAiAdapter({ stream }); await adapter.complete( { @@ -4341,7 +4952,7 @@ describe("Phase 4 Pi runner and model-call cache", () => { apiKey: "fake-api-key" }, { messages: [], tools: [] }, - { reasoning: "high", toolChoice: { type: "tool", name: "submit_review" }, maxRetries: 0, forceSubmitToolChoice: false } as never + { reasoning: "high", toolChoice: { type: "tool", name: "submit_review" }, maxRetries: 0, forceSubmitToolChoice: false, submitToolName: "submit_review" } ); expect(rawOptionsSeen).toEqual([ @@ -4372,20 +4983,20 @@ describe("Phase 4 Pi runner and model-call cache", () => { clear: () => entries.clear() }; const optionsSeen: Record[] = []; - const completeSimple = (async (_model, _context, options) => { + const streamSimple = ((_model, _context, options) => { optionsSeen.push(options as Record); - return assistant([validSubmitReviewCall("submit-oauth-refresh")]); - }) as PiCompleteSimpleForTest; + return streamForMessage(assistant([validSubmitReviewCall("submit-oauth-refresh")])); + }) as PiStreamSimpleForTest; const getOAuthApiKey = vi.fn(async (_provider: string, credentials: Record) => { expect(credentials["github-copilot"]).toEqual(oldCredentials); return { newCredentials, apiKey: "new-oauth-api-key" }; }) as NonNullable; - const adapter = createRealPiAiAdapter({ authStorage, completeSimple, getOAuthApiKey }); + const adapter = createRealPiAiAdapter({ authStorage, streamSimple, getOAuthApiKey }); await adapter.complete( { provider: "github-copilot", id: "fake-model", raw: { id: "fake-model" }, oauthProvider: "github-copilot" }, { messages: [], tools: [] }, - { maxRetries: 0 } + { maxRetries: 0, submitToolName: "submit_review" } ); expect(getOAuthApiKey).toHaveBeenCalledTimes(1); @@ -4602,22 +5213,58 @@ function scriptedAdapter(messages: PiAssistantMessage[]): PiAiAdapter & { contex } function assistant(content: PiAssistantMessage["content"]): PiAssistantMessage { + 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 + ); return { role: "assistant", provider: "fake", model: "fake-model", - content, + content: contentWithProvenance, usage: { input: 10, output: 5, totalTokens: 15, cost: { total: 0.01 } }, - stopReason: content.some((block) => (block as PiToolCall).type === "toolCall") ? "toolUse" : "stop", + stopReason: contentWithProvenance.some((block) => (block as PiToolCall).type === "toolCall") ? "toolUse" : "stop", timestamp: 0 }; } +function streamForMessage(message: PiAssistantMessage) { + const stream = createAssistantMessageEventStream(); + stream.push({ type: "start", partial: message as never }); + message.content.forEach((block, contentIndex) => { + if ((block as { type?: unknown }).type !== "toolCall") { + return; + } + const call = block as PiToolCall; + stream.push({ type: "toolcall_start", contentIndex, partial: message as never }); + stream.push({ + type: "toolcall_delta", + contentIndex, + delta: JSON.stringify(call.arguments), + partial: message as never + }); + stream.push({ type: "toolcall_end", contentIndex, toolCall: call as never, partial: message as never }); + }); + if (message.stopReason === "error" || message.stopReason === "aborted") { + stream.push({ type: "error", reason: message.stopReason, error: message as never }); + } else { + stream.push({ + type: "done", + reason: message.stopReason === "length" ? "length" : message.stopReason === "toolUse" ? "toolUse" : "stop", + message: message as never + }); + } + return stream; +} + function assistantError(errorMessage: string): PiAssistantMessage { return { role: "assistant", @@ -4649,6 +5296,21 @@ function validSubmitReviewCall(id: string): PiToolCall { }; } +function trustedSubmitCall(call: PiToolCall): PiToolCall { + return { ...call, argumentParse: { state: "strict" } }; +} + +function invalidSubmitCall(id: string, name: string, argumentParse: PiUntrustedArgumentParse): PiInvalidToolCall { + return { type: "invalidToolCall", id, name, argumentParse }; +} + +function provenanceClassification(parse: PiUntrustedArgumentParse): string { + if (parse.state === "length_stopped") return "length_stopped"; + if (parse.state === "partial") return "final_arguments_partial"; + if (parse.state === "invalid") return "final_arguments_invalid"; + return parse.state; +} + function validCandidateSubmitReviewCall(id: string): PiToolCall { return { type: "toolCall", diff --git a/tests/pipeline-phase5.test.ts b/tests/pipeline-phase5.test.ts index 453a13a..9c0ba82 100644 --- a/tests/pipeline-phase5.test.ts +++ b/tests/pipeline-phase5.test.ts @@ -4719,7 +4719,7 @@ describe("phase 5 pipeline regressions", () => { runStructured: async (request: LlmStructuredRequest) => { expect(request.stage).toBe(5); expect(request.schemaRepair?.replaceConversation).toBe(true); - expect(request.schemaRepair?.failAfterRepair).toBe(true); + expect(request.schemaRepair?.failAfterRepair).toBe(false); repairPrompt = request.schemaRepair?.buildPrompt?.({ stage: 5, submitTool: "submit_plan", @@ -6547,6 +6547,12 @@ describe("phase 5 pipeline regressions", () => { expect(result.postingPlan?.reviewBody).toContain("Coverage disclosure:"); expect(result.postingPlan?.reviewBody).toContain(partialReason); + expect(result.postingPlan?.reviewBody).toContain("**Review incomplete.**"); + expect(result.postingPlan?.reviewBody).toContain("incomplete coverage or verification prevents a clean conclusion"); + expect(result.postingPlan?.reviewBody).not.toContain("Everything looks good"); + expect(result.postingPlan?.reviewBody.indexOf("**Review incomplete.**")).toBeLessThan( + result.postingPlan?.reviewBody.indexOf("Review incomplete: completed work") ?? Number.MAX_SAFE_INTEGER + ); }); it("does not disclose deterministic default coverage as a planner fallback", () => { @@ -6598,14 +6604,49 @@ describe("phase 5 pipeline regressions", () => { expect(coverage.failedHunks).toBe(1); expect(coverage.partial).toBe(true); expect(coverage.reasons).toContain("1 hunk(s) could not be reviewed"); - expect(renderMarkdownReview({ + const markdown = renderMarkdownReview({ summary: "Review completed.", coverage, findings: [], summaryOnlyFindings: [], needsHumanAttention: [], noFindings: true - })).toContain("**Partial review:** 1 hunk did not complete review."); + }); + expect(markdown).toContain("**Partial review:** 1 hunk did not complete review."); + expect(markdown).toContain("## ⚠️ Review Incomplete"); + expect(markdown).not.toContain("Everything looks good"); + expect(markdown.indexOf("**Review incomplete.**")).toBeLessThan(markdown.indexOf("Review completed.")); + }); + + it("renders planner degradation prominently without inventing partial coverage", () => { + const coverage: RunCoverageStatus = { + totalHunks: 1, + reviewedHunks: 1, + skippedHunks: 0, + failedHunks: 0, + coverageByLevel: { deep: 0, normal: 1, light: 0, skip: 0 }, + degradedPlanning: true, + budgetStopped: false, + verificationIncompleteCount: 0, + partial: false, + reasons: [] + }; + const markdown = renderMarkdownReview({ + summary: "Review completed.", + coverage, + findings: [], + summaryOnlyFindings: [], + needsHumanAttention: [], + noFindings: true + }); + + expect(markdown).toContain("**Degraded run: planner fallback.**"); + expect(markdown.indexOf("**Degraded run: planner fallback.**")).toBeLessThan(markdown.indexOf("Review completed.")); + expect(markdown).toContain("## ✅ No Findings"); + expect(markdown).toContain("Everything looks good"); + expect(markdown).not.toContain("**Review incomplete.**"); + expect(markdown).not.toContain("## ⚠️ Review Incomplete"); + expect(markdown).not.toContain("**Partial review:**"); }); it("attaches left-side static signals to old-side dossier hunks", async () => { @@ -8385,6 +8426,69 @@ describe("phase 5 pipeline regressions", () => { ])); }); + it("preserves the explicit final-argument failure classification in verifier repair", async () => { + let repairPrompt = ""; + const events: Array> = []; + const runner: LlmRunner = { + runStructured: async (request: LlmStructuredRequest) => { + repairPrompt = request.schemaRepair?.buildPrompt?.({ + stage: 9, + submitTool: "submit_verdict", + error: "The submit_verdict final arguments were not trusted: final_arguments_invalid.", + submitCalls: [], + untrustedSubmitCalls: [{ + id: "invalid-verdict", + name: "submit_verdict", + state: "invalid", + errorKind: "invalid_syntax" + }], + extraToolNames: [], + classification: "final_arguments_invalid" + }) ?? ""; + return { + verdict: "keep", + reason: "The bounded candidate evidence proves the issue after one stateless repair.", + requiredEvidencePresent: true, + falsePositiveRisk: "low" + } as T; + } + }; + + const verified = await verifyFindings( + { + packetResults: [{ packetId: "packet-1", lenses: ["core/code-review"], findings: [fakeFinding()], followUpHints: [], uncertainties: [], status: "completed" }], + packets: [fakePacket()] + }, + fakeTools(), + config(), + { + ...nullTelemetry(), + event: (event: Omit) => events.push(event) + }, + { + runner, + promptBuilder: fakePromptBuilder(), + lensRegistry: fakeLensRegistry(), + diff: fakeDiff(), + checkpoint: () => "ok" + } + ); + + expect(repairPrompt).toContain("- class: final_arguments_invalid"); + expect(repairPrompt).not.toContain("- class: missing_submit_tool"); + expect(verified.verified).toHaveLength(1); + expect(events).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: "verification_schema_invalid", + data: expect.objectContaining({ classification: "final_arguments_invalid" }) + }), + expect.objectContaining({ + message: "verification_schema_repair_attempted", + data: expect.objectContaining({ classification: "final_arguments_invalid" }) + }) + ])); + }); + it("fails closed when an empty authoritative Stage 9 submit precedes later XML", async () => { let repairPrompt = ""; const events: Array> = []; @@ -8406,6 +8510,12 @@ describe("phase 5 pipeline regressions", () => { }; const runner: LlmRunner = { runStructured: async (request: LlmStructuredRequest) => { + expect(request.validateSubmit?.({ + verdict: "revise", + reason: "prose-only revision", + requiredEvidencePresent: true, + falsePositiveRisk: "low" + } as T)).toEqual({ ok: false, classification: "revise_without_revision_payload" }); repairPrompt = request.schemaRepair?.buildPrompt?.({ stage: 9, submitTool: "submit_verdict", @@ -8550,6 +8660,7 @@ describe("phase 5 pipeline regressions", () => { expect(repairPrompt).toContain("- class: revise_without_revision_payload"); expect(repairPrompt).toContain("include finalFinding or revisedAnchor"); + expect(repairPrompt).toContain("at most 2,000 characters"); expect(verified.verdicts[0]).toMatchObject({ verdict: "revise", revisedAnchor: { path: "app.ts", line: 1 } }); expect(verified.verified).toHaveLength(1); expect(events).toContainEqual(expect.objectContaining({ @@ -8558,6 +8669,49 @@ describe("phase 5 pipeline regressions", () => { })); }); + it("accepts an evidence-backed verifier reason inside the hard buffer unchanged and records target friction", async () => { + const reason = "r".repeat(2_327); + const events: Array> = []; + const verified = await verifyFindings( + { + packetResults: [{ packetId: "packet-1", lenses: ["core/code-review"], findings: [fakeFinding()], followUpHints: [], uncertainties: [], status: "completed" }], + packets: [fakePacket()] + }, + fakeTools(), + config(), + { + ...nullTelemetry(), + event: (event) => events.push(event) + }, + { + runner: { + runStructured: async () => ({ + verdict: "keep", + reason, + requiredEvidencePresent: true, + falsePositiveRisk: "low" + }) as T + }, + promptBuilder: createPromptBuilder(fakeLensRegistry()), + lensRegistry: fakeLensRegistry(), + diff: fakeDiff(), + checkpoint: () => "ok" + } + ); + + expect(verified.verdicts[0]?.reason).toBe(reason); + expect(events).toContainEqual(expect.objectContaining({ + stage: 9, + message: "verification_reason_target_exceeded", + data: expect.objectContaining({ + actualLength: 2_327, + target: 2_000, + hardMaximum: 4_000, + followedModelRepair: false + }) + })); + }); + it("marks an empty revise incomplete when its compact repair also fails", async () => { const events: Array> = []; const artifacts = new Map(); @@ -13580,7 +13734,8 @@ function toolCall(id: string, name: string, args: Record): PiTo type: "toolCall", id, name, - arguments: args + arguments: args, + ...(name.startsWith("submit_") ? { argumentParse: { state: "strict" as const } } : {}) }; } diff --git a/tests/pipeline-phase6.test.ts b/tests/pipeline-phase6.test.ts index 4c72f8f..9b95fc5 100644 --- a/tests/pipeline-phase6.test.ts +++ b/tests/pipeline-phase6.test.ts @@ -446,7 +446,8 @@ function toolCall(id: string, name: string, args: Record): PiTo type: "toolCall", id, name, - arguments: args + arguments: args, + ...(name.startsWith("submit_") ? { argumentParse: { state: "strict" as const } } : {}) }; } diff --git a/tests/repository-intelligence.test.ts b/tests/repository-intelligence.test.ts index daf3737..9844379 100644 --- a/tests/repository-intelligence.test.ts +++ b/tests/repository-intelligence.test.ts @@ -664,7 +664,7 @@ export { internal as Public } }) ]) ); - }); + }, 15_000); it("finds declaration text when parser summaries miss a symbol in a parsed file", async () => { const source = `package quotes diff --git a/tests/skill-semantic-fixtures.test.ts b/tests/skill-semantic-fixtures.test.ts index 363d69f..0d9a3a3 100644 --- a/tests/skill-semantic-fixtures.test.ts +++ b/tests/skill-semantic-fixtures.test.ts @@ -105,7 +105,7 @@ describe("Plan 101 language-skill semantic fixtures", () => { expect(projection.perSkill.every((entry) => !entry.omitted && entry.truncatedChars === 0), `${semanticCase.language} stage ${String(stage)}`).toBe(true); } } - }); + }, 15_000); it("builds TypeScript base/feature, preserves the safe control, and reproduces the runtime-validation failure", () => { const repo = materialize("typescript", false); diff --git a/tests/telemetry.test.ts b/tests/telemetry.test.ts index 4a9e229..80d07fb 100644 --- a/tests/telemetry.test.ts +++ b/tests/telemetry.test.ts @@ -717,6 +717,103 @@ describe("run telemetry", () => { }); }); + it("summarizes only bounded final-argument provenance and repair outcomes", async () => { + const repoRoot = tempDir(); + const run = createRunTelemetry({ + telemetryConfig: { ...defaultConfig.telemetry, enabled: true, logLevel: "debug" }, + idFactory: () => "20260805-final-argument-provenance" + }); + const attached = await run.attachRunDirectory(repoRoot); + const base = { + stage: 7 as const, + role: "packetReview" as const, + model: "model", + provider: "provider", + promptChars: 12, + promptHash: "prompt", + outputChars: 0, + outputHash: "bounded-output-hash", + durationMs: 10, + stopReason: "submit" as const + }; + run.recorder.recordModelCall({ + ...base, + callId: "mc-1", + kind: "initial", + attempt: 1, + cacheStatus: "miss", + schemaValid: false, + status: "schema_invalid", + submitTool: "submit_review", + finalArgumentState: "partial", + finalArgumentErrorKind: "unterminated", + finalArgumentCorrelationId: "mc-1:submit" + }); + run.recorder.recordModelCall({ + ...base, + callId: "mc-2", + kind: "repair", + attempt: 1, + cacheStatus: "write", + schemaValid: true, + status: "ok", + submitTool: "submit_review", + finalArgumentState: "repaired", + finalArgumentRepairKind: "pi_narrow_string_repair", + finalArgumentCorrelationId: "mc-2:submit" + }); + run.recorder.recordModelCall({ + ...base, + callId: "mc-3", + kind: "initial", + attempt: 1, + cacheStatus: "hit", + schemaValid: true, + status: "ok", + submitTool: "submit_review", + finalArgumentState: "strict", + finalArgumentCorrelationId: "mc-3:submit" + }); + run.recorder.event({ + stage: 7, + level: "info", + message: "final_argument_repair_outcome", + data: { correlationId: "mc-1:submit", outcome: "recovered" } + }); + await run.finalize({ status: "completed_full", exitCode: 0 }); + + const summary = readJson(runFilePath(attached.runDir, "model-calls-summary.json")); + 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 }); + expect(summary).toMatchObject({ + totalRecords: 3, + providerCalls: 2, + cache: { hit: 1, miss: 2, write: 1 } + }); + + const modelCalls = readJsonl(runFilePath(attached.runDir, "model-calls.jsonl")); + expect(modelCalls).toEqual(expect.arrayContaining([ + expect.objectContaining({ + callId: "mc-3", + cacheStatus: "hit", + finalArgumentState: "strict", + finalArgumentCorrelationId: "mc-3:submit" + }) + ])); + const allowedFinalArgumentFields = new Set([ + "finalArgumentState", + "finalArgumentErrorKind", + "finalArgumentRepairKind", + "finalArgumentCorrelationId" + ]); + for (const record of modelCalls) { + const finalArgumentFields = Object.keys(record).filter((key) => key.startsWith("finalArgument")); + expect(finalArgumentFields.length).toBeGreaterThan(0); + expect(finalArgumentFields.every((key) => allowedFinalArgumentFields.has(key))).toBe(true); + } + }); + it("aggregates tool-result cache telemetry in run artifacts", async () => { const repoRoot = tempDir(); const run = createRunTelemetry({ diff --git a/tests/verifier.test.ts b/tests/verifier.test.ts index 7df3630..09ca3d1 100644 --- a/tests/verifier.test.ts +++ b/tests/verifier.test.ts @@ -1083,7 +1083,7 @@ describe("plan 106 verifier revision semantics", () => { data: { candidateId: finding.id, submitTool: "submit_verdict", - schemaVersion: 3, + schemaVersion: 4, argumentsNonEmpty: true, schemaRepairUsed: false }