From 7ca8140fd7f0cc632b640711d938158821ae7e72 Mon Sep 17 00:00:00 2001 From: Peter Kieltyka Date: Thu, 24 Sep 2026 11:56:05 -0400 Subject: [PATCH 1/2] Improve review defaults, routing, and text-mode accounting for 0.6.1 Enable compositionReasoningStepDown by default while preserving explicit false values from configuration and the existing CLI overrides. Update the README and configuration/CLI regression coverage for the new default. Treat unsupported formats as normal text-mode operation instead of syntax fallback degradation. Apply this consistently to outlines, symbol reads, definition discovery, symbol mentions, packet accounting, and high-risk context warnings. Continue reporting fallback from configured parsers as degraded, preserve text/heuristic precision, and avoid claiming syntax verification for empty mention results or mixed text/syntax definitions. Cover RIDL, JSON, YAML, Markdown, SQL, unknown extensions, extensionless files, mixed search results, unavailable configured grammars, and generic packet context. Preserve real retrieval errors and context-limit reporting. Update OpenRouter DeepSeek routing to use deepseek, fireworks, and together in that order for both only/order, retaining allow_fallbacks=false and the focused automatic submit-tool compatibility path. Update payload tests and documentation to match the routing configuration. Add repository review configuration that excludes specs and eval/test fixture data. Bump the package to 0.6.1 and synchronize documented GitHub Action references with that version. Add reviewed Plan 123 and its index entry. Document reproduced OMSX search scope and result-packing defects, honest failure/completion reporting, evidence-backed question reconciliation, and consequential missing-test criteria. Plan 123 remains unimplemented for the next iteration. Validation: all 1,309 tests in 60 files, workflow checks, TypeScript checking, package build, and git diff --check pass. No live inference calls were needed for validation. --- README.md | 8 +- codegenie.toml | 12 ++ .../workflows/codegenie-review-comment.yml | 2 +- examples/workflows/codegenie-review-pr.yml | 2 +- package.json | 2 +- ...rch-evidence-and-honest-review-outcomes.md | 166 ++++++++++++++++++ specs/plans/README.md | 1 + src/config/schema.ts | 2 +- src/pipeline/packet-builder.ts | 7 +- src/provider/models-override.ts | 7 +- src/repo/packet-context.ts | 10 +- src/repo/repository-index.ts | 27 ++- src/repo/search.ts | 19 +- tests/config-loader.test.ts | 3 + tests/language-foundation.test.ts | 9 +- tests/models-override.test.ts | 2 +- tests/repository-intelligence.test.ts | 82 ++++++++- tests/review-command.test.ts | 2 +- 18 files changed, 328 insertions(+), 35 deletions(-) create mode 100644 codegenie.toml create mode 100644 specs/plans/123-issue-123-search-evidence-and-honest-review-outcomes.md diff --git a/README.md b/README.md index c2d873c..957ec8c 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ codegenie provider use glm-5.3:max # -> openrouter/z-ai/glm-5.3 codegenie review ``` -Codegenie's built-in model overrides pin all OpenRouter models with IDs starting with `deepseek/` to the `deepseek` upstream with `only`/`order` and `allow_fallbacks: false`. For this pinned model family, submit calls expose only the submit tool and use `tool_choice: "auto"`, to accommodate the named forced-tool rejection observed on DeepSeek V4.1 Flash. Returned submissions still undergo strict validation, and missing submissions have bounded retries. These [OpenRouter routing preferences](https://openrouter.ai/docs/guides/routing/provider-selection) apply to every stage, including repairs; Codegenie's stage-specific reasoning levels still apply. Requests cannot fall back to another upstream if DeepSeek is unavailable. Routing is included in debug request traces and local model-call cache keys. The overrides live in `src/provider/models-override.ts`. +Codegenie's built-in model overrides pin all OpenRouter models with IDs starting with `deepseek/` to the `deepseek`, `fireworks`, and `together` upstreams (in that order) with `only`/`order` and `allow_fallbacks: false`. For this pinned model family, submit calls expose only the submit tool and use `tool_choice: "auto"`, to accommodate the named forced-tool rejection observed on DeepSeek V4.1 Flash. Returned submissions still undergo strict validation, and missing submissions have bounded retries. These [OpenRouter routing preferences](https://openrouter.ai/docs/guides/routing/provider-selection) apply to every stage, including repairs; Codegenie's stage-specific reasoning levels still apply. Requests cannot fall back to upstreams outside that list. Routing is included in debug request traces and local model-call cache keys. The overrides live in `src/provider/models-override.ts`. OpenRouter models with IDs starting with `z-ai/` use `only: ["together", "fireworks", "cloudflare"]`, `order: ["together", "fireworks", "cloudflare"]`, and `allow_fallbacks: false`. This routing override applies across stages, including repairs, and preserves the model's reasoning and tool-choice behavior. It does not add `require_parameters`. @@ -102,7 +102,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.6.0 + - uses: 0xPolygon/codegenie@v0.6.1 with: # Works with any model! model: "openrouter/deepseek/deepseek-v4.1-flash:max" @@ -145,7 +145,7 @@ baseBranch = "main" depth = "normal" maxTime = 60 # positive number of minutes; --max-time overrides this per run budgetBoost = 1.0 # scales per-packet review budgets; does not change finding caps -compositionReasoningStepDown = false # opt in to one lower supported reasoning level for composition +compositionReasoningStepDown = true # default; set false to keep configured reasoning for composition [telemetry] enabled = true # opt into local run artifacts under .codegenie/runs @@ -267,7 +267,7 @@ expect: The planning check rejects degraded plans even when every hunk was reviewed. The composition check rejects degraded report synthesis separately from coverage completeness. The recovery check requires complete telemetry, no unresolved structured-output obligations, and demonstrated preservation; regenerated or revised content is reported as `unknown`, not assumed preserved. Repairs retain draft progress across retries and validate the whole merged submission. For unreadable JSON, repair prompts include a bounded, redacted syntax excerpt and parser diagnostic when available. Fragments remain untrusted diagnostics, never accepted data or proof that a replacement preserved the original. -Composition uses the configured review reasoning level by default, including retries. Set `[review] compositionReasoningStepDown = true` in `codegenie.toml` to use the next lower level supported by the model: for a model supporting `low`, `high`, and `max`, `max` becomes `high`. The lowest supported level stays unchanged; models without advertised reasoning levels retain the configured behavior. Override this per run with `codegenie review --composition-reasoning-step-down` or `--no-composition-reasoning-step-down`. Omitting both flags preserves the configuration, which defaults to `false`. Investigation and verification keep their configured reasoning; traces record configured and selected levels. Structured-output repairs continue to use the model’s lowest supported reasoning level. Each composition attempt has a 300-second deadline, with at most one retry. The outer composition deadline is 780 seconds (two attempts plus the shared 180-second repair allowance); overall review cancellation still takes precedence. Repair attempts share that 180-second allowance, rather than receiving 180 seconds each. +Composition uses the next lower supported reasoning level by default, including retries: for a model supporting `low`, `high`, and `max`, `max` becomes `high`. Set `[review] compositionReasoningStepDown = false` in `codegenie.toml` to keep the configured review reasoning level for composition. The lowest supported level stays unchanged; models without advertised reasoning levels retain the configured behavior. Override this per run with `codegenie review --composition-reasoning-step-down` or `--no-composition-reasoning-step-down`. Omitting both flags preserves the configuration, which defaults to `true`. Investigation and verification keep their configured reasoning; traces record configured and selected levels. Structured-output repairs continue to use the model’s lowest supported reasoning level. Each composition attempt has a 300-second deadline, with at most one retry. The outer composition deadline is 780 seconds (two attempts plus the shared 180-second repair allowance); overall review cancellation still takes precedence. Repair attempts share that 180-second allowance, rather than receiving 180 seconds each. Composition validates source references before acceptance. It locally removes repeated known references and misplaced references already correctly accounted for in the same finding, records those removals, and validates the whole result. Remaining attribution errors receive bounded repairs in a fresh context with exact field paths and source inventories. Attribution patches replace only permitted reference lists; finding order and prose stay intact, and the assembled report must pass full validation. If a recommendation lacks support, a bounded composition repair may instead omit or rewrite that advice section while preserving the diagnosis and retaining its original sources. Reports consolidate identical evidence and keep additional verbatim evidence and caveats in expandable sections. If synthesis fails, the report identifies its source-based presentation and retains distinct contributions. `stages/10-composition/composition-sources.json` records all inputs and dispositions; references establish attribution, not proof of semantic equivalence. Verification distinguishes essential missing proof from secondary uncertainty: unresolved hypotheses remain visible under human attention, while established defects may still have uncertainty about severity. diff --git a/codegenie.toml b/codegenie.toml new file mode 100644 index 0000000..88b989f --- /dev/null +++ b/codegenie.toml @@ -0,0 +1,12 @@ +[review] +compositionReasoningStepDown = true + +[[classification.pathRules]] +pattern = "**/specs/**" +processingMode = "skip" +reason = "spec documents are not reviewed" + +[[classification.pathRules]] +pattern = "{evals,tests/fixtures}/**" +processingMode = "skip" +reason = "eval and test fixture data, often intentionally defective" diff --git a/examples/workflows/codegenie-review-comment.yml b/examples/workflows/codegenie-review-comment.yml index f8a1cdd..d5a649f 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.6.0 + - uses: 0xPolygon/codegenie@v0.6.1 with: model: "openrouter/deepseek/deepseek-v4.1-flash:max" # model: "openrouter/z-ai/glm-5.3:max" diff --git a/examples/workflows/codegenie-review-pr.yml b/examples/workflows/codegenie-review-pr.yml index 492bce4..1ecfcbd 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.6.0 + - uses: 0xPolygon/codegenie@v0.6.1 with: model: "openrouter/deepseek/deepseek-v4.1-flash:max" # model: "openrouter/z-ai/glm-5.3:max" diff --git a/package.json b/package.json index c7bf52b..16b5357 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@0xsequence/codegenie", - "version": "0.6.0", + "version": "0.6.1", "description": "High-signal AI code review agent", "type": "module", "bin": { diff --git a/specs/plans/123-issue-123-search-evidence-and-honest-review-outcomes.md b/specs/plans/123-issue-123-search-evidence-and-honest-review-outcomes.md new file mode 100644 index 0000000..02719d4 --- /dev/null +++ b/specs/plans/123-issue-123-search-evidence-and-honest-review-outcomes.md @@ -0,0 +1,166 @@ +# Issue 123: Reliable Search Evidence and Honest Review Outcomes + +Status: DRAFT — reviewed; implementation pending +Based on: OMSX runs `20260924-150110-7e5144d6` and `20260924-151933-2809b013`, 2026-09-24 +Depends on: existing repository tools, bounded budgets, strict verification/composition validation, and plans 118–122 + +## Objective + +Prevent avoidable evidence loss, distinguish operational failure from an absence of confirmed findings, reconcile questions with evidence already collected, and make missing-test findings proportionate to demonstrated consequences. Implement in that order. Keep reasoning settings, budgets, deadlines, repair counts, and provider routing unchanged during measurement. + +The governing reporting requirement is: a known harness or provider failure that materially prevents reliable completion must appear at the top of the report, with its available reason. It must never become an unqualified “no findings” or “everything looks good” result. Preserve useful findings from completed work below that notice. + +## Evidence and limits + +Both runs reviewed OMSX head `d42543c08c1a6f2e11f78469d8ff246aa263d709` against merge base `4280b3f633ac1fff83898d5a922c15ad8221ceb3`, with identical 50,511-character diffs. They were not a controlled model-only comparison: DeepSeek used a clean Codegenie build at `60ab24b474`, whereas Astra used a dirty build at the same commit. Composition used max for DeepSeek and low for Astra, stepped down from medium. + +| Observation | DeepSeek 4.1 Flash / max | GPT-6 Astra / medium | +| --- | --- | --- | +| Completion | Complete; no global budget stop | Complete; no global budget stop | +| Elapsed | 10m12s | 1m59s | +| Calls / tokens | 70 / 1,516,926 | 41 / 522,729 | +| Recorded estimated cost | $0.2668 | $3.0319 | +| Schema repairs | Two, both recovered | None | +| Final findings | Two low-severity missing-test findings | None | +| Local refusals | Two in packet review | One in packet review; two in verification | +| Human-attention items | Three | One | + +Neither run had a provider outage, model timeout, or composition fallback. These are recorded usage estimates, not independently verified account charges. Their degradation counters predate the unsupported-language accounting correction and are not comparable measures of lost evidence. + +### Reproduced defects + +1. **Inconsistent path-glob interpretation.** Astra's verifier searched `Statuses|func List` in `apps/api/{data,domain/users}/**`. File listing resolves this glob to 196 files; search returns zero matches because it passes the pattern directly to Git's different glob syntax. Separate directory searches return nine matches, including the downstream implementation. The corrective searches were then refused after eight tool calls, despite 25,810 of 32,000 result characters remaining. The verifier correctly withheld an unsupported conclusion but lacked evidence our tools should have supplied. +2. **Optional metadata removes all search results.** Both runs' automatic symbol searches returned no results after truncation. The enclosing Go `expectedPerms` variable carried its large initializer in a roughly 22,000-character symbol record. The search packer drops records until below its 16,000-character cap. A local experiment omitting the optional signature recovered four `expectedPerms` mentions and 27 `ListProjectAccess` mentions without raising the cap. Match contents and locations should take precedence over verbose surrounding metadata. +3. **Contradictory report outcomes.** Astra's report ends “Everything looks good” despite a retained essential verification question. Both reports call deliberate generated-file exclusions “Incomplete work.” DeepSeek's human-attention section asks whether `ProjectRole` resolves after verification explicitly confirmed its declaration and import. +4. **Different thresholds for testing findings.** DeepSeek promoted two missing invalid-request tests; Astra judged the same test suites meaningful and found no concrete defect in them. This does not prove the proposed tests are useless or every coverage finding false. It exposes the need to distinguish a consequential regression gap from a generic opportunity to add tests. + +The failing combined-glob request and its full arguments are in Astra's `mc-000038.response.json`; refused corrections and an allowed exact-source extension are in `mc-000039.response.json` and tool records `tc-000100`–`tc-000102`. The unresolved verdict is in `stages/09-verification/verification.json`. DeepSeek's answered import question is in that run's verification artifact and still appears in `final-review.md`. + +## Already completed; preserve these changes + +The working branch now treats the generic text adapter as normal operation for every unsupported language/format, including unknown extensions and extensionless files. Configured syntax-adapter fallbacks still report degradation; text results retain truthful precision metadata. This applies to outlines, symbol reads, definitions, mentions, packet accounting, and high-risk context warnings. Empty mention results no longer claim syntax verification. Regression tests cover unsupported formats, mixed results, and a failed configured parser. + +Do not reimplement that accounting or treat text mode as evidence loss. Actual truncation, missing files, tool refusals, and parser failures remain observable. Historical artifacts are immutable; do not silently rewrite old counters or verdicts. + +## 1. Make repository glob semantics consistent + +- Give model-facing `pathGlob` one contract across file listing, search, symbol mentions, and definition discovery. Reuse the existing containment rules and shared matcher; do not maintain unrelated interpretations for different tools. +- Explicitly support brace alternatives such as `apps/api/{data,domain/users}/**` through that shared matcher. This request should succeed, not merely receive a new unsupported-pattern error. Its query `Statuses|func List` is valid POSIX extended regular-expression alternation and must continue to work. +- Prefer resolving the glob against the selected revision's tracked paths using the existing matcher, then searching those literal paths through Git. Keep any native-pathspec fast path only where equivalence is established. Use argument arrays and literal pathspecs: file names containing Git pathspec metacharacters must not broaden the search. +- Preserve committed-revision reads, head/base selection, case sensitivity, regex versus fixed-string query behavior, and repository containment. Never traverse worktree symlinks or include untracked files to implement matching. +- Bound expanded paths/argument sizes, chunking as necessary. Maintain deterministic result ordering and one overall match/result allowance across chunks. Do not turn an unsearched remainder, rejected pattern, or limit into an authoritative zero-match result. +- For a pattern outside the supported contract, return an actionable tool error. An accepted pattern with no matching tracked paths is a legitimate empty result; an unsupported pattern is not. +- Validate both inputs against their documented dialects: `pathGlob` uses the shared glob contract; `query` uses the existing POSIX ERE contract (or literal matching for fixed-string tools). Surface malformed syntax and recognized unsupported dialect constructs as argument errors, including cases where the chosen scope contains no files. Preserve genuine Git/revision/backend failures as execution errors rather than calling them invalid patterns or swallowing them into `[]`. Do not validate ERE using JavaScript's different regex grammar. +- Error results must reach the model with `isError: true`, identify the offending argument and syntax issue, and give a bounded correction hint or supported example. Record the same distinction in telemetry. The model may correct the request through the existing bounded tool loop; a fully corrected input mistake is not an unrecovered harness/provider failure under section 3A. +- Do not infer unsupported syntax from zero matches or guess intent when a pattern is valid in the documented dialect. Literal metacharacters must remain representable. The guarantee covers invalid/unsupported constructs we can identify under that contract, not arbitrary future model intent. +- Include the effective path glob and requested result limit in concise tool telemetry. The full debug request already retained these arguments, but the compact records omitted them. Keep existing redaction and size limits. +- Clarify tool descriptions: `read_symbol` requires a path; discovery without a path uses `find_definition`; document supported `contextMode` values and glob semantics. Do not introduce a new argument-repair loop or increase the tool-call allowance. + +Tests: brace alternatives, simple and recursive globs, literal metacharacters in tracked filenames, head/base differences, no matching files, unsupported/unsafe patterns, bounded expansion/chunking, and mixed source/schema directories. Cover query matching as well as filename matching. Confirm equivalent listing/search scope without requiring identical output formats. Use unrelated fixtures; the OMSX request is a read-only reproduction case, not a production special case. + +Include a combined brace-glob plus `|`-query regression; malformed glob and regex inputs; a recognized unsupported regex construct; invalid syntax with an empty file scope; valid patterns matching zero files or zero lines; literal/fixed-string metacharacters; and a backend failure. Assert model-facing errors, correction guidance, and telemetry, not only thrown exceptions. None of the error cases may arrive as a successful empty-result payload. + +Likely files: `src/repo/source-resolver.ts`, `src/repo/search.ts`, `src/git/git-client.ts`, `src/repo/path-guard.ts`, `src/llm/tool-definitions.ts`, tool argument telemetry, and repository/git/runner tests. + +Acceptance: an accepted combined glob retrieves the same bounded matches as its component scopes, and cannot silently report no matches because another backend interprets its syntax differently. + +## 2. Preserve useful matches within existing result budgets + +- Build bounded search-result representations before serialization. Preserve path, line/column, and a bounded match excerpt; attach compact enclosing-symbol identity/location rather than an entire declaration initializer or body. +- Trim optional symbol signatures and surrounding context before discarding matches. An oversized first entry must not force all subsequent useful entries out. Preserve honest omission/truncation information when even minimal entries cannot fit. +- Deduplicate repeated optional enclosing-symbol context where useful, without collapsing distinct match locations. Do not change the canonical symbol/source data used elsewhere merely to shrink a search response. +- For symbol mentions, perform the existing identifier/comment/string classification before the final enriched-result packing, within existing discovery and syntax-inspection limits. Discarded non-mentions must not exhaust the delivery allowance before valid mentions are considered. Preserve text precision for unsupported formats and explicit limitation metadata when inspection/discovery itself was bounded. +- Apply the actual delivery allowance when packing search/mention results, including the verifier's per-result cap and remaining character budget. Avoid packing to one cap and then slicing serialized JSON mid-entry at a smaller cap. Reuse the existing tool execution/budget plumbing with a narrow delivery-limit parameter if needed; do not add a generic response-rewriting framework. +- Keep per-consumer packing outside the shared tool-result cache: cache the bounded canonical search result, then fit a copy to each caller's allowance. A small-budget caller must not poison a later larger-budget hit, and a cached larger response must not bypass a smaller caller's cap. Account for result metadata/truncation notices within the existing charged-text allowance; preserve the runner's untrusted-data fencing. If no complete minimal entry and notice fit, return the existing explicit budget refusal rather than silently returning `[]` or sliced JSON. +- Keep complete entries and explicit continuation guidance such as narrowing `pathGlob`, reducing context, or reading a known range. Distinguish no matches from matches withheld by limits. Do not infer code absence from a truncated or rejected result. +- Keep omission accounting honest: a discovery limit established with one extra match proves that more results exist, not their exact total. Distinguish withheld matches from shortened optional metadata; do not claim a complete result count or complete code excerpt after truncation. Preserve the matching portion of long lines with its actual location, and mark excerpts as excerpts rather than reconstructing source text. +- Preserve the existing source-reading reserve and exact-source extension restrictions. A broad lookup should not consume the whole packet allowance solely through duplicated optional metadata; bounds still apply when there are genuinely many relevant matches. + +Tests: large Go composite literals, an unrelated long declaration in another supported language, repeated enclosing symbols, mixed supported/unsupported files, small final delivery caps, and a single unusually long path/line. Include cache hits and shared in-flight results delivered under different caps, both caller orderings, comments before real identifier mentions, and an allowance too small for one complete result. Check surviving locations, valid structured result text, bounded size, omission metadata, and follow-up reads. Do not assert that every truncated response must fit all matches. + +Acceptance: the two reproduced automatic searches retain useful matches under unchanged caps; smaller delivery caps preserve complete entries and cannot masquerade as successful exhaustive searches. + +## 3. Report failures, uncertainty, and answered questions honestly + +### 3A. Failure and completion reporting + +Use existing typed errors, stage outcomes, coverage/verification status, budget stops, and recovery records to derive report health once. Reuse it across renderers and entrypoints; do not infer failure by scanning model prose, counting all degraded results, or asking another model to classify run health. Add only the minimal structured diagnostics needed where existing outcomes lose an error's provenance. + +Apply precedence consistently: unrecovered fundamental failure, then incomplete required work, then completed work with unresolved questions, then clean completion. Preserve the underlying finding count independently of this status: zero findings is a count, not proof of successful review. Derive unresolved-question status from retained structured concerns after reconciliation, not from the rendered text. + +Determine health from all known required-work outcomes and unresolved concerns before presentation caps, including omitted attention items and skipped verification when candidates required it. A report-size cap must not hide the only limitation and thereby create a clean outcome. A successful optional adaptive pass does not repair a failed required pass, and an unsuccessful optional pass does not by itself invalidate a successfully completed baseline; retain its existing supplemental-work disclosure. + +| Situation | Required visible outcome | +| --- | --- | +| Unrecovered harness/provider error prevents required review, validation, or trustworthy report assembly | **Review failed** notice immediately below the report title, before any model summary; no clean/no-findings verdict. Preserve completed findings with an explicit partial-results label. | +| Required work stopped by a budget/deadline, without a separate fundamental failure | **Review incomplete** notice with the limiting stage/reason and completed scope; no clean conclusion. | +| Work completed, but a material question remains unresolved | **Review completed with unresolved questions** notice before the summary. “No confirmed findings” may describe the count, but must not imply the unresolved behavior is safe. | +| An earlier error recovered and the required work subsequently passed full validation | Completed outcome; preserve recovery diagnostics. Do not turn every recovered schema repair or corrected tool argument into a failed review. | +| Normal unsupported-language text mode or deliberate configured exclusions | Normal capability/scope disclosure, not failure or incomplete-work accounting. | +| No confirmed findings, no unresolved questions, and no incomplete/failed required work | A scoped no-findings conclusion is permitted; avoid an absolute safety guarantee. | + +- For errors, show affected stage/work item, stable error code/category, a bounded redacted underlying reason when available, and whether retry/recovery exhausted. Say that the cause was not captured if it is unknown; never guess billing, credentials, provider failure, or missing code from an empty result alone. +- A known tool/harness fault that leaves a required evidence question unresolved must not be hidden by a schema-valid `reject` verdict. Retain the operational diagnostic alongside the semantic verdict. A source limit with unresolved evidence is a limitation, not proof of a software defect or provider outage. +- Successful bounded truncation or an isolated refused request is not automatically fatal. Track whether required work remains unresolved; when recovery cannot be established, retain that limitation rather than claiming complete evidence. +- Classify a timeout by its cause and scope: a run/worker deadline that stops required work is incomplete work; an unrecovered provider or harness execution error is an operational failure. Keep successful, validated deterministic composition fallback visible as fallback, without automatically relabeling sound retained findings as a failed review. Failure to validate or faithfully assemble that fallback remains a failure. Use existing stage outcomes and recovery links; do not build a global semantic dependency tracker or infer recovery merely from a later schema-valid payload. +- On terminal exceptions before composition, write a minimal error report and bounded diagnostic artifact where output paths are available. Report creation must not require another LLM call. If diagnostic writing fails, preserve the original error and surface it on stderr/Action logs. +- Make CLI, saved Markdown/JSON, GitHub summaries/comments, and Action lifecycle agree. Unrecovered fundamental failures must retain/produce a nonzero failure outcome, including paths where workers currently return errors instead of throwing. Do not route a returned failed review through `finalizeSuccess`. Preserve successful partial findings/artifacts and existing exit conventions for nonfatal limitations; positive findings alone are not execution errors. +- Assemble/reuse the same host-derived outcome before creating the saved report and GitHub posting body. Patching only stdout after composition would leave a contradictory prebuilt PR summary or successful run artifact. Reuse existing publication authorization; this plan does not enable posting for failed runs where posting was not requested or permitted. +- Remove “Everything looks good” when there is retained uncertainty or incomplete work. Derive the trust notice and no-findings eligibility on the host. For failed, incomplete, or unresolved outcomes, use a factual host status/count summary; preserve substantive findings below it. Do not rely on a brittle English-phrase blacklist or another model call to detect contradictory reassurance in a generated summary. +- Separate **Excluded by configuration/planning** counts from **Incomplete/failed work**. Deliberately skipped generated files remain visible in coverage, without falsely implying failed execution. A planner/budget skip caused by inability to complete work must not be relabeled intentional to obtain a clean result. + +Tests must exercise the complete report path, not just a renderer flag: all review workers failing while the pipeline returns; provider authentication/billing/network failure after retries; incomplete or deliberately skipped verification; global/local budget limits; a fully recovered failure; a fundamental failure with some verified findings retained; zero findings with essential uncertainty; and successful reviews containing only intentional exclusions. Include attention-cap omission, optional-pass failure with a successful baseline, and validated versus invalid deterministic fallback. Check top-of-report precedence, absence of contradictory clean wording, diagnostic redaction, saved artifacts, the prebuilt GitHub body, CLI outcome, and Action finalization. A pre-fix silent search bug cannot be detected retrospectively from `[]` alone; do not claim automatic detection of arbitrary undiscovered harness defects. + +### 3B. Reconcile existing questions against explicit evidence + +- Extend the existing bounded reconciliation path to eligible packet-origin attention questions, which the current verifier-assumption-only inventory does not cover. Reuse stable packet/hint/group provenance and existing composition; no extra mandatory model call. +- Remove the current `publishableCount > 0` eligibility dependency when composition completed and resolution proposals passed validation. Explicit evidence may answer a question even when every candidate was rejected. Reuse the already-scheduled composition call, including its zero-findings case; if composition is skipped or falls back, retain concerns without an extra call. +- Supply relevant verified observations, including explicit refutations. Resolve a question only with exact concern identity and supporting source references that establish the same predicate at the reviewed revision. +- Assign stable IDs to original packet hints/questions before display grouping, and reconcile those records before rebuilding grouped notes. Treat an indivisible free-text question as one concern; do not split prose heuristically. Partial grouping resolution retains untouched member questions and their provenance. Reuse the existing resolution fields with minimally generalized concern identities; update their schema/prompt/cache versions if their contract changes. +- Preserve unresolved portions of grouped questions. Confirmation that a type is imported does not automatically prove every consumer import, lack of duplicate definitions, or generated-code compatibility. +- A reject verdict alone, shared file, matching keyword, or approximate question similarity is never a resolution rule. Incomplete verification cannot supply conclusive evidence. Missing/invalid references, omitted inventory entries, or fallback composition retain the concern. +- Record resolved, narrowed, and retained decisions with supporting IDs for audit. Reconcile before deriving the final report-health notice so genuinely answered questions do not keep it in an unresolved state. + +Tests: a rejected candidate whose explicit proof answers an exact packet question, including zero published findings; an unrelated question in the same file; grouped questions only partly answered; an indivisible question with only partial evidence; different revisions; incomplete verifier evidence; missing references; bounded inventory omission; and deterministic composition fallback. Include a non-schema example such as a test helper's assertion contract. A syntactically valid reference is necessary but does not itself prove semantic entailment: inspect resolution rationales in live comparisons, and do not describe this validation as an automatic proof of correctness. + +Likely files: `src/pipeline/attention-reconciliation.ts`, `human-attention.ts`, `composer.ts`, `review-runner.ts`, existing worker/verifier outcomes and shared result types, `src/util/coverage-summary.ts`, output renderers, CLI/Action finalization, and corresponding tests. Reuse plans 121/122 mechanisms rather than replacing them. + +Acceptance: fundamental errors are prominent even with zero findings; Astra-shaped uncertainty cannot end in a clean bill of health; answered concerns disappear only with explicit evidence; intentional exclusions do not contradict review completeness. + +## 4. Calibrate consequential missing-test findings + +- Refine the existing review and verification guidance, especially the tests lens. The absence of a test for a newly added branch alone is insufficient for an actionable finding. +- Revise the existing new-branch/negative-path bullets in `bundled-skills/core/tests.md` and align verification guidance with them; do not simply append competing instructions. Use the existing skill/prompt fingerprinting and version mechanisms so cached calls cannot reuse the previous instruction contract. No blanket severity/confidence demotion based on the `testing` category. +- Require an established material behavioral requirement, a concrete regression that would violate it, and evidence that the inspected relevant tests fail to protect that boundary. Do not demand an already-present production bug: tests that pass while violating a known important requirement remain actionable. +- Distinguish local evidence (“this suite has no malformed-input assertion”) from a repository-wide claim (“no test covers this behavior”). Inspect likely sister tests and transport/middleware validation where they bear on the claim; bounded or unsuccessful search cannot prove global absence. +- Explain why the gap matters here rather than merely citing a sibling testing convention or inventing a possible mutation. Optional extra coverage can be omitted; do not automatically move every withheld suggestion into human-attention noise. +- Establish material impact from repository/caller behavior or an explicit requirement, not only a commit title, a neighboring test style, or the model's claim that an error code is client-visible. Behavioral contracts may be evidenced by callers and boundary tests without written specifications. Preserve actionable coverage regressions where a valid guard could be removed without detection; no requirement to demonstrate an existing production failure or execute mutations during review. +- Keep existing recommendation checks: the proposed test must exercise a reachable boundary, accept valid remedies, and reject weakening the established requirement. Preserve uncertainty about uninspected contracts instead of manufacturing confidence. +- Use existing finding/proof/recommendation fields. No new mandatory assessment schema, repository-wide call sequence, blanket category suppression, or model-specific threshold. + +Regression cases: routine uncovered validation with no established material consequence; an already-covered sibling/transport boundary; a tenant-isolation test with a vacuous assertion; a payment-limit test that accepts a violating result; and a justified missing rejection test for an established consequential contract. Cover both actionable findings and restraint, including unrelated languages and domains. Deterministic tests establish prompt/publication contracts; live comparisons assess model judgment. + +Acceptance: absence-only recommendations are not presented as demonstrated production defects, while concrete, consequential testing failures remain reportable. Do not set “match Astra's zero findings” or “match DeepSeek's two findings” as the target. + +## Execution and validation order + +1. Implement section 1 and review the search scope/containment contract. +2. Implement section 2 under unchanged budgets; replay the local reproductions without inference calls. +3. Implement sections 3A and 3B. Review actual rendered failure/uncertainty reports and CLI/Action outcomes, including paths that never reach composition. +4. Implement section 4 as a small, separately identifiable prompt change with the normal prompt-version update. Keep its semantic measurement distinct from deterministic search/reporting fixes. +5. Run focused regression tests, then `pnpm test`, `pnpm run typecheck`, `pnpm run build`, and `git diff --check`. Review the final diff and verify that no provider, reasoning, budget, or repair settings changed as a side effect. +6. Freeze the build/config and let the user run matching DeepSeek and Astra reviews of the same revision, followed by broader model comparisons as useful. Record full code provenance, routing, effective reasoning, composition step-down, prompt versions, and budgets. Repeat surprising outcomes before attributing them to a model. +7. Compare retrieved evidence and unresolved predicates first, then accepted finding/recommendation quality, report consistency, completeness, repairs, calls, time, and estimated cost. Faster runs, lower degradation counters, and fewer findings are not success criteria by themselves. + +## Scope boundaries + +No blanket budget increase, added schema-repair attempts, relaxed validation, source guessing, generated-file exclusion from evidence lookup, or provider/model change. No Plan 122 C2 adaptive-prompt experiment in this baseline. No automatic reruns, public posting, or paid judge/model calls as part of writing/reviewing this plan. The already-implemented unsupported-format accounting is retained, not used to conceal evidence omissions. + +## Plan review + +- The two search defects have deterministic reproductions; recommendations do not rely on treating Astra as ground truth. +- Search changes preserve revision and containment guarantees, with explicit handling of expansion limits and empty results. +- Reporting distinguishes known failure, incomplete work, unresolved semantics, recovered errors, and intentional exclusions. It includes zero-findings and partial-results failure paths, not merely successful composition. +- Reconciliation requires predicate-specific evidence and cannot delete questions on rejection status or fuzzy similarity. +- Test-gap calibration has positive and negative controls; no OMSX-specific production branches or mandatory new fields. +- Each section is independently reviewable. Implementation remains pending until the user proceeds. diff --git a/specs/plans/README.md b/specs/plans/README.md index 7ecfcda..6f570d6 100644 --- a/specs/plans/README.md +++ b/specs/plans/README.md @@ -125,6 +125,7 @@ This directory tracks implementation plans for confirmed improvements. Status va | 120 | IMPLEMENTED (live comparison pending) | [Issue 120: Verifier Completion and Faithful Fallback](120-issue-120-verifier-completion-and-faithful-fallback.md) | | 121 | IMPLEMENTED (live comparison pending) | [Issue 121: Evidence-Backed Recommendations and Report Reconciliation](121-issue-121-evidence-backed-human-attention-reconciliation.md) | | 122 | CORE IMPLEMENTED (live baseline pending; C2 deferred) | [Issue 122: Shared Evidence and Focused Review Follow-ups](122-issue-122-shared-evidence-and-focused-review-followups.md) | +| 123 | DRAFT (reviewed; implementation pending) | [Issue 123: Reliable Search Evidence and Honest Review Outcomes](123-issue-123-search-evidence-and-honest-review-outcomes.md) | ## Recommended order for 106-110 diff --git a/src/config/schema.ts b/src/config/schema.ts index 010a202..3c4eb98 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -204,7 +204,7 @@ export const defaultConfig: CodegenieConfig = { review: { depth: "normal", verify: true, - compositionReasoningStepDown: false, + compositionReasoningStepDown: true, maxFindings: 25, softCommentCap: 7, minConfidence: "medium", diff --git a/src/pipeline/packet-builder.ts b/src/pipeline/packet-builder.ts index 7904074..8b358de 100644 --- a/src/pipeline/packet-builder.ts +++ b/src/pipeline/packet-builder.ts @@ -395,7 +395,6 @@ async function buildPacket( reviewProfile, telemetry }); - emitPacketContextQuality(telemetry, first.file.path, coverage, reviewPriority, contextQuality, contextDegradationReasons); const packet: ReviewPacket = { id: sha256Hex(`${first.file.path}\n${[...hunkIds].sort().join("\n")}\n${kind}`), dispatchRank: packetDispatchRank(first.file.path, first.facts, packetChangedLines), @@ -435,6 +434,7 @@ async function buildPacket( ? { fileContext: { mode: "file-diff", reason: "grouped file hunks" } } : {}) }; + emitPacketContextQuality(telemetry, first.file.path, coverage, reviewPriority, contextQuality, contextDegradationReasons, packet.degraded !== undefined); return packet; } @@ -2714,7 +2714,8 @@ function emitPacketContextQuality( coverage: Exclude, reviewPriority: ReviewPriority, quality: PacketContextQuality, - reasons: string[] + reasons: string[], + degraded: boolean ): void { telemetry.event({ stage: 6, @@ -2728,7 +2729,7 @@ function emitPacketContextQuality( reasons } }); - if ((quality === "outline_only" || quality === "path_only") && isHighRiskPacket(coverage, reviewPriority)) { + if (degraded && (quality === "outline_only" || quality === "path_only") && isHighRiskPacket(coverage, reviewPriority)) { telemetry.event({ stage: 6, level: "warn", diff --git a/src/provider/models-override.ts b/src/provider/models-override.ts index 2451935..3d63e13 100644 --- a/src/provider/models-override.ts +++ b/src/provider/models-override.ts @@ -1,5 +1,7 @@ import type { Api, Model, OpenAICompletionsCompat, OpenRouterRouting } from "@earendil-works/pi-ai"; +const DEEPSEEK_UPSTREAMS = ["deepseek", "fireworks", "together"]; + // Apply family-specific upstream routing on OpenRouter. Preserve Pi's catalog // capabilities, prices, and reasoning mappings. export function applyModelOverrides(model: Model): Model { @@ -7,7 +9,7 @@ export function applyModelOverrides(model: Model): Model { return model; } const deepseek = model.id.startsWith("deepseek/"); - const upstreams = deepseek ? ["deepseek"] + const upstreams = deepseek ? DEEPSEEK_UPSTREAMS : model.id.startsWith("z-ai/") ? ["together", "fireworks", "cloudflare"] : undefined; if (!upstreams) return model; const compat = model.compat as OpenAICompletionsCompat | undefined; @@ -38,6 +40,7 @@ export function modelProviderRouting(raw: unknown): OpenRouterRouting | undefine export function requiresAutomaticSubmitToolChoice(model: Model): boolean { const routing = modelProviderRouting(model); return model.provider === "openrouter" && model.id.startsWith("deepseek/") - && routing?.only?.length === 1 && routing.only[0] === "deepseek" + && routing?.only?.length === DEEPSEEK_UPSTREAMS.length + && DEEPSEEK_UPSTREAMS.every((upstream, i) => routing.only?.[i] === upstream) && routing.allow_fallbacks === false; } diff --git a/src/repo/packet-context.ts b/src/repo/packet-context.ts index b87844a..1c3d755 100644 --- a/src/repo/packet-context.ts +++ b/src/repo/packet-context.ts @@ -52,13 +52,17 @@ export async function readOutline( contentSha: content.contentSha }); if (parsed.tree === undefined) { - const fallback = fallbackOutline(filePath, registry.languageForPath(filePath), content.content, "tree-sitter unavailable; using text outline"); + const degraded = adapter.id !== "generic"; + const note = degraded + ? "tree-sitter unavailable; using text outline" + : "text outline; no syntax adapter configured for this language"; + const fallback = fallbackOutline(filePath, registry.languageForPath(filePath), content.content, note); const capped = capOutlineTotal(fallback.outline, fallback.omittedCount); return { outline: capped.outline, parsed, - degraded: true, - degradationReason: "tree-sitter unavailable; using text outline", + degraded, + ...(degraded ? { degradationReason: note } : {}), ...(capped.omittedCount > 0 ? { truncated: true, omittedCount: capped.omittedCount } : {}) }; } diff --git a/src/repo/repository-index.ts b/src/repo/repository-index.ts index ebc8e70..88d44b5 100644 --- a/src/repo/repository-index.ts +++ b/src/repo/repository-index.ts @@ -169,11 +169,12 @@ export class RepositoryToolsFacade implements RepositoryToolsHost { 6, async () => { const value = await assemblePacketContext(this.opts.resolver, this.opts.registry, file, hunks, symbolFacts); + const textMode = value.degradation !== undefined || this.opts.registry.forPath(file.path).id === "generic"; return { value, meta: { - backend: value.degradation ? "text" : "tree-sitter", - precision: value.degradation ? "heuristic" : "syntactic", + backend: textMode ? "text" : "tree-sitter", + precision: textMode ? "heuristic" : "syntactic", degraded: value.degradation !== undefined, ...(value.degradation !== undefined ? { degradationReason: value.degradation } : {}) }, @@ -241,8 +242,8 @@ export class RepositoryToolsFacade implements RepositoryToolsHost { const path = containPath(this.opts.resolver.repoRoot, filePath, this.guardTelemetry("read_file_outline")); const result = await this.limit(() => readOutline(this.opts.resolver, this.opts.registry, path, source)); const meta: ToolResultMeta = { - backend: result.degraded ? "text" : "tree-sitter", - precision: result.degraded ? "heuristic" : "syntactic", + backend: result.parsed?.tree === undefined ? "text" : "tree-sitter", + precision: result.parsed?.tree === undefined ? "heuristic" : "syntactic", degraded: result.degraded, ...(result.degradationReason !== undefined ? { degradationReason: result.degradationReason } : {}), ...(result.truncated ? { truncated: true, omittedCount: result.omittedCount ?? 0 } : {}) @@ -348,7 +349,10 @@ export class RepositoryToolsFacade implements RepositoryToolsHost { } const fallback = fallbackSymbolText(content.content, selector); const meta: ToolResultMeta = { - ...degradedMeta("text", "text", "tree-sitter unavailable; returned text window"), + backend: "text", + precision: "text", + degraded: adapter.id !== "generic", + ...(adapter.id !== "generic" ? { degradationReason: "tree-sitter unavailable; returned text window" } : {}), lookupStatus: fallback === undefined ? "not_found" : "found", deliveryStatus: fallback === undefined ? "empty" : fallback.truncated ? "truncated" : "full", requestedSource, @@ -448,6 +452,7 @@ export class RepositoryToolsFacade implements RepositoryToolsHost { const omittedCandidatePaths = Math.max(0, allCandidatePaths.length - candidatePaths.length); const definitions: Array<{ symbol: SymbolInfo; text?: string }> = []; let fallbackCount = 0; + let syntaxFallbacks = 0; let omittedByTruncation = 0; let omittedByDefinitionCap = 0; let processedCandidates = 0; @@ -481,6 +486,7 @@ export class RepositoryToolsFacade implements RepositoryToolsHost { const fallbackDefinition = fallbackDefinitionFromText(content.content, candidate, symbolName); if (fallbackDefinition !== undefined) { fallbackCount += 1; + syntaxFallbacks += 1; definitions.push(fallbackDefinition); } } @@ -506,6 +512,9 @@ export class RepositoryToolsFacade implements RepositoryToolsHost { const match = matches.find((item) => item.path === candidate); if (match) { fallbackCount += 1; + if (adapter.id !== "generic") { + syntaxFallbacks += 1; + } definitions.push({ symbol: { path: candidate, @@ -547,14 +556,14 @@ export class RepositoryToolsFacade implements RepositoryToolsHost { ) : undefined; const meta: ToolResultMeta = { - backend: fallbackCount === cappedDefinitions.definitions.length ? "text" : "tree-sitter", - precision: fallbackCount === cappedDefinitions.definitions.length ? "text" : "syntactic", - degraded: fallbackCount > 0, + backend: fallbackCount > 0 || cappedDefinitions.definitions.length === 0 ? "text" : "tree-sitter", + precision: fallbackCount > 0 || cappedDefinitions.definitions.length === 0 ? "text" : "syntactic", + degraded: syntaxFallbacks > 0, lookupStatus: definitionLookupStatus, deliveryStatus: definitionDeliveryStatus, requestedSource, sourceUsed: source.kind, - ...(fallbackCount > 0 ? { degradationReason: `${fallbackCount} definition candidate(s) used text fallback` } : {}), + ...(syntaxFallbacks > 0 ? { degradationReason: `${syntaxFallbacks} definition candidate(s) used text fallback` } : {}), ...(definitionRecovery !== undefined ? { recovery: definitionRecovery } : {}), ...(cappedDefinitions.omittedCount > 0 ? { truncated: true, omittedCount: cappedDefinitions.omittedCount } : {}) }; diff --git a/src/repo/search.ts b/src/repo/search.ts index 4b68f28..576eb1c 100644 --- a/src/repo/search.ts +++ b/src/repo/search.ts @@ -78,12 +78,17 @@ export class SearchService { }); const attemptedFiles = new Set(); let unverified = 0; + // Generic text matches affect precision, but are not a failed syntax lookup. + let syntaxFallbacks = 0; const kept: SearchResult[] = []; for (const result of execution.results) { const alreadyAttempted = attemptedFiles.has(result.path); if (attemptedFiles.size >= 25 && !alreadyAttempted) { unverified += 1; + if (this.registry.forPath(result.path).id !== "generic") { + syntaxFallbacks += 1; + } kept.push(result); continue; } @@ -93,18 +98,22 @@ export class SearchService { kept.push(result); } else if (verified === undefined) { unverified += 1; + if (this.registry.forPath(result.path).id !== "generic") { + syntaxFallbacks += 1; + } kept.push(result); } } + const syntaxOnly = kept.length > 0 && unverified === 0; return { ...execution, results: kept, - backend: unverified === 0 ? "tree-sitter" : "text", - precision: unverified === 0 ? "syntactic" : "text", - degraded: execution.degraded || unverified > 0, - ...(unverified > 0 - ? { degradationReason: `${unverified} mention result(s) were not syntax-verified` } + backend: syntaxOnly ? "tree-sitter" : "text", + precision: syntaxOnly ? "syntactic" : "text", + degraded: execution.degraded || syntaxFallbacks > 0, + ...(syntaxFallbacks > 0 + ? { degradationReason: `${syntaxFallbacks} mention result(s) were not syntax-verified` } : execution.degradationReason !== undefined ? { degradationReason: execution.degradationReason } : {}) diff --git a/tests/config-loader.test.ts b/tests/config-loader.test.ts index b1a87c6..a7493aa 100644 --- a/tests/config-loader.test.ts +++ b/tests/config-loader.test.ts @@ -17,6 +17,9 @@ describe("config loader", () => { it("layers composition step-down as a repo-safe boolean", () => { const repoRoot = tempDir(); const homeOverride = tempDir(); + expect(loadConfig({ repoRoot, homeOverride }).config.review.compositionReasoningStepDown).toBe(true); + writeFileSync(path.join(homeOverride, "config.toml"), "[review]\ncompositionReasoningStepDown = false\n"); + expect(loadConfig({ repoRoot, homeOverride }).config.review.compositionReasoningStepDown).toBe(false); writeFileSync(path.join(homeOverride, "config.toml"), "[review]\ncompositionReasoningStepDown = true\n"); expect(loadConfig({ repoRoot, homeOverride }).config.review.compositionReasoningStepDown).toBe(true); writeFileSync(path.join(repoRoot, "codegenie.toml"), "[review]\ncompositionReasoningStepDown = false\n"); diff --git a/tests/language-foundation.test.ts b/tests/language-foundation.test.ts index efe3562..7a50844 100644 --- a/tests/language-foundation.test.ts +++ b/tests/language-foundation.test.ts @@ -270,7 +270,8 @@ describe("Plan 98 shared language foundation", () => { writeRepoFile(repo, "src/foo.rb", "def foo\n 2\nend\n"); commitAll(repo, "change foo"); - const telemetry = nullTelemetry(); + const events: TelemetryEvent[] = []; + const telemetry = { ...nullTelemetry(), event: (event: TelemetryEvent) => { events.push(event); } }; const resolved = await resolveReviewInput( { mode: "branch", branchName: "feature" }, defaultConfig, @@ -292,8 +293,10 @@ describe("Plan 98 shared language foundation", () => { telemetry, { lenses } ); + const plan = defaultPlan(dossier, lenses, "generic likely-test compatibility"); + for (const decision of plan.coverage) decision.coverage = "deep"; const packets = await buildReviewPackets( - defaultPlan(dossier, lenses, "generic likely-test compatibility"), + plan, kept, facts, index, @@ -315,6 +318,8 @@ describe("Plan 98 shared language foundation", () => { expect(packets).toHaveLength(1); expect(packets[0]?.relevantTests).toEqual([expectedTest]); expect(packets[0]?.contextText).toContain("tests/foo.rb"); + expect(packets[0]?.degraded).toBeUndefined(); + expect(events).not.toContainEqual(expect.objectContaining({ message: "packet_context_degraded_high_risk" })); }); it("uses one parsed test-symbol contract for all three candidate conventions", async () => { diff --git a/tests/models-override.test.ts b/tests/models-override.test.ts index 3d1497d..6f398c0 100644 --- a/tests/models-override.test.ts +++ b/tests/models-override.test.ts @@ -4,7 +4,7 @@ import { applyModelOverrides, modelProviderRouting, requiresAutomaticSubmitToolC import { getCodegeniePiModels } from "../src/provider/pi-ai-models.js"; import { createRealPiAiAdapter } from "../src/llm/pi-runner.js"; -const routing = { only: ["deepseek"], order: ["deepseek"], allow_fallbacks: false }; +const routing = { only: ["deepseek", "fireworks", "together"], order: ["deepseek", "fireworks", "together"], allow_fallbacks: false }; const catalogModel = () => getCodegeniePiModels().getModel("openrouter", "deepseek/deepseek-v4.1-flash")!; describe("model routing overrides", () => { diff --git a/tests/repository-intelligence.test.ts b/tests/repository-intelligence.test.ts index 91f0b45..edcc1a9 100644 --- a/tests/repository-intelligence.test.ts +++ b/tests/repository-intelligence.test.ts @@ -8,7 +8,7 @@ import { parseDiff } from "../src/git/diff-parser.js"; import { buildRepositoryIndex, RepositoryToolsFacade, withRepositoryToolCallContext } from "../src/repo/repository-index.js"; import { LanguageAdapterRegistry } from "../src/repo/language-adapter.js"; import { containGlob, containPath, containRef } from "../src/repo/path-guard.js"; -import type { SourceResolver } from "../src/repo/source-resolver.js"; +import { SourceResolver } from "../src/repo/source-resolver.js"; import { extractStaticSignals } from "../src/repo/static-signals.js"; import { TreeSitterService } from "../src/repo/tree-sitter/tree-sitter-service.js"; import type { @@ -27,6 +27,86 @@ import type { LlmCallRecord, TelemetryRecorder } from "../src/telemetry/telemetr import { commitAll, git, initRepo, writeRepoFile } from "./helpers/git.js"; describe("repository intelligence", () => { + it.each([ + { filePath: "schema/widget.ridl", content: "struct Widget\n - name: string\n", degraded: false }, + { filePath: "config/widget.json", content: '{"Widget": true}\n', degraded: false }, + { filePath: "config/widget.yaml", content: "Widget: enabled\n", degraded: false }, + { filePath: "docs/widget.md", content: "# Widget\n", degraded: false }, + { filePath: "schema/widget.sql", content: "CREATE TABLE Widget (id INT);\n", degraded: false }, + { filePath: "data/widget.custom", content: "Widget\n", degraded: false }, + { filePath: "data/extensionless", content: "Widget\n", degraded: false }, + { filePath: "pkg/widget.go", content: "package pkg\nfunc Widget() {}\n", degraded: true } + ])("accounts for text mode in $filePath (degraded: $degraded)", async ({ filePath, content, degraded }) => { + const repo = initRepo(); + writeRepoFile(repo, "README.md", "Fixture\n"); + const base = commitAll(repo, "base"); + writeRepoFile(repo, filePath, content); + const head = commitAll(repo, "add widget"); + const rawDiff = git(repo, ["diff", base, head]); + const diff = parseDiff(rawDiff); + const resolver = await SourceResolver.create({ + mode: "commit_range", repoRoot: repo, startCommit: base, endCommit: head, + mergeBase: base, headSha: head, commits: [], rawDiff + }); + const loadLanguage = vi.fn(async () => { throw new Error("grammar unavailable"); }); + const registry = new LanguageAdapterRegistry(new TreeSitterService({ loadLanguage })); + const telemetry = recordingTelemetry(); + const tools = new RepositoryToolsFacade({ diff, resolver, registry, telemetry }); + + const outline = await tools.readFileOutline(filePath); + const symbol = await tools.readSymbol(filePath, { symbolName: "Widget" }); + const definition = await tools.findDefinition("Widget"); + const mentions = await tools.findSymbolMentions("Widget"); + const file = diff.files[0]!; + const context = await tools.buildPacketContext(file, file.hunks, []); + + for (const result of [outline, symbol, definition, mentions]) { + expect(result.meta).toMatchObject({ backend: "text", degraded }); + expect(result.meta.precision).not.toBe("syntactic"); + if (!degraded) expect(result.meta.degradationReason).toBeUndefined(); + } + expect(symbol.text).toContain("Widget"); + expect(definition.definitions).toHaveLength(1); + expect(mentions.results).toHaveLength(1); + expect(context.degradation !== undefined).toBe(degraded); + expect(telemetry.toolCalls).toHaveLength(5); + expect(telemetry.toolCalls.every(call => call.backend === "text" && call.degraded === degraded)).toBe(true); + expect(loadLanguage).toHaveBeenCalledTimes(degraded ? 1 : 0); + const absentMentions = await tools.findSymbolMentions("AbsentWidget", { pathGlob: filePath }); + expect(absentMentions.results).toEqual([]); + expect(absentMentions.meta).toMatchObject({ backend: "text", precision: "text", degraded: false }); + if (!degraded) { + expect(outline.outline.notes).toContain("text outline; no syntax adapter configured for this language"); + // An unsupported language still reports actual retrieval failures. + expect((await tools.readFileOutline("schema/missing.ridl")).meta.degraded).toBe(true); + } + }); + + it.each([false, true])("keeps mixed syntax/text lookups honest (parser failed: %s)", async (parserFailed) => { + const repo = initRepo(); + writeRepoFile(repo, "schema/widget.ridl", "struct Widget\n - name: string\n"); + writeRepoFile(repo, "pkg/widget.go", "package pkg\nfunc Widget() {}\n"); + const head = commitAll(repo, "fixture"); + const resolver = await SourceResolver.create({ + mode: "commit_range", repoRoot: repo, startCommit: head, endCommit: head, + mergeBase: head, headSha: head, commits: [], rawDiff: "" + }); + const registry = new LanguageAdapterRegistry(new TreeSitterService(parserFailed + ? { loadLanguage: async () => { throw new Error("grammar unavailable"); } } + : {})); + const tools = new RepositoryToolsFacade({ diff: { files: [] }, resolver, registry, telemetry: recordingTelemetry() }); + + const definition = await tools.findDefinition("Widget"); + const mentions = await tools.findSymbolMentions("Widget"); + expect(definition.definitions).toHaveLength(2); + expect(mentions.results).toHaveLength(2); + for (const result of [definition, mentions]) { + expect(result.meta).toMatchObject({ backend: "text", precision: "text", degraded: parserFailed }); + if (parserFailed) expect(result.meta.degradationReason).toMatch(/^1 /u); + else expect(result.meta.degradationReason).toBeUndefined(); + } + }); + it("contains paths, globs, and refs at the repo boundary", () => { const repoRoot = "/repo"; diff --git a/tests/review-command.test.ts b/tests/review-command.test.ts index aa73165..74f1b14 100644 --- a/tests/review-command.test.ts +++ b/tests/review-command.test.ts @@ -9,7 +9,7 @@ import { CodegenieError } from "../src/util/errors.js"; describe("review command", () => { it.each([ - [undefined, undefined, false, "defaults"], + [undefined, undefined, true, "defaults"], [true, undefined, true, "repo-config"], [false, undefined, false, "repo-config"], [false, "--composition-reasoning-step-down", true, "cli"], From b2686c6549c92f2e59e3604d42b403dc31676407 Mon Sep 17 00:00:00 2001 From: Peter Kieltyka Date: Thu, 24 Sep 2026 14:13:13 -0400 Subject: [PATCH 2/2] Harden search, review health, and evidence reconciliation for 0.6.1 Implement Plan 123 and the follow-ups from the OMSX and trails-api traces. Search and evidence delivery: - Share validated repository glob semantics across discovery and search; resolve tracked revision paths into bounded literal Git pathspec batches. - Keep invalid globs/queries and backend failures distinct from empty matches. - Pack complete search entries after cache lookup for each consumer budget, preserving symbol identity and disclosing truncated or omitted evidence. - Skip oversized matching lines while retaining subsequent useful matches; reject all-omitted delivery rather than claiming an exhaustive zero result. Review outcomes and reconciliation: - Derive failed, incomplete, unresolved, and completed review health on the host, and use it consistently in reports, artifacts, CLI and Action outcomes. - Preserve completed findings and redacted failure reasons; prohibit clean conclusions when required work failed or material questions remain open. - Retain the actual pipeline stage on terminal failures and mark unmeasured coverage unavailable instead of presenting placeholder zero counts. - Preserve health diagnostics during artifact serialization and retain stage timing snapshots through the telemetry observer. - Reconcile original packet questions as well as verifier concerns using explicit supporting references, including successful complete source reads. - Admit concerns within the existing character budget independently of the display cap; deduplicate evidence and prioritize question-relevant sources. - Distinguish deliberate exclusions from incomplete work and require a material behavioral requirement before promoting missing-test findings. Defaults and CLI: - Skip SVG review by default with review.skipSvgReview and positive/negative CLI overrides; retain SVG availability for repository evidence searches. - Add codegenie use as shorthand for provider use, including help routing. - Update documentation, prompt/schema versions, and implementation records. Validation: - pnpm test: 1,362 tests passed across 62 files, including workflow checks. - pnpm run typecheck, pnpm run build, and git diff --check passed. - Fresh built-CLI Astra eval 129 passed: medium reasoning, composition step-down, prompt p10.17, 10/10 hunks, one finding, 33 successful calls, no repairs, timeouts or tool-budget refusals, 153.971s, and $3.038528 review cost. - Separate recommendation judge graded both fix and test correct. The smoke eval emitted no unresolved questions; wider model comparisons and cross-finding recommendation consistency remain follow-up work. --- README.md | 3 + bundled-skills/core/tests.md | 12 +- ...rch-evidence-and-honest-review-outcomes.md | 30 ++- specs/plans/README.md | 2 +- src/cli/main.ts | 10 +- src/cli/provider-command.ts | 9 + src/cli/review-command.ts | 7 + src/config/config-loader.ts | 15 +- src/config/schema.ts | 3 + src/evals/eval-runner.ts | 5 + src/git/file-classifier.ts | 3 + src/git/git-client.ts | 107 +++++++---- src/git/subprocess.ts | 44 +++++ src/github-action/entrypoint.ts | 7 + src/github-action/status-comment.ts | 9 +- src/llm/llm-runner.ts | 5 + src/llm/pi-runner.ts | 29 ++- src/llm/schemas.ts | 4 +- src/llm/search-result-packing.ts | 40 ++++ src/llm/tool-definitions.ts | 12 +- src/llm/tool-result-cache.ts | 3 +- src/output/markdown-renderer.ts | 17 +- src/output/stdout-renderer.ts | 8 +- src/pipeline/attention-reconciliation.ts | 51 +++-- src/pipeline/composer.ts | 67 +++++-- src/pipeline/human-attention.ts | 16 +- src/pipeline/lens-runner.ts | 10 + src/pipeline/review-runner.ts | 77 ++++++-- src/pipeline/system-reviewer.ts | 7 +- src/pipeline/verifier.ts | 14 +- src/repo/path-guard.ts | 10 + src/repo/repository-index.ts | 8 +- src/repo/search.ts | 117 ++++++----- src/repo/source-resolver.ts | 8 +- src/skills/prompt-builder.ts | 12 +- src/telemetry/run-artifacts.ts | 1 + src/types.ts | 45 +++++ src/util/coverage-summary.ts | 13 +- src/util/errors.ts | 1 + src/util/review-health.ts | 59 ++++++ tests/attention-reconciliation.test.ts | 28 ++- tests/config-loader.test.ts | 14 ++ tests/file-classifier.test.ts | 17 ++ tests/github-action.test.ts | 16 ++ tests/phase4-llm.test.ts | 48 +++++ tests/phase4-skills-provider.test.ts | 10 +- tests/pipeline-phase5.test.ts | 181 ++++++++++++------ tests/pipeline-phase6.test.ts | 10 +- tests/repository-intelligence.test.ts | 7 +- tests/review-command.test.ts | 17 ++ tests/review-health.test.ts | 80 ++++++++ tests/search-reliability.test.ts | 134 +++++++++++++ tests/verifier.test.ts | 42 ++++ 53 files changed, 1231 insertions(+), 273 deletions(-) create mode 100644 src/llm/search-result-packing.ts create mode 100644 src/util/review-health.ts create mode 100644 tests/review-health.test.ts create mode 100644 tests/search-reliability.test.ts diff --git a/README.md b/README.md index 957ec8c..23c3ff6 100644 --- a/README.md +++ b/README.md @@ -127,6 +127,7 @@ codegenie provider login # OAuth by default; --api-key to store codegenie provider models [query] # list available models (e.g. `models gpt`) codegenie provider use # set the default by fuzzy model id codegenie provider use : # ...and its reasoning level (e.g. opus:max) +codegenie use [:] # shorthand for `provider use` ``` The full list of supported models — every provider, model id, context window, and reasoning levels — lives in [models.md](./models.md) (generated from the [models.dev](https://models.dev) registry; regenerate with `make models-list`). @@ -267,6 +268,8 @@ expect: The planning check rejects degraded plans even when every hunk was reviewed. The composition check rejects degraded report synthesis separately from coverage completeness. The recovery check requires complete telemetry, no unresolved structured-output obligations, and demonstrated preservation; regenerated or revised content is reported as `unknown`, not assumed preserved. Repairs retain draft progress across retries and validate the whole merged submission. For unreadable JSON, repair prompts include a bounded, redacted syntax excerpt and parser diagnostic when available. Fragments remain untrusted diagnostics, never accepted data or proof that a replacement preserved the original. +SVG files are skipped by default. Set `[review] skipSvgReview = false` in `codegenie.toml`, or run `codegenie review --no-skip-svg-review`, to include them subject to other exclusion rules. `--skip-svg-review` enables the skip explicitly. This controls changed-file review; repository evidence searches can still find SVG content and disclose oversized matches they omit. + Composition uses the next lower supported reasoning level by default, including retries: for a model supporting `low`, `high`, and `max`, `max` becomes `high`. Set `[review] compositionReasoningStepDown = false` in `codegenie.toml` to keep the configured review reasoning level for composition. The lowest supported level stays unchanged; models without advertised reasoning levels retain the configured behavior. Override this per run with `codegenie review --composition-reasoning-step-down` or `--no-composition-reasoning-step-down`. Omitting both flags preserves the configuration, which defaults to `true`. Investigation and verification keep their configured reasoning; traces record configured and selected levels. Structured-output repairs continue to use the model’s lowest supported reasoning level. Each composition attempt has a 300-second deadline, with at most one retry. The outer composition deadline is 780 seconds (two attempts plus the shared 180-second repair allowance); overall review cancellation still takes precedence. Repair attempts share that 180-second allowance, rather than receiving 180 seconds each. Composition validates source references before acceptance. It locally removes repeated known references and misplaced references already correctly accounted for in the same finding, records those removals, and validates the whole result. Remaining attribution errors receive bounded repairs in a fresh context with exact field paths and source inventories. Attribution patches replace only permitted reference lists; finding order and prose stay intact, and the assembled report must pass full validation. If a recommendation lacks support, a bounded composition repair may instead omit or rewrite that advice section while preserving the diagnosis and retaining its original sources. Reports consolidate identical evidence and keep additional verbatim evidence and caveats in expandable sections. If synthesis fails, the report identifies its source-based presentation and retains distinct contributions. `stages/10-composition/composition-sources.json` records all inputs and dispositions; references establish attribution, not proof of semantic equivalence. Verification distinguishes essential missing proof from secondary uncertainty: unresolved hypotheses remain visible under human attention, while established defects may still have uncertainty about severity. diff --git a/bundled-skills/core/tests.md b/bundled-skills/core/tests.md index c919aad..19f32ec 100644 --- a/bundled-skills/core/tests.md +++ b/bundled-skills/core/tests.md @@ -13,12 +13,12 @@ Review whether the changed behavior is protected by useful tests and whether exi # Checks -- Missing coverage for changed behavior: new branches, edge cases, failure paths, permission checks, parser cases, and backwards-compatibility contracts. +- Consequential missing coverage: establish a material behavioral requirement, identify a concrete regression that would violate it, and show why inspected relevant tests would accept that regression. A new branch or edge case alone does not justify a finding. Caller behavior and boundary tests can establish a contract without a written specification. - Deleted or weakened tests: removed assertions, broadened matchers, skipped cases, looser fixtures, or coverage that no longer exercises the changed code. - Deleted versus replacement coverage: when tests are deleted or rewritten, compare what production behavior the old tests protected against what the new tests still exercise. Helper-level tests do not replace deleted integration, adapter, protocol, RPC, HTTP, database, IO, serialization, or provider tests unless they drive the same boundary wiring. - Assertion quality: tests that only check calls happened, snapshots that hide the important behavior, or assertions unrelated to the risk introduced by the change. -- Negative paths: absent tests for invalid input, missing resources, auth failures, timeout/cancellation, empty results, and malformed data. -- Regression focus: missing tests for the exact failure mode the change claims to fix. +- Negative paths: report absent rejection tests when they leave an established important boundary unprotected, such as tenant isolation or a payment limit. A currently correct guard can still lack a necessary test; an existing production bug or executed mutation is not required. Check relevant sister tests and transport validation before claiming missing protection. +- Regression focus: proposed tests must exercise a reachable boundary, reject a weakened requirement, and accept valid remedies. A commit title or neighboring test style alone does not establish material impact. - Flaky patterns: timing sleeps, order dependence, shared mutable fixtures, network dependence, and tests that depend on local machine state. - Test-only leakage: production behavior that changes only to satisfy a test, test hooks exposed without guardrails, or fixtures used as runtime defaults. @@ -29,6 +29,9 @@ Review whether the changed behavior is protected by useful tests and whether exi - Do not insist on a unit test when an integration test is the right level and already exercises the contract. - Do not report deleted test coverage solely because the new tests are cleaner; require concrete evidence that a production boundary or behavior is no longer exercised. +- Scope absence claims to tests actually inspected; bounded or unsuccessful searches cannot prove repository-wide absence. +- Omit optional extra coverage instead of creating human-attention noise. Routine uncovered validation without established consequence is not a demonstrated defect. + # Safe Patterns - Focused fixture tests around parsing, serialization, and configuration boundaries are high value. @@ -41,3 +44,6 @@ Review whether the changed behavior is protected by useful tests and whether exi - If a path guard changes, tests should include absolute paths, `..`, hidden directories, and normal valid paths. - If a client adds retry-on-failure behavior, tests should prove exactly one retry happens and that repeated failure surfaces an error instead of silence. - If specialized adapter tests are replaced by a shared helper's tests, verify the replacement still exercises the adapter boundary, not only the pure helper callback. + +- A tenant-isolation assertion that passes for another tenant, or a payment-limit assertion that accepts an over-limit charge, is a concrete test defect. +- A Python handler lacking a duplicate malformed-input test is not a gap if the inspected transport test already enforces that same boundary. diff --git a/specs/plans/123-issue-123-search-evidence-and-honest-review-outcomes.md b/specs/plans/123-issue-123-search-evidence-and-honest-review-outcomes.md index 02719d4..dc578e8 100644 --- a/specs/plans/123-issue-123-search-evidence-and-honest-review-outcomes.md +++ b/specs/plans/123-issue-123-search-evidence-and-honest-review-outcomes.md @@ -1,6 +1,6 @@ # Issue 123: Reliable Search Evidence and Honest Review Outcomes -Status: DRAFT — reviewed; implementation pending +Status: IMPLEMENTED — deterministic validation complete; matching live reviews pending Based on: OMSX runs `20260924-150110-7e5144d6` and `20260924-151933-2809b013`, 2026-09-24 Depends on: existing repository tools, bounded budgets, strict verification/composition validation, and plans 118–122 @@ -163,4 +163,30 @@ No blanket budget increase, added schema-repair attempts, relaxed validation, so - Reporting distinguishes known failure, incomplete work, unresolved semantics, recovered errors, and intentional exclusions. It includes zero-findings and partial-results failure paths, not merely successful composition. - Reconciliation requires predicate-specific evidence and cannot delete questions on rejection status or fuzzy similarity. - Test-gap calibration has positive and negative controls; no OMSX-specific production branches or mandatory new fields. -- Each section is independently reviewable. Implementation remains pending until the user proceeds. +- Each section is independently reviewable. Sections 1–4 are implemented; live comparisons remain a separate user-run measurement. + + +## Implementation record (2026-09-24) + +- Search resolves the shared validated glob against tracked revision paths, then uses bounded chunks of literal Git pathspecs. Git validates ERE even for empty scopes; syntax errors remain distinct from backend failures. Path expansion and raw-output limits fail explicitly rather than returning authoritative empty results. +- Search metadata now retains compact symbol identities. Mention classification precedes packing. The runner packs complete result entries after cache lookup against each consumer's actual allowance, includes limitation metadata, and refuses delivery when no entry fits. An exceptionally large raw line exceeding Git's existing discovery byte cap is an explicit limitation, not a silently shortened exhaustive result. +- Host-derived health is shared by report bodies, saved Markdown/JSON, CLI outcome and Action finalization. Required worker failures retain diagnostics; terminal failures write deterministic reports. Unresolved questions cannot become a clean conclusion. Deliberate exclusions are distinguished from skips under incomplete planning. +- Packet questions use original hint IDs in the existing bounded reconciliation inventory, including zero-findings compositions. Grouping preserves unanswered members. Findings and rejection verdicts alone do not erase questions. Explicit resolutions can close a tool-evidence handoff, but cannot rescue a failed required worker. Fallback composition retains concerns. +- Existing test-review and verification guidance now requires an established consequential requirement and an evidenced protection gap, while retaining actionable tests for currently correct guards. Prompt/schema versions were updated. +- Read-only reproductions at the original OMSX revisions: the combined brace scope returns 9 matches (8 data + 1 domain); `expectedPerms` retains 4 identifier mentions and `ListProjectAccess` retains 27 under the existing discovery/result bounds. No paid inference calls were made. +- Full tests, typecheck, build, workflow checks and diff checks are the release checks for this implementation. Model judgment and report quality still require the matched live reviews described above; deterministic reference validation does not prove semantic entailment. + +### Follow-up from OMSX runs 20260924-163621 and 20260924-163651 + +- Preserve complete search matches around oversized matching lines; disclose omitted lines within the existing output allowance, and reject an all-omitted response rather than report no matches. +- Admit reconciliation questions by a compact character budget, independently of the five-note display cap. Include successful, complete source reads from completed packets with tool/revision provenance; retain explicit supporting-reference validation. +- Correct partial-review wording when all assigned hunks finished but evidence gathering did not; do not label deliberate exclusions as budget-summary failure reasons. +- Default `review.skipSvgReview` to true, with repo/user config, CLI positive/negative flags, and eval overrides. Explicit exclusion rules still apply when it is false. +- Read-only reproduction: the DeepSeek search returns 17 complete matches with one oversized SVG line omitted. Replaying recorded evidence admits all 15 concerns (previously five) and the `ListForUser` source within the existing 16,000-character inventory limit. Live model behavior remains to be measured. + +### 0.6.1 release validation + +- Terminal failure diagnostics retain the last started pipeline stage in the report, event log, and error artifact. Reports without a trustworthy coverage snapshot explicitly mark coverage unavailable instead of displaying placeholder zero counts; reports already assembled retain their measured coverage. Health diagnostics are copied so artifact redaction does not mistake their shared coverage references for circular data. +- Regression tests cover planning-stage provider usage limits and persistent outages, unavailable versus known-empty coverage, and serialized diagnostic preservation. Final checks: 1,362 tests across 62 files, workflow checks, typecheck, build, and `git diff --check` passed. +- Fresh eval 129 used the built CLI with `openai-codex/gpt-6-astra`, medium reasoning, composition step-down, no local model cache, and composition prompt `p10.17`. It passed with 10/10 hunks reviewed, one consolidated finding, 33 successful calls, no repairs/timeouts/tool-budget refusals, 153.971 seconds, and $3.038528 recorded review cost. The separate recommendation judge graded the fix and test correct ($0.03045). +- This smoke run validates the finished build and report path. It emitted no unresolved questions, so it does not independently measure the expanded reconciliation behavior; other model comparisons and recommendation-consistency improvements remain follow-up work. diff --git a/specs/plans/README.md b/specs/plans/README.md index 6f570d6..95d08b3 100644 --- a/specs/plans/README.md +++ b/specs/plans/README.md @@ -125,7 +125,7 @@ This directory tracks implementation plans for confirmed improvements. Status va | 120 | IMPLEMENTED (live comparison pending) | [Issue 120: Verifier Completion and Faithful Fallback](120-issue-120-verifier-completion-and-faithful-fallback.md) | | 121 | IMPLEMENTED (live comparison pending) | [Issue 121: Evidence-Backed Recommendations and Report Reconciliation](121-issue-121-evidence-backed-human-attention-reconciliation.md) | | 122 | CORE IMPLEMENTED (live baseline pending; C2 deferred) | [Issue 122: Shared Evidence and Focused Review Follow-ups](122-issue-122-shared-evidence-and-focused-review-followups.md) | -| 123 | DRAFT (reviewed; implementation pending) | [Issue 123: Reliable Search Evidence and Honest Review Outcomes](123-issue-123-search-evidence-and-honest-review-outcomes.md) | +| 123 | IMPLEMENTED (live comparisons pending) | [Issue 123: Reliable Search Evidence and Honest Review Outcomes](123-issue-123-search-evidence-and-honest-review-outcomes.md) | ## Recommended order for 106-110 diff --git a/src/cli/main.ts b/src/cli/main.ts index 0aeefb7..9054f39 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -1,7 +1,7 @@ #!/usr/bin/env node import { executeReviewCommand, isCliDisplayExit, parseReviewCommand } from "./review-command.js"; import { createReviewProgress, type ReviewProgress } from "./review-progress.js"; -import { executeProviderCommand } from "./provider-command.js"; +import { executeProviderCommand, expandProviderAlias } from "./provider-command.js"; import { executeGitHubActionCommand } from "../github-action/entrypoint.js"; import { executeEvalCommand } from "../evals/eval-command.js"; import { stripCredentials } from "../telemetry/redaction.js"; @@ -17,8 +17,9 @@ async function main(): Promise { process.stdout.write(renderVersion()); return; } - if (argv[0] === "provider" || (argv[0] === "help" && argv[1] === "provider")) { - await executeProviderCommand(argv, { allowOutput: true }); + const providerArgv = expandProviderAlias(argv); + if (providerArgv) { + await executeProviderCommand(providerArgv, { allowOutput: true }); return; } if (argv[0] === "github-action") { @@ -38,7 +39,7 @@ async function main(): Promise { env: process.env, stream: process.stderr }); - await executeReviewCommand(parsed, { + const completed = await executeReviewCommand(parsed, { ...(progress !== undefined ? { onTelemetryEvent: progress.onTelemetryEvent } : {}), writeOutput: (text) => { progress?.stop(); @@ -46,6 +47,7 @@ async function main(): Promise { process.stdout.write(text); } }); + if (completed.review.health?.status === "failed") process.exitCode = 1; progress?.stop(); } catch (error) { progress?.stop(); diff --git a/src/cli/provider-command.ts b/src/cli/provider-command.ts index ec93121..4e8a1fa 100644 --- a/src/cli/provider-command.ts +++ b/src/cli/provider-command.ts @@ -21,6 +21,15 @@ export async function executeProviderCommand( await runProviderCommand(parsed.args, { ...opts, ...parsed.options }); } +// `codegenie use ` is shorthand for `codegenie provider use `. +// Returns the expanded provider argv, or undefined for any other command. +export function expandProviderAlias(argv: string[]): string[] | undefined { + if (argv[0] === "provider" || (argv[0] === "help" && argv[1] === "provider")) return argv; + if (argv[0] === "use") return ["provider", ...argv]; + if (argv[0] === "help" && argv[1] === "use") return ["provider", "use", "--help"]; + return undefined; +} + export function parseProviderCommand( argv: string[], opts: ParseProviderCommandOptions = {} diff --git a/src/cli/review-command.ts b/src/cli/review-command.ts index 68678c4..c906107 100644 --- a/src/cli/review-command.ts +++ b/src/cli/review-command.ts @@ -50,6 +50,7 @@ type CommanderReviewOptions = { model?: string; reasoning?: string; compositionReasoningStepDown?: boolean; + skipSvgReview?: boolean; format?: string; postGithubComments?: boolean; cache?: boolean; @@ -92,6 +93,8 @@ export function parseReviewCommand( .option("--provider ", "provider override") .option("--model ", "model override, optionally with a :reasoning suffix (e.g. claude-opus-5:max)") .option("--reasoning ", "reasoning level: minimal, low, medium, high, xhigh, max, or auto") + .option("--skip-svg-review", "skip SVG files during review (default)") + .option("--no-skip-svg-review", "include SVG files in review, subject to other exclusion rules") .option("--composition-reasoning-step-down", "use the next lower supported reasoning level for composition only") .option("--no-composition-reasoning-step-down", "use the configured reasoning level for composition") .option("--format ", "output format: markdown or json", "markdown") @@ -120,6 +123,7 @@ export function parseReviewCommand( providerConfig.command("set-model").argument("").argument(""); providerConfig.command("set-depth").argument(""); providerConfig.command("set-reasoning").argument(REASONING_USAGE); + program.command("use").description("shorthand for `provider use`").argument(""); program.command("version").description("show codegenie version"); program.command("eval").description("run codegenie eval suites"); @@ -316,6 +320,9 @@ function buildCliOverrides(options: CommanderReviewOptions): CliConfigOverrides if (options.compositionReasoningStepDown !== undefined) { cli.compositionReasoningStepDown = options.compositionReasoningStepDown; } + if (options.skipSvgReview !== undefined) { + cli.skipSvgReview = options.skipSvgReview; + } if (options.cache !== undefined) { cli.cacheEnabled = options.cache; } diff --git a/src/config/config-loader.ts b/src/config/config-loader.ts index 946e86f..def31a0 100644 --- a/src/config/config-loader.ts +++ b/src/config/config-loader.ts @@ -28,6 +28,7 @@ export type CliConfigOverrides = { depth?: ReviewDepth; budgetBoost?: number; compositionReasoningStepDown?: boolean; + skipSvgReview?: boolean; maxTimeMs?: number; lenses?: string[]; provider?: string; @@ -59,6 +60,7 @@ const DEFAULT_SOURCE_PATHS = [ "review.depth", "review.verify", "review.compositionReasoningStepDown", + "review.skipSvgReview", "review.maxFindings", "review.softCommentCap", "review.minConfidence", @@ -82,7 +84,7 @@ const DEFAULT_SOURCE_PATHS = [ "eval.logsDir" ]; -const REPO_SAFE_REVIEW_KEYS = new Set(["depth", "maxFindings", "softCommentCap", "budgetBoost", "maxTime", "compositionReasoningStepDown"]); +const REPO_SAFE_REVIEW_KEYS = new Set(["depth", "maxFindings", "softCommentCap", "budgetBoost", "maxTime", "compositionReasoningStepDown", "skipSvgReview"]); const CREDENTIAL_KEY_PATTERN = /(?:api[_-]?key|apikey|secret|token|password|passwd|authorization|credentials|auth)/i; export function loadConfig(opts: LoadConfigOptions): LoadedConfig { @@ -257,6 +259,10 @@ function applyRawConfig( config.review.compositionReasoningStepDown = raw.review.compositionReasoningStepDown; sources["review.compositionReasoningStepDown"] = source; } + if (raw.review?.skipSvgReview !== undefined) { + config.review.skipSvgReview = raw.review.skipSvgReview; + sources["review.skipSvgReview"] = source; + } if (raw.review?.budgetBoost !== undefined) { config.review.budgetBoost = raw.review.budgetBoost; sources["review.budgetBoost"] = source; @@ -372,6 +378,9 @@ function filterRepoConfig(raw: RawCodegenieConfig, warnings: ConfigWarning[]): R if (raw.review.compositionReasoningStepDown !== undefined) { safe.review.compositionReasoningStepDown = raw.review.compositionReasoningStepDown; } + if (raw.review.skipSvgReview !== undefined) { + safe.review.skipSvgReview = raw.review.skipSvgReview; + } if (raw.review.budgetBoost !== undefined) { safe.review.budgetBoost = raw.review.budgetBoost; } @@ -483,6 +492,10 @@ function applyCliOverrides( config.review.compositionReasoningStepDown = cli.compositionReasoningStepDown; sources["review.compositionReasoningStepDown"] = "cli"; } + if (cli.skipSvgReview !== undefined) { + config.review.skipSvgReview = cli.skipSvgReview; + sources["review.skipSvgReview"] = "cli"; + } if (cli.budgetBoost !== undefined) { config.review.budgetBoost = cli.budgetBoost; sources["review.budgetBoost"] = "cli"; diff --git a/src/config/schema.ts b/src/config/schema.ts index 3c4eb98..9020086 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -44,6 +44,7 @@ export const rawConfigSchema = z depth: reviewDepthSchema.optional(), verify: z.boolean().optional(), compositionReasoningStepDown: z.boolean().optional(), + skipSvgReview: z.boolean().optional(), minSeverity: severitySchema.optional(), maxFindings: positiveIntSchema.optional(), softCommentCap: positiveIntSchema.optional(), @@ -132,6 +133,7 @@ export const codegenieConfigSchema = z depth: reviewDepthSchema, verify: z.boolean(), compositionReasoningStepDown: z.boolean(), + skipSvgReview: z.boolean(), minSeverity: severitySchema.optional(), maxFindings: positiveIntSchema, softCommentCap: positiveIntSchema, @@ -205,6 +207,7 @@ export const defaultConfig: CodegenieConfig = { depth: "normal", verify: true, compositionReasoningStepDown: true, + skipSvgReview: true, maxFindings: 25, softCommentCap: 7, minConfidence: "medium", diff --git a/src/evals/eval-runner.ts b/src/evals/eval-runner.ts index fb533d4..406a58a 100644 --- a/src/evals/eval-runner.ts +++ b/src/evals/eval-runner.ts @@ -141,6 +141,7 @@ const caseSchema = z deepEnsemblePasses: positiveIntSchema.max(MAX_DEEP_ENSEMBLE_PASSES).optional(), adaptiveSecondPass: z.boolean().optional(), compositionReasoningStepDown: z.boolean().optional(), + skipSvgReview: z.boolean().optional(), verify: z.boolean().optional(), cache: z.boolean().optional(), cacheDir: z.string().min(1).optional(), @@ -867,6 +868,9 @@ function applyCaseReviewConfig( if (review?.compositionReasoningStepDown !== undefined) { config.review.compositionReasoningStepDown = review.compositionReasoningStepDown; } + if (review?.skipSvgReview !== undefined) { + config.review.skipSvgReview = review.skipSvgReview; + } if (review?.verify !== undefined) { config.review.verify = review.verify; } @@ -908,6 +912,7 @@ function evalEffectiveConfig(config: CodegenieConfig): NonNullable; catFile(ref: string, path: string): Promise; @@ -19,8 +21,8 @@ export interface GitClient { grep( ref: string, pattern: string, - opts?: { glob?: string; maxResults?: number; caseSensitive?: boolean; fixedString?: boolean; word?: boolean } - ): Promise; + opts?: { glob?: string; paths?: string[]; maxResults?: number; caseSensitive?: boolean; fixedString?: boolean; word?: boolean } + ): Promise; mergeBase(a: string, b: string): Promise; log(range: string): Promise; diff(base: string, head: string): Promise; @@ -130,42 +132,65 @@ export function createGitClient(repoRoot: string, opts: CreateGitClientOptions = async grep( ref: string, pattern: string, - grepOpts: { glob?: string; maxResults?: number; caseSensitive?: boolean; fixedString?: boolean; word?: boolean } = {} - ): Promise { + grepOpts: { glob?: string; paths?: string[]; maxResults?: number; caseSensitive?: boolean; fixedString?: boolean; word?: boolean } = {} + ): Promise { assertSafeRef(ref); if (grepOpts.glob !== undefined) { assertSafeGlob(grepOpts.glob); } - const args = [ - "grep", - "-I", - "-n", - "--column", - "--no-color", - grepOpts.fixedString === true ? "-F" : "-E", - ...(grepOpts.word === true ? ["-w"] : []), - ...(grepOpts.caseSensitive === false ? ["-i"] : []), - "-e", - pattern, - ref, - "--" - ]; - if (grepOpts.glob !== undefined) { - args.push(`:(glob)${grepOpts.glob}`); + if (!grepOpts.fixedString && hasUnsupportedEreSyntax(pattern)) { + throw new CodegenieError("invalid_args", "query uses unsupported regex syntax. Use POSIX ERE (e.g. name|other, [0-9], [[:space:]]); lookarounds and Perl digit classes are unsupported."); } const maxResults = grepOpts.maxResults ?? 50; - const stdout = await runGitCapped(repoRoot, args, { - maxBytes: Math.max(64 * 1024, maxResults * 1024), - maxLines: maxResults, - allowedExitCodes: [0, 1], - errorCode: "git_ref_missing" - }); - return stdout - .split("\n") - .filter(Boolean) - .slice(0, maxResults) - .map((line) => parseGrepLine(line, ref)) - .filter((result): result is SearchResult => result !== undefined); + if ((grepOpts.paths?.length ?? 0) > 20_000) throw new CodegenieError("budget_exhausted", "pathGlob expands to more than 20000 tracked paths; narrow the scope before searching (no files searched)"); + const chunks: string[][] = [[]]; + let chunkBytes = 0; + for (const path of grepOpts.paths ?? []) { + assertSafePathspec(path); + const bytes = Buffer.byteLength(path) + 16; + if (bytes > 24_000) throw new CodegenieError("budget_exhausted", "search path exceeds argument allowance; narrow pathGlob"); + if (chunkBytes + bytes > 24_000 || chunks[chunks.length - 1]!.length >= 256) { + chunks.push([]); + chunkBytes = 0; + } + chunks[chunks.length - 1]!.push(`:(literal)${path}`); + chunkBytes += bytes; + } + const results: GrepResults = []; + let omittedLines = 0; + for (const paths of chunks) { + const args = ["grep", "-I", "-n", "--column", "--no-color", + grepOpts.fixedString === true ? "-F" : "-E", + ...(grepOpts.word === true ? ["-w"] : []), + ...(grepOpts.caseSensitive === false ? ["-i"] : []), + "-e", pattern, ref, "--", + ...paths, + // Still ask Git to compile ERE when the selected scope is empty. + ...(grepOpts.paths?.length === 0 ? [":(exclude,glob)**"] : []), + ...(grepOpts.glob !== undefined ? [`:(glob)${grepOpts.glob}`] : [])]; + let stdout: string; + try { + stdout = await runGitCapped(repoRoot, args, { + maxBytes: Math.max(64 * 1024, (maxResults - results.length) * 1024), + maxLines: maxResults - results.length, + lineLimitBytes: 32 * 1024, + onOmittedLine: () => { omittedLines++; }, + allowedExitCodes: [0, 1], errorCode: "git_ref_missing" + }); + } catch (error) { + const stderr = error instanceof CodegenieError ? String(error.context?.stderr ?? "") : ""; + if (/^fatal: (?:command line, |-e option, )/u.test(stderr)) { + throw new CodegenieError("invalid_args", `query is invalid POSIX ERE: ${stderr.slice(0, 300)}. Check brackets and parentheses; use name|other for alternatives.`, { cause: error }); + } + throw error; + } + results.push(...stdout.split("\n").filter(Boolean) + .slice(0, maxResults - results.length).map((line) => parseGrepLine(line, ref)) + .filter((result): result is SearchResult => result !== undefined)); + if (results.length >= maxResults) break; + } + if (omittedLines) results.omittedLines = omittedLines; + return results; }, async mergeBase(a: string, b: string): Promise { @@ -485,3 +510,21 @@ function orderRemotes(remotes: GitRemote[]): GitRemote[] { return a.name.localeCompare(b.name); }); } + +// Detect recognizable dialect mistakes without using JavaScript to validate ERE. +// Escaped punctuation and character-class contents remain literal. +function hasUnsupportedEreSyntax(pattern: string): boolean { + let inClass = false; + for (let i = 0; i < pattern.length; i++) { + const char = pattern[i]; + if (char === "\\") { + if (!inClass && /[dD]/u.test(pattern[i + 1] ?? "")) return true; + i++; + continue; + } + if (char === "[") inClass = true; + else if (char === "]") inClass = false; + else if (!inClass && char === "(" && pattern[i + 1] === "?") return true; + } + return false; +} diff --git a/src/git/subprocess.ts b/src/git/subprocess.ts index 5882439..17130a9 100644 --- a/src/git/subprocess.ts +++ b/src/git/subprocess.ts @@ -16,6 +16,9 @@ export type GitCommandOptions = { }; export type GitCappedCommandOptions = { + /** Skip complete oversized records, retaining other lines within the same allowance. */ + lineLimitBytes?: number; + onOmittedLine?: () => void; maxBytes: number; maxLines: number; timeoutMs?: number; @@ -72,9 +75,29 @@ export async function runGitCapped( let collectedLines = 0; let reachedLimit = false; let settled = false; + let timedOut = false; + let pending = Buffer.alloc(0); + let oversized = false; + + const finishLine = () => { + if (oversized || collectedBytes + pending.length + 1 > opts.maxBytes) { + opts.onOmittedLine?.(); + } else { + chunks.push(pending, Buffer.from("\n")); + collectedBytes += pending.length + 1; + collectedLines++; + } + pending = Buffer.alloc(0); + oversized = false; + if (collectedLines >= opts.maxLines) { + reachedLimit = true; + child.kill("SIGTERM"); + } + }; const timeout = setTimeout(() => { if (!settled) { + timedOut = true; reachedLimit = false; child.kill("SIGTERM"); } @@ -84,6 +107,22 @@ export async function runGitCapped( if (reachedLimit) { return; } + if (opts.lineLimitBytes !== undefined) { + let offset = 0; + while (offset < chunk.length && !reachedLimit) { + const newline = chunk.indexOf(0x0a, offset); + const end = newline < 0 ? chunk.length : newline; + if (!oversized) { + if (pending.length + end - offset > opts.lineLimitBytes) { + oversized = true; + pending = Buffer.alloc(0); + } else pending = Buffer.concat([pending, chunk.subarray(offset, end)]); + } + if (newline >= 0) finishLine(); + offset = end + 1; + } + return; + } const remaining = opts.maxBytes - collectedBytes; if (remaining > 0) { const slice = chunk.subarray(0, Math.max(0, remaining)); @@ -118,6 +157,11 @@ export async function runGitCapped( return; } settled = true; + if (timedOut) { + reject(new CodegenieError("timeout", "Git search/read timed out; results are unavailable.")); + return; + } + if (opts.lineLimitBytes !== undefined && !reachedLimit && (pending.length || oversized)) finishLine(); const output = truncateLines(Buffer.concat(chunks).toString("utf8"), opts.maxLines); if (reachedLimit || (typeof code === "number" && allowedExitCodes.has(code))) { resolve(output); diff --git a/src/github-action/entrypoint.ts b/src/github-action/entrypoint.ts index c47af9e..47e3274 100644 --- a/src/github-action/entrypoint.ts +++ b/src/github-action/entrypoint.ts @@ -39,6 +39,7 @@ export type RunReviewResult = RunAttachment & { // The full markdown review. Never the writeOutput capture: with inline // posting enabled, stdout carries the short posting summary, not the report. reportMarkdown: string; + failed?: boolean; }; export type ExecuteGitHubActionOptions = { @@ -229,6 +230,11 @@ export async function executeGitHubActionCommand( // Fallback copies land before the terminal PATCH so the report survives a // failed edit (which still fails the run as github_post_failed). publishReportFiles(runResult.reportMarkdown, env); + if (runResult.failed) { + await controller.finalizeFailure("review_failed", undefined, "Required review work failed. See the saved report for diagnostics.", runResult.reportMarkdown); + emitActionRecord(runResult.runDir, eventName, authorized, "review_failed", controller.stats(), env, write, "review_failed"); + throw new CodegenieError("review_failed", "Required review work failed; partial report retained."); + } try { await controller.finalizeSuccess(runResult.reportMarkdown); } catch (error) { @@ -256,6 +262,7 @@ export function toRunReviewResult(result: { runId: string; runDir: string; revie return { runId: result.runId, runDir: result.runDir, + failed: result.review.health?.status === "failed", reportMarkdown: scrubGitHubSecrets(renderMarkdownReview(result.review)) }; } diff --git a/src/github-action/status-comment.ts b/src/github-action/status-comment.ts index 59e83ea..271bb60 100644 --- a/src/github-action/status-comment.ts +++ b/src/github-action/status-comment.ts @@ -34,7 +34,8 @@ export type StatusCommentController = { finalizeFailure( errorCode: string, diagnostic?: StructuredSubmitFailureDiagnostic, - providerMessage?: string + providerMessage?: string, + reportMarkdown?: string ): Promise; settle(): Promise; stats(): StatusCommentStats; @@ -228,14 +229,16 @@ export function createStatusCommentController(options: StatusCommentOptions): St async function finalizeFailure( errorCode: string, diagnostic?: StructuredSubmitFailureDiagnostic, - providerMessage?: string + providerMessage?: string, + reportMarkdown?: string ): Promise { if (commentId === undefined) { return false; } terminal = true; await settle(); - const body = appendStatusCommentMarker(renderFailureBody(errorCode, options.runUrl, diagnostic, providerMessage)); + const body = appendStatusCommentMarker(reportMarkdown === undefined ? renderFailureBody(errorCode, options.runUrl, diagnostic, providerMessage) + : capTerminalBody(sanitizeGitHubCommentBody(reportMarkdown), options.runUrl, STATUS_COMMENT_MARKER.length + 4).body); stats.terminalState = "failure"; const bodyBytes = Buffer.byteLength(body, "utf8"); stats.finalBodyBytes = bodyBytes; diff --git a/src/llm/llm-runner.ts b/src/llm/llm-runner.ts index f4c7cae..2aa512a 100644 --- a/src/llm/llm-runner.ts +++ b/src/llm/llm-runner.ts @@ -26,6 +26,8 @@ export type LlmCallUsage = { }; export type ToolExecutionResult = { + /** Canonical bounded matches; packed after cache lookup for each consumer. */ + searchResults?: import("../types.js").SearchResult[]; text: string; isError?: boolean; errorCode?: CodegenieErrorCode; @@ -54,6 +56,8 @@ export interface ToolResultCache { } export type LlmToolResultSummary = { + repositoryEvidence?: import("../types.js").RepositoryEvidence; + requestKey?: string; id: string; tool: string; target: string; @@ -86,6 +90,7 @@ export type ToolDefinition = { }; export type LlmStructuredRequest = { + onToolResults?(results: LlmToolResultSummary[]): void; /** Worker cancellation, combined with the overall run signal, including repairs. */ signal?: AbortSignal; /** Type-only link between the request and the expected submit payload. */ diff --git a/src/llm/pi-runner.ts b/src/llm/pi-runner.ts index 32a4805..65ae974 100644 --- a/src/llm/pi-runner.ts +++ b/src/llm/pi-runner.ts @@ -1,3 +1,4 @@ +import { packSearchToolResult } from "./search-result-packing.js"; import { createFieldRepair, mergeRepairDraft, type FieldRepair } from "./field-repair.js"; import { randomUUID } from "node:crypto"; import { cleanupSubmitShape, focusedRepairDiagnostics, preservationViolations, submissionIssues } from "./submit-preservation.js"; @@ -658,6 +659,7 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { obligationId: recoveryObligations.get(obligationKey)?.id, validation: "complete_schema_and_semantics_passed" }); } resolveObligation(localEdits.length && !schemaRepairUsed ? "deterministic_correction" : "model_repair"); + request.onToolResults?.(toolResultSummaries); return validated as T; } catch (cause) { if (fieldRepair) { @@ -845,7 +847,16 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { // Fixed budget-status messages are control information, not source content. // Keep them visible even at zero remaining characters, without spending // the reserve for decisive source reads. Rejected calls still count above. - const resultText = budgetRejected + if (!budgetRejected && outcome.result.searchResults) { + outcome.result = packSearchToolResult(outcome.result, remainingResultChars); + if (outcome.result.isError) { + outcome.status = "rejected"; + outcome.rejectionReason = "tool_result_budget_exhausted"; + if (outcome.result.errorCode) outcome.errorCode = outcome.result.errorCode; + } + } + const searchBudgetRejected = outcome.result.errorCode === "budget_exhausted" && outcome.result.meta?.deliveryStatus === "budget_rejected"; + const resultText = budgetRejected || searchBudgetRejected ? outcome.result.text : fitToolResultText(outcome.result.text, remainingResultChars); if (resultText.length < outcome.result.text.length) { @@ -855,7 +866,7 @@ export function createPiRunner(opts: CreateRunnerOptions): LlmRunner { meta: markTruncated(outcome.result.meta) }; } - if (!budgetRejected) { + if (!budgetRejected && !searchBudgetRejected) { resultCharsUsed += resultText.length; } if (extensionDecision?.status === "granted") { @@ -1114,9 +1125,19 @@ function summarizeToolResult(toolCall: PiToolCall, outcome: ToolRunOutcome, resu id: toolCall.id || safeFenceLabelPart(toolCall.name), tool: toolCall.name, target: toolTargetSummary(toolCall), + requestKey: sha256Hex(stableJson({ tool: toolCall.name, arguments: toolCall.arguments })), status: outcome.status, resultChars: resultText.length, preview: firstMeaningfulLine(resultText), + repositoryEvidence: outcome.status === "ok" && !outcome.result.isError && !meta?.truncated && !meta?.degraded + && meta?.deliveryStatus === "full" && meta.lookupStatus === "found" + && ["read_range", "read_symbol", "find_definition"].includes(toolCall.name) + && resultText.trim() && resultText.length <= 8_000 + ? { id: toolCall.id, tool: toolCall.name, + ...(typeof toolCall.arguments.symbolName === "string" ? { symbols: [toolCall.arguments.symbolName] } : {}), + ...(typeof toolCall.arguments.path === "string" ? { path: toolCall.arguments.path } : {}), + source: meta.sourceUsed ?? (toolCall.arguments.source === "base" ? "base" : "head"), + text: resultText } : undefined, errorCode: outcome.errorCode, rejectionReason: outcome.rejectionReason, degraded: meta?.degraded, @@ -2449,6 +2470,8 @@ function recordToolCall( degradationReason: meta.degradationReason, truncated: meta.truncated, omittedCount: meta.omittedCount, + omittedCountIsLowerBound: meta.omittedCountIsLowerBound, + discoveryLimited: meta.discoveryLimited, lookupStatus: meta.lookupStatus, deliveryStatus: meta.deliveryStatus, recovery: meta.recovery, @@ -4187,6 +4210,8 @@ function toolArgsForRecord(args: Record): ToolCallRecord["args" startLine: typeof args.startLine === "number" ? args.startLine : undefined, endLine: typeof args.endLine === "number" ? args.endLine : undefined, query: typeof args.query === "string" ? args.query : undefined, + pathGlob: typeof args.pathGlob === "string" ? args.pathGlob.slice(0, 500) : undefined, + maxResults: typeof args.maxResults === "number" ? args.maxResults : undefined, glob: typeof args.glob === "string" ? args.glob : undefined, source: sourceForRecord(args.source), contextMode: typeof args.contextMode === "string" ? args.contextMode : undefined diff --git a/src/llm/schemas.ts b/src/llm/schemas.ts index 7144892..910421f 100644 --- a/src/llm/schemas.ts +++ b/src/llm/schemas.ts @@ -279,7 +279,7 @@ export const SubmitCompositionSchema = Type.Object( supportingRefs: Type.Array(Type.String({ minLength: 1, maxLength: 300 }), { minItems: 1, maxItems: 20 }), rationale: Type.String({ minLength: 1, maxLength: 2000 }), remainingQuestion: Type.Optional(Type.String({ minLength: 1, maxLength: 2000 })) - }, { additionalProperties: false }), { maxItems: 30, description: "Optional resolutions of supplied verifier concerns only. Omission leaves concerns unchanged. Attention-only references cannot account for finding sources." })), + }, { additionalProperties: false }), { maxItems: 30, description: "Optional resolutions of supplied verifier or packet concerns only. Omission leaves concerns unchanged. Attention-only references cannot account for finding sources." })), composedFindings: Type.Array( Type.Object( { @@ -320,7 +320,7 @@ export const SCHEMA_VERSIONS = { submit_review: 5, submit_system_review: 2, submit_verdict: 11, - submit_composition: 7 + submit_composition: 8 } as const; export function submitToolNameForStage(stage: ReviewStage): keyof typeof SCHEMA_VERSIONS { diff --git a/src/llm/search-result-packing.ts b/src/llm/search-result-packing.ts new file mode 100644 index 0000000..f309ae7 --- /dev/null +++ b/src/llm/search-result-packing.ts @@ -0,0 +1,40 @@ +import type { ToolExecutionResult } from "./llm-runner.js"; + +/** Pack whole entries after cache lookup, never mutate the shared canonical result. */ +export function packSearchToolResult(input: ToolExecutionResult, allowance: number): ToolExecutionResult { + if (!input.searchResults) return input; + let results = structuredClone(input.searchResults); + let omitted = input.meta?.omittedCount ?? 0; + let shortened = false; + const render = (): string => JSON.stringify({ + results, + meta: { ...input.meta, ...(omitted || shortened ? { degraded: input.meta?.degraded || omitted > (input.meta?.omittedCount ?? 0), truncated: true, omittedCount: omitted, omittedCountIsLowerBound: true } : {}) }, + ...(omitted || shortened || input.meta?.truncated ? { notice: "Bounded results, not exhaustive. Optional context/excerpts may be shortened. Omitted count is a lower bound. Narrow pathGlob or read a known range." } : {}) + }); + if (render().length > allowance) { + shortened = true; + for (const result of results) { + delete result.enclosingSymbol; + delete result.contextBefore; + delete result.contextAfter; + } + } + if (render().length > allowance) { + const available = results; + results = []; + // Reserve the omission notice before admitting entries; an oversized first + // match must not evict later matches that fit. + omitted += available.length; + for (const result of available) { + results.push(result); + omitted--; + if (render().length > allowance) { results.pop(); omitted++; } + } + } + if (render().length > allowance || (input.searchResults.length > 0 && results.length === 0)) { + return { text: "Search results could not fit the remaining result budget. Narrow pathGlob or read a known range; this is not a zero-match result.", isError: true, errorCode: "budget_exhausted", + ...(input.meta ? { meta: { ...input.meta, truncated: true, deliveryStatus: "budget_rejected" } } : {}) }; + } + return { ...input, text: render(), ...(input.meta ? { meta: { ...input.meta, + ...(omitted || shortened ? { degraded: input.meta.degraded || omitted > (input.meta.omittedCount ?? 0), truncated: true, omittedCount: omitted, omittedCountIsLowerBound: true } : {}) } } : {}) }; +} diff --git a/src/llm/tool-definitions.ts b/src/llm/tool-definitions.ts index 1a4a337..0b2edac 100644 --- a/src/llm/tool-definitions.ts +++ b/src/llm/tool-definitions.ts @@ -64,7 +64,7 @@ export function buildRepositoryToolDefinitions(tools: RepositoryTools, options: }, { name: "read_symbol", - description: "Read a symbol by exact symbolName or by the smallest enclosing symbol at line; provide exactly one selector. Use source {kind:\"auto\"} for renamed or deleted symbols so head is searched first, then base.", + description: "Requires path; discover an unknown path with find_definition first. Read a symbol by exact symbolName or by the smallest enclosing symbol at line; provide exactly one selector. Use source {kind:\"auto\"} for renamed or deleted symbols so head is searched first, then base.", parameters: Type.Object( { path: Type.String({ minLength: 1 }), @@ -138,7 +138,7 @@ export function buildRepositoryToolDefinitions(tools: RepositoryTools, options: }, { name: "search_files", - description: "Search file contents with a POSIX ERE query at the head or base revision.", + description: "Search committed contents with POSIX ERE (name|other, no lookarounds). pathGlob uses the same glob dialect as list_files: **, *, ?, character classes and {api,data} alternatives. contextMode: none, lines, symbols. Empty or truncated results do not prove repository-wide absence.", parameters: Type.Object( { query: Type.String({ minLength: 1, maxLength: 500 }), @@ -159,12 +159,12 @@ export function buildRepositoryToolDefinitions(tools: RepositoryTools, options: caseSensitive: input.caseSensitive, source: input.source }))); - return { text: withMeta(JSON.stringify(result.results, null, 2), result.meta), meta: result.meta }; + return { text: withMeta(JSON.stringify(result.results), result.meta), searchResults: result.results, meta: result.meta }; }) }, { name: "find_symbol_mentions", - description: "Find text/token mentions of an identifier, optionally constrained by pathGlob, contextMode, maxResults, and source.", + description: "Find identifier mentions. pathGlob uses list_files glob semantics including {api,data}/** alternatives. contextMode: none, lines, symbols; discovery and syntax inspection are bounded.", parameters: Type.Object( { symbolName: Type.String({ minLength: 1, maxLength: 200 }), @@ -183,7 +183,7 @@ export function buildRepositoryToolDefinitions(tools: RepositoryTools, options: maxResults: input.maxResults, source: input.source }))); - return { text: withMeta(JSON.stringify(result.results, null, 2), result.meta), meta: result.meta }; + return { text: withMeta(JSON.stringify(result.results), result.meta), searchResults: result.results, meta: result.meta }; }) }, { @@ -311,7 +311,7 @@ function withMeta(text: string, meta: ToolResultMeta): string { notes.push(`degraded${meta.degradationReason ? `: ${meta.degradationReason}` : ""}`); } if (meta.truncated) { - notes.push(`truncated${meta.omittedCount ? `: ${meta.omittedCount} omitted` : ""}`); + notes.push(`truncated${meta.omittedCount ? `: ${meta.omittedCountIsLowerBound ? "at least " : ""}${meta.omittedCount} omitted` : ""}`); } return notes.length > 0 ? `${text}\n\n[tool meta: ${notes.join("; ")}]` : text; } diff --git a/src/llm/tool-result-cache.ts b/src/llm/tool-result-cache.ts index 049a650..9be68ad 100644 --- a/src/llm/tool-result-cache.ts +++ b/src/llm/tool-result-cache.ts @@ -58,7 +58,7 @@ export function createToolResultCache(opts: CreateToolResultCacheOptions = {}): }; const write = (key: string, result: ToolExecutionResult): number => { - const entry = { result: cloneToolResult(result), resultChars: result.text.length }; + const entry = { result: cloneToolResult(result), resultChars: result.text.length + (result.searchResults ? JSON.stringify(result.searchResults).length : 0) }; const existing = entries.get(key); if (existing !== undefined) { storedResultChars -= existing.resultChars; @@ -183,6 +183,7 @@ function isCacheableResult(result: ToolExecutionResult): boolean { function cloneToolResult(result: ToolExecutionResult): ToolExecutionResult { const output: ToolExecutionResult = { text: result.text }; + if (result.searchResults !== undefined) output.searchResults = structuredClone(result.searchResults); if (result.isError !== undefined) { output.isError = result.isError; } diff --git a/src/output/markdown-renderer.ts b/src/output/markdown-renderer.ts index a342591..d89fba1 100644 --- a/src/output/markdown-renderer.ts +++ b/src/output/markdown-renderer.ts @@ -1,14 +1,17 @@ +import { healthForResult, renderReviewHealth, factualReviewSummary } from "../util/review-health.js"; import type { BudgetLimitEvent, BudgetSummary, FinalFinding, ReviewResult, ReviewRunStats, RunCoverageStatus, RunPostingRecord } from "../types.js"; import { renderBudgetStopNotice, renderCoverageSummaryLines, renderCoverageTrustBanner } from "../util/coverage-summary.js"; import { inlineCode, severityBadge } from "../util/markdown.js"; export function renderMarkdownReview(result: ReviewResult): string { + const health = healthForResult(result); const sections = [ "# 🧞 Codegenie Review", "", - renderCoverageTrustBanner(result.coverage), + renderReviewHealth(health), + ...(health.status === "completed" ? [renderCoverageTrustBanner(result.coverage)] : []), renderBudgetStopNotice(result.coverage), - result.summary.trim() || "Review completed.", + health.status === "completed" ? result.summary.trim() || "Review completed." : factualReviewSummary(health, result.findings.length + result.summaryOnlyFindings.length), "", renderCoverage(result.coverage), renderFindings("⚠️ Findings", result.findings, result.runStats?.git), @@ -22,16 +25,16 @@ export function renderMarkdownReview(result: ReviewResult): string { } function renderNoFindings(result: ReviewResult): string { - if (!result.noFindings) { + if (!result.noFindings || healthForResult(result).status === "failed") { return ""; } - if (result.coverage.partial) { + if (healthForResult(result).status !== "completed") { return ( - "## ⚠️ Review Incomplete\n\n" + - "Completed review work produced no credible verified findings, but incomplete coverage or verification prevents a clean conclusion." + "## No confirmed findings\n\n" + + "No confirmed findings were retained. The limitations above prevent a clean conclusion." ); } - return "## ✅ No Findings\n\nNo credible findings were found. Everything looks good."; + return "## ✅ No Findings\n\nNo credible findings were found within the reviewed scope."; } function renderCoverage(coverage: RunCoverageStatus): string { diff --git a/src/output/stdout-renderer.ts b/src/output/stdout-renderer.ts index 94e81d9..315d1fc 100644 --- a/src/output/stdout-renderer.ts +++ b/src/output/stdout-renderer.ts @@ -1,3 +1,4 @@ +import { healthForResult, renderReviewHealth } from "../util/review-health.js"; import type { OutputFormat, ReviewResult } from "../types.js"; import { renderJsonReview } from "./json-renderer.js"; import { renderMarkdownReview } from "./markdown-renderer.js"; @@ -11,12 +12,15 @@ export function renderPostingSummaryForStdout( format: OutputFormat, opts: { postRequested?: boolean } = {} ): string { + const health = healthForResult(result); + const notice = renderReviewHealth(health); if (result.posting !== undefined) { if (format === "json") { - return `${JSON.stringify(result.posting, null, 2)}\n`; + return `${JSON.stringify({ ...result.posting, health }, null, 2)}\n`; } return [ "codegenie GitHub posting summary", + ...(notice ? [notice] : []), `Status: ${result.posting.status}`, `Inline comments posted: ${result.posting.inlinePosted}`, `Demoted to review body: ${result.posting.demotedToBody}`, @@ -26,6 +30,7 @@ export function renderPostingSummaryForStdout( } const summary = { + health, summary: result.summary, findings: result.findings.length, summaryOnlyFindings: result.summaryOnlyFindings.length, @@ -38,6 +43,7 @@ export function renderPostingSummaryForStdout( } return [ "codegenie GitHub posting summary", + ...(notice ? [notice] : []), `Findings ready to post: ${result.findings.length}`, `Summary-only findings: ${result.summaryOnlyFindings.length}`, result.postingPlan diff --git a/src/pipeline/attention-reconciliation.ts b/src/pipeline/attention-reconciliation.ts index 7b3dd19..8725db5 100644 --- a/src/pipeline/attention-reconciliation.ts +++ b/src/pipeline/attention-reconciliation.ts @@ -1,7 +1,7 @@ import type { SubmitComposition } from "../llm/schemas.js"; import type { CandidateFinding, NeedsHumanAttentionNote, PacketReviewResult, ResolvedReviewInput, VerificationVerdict } from "../types.js"; import { compositionSources, safeReportProse, type CompositionSource } from "./composition-content.js"; -import { MAX_HUMAN_ATTENTION_NOTES } from "./human-attention.js"; +import type { RawAttentionHint } from "./human-attention.js"; export const MAX_ATTENTION_RECONCILIATION_CHARS = 16_000; type Resolution = NonNullable[number]; @@ -11,7 +11,7 @@ type Concern = { assumptionIndex: number; question: string; essential: boolean; - verdict: VerificationVerdict["verdict"]; + verdict: VerificationVerdict["verdict"] | "not_verified"; scope: NeedsHumanAttentionNote; }; type Evidence = { @@ -23,7 +23,10 @@ type Evidence = { text: string; path?: string; explanation?: string; - verdict: VerificationVerdict["verdict"]; + verdict: VerificationVerdict["verdict"] | "not_verified"; + origin?: "repository_tool"; + symbols?: string[]; + source?: "head" | "base"; proofStatus?: NonNullable["status"]; assumptions?: NonNullable["assumptions"]; }; @@ -37,7 +40,7 @@ type Inventory = { // This is review context, not a claim that each excerpt was read at head. // Individual excerpts/observations retain their original source wording. reviewRevision: { base?: string; head?: string }; - concerns: Concern[]; + concerns: Array & { files: string[] }>; // Published observations are already in the composition source components. // Register their IDs without duplicating their text in this bounded inventory. evidence: Array & { text?: string }>; @@ -82,12 +85,21 @@ export function buildAttentionReconciliation( verdicts: VerificationVerdict[], publishedInputs: CandidateFinding[], packetResults: PacketReviewResult[], - resolved: ResolvedReviewInput + resolved: ResolvedReviewInput, + packetHints: RawAttentionHint[] = [] ): AttentionReconciliation { const groups = verdicts.flatMap(verdict => { const group = concernGroup(verdict); return group ? [group] : []; }); + for (const hint of packetHints) { + if (hint.confidence === "low" || !hint.question.trim() || (!hint.files.length && !hint.symbols.length)) continue; + const original: NeedsHumanAttentionNote = { question: hint.question, files: hint.files, symbols: hint.symbols, + reason: hint.reason, confidence: hint.confidence, sourcePacketIds: [hint.packetId] }; + const candidateId = `packet:${hint.packetId}`; + groups.push({ candidateId, original, concerns: [{ id: `packet/${hint.id}`, candidateId, assumptionIndex: 0, + question: hint.question, essential: true, verdict: "not_verified", scope: original }] }); + } const candidates = new Map(packetResults.flatMap(result => result.findings).map(finding => [finding.id, finding])); for (const verdict of verdicts) if (verdict.finalFinding) candidates.set(verdict.candidateId, verdict.finalFinding); for (const finding of publishedInputs) candidates.set(finding.id, finding); @@ -138,6 +150,21 @@ export function buildAttentionReconciliation( }); } } + // Source reads are independent evidence, not a packet's own conclusion. + // Retain revision and tool provenance; failed/incomplete work cannot settle questions. + const seenReads = new Set(); + for (const packet of packetResults) { + if (packet.status !== "completed") continue; + for (const read of packet.repositoryEvidence ?? []) { + const key = JSON.stringify([read.source, read.path, read.text]); + if (!read.text.trim() || seenReads.has(key)) continue; + seenReads.add(key); + allEvidence.push({ id: `repository/${packet.packetId}/${read.id}`, candidateId: `repository:${packet.packetId}`, + kind: "excerpt", text: read.text, ...(read.path ? { path: read.path } : {}), + explanation: `Successful ${read.tool} at ${read.source}; packet ${packet.packetId}`, + origin: "repository_tool", ...(read.symbols ? { symbols: read.symbols } : {}), source: read.source, verdict: "not_verified" }); + } + } const inventory: Inventory = { reviewRevision: { ...(resolved.baseRef ? { base: resolved.baseRef } : {}), @@ -147,16 +174,15 @@ export function buildAttentionReconciliation( const omittedConcernIds: string[] = []; const omittedEvidenceIds: string[] = []; const excludedEvidenceIds: string[] = []; - let admittedGroups = 0; for (const group of groups) { if (!group.concerns.length) continue; - const next = [...inventory.concerns, ...group.concerns]; - if (admittedGroups >= MAX_HUMAN_ATTENTION_NOTES || JSON.stringify({ ...inventory, concerns: next }).length > MAX_ATTENTION_RECONCILIATION_CHARS) { + const next = [...inventory.concerns, ...group.concerns.map(concern => ({ id: concern.id, question: concern.question, files: concern.scope.files }))]; + // Reserve half the existing allowance for supporting evidence. + if (JSON.stringify({ ...inventory, concerns: next }).length > MAX_ATTENTION_RECONCILIATION_CHARS / 2) { omittedConcernIds.push(...group.concerns.map(concern => concern.id)); continue; } inventory.concerns = next; - admittedGroups++; } const eligible: Evidence[] = []; for (const evidence of allEvidence) { @@ -168,7 +194,8 @@ export function buildAttentionReconciliation( } // Give each question a turn instead of allowing the first candidate's excerpts // to consume the inventory. Ranking only allocates context; it proves nothing. - const queues = inventory.concerns.map(concern => [...eligible].sort((a, b) => + const fullConcerns = new Map(groups.flatMap(group => group.concerns).map(concern => [concern.id, concern])); + const queues = inventory.concerns.map(({ id }) => fullConcerns.get(id)!).map(concern => [...eligible].sort((a, b) => evidencePriority(b, concern) - evidencePriority(a, concern))); const considered = new Set(); const observations = new Set(); @@ -213,6 +240,7 @@ function evidencePriority(evidence: Evidence, concern: Concern): number { .replace(/([a-z])([A-Z])/g, "$1 $2").toLowerCase().match(/[\p{L}\p{N}_]{4,}/gu) ?? []); const overlap = [...terms].filter(term => words.has(term)).length / Math.max(terms.size, 1); return (evidence.candidateId !== concern.candidateId ? 8 : 0) + + (evidence.symbols?.filter(symbol => concern.scope.symbols.includes(symbol) || concern.question.includes(symbol)).length ?? 0) * 2 + overlap * 4 + (concern.scope.files.includes(evidence.path ?? "") ? 1 : 0) + (evidence.kind === "observation" ? 0.5 : 0) @@ -224,7 +252,8 @@ export function reconcileAttention( proposals: Resolution[] | undefined, enabled: boolean ) { - const concerns = new Map(input.inventory.concerns.map(concern => [concern.id, concern])); + const supplied = new Set(input.inventory.concerns.map(concern => concern.id)); + const concerns = new Map(input.groups.flatMap(group => group.concerns).filter(concern => supplied.has(concern.id)).map(concern => [concern.id, concern])); const evidence = new Map(input.inventory.evidence.map(source => [source.id, source])); const counts = new Map(); for (const proposal of proposals ?? []) counts.set(proposal.concernId, (counts.get(proposal.concernId) ?? 0) + 1); diff --git a/src/pipeline/composer.ts b/src/pipeline/composer.ts index 31a6b3f..41befd8 100644 --- a/src/pipeline/composer.ts +++ b/src/pipeline/composer.ts @@ -1,3 +1,4 @@ +import { deriveReviewHealth, factualReviewSummary, renderReviewHealth } from "../util/review-health.js"; import { createCompositionAttributionRepair, normalizeCompositionReferences } from "./composition-repair.js"; import { buildAttentionReconciliation, reconcileAttention, type AttentionReconciliation } from "./attention-reconciliation.js"; import { type CompositionMetrics, eligibleCompositionSource, compositionSources, composePresentation, safeReportProse, validateCompositionSubmission, renderRetainedComposition } from "./composition-content.js"; @@ -34,11 +35,10 @@ import { hasCriticalOrHighGuarantee } from "./severity-policy.js"; import type { LlmSchemaInvalidSubmitRecoveryInput, LlmSchemaRepairInput } from "../llm/llm-runner.js"; import { buildHumanAttentionNotes, - buildVerificationResolutionIndex, humanAttentionArtifact, selectHumanAttentionForOutput, selectHumanAttentionGroups, - suppressAttentionGroupsResolvedByVerification + MAX_HUMAN_ATTENTION_NOTES } from "./human-attention.js"; type ComposeOptions = { @@ -104,14 +104,10 @@ export async function dedupeRankAndComposeReview( ...(opts.diff !== undefined ? { diff: opts.diff } : {}), telemetry }); - const verificationResolutions = buildVerificationResolutionIndex(verified.verdicts, opts.packetResults ?? [], verified.verified, packetsById, coverage); - const preComposerAttentionGroups = suppressAttentionGroupsResolvedByVerification( - attention.groups, - verificationResolutions.filter((resolution) => resolution.verdict === "reject" && !verified.verdicts.some(verdict => verdict.candidateId === resolution.candidateId && verdict.unresolvedConcern)) - ).available; + const preComposerAttentionGroups = attention.groups; const composerPromptSelection = selectHumanAttentionGroups(preComposerAttentionGroups); const composerPromptNotes = composerPromptSelection.notes; - const attentionReconciliation = buildAttentionReconciliation(verified.verdicts, pretrim.kept, opts.packetResults ?? [], resolved); + const attentionReconciliation = buildAttentionReconciliation(verified.verdicts, pretrim.kept, opts.packetResults ?? [], resolved, attention.raw); if (pretrim.suppressed.length > 0) { const reason = `composer pre-trim suppressed ${pretrim.suppressed.length} verified finding${pretrim.suppressed.length === 1 ? "" : "s"} above the ${MAX_COMPOSER_FINDINGS}-finding composer input cap`; coverage.reasons.push(reason); @@ -246,15 +242,21 @@ export async function dedupeRankAndComposeReview( const summaryOnlyFindings = capped.findings.filter((finding) => finding.publication === "summary-only"); const publishableCount = findings.length + summaryOnlyFindings.length; const reconciledAttention = reconcileAttention(attentionReconciliation, composition.attentionResolutions, - compositionMode === "llm" && publishableCount > 0); + compositionMode === "llm"); + const decisionsByHint = new Map(reconciledAttention.decisions.filter(decision => decision.accepted).map(decision => [decision.concernId, decision])); + const regroupedAttention = buildHumanAttentionNotes([], { packets: opts.packets ?? [], rawHints: attention.raw.flatMap(hint => { + const decision = decisionsByHint.get(`packet/${hint.id}`); + return decision?.disposition === "resolved" ? [] : [{ ...hint, question: decision?.remainingQuestion ?? hint.question }]; + }) }); const humanAttention = selectHumanAttentionForOutput( - attention.groups, - capped.findings.filter((finding) => finding.publication !== "suppressed"), + regroupedAttention.groups, + [], // Finding overlap alone does not establish that every question was answered. packetsById, - verificationResolutions, + [], // Packet questions require explicit evidence-backed reconciliation, not verdict/word similarity. telemetry ); - for (const note of reconciledAttention.notes) { + const verifierQuestions = new Set(reconciledAttention.outcomes.filter(outcome => !outcome.candidateId.startsWith("packet:")).map(outcome => outcome.remainingQuestion)); + for (const note of reconciledAttention.notes.filter(note => verifierQuestions.has(note.question))) { if (!humanAttention.notes.some(existing => existing.question === note.question)) humanAttention.notes.push(note); } telemetry.event({ stage: 10, level: "info", message: "human_attention_reconciliation", data: { @@ -274,7 +276,24 @@ export async function dedupeRankAndComposeReview( const fallbackPresentation = presentationMeasurements.find(record => record.synthesis === "fallback" && record.findingIds.includes(finding.id)); publishSupportedSuggestions(finding, pretrim.kept, fallbackPresentation?.metrics.publishedSuggestionSources); } - const summary = publishableCount === 0 + // Explicitly answering every retained question can finish an evidence handoff. + // This never rescues a worker that failed to complete its required review. + if (coverage.diagnostics) coverage.diagnostics = coverage.diagnostics.filter(diagnostic => { + if (diagnostic.origin !== "tool" || !diagnostic.workItem) return true; + if (diagnostic.stage === 7 && !opts.packetResults?.some(packet => packet.packetId === diagnostic.workItem && packet.status === "completed")) return true; + const key = diagnostic.stage === 7 ? `packet:${diagnostic.workItem}` : diagnostic.workItem; + const groups = attentionReconciliation.groups.filter(group => group.candidateId === key); + const answered = groups.length > 0 && groups.every(group => !group.ineligibleReason && group.concerns.length > 0 && group.concerns.every(concern => decisionsByHint.get(concern.id)?.disposition === "resolved")); + if (answered) telemetry.event({ stage: 10, level: "info", message: "tool_evidence_limitation_reconciled", data: { diagnostic, concernIds: groups.flatMap(group => group.concerns.map(concern => concern.id)) } }); + return !answered; + }); + const unresolvedCount = humanAttention.notes.length + humanAttention.omittedCount; + if (humanAttention.notes.length > MAX_HUMAN_ATTENTION_NOTES) { + humanAttention.omittedCount += humanAttention.notes.length - MAX_HUMAN_ATTENTION_NOTES; + humanAttention.notes = humanAttention.notes.slice(0, MAX_HUMAN_ATTENTION_NOTES); + } + const health = deriveReviewHealth(coverage, unresolvedCount, verified.verified.length > 0); + const summary = health.status !== "completed" ? factualReviewSummary(health, publishableCount) : publishableCount === 0 ? coverage.partial ? "Review incomplete: completed work produced no credible verified findings, but incomplete coverage or verification prevents a clean conclusion." : fallbackSummary(0) @@ -284,6 +303,7 @@ export async function dedupeRankAndComposeReview( const createPostingPlan = opts.postGithubComments === true && (publishableCount > 0 || config.github.summaryWhenNoFindings); const result: ReviewResult = { summary, + health, coverage, findings, summaryOnlyFindings, @@ -294,7 +314,7 @@ export async function dedupeRankAndComposeReview( ? { postingPlan: { inline: findings.flatMap((finding) => (finding.anchor ? [{ findingId: finding.id, anchor: finding.anchor }] : [])), - reviewBody: renderReviewBody(summary, summaryOnlyFindings, humanAttention.notes, coverage, humanAttention.omittedCount) + reviewBody: renderReviewBody(summary, summaryOnlyFindings, humanAttention.notes, coverage, humanAttention.omittedCount, health) } } : {}) @@ -325,7 +345,7 @@ export async function dedupeRankAndComposeReview( })) }); await telemetry.writeArtifact("human-attention-notes.json", scrubGitHubSecrets( - { ...humanAttentionArtifact(attention, humanAttention, composerPromptSelection.groups), + { ...humanAttentionArtifact(attention, humanAttention, composerPromptSelection.groups, regroupedAttention), // These records deliberately share original scope/evidence objects. // Serialize to a tree before the scrubber's repeated-object protection // so shared provenance is not mistaken for circular data and redacted. @@ -1708,9 +1728,10 @@ function renderReviewBody( summaryOnly: FinalFinding[], notes: NeedsHumanAttentionNote[], coverage: RunCoverageStatus, - omittedNoteCount = 0 + omittedNoteCount = 0, + health?: import("../types.js").ReviewHealth ): string { - const trustBanner = renderCoverageTrustBanner(coverage); + const trustBanner = health && health.status !== "completed" ? renderReviewHealth(health) : renderCoverageTrustBanner(coverage); const lines = [ "### 🧞 Codegenie Review", "", @@ -2013,3 +2034,13 @@ function belowSeverity(actual: Severity, minimum: Severity | undefined): boolean function normalize(input: string): string { return input.toLowerCase().replace(/\s+/gu, " ").trim(); } + +/** Rebuild publication text if a budget stop occurred during composition itself. */ +export function refreshReviewHealth(result: ReviewResult): void { + if (result.health?.status === "failed" || result.health?.status === "incomplete") result.coverage.partial = true; + result.health = deriveReviewHealth(result.coverage, result.health?.unresolvedCount ?? result.needsHumanAttention.length + (result.needsHumanAttentionOmittedCount ?? 0), + result.findings.length + result.summaryOnlyFindings.length > 0); + if (result.health.status === "failed" || result.health.status === "incomplete") result.coverage.partial = true; + if (result.health.status !== "completed") result.summary = factualReviewSummary(result.health, result.findings.length + result.summaryOnlyFindings.length); + if (result.postingPlan) result.postingPlan.reviewBody = renderReviewBody(result.summary, result.summaryOnlyFindings, result.needsHumanAttention, result.coverage, result.needsHumanAttentionOmittedCount, result.health); +} diff --git a/src/pipeline/human-attention.ts b/src/pipeline/human-attention.ts index 5f34696..45dff3e 100644 --- a/src/pipeline/human-attention.ts +++ b/src/pipeline/human-attention.ts @@ -123,9 +123,9 @@ export type HumanAttentionOutput = { export function buildHumanAttentionNotes( packetResults: PacketReviewResult[], - options: { packets: ReviewPacket[]; diff?: UnifiedDiff; telemetry?: TelemetryRecorder } + options: { packets: ReviewPacket[]; diff?: UnifiedDiff; telemetry?: TelemetryRecorder; rawHints?: RawAttentionHint[] } ): HumanAttentionNotes { - const raw = rawAttentionHints(packetResults, knownAttentionPaths(options.packets, options.diff), options.telemetry); + const raw = options.rawHints ?? rawAttentionHints(packetResults, knownAttentionPaths(options.packets, options.diff), options.telemetry); const groups = new Map(); let eligibleHints = 0; @@ -189,6 +189,14 @@ export function buildHumanAttentionNotes( const exactGroups = [...groups.values()]; const merged = mergeNearDuplicateAttentionGroups(exactGroups); const ranked = merged.groups.sort(compareAttentionGroups); + if (options.rawHints) { + // Reconciliation works on original IDs. Grouping must not erase untouched + // members just because they share a file or symbol with the representative. + for (const group of ranked) { + const questions = [...new Set(raw.filter(hint => group.rawNoteIds.has(hint.id)).map(hint => hint.question))]; + group.representative = { ...group.representative, question: questions.join("; ") }; + } + } const selected = selectHumanAttentionGroups(ranked); const mergeStats: HumanAttentionMergeStats = { exactDuplicateHints: Math.max(0, eligibleHints - exactGroups.length), @@ -433,12 +441,14 @@ export function buildVerificationResolutionIndex( export function humanAttentionArtifact( attention: HumanAttentionNotes, output: HumanAttentionOutput, - composerPromptGroups: AttentionHintGroup[] + composerPromptGroups: AttentionHintGroup[], + reconciled?: HumanAttentionNotes ): Record { return { schemaVersion: 3, notes: attention.raw.map(rawAttentionHintArtifact), groups: attention.groups.map(attentionGroupArtifact), + ...(reconciled ? { reconciledGroups: reconciled.groups.map(attentionGroupArtifact) } : {}), mergeStats: attention.mergeStats, composerPromptGroupIds: composerPromptGroups.map((group) => group.key), outputGroupIds: output.selectedGroups.map((group) => group.key), diff --git a/src/pipeline/lens-runner.ts b/src/pipeline/lens-runner.ts index b767927..927825d 100644 --- a/src/pipeline/lens-runner.ts +++ b/src/pipeline/lens-runner.ts @@ -1,3 +1,4 @@ +import { reviewDiagnostic, unresolvedToolDiagnostic } from "../util/review-health.js"; import { clarifyFindingLocations } from "./finding-location.js"; import { SCHEMA_REPAIR_TIMEOUT_MS } from "../util/budget.js"; import { buildRepositoryToolDefinitions } from "../llm/tool-definitions.js"; @@ -155,6 +156,7 @@ export async function runLensPackets( findings: [], followUpHints: [], uncertainties: [], + diagnostics: [reviewDiagnostic(7, outcome.error, packetId, outcome.outcome)], status: outcome.outcome === "not_dispatched" || budgetSkipped ? "skipped" : "failed" }; }); @@ -438,6 +440,8 @@ function poolEnsemblePassResults( packetId: packet.id, lenses: packet.lenses, passesRun: passResults.reduce((sum, result) => sum + (result.passesRun ?? 1), 0), + ...(!passResults.some(result => result.status === "completed" && !result.diagnostics?.length) ? { diagnostics: passResults.flatMap(result => result.diagnostics ?? []) } : {}), + repositoryEvidence: source.filter(result => result.status === "completed").flatMap(result => result.repositoryEvidence ?? []), findings: pooled, ...(reviewStatus !== undefined ? { reviewStatus } : {}), ...(noFindingReason !== undefined ? { noFindingReason } : {}), @@ -493,7 +497,9 @@ async function runPacket( const repositoryTools = packet.reviewProfile === "simple" || packet.toolBudget.maxToolCalls <= 0 ? [] : buildRepositoryToolDefinitions(tools, { includeLikelyTests: shouldExposeLikelyTestsForPacket(packet) }); + let toolResults: import("../llm/llm-runner.js").LlmToolResultSummary[] = []; const submitted = await opts.runner.runStructured({ + onToolResults: results => { toolResults = results; }, stage: 7, prompt: prompt.prompt, schema: SubmitPacketReviewSchema, @@ -544,7 +550,11 @@ async function runPacket( } const followUpHints = normalizeFollowUpHints(submitted.followUpHints, packet, passSkillIds, telemetry, workerId); const uncertainties = normalizeUncertainties(submitted.uncertainties, packet, passSkillIds, telemetry, workerId); + const diagnostic = reviewStatus === "incomplete" || followUpHints.kept.length > 0 || uncertainties.kept.length > 0 + ? unresolvedToolDiagnostic(7, toolResults, packet.id) : undefined; const result: PacketReviewResult = { + repositoryEvidence: toolResults.flatMap(result => result.repositoryEvidence ? [result.repositoryEvidence] : []), + ...(diagnostic ? { diagnostics: [diagnostic] } : {}), packetId: packet.id, lenses: packet.lenses, findings, diff --git a/src/pipeline/review-runner.ts b/src/pipeline/review-runner.ts index e2d218c..734045e 100644 --- a/src/pipeline/review-runner.ts +++ b/src/pipeline/review-runner.ts @@ -1,3 +1,4 @@ +import { deriveReviewHealth, reviewDiagnostic } from "../util/review-health.js"; import path from "node:path"; import { mkdir, readFile, rm, stat, writeFile } from "node:fs/promises"; import { summarizeAdaptiveReviews } from "../util/adaptive-review.js"; @@ -55,7 +56,7 @@ import { applyCoverageEscalations } from "./coverage-escalation.js"; import { runTargetedSystemReviews, suppressResolvedFollowUpHints } from "./system-reviewer.js"; import { promoteUncertaintiesForVerification } from "./uncertainty-promotion.js"; import { verifyFindings } from "./verifier.js"; -import { dedupeRankAndComposeReview } from "./composer.js"; +import { dedupeRankAndComposeReview, refreshReviewHealth } from "./composer.js"; import { renderMarkdownReview } from "../output/markdown-renderer.js"; import { renderReviewForStdout, renderPostingSummaryForStdout } from "../output/stdout-renderer.js"; import { isDisclosableCoverageReason, uniqueDisclosableCoverageReasons } from "../util/coverage-reasons.js"; @@ -82,6 +83,7 @@ type RunReviewOverrides = { type RunContext = { runId: string; + readonly currentStage: ReviewStage | 0; telemetry: TelemetryRecorder; logger: ReturnType["logger"]; budget: BudgetLedger; @@ -114,6 +116,7 @@ export async function runReview( ): Promise { const repoRoot = await resolveRunRepoRoot(overrides.repoRoot); const run = await startRun(config, input, repoRoot, overrides); + let retainedReview: ReviewResult | undefined; try { await registerPullRequestRefCleanup(input, repoRoot, run); @@ -306,6 +309,7 @@ export async function runReview( } } }); + coverage.diagnostics = [...(coverage.diagnostics ?? []), ...(systemReview.diagnostics ?? [])]; discloseSkillLoadFailures(coverage, services.skills, services.skillFailures); const finalReview = await dedupeRankAndComposeReview(verified, plannerResult.plan, resolved, coverage, config, run.telemetry, { runner: services.runner, @@ -318,6 +322,8 @@ export async function runReview( if (run.budget.hasDispatchBlocks()) { markCoverageBudgetStopped(finalReview.coverage, run.budget.stopSnapshot()); } + refreshReviewHealth(finalReview); + retainedReview = finalReview; emitBudgetStop(run, finalReview.coverage.budgetStop); finalReview.budgetSummary = run.budget.summary(finalReview.coverage, buildContextPressureSummary(run.telemetry, packets, finalReview)); throwIfHardAborted(run); @@ -365,16 +371,18 @@ export async function runReview( finalReview.runStats = buildRunStats(config, resolved, run, plannerResult.plannerCoverage); await renderOutputs(finalReview, overrides, run.telemetry); await run.finalize({ - status: finalReview.coverage.partial ? "completed_partial" : "completed_full", - exitCode: 0, + status: finalReview.health?.status === "failed" ? "failed" : finalReview.coverage.partial ? "completed_partial" : "completed_full", + exitCode: finalReview.health?.status === "failed" ? 1 : 0, + ...(finalReview.health?.status === "failed" ? { errorCode: "review_failed" as const } : {}), ...(finalReview.coverage.budgetStop !== undefined ? { budgetStop: finalReview.coverage.budgetStop } : {}) }); return finalReview; } catch (error) { const failure = reviewFailureRecord(error); + const failureStage = run.currentStage; run.logger.error({ runId: run.runId, - stage: 0, + stage: failureStage, event: "review_pipeline_failed", // Carry the code and message inline so run.log states the cause without // a reader having to open the failure record beside it. @@ -384,25 +392,35 @@ export async function runReview( data: failure }); run.telemetry.event({ - stage: 0, + stage: failureStage, level: "error", message: "review pipeline failed", data: failure }); - await run.telemetry.writeArtifact("error.json", { - schemaVersion: 1, - runId: run.runId, - ...failure - }); + try { + await run.telemetry.writeArtifact("error.json", { schemaVersion: 1, runId: run.runId, stage: failureStage, ...failure }); + const coverage: RunCoverageStatus = { unavailable: true, totalHunks: 0, reviewedHunks: 0, skippedHunks: 0, failedHunks: 0, + coverageByLevel: { deep: 0, normal: 0, light: 0, skip: 0 }, degradedPlanning: false, budgetStopped: false, + verificationIncompleteCount: 0, partial: true, reasons: [], diagnostics: [reviewDiagnostic(failureStage, error)] }; + const failureReport: ReviewResult = retainedReview + ? { ...retainedReview, coverage: { ...retainedReview.coverage, partial: true, + diagnostics: [...(retainedReview.coverage.diagnostics ?? []), reviewDiagnostic(failureStage, error)] } } + : { summary: "Review terminated before a trustworthy report could be assembled.", coverage, + health: deriveReviewHealth(coverage, 0), findings: [], summaryOnlyFindings: [], needsHumanAttention: [], noFindings: true }; + refreshReviewHealth(failureReport); + await renderOutputs(failureReport, overrides, run.telemetry); + } catch (diagnosticError) { + run.logger.error({ runId: run.runId, stage: 0, event: "failure_artifact_write_failed", message: "Could not write failure report; original error retained", data: { diagnosticError: String(diagnosticError) } }); + } const budgetStop = run.budget.stopSnapshot(); emitBudgetStop(run, budgetStop); - await run.telemetry.flush(); - await run.finalize({ - status: "failed", - ...(isCodegenieError(error) ? { errorCode: error.code } : {}), - exitCode: errorExitCode(error), - ...(budgetStop !== undefined ? { budgetStop } : {}) - }); + try { + await run.telemetry.flush(); + await run.finalize({ status: "failed", ...(isCodegenieError(error) ? { errorCode: error.code } : {}), + exitCode: errorExitCode(error), ...(budgetStop !== undefined ? { budgetStop } : {}) }); + } catch (diagnosticError) { + run.logger.error({ runId: run.runId, stage: 0, event: "failure_finalize_failed", message: "Could not finalize diagnostics; original error retained", data: { diagnosticError: String(diagnosticError) } }); + } throw error; } } @@ -442,7 +460,11 @@ async function startRun( }); const attached = await run.attachRunDirectory(repoRoot); overrides.onRunStart?.(attached); - const telemetry = observeTelemetry(run.recorder, overrides.onTelemetryEvent); + let currentStage: ReviewStage | 0 = 0; + const telemetry = observeTelemetry(run.recorder, event => { + if (event.message === "stage_started" && event.stage !== 0) currentStage = event.stage; + overrides.onTelemetryEvent?.(event); + }); emitConfigWarnings(overrides.configWarnings ?? [], telemetry.runId, run.logger, telemetry); emitConcurrencyTuningEvent(config, telemetry); const budget = new BudgetLedger(config, telemetry); @@ -456,6 +478,7 @@ async function startRun( let finalized = false; return { runId: run.recorder.runId, + get currentStage() { return currentStage; }, telemetry, logger: run.logger, budget, @@ -505,6 +528,9 @@ function observeTelemetry( if (recorder.snapshotContextPressure !== undefined) { observed.snapshotContextPressure = recorder.snapshotContextPressure.bind(recorder); } + if (recorder.snapshotStageTimings !== undefined) { + observed.snapshotStageTimings = recorder.snapshotStageTimings.bind(recorder); + } return observed; } @@ -902,6 +928,7 @@ async function maybeZeroWork( totalHunks, reviewedHunks: 0, skippedHunks: totalHunks, + excludedHunks: totalHunks, failedHunks: 0, coverageByLevel: { deep: 0, normal: 0, light: 0, skip: totalHunks }, degradedPlanning: false, @@ -948,7 +975,7 @@ export function aggregateRunCoverage( plan: ReviewPlan, decisions: FileFilterDecision[], packetResults: PacketReviewResult[], - verified: { incompleteCount: number; verificationSkipped?: boolean }, + verified: { incompleteCount: number; verificationSkipped?: boolean; verdicts?: import("../types.js").VerificationVerdict[] }, _telemetry: TelemetryRecorder, opts: CoverageOptions = {} ): RunCoverageStatus { @@ -1016,9 +1043,12 @@ export function aggregateRunCoverage( return { totalHunks, + diagnostics: [...packetResults.flatMap(result => result.diagnostics ?? (result.status === "failed" ? [reviewDiagnostic(7, undefined, result.packetId)] : [])), + ...(verified.verdicts ?? []).flatMap(verdict => verdict.diagnostic ? [verdict.diagnostic] : [])], ...(adaptiveReviews ? { adaptiveReviews } : {}), reviewedHunks, skippedHunks, + excludedHunks: skippedByFilter + (plan.partialReview?.isPartial ? 0 : plan.coverage.filter(decision => decision.coverage === "skip").length), failedHunks, coverageByLevel, degradedPlanning: opts.degradedPlanning === true, @@ -1277,6 +1307,7 @@ async function renderOutputs( ): Promise { const markdown = scrubGitHubSecrets(renderMarkdownReview(result)); await telemetry.writeArtifact("final-review.md", markdown); + await telemetry.writeArtifact("final-review.json", scrubGitHubSecrets(result)); const rendered = overrides.postGithubComments ? renderPostingSummaryForStdout(result, overrides.format ?? "markdown", { postRequested: true }) : renderReviewForStdout(result, overrides.format ?? "markdown"); @@ -1507,7 +1538,13 @@ export class BudgetLedger { summary(coverage?: RunCoverageStatus, contextPressure?: ContextPressureSummary): BudgetSummary { return { completeness: coverage?.partial === true ? "partial" : "complete", - partialReasons: coverage?.partial === true ? [...coverage.reasons] : [], + partialReasons: coverage?.partial === true ? [ + ...(coverage.diagnostics ?? []).map(diagnostic => `Stage ${diagnostic.stage}: ${diagnostic.reason}`), + ...(coverage.failedHunks ? [`${coverage.failedHunks} hunks failed review`] : []), + ...(coverage.verificationIncompleteCount ? [`${coverage.verificationIncompleteCount} verifications incomplete`] : []), + ...(coverage.budgetStopped ? ["Review budget exhausted"] : []), + ...(coverage.degradedPlanning ? ["Planning incomplete"] : []) + ] : [], multiplier: this.config.review.budgetBoost, configured: { timeoutMs: this.config.review.maxTimeMs, diff --git a/src/pipeline/system-reviewer.ts b/src/pipeline/system-reviewer.ts index 6577dee..56d7d0a 100644 --- a/src/pipeline/system-reviewer.ts +++ b/src/pipeline/system-reviewer.ts @@ -1,3 +1,4 @@ +import { reviewDiagnostic } from "../util/review-health.js"; import { SCHEMA_REPAIR_TIMEOUT_MS } from "../util/budget.js"; import { buildRepositoryToolDefinitions } from "../llm/tool-definitions.js"; import type { LlmRunner } from "../llm/llm-runner.js"; @@ -158,11 +159,13 @@ export async function runTargetedSystemReviews( }); return []; }); + const diagnostics = outcomes.filter(outcome => outcome.outcome !== "completed").map(outcome => reviewDiagnostic(8, outcome.error, outcome.task.packetId, outcome.outcome)); const packetResults = completed.map((result) => result.packetResult); const resolvedHints = completed.flatMap((result) => result.resolvedHints); await telemetry.writeArtifact("system-review-results.json", { packetResults, - resolvedHints + resolvedHints, + diagnostics }); telemetry.event({ stage: 8, @@ -192,7 +195,7 @@ export async function runTargetedSystemReviews( resolvedHints: resolvedHints.length } }); - return { tasks, packetResults, resolvedHints }; + return { tasks, packetResults, resolvedHints, diagnostics }; } export function suppressResolvedFollowUpHints( diff --git a/src/pipeline/verifier.ts b/src/pipeline/verifier.ts index e02060d..0786d41 100644 --- a/src/pipeline/verifier.ts +++ b/src/pipeline/verifier.ts @@ -1,3 +1,4 @@ +import { reviewDiagnostic, unresolvedToolDiagnostic } from "../util/review-health.js"; import { assessFinalSuggestions, suggestionAssessment } from "./suggestion-assessment.js"; import { SCHEMA_REPAIR_TIMEOUT_MS } from "../util/budget.js"; import { normalizeVerifierSubmission, expandVerifierRevision, promotedCompletionIssues } from "../llm/verifier-revision.js"; @@ -54,6 +55,7 @@ const VERIFIER_BASE_TOKEN_ESTIMATE = 1_000; const EVIDENCE_RESOLUTION_LANE_MAX = 4; type VerifyOptions = { + onToolResults?: import("../llm/llm-runner.js").LlmStructuredRequest["onToolResults"]; runner: LlmRunner; promptBuilder: PromptBuilder; lensRegistry: LensRegistry; @@ -373,6 +375,7 @@ export async function verifyFindings( } const candidateId = outcome.task.candidateId ?? "unknown"; const verdict = incompleteVerificationVerdict(candidateId, verifierOutcomeReason(outcome)); + verdict.diagnostic = reviewDiagnostic(9, outcome.error, candidateId, outcome.outcome); verdicts.push(verdict); const meta = { ...candidateRecordMeta(candidateId, verificationLaneByCandidateId, gateReasonByCandidateId, gateFactsByCandidateId), @@ -604,7 +607,8 @@ async function verifyCandidate( ...(packet?.intentSignals !== undefined ? { intentSignals: packet.intentSignals } : {}), skills }); - const submitted = await runVerifierStructured(candidate, prompt, tools, config, opts, workerId, telemetry, runtimeStats); + let toolResults: import("../llm/llm-runner.js").LlmToolResultSummary[] = []; + const submitted = await runVerifierStructured(candidate, prompt, tools, config, { ...opts, onToolResults: results => { toolResults = results; } }, workerId, telemetry, runtimeStats); const normalized = normalizeSubmittedVerdict(candidate, submitted, telemetry); const submittedFinalFinding = normalized.finalFinding; const revised = submittedFinalFinding !== undefined @@ -626,10 +630,15 @@ async function verifyCandidate( falsePositiveRisk: normalized.falsePositiveRisk, ...(revised !== undefined ? { finalFinding: revised } : {}), ...(revisedAnchor !== undefined ? { revisedAnchor } : {}), - ...(verificationIncomplete ? { verificationIncomplete: true } : {}), + ...(verificationIncomplete ? { verificationIncomplete: true, + diagnostic: reviewDiagnostic(9, new CodegenieError(normalized.reason.includes("budget") ? "budget_exhausted" : "llm_schema_invalid", normalized.reason), candidate.id) } : {}), ...(normalized.behaviorChange !== undefined ? { behaviorChange: normalized.behaviorChange } : {}), ...(normalized.intentEvidence !== undefined ? { intentEvidence: normalized.intentEvidence } : {}) }; + if (verdict.unresolvedConcern && !verdict.diagnostic) { + const diagnostic = unresolvedToolDiagnostic(9, toolResults, candidate.id); + if (diagnostic) verdict.diagnostic = diagnostic; + } telemetry.event({ stage: 9, level: "info", message: "verification_suggestion_assessments", data: { candidateId: candidate.id, suppliedAssessments: normalized.suggestionAssessments ?? {}, assessments: verdict.suggestionAssessments } }); for (const field of ["suggestedFix", "suggestedTest"] as const) { @@ -824,6 +833,7 @@ async function runVerifierStructured( try { const result = await opts.runner.runStructured({ stage: 9, + ...(opts.onToolResults ? { onToolResults: opts.onToolResults } : {}), prompt: prompt.prompt, schema: { ...SubmitVerificationVerdictSchema, required: [...SubmitVerificationVerdictSchema.required, "proofAssessment"] }, diff --git a/src/repo/path-guard.ts b/src/repo/path-guard.ts index 76683d0..45d08c5 100644 --- a/src/repo/path-guard.ts +++ b/src/repo/path-guard.ts @@ -1,3 +1,4 @@ +import picomatch from "picomatch"; import type { ReviewStage } from "../types.js"; import type { TelemetryRecorder } from "../telemetry/telemetry-recorder.js"; import { CodegenieError, type CodegenieErrorCode } from "../util/errors.js"; @@ -104,3 +105,12 @@ function throwViolation( function truncateForTelemetry(value: string): string { return value.length <= 200 ? value : `${value.slice(0, 200)}...`; } + +/** One dialect for tracked-file discovery and all model-facing search scopes. */ +export function matchRepositoryGlob(pattern: string): (path: string) => boolean { + try { + return picomatch(pattern, { dot: true, strictBrackets: true }); + } catch (error) { + throw new CodegenieError("invalid_args", `pathGlob is invalid: ${String(error).slice(0, 200)}. Use a repo-relative glob such as src/{api,data}/**; literal brackets can be expressed as character classes.`, { cause: error }); + } +} diff --git a/src/repo/repository-index.ts b/src/repo/repository-index.ts index 88d44b5..7882a37 100644 --- a/src/repo/repository-index.ts +++ b/src/repo/repository-index.ts @@ -722,6 +722,8 @@ export class RepositoryToolsFacade implements RepositoryToolsHost { ...(measurement.meta.degradationReason !== undefined ? { degradationReason: measurement.meta.degradationReason } : {}), ...(measurement.meta.truncated !== undefined ? { truncated: measurement.meta.truncated } : {}), ...(measurement.meta.omittedCount !== undefined ? { omittedCount: measurement.meta.omittedCount } : {}), + ...(measurement.meta.discoveryLimited ? { discoveryLimited: true } : {}), + ...(measurement.meta.omittedCountIsLowerBound ? { omittedCountIsLowerBound: true } : {}), ...(measurement.meta.lookupStatus !== undefined ? { lookupStatus: measurement.meta.lookupStatus } : {}), ...(measurement.meta.deliveryStatus !== undefined ? { deliveryStatus: measurement.meta.deliveryStatus } : {}), ...(measurement.meta.recovery !== undefined ? { recovery: measurement.meta.recovery } : {}), @@ -811,6 +813,8 @@ function metaFromSearch(execution: { degradationReason?: string; truncated?: boolean; omittedCount?: number; + omittedCountIsLowerBound?: boolean; + discoveryLimited?: boolean; }): ToolResultMeta { return { backend: execution.backend, @@ -818,7 +822,9 @@ function metaFromSearch(execution: { degraded: execution.degraded, ...(execution.degradationReason !== undefined ? { degradationReason: execution.degradationReason } : {}), ...(execution.truncated !== undefined ? { truncated: execution.truncated } : {}), - ...(execution.omittedCount !== undefined ? { omittedCount: execution.omittedCount } : {}) + ...(execution.omittedCount !== undefined ? { omittedCount: execution.omittedCount } : {}), + ...(execution.omittedCountIsLowerBound ? { omittedCountIsLowerBound: true } : {}), + ...(execution.discoveryLimited ? { discoveryLimited: true } : {}) }; } diff --git a/src/repo/search.ts b/src/repo/search.ts index 576eb1c..2bb449d 100644 --- a/src/repo/search.ts +++ b/src/repo/search.ts @@ -15,6 +15,8 @@ export type SearchExecution = { degradationReason?: string; truncated?: boolean; omittedCount?: number; + discoveryLimited?: boolean; + omittedCountIsLowerBound?: boolean; }; type RawSearchOptions = SearchOptions & { @@ -22,6 +24,7 @@ type RawSearchOptions = SearchOptions & { word?: boolean; defaultMaxResults?: number; hardMaxResults?: number; + mention?: string; }; const DEFAULT_MAX_RESULTS = 50; @@ -50,39 +53,40 @@ export class SearchService { ...(pathGlob !== undefined ? { pathGlob } : {}) }); - const countOmitted = raw.length > maxResults ? raw.length - maxResults : 0; - const lineCapped = capMatchTexts(raw.slice(0, maxResults)); + const omittedLines = raw.omittedLines ?? 0; + if (!raw.length && omittedLines) throw new CodegenieError("budget_exhausted", "All matching lines exceeded the search output allowance; narrow pathGlob or read a known range. This is not a zero-match result."); + const discoveryLimited = raw.length > maxResults || omittedLines > 0; + // The extra text match may be a comment, not a verified identifier mention. + const countOmitted = options.mention === undefined ? omittedLines + Number(raw.length > maxResults) : 0; + const classified = options.mention === undefined ? undefined : await this.classifyMentions(raw.slice(0, maxResults), options.mention, source); + const lineCapped = capMatchTexts(classified?.results ?? raw.slice(0, maxResults)); await this.enrich(lineCapped.results, source, options.contextMode ?? "none"); - const capped = capSearchResultsTotal(lineCapped.results, countOmitted + lineCapped.truncatedTextCount); + const capped = capSearchResultsTotal(lineCapped.results, countOmitted); + if (lineCapped.results.length > 0 && capped.results.length === 0) throw new CodegenieError("budget_exhausted", "No complete search entry fits the result allowance; narrow pathGlob or read a known range. This is not a zero-match result."); + const shortened = lineCapped.truncatedTextCount > 0 || capped.shortened; + const degraded = discoveryLimited || capped.omittedCount > 0 || shortened || (classified?.syntaxFallbacks ?? 0) > 0; return { - results: capped.results, - engine: "git-grep", - backend: "text", - precision: "text", - degraded: capped.omittedCount > 0, - ...(capped.omittedCount > 0 ? { degradationReason: "search results truncated" } : {}), - ...(capped.omittedCount > 0 ? { truncated: true, omittedCount: capped.omittedCount } : {}) + results: capped.results, engine: "git-grep", + backend: classified?.syntaxOnly ? "tree-sitter" : "text", + precision: classified?.syntaxOnly ? "syntactic" : "text", + degraded, + ...(degraded ? { degradationReason: classified?.syntaxFallbacks ? `${classified.syntaxFallbacks} mention result(s) were not syntax-verified` : "search bounded: excerpts shortened or results omitted" } : {}), + ...(discoveryLimited || capped.omittedCount > 0 || shortened ? { truncated: true, omittedCount: capped.omittedCount, omittedCountIsLowerBound: true, discoveryLimited } : {}) }; } - async findSymbolMentions( - symbolName: string, - options: SymbolMentionOptions = {} - ): Promise { - const execution = await this.search(symbolName, { - ...options, - fixedString: true, - word: true, - defaultMaxResults: 100, - hardMaxResults: 300 - }); + async findSymbolMentions(symbolName: string, options: SymbolMentionOptions = {}): Promise { + return this.search(symbolName, { ...options, fixedString: true, word: true, defaultMaxResults: 100, hardMaxResults: 300, mention: symbolName }); + } + + private async classifyMentions(results: SearchResult[], symbolName: string, source: SourceSelector) { const attemptedFiles = new Set(); let unverified = 0; // Generic text matches affect precision, but are not a failed syntax lookup. let syntaxFallbacks = 0; const kept: SearchResult[] = []; - for (const result of execution.results) { + for (const result of results) { const alreadyAttempted = attemptedFiles.has(result.path); if (attemptedFiles.size >= 25 && !alreadyAttempted) { unverified += 1; @@ -93,7 +97,7 @@ export class SearchService { continue; } attemptedFiles.add(result.path); - const verified = await this.verifyIdentifierMention(result, symbolName, options.source ?? { kind: "head" }); + const verified = await this.verifyIdentifierMention(result, symbolName, source); if (verified === true) { kept.push(result); } else if (verified === undefined) { @@ -106,36 +110,18 @@ export class SearchService { } const syntaxOnly = kept.length > 0 && unverified === 0; - return { - ...execution, - results: kept, - backend: syntaxOnly ? "tree-sitter" : "text", - precision: syntaxOnly ? "syntactic" : "text", - degraded: execution.degraded || syntaxFallbacks > 0, - ...(syntaxFallbacks > 0 - ? { degradationReason: `${syntaxFallbacks} mention result(s) were not syntax-verified` } - : execution.degradationReason !== undefined - ? { degradationReason: execution.degradationReason } - : {}) - }; + return { results: kept, syntaxOnly, syntaxFallbacks }; } - private async gitGrep(query: string, options: RawSearchOptions & { pathGlob?: string; source: SourceSelector; maxResults: number }): Promise { - try { - return await this.limit(() => this.resolver.grep(query, { - source: options.source, - maxResults: options.maxResults, - ...(options.pathGlob !== undefined ? { glob: options.pathGlob } : {}), - ...(options.caseSensitive !== undefined ? { caseSensitive: options.caseSensitive } : {}), - ...(options.fixedString !== undefined ? { fixedString: options.fixedString } : {}), - ...(options.word !== undefined ? { word: options.word } : {}) - })); - } catch (error) { - if (error instanceof CodegenieError && error.code === "git_ref_missing") { - throw new CodegenieError("invalid_args", "search pattern or revision could not be searched", { cause: error }); - } - throw error; - } + private async gitGrep(query: string, options: RawSearchOptions & { pathGlob?: string; source: SourceSelector; maxResults: number }): Promise { + return this.limit(() => this.resolver.grep(query, { + source: options.source, + maxResults: options.maxResults, + ...(options.pathGlob !== undefined ? { glob: options.pathGlob } : {}), + ...(options.caseSensitive !== undefined ? { caseSensitive: options.caseSensitive } : {}), + ...(options.fixedString !== undefined ? { fixedString: options.fixedString } : {}), + ...(options.word !== undefined ? { word: options.word } : {}) + })); } private async enrich(results: SearchResult[], source: SourceSelector, mode: SearchOptions["contextMode"]): Promise { @@ -178,7 +164,8 @@ export class SearchService { }); const symbol = adapter.getEnclosingSymbol(parsed, match.line); if (symbol) { - match.enclosingSymbol = symbol; + const { name, kind, path, lineRange } = symbol; + match.enclosingSymbol = { name, kind, path, lineRange }; } } } @@ -234,20 +221,32 @@ function capMatchTexts(results: SearchResult[]): { results: SearchResult[]; trun return result; } truncatedTextCount += 1; - return { ...result, matchText: `${result.matchText.slice(0, MAX_MATCH_TEXT_CHARS)}...` }; + const matchOffset = Buffer.from(result.matchText).subarray(0, (result.column ?? 1) - 1).toString("utf8").length; + const offset = Math.max(0, matchOffset - 120); + return { ...result, matchText: result.matchText.slice(offset, offset + MAX_MATCH_TEXT_CHARS), excerptStartColumn: Buffer.byteLength(result.matchText.slice(0, offset)) + 1, excerpt: true }; }), truncatedTextCount }; } -function capSearchResultsTotal(results: SearchResult[], initialOmittedCount: number): { results: SearchResult[]; omittedCount: number } { - const capped = [...results]; +function capSearchResultsTotal(results: SearchResult[], initialOmittedCount: number): { results: SearchResult[]; omittedCount: number; shortened: boolean } { + const capped = results.map(result => ({ ...result })); + // Optional context must never evict core match locations. + const shortened = JSON.stringify(capped).length > MAX_TOTAL_RESULT_CHARS; + if (shortened) { + for (const result of capped) { + delete result.contextBefore; + delete result.contextAfter; + delete result.enclosingSymbol; + } + } let omittedCount = initialOmittedCount; - while (JSON.stringify(capped).length > MAX_TOTAL_RESULT_CHARS && capped.length > 0) { - capped.pop(); - omittedCount += 1; + const packed: SearchResult[] = []; + for (const result of capped) { + if (JSON.stringify([...packed, result]).length <= MAX_TOTAL_RESULT_CHARS) packed.push(result); + else omittedCount++; } - return { results: capped, omittedCount }; + return { results: packed, omittedCount, shortened }; } function groupByPath(results: SearchResult[]): Map { diff --git a/src/repo/source-resolver.ts b/src/repo/source-resolver.ts index ddbfae2..c6be132 100644 --- a/src/repo/source-resolver.ts +++ b/src/repo/source-resolver.ts @@ -1,4 +1,4 @@ -import picomatch from "picomatch"; +import { matchRepositoryGlob } from "./path-guard.js"; import type { SearchResult, SourceSelector, ResolvedReviewInput } from "../types.js"; import { createGitClient, type InternalGitClient } from "../git/git-client.js"; import { CodegenieError } from "../util/errors.js"; @@ -62,7 +62,7 @@ export class SourceResolver { if (containedGlob === undefined) { return paths; } - const isMatch = picomatch(containedGlob, { dot: true }); + const isMatch = matchRepositoryGlob(containedGlob); return paths.filter((filePath) => isMatch(filePath)); } @@ -76,11 +76,11 @@ export class SourceResolver { fixedString?: boolean; word?: boolean; } = {} - ): Promise { + ): Promise { const commit = this.resolveSource(opts.source); const containedGlob = opts.glob === undefined ? undefined : containGlob(this.repoRoot, opts.glob); return this.git.grep(commit, pattern, { - ...(containedGlob !== undefined ? { glob: containedGlob } : {}), + ...(containedGlob !== undefined ? { paths: await this.listFiles(containedGlob, opts.source) } : {}), ...(opts.maxResults !== undefined ? { maxResults: opts.maxResults } : {}), ...(opts.caseSensitive !== undefined ? { caseSensitive: opts.caseSensitive } : {}), ...(opts.fixedString !== undefined ? { fixedString: opts.fixedString } : {}), diff --git a/src/skills/prompt-builder.ts b/src/skills/prompt-builder.ts index 923d366..4b13747 100644 --- a/src/skills/prompt-builder.ts +++ b/src/skills/prompt-builder.ts @@ -75,12 +75,14 @@ export type PromptBuilder = { export const PROMPT_TEMPLATE_VERSIONS: Record<5 | 7 | 8 | 9 | 10, string> = { 5: "p5.7", - 7: "p7.14", + 7: "p7.15", 8: "p8.2", - 9: "p9.25", - 10: "p10.15" + 9: "p9.26", + 10: "p10.17" }; +export const TEST_COVERAGE_GUIDANCE = "A missing test for a new branch alone is not a finding. Establish a material behavioral requirement from callers, boundary tests or an explicit contract, identify a concrete violating regression, and show why inspected relevant tests would accept it. A currently correct production guard may still need an important rejection test; no existing production bug or executed mutation is required. Inspect likely sister tests and transport validation where relevant. Scope absence claims to inspected evidence: a bounded or unsuccessful search cannot prove repository-wide absence. Neither neighboring test style, a commit title, nor an unsupported client-visible label establishes material consequence. Omit optional extra coverage rather than turning it into human-attention noise. A proposed regression test must exercise a reachable boundary, reject weakened requirements, and accept valid remedies."; + const DOCUMENTATION_IMPACT_GUIDANCE = "For documentation findings, identify the specific instruction or decision the reader would get wrong and the concrete consequence supported by inspected evidence. Read the surrounding example and corrective instructions before treating an isolated wording or arithmetic inconsistency as a defect. A possible future misunderstanding or hypothetical monitoring/implementation mistake alone does not establish material impact. Do not promote wording-only corrections as findings; retain documentation findings that demonstrably prescribe an unsafe action or violate an established requirement."; type PromptLedgerEntry = { @@ -292,6 +294,7 @@ export function createPromptBuilder(_registry: LensRegistry, options: ProjectSki injectionInstruction(), "Review the packet for real defects only. Use repository tools when needed to verify nearby code, definitions, or tests. Return no findings when there is no concrete failure mode.", DOCUMENTATION_IMPACT_GUIDANCE, + TEST_COVERAGE_GUIDANCE, "A finding may point to another changed file or hunk in this PR when investigation supports it. Use exact file, side, line, and hunk coordinates from inspected diff evidence; do not move a finding onto the assigned packet merely for placement. Omit an anchor you cannot establish.", "Raise candidate findings for concrete changed-line failure modes. If the evidence shows a plausible changed-line correctness, security, performance, architecture, or testing risk but one narrow predicate still needs confirmation, surface it as a candidate finding or a pointer-rich followUpHint/uncertainty for the verifier instead of suppressing it.", "A later verification stage filters false positives. Do not publish speculation as a finding, but do not hide a plausible verifier-resolvable concern behind reviewStatus:\"no_findings\". No-findings is appropriate only after the changed-line risk has been checked and no concrete failure mode or pointer-rich unresolved predicate remains.", @@ -354,6 +357,7 @@ export function createPromptBuilder(_registry: LensRegistry, options: ProjectSki "Assess the defect independently from its remedy. Before endorsing advice, ask whether it removes the symptom by weakening the caller's original requirement. Internal consistency alone is insufficient: fixing a permission failure by granting access to everyone violates the authorization contract. Use supplied evidence or prioritize a focused caller/spec check over repeated local confirmation within the existing budget. Unresolved compatibility means unverified advice, not rejection of a proven defect.", "As a final verification decision, assess each final suggestion when inspected evidence permits. Use suggestionAssessments.suggestedFix and/or suggestedTest with status, rationale, contractCheck and source evidence; each assessment refers to that named field in the final finding after findingUpdates. Omit suggestionText to bind it automatically to that exact final text; if supplied, copy it verbatim without punctuation or wording edits. Assessment text never revises a suggestion: use findingUpdates for changes. Assess fix and test independently. Missing assessments mean unverified advice, not a failed defect. If evidence is insufficient, mark unverified or omit the assessment; never invent support.", DOCUMENTATION_IMPACT_GUIDANCE, + TEST_COVERAGE_GUIDANCE, "Provide proofAssessment with status established, refuted, or unresolved; cite concrete source evidence and list unresolved assumptions with essential=true only when the defect itself depends on them. Changed behavior, a commit-title mismatch, or a newly rejected input alone does not prove a violated requirement. If a plausible answer to an open question eliminates the defect, that assumption is essential. Keep/revise requires established proof and no unresolved essential assumption. Return reject with status unresolved when proof is missing; the harness retains those questions for human attention. Only uncertainty that cannot overturn the defect is secondary. When source observations disagree, resolve them from inspected code or retain the disagreement explicitly; do not present incompatible statements as settled facts. Distinguish an observed interface guarantee violation from an inferred execution consequence: follow already-available caller/precondition evidence, but do not assert downstream failure without support or dismiss a hard precondition violation solely because its numerical shortfall is small.", "Categorical absence claims (no tests, guards, validation, or callers) require a sufficiently complete search of the relevant scope. A zero-hit wording search, narrow excerpt, or truncated result alone is not proof. If unseen scope could refute the defect, the gap is essential: use a focused read or reject as unresolved. Narrowing to a selected subset does not establish a gap when uninspected scope could supply the required protection. Do not label decisive missing evidence secondary because budget ran out; no exhaustive repository search is required when bounded authoritative evidence settles the claim.", "After narrowing a finding, review the entire assembled result: title, failureMode, whyThisMatters, verification and suggestions must describe the same remaining claim. Include compact updates for every dependent field that became false or stale; independent fields need not be repeated. Saying in reason that a claim was removed does not remove it from the finding text.", @@ -400,7 +404,7 @@ export function createPromptBuilder(_registry: LensRegistry, options: ProjectSki "Sources for fixes/tests carry suggestionAssessment. Supported applies only to that exact proposal and cited caller contract. Check it against all supplied relevant evidence: the label cannot override a contradiction. Distinguish counterevidence from missing verification: an unverified alternative, an uninspected check, or an assessment that omits the caller requirement does not by itself contradict an independently supported proposal. Compare the exact proposal, requirement and inspected predicates; do not call differently worded alternatives identical. Select a supported remedy when no supplied evidence refutes its compatibility, retaining unverified alternatives in provenance. When withholding for a real conflict, identify the conflicting observation and its supporting source in verification prose, rather than treating different assessment labels as a veto. If compatibility is unresolved, withhold the advice, retain its sources in provenance, and explain the conflict using valid verification sources; a caveat cannot authorize disputed advice. Do not mutate verifier assessments or invent a remedy. Fix/test sections may reference only effectively supported current suggestions. Put unverified, incompatible, stale, conflicting, and historical proposals in retainedSourceRefs, never in prominent advice, even with a qualification. If none are supported, omit that section; the renderer supplies a concise status note. Do not invent advice, strengthen a supported proposal, or combine proposals into an unassessed remedy. Preserve all original proposals and assessments in provenance. Do not resolve conflicting remedies by vote, and do not suggest a test that pins behavior a caller rejects.", "Groups are starting clusters, not mandatory final boundaries. Combine findingIds across groups when trigger, mechanism, violated requirement and corrective action describe one causal defect, even at different helper/caller lines or inline/summary-only locations. Keep distinct triggers/contracts or independently actionable corrections separate; sharing a helper is not enough. A regression test for the same production bug is not by itself a second testing defect. Preserve all identities and evidence when merging.", "Check the supplied assembled findings for internal contradictions (including stale impact after a narrower revision). Explanations of guards must agree with their branch conditions, and separate assertions must not be described as a cross-value invariant. Preserve the distinction between an observed guarantee violation and an inferred execution consequence. Fix/test wording must agree: explicitly scope a remedy-specific test to its assessed alternative rather than imply it accepts every recommended remedy. Select or withhold supplied assessed advice; never invent a replacement test or remedy to repair a contradiction. Write the diagnosis-only summary from the composed conclusions: state each distinct conclusion once, with its remaining limitations. When supplied caller evidence resolves an earlier disagreement, reflect that conclusion in the summary and retain the old disagreement in provenance; do not still call it an open question. When evidence conflicts or is insufficient, preserve uncertainty in both summary and findings. A newer statement or supported label does not settle a conflict. Limit test-existence and absence claims to the inspected revision, configuration and scope; uninspected or truncated scope cannot establish repository-wide absence. This is synthesis of verified inputs, not a new investigation.", - "If an attention-reconciliation block is supplied, compare each concern with independent supplied evidence before carrying the question into the report. Return attentionResolutions for answered questions or answered parts, using exact concern IDs and only inventory evidence IDs. Shared verdict, proofStatus and assumptions are in evidenceContexts, keyed by candidateId; apply those qualifications to every corresponding evidence entry. Entries with sourceRef reuse that source component in the findings input instead of repeating its text; sourceField=suggestionAssessment selects its assessment, including evidence and qualifications. Other entries carry their observation inline. Assessment evidence may answer a question even when the proposal is unverified or incompatible, but a proposal or assessment label alone is not proof: use the inspected observations and preserve their scope, contradictions and uncertainty. This inventory is for human-attention presentation, not finding source accounting or new findings. Rejected candidates can contain useful observations; their verdict alone answers nothing. Match the exact question, revision, configuration and scope: test existence is not deployment, and same-change intent is not an external contract. reviewRevision is review context, not proof of the revision observed in an excerpt; preserve uncertainty when source scope is unspecified. Resolve only when evidence answers the entire question. For narrowed, provide remainingQuestion preserving every unanswered condition and explain which part was answered in rationale. Do not use the concern's own candidate as supporting evidence, shared vocabulary as proof, or chronology/majority to resolve contradictions. Incomplete evidence for the whole question can still justify narrowing: remove only the independently answered part and keep all unanswered deployment, configuration, caller-contract or other conditions in remainingQuestion. If no part can be answered without ambiguity or conflict, omit the resolution. Unlisted concerns remain unchanged. Resolutions never change finding verdicts, essential assumptions or review completeness.", + "If an attention-reconciliation block is supplied, compare each concern with independent supplied evidence before carrying the question into the report. Packet concerns use stable packet/ hint IDs; each original free-text question is indivisible unless explicitly narrowed with all remaining conditions retained. Return attentionResolutions for answered questions or answered parts, using exact concern IDs and only inventory evidence IDs. Shared verdict, proofStatus and assumptions are in evidenceContexts, keyed by candidateId; apply those qualifications to every corresponding evidence entry. Entries with sourceRef reuse that source component in the findings input instead of repeating its text; sourceField=suggestionAssessment selects its assessment, including evidence and qualifications. Other entries carry their observation inline. Assessment evidence may answer a question even when the proposal is unverified or incompatible, but a proposal or assessment label alone is not proof: use the inspected observations and preserve their scope, contradictions and uncertainty. This inventory is for human-attention presentation, not finding source accounting or new findings. Rejected candidates can contain useful observations; their verdict alone answers nothing. Match the exact question, revision, configuration and scope: test existence is not deployment, and same-change intent is not an external contract. reviewRevision is review context, not proof of the revision observed in an excerpt; preserve uncertainty when source scope is unspecified. Resolve only when evidence answers the entire question. For narrowed, provide remainingQuestion preserving every unanswered condition and explain which part was answered in rationale. Do not use the concern's own candidate as supporting evidence, shared vocabulary as proof, or chronology/majority to resolve contradictions. Incomplete evidence for the whole question can still justify narrowing: remove only the independently answered part and keep all unanswered deployment, configuration, caller-contract or other conditions in remainingQuestion. If no part can be answered without ambiguity or conflict, omit the resolution. Unlisted concerns remain unchanged. Resolutions never change finding verdicts, essential assumptions or execution completeness; they can change whether unresolved questions remain.", "Combine equivalent secondary uncertainties by meaning in one current verification conclusion and explain their remaining consequences. Cite proof sources with unresolved assumptions and affected sources of unresolved reconciliations in that visible verification section, not only retainedSourceRefs, unless a valid supported supersession discharges a nonessential source. Essential uncertainty cannot be superseded. Historic severity votes stay in provenance; explain current material severity uncertainty once. Do not rely on the renderer to append secondary caveats for you.", "For identical suggestion text, conflicting supported/incompatible assessments cannot be bypassed by selecting the favorable source. Withhold the disputed proposal and describe the existence of the conflict in verification without restating the advice; retain incompatible sources only as provenance. A missing/unverified assessment alone is not contradictory evidence. Preserve conditions when summarizing supported remedies.", "Final finding titles must be concrete issue statements. Do not preserve task-shaped titles that start with Verify, Check, Confirm, Investigate, Does, Can, Could, or Should, or titles phrased as questions; use the verified behavior delta or failure mode instead.", diff --git a/src/telemetry/run-artifacts.ts b/src/telemetry/run-artifacts.ts index 6377de4..0e459a0 100644 --- a/src/telemetry/run-artifacts.ts +++ b/src/telemetry/run-artifacts.ts @@ -79,6 +79,7 @@ export const ARTIFACT_LOCATION = { "final-findings.json": "stages/10-composition/final-findings.json", "github-posting.json": "stages/11-github-posting/github-posting.json", "final-review.md": "final-review.md", + "final-review.json": "final-review.json", "run.json": "run.json", "telemetry.json": "telemetry.json", "artifact-manifest.json": "artifact-manifest.json" diff --git a/src/types.ts b/src/types.ts index 79e53cd..5a73502 100644 --- a/src/types.ts +++ b/src/types.ts @@ -36,6 +36,7 @@ export type CodegenieConfig = { depth: ReviewDepth; verify: boolean; compositionReasoningStepDown: boolean; + skipSvgReview: boolean; minSeverity?: Severity; maxFindings: number; softCommentCap: number; @@ -342,6 +343,8 @@ export type ToolResultMeta = { degradationReason?: string; truncated?: boolean; omittedCount?: number; + omittedCountIsLowerBound?: boolean; + discoveryLimited?: boolean; lookupStatus?: "found" | "not_found" | "ambiguous" | "file_missing" | "unavailable"; deliveryStatus?: "full" | "truncated" | "budget_rejected" | "empty"; recovery?: { @@ -855,7 +858,18 @@ export type StructuredUncertainty = { projectedSkillIds: string[]; }; +export type RepositoryEvidence = { + symbols?: string[]; + id: string; + tool: string; + path?: string; + source: "head" | "base"; + text: string; +}; + export type PacketReviewResult = { + repositoryEvidence?: RepositoryEvidence[]; + diagnostics?: ReviewDiagnostic[]; packetId: string; lenses: string[]; findings: CandidateFinding[]; @@ -930,12 +944,14 @@ export type ResolvedFollowUpHint = { }; export type SystemReviewResult = { + diagnostics?: ReviewDiagnostic[]; tasks: SystemReviewTask[]; packetResults: PacketReviewResult[]; resolvedHints: ResolvedFollowUpHint[]; }; export type VerificationVerdict = { + diagnostic?: ReviewDiagnostic; candidateId: string; suggestionAssessments?: SuggestionAssessments; // Host-owned, provenance-only original proposals replaced during verification. @@ -974,11 +990,16 @@ export type FinalFinding = CandidateFinding & { }; export type RunCoverageStatus = { + /** No trustworthy coverage snapshot was assembled; numeric counters are placeholders. */ + unavailable?: boolean; + diagnostics?: ReviewDiagnostic[]; /** Absent in legacy artifacts or when supplemental outcomes were not recorded. */ adaptiveReviews?: AdaptiveReviewSummary; totalHunks: number; reviewedHunks: number; skippedHunks: number; + /** Proven deliberate exclusions; other skips may be incomplete planning. */ + excludedHunks?: number; failedHunks: number; coverageByLevel: Record; degradedPlanning: boolean; @@ -1037,7 +1058,23 @@ export type PlannerCoverageStats = { rejectedUnknownHunk: number; }; +export type ReviewDiagnostic = { + origin?: "tool"; + stage: number; + workItem?: string; + kind: "failure" | "incomplete"; + code: string; + reason: string; + recoveryExhausted: boolean; +}; +export type ReviewHealth = { + status: "failed" | "incomplete" | "unresolved" | "completed"; + diagnostics: ReviewDiagnostic[]; + unresolvedCount: number; +}; + export type ReviewResult = { + health?: ReviewHealth; summary: string; coverage: RunCoverageStatus; runStats?: ReviewRunStats; @@ -1097,6 +1134,7 @@ export type EvalCase = { adaptiveSecondPass?: boolean; verify?: boolean; compositionReasoningStepDown?: boolean; + skipSvgReview?: boolean; cache?: boolean; cacheDir?: string; debug?: boolean; @@ -1359,6 +1397,7 @@ export type EvalRunInfo = { review: { concurrency: number; compositionReasoningStepDown?: boolean; + skipSvgReview?: boolean; timeoutMs: number; maxBudgetTokens?: number; }; @@ -1628,6 +1667,8 @@ export type PlannerDossier = { }; export type SearchResult = { + excerpt?: boolean; + excerptStartColumn?: number; path: string; line: number; column?: number; @@ -1704,6 +1745,8 @@ export type ToolCallRecord = { startLine?: number; endLine?: number; query?: string; + pathGlob?: string; + maxResults?: number; glob?: string; source?: string; contextMode?: string; @@ -1715,6 +1758,8 @@ export type ToolCallRecord = { degradationReason?: string; truncated?: boolean; omittedCount?: number; + omittedCountIsLowerBound?: boolean; + discoveryLimited?: boolean; lookupStatus?: ToolResultMeta["lookupStatus"]; deliveryStatus?: ToolResultMeta["deliveryStatus"]; recovery?: ToolResultMeta["recovery"]; diff --git a/src/util/coverage-summary.ts b/src/util/coverage-summary.ts index 1c02db5..ecdf7f3 100644 --- a/src/util/coverage-summary.ts +++ b/src/util/coverage-summary.ts @@ -24,10 +24,15 @@ export function renderCoverageTrustBanner(coverage: RunCoverageStatus): string { } export function renderCoverageSummaryLines(coverage: RunCoverageStatus): string[] { + if (coverage.unavailable) { + return ["**Coverage unavailable:** the review failed before a trustworthy coverage summary could be assembled."]; + } const lines = [coverageHeadline(coverage)]; if (coverage.partial) { - lines.push(`Reviewed ${coverage.reviewedHunks}/${coverage.totalHunks} hunks before stopping.`); + lines.push(`Reviewed ${coverage.reviewedHunks}/${coverage.totalHunks} hunks${coverage.budgetStopped ? " before stopping" : ""}.`); } + const excluded = coverage.excludedHunks ?? coverage.skippedHunks; + if (excluded > 0) lines.push(`**Excluded by configuration/planning:** ${excluded} hunks.`); const statusLine = coverageStatusLine(coverage); if (statusLine !== undefined) { lines.push(statusLine); @@ -119,6 +124,7 @@ function coverageHeadline(coverage: RunCoverageStatus): string { if (unreviewed === 0 && coverage.degradedPlanning) { return "Review completed with degraded planning."; } + if (unreviewed === 0) return "**Partial review:** assigned hunks were reviewed, but required evidence gathering remains incomplete."; return `**Partial review:** ${unreviewed} ${hunkNoun(unreviewed)} did not complete review.`; } @@ -128,9 +134,8 @@ function hunkNoun(count: number): string { function coverageStatusLine(coverage: RunCoverageStatus): string | undefined { const parts: string[] = []; - if (coverage.skippedHunks > 0) { - parts.push(`skipped ${coverage.skippedHunks}`); - } + const uncertainSkips = coverage.skippedHunks - (coverage.excludedHunks ?? coverage.skippedHunks); + if (uncertainSkips > 0) parts.push(`skipped under incomplete planning ${uncertainSkips}`); if (coverage.failedHunks > 0) { parts.push(`failed ${coverage.failedHunks}`); } diff --git a/src/util/errors.ts b/src/util/errors.ts index 75852b7..df4e471 100644 --- a/src/util/errors.ts +++ b/src/util/errors.ts @@ -14,6 +14,7 @@ export type CodegenieErrorCode = | "parser_unavailable" | "skill_invalid" | "path_outside_repo" + | "review_failed" | "llm_call_failed" | "llm_schema_invalid" | "github_post_failed" diff --git a/src/util/review-health.ts b/src/util/review-health.ts new file mode 100644 index 0000000..0b71be6 --- /dev/null +++ b/src/util/review-health.ts @@ -0,0 +1,59 @@ +import type { ReviewDiagnostic, ReviewHealth, ReviewResult, RunCoverageStatus } from "../types.js"; +import { isCodegenieError } from "./errors.js"; +import { stripCredentials } from "../telemetry/redaction.js"; + +export function reviewDiagnostic(stage: number, error: unknown, workItem?: string, outcome?: string): ReviewDiagnostic { + const code = isCodegenieError(error) ? error.code : "unknown_execution_error"; + const limited = code === "timeout" || code === "budget_exhausted" || outcome === "timed_out" || outcome === "not_dispatched" + || (isCodegenieError(error) && error.context?.reason === "timeout"); + const headline = error instanceof Error ? error.message : "The underlying cause was not captured."; + const providerMessage = isCodegenieError(error) && typeof error.context?.providerMessage === "string" ? error.context.providerMessage : undefined; + return { stage, ...(workItem ? { workItem } : {}), kind: limited ? "incomplete" : "failure", code, + reason: stripCredentials(providerMessage ? `${headline}: ${providerMessage}` : headline).slice(0, 600), + recoveryExhausted: outcome !== "not_dispatched" }; +} + +export function deriveReviewHealth(coverage: RunCoverageStatus, unresolvedCount: number, verificationNeeded = false): ReviewHealth { + // Health and coverage are serialized together; shared objects are redacted as circular references. + const diagnostics = (coverage.diagnostics ?? []).map(diagnostic => ({ ...diagnostic })); + const status = diagnostics.some(d => d.kind === "failure") ? "failed" + : coverage.partial || coverage.failedHunks > 0 || coverage.verificationIncompleteCount > 0 || coverage.budgetStopped + || (coverage.verificationSkipped && verificationNeeded) || diagnostics.some(d => d.kind === "incomplete") ? "incomplete" + : unresolvedCount > 0 ? "unresolved" : "completed"; + return { status, diagnostics, unresolvedCount }; +} + +export function healthForResult(result: ReviewResult): ReviewHealth { + return result.health ?? deriveReviewHealth(result.coverage, + result.needsHumanAttention.length + (result.needsHumanAttentionOmittedCount ?? 0), + result.findings.length + result.summaryOnlyFindings.length > 0); +} + +export function renderReviewHealth(health: ReviewHealth): string { + if (health.status === "completed") return ""; + const title = health.status === "failed" ? "Review failed" : health.status === "incomplete" ? "Review incomplete" : "Review completed with unresolved questions"; + const detail = health.status === "unresolved" ? `${health.unresolvedCount} question(s) remain unresolved; absence of a confirmed finding does not establish safety.` + : "Required work did not complete reliably. Findings below are partial results."; + const diagnostics = [...health.diagnostics].sort((a, b) => Number(b.kind === "failure") - Number(a.kind === "failure")).slice(0, 5).map(d => + `> - Stage ${d.stage}${d.workItem ? ` (${safeText(d.workItem)})` : ""}: ${safeText(d.code)} — ${safeText(d.reason)}${d.recoveryExhausted ? " Recovery exhausted." : ""}`); + return [`> [!WARNING]`, `> **${title}.** ${detail}`, ...diagnostics].join("\n"); +} + +export function factualReviewSummary(health: ReviewHealth, count: number): string { + return `${count} confirmed finding${count === 1 ? "" : "s"} retained from completed work. ${health.status === "unresolved" ? "Unresolved questions require attention." : "Incomplete required work prevents a clean conclusion."}`; +} + +function safeText(text: string): string { + return stripCredentials(text).replace(/[\r\n]+/g, " ").replace(/[<>&`*\[\]]/g, "").slice(0, 600); +} + +/** Operational recovery requires the same complete request to succeed, not just a valid final payload. */ +export function unresolvedToolDiagnostic(stage: number, results: import("../llm/llm-runner.js").LlmToolResultSummary[], workItem: string): ReviewDiagnostic | undefined { + const failures = results.filter((result, index) => result.errorCode && + !["invalid_args", "path_outside_repo"].includes(result.errorCode) && + !results.slice(index + 1).some(later => result.requestKey !== undefined && later.requestKey === result.requestKey && later.status === "ok" && !later.truncated)); + const failed = failures.find(result => result.errorCode !== "budget_exhausted" && result.errorCode !== "timeout") ?? failures[0]; + if (!failed) return undefined; + return { stage, workItem, origin: "tool", code: failed.errorCode!, kind: failed.errorCode === "budget_exhausted" || failed.errorCode === "timeout" ? "incomplete" : "failure", + reason: stripCredentials(failed.preview ?? "A tool failed and required evidence remains unresolved; recovery could not be established.").slice(0, 600), recoveryExhausted: true }; +} diff --git a/tests/attention-reconciliation.test.ts b/tests/attention-reconciliation.test.ts index e52bf8a..835a54c 100644 --- a/tests/attention-reconciliation.test.ts +++ b/tests/attention-reconciliation.test.ts @@ -28,6 +28,22 @@ function fixture(questions = ["Does head include a revoked-session test?", "Is t } describe("evidence-backed attention reconciliation", () => { + it("uses completed packet source reads without treating a no-finding conclusion as evidence", () => { + const f = fixture(); + f.packet.findings = []; + f.packet.noFindingReason = "Everything is safe."; + f.packet.repositoryEvidence = [{ id: "read-1", tool: "read_range", path: "documents.test.ts", source: "base", text: "test('revoked session', () => expect(access(revoked)).toBe(false));" }]; + f.verdicts.splice(1); + const input = f.build(); + const source = input.inventory.evidence.find(item => item.origin === "repository_tool")!; + expect(source).toMatchObject({ source: "base", text: f.packet.repositoryEvidence[0]!.text }); + expect(JSON.stringify(input.inventory)).not.toContain("Everything is safe"); + expect(reconcileAttention(input, [], true).notes).toEqual([f.concern.unresolvedConcern]); + expect(reconcileAttention(input, [{ ...f.proposal, supportingRefs: [source.id] }], true).decisions[0]!.accepted).toBe(true); + f.packet.status = "incomplete"; + expect(f.build().inventory.evidence.some(item => item.origin === "repository_tool")).toBe(false); + }); + it("stores repeated evidence qualifications once without dropping their uncertainty", () => { const f = fixture(); f.observation.proofAssessment!.assumptions = [{ question: "Deployment is unconfirmed. ".repeat(50), essential: true }]; @@ -152,15 +168,15 @@ describe("evidence-backed attention reconciliation", () => { expect(reconcileAttention(input, [f.proposal], true).notes).toEqual([f.concern.unresolvedConcern]); }); - it("caps selected concern groups and does not remove an omitted sibling or same-text concern", () => { + it("admits more concerns than the display cap without removing unanswered siblings", () => { const f = fixture(["Does a test exist?"]); - for (let i = 1; i < 7; i++) f.verdicts.push({ ...structuredClone(f.concern), candidateId: "question-" + i }); + for (let i = 1; i < 15; i++) f.verdicts.push({ ...structuredClone(f.concern), candidateId: "question-" + i }); const input = f.build(); - expect(input.inventory.concerns).toHaveLength(5); - expect(input.omittedConcernIds).toHaveLength(2); + expect(input.inventory.concerns).toHaveLength(15); + expect(input.omittedConcernIds).toHaveLength(0); const result = reconcileAttention(input, [f.proposal], true); expect(result.notes).toEqual([f.concern.unresolvedConcern]); - expect(result.outcomes.filter(outcome => outcome.remainingQuestion)).toHaveLength(6); + expect(result.outcomes.filter(outcome => outcome.remainingQuestion)).toHaveLength(14); }); it("reuses published source IDs but never permits attention-only evidence to account for finding content", () => { @@ -224,7 +240,7 @@ describe("evidence-backed attention reconciliation", () => { expect(supplied.suggestionAssessment).toMatchObject({ status: "unverified", rationale: expect.stringContaining("Conflicting compatibility assessments") }); expect(input.inventory.evidence.find(source => source.id === id)).toMatchObject({ sourceRef: id }); // Reuse this valid registered reference against its own candidate's concern. - input.inventory.concerns[0]!.candidateId = f.candidate.id; + input.groups[0]!.concerns[0]!.candidateId = f.candidate.id; const result = reconcileAttention(input, [{ ...f.proposal, supportingRefs: [id] }], true); expect(result.decisions[0]).toMatchObject({ accepted: false, rejectionReason: "self_support" }); expect(result.notes).toEqual([f.concern.unresolvedConcern]); diff --git a/tests/config-loader.test.ts b/tests/config-loader.test.ts index a7493aa..5f7f82a 100644 --- a/tests/config-loader.test.ts +++ b/tests/config-loader.test.ts @@ -28,6 +28,20 @@ describe("config loader", () => { expect(rawConfigSchema.safeParse({ review: { compositionReasoningStepDown: "true" } }).success).toBe(false); }); + it("layers SVG skipping as a repo-safe boolean", () => { + const repoRoot = tempDir(); + const homeOverride = tempDir(); + expect(loadConfig({ repoRoot, homeOverride }).config.review.skipSvgReview).toBe(true); + writeFileSync(path.join(homeOverride, "config.toml"), "[review]\nskipSvgReview = false\n"); + expect(loadConfig({ repoRoot, homeOverride }).config.review.skipSvgReview).toBe(false); + writeFileSync(path.join(homeOverride, "config.toml"), "[review]\nskipSvgReview = true\n"); + expect(loadConfig({ repoRoot, homeOverride }).config.review.skipSvgReview).toBe(true); + writeFileSync(path.join(repoRoot, "codegenie.toml"), "[review]\nskipSvgReview = false\n"); + expect(loadConfig({ repoRoot, homeOverride }).config.review.skipSvgReview).toBe(false); + expect(loadConfig({ repoRoot, homeOverride, cli: { skipSvgReview: true } }).config.review.skipSvgReview).toBe(true); + expect(rawConfigSchema.safeParse({ review: { skipSvgReview: "true" } }).success).toBe(false); + }); + it("resolves maxTime minutes from defaults, user config, repo config, and CLI in precedence order", () => { const repoRoot = tempDir(); const home = tempDir(); diff --git a/tests/file-classifier.test.ts b/tests/file-classifier.test.ts index 15ab01b..7f5858f 100644 --- a/tests/file-classifier.test.ts +++ b/tests/file-classifier.test.ts @@ -8,6 +8,23 @@ import { resolveReviewInput } from "../src/git/review-input-resolver.js"; import { commitAll, git, initRepo, nullTelemetry, writeRepoFile } from "./helpers/git.js"; describe("file filtering and classification", () => { + it("skips SVGs by default, permits opt-in, and retains explicit exclusions", async () => { + const repo = initRepo(); + writeRepoFile(repo, "README.md", "base\n"); + commitAll(repo, "base"); + git(repo, ["checkout", "-b", "feature"]); + for (const file of ["icon.svg", "assets/diagram.svg", "assets/upper.SVG"]) writeRepoFile(repo, file, "\n"); + commitAll(repo, "SVGs"); + const resolved = await resolveReviewInput({ mode: "branch", branchName: "feature" }, defaultConfig, nullTelemetry(), { repoRoot: repo }); + const diff = parseDiff(resolved.rawDiff); + expect((await filterDiffFiles(resolved, diff, defaultConfig, nullTelemetry())).kept).toHaveLength(0); + const config = structuredClone(defaultConfig); + config.review.skipSvgReview = false; + expect((await filterDiffFiles(resolved, diff, config, nullTelemetry())).kept).toHaveLength(3); + config.classification.pathRules.push({ pattern: "**/*.svg", processingMode: "skip", reason: "explicit exclusion" }); + expect((await filterDiffFiles(resolved, diff, config, nullTelemetry())).kept.map(file => file.path)).toEqual(["assets/upper.SVG"]); + }); + it("classifies Rust, Python, and Solidity path roles without ambiguous directory skips", async () => { const repo = initRepo(); writeRepoFile(repo, "README.md", "base\n"); diff --git a/tests/github-action.test.ts b/tests/github-action.test.ts index 881a9e4..0b25559 100644 --- a/tests/github-action.test.ts +++ b/tests/github-action.test.ts @@ -914,6 +914,22 @@ describe("github-action entrypoint", () => { expect(reviewArgv).not.toContain("--post-github-comments"); }); + it("finalizes returned worker failures as failure while retaining the partial report", async () => { + const fake = createFakeComments(); + const reportPath = path.join(scratch, "failed-workers.md"); + const runDir = mkdtempSync(path.join(scratch, "failed-workers-")); + const report = "# Codegenie Review\n\n> **Review failed.** Stage 7: llm_schema_invalid\n\nPartial findings retained."; + await expect(executeGitHubActionCommand([], { + env: actionEnv(issueCommentPayload(), "issue_comment", { CODEGENIE_REPORT_PATH: reportPath }), + issueComments: fake.client, writeOutput: () => undefined, + runReview: async () => ({ runId: "r1", runDir, reportMarkdown: report, failed: true }) + })).rejects.toMatchObject({ code: "review_failed" }); + expect(readFileSync(reportPath, "utf8")).toContain("Partial findings retained"); + expect(JSON.parse(readFileSync(path.join(runDir, "github-action.json"), "utf8"))).toMatchObject({ + outcome: "review_failed", statusComment: { terminalState: "failure" } + }); + }); + it("publishes the report fallback even when the terminal edit fails", async () => { const fake = createFakeComments({ failUpdates: true }); const reportPath = path.join(scratch, `fallback-${Math.random().toString(36).slice(2)}.md`); diff --git a/tests/phase4-llm.test.ts b/tests/phase4-llm.test.ts index 34329cc..ecd7797 100644 --- a/tests/phase4-llm.test.ts +++ b/tests/phase4-llm.test.ts @@ -4908,6 +4908,54 @@ describe("Phase 4 Pi runner and model-call cache", () => { expect(lineRangeSchema).not.toHaveProperty("additionalItems"); }); + it.each(["full", "truncated", "error", "missing"] as const)("retains only usable source reads for reconciliation: %s", async mode => { + const telemetry = fakeTelemetry(); + const tool: ToolDefinition = { name: "read_range", description: "read", parameters: Type.Object({ path: Type.String() }), + execute: async () => ({ text: "return db.list(tenantId);", ...(mode === "error" ? { isError: true } : {}), + meta: { backend: "text", precision: "exact", degraded: false, sourceUsed: "base", + deliveryStatus: mode === "missing" ? "empty" : "full", lookupStatus: mode === "missing" ? "file_missing" : "found", + ...(mode === "truncated" ? { truncated: true } : {}) } }) }; + const adapter = scriptedAdapter([assistant([{ type: "toolCall", id: "source", name: "read_range", arguments: { path: "store.ts" } }]), + assistant([validSubmitReviewCall("done")])]); + 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() } }); + const captured: import("../src/llm/llm-runner.js").LlmToolResultSummary[] = []; + await runner.runStructured({ ...submitReviewRequest("evidence"), tools: [tool], onToolResults: results => captured.push(...results), + toolBudget: { maxToolCalls: 2, maxInvestigationRounds: 2, maxResultChars: 2000 } }); + expect(captured).toHaveLength(1); + if (mode === "full") expect(captured[0]!.repositoryEvidence).toMatchObject({ path: "store.ts", source: "base", text: "return db.list(tenantId);" }); + else expect(captured[0]!.repositoryEvidence).toBeUndefined(); + }); + + it("packs cached search data into complete JSON under each caller cap and records its scope", async () => { + const telemetry = fakeTelemetry(); + const matches = Array.from({ length: 20 }, (_, index) => ({ path: "src/a.ts", line: index + 1, matchText: "needle " + "x".repeat(100) })); + const execute = vi.fn(async () => ({ text: JSON.stringify(matches), searchResults: matches, + meta: { backend: "text" as const, precision: "text" as const, degraded: false } })); + const tool: ToolDefinition = { name: "search_files", description: "search", execute, + parameters: Type.Object({ query: Type.String(), pathGlob: Type.String(), maxResults: Type.Number() }) }; + const lookup = (id: string): PiToolCall => ({ type: "toolCall", id, name: "search_files", arguments: { query: "needle", pathGlob: "src/{api,data}/**", maxResults: 20 } }); + const adapter = scriptedAdapter([assistant([lookup("small")]), assistant([validSubmitReviewCall("done-small")]), assistant([lookup("large")]), assistant([validSubmitReviewCall("done-large")])]); + const runner = createPiRunner({ llmConfig: { provider: "fake", model: "fake-model", maxConcurrentCalls: 1 }, + telemetry: telemetry.recorder, logger: fakeLogger(), runSignal: new AbortController().signal, adapter, toolResultCache: createToolResultCache(), + hooks: { checkpoint: () => "ok", onUsage: vi.fn() } }); + for (const limit of [1200, 6000]) await runner.runStructured({ ...submitReviewRequest(`search-${limit}`), tools: [tool], + toolBudget: { maxToolCalls: 2, maxInvestigationRounds: 2, maxResultChars: limit } }); + const delivered = [adapter.contexts[1]!, adapter.contexts[3]!].map(context => { + const messages = JSON.parse(context) as Array<{role: string; content: Array<{ text: string }>}>; + const text = messages.find(message => message.role === "toolResult")!.content[0]!.text; + const payload = text.split("\n").slice(3, -3).join("\n"); + return { payload, parsed: JSON.parse(payload) }; + }); + expect(delivered[0]!.payload.length).toBeLessThanOrEqual(1200); + expect(delivered[0]!.parsed.results.length).toBeGreaterThan(0); + expect(delivered[0]!.parsed.results.length).toBeLessThan(20); + expect(delivered[1]!.parsed.results).toHaveLength(20); + expect(execute).toHaveBeenCalledTimes(1); + expect(telemetry.toolCalls[1]).toMatchObject({ cacheStatus: "hit", args: { pathGlob: "src/{api,data}/**", maxResults: 20 } }); + }); + it("rejects tools before execution when result-character budget is exhausted", async () => { const telemetry = fakeTelemetry(); const execute = vi.fn(async () => ({ diff --git a/tests/phase4-skills-provider.test.ts b/tests/phase4-skills-provider.test.ts index 6ad501f..83132eb 100644 --- a/tests/phase4-skills-provider.test.ts +++ b/tests/phase4-skills-provider.test.ts @@ -4,7 +4,7 @@ import { tmpdir } from "node:os"; import path from "node:path"; import type { OAuthAuth, OAuthCredential, ProviderAuthInteraction } from "@earendil-works/pi-ai"; import { describe, expect, it, vi } from "vitest"; -import { executeProviderCommand, parseProviderCommand } from "../src/cli/provider-command.js"; +import { executeProviderCommand, expandProviderAlias, parseProviderCommand } from "../src/cli/provider-command.js"; import { getCodegeniePaths } from "../src/config/paths.js"; import { defaultConfig } from "../src/config/schema.js"; import { @@ -361,6 +361,14 @@ describe("Phase 4 provider commands", () => { expect(error.context?.helpText).toContain("--api-key store an API key instead of using OAuth"); }); + it("expands the top-level `use` shorthand to `provider use`", () => { + expect(expandProviderAlias(["use", "opus:max"])).toEqual(["provider", "use", "opus:max"]); + expect(expandProviderAlias(["help", "use"])).toEqual(["provider", "use", "--help"]); + expect(expandProviderAlias(["provider", "list"])).toEqual(["provider", "list"]); + expect(expandProviderAlias(["HEAD~1"])).toBeUndefined(); + expect(parseProviderCommand(expandProviderAlias(["use", "opus:max"])!)).toEqual({ args: ["provider", "use", "opus:max"], options: {} }); + }); + it("includes command help for other missing required provider arguments", () => { let thrown: unknown; diff --git a/tests/pipeline-phase5.test.ts b/tests/pipeline-phase5.test.ts index cb07a3e..9dede83 100644 --- a/tests/pipeline-phase5.test.ts +++ b/tests/pipeline-phase5.test.ts @@ -133,12 +133,49 @@ describe("phase 5 pipeline regressions", () => { outputNotes: [{ ...original, question: expected }], reconciliation: { outcomes: [{ candidateId: "policy-question", original, remainingQuestion: expected }] } }); expect(JSON.stringify(artifacts.get("human-attention-notes.json"))).not.toContain("[redacted:circular]"); if (mode !== "fallback" && mode !== "legacy" && mode !== "no-findings") { - expect(result.summary).toContain("violates current-membership policy"); + expect(result.summary).toContain("Unresolved questions require attention"); expect(result.summaryOnlyFindings[0]!.finalBody).toContain(finding.proofAssessment!.evidence); expect(artifacts.get("composition-sources.json")).toMatchObject({ mode: "llm" }); } }); + it.each(["resolved", "partial", "missing-ref", "fallback", "fully-resolved"] as const)("reconciles packet questions with rejected-candidate evidence even with zero findings: %s", async mode => { + const candidate = verifierResolutionCandidate(); + const packet = verifierResolutionPacket(); + const packetResult = packetResultWithFindingAndHint(candidate, "billing/fee.ts"); + packetResult.followUpHints.push({ ...packetResult.followUpHints[0]!, question: "Does the remote payment provider enforce currency limits?" }); + if (mode === "fully-resolved") packetResult.followUpHints = packetResult.followUpHints.slice(0, 1); + const coverage = fakeCoverage(); + if (mode === "fully-resolved") coverage.diagnostics = [{ stage: 7, workItem: packetResult.packetId, kind: "incomplete", code: "budget_exhausted", reason: "Caller evidence lookup refused", recoveryExhausted: true, origin: "tool" }]; + let inventory: { concerns: Array<{ id: string; question: string }>; evidence: Array<{ id: string }> }; + const builder = createPromptBuilder(fakeLensRegistry()); + const result = await dedupeRankAndComposeReview({ verified: [], verdicts: [{ candidateId: candidate.id, verdict: "reject", + reason: "The helper already rejects zero.", requiredEvidencePresent: true, falsePositiveRisk: "low", + proofAssessment: { status: "refuted", evidence: "At the reviewed head, normalizeAmount rejects zero before fee calculation.", assumptions: [] } + }] }, fakePlan(), { mode: "branch", repoRoot: "/repo", commits: [], rawDiff: "", headSha: "head" }, coverage, { ...config(), github: { ...config().github, summaryWhenNoFindings: true } }, nullTelemetry(), { + postGithubComments: true, packets: [packet], packetResults: [packetResult], + promptBuilder: { ...builder, buildComposerPrompt: input => { + inventory = JSON.parse(input.attentionReconciliationJson!); + return builder.buildComposerPrompt(input); + } }, runner: { runStructured: async () => { + if (mode === "fallback") throw new CodegenieError("llm_schema_invalid", "invalid composition", { recoverable: true }); + const concern = inventory.concerns.find(item => item.question.includes("normalizeAmount"))!; + return { summary: "Everything looks good", composedFindings: [], attentionResolutions: [{ concernId: concern.id, + disposition: mode === "partial" ? "narrowed" : "resolved", supportingRefs: [mode === "missing-ref" ? "not-supplied" : inventory.evidence[0]!.id], + rationale: "The inspected head helper answers the zero-value guard question; it says nothing about remote provider policy.", + ...(mode === "partial" ? { remainingQuestion: "Does the deployed helper match this reviewed guard?" } : {}) }] } as T; + } } + }); + expect(result.noFindings).toBe(true); + expect(result.needsHumanAttention.some(note => note.question.includes("remote payment"))).toBe(mode !== "fully-resolved"); + expect(result.needsHumanAttention.some(note => note.question.includes("normalizeAmount"))).toBe(mode === "missing-ref" || mode === "fallback"); + if (mode === "partial") expect(result.needsHumanAttention.some(note => note.question.includes("deployed helper"))).toBe(true); + expect(result.health?.status).toBe(mode === "fully-resolved" ? "completed" : "unresolved"); + if (mode === "fully-resolved") expect(result.coverage.diagnostics).toEqual([]); + else expect(result.postingPlan?.reviewBody).toContain("Review completed with unresolved questions"); + expect(renderMarkdownReview(result)).not.toContain("Everything looks good"); + }); + it.each([true, false])("publishes supplied summary conclusions with original disagreement retained, resolved=%s", async resolved => { const fixture = contractComposition(); const historical = "Whether caller requirements permit reducing the advertised minimum is unknown."; @@ -5663,6 +5700,36 @@ describe("phase 5 pipeline regressions", () => { } }); + it("returns a failed report when required workers exhaust schema recovery, saving diagnostics and a failed run outcome", async () => { + const repo = initRepo(); + writeRepoFile(repo, "a.ts", "export const a = 1;\n"); + commitAll(repo, "base"); + git(repo, ["checkout", "-b", "feature"]); + writeRepoFile(repo, "a.ts", "export const a = 2;\n"); + commitAll(repo, "feature"); + const runArtifactDir = path.join(mkdtempSync(path.join(tmpdir(), "codegenie-health-")), "run"); + const review = await runReview({ mode: "branch", branchName: "feature" }, config(), { + repoRoot: repo, runArtifactDir, + runner: { runStructured: async (request: LlmStructuredRequest) => { + if (request.stage === 5) { + const dossier = extractPromptJson(request.prompt, "planner-dossier"); + return { diffUnderstanding: { declaredIntent: "fixture", inferredBehavior: "fixture" }, coverage: dossier!.files.flatMap(file => file.hunks.map(hunk => ({ + hunkId: hunk.hunkId, path: file.path, coverage: "normal", lenses: ["core/code-review"], surroundingContextHints: [], reason: "review" + }))) } as T; + } + if (request.stage === 7) throw new CodegenieError("llm_schema_invalid", "required submission invalid after recovery", { recoverable: true }); + return { summary: "Everything looks good", composedFindings: [] } as T; + } } + }); + expect(review.health?.status).toBe("failed"); + const report = readFileSync(path.join(runArtifactDir, "final-review.md"), "utf8"); + expect(report).toContain("**Review failed."); + expect(report).toContain("required submission invalid after recovery"); + expect(report).not.toContain("Everything looks good"); + expect(JSON.parse(readFileSync(path.join(runArtifactDir, "final-review.json"), "utf8")).health.status).toBe("failed"); + expect(JSON.parse(readFileSync(path.join(runArtifactDir, "run.json"), "utf8"))).toMatchObject({ outcome: { status: "failed", exitCode: 1 } }); + }); + it("records undispatched budget-stopped packets as failed coverage records", async () => { const repo = initRepo(); writeRepoFile(repo, "a.ts", "export const a = 1;\n"); @@ -5795,7 +5862,7 @@ describe("phase 5 pipeline regressions", () => { const dossier = JSON.parse(request.prompt) as PlannerDossier; return { diffUnderstanding: { declaredIntent: "chunk intent", inferredBehavior: dossier.compaction.chunkRoot ?? "single" }, - coverage: dossier.files.flatMap((file) => + coverage: dossier!.files.flatMap((file) => file.hunks.map((hunk) => ({ hunkId: hunk.hunkId, path: file.path, @@ -5854,7 +5921,7 @@ describe("phase 5 pipeline regressions", () => { omittedCounts.push(dossier.compaction.omitted.length); return { diffUnderstanding: { declaredIntent: "chunk intent", inferredBehavior: dossier.compaction.chunkRoot ?? "single" }, - coverage: dossier.files.flatMap((file) => + coverage: dossier!.files.flatMap((file) => file.hunks.map((hunk) => ({ hunkId: hunk.hunkId, path: file.path, @@ -6888,10 +6955,10 @@ 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).toContain("Incomplete required work prevents a clean conclusion"); + expect(result.postingPlan?.reviewBody).not.toContain("No credible findings were found within the reviewed scope"); expect(result.postingPlan?.reviewBody.indexOf("**Review incomplete.**")).toBeLessThan( - result.postingPlan?.reviewBody.indexOf("Review incomplete: completed work") ?? Number.MAX_SAFE_INTEGER + result.postingPlan?.reviewBody.indexOf("0 confirmed findings") ?? Number.MAX_SAFE_INTEGER ); }); @@ -6953,9 +7020,9 @@ describe("phase 5 pipeline regressions", () => { noFindings: true }); 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.")); + expect(markdown).toContain("**Review incomplete."); + expect(markdown).not.toContain("No credible findings were found within the reviewed scope"); + expect(markdown.indexOf("**Review incomplete.**")).toBeLessThan(markdown.indexOf("0 confirmed findings")); }); it("renders planner degradation prominently without inventing partial coverage", () => { @@ -6983,9 +7050,9 @@ describe("phase 5 pipeline regressions", () => { 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).toContain("No credible findings were found within the reviewed scope"); expect(markdown).not.toContain("**Review incomplete.**"); - expect(markdown).not.toContain("## ⚠️ Review Incomplete"); + expect(markdown).not.toContain("**Review incomplete."); expect(markdown).not.toContain("**Partial review:**"); }); @@ -9702,7 +9769,10 @@ describe("phase 5 pipeline regressions", () => { expect(result.findings[0]?.finalBody).not.toContain("Compact repaired body."); }); - it("fails the run on persistent provider-wide non-auth failures and writes failure logs", async () => { + it.each([ + { status: 503, message: "provider unavailable", reason: "transient_error", calls: 4, error: "LLM provider call failed" }, + { status: 400, message: "You have reached your specified API usage limits.", reason: "usage_limit", calls: 1, error: "LLM provider usage limit reached" } + ])("reports planning failures and unavailable coverage for provider HTTP $status", async (providerFailure) => { const repo = initRepo(); writeRepoFile(repo, "app.ts", "export const value = 1;\n"); commitAll(repo, "base"); @@ -9716,8 +9786,8 @@ describe("phase 5 pipeline regressions", () => { resolveModel: () => ({ provider: "scripted", id: "scripted-model", raw: { id: "scripted-model", api: "faux" } }), complete: async () => { providerCalls += 1; - const error = new Error("provider unavailable") as Error & { status: number }; - error.status = 503; + const error = new Error(providerFailure.message) as Error & { status: number }; + error.status = providerFailure.status; throw error; }, validateToolCall: (_tools, call) => call.arguments @@ -9734,9 +9804,20 @@ describe("phase 5 pipeline regressions", () => { }, { repoRoot: repo, runArtifactDir, piAdapter: adapter } ) - ).rejects.toMatchObject({ code: "llm_call_failed", context: { reason: "transient_error" } }); - - expect(providerCalls).toBe(4); + ).rejects.toMatchObject({ code: "llm_call_failed", context: { reason: providerFailure.reason } }); + + expect(providerCalls).toBe(providerFailure.calls); + const markdown = readFileSync(path.join(runArtifactDir, "final-review.md"), "utf8"); + expect(markdown).toContain("**Review failed."); + expect(markdown).toContain("Stage 5: llm_call_failed"); + expect(markdown).toContain("**Coverage unavailable:"); + expect(markdown).not.toContain("0/0 hunks"); + expect(markdown).not.toContain("assigned hunks were reviewed"); + expect(markdown).not.toContain("Coverage levels"); + expect(JSON.parse(readFileSync(path.join(runArtifactDir, "final-review.json"), "utf8"))).toMatchObject({ + health: { status: "failed", diagnostics: [expect.objectContaining({ stage: 5 })] }, + coverage: { unavailable: true, partial: true } + }); const runJson = JSON.parse(readFileSync(path.join(runArtifactDir, "run.json"), "utf8")) as { outcome: { status: string; errorCode: string | null }; }; @@ -9747,9 +9828,10 @@ describe("phase 5 pipeline regressions", () => { context: { reason: string }; }; expect(errorJson).toMatchObject({ + stage: 5, errorCode: "llm_call_failed", - error: "LLM provider call failed", - context: { reason: "transient_error" } + error: providerFailure.error, + context: { reason: providerFailure.reason } }); const runLog = readFileSync(path.join(runArtifactDir, "run.log"), "utf8"); expect(runLog).toContain("model_call_started"); @@ -12803,7 +12885,7 @@ describe("phase 5 pipeline regressions", () => { ); expect(result.needsHumanAttention).toContainEqual(expect.objectContaining({ question: "Which denomination does the deployed contract expect?" })); - expect(result.summary).toBe("No security issues, but one correctness bug remains."); + expect(result.summary).toContain("Unresolved questions require attention"); expect([...result.findings, ...result.summaryOnlyFindings]).toHaveLength(1); }); @@ -13543,7 +13625,7 @@ describe("phase 5 pipeline regressions", () => { }); }); - it("suppresses human-attention notes already covered by final findings", async () => { + it("retains packet questions when overlapping findings lack an explicit evidence resolution", async () => { const finding: CandidateFinding = { ...fakeFinding(), id: "finding-cache-stale", @@ -13621,7 +13703,7 @@ describe("phase 5 pipeline regressions", () => { ); expect(result.summaryOnlyFindings).toHaveLength(1); - expect(result.needsHumanAttention).toEqual([]); + expect(result.needsHumanAttention).toHaveLength(1); }); it("renders the matching fallback note when a completed keep is suppressed by publication quality", async () => { @@ -13685,17 +13767,14 @@ describe("phase 5 pipeline regressions", () => { }); expect(artifacts.get("human-attention-notes.json")).toMatchObject({ outputNotes: [expect.objectContaining({ question: "Check whether normalizeAmount rejects zero prices before fee calculation." })], - fallbackGroupCount: 1, + fallbackGroupCount: 0, omittedFallbackCount: 0, - publicationFallbacks: [expect.objectContaining({ candidateId: candidate.id, verdict: "keep" })] + publicationFallbacks: [] }); - expect(events).toContainEqual(expect.objectContaining({ - message: "human_attention_publication_fallback", - data: expect.objectContaining({ fallbackCandidateIds: [candidate.id], fallbackGroupCount: 1, omittedFallbackCount: 0 }) - })); + expect(events.some(event => event.message === "human_attention_publication_fallback")).toBe(false); }); - it("publishes a fully concrete completed keep summary-only and suppresses its redundant note", async () => { + it("publishes a concrete keep without implicitly deleting its outstanding question", async () => { const base = verifierResolutionCandidate(); const candidate: CandidateFinding = { ...base, @@ -13745,18 +13824,18 @@ describe("phase 5 pipeline regressions", () => { expect(result.summaryOnlyFindings).toEqual([ expect.objectContaining({ id: candidate.id, publication: "summary-only", changedLine: false }) ]); - expect(result.needsHumanAttention).toEqual([]); + expect(result.needsHumanAttention).toHaveLength(1); expect(artifacts.get("final-selection.json")).toMatchObject({ records: [expect.objectContaining({ findingId: candidate.id, decision: "published", reason: "low-confidence-anchorless" })] }); expect(artifacts.get("human-attention-notes.json")).toMatchObject({ - outputNotes: [], + outputNotes: [expect.objectContaining({ question: expect.any(String) })], fallbackGroupCount: 0, omittedFallbackCount: 0 }); }); - it("suppresses human-attention notes resolved by verifier rejection with evidence", async () => { + it("retains packet questions when a reject supplies no explicit composition resolution", async () => { const artifacts = new Map(); const events: Array> = []; let composerNotes: string[] | undefined; @@ -13807,29 +13886,10 @@ describe("phase 5 pipeline regressions", () => { } ); - expect(composerNotes).toEqual([]); - expect(result.needsHumanAttention).toEqual([]); - expect(artifacts.get("human-attention-notes.json")).toMatchObject({ - schemaVersion: 3, - suppressedByVerification: [ - expect.objectContaining({ - candidateId: "finding-helper-guard", - verdict: "reject", - noteIds: [expect.stringMatching(/^note-/u)], - match: expect.objectContaining({ - sharedFiles: ["billing/fee.ts"], - questionMatched: true, - provenanceMatched: true - }) - }) - ], - keptForOutputGroupIds: [] - }); - expect(events).toContainEqual(expect.objectContaining({ - stage: 10, - message: "human_attention_hints_suppressed_by_verification", - data: expect.objectContaining({ suppressed: 1, remainingGroups: 0 }) - })); + expect(composerNotes).toHaveLength(1); + expect(result.needsHumanAttention).toHaveLength(1); + expect(result.health?.status).toBe("unresolved"); + expect(artifacts.get("human-attention-notes.json")).toMatchObject({ suppressedByVerification: [] }); }); it("does not suppress unrelated human-attention notes through weak same-file verifier overlap", async () => { @@ -13975,7 +14035,7 @@ describe("phase 5 pipeline regressions", () => { ]); }); - it("drops unknown human-attention paths and allows verifier suppression by predicate", async () => { + it("drops unknown paths but retains unanswered packet predicates", async () => { const artifacts = new Map(); const events: Array> = []; const candidate = verifierResolutionCandidate(); @@ -14018,7 +14078,7 @@ describe("phase 5 pipeline regressions", () => { } ); - expect(result.needsHumanAttention).toEqual([]); + expect(result.needsHumanAttention).toEqual([expect.objectContaining({ files: [], symbols: ["calculateFee", "normalizeAmount"] })]); expect(events).toContainEqual(expect.objectContaining({ stage: 10, message: "human_attention_note_path_dropped", @@ -14036,12 +14096,7 @@ describe("phase 5 pipeline regressions", () => { droppedPaths: [{ path: "billing/quotes.ts", reason: "unknown_path" }] }) ], - suppressedByVerification: [ - expect.objectContaining({ - candidateId: "finding-helper-guard", - match: expect.objectContaining({ sharedFiles: [] }) - }) - ] + suppressedByVerification: [] }); }); diff --git a/tests/pipeline-phase6.test.ts b/tests/pipeline-phase6.test.ts index d2ba1a8..5aac490 100644 --- a/tests/pipeline-phase6.test.ts +++ b/tests/pipeline-phase6.test.ts @@ -48,7 +48,7 @@ describe("phase 6 live review path", () => { ).not.toBe(reviewCacheFingerprint(baseConfig, "/repo", resolved, "registry")); }); - it("uses the Pi runner end to end for branch reviews with repair, retry, and covered follow-up suppression", async () => { + it("uses the Pi runner end to end for branch reviews with repair, retry, and explicit unresolved follow-ups", async () => { const repo = initRepo(); writeRepoFile(repo, "app.ts", "export function divide(total: number, count: number) {\n return total / Math.max(1, count);\n}\n"); commitAll(repo, "base"); @@ -73,7 +73,7 @@ describe("phase 6 live review path", () => { } ); - expect(result.summary).toBe("⚠️ Found 1 verified issue."); + expect(result.summary).toContain("1 confirmed finding"); expect(result.findings).toHaveLength(1); expect(result.findings[0]).toMatchObject({ title: "Division by zero guard was removed", @@ -81,9 +81,9 @@ describe("phase 6 live review path", () => { publication: "inline", finalBody: expect.stringContaining("Restore the guard") }); - expect(result.needsHumanAttention).toEqual([]); - expect(output.join("\n")).toContain("Found 1 verified issue."); - expect(output.join("\n")).not.toContain("Needs Human Attention"); + expect(result.needsHumanAttention).toEqual([expect.objectContaining({ question: "Check whether callers can pass zero count." })]); + expect(output.join("\n")).toContain("1 confirmed finding"); + expect(output.join("\n")).toContain("Needs Human Attention"); expect(output.join("\n")).not.toContain("ghp_abcdefghijklmnopqrstuvwxyz1234567890"); expect(output.join("\n")).toContain("Diagnostic token: [redacted:"); expect(adapter.callsByPrompt).toMatchObject({ diff --git a/tests/repository-intelligence.test.ts b/tests/repository-intelligence.test.ts index edcc1a9..f17109a 100644 --- a/tests/repository-intelligence.test.ts +++ b/tests/repository-intelligence.test.ts @@ -696,11 +696,8 @@ export { internal as Public } const totalCappedSearch = await tools.searchFiles("SearchNeedle", { maxResults: 100 }); expect(totalCappedSearch.meta.truncated).toBe(true); expect(JSON.stringify(totalCappedSearch.results).length).toBeLessThanOrEqual(16_000); - const hugeLineSearch = await tools.searchFiles("HugeNeedle", { maxResults: 1 }); - expect(hugeLineSearch.meta.degraded).toBe(true); - expect(hugeLineSearch.meta.truncated).toBe(true); - expect(hugeLineSearch.meta.omittedCount).toBeGreaterThan(0); - expect(hugeLineSearch.results[0]?.path).toBe("huge/huge.txt"); + // Oversized raw lines must disclose the discovery limit, never masquerade as exhaustive matches. + await expect(tools.searchFiles("HugeNeedle", { maxResults: 1 })).rejects.toMatchObject({ code: "budget_exhausted" }); const untrackedSearch = await tools.searchFiles("UniqueUntracked"); expect(untrackedSearch.results).toEqual([]); const ignoredTrackedSearch = await tools.searchFiles("IgnoredTrackedNeedle"); diff --git a/tests/review-command.test.ts b/tests/review-command.test.ts index 74f1b14..a1a0857 100644 --- a/tests/review-command.test.ts +++ b/tests/review-command.test.ts @@ -25,6 +25,23 @@ describe("review command", () => { expect(parsed.warnings).toEqual([]); }); + it.each([ + [undefined, undefined, true, "defaults"], + [true, undefined, true, "repo-config"], + [false, undefined, false, "repo-config"], + [false, "--skip-svg-review", true, "cli"], + [true, "--no-skip-svg-review", false, "cli"], + [undefined, "--skip-svg-review", true, "cli"], + [undefined, "--no-skip-svg-review", false, "cli"] + ] as const)("resolves SVG skipping config=%s flag=%s", (configured, flag, expected, source) => { + const ctx = testContext(); + if (configured !== undefined) writeFileSync(path.join(ctx.repoRoot, "codegenie.toml"), `[review]\nskipSvgReview = ${configured}\n`); + const parsed = parseReviewCommand(["review", "--branch", "feature", ...(flag ? [flag] : [])], ctx); + expect(parsed.config.review.skipSvgReview).toBe(expected); + expect(parsed.configSources["review.skipSvgReview"]).toBe(source); + expect(parsed.warnings).toEqual([]); + }); + it("treats top-level help as a successful display exit", () => { expect(() => parseReviewCommand(["--help"], testContext())).toThrow(CliDisplayExit); diff --git a/tests/review-health.test.ts b/tests/review-health.test.ts new file mode 100644 index 0000000..512fe2c --- /dev/null +++ b/tests/review-health.test.ts @@ -0,0 +1,80 @@ +import { describe, it, expect } from "vitest"; +import { deriveReviewHealth, reviewDiagnostic } from "../src/util/review-health.js"; +import { renderMarkdownReview } from "../src/output/markdown-renderer.js"; +import { renderPostingSummaryForStdout } from "../src/output/stdout-renderer.js"; +import { CodegenieError } from "../src/util/errors.js"; +import type { ReviewResult, RunCoverageStatus } from "../src/types.js"; + +const coverage: RunCoverageStatus = { totalHunks: 4, reviewedHunks: 2, skippedHunks: 2, failedHunks: 0, + coverageByLevel: { deep: 0, normal: 2, light: 0, skip: 2 }, degradedPlanning: false, budgetStopped: false, + verificationIncompleteCount: 0, partial: false, reasons: [] }; +function result(c: RunCoverageStatus = coverage): ReviewResult { + return { coverage: c, summary: "Everything looks good", findings: [], summaryOnlyFindings: [], needsHumanAttention: [], noFindings: true }; +} + +describe("review health", () => { + it("distinguishes unavailable coverage from a known empty review", () => { + const empty = { ...coverage, totalHunks: 0, reviewedHunks: 0, skippedHunks: 0, + coverageByLevel: { deep: 0, normal: 0, light: 0, skip: 0 } }; + const unavailable = renderMarkdownReview(result({ ...empty, unavailable: true, partial: true, + diagnostics: [reviewDiagnostic(1, new CodegenieError("invalid_args", "Unknown revision"))] })); + expect(unavailable).toContain("Stage 1: invalid_args"); + expect(unavailable).toContain("**Coverage unavailable:"); + expect(unavailable).not.toContain("0/0 hunks"); + expect(unavailable).not.toContain("0 hunks did not complete"); + expect(renderMarkdownReview(result(empty))).toContain("Reviewed 0/0 hunks."); + expect(renderMarkdownReview(result(empty))).not.toContain("Coverage unavailable"); + }); + it("describes unfinished evidence without claiming zero hunks failed or an early stop", () => { + const markdown = renderMarkdownReview(result({ ...coverage, partial: true, + diagnostics: [reviewDiagnostic(7, new CodegenieError("budget_exhausted", "source unavailable"))] })); + expect(markdown).toContain("required evidence gathering remains incomplete"); + expect(markdown).not.toContain("0 hunks did not complete"); + expect(markdown).not.toContain("before stopping"); + }); + it("gives unrecovered failures precedence and redacts diagnostics before rendering", () => { + const diagnostic = reviewDiagnostic(9, new CodegenieError("llm_call_failed", "HTTP 402 token=very-secret-value"), "candidate-1", "failed"); + const r = result({ ...coverage, partial: true, diagnostics: [diagnostic] }); + r.health = deriveReviewHealth(r.coverage, 10); + expect(r.health.status).toBe("failed"); + const markdown = renderMarkdownReview(r); + expect(markdown.indexOf("**Review failed.")).toBeLessThan(markdown.indexOf("confirmed findings")); + expect(markdown).toContain("Stage 9 (candidate-1): llm_call_failed"); + expect(markdown).not.toContain("very-secret-value"); + expect(markdown).not.toContain("Everything looks good"); + expect(markdown).not.toContain("## No confirmed findings"); + expect(renderPostingSummaryForStdout(r, "markdown")).toContain("Review failed"); + }); + it.each(["timeout", "budget_exhausted"] as const)("treats required limits as incomplete, not operational failure: %s", code => { + const r = result({ ...coverage, diagnostics: [reviewDiagnostic(7, new CodegenieError(code, "limit"))] }); + expect(deriveReviewHealth(r.coverage, 0).status).toBe("incomplete"); + expect(renderMarkdownReview(r)).toContain("Review incomplete"); + }); + it("retains bounded provider reasons and puts fundamental errors ahead of limits", () => { + const diagnostic = reviewDiagnostic(9, new CodegenieError("llm_call_failed", "Provider call failed", { context: { providerMessage: "HTTP 402: no available credits" } })); + const r = result({ ...coverage, diagnostics: [...Array.from({length: 5}, () => reviewDiagnostic(7, new CodegenieError("budget_exhausted", "limit"))), diagnostic] }); + expect(renderMarkdownReview(r)).toContain("HTTP 402: no available credits"); + expect(renderMarkdownReview(r)).toContain("**Review failed."); + }); + it("does not label skips under incomplete planning as deliberate exclusions", () => { + const r = result({ ...coverage, partial: true, excludedHunks: 0 }); + expect(renderMarkdownReview(r)).toContain("skipped under incomplete planning 2"); + expect(renderMarkdownReview(r)).not.toContain("Excluded by configuration/planning"); + }); + it("keeps unresolved status even when presentation hides every question", () => { + const r = result(); + r.needsHumanAttentionOmittedCount = 3; + expect(renderMarkdownReview(r)).toContain("Review completed with unresolved questions"); + expect(renderMarkdownReview(r)).not.toContain("Everything looks good"); + }); + it("does not call deliberate exclusions incomplete, and does not fail recovered execution", () => { + expect(deriveReviewHealth(coverage, 0).status).toBe("completed"); + const markdown = renderMarkdownReview({ ...result(), summary: "No confirmed findings within reviewed scope." }); + expect(markdown).toContain("Excluded by configuration/planning:** 2"); + expect(markdown).not.toContain("Incomplete work"); + }); + it("marks skipped verification incomplete only when candidates needed it", () => { + expect(deriveReviewHealth({ ...coverage, verificationSkipped: true }, 0, true).status).toBe("incomplete"); + expect(deriveReviewHealth({ ...coverage, verificationSkipped: true }, 0, false).status).toBe("completed"); + }); +}); diff --git a/tests/search-reliability.test.ts b/tests/search-reliability.test.ts new file mode 100644 index 0000000..65a6e04 --- /dev/null +++ b/tests/search-reliability.test.ts @@ -0,0 +1,134 @@ +import { buildRepositoryToolDefinitions } from "../src/llm/tool-definitions.js"; +import { RepositoryToolsFacade } from "../src/repo/repository-index.js"; +import { nullTelemetry } from "./helpers/git.js"; +import { describe, it, expect, vi } from "vitest"; +import { SourceResolver } from "../src/repo/source-resolver.js"; +import { SearchService } from "../src/repo/search.js"; +import { LanguageAdapterRegistry } from "../src/repo/language-adapter.js"; +import { TreeSitterService } from "../src/repo/tree-sitter/tree-sitter-service.js"; +import { createGitClient } from "../src/git/git-client.js"; +import { packSearchToolResult } from "../src/llm/search-result-packing.js"; +import { createToolResultCache } from "../src/llm/tool-result-cache.js"; +import { commitAll, initRepo, writeRepoFile } from "./helpers/git.js"; + +async function fixture() { + const repo = initRepo(); + writeRepoFile(repo, "src/data/a.txt", "Statuses\n"); + writeRepoFile(repo, "src/domain/b.txt", "func List\n"); + writeRepoFile(repo, "src/data/[x].txt", "literal\n"); + writeRepoFile(repo, "src/data/x.txt", "other\n"); + const base = commitAll(repo, "base"); + writeRepoFile(repo, "src/data/a.txt", "Statuses changed\n"); + const head = commitAll(repo, "head"); + const git = createGitClient(repo); + const resolver = await SourceResolver.create({ mode: "commit_range", repoRoot: repo, startCommit: base, endCommit: head, mergeBase: base, headSha: head, commits: [], rawDiff: "" }, git); + return { resolver, repo, git, service: new SearchService(resolver, new LanguageAdapterRegistry(new TreeSitterService())) }; +} + +describe("reliable search", () => { + it("retains useful matches on both sides of oversized lines with explicit omissions", async () => { + const { resolver, repo, service } = await fixture(); + writeRepoFile(repo, "big.txt", `needle before\nneedle ${"x".repeat(320_000)}\nneedle after\n`); + resolver.binding.headCommit = commitAll(repo, "oversized matching line"); + const result = await service.search("needle", { pathGlob: "big.txt" }); + expect(result.results.map(match => match.line)).toEqual([1, 3]); + expect(result).toMatchObject({ truncated: true, degraded: true, omittedCount: 1, discoveryLimited: true }); + const packed = packSearchToolResult({ text: "", searchResults: result.results, meta: result }, 2000); + expect(JSON.parse(packed.text).results).toHaveLength(2); + expect(JSON.parse(packed.text).notice).toContain("not exhaustive"); + await expect(service.search("x+", { pathGlob: "big.txt" })).rejects.toMatchObject({ code: "budget_exhausted" }); + expect((await service.search("absent", { pathGlob: "big.txt" })).results).toEqual([]); + }); + + it("shares brace glob scope and POSIX alternation, preserving revisions and literal paths", async () => { + const { resolver } = await fixture(); + const glob = "src/{data,domain}/**"; + expect(await resolver.listFiles(glob)).toHaveLength(4); + const matches = await resolver.grep("Statuses|func List", { glob }); + expect(matches.map(m => m.path)).toEqual(["src/data/a.txt", "src/domain/b.txt"]); + expect((await resolver.grep("Statuses", { glob, source: { kind: "base" } }))[0]?.matchText).toBe("Statuses"); + expect((await resolver.grep("literal|other", { glob: "src/data/[[]x[]].txt" })).map(m => m.path)).toEqual(["src/data/[x].txt"]); + expect(await resolver.grep("absent", { glob })).toEqual([]); + expect(await resolver.grep("valid", { glob: "missing/**" })).toEqual([]); + }); + it("rejects invalid syntax even in empty scopes and preserves backend failure", async () => { + const { resolver, git } = await fixture(); + for (const query of ["[", "(?=x)"]) await expect(resolver.grep(query, { glob: "missing/**" })).rejects.toMatchObject({ code: "invalid_args", message: expect.stringContaining("query") }); + await expect(resolver.grep("valid", { glob: "src/{data" })).rejects.toMatchObject({ code: "invalid_args", message: expect.stringContaining("pathGlob") }); + expect(await resolver.grep("(?=x)", { fixedString: true })).toEqual([]); + vi.spyOn(git, "lsTree").mockRejectedValueOnce(new Error("backend unavailable")); + await expect(resolver.grep("valid", { glob: "**" })).rejects.toThrow("backend unavailable"); + }); + it("delivers invalid inputs as actionable model-facing errors, not empty matches", async () => { + const { resolver } = await fixture(); + const tools = new RepositoryToolsFacade({ resolver, diff: { files: [] }, registry: new LanguageAdapterRegistry(new TreeSitterService()), telemetry: nullTelemetry() }); + const search = buildRepositoryToolDefinitions(tools).find(tool => tool.name === "search_files")!; + const bad = await search.execute({ query: "[", pathGlob: "missing/**" }, new AbortController().signal); + expect(bad).toMatchObject({ isError: true, errorCode: "invalid_args" }); + expect(bad.text).toContain("query is invalid POSIX ERE"); + expect(bad.text).toContain("Check brackets"); + const valid = await search.execute({ query: "absent", pathGlob: "missing/**" }, new AbortController().signal); + expect(valid.isError).not.toBe(true); + expect(valid.searchResults).toEqual([]); + expect(await resolver.grep("\\(\\?", { glob: "missing/**" })).toEqual([]); + }); + + it("chunks literal scopes with a single global allowance", async () => { + const { resolver, repo } = await fixture(); + for (let i = 0; i < 270; i++) writeRepoFile(repo, `many/${String(i).padStart(3, "0")}.txt`, "needle\n"); + const head = commitAll(repo, "many"); + resolver.binding.headCommit = head; + expect(await resolver.grep("needle", { glob: "many/**", maxResults: 260 })).toHaveLength(260); + expect(await resolver.grep("needle", { glob: "many/**", maxResults: 2 })).toHaveLength(2); + }); + it("keeps compact matches for large TypeScript declarations without changing canonical symbols", async () => { + const { resolver, repo, service } = await fixture(); + writeRepoFile(repo, "locale.ts", `export const translations = { greeting: "${"x".repeat(30000)}" };\nconsole.log(translations);\n`); + resolver.binding.headCommit = commitAll(repo, "large TypeScript declaration"); + const matches = await service.search("translations", { contextMode: "symbols" }); + expect(matches.results).toHaveLength(2); + expect(matches.results.map(match => match.line)).toEqual([1, 2]); + expect(JSON.stringify(matches.results).length).toBeLessThan(1500); + expect((await resolver.readFile("locale.ts"))!.content.length).toBeGreaterThan(30000); + }); + + it("retains locations instead of large enclosing initializers and filters comments first", async () => { + const { resolver, repo, service } = await fixture(); + writeRepoFile(repo, "many.go", `package p\nvar values = []string{\n${Array.from({length: 60}, () => '"' + "x".repeat(500) + '",').join("\n")}\n}\n// values is discussed here\nfunc use() { println(values) }\n`); + writeRepoFile(repo, "long.txt", "x".repeat(5000) + "needle at the end\n"); + resolver.binding.headCommit = commitAll(repo, "large symbols"); + const result = await service.findSymbolMentions("values", { contextMode: "symbols" }); + expect(result.results.map(m => m.line)).toEqual([2, 65]); + expect(JSON.stringify(result.results).length).toBeLessThan(2000); + expect(result.results.every(m => !("signature" in (m.enclosingSymbol ?? {})))).toBe(true); + const long = await service.search("needle"); + expect(long.results[0]).toMatchObject({ column: 5001, excerpt: true }); + expect(long.results[0]?.matchText).toContain("needle"); + }); +}); + +describe("per-consumer result packing", () => { + it("retains later matches when the first path alone exceeds the delivery allowance", () => { + const results = [{ path: "x".repeat(2000), line: 1, matchText: "needle" }, { path: "a.ts", line: 2, matchText: "needle" }]; + const output = packSearchToolResult({ text: "", searchResults: results, meta: { backend: "text", precision: "text", degraded: false } }, 800); + expect(JSON.parse(output.text).results).toEqual([results[1]]); + expect(JSON.parse(output.text).meta.omittedCount).toBe(1); + }); + it.each([true, false])("does not poison cached results (small first: %s)", async smallFirst => { + const cache = createToolResultCache(); + const results = Array.from({length: 15}, (_, i) => ({ path: "src/a.ts", line: i + 1, matchText: "needle " + "x".repeat(100), contextAfter: ["y".repeat(500)] })); + const canonical = { text: JSON.stringify(results), searchResults: results, meta: { backend: "text" as const, precision: "text" as const, degraded: false } }; + const run = vi.fn(async () => canonical); + const lookup = () => cache.execute({toolName: "search_files", args: {query: "needle"}, run}); + const [first, concurrent] = await Promise.all([lookup(), lookup()]); + const small = packSearchToolResult(smallFirst ? first.result : concurrent.result, 1200); + const large = packSearchToolResult(smallFirst ? concurrent.result : first.result, 16000); + expect(small.text.length).toBeLessThanOrEqual(1200); + expect(JSON.parse(small.text).results.length).toBeGreaterThan(0); + expect(JSON.parse(small.text).meta.omittedCount).toBeGreaterThan(0); + expect(JSON.parse(large.text).results).toHaveLength(15); + expect((await lookup()).result.searchResults).toEqual(results); + expect(run).toHaveBeenCalledTimes(1); + expect(packSearchToolResult(canonical, 10)).toMatchObject({isError: true, errorCode: "budget_exhausted"}); + }); +}); diff --git a/tests/verifier.test.ts b/tests/verifier.test.ts index dabd50c..3951a76 100644 --- a/tests/verifier.test.ts +++ b/tests/verifier.test.ts @@ -23,6 +23,48 @@ import { scoreEvalRun } from "../src/evals/eval-scoring.js"; import { nullTelemetry } from "./helpers/git.js"; describe("stage 9 evidence-aware verification", () => { + it.each(["unresolved", "recovered", "refuted", "bad-argument", "budget"] as const)("retains source-failure diagnostics only when evidence remains unresolved: %s", async mode => { + const fixture = reviewFixture(["service.ts"]); + const packet = fixture.packets[0]!; + const finding = candidate("source-failure", packet); + const result = await verifyFindings({ packetResults: [packetResult(packet.id, [finding])], packets: [packet] }, fakeTools(), config(), nullTelemetry(), { + runner: { runStructured: async (request: LlmStructuredRequest) => { + request.onToolResults?.([{ id: "t1", tool: "search_files", target: "service.ts", requestKey: "exact", status: "error", resultChars: 50, + errorCode: mode === "bad-argument" ? "invalid_args" : mode === "budget" ? "budget_exhausted" : "git_ref_missing", preview: "Unable to inspect selected revision" }, + ...(mode === "recovered" ? [{ id: "t2", tool: "search_files", target: "service.ts", requestKey: "exact", status: "ok" as const, resultChars: 100 }] : [])]); + return { verdict: "reject", reason: "No supported defect.", requiredEvidencePresent: mode === "refuted", falsePositiveRisk: "high", + proofAssessment: { status: mode === "refuted" ? "refuted" : "unresolved", evidence: "Caller policy inspected only in supplied scope.", + assumptions: mode === "refuted" ? [] : [{ question: "Which caller requires this route?", essential: true }] } } as T; + } }, promptBuilder: createPromptBuilder(fakeLensRegistry()), lensRegistry: fakeLensRegistry(), diff: fixture.diff + }); + const verdict = result.verdicts[0]!; + expect(verdict.verdict).toBe("reject"); + if (mode === "unresolved" || mode === "budget") expect(verdict.diagnostic).toMatchObject({ stage: 9, kind: mode === "budget" ? "incomplete" : "failure" }); + else expect(verdict.diagnostic).toBeUndefined(); + }); + + it.each([ + { title: "An extra malformed-input case", actionable: false, evidence: "No consequential contract gap established; transport tests already exercise rejection." }, + { title: "Tenant assertion accepts another tenant", actionable: true, evidence: "The inspected tenant isolation boundary requires denial, but this assertion accepts cross-tenant access." }, + { title: "Payment limit test accepts an excessive charge", actionable: true, evidence: "The caller contract caps the amount; the modified assertion accepts a violating charge." }, + { title: "Missing rejection test for a correct authorization guard", actionable: true, evidence: "The established authorization boundary requires rejection. Inspected relevant tests would still pass if its currently correct guard were removed." } + ])("keeps testing findings evidence-based without blanket suppression: $title", async ({ title, actionable, evidence }) => { + // Scripted decisions test the prompt/publication contract, not live model judgment. + const fixture = reviewFixture(["test_boundary.py"]); + const packet = fixture.packets[0]!; + const finding = candidate("test-contract", packet, { category: "testing", title }); + const result = await verifyFindings({ packetResults: [packetResult(packet.id, [finding])], packets: [packet] }, fakeTools(), config(), nullTelemetry(), { + runner: { runStructured: async (request: LlmStructuredRequest) => { + expect(request.prompt).toContain("A missing test for a new branch alone is not a finding"); + expect(request.prompt).toContain("no existing production bug or executed mutation is required"); + expect(request.prompt).toContain("accept valid remedies"); + return { verdict: actionable ? "keep" : "reject", reason: evidence, requiredEvidencePresent: actionable, + falsePositiveRisk: actionable ? "low" : "high", proofAssessment: { status: actionable ? "established" : "refuted", evidence, assumptions: [] } } as T; + } }, promptBuilder: createPromptBuilder(fakeLensRegistry()), lensRegistry: fakeLensRegistry(), diff: fixture.diff + }); + expect(result.verified).toHaveLength(actionable ? 1 : 0); + }); + it.each([ { material: true, title: "Recovery instructions disable authorization on a public listener", reason: "The documented public recovery command disables authorization, contradicting the deployment requirement." },