diff --git a/docs/plans/2026-04-04-template-context-consolidation-design.md b/docs/plans/2026-04-04-template-context-consolidation-design.md new file mode 100644 index 00000000..8eb1659f --- /dev/null +++ b/docs/plans/2026-04-04-template-context-consolidation-design.md @@ -0,0 +1,567 @@ +# Prompt template context consolidation + +## Summary + +Refactor `internal/prompt/` to replace the current set of narrow template-only `...View` structs with a smaller, consolidated template context model. The goal is not to enable user-customizable templates in this change, but to stop baking the current built-in template needs into the shape of the data model so that future customization is not blocked by overly tight, one-off structs. + +This keeps the current direction of making prompt construction template-driven end to end, but changes the internal model from "many tiny wrappers designed around today’s exact templates" to "a broader, intentional prompt context API that built-in templates happen to use first." + +## Goals + +- Reduce the number of narrow prompt/template structs in `internal/prompt/`. +- Introduce a consolidated template-facing context model that can plausibly become the basis for future customizable templates. +- Preserve the hard architectural boundary that git access, DB lookups, config loading, and fallback selection happen before constructing the presentation context. +- Keep fitting, trimming, and fallback selection logic template-driven and free from stringbuilder-style prompt assembly. +- Move prompt-context manipulation helpers toward pointer receiver methods on the consolidated context types where that improves clarity. +- Preserve current prompt behavior for single, range, dirty, address, and system prompt rendering. + +## Non-goals + +- Enabling user-defined templates in this change. +- Using storage / DB structs directly as template inputs. +- Reintroducing map-driven or loosely typed prompt rendering. +- Moving git, DB, or config logic into template context methods. +- Changing prompt wording, trimming priorities, or fallback behavior except where required to preserve current behavior during the refactor. + +## Current problems + +The current template layer in `internal/prompt/prompt_body_templates.go` contains many very small structs: + +- `singlePromptView` +- `rangePromptView` +- `dirtyPromptView` +- `addressPromptView` +- `systemPromptView` +- `optionalSectionsView` +- `currentCommitSectionView` +- `commitRangeSectionView` +- `dirtyChangesSectionView` +- `diffSectionView` +- multiple fallback-specific view structs +- multiple history/comment wrapper structs + +Those types are explicit, which is better than procedural string assembly, but they are too tightly shaped around the current built-in templates. That creates two problems: + +1. The type surface is larger than it needs to be, which makes render/fitting logic harder to understand. +2. The template data contract is too constrained. If templates ever become customizable, users would only be able to access whatever fields the current built-in templates happened to need. + +## Design principles + +### 1. Keep data acquisition separate from presentation + +All git access, DB lookups, config resolution, review history loading, fallback command generation, and review-type normalization must happen before the presentation context is built. + +The architecture should be: + +1. Acquire data from git / config / storage +2. Normalize and derive prompt-specific state +3. Build a consolidated presentation context +4. Render and fit templates from that context + +The presentation context layer must remain pure and deterministic. + +### 2. Use a broader template context, not DB models + +The consolidated model should not use `storage.ReviewJob`, `storage.Review`, `storage.Response`, or other persistence structs directly as the template API. + +Those types describe persisted entities and storage concerns. The template layer needs normalized prompt concepts such as: + +- the current review target +- optional history/context sections +- rendered diff/fallback intent +- trimming state +- prompt metadata used only by prompt/system templates + +The template context should therefore be a prompt-domain model, not a storage-domain model. + +### 3. Consolidate by concern, not by prompt file + +The new model should use one shared root context plus a small number of reusable nested contexts grouped by responsibility. This avoids both extremes: + +- too many one-off view structs +- one giant unstructured god object + +### 4. Put prompt-state behavior next to the prompt-state data + +Context mutation and presentation-oriented helpers should move onto the consolidated types as pointer/value receiver methods when doing so makes the fitting logic clearer. + +Allowed responsibilities for context methods: + +- cloning +- trimming optional sections in priority order +- dropping or blanking range metadata in priority order +- testing whether data is present +- returning derived display text that depends only on already-loaded state + +Forbidden responsibilities for context methods: + +- git access +- DB/storage access +- config loading +- template parsing/execution side effects + +## Proposed context model + +### Single-source-of-truth rule + +Every datum in the consolidated template model must have exactly one home. The same concept must not appear in both `Meta` and a prompt-specific child context, or in both `Diff` and `Fallback`. + +The model should follow this ownership table: + +| Datum | Owner | +|------|-------| +| agent name / prompt kind / review type | `PromptMeta` | +| current date / no-skills instruction | `SystemTemplateContext` | +| single/range/dirty optional review context | `ReviewTemplateContext.Optional` | +| current single/range/dirty subject | `ReviewTemplateContext.Subject` | +| inline diff body | `ReviewTemplateContext.Diff` | +| structured fallback commands / fallback mode | `ReviewTemplateContext.Fallback` | +| address-prompt findings, diff, severity, prior attempts, guidelines, job id | `AddressTemplateContext` | + +That rule is intentionally stricter than the current sketch. The refactor should consolidate the model, not create multiple overlapping sources of truth. + +### Root contexts + +Introduce a small set of reusable root/nested contexts. The exact field names can change during implementation, but the shape should be along these lines: + +```go +type TemplateContext struct { + Meta PromptMeta + Review *ReviewTemplateContext + Address *AddressTemplateContext + System *SystemTemplateContext +} + +type PromptMeta struct { + AgentName string + PromptType string + ReviewType string +} +``` + +The root context intentionally contains more than any one built-in template needs today. That is part of the point: the context should reflect the prompt domain, not only the current layout. + +### Review optional/history context + +```go +type ReviewOptionalContext struct { + ProjectGuidelines *MarkdownSection + AdditionalContext string + PreviousReviews []PreviousReviewTemplateContext + PreviousAttempts []ReviewAttemptTemplateContext +} +``` + +This replaces the current optional section view wrappers with a single reusable review-prompt context. + +Suggested methods: + +```go +func (o *ReviewOptionalContext) TrimNext() bool +func (o ReviewOptionalContext) Clone() ReviewOptionalContext +func (o ReviewOptionalContext) IsEmpty() bool +``` + +`TrimNext` should preserve the current removal priority used by fitting logic. + +### Review template context + +```go +type ReviewTemplateContext struct { + Kind ReviewKind + Optional ReviewOptionalContext + Subject SubjectContext + Diff DiffContext + Fallback FallbackContext +} + +type ReviewKind string + +const ( + ReviewKindSingle ReviewKind = "single" + ReviewKindRange ReviewKind = "range" + ReviewKindDirty ReviewKind = "dirty" +) +``` + +This lets single/range/dirty prompts share a common template-facing shape instead of each needing a separate root view type. + +### Subject context + +```go +type SubjectContext struct { + Single *SingleSubjectContext + Range *RangeSubjectContext + Dirty *DirtySubjectContext +} + +type SingleSubjectContext struct { + Commit string + Subject string + Author string + Message string +} + +type RangeSubjectContext struct { + Count int + Entries []RangeEntryContext +} + +type RangeEntryContext struct { + Commit string + Subject string +} + +type DirtySubjectContext struct { + Description string +} +``` + +Exactly one of `Single`, `Range`, or `Dirty` should be populated, consistent with `ReviewTemplateContext.Kind`. + +### Runtime concept mapping + +To avoid ambiguity, the new model should use the current runtime concepts as follows: + +| Current concept | Meaning | New model field | +|----------------|---------|-----------------| +| `PromptType` | system prompt family / entry-point family (`review`, `dirty`, `range`, `address`, `security`, `design-review`, `run`) | `PromptMeta.PromptType` | +| `ReviewType` | review policy variant (`""`, `security`, `design`, etc.) | `PromptMeta.ReviewType` | +| `ReviewKind` | reviewed artifact shape (`single`, `range`, `dirty`) | `ReviewTemplateContext.Kind` | + +Rules: + +- `PromptMeta.PromptType` must not be used to infer whether the subject is single/range/dirty when `ReviewTemplateContext` is present. +- `ReviewTemplateContext.Kind` applies only to review prompts, never address/system-only prompts. +- `PromptMeta.ReviewType` continues to drive system prompt selection rules, including aliases such as design-review handling. + +Suggested methods: + +```go +func (s SubjectContext) Clone() SubjectContext +func (s *SubjectContext) TrimSingleMessage() bool +func (s *SubjectContext) TrimSingleAuthor() bool +func (s *SubjectContext) TrimSingleSubjectTo(maxBytes int) bool +func (s *SubjectContext) BlankNextRangeSubject() bool +func (s *SubjectContext) DropLastRangeEntry() bool +``` + +The methods should express the current fitting strategy directly instead of scattering equivalent logic across package-level helpers. + +### Diff and fallback contexts + +```go +type DiffContext struct { + Heading string + Body string +} + +type FallbackContext struct { + Mode FallbackMode + Commit *CommitFallbackContext + Range *RangeFallbackContext + Dirty *DirtyFallbackContext + Generic *GenericFallbackContext +} +``` + +The exact nested fallback types can be consolidated further during implementation. The important shift is that fallback-related template data lives under one coherent branch of the context model rather than as several unrelated one-purpose structs. + +Suggested methods: + +```go +func (d DiffContext) Clone() DiffContext +func (d DiffContext) HasBody() bool +func (f FallbackContext) Clone() FallbackContext +func (f FallbackContext) IsEmpty() bool +``` + +Templates should render fallback sections from structured fallback fields, not from a second duplicate markdown string stored beside them. + +### Address and system contexts + +```go +type AddressTemplateContext struct { + ProjectGuidelines *MarkdownSection + PreviousAttempts []AddressAttemptTemplateContext + SeverityFilter string + ReviewFindings string + OriginalDiff string + JobID int64 +} + +type SystemTemplateContext struct { + NoSkillsInstruction string + CurrentDate string +} +``` + +`AddressTemplateContext` and `SystemTemplateContext` remain separate branches inside the one universal `TemplateContext` root because they represent different prompt families, but each datum still lives in only one place. + +### Shared simple types + +```go +type MarkdownSection struct { + Heading string + Body string +} + +type ReviewCommentTemplateContext struct { + Responder string + Response string +} + +type PreviousReviewTemplateContext struct { + Commit string + Output string + Comments []ReviewCommentTemplateContext + Available bool +} + +type ReviewAttemptTemplateContext struct { + Label string + Agent string + When string + Output string + Comments []ReviewCommentTemplateContext +} + +type AddressAttemptTemplateContext struct { + Responder string + Response string + When string +} +``` + +These should replace the repetitive "same shape, different name" wrapper types where possible, and the implementation plan should use these names consistently rather than mixing `...Context`, `...TemplateContext`, and old `...View` names. + +## Behavioral invariants to preserve + +The consolidation must preserve the current prompt-building integration behavior, not merely the current template layout. + +### Guideline source invariants + +- Single and range review prompts must continue to source review guidelines through `LoadGuidelines(repoPath)`, which prefers the repo default branch config and only falls back to working-tree filesystem config when the default branch does not provide `.roborev.toml`. +- Dirty prompts must continue to read guidelines from the working tree via `config.LoadRepoConfig(repoPath)`. +- Address prompts must continue to read guidelines from the working tree via `config.LoadRepoConfig(repoPath)`. +- The consolidation must not normalize these into one generic "guidelines loader" that changes the security behavior for single/range prompts. + +### Explicit ordering invariants + +- Optional review sections must be trimmed in this exact order: previous attempts, previous reviews, additional context, project guidelines. +- Previous review context must render oldest → newest in the final prompt. +- Review attempts and address attempts must preserve their input order when rendered. + +### Address-prompt diff invariant + +- `BuildAddressPrompt` must continue to include the original diff without applying current exclude patterns. This behavior is intentional and is covered by `TestBuildAddressPromptShowsFullDiff`. + +### Ordering and content invariants + +- Previous reviews must continue to render in the same chronological order used by the current prompt package. +- Previous attempts must continue to include stored response comments where they exist. +- Section ordering for single, range, dirty, address, and system prompts must remain stable unless a future spec explicitly changes wording/layout. + +### Size-budget invariants + +- Prompt fitting remains byte-budget based. +- UTF-8 truncation must remain rune-safe. +- Dirty truncated fallback rendering must continue to reserve room for rendered wrapper content such as closing fences and `... (truncated)` markers. +- Hard-capping remains a final guardrail, not the primary fitting strategy. + +## Rendering model + +### Single shared template contract + +All prompt templates should be rendered from the consolidated context model. + +The implementation may still keep convenience wrappers such as: + +- `renderReviewPrompt(...)` +- `renderAddressPrompt(...)` +- `renderSystemPrompt(...)` + +but those wrappers should all delegate to a single shared execution layer that takes the universal `TemplateContext` root. Prompt-family-specific branches (`Review`, `Address`, `System`) are populated as needed for a given template family. + +That means the template API becomes intentional and consistent across: + +- assembled review prompt templates +- shared section partials +- fallback partials +- agent-specific system prompt templates +- default system prompt templates + +### Fallback contract + +The fallback contract needs to stay explicit so the planner does not accidentally reintroduce duplicate representations. + +- Templates should receive structured fallback data via `FallbackContext`. +- Fallback partials should render from that structured data. +- `DiffContext` should contain only the inline diff body that is already ready to render. +- The model should not store the same fallback as both structured fields and a duplicate markdown blob. +- The only pre-rendered fallback body that may remain as template input is the dirty truncated snippet payload when the fitting algorithm has already produced final snippet text that must be wrapped by a template. + +This preserves a single source of truth while still allowing fitting logic to choose among fallback variants before template execution. + +### Context-aware receiver helpers + +Methods on the context types should replace some of the current package-level prompt manipulation helpers. Examples: + +- `ReviewOptionalContext.TrimNext()` +- `SubjectContext.BlankNextRangeSubject()` +- `SubjectContext.DropLastRangeEntry()` +- `TemplateContext.Clone()` +- `ReviewTemplateContext.Clone()` + +The top-level fitting functions should become thin orchestrators that: + +1. clone a context +2. apply one context mutation step +3. re-render +4. repeat until the prompt fits or the hard cap is needed + +## Fitting and selection behavior + +The current fitting behavior should be preserved, but the operations should be expressed through the consolidated contexts. + +### Single prompt fitting + +Preserve the current priority: + +1. trim optional sections +2. drop commit message +3. drop author +4. shrink subject +5. hard cap only as final guardrail + +### Range prompt fitting + +Preserve the current priority: + +1. trim optional sections +2. blank trailing range subjects +3. drop trailing entries +4. only then fall back to final hard cap if needed + +When comparing fallback candidates, selection must still account for how much review context is sacrificed. The current regressions fixed on `gotmpl` should remain covered: + +- isolated candidate evaluation +- metadata-loss-aware comparison +- optional-section-loss-aware comparison + +### Dirty prompt fitting + +Preserve the current behavior for: + +- trimming optional context before over-shrinking dirty fallback snippets +- maintaining the fallback-only path when no snippet fits +- preserving rendered fallback wrappers such as closing fences and truncation markers + +## Migration plan + +### Phase 1: Introduce consolidated contexts + +- Add the new shared context types to `internal/prompt/`. +- Rename or retire the current `prompt.ReviewContext` history type before introducing `ReviewTemplateContext`; the two names must not coexist with ambiguous meaning. Prefer a migration rename such as `HistoricalReviewContext` for the existing storage-backed helper type. +- Add constructors/builders that translate existing gathered prompt data into the new contexts. +- Keep the old render helpers temporarily so behavior can be migrated in small steps. + +### Migration bridge and coexistence rules + +The implementation plan should name the existing types/functions being phased out and the bridge strategy for each: + +- `singlePromptView` +- `rangePromptView` +- `dirtyPromptView` +- `addressPromptView` +- `systemPromptView` +- `optionalSectionsView` +- `diffSectionView` and the fallback-specific view structs +- the current storage-backed `ReviewContext` in `prompt.go` +- render helpers such as `renderSinglePrompt`, `renderRangePrompt`, `renderDirtyPrompt`, `renderAddressPrompt`, and `renderSystemPrompt` + +The bridge should allow incremental migration by keeping wrappers/adapters temporarily, but the target end state should be the consolidated universal `TemplateContext` model described above. + +### Phase 2: Move templates to the new context shape + +- Update assembled prompt templates and partials to use the consolidated context. +- Update system prompt templates to use the same broader model where appropriate. +- Remove reliance on the narrow `...View` roots. + +### Phase 3: Move fitting mutations onto context methods + +- Replace scattered package-level trimming/mutation helpers with context receiver methods. +- Keep builder functions responsible only for orchestration and data acquisition. + +### Phase 4: Delete obsolete narrow types and helpers + +- Remove the old prompt-specific wrappers once templates and fitters no longer depend on them. +- Collapse redundant render helpers that existed only to adapt to the old type surface. + +## Testing strategy + +Keep the current regression-heavy test approach, but shift coverage to the new consolidated model. + +### Rendering tests + +Verify that built-in templates still render the same visible behavior for: + +- single review prompts +- range review prompts +- dirty review prompts +- address prompts +- system prompt templates +- unchanged public builder entry points (`Build`, `BuildWithAdditionalContext`, `BuildDirty`, `BuildAddressPrompt`, `GetSystemPrompt`, and `LoadGuidelines`) + +### Context-method tests + +Add focused tests for receiver methods such as: + +- optional trimming order +- range subject blanking order +- range entry dropping order +- clone isolation +- fallback candidate comparison inputs + +### Regression tests + +Preserve the currently important regressions around: + +- previous review ordering +- previous attempt comments +- single/range guideline sourcing through `LoadGuidelines` +- dirty/address guideline sourcing from working-tree config +- address prompts continuing to show the full original diff regardless of current exclude patterns +- dirty truncated fallback fence preservation +- dirty fallback-only optional restoration +- range diff preservation when entries are trimmed +- fallback richness selection without excessive metadata/optional-context loss +- system prompt template coverage through embedded template discovery +- the explicit optional trimming order: previous attempts → previous reviews → additional context → project guidelines + +## Risks and mitigations + +### Risk: consolidated context turns into a god object + +Mitigation: +- use one shared root but keep concerns grouped into a small number of nested structs +- keep data acquisition out of the context layer +- keep methods presentation-oriented only + +### Risk: templates become harder to read because they see a broader model + +Mitigation: +- keep nested field organization clean and predictable +- prefer small reusable partials +- add helper methods for common presence checks or derived strings + +### Risk: refactor breaks subtle fit behavior + +Mitigation: +- preserve current regression tests +- add direct tests for context mutation methods +- migrate in phases rather than rewriting builder and templates in one step + +## Recommendation + +Implement the consolidation using a shared root context plus a small set of reusable nested prompt-domain contexts. Do not move to raw DB/storage models. Do not move to untyped map-based rendering. Keep all external data loading outside the presentation layer, and make context methods responsible only for deterministic prompt-state manipulation and derived presentation helpers. diff --git a/docs/plans/2026-04-06-template-context-consolidation.md b/docs/plans/2026-04-06-template-context-consolidation.md new file mode 100644 index 00000000..4697a567 --- /dev/null +++ b/docs/plans/2026-04-06-template-context-consolidation.md @@ -0,0 +1,271 @@ +# Prompt Template Context Consolidation Implementation Plan + +> **For agentic workers:** REQUIRED: Use `/skill:orchestrator-implements` (in-session, orchestrator implements), `/skill:subagent-driven-development` (in-session, subagents implement), or `/skill:executing-plans` (parallel session) to implement this plan. Steps use checkbox syntax for tracking. + +**Goal:** Consolidate `internal/prompt` template data into a broader prompt-domain context model without changing prompt behavior, so future customizable templates are not blocked by today’s narrow `...View` structs. + +**Architecture:** Keep git/config/storage lookups in `prompt.go`, normalize them into one universal `TemplateContext` root with prompt-family branches, and make templates plus fitting logic operate on that context and its receiver methods. Migrate incrementally through adapters so behavior stays locked to existing regression tests while the old view structs are removed. + +**Tech Stack:** Go, `text/template`, embedded template files, testify, existing prompt regression tests in `internal/prompt`. + +--- + +### Task 1: Introduce the consolidated prompt template context model + +**TDD scenario:** Modifying tested code — run existing tests first + +**Files:** +- Modify: `internal/prompt/prompt.go` +- Modify: `internal/prompt/prompt_body_templates.go` +- Create: `internal/prompt/template_context.go` +- Test: `internal/prompt/prompt_body_templates_test.go` +- Test: `internal/prompt/prompt_test.go` + +- [ ] **Step 1: Run the current prompt package tests as baseline** + +Run: `go test ./internal/prompt -count=1` +Expected: PASS + +- [ ] **Step 2: Add the consolidated context types and clone/trim receiver methods** + +```go +type TemplateContext struct { + Meta PromptMeta + Review *ReviewTemplateContext + Address *AddressTemplateContext + System *SystemTemplateContext +} + +type ReviewTemplateContext struct { + Kind ReviewKind + Optional ReviewOptionalContext + Subject SubjectContext + Diff DiffContext + Fallback FallbackContext +} +``` + +Include receiver helpers for: +- cloning review/optional/subject context +- optional trim order: previous attempts → previous reviews → additional context → project guidelines +- single-message/author/subject trimming +- range subject blanking and trailing entry dropping + +- [ ] **Step 3: Rename the existing storage-backed `ReviewContext` to avoid the template-context collision** + +```go +type HistoricalReviewContext struct { + SHA string + Review *storage.Review + Responses []storage.Response +} +``` + +Update helper signatures that currently take or return the old `ReviewContext` history type. + +- [ ] **Step 4: Add focused unit coverage for the new receiver methods and naming bridge** + +Add tests that directly assert: +- optional trim order +- clone isolation +- range subject blanking/drop order +- historical review helpers still preserve previous-review ordering + +Run: `go test ./internal/prompt -run 'Test(TemplateContext|ReviewOptionalContext|HistoricalReviewContext|SelectRichestRangePromptView)' -count=1` +Expected: PASS + +- [ ] **Step 5: Commit Task 1** + +```bash +git add internal/prompt/template_context.go internal/prompt/prompt.go internal/prompt/prompt_body_templates.go internal/prompt/prompt_body_templates_test.go internal/prompt/prompt_test.go +git commit -m "refactor(prompt): add consolidated template context model" +``` + +### Task 2: Move rendering and templates onto the universal `TemplateContext` + +**TDD scenario:** Modifying tested code — run existing tests first + +**Files:** +- Modify: `internal/prompt/prompt_body_templates.go` +- Modify: `internal/prompt/templates/prompt_sections.md.gotmpl` +- Modify: `internal/prompt/templates/assembled_single.md.gotmpl` +- Modify: `internal/prompt/templates/assembled_range.md.gotmpl` +- Modify: `internal/prompt/templates/assembled_dirty.md.gotmpl` +- Modify: `internal/prompt/templates/assembled_address.md.gotmpl` +- Modify: `internal/prompt/templates.go` +- Test: `internal/prompt/prompt_body_templates_test.go` +- Test: `internal/prompt/templates_test.go` + +- [ ] **Step 1: Add rendering tests that exercise the new nested template field paths** + +Cover at least: +- review templates render through `.Review.Optional`, `.Review.Subject`, `.Review.Diff`, `.Review.Fallback` +- address templates render through `.Address.*` +- system templates render through `.System.*` + +Run: `go test ./internal/prompt -run 'TestRender(SystemPrompt|SinglePrompt|RangePrompt|DirtyPrompt|AddressPrompt)' -count=1` +Expected: FAIL where templates still expect the old root view shapes + +- [ ] **Step 2: Update render helpers to accept the universal root context** + +```go +func renderReviewPrompt(name string, ctx TemplateContext) (string, error) +func renderAddressPrompt(ctx TemplateContext) (string, error) +func renderSystemPrompt(name string, ctx TemplateContext) (string, error) +``` + +Keep wrappers if they improve readability, but all template execution should take `TemplateContext`. + +- [ ] **Step 3: Rewrite built-in templates to use the new shared root contract** + +Examples: + +```gotmpl +{{template "optional_sections" .Review.Optional}} +{{template "current_commit" .Review.Subject.Single}} +{{template "diff_block" .Review}} +``` + +and: + +```gotmpl +{{template "address_attempts" .Address}} +{{template "address_findings" .Address}} +``` + +- [ ] **Step 4: Preserve the explicit fallback contract** + +Update fallback partials so they render from structured fallback fields under `.Review.Fallback`, not duplicate ad hoc strings stored on multiple structs. + +- [ ] **Step 5: Run rendering/system-template regression coverage** + +Run: `go test ./internal/prompt -run 'TestRender|TestRenderSystemPrompt_AllEmbeddedAgentSpecificTemplates' -count=1` +Expected: PASS + +- [ ] **Step 6: Commit Task 2** + +```bash +git add internal/prompt/prompt_body_templates.go internal/prompt/templates/prompt_sections.md.gotmpl internal/prompt/templates/assembled_single.md.gotmpl internal/prompt/templates/assembled_range.md.gotmpl internal/prompt/templates/assembled_dirty.md.gotmpl internal/prompt/templates/assembled_address.md.gotmpl internal/prompt/templates.go internal/prompt/prompt_body_templates_test.go internal/prompt/templates_test.go +git commit -m "refactor(prompt): route templates through unified context" +``` + +### Task 3: Migrate builders and fitters to mutate the consolidated context + +**TDD scenario:** Modifying tested code — run existing tests first + +**Files:** +- Modify: `internal/prompt/prompt.go` +- Modify: `internal/prompt/prompt_body_templates.go` +- Test: `internal/prompt/prompt_test.go` +- Test: `internal/prompt/prompt_body_templates_test.go` + +- [ ] **Step 1: Add/refresh builder-level regressions around preserved behavior** + +Ensure coverage still explicitly checks: +- single/range guidelines come from `LoadGuidelines` +- dirty/address guidelines use working-tree config +- address prompts still include the full original diff without current excludes +- dirty truncated fallback and range fallback selection behavior remain unchanged + +Run: `go test ./internal/prompt -run 'Test(Build|LoadGuidelines|SelectRichestRangePromptView|FitRangePrompt|BuildAddressPrompt)' -count=1` +Expected: FAIL anywhere the new context migration breaks preserved behavior + +- [ ] **Step 2: Replace old view assembly in builder code with context constructors** + +Introduce helpers that build a `TemplateContext` from already loaded data, for example: + +```go +func buildSingleTemplateContext(...) TemplateContext +func buildRangeTemplateContext(...) TemplateContext +func buildDirtyTemplateContext(...) TemplateContext +func buildAddressTemplateContext(...) TemplateContext +func buildSystemTemplateContext(...) TemplateContext +``` + +These helpers must receive already-loaded git/config/storage data and must not perform I/O themselves. + +- [ ] **Step 3: Move fitting mutations onto context receiver methods** + +Keep orchestration in `prompt.go`, but drive mutations through methods like: +- `ctx.Review.Optional.TrimNext()` +- `ctx.Review.Subject.TrimSingleMessage()` +- `ctx.Review.Subject.BlankNextRangeSubject()` +- `ctx.Review.Subject.DropLastRangeEntry()` + +- [ ] **Step 4: Re-run the full prompt package suite** + +Run: `go test ./internal/prompt -count=1` +Expected: PASS + +- [ ] **Step 5: Commit Task 3** + +```bash +git add internal/prompt/prompt.go internal/prompt/prompt_body_templates.go internal/prompt/prompt_test.go internal/prompt/prompt_body_templates_test.go +git commit -m "refactor(prompt): fit prompts from consolidated context" +``` + +### Task 4: Remove obsolete narrow views and run full verification + +**TDD scenario:** Modifying tested code — run existing tests first + +**Files:** +- Modify: `internal/prompt/prompt_body_templates.go` +- Modify: `internal/prompt/prompt.go` +- Modify: `internal/prompt/templates_test.go` +- Modify: `internal/prompt/prompt_body_templates_test.go` +- Modify: `internal/prompt/prompt_test.go` + +- [ ] **Step 1: Delete the obsolete narrow template view types and dead adapters** + +Remove the old prompt-specific wrappers once all callers/templates use `TemplateContext`: +- `singlePromptView` +- `rangePromptView` +- `dirtyPromptView` +- `addressPromptView` +- `systemPromptView` +- `optionalSectionsView` +- `diffSectionView` +- fallback-specific one-off view structs no longer needed + +- [ ] **Step 2: Remove any render helpers that only existed for the old shape** + +Keep only the helpers needed by the consolidated context contract and current tests. + +- [ ] **Step 3: Run repo verification** + +Run: +- `go fmt ./...` +- `go vet ./...` +- `go test ./...` + +Expected: PASS + +- [ ] **Step 4: Commit Task 4** + +```bash +git add internal/prompt/prompt_body_templates.go internal/prompt/prompt.go internal/prompt/templates_test.go internal/prompt/prompt_body_templates_test.go internal/prompt/prompt_test.go +git commit -m "refactor(prompt): remove narrow template view wrappers" +``` + +### Task 5: Final review and landing prep + +**TDD scenario:** Trivial change — use judgment + +**Files:** +- Modify: `docs/plans/2026-04-06-template-context-consolidation.md` (only if implementation notes are required) + +- [ ] **Step 1: Review the final diff for accidental behavior changes** + +Run: `git diff origin/main...HEAD -- internal/prompt` +Expected: only the intended context consolidation and test/template updates + +- [ ] **Step 2: Request a code review if the subagent harness cooperates; otherwise note the timeout block in the handoff summary** + +Run a reviewer or capture the blocker explicitly. + +- [ ] **Step 3: Commit any final cleanups** + +```bash +git add -A +git commit -m "chore(prompt): finalize template context consolidation" || true +``` diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index 57beebf3..8bc57989 100644 --- a/internal/prompt/prompt.go +++ b/internal/prompt/prompt.go @@ -34,8 +34,8 @@ IMPORTANT: You are being invoked by roborev to perform this review directly. Do Return only the final review content. Do NOT include process narration, progress updates, or front matter such as "Reviewing the diff..." or "I'm checking...". If you use tools while reviewing, finish all tool use before emitting the final review, and put the final review only after the last tool call.` -// ReviewContext holds a commit SHA and its associated review (if any) plus responses -type ReviewContext struct { +// HistoricalReviewContext holds a commit SHA and its associated review (if any) plus responses. +type HistoricalReviewContext struct { SHA string Review *storage.Review Responses []storage.Response @@ -91,66 +91,50 @@ func (b *Builder) Build(repoPath, gitRef string, repoID int64, contextCount int, // BuildWithAdditionalContext constructs a review prompt with an optional // caller-provided markdown context block inserted ahead of the current diff. func (b *Builder) BuildWithAdditionalContext(repoPath, gitRef string, repoID int64, contextCount int, agentName, reviewType, minSeverity, additionalContext string) (string, error) { - opts := buildOpts{ + return b.buildWithOpts(repoPath, gitRef, repoID, contextCount, agentName, reviewType, buildOpts{ additionalContext: additionalContext, minSeverity: minSeverity, - } - if git.IsRange(gitRef) { - return b.buildRangePrompt(repoPath, gitRef, repoID, contextCount, agentName, reviewType, opts) - } - return b.buildSinglePrompt(repoPath, gitRef, repoID, contextCount, agentName, reviewType, opts) + }) } // BuildWithAdditionalContextAndDiffFile constructs a review prompt with -// caller-provided markdown context and an optional oversized-diff file -// reference for sandboxed Codex reviews. +// caller-provided markdown context and an optional oversized-diff file reference. func (b *Builder) BuildWithAdditionalContextAndDiffFile(repoPath, gitRef string, repoID int64, contextCount int, agentName, reviewType, minSeverity, additionalContext, diffFilePath string) (string, error) { - opts := buildOpts{ + return b.buildWithOpts(repoPath, gitRef, repoID, contextCount, agentName, reviewType, buildOpts{ additionalContext: additionalContext, diffFilePath: diffFilePath, requireDiffFile: true, minSeverity: minSeverity, - } - if git.IsRange(gitRef) { - return b.buildRangePrompt(repoPath, gitRef, repoID, contextCount, agentName, reviewType, opts) - } - return b.buildSinglePrompt(repoPath, gitRef, repoID, contextCount, agentName, reviewType, opts) + }) } -// BuildWithDiffFile constructs a review prompt where a pre-written diff -// file is referenced for large diffs instead of git commands. This is -// used for Codex agents running in a sandboxed environment that cannot -// execute git directly. +// BuildWithDiffFile constructs a review prompt where a pre-written diff file +// is referenced for large diffs instead of inline content. func (b *Builder) BuildWithDiffFile(repoPath, gitRef string, repoID int64, contextCount int, agentName, reviewType, minSeverity, diffFilePath string) (string, error) { - opts := buildOpts{ + return b.buildWithOpts(repoPath, gitRef, repoID, contextCount, agentName, reviewType, buildOpts{ diffFilePath: diffFilePath, requireDiffFile: true, minSeverity: minSeverity, - } + }) +} + +func (b *Builder) buildWithOpts(repoPath, gitRef string, repoID int64, contextCount int, agentName, reviewType string, opts buildOpts) (string, error) { if git.IsRange(gitRef) { return b.buildRangePrompt(repoPath, gitRef, repoID, contextCount, agentName, reviewType, opts) } return b.buildSinglePrompt(repoPath, gitRef, repoID, contextCount, agentName, reviewType, opts) } -// SnapshotResult holds a prompt and an optional cleanup function for -// a diff snapshot file that was written during prompt construction. +// SnapshotResult holds a prompt and an optional cleanup function for a diff snapshot file. type SnapshotResult struct { Prompt string Cleanup func() } -// BuildWithSnapshot builds a review prompt, automatically writing a -// diff snapshot file when the diff is too large to inline. -func (b *Builder) BuildWithSnapshot( - repoPath, gitRef string, repoID int64, - contextCount int, agentName, reviewType, minSeverity string, - excludes []string, -) (SnapshotResult, error) { - p, err := b.BuildWithDiffFile( - repoPath, gitRef, repoID, - contextCount, agentName, reviewType, minSeverity, "", - ) +// BuildWithSnapshot builds a review prompt, automatically writing a diff snapshot file +// when the diff is too large to inline. +func (b *Builder) BuildWithSnapshot(repoPath, gitRef string, repoID int64, contextCount int, agentName, reviewType, minSeverity string, excludes []string) (SnapshotResult, error) { + p, err := b.BuildWithDiffFile(repoPath, gitRef, repoID, contextCount, agentName, reviewType, minSeverity, "") if !errors.Is(err, ErrDiffTruncatedNoFile) { return SnapshotResult{Prompt: p}, err } @@ -158,10 +142,7 @@ func (b *Builder) BuildWithSnapshot( if writeErr != nil { return SnapshotResult{}, fmt.Errorf("write diff snapshot: %w", writeErr) } - p, err = b.BuildWithDiffFile( - repoPath, gitRef, repoID, - contextCount, agentName, reviewType, minSeverity, diffFile, - ) + p, err = b.BuildWithDiffFile(repoPath, gitRef, repoID, contextCount, agentName, reviewType, minSeverity, diffFile) if err != nil { cleanup() return SnapshotResult{}, err @@ -169,8 +150,7 @@ func (b *Builder) BuildWithSnapshot( return SnapshotResult{Prompt: p, Cleanup: cleanup}, nil } -// WriteDiffSnapshot writes the full diff for a git ref to a file in -// the repo's git dir. Returns the file path and a cleanup function. +// WriteDiffSnapshot writes the full diff for a git ref to a file in the repo's git dir. func WriteDiffSnapshot(repoPath, gitRef string, excludes []string) (string, func(), error) { var ( fullDiff string @@ -208,13 +188,9 @@ func WriteDiffSnapshot(repoPath, gitRef string, excludes []string) (string, func return diffFile, func() { os.Remove(diffFile) }, nil } -// BuildDirtyWithSnapshot builds a dirty review prompt, writing the diff -// to a snapshot file when it's too large to inline. The caller must -// call Cleanup (if non-nil) after the prompt is no longer needed. -func (b *Builder) BuildDirtyWithSnapshot( - repoPath, diff string, repoID int64, - contextCount int, agentName, reviewType, minSeverity string, -) (SnapshotResult, error) { +// BuildDirtyWithSnapshot builds a dirty review prompt, writing the diff to a snapshot file +// when it's too large to inline. +func (b *Builder) BuildDirtyWithSnapshot(repoPath, diff string, repoID int64, contextCount int, agentName, reviewType, minSeverity string) (SnapshotResult, error) { p, err := b.BuildDirty(repoPath, diff, repoID, contextCount, agentName, reviewType, minSeverity) if err != nil { return SnapshotResult{}, err @@ -248,26 +224,11 @@ func (b *Builder) BuildDirtyWithSnapshot( // The diff is provided directly since it was captured at enqueue time. // reviewType selects the system prompt variant (e.g., "security"); any default alias (see config.IsDefaultReviewType) uses the standard prompt. func (b *Builder) BuildDirty(repoPath, diff string, repoID int64, contextCount int, agentName, reviewType, minSeverity string) (string, error) { - // Start with system prompt for dirty changes - promptType := "dirty" - if !config.IsDefaultReviewType(reviewType) { - promptType = reviewType - } - if promptType == config.ReviewTypeDesign { - promptType = "design-review" - } - promptCap := b.resolveMaxPromptSize(repoPath) - requiredPrefix := GetSystemPrompt(agentName, promptType) + "\n" - if inst := config.SeverityInstruction(minSeverity); inst != "" { - requiredPrefix += inst + "\n" - } - requiredPrefix = hardCapPrompt(requiredPrefix, promptCap) - - optional := optionalSectionsView{} + ctx := b.newPromptBuildContext(repoPath, agentName, reviewType, minSeverity, "dirty", optionalSectionsView{}) // Add project-specific guidelines if configured if repoCfg, err := config.LoadRepoConfig(repoPath); err == nil && repoCfg != nil { - optional.ProjectGuidelines = buildProjectGuidelinesSectionView(repoCfg.ReviewGuidelines) + ctx.optional.ProjectGuidelines = buildProjectGuidelinesSectionView(repoCfg.ReviewGuidelines) } // Get previous reviews for context (use HEAD as reference point) @@ -276,18 +237,18 @@ func (b *Builder) BuildDirty(repoPath, diff string, repoID int64, contextCount i if err == nil { contexts, err := b.getPreviousReviewContexts(repoPath, headSHA, contextCount) if err == nil && len(contexts) > 0 { - optional.PreviousReviews = orderedPreviousReviewViews(contexts) + ctx.optional.PreviousReviews = orderedPreviousReviewViews(contexts) } } } - bodyLimit := max(0, promptCap-len(requiredPrefix)) + bodyLimit := max(0, ctx.promptCap-len(ctx.requiredPrefix)) inlineDiff, err := renderInlineDiff(diff) if err != nil { return "", err } view := dirtyPromptView{ - Optional: optional, + Optional: ctx.optional, Current: dirtyChangesSectionView{ Description: "The following changes have not yet been committed.", }, @@ -380,11 +341,15 @@ func (b *Builder) BuildDirty(repoPath, diff string, repoID int64, contextCount i } } - body, err := fitDirtyPrompt(bodyLimit, view) + body, err := fitDirtyPromptContext(bodyLimit, templateContextFromDirtyView(view)) if err != nil { return "", err } - return requiredPrefix + hardCapPrompt(body, bodyLimit), nil + return ctx.requiredPrefix + hardCapPrompt(body, bodyLimit), nil +} + +func isCodexReviewAgent(agentName string) bool { + return strings.EqualFold(strings.TrimSpace(agentName), "codex") } func truncateUTF8(s string, maxBytes int) string { @@ -410,8 +375,6 @@ func hardCapPrompt(prompt string, limit int) string { return truncateUTF8(prompt, limit) } -// buildOpts groups optional parameters for buildSinglePrompt and -// buildRangePrompt to keep the positional parameter count manageable. type buildOpts struct { additionalContext string // diffFilePath, when non-empty, is a file containing the full @@ -426,9 +389,39 @@ type buildOpts struct { minSeverity string } -// diffFileFallbackVariants returns progressively shorter prompt -// variants for oversized diffs. When filePath is non-empty, the -// variants reference the file; otherwise they just note truncation. +type promptBuildContext struct { + requiredPrefix string + optional optionalSectionsView + promptCap int +} + +func (b *Builder) newPromptBuildContext(repoPath, agentName, reviewType, minSeverity, defaultPromptType string, optional optionalSectionsView) promptBuildContext { + promptType := defaultPromptType + if !config.IsDefaultReviewType(reviewType) { + promptType = reviewType + } + if promptType == config.ReviewTypeDesign { + promptType = "design-review" + } + promptCap := b.resolveMaxPromptSize(repoPath) + requiredPrefix := GetSystemPrompt(agentName, promptType) + "\n" + if inst := config.SeverityInstruction(minSeverity); inst != "" { + requiredPrefix += inst + "\n" + } + return promptBuildContext{ + requiredPrefix: hardCapPrompt(requiredPrefix, promptCap), + optional: optional, + promptCap: promptCap, + } +} + +func defaultOptionalSections(repoPath, additionalContext string) optionalSectionsView { + return optionalSectionsView{ + ProjectGuidelines: buildProjectGuidelinesSectionView(LoadGuidelines(repoPath)), + AdditionalContext: buildAdditionalContextSection(additionalContext), + } +} + func diffFileFallbackVariants(heading, filePath string) []string { if filePath == "" { return []string{heading + "\n\n(Diff too large to include inline)\n"} @@ -486,38 +479,238 @@ func buildPromptPreservingCurrentSection(requiredPrefix, optionalContext, curren return hardCapPrompt(sb.String(), limit) } -// buildSinglePrompt constructs a prompt for a single commit -func (b *Builder) buildSinglePrompt(repoPath, sha string, repoID int64, contextCount int, agentName, reviewType string, opts buildOpts) (string, error) { - // Start with system prompt - promptType := "review" - if !config.IsDefaultReviewType(reviewType) { - promptType = reviewType +// safeForMarkdown filters pathspec args to only those that can be +// safely embedded in markdown inline code spans. Args containing +// backticks or control characters are dropped. +func safeForMarkdown(args []string) []string { + var safe []string + for _, a := range args { + ok := true + for _, r := range a { + if r < ' ' || r == '`' || r == 0x7f { + ok = false + break + } + } + if ok { + safe = append(safe, a) + } } - if promptType == config.ReviewTypeDesign { - promptType = "design-review" + return safe +} + +func shellQuote(s string) string { + if s == "" { + return "''" } - promptCap := b.resolveMaxPromptSize(repoPath) - requiredPrefix := GetSystemPrompt(agentName, promptType) + "\n" - if inst := config.SeverityInstruction(opts.minSeverity); inst != "" { - requiredPrefix += inst + "\n" + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +func renderShellCommand(args ...string) string { + var quoted []string + for _, arg := range args { + if needsShellQuoting(arg) { + quoted = append(quoted, shellQuote(arg)) + continue + } + quoted = append(quoted, arg) } - requiredPrefix = hardCapPrompt(requiredPrefix, promptCap) + return strings.Join(quoted, " ") +} - optional := optionalSectionsView{ - ProjectGuidelines: buildProjectGuidelinesSectionView(LoadGuidelines(repoPath)), - AdditionalContext: buildAdditionalContextSection(opts.additionalContext), +func needsShellQuoting(s string) bool { + if s == "" { + return true + } + for _, r := range s { + switch { + case r >= 'a' && r <= 'z': + case r >= 'A' && r <= 'Z': + case r >= '0' && r <= '9': + case strings.ContainsRune("@%_+=:,./-~", r): + default: + return true + } } + return false +} + +func codexCommitInspectionFallbackVariants(sha string, pathspecArgs []string) []diffSectionView { + view := commitInspectionFallbackView{ + SHA: sha, + StatCmd: renderShellCommand(append([]string{"git", "show", "--stat", "--summary", sha, "--"}, pathspecArgs...)...), + DiffCmd: renderShellCommand(append([]string{"git", "show", "--format=medium", "--unified=80", sha, "--"}, pathspecArgs...)...), + FilesCmd: renderShellCommand(append([]string{"git", "diff-tree", "--no-commit-id", "--name-only", "-r", sha, "--"}, pathspecArgs...)...), + ShowPathCmd: renderShellCommand(append([]string{"git", "show", sha, "--"}, pathspecArgs...)...), + } + names := []string{"codex_commit_fallback_full", "codex_commit_fallback_medium", "codex_commit_fallback_short", "codex_commit_fallback_shortest"} + variants := make([]diffSectionView, 0, len(names)) + for _, name := range names { + fallback, err := renderCommitInspectionFallback(name, view) + if err != nil { + continue + } + variants = append(variants, diffSectionView{Heading: "### Diff", Fallback: fallback}) + } + return variants +} + +func codexRangeInspectionFallbackVariants(rangeRef string, pathspecArgs []string) []diffSectionView { + view := rangeInspectionFallbackView{ + RangeRef: rangeRef, + LogCmd: renderShellCommand("git", "log", "--oneline", rangeRef), + StatCmd: renderShellCommand(append([]string{"git", "diff", "--stat", rangeRef, "--"}, pathspecArgs...)...), + DiffCmd: renderShellCommand(append([]string{"git", "diff", "--unified=80", rangeRef, "--"}, pathspecArgs...)...), + FilesCmd: renderShellCommand(append([]string{"git", "diff", "--name-only", rangeRef, "--"}, pathspecArgs...)...), + ViewCmd: renderShellCommand(append([]string{"git", "diff", rangeRef, "--"}, pathspecArgs...)...), + } + names := []string{"codex_range_fallback_full", "codex_range_fallback_medium", "codex_range_fallback_short", "codex_range_fallback_shortest"} + variants := make([]diffSectionView, 0, len(names)) + for _, name := range names { + fallback, err := renderRangeInspectionFallback(name, view) + if err != nil { + continue + } + variants = append(variants, diffSectionView{Heading: "### Combined Diff", Fallback: fallback}) + } + return variants +} + +func selectDiffSectionVariant(variants []diffSectionView, remaining int) (diffSectionView, error) { + if len(variants) == 0 { + return diffSectionView{}, nil + } + selected := variants[len(variants)-1] + for _, variant := range variants { + block, err := renderDiffBlock(variant) + if err != nil { + return diffSectionView{}, err + } + if len(block) <= remaining { + return variant, nil + } + } + return truncateDiffSectionFallbackToFit(selected, remaining) +} + +func truncateDiffSectionFallbackToFit(view diffSectionView, limit int) (diffSectionView, error) { + block, err := renderDiffBlock(view) + if err != nil || len(block) <= limit { + return view, err + } + baseBlock, err := renderDiffBlock(diffSectionView{Heading: view.Heading, Body: ""}) + if err != nil { + return diffSectionView{}, err + } + view.Fallback = truncateUTF8(view.Fallback, max(0, limit-len(baseBlock))) + return view, nil +} + +type rangeMetadataLoss struct { + RemovedEntries int + BlankedSubject int + TrimmedOptional int +} + +func compareRangeMetadataLoss(a, b rangeMetadataLoss) int { + switch { + case a.RemovedEntries != b.RemovedEntries: + return a.RemovedEntries - b.RemovedEntries + case a.BlankedSubject != b.BlankedSubject: + return a.BlankedSubject - b.BlankedSubject + default: + return a.TrimmedOptional - b.TrimmedOptional + } +} + +func measureOptionalSectionsLoss(original, trimmed ReviewOptionalContext) int { + loss := 0 + if len(original.PreviousAttempts) > 0 && len(trimmed.PreviousAttempts) == 0 { + loss++ + } + if len(original.PreviousReviews) > 0 && len(trimmed.PreviousReviews) == 0 { + loss++ + } + if original.AdditionalContext != "" && trimmed.AdditionalContext == "" { + loss++ + } + if original.ProjectGuidelines != nil && trimmed.ProjectGuidelines == nil { + loss++ + } + return loss +} + +func measureRangeMetadataLoss(original, trimmed TemplateContext) rangeMetadataLoss { + if original.Review == nil || trimmed.Review == nil || original.Review.Subject.Range == nil || trimmed.Review.Subject.Range == nil { + return rangeMetadataLoss{} + } + loss := rangeMetadataLoss{ + RemovedEntries: len(original.Review.Subject.Range.Entries) - len(trimmed.Review.Subject.Range.Entries), + TrimmedOptional: measureOptionalSectionsLoss(original.Review.Optional, trimmed.Review.Optional), + } + for i := range trimmed.Review.Subject.Range.Entries { + if i >= len(original.Review.Subject.Range.Entries) { + break + } + if original.Review.Subject.Range.Entries[i].Subject != "" && trimmed.Review.Subject.Range.Entries[i].Subject == "" { + loss.BlankedSubject++ + } + } + return loss +} + +func selectRichestRangePromptView(limit int, view TemplateContext, variants []diffSectionView) (TemplateContext, error) { + fallback := view.Clone() + if len(variants) > 0 && fallback.Review != nil { + fallback.Review.Diff = DiffContext{Heading: variants[len(variants)-1].Heading, Body: variants[len(variants)-1].Body} + fallback.Review.Fallback = fallbackContextFromDiffSection(variants[len(variants)-1]) + } + var ( + best TemplateContext + bestLoss rangeMetadataLoss + haveBest bool + ) + for _, variant := range variants { + candidate := view.Clone() + if candidate.Review != nil { + candidate.Review.Diff = DiffContext{Heading: variant.Heading, Body: variant.Body} + candidate.Review.Fallback = fallbackContextFromDiffSection(variant) + } + trimmed, body, err := trimRangePromptContext(limit, candidate) + if err != nil { + return TemplateContext{}, err + } + fallback = trimmed + if len(body) > limit { + continue + } + loss := measureRangeMetadataLoss(view, trimmed) + if !haveBest || compareRangeMetadataLoss(loss, bestLoss) < 0 { + best = trimmed + bestLoss = loss + haveBest = true + } + } + if haveBest { + return best, nil + } + return fallback, nil +} + +// buildSinglePrompt constructs a prompt for a single commit +func (b *Builder) buildSinglePrompt(repoPath, sha string, repoID int64, contextCount int, agentName, reviewType string, opts buildOpts) (string, error) { + ctx := b.newPromptBuildContext(repoPath, agentName, reviewType, opts.minSeverity, "review", defaultOptionalSections(repoPath, opts.additionalContext)) // Get previous reviews if requested if contextCount > 0 && b.db != nil { contexts, err := b.getPreviousReviewContexts(repoPath, sha, contextCount) if err == nil && len(contexts) > 0 { - optional.PreviousReviews = orderedPreviousReviewViews(contexts) + ctx.optional.PreviousReviews = orderedPreviousReviewViews(contexts) } } // Include previous review attempts for this same commit (for re-reviews) - optional.PreviousAttempts = previousAttemptViewsFromContexts(b.previousAttemptContexts(sha)) + ctx.optional.PreviousAttempts = previousAttemptViewsFromContexts(b.previousAttemptContexts(sha)) // Current commit section shortSHA := git.ShortSHA(sha) @@ -552,7 +745,7 @@ func (b *Builder) buildSinglePrompt(repoPath, sha string, repoID int64, contextC } excludes := b.resolveExcludes(repoPath, reviewType) - bodyLimit := max(0, promptCap-len(requiredPrefix)) + bodyLimit := max(0, ctx.promptCap-len(ctx.requiredPrefix)) diffLimit := max(0, bodyLimit-len(currentRequired)-len(currentOverflow)-len(emptyDiffBlock)) diff, truncated, err := git.GetDiffLimited(repoPath, sha, diffLimit, excludes...) if err != nil { @@ -561,21 +754,42 @@ func (b *Builder) buildSinglePrompt(repoPath, sha string, repoID int64, contextC diffView := diffSectionView{Heading: "### Diff"} if truncated { - if opts.diffFilePath == "" && opts.requireDiffFile { - return "", ErrDiffTruncatedNoFile + if opts.diffFilePath != "" || opts.requireDiffFile { + if opts.diffFilePath == "" && opts.requireDiffFile { + return "", ErrDiffTruncatedNoFile + } + optionalPrefix, err := renderOptionalSectionsPrefix(ctx.optional) + if err != nil { + return "", err + } + return buildPromptPreservingCurrentSection(ctx.requiredPrefix, optionalPrefix, currentRequired, currentOverflow, ctx.promptCap, diffFileFallbackVariants("### Diff", opts.diffFilePath)...), nil } - optionalPrefix, err := renderOptionalSectionsPrefix(optional) - if err != nil { - return "", err + pathspecArgs := safeForMarkdown(git.FormatExcludeArgs(excludes)) + if isCodexReviewAgent(agentName) { + variants := codexCommitInspectionFallbackVariants(sha, pathspecArgs) + shortestBlock, err := renderDiffBlock(variants[len(variants)-1]) + if err != nil { + return "", err + } + optionalPrefix, err := renderOptionalSectionsPrefix(ctx.optional) + if err != nil { + return "", err + } + softBudget := max(0, bodyLimit-len(currentRequired)-len(shortestBlock)) + softLen := len(optionalPrefix) + len(currentOverflow) + effectiveSoftLen := min(softLen, softBudget) + remaining := max(0, bodyLimit-len(currentRequired)-effectiveSoftLen) + diffView, err = selectDiffSectionVariant(variants, remaining) + if err != nil { + return "", err + } + } else { + fallback, err := renderGenericCommitFallback(renderShellCommand("git", "show", sha)) + if err != nil { + return "", err + } + diffView.Fallback = fallback } - return buildPromptPreservingCurrentSection( - requiredPrefix, - optionalPrefix, - currentRequired, - currentOverflow, - promptCap, - diffFileFallbackVariants("### Diff", opts.diffFilePath)..., - ), nil } else { inlineDiff, err := renderInlineDiff(diff) if err != nil { @@ -584,41 +798,23 @@ func (b *Builder) buildSinglePrompt(repoPath, sha string, repoID int64, contextC diffView.Body = inlineDiff } - body, err := fitSinglePrompt( + body, err := fitSinglePromptContext( bodyLimit, - singlePromptView{ - Optional: optional, + templateContextFromSingleView(singlePromptView{ + Optional: ctx.optional, Current: currentView, Diff: diffView, - }, + }), ) if err != nil { return "", err } - return requiredPrefix + body, nil + return ctx.requiredPrefix + body, nil } // buildRangePrompt constructs a prompt for a commit range func (b *Builder) buildRangePrompt(repoPath, rangeRef string, repoID int64, contextCount int, agentName, reviewType string, opts buildOpts) (string, error) { - // Start with system prompt for ranges - promptType := "range" - if !config.IsDefaultReviewType(reviewType) { - promptType = reviewType - } - if promptType == config.ReviewTypeDesign { - promptType = "design-review" - } - promptCap := b.resolveMaxPromptSize(repoPath) - requiredPrefix := GetSystemPrompt(agentName, promptType) + "\n" - if inst := config.SeverityInstruction(opts.minSeverity); inst != "" { - requiredPrefix += inst + "\n" - } - requiredPrefix = hardCapPrompt(requiredPrefix, promptCap) - - optional := optionalSectionsView{ - ProjectGuidelines: buildProjectGuidelinesSectionView(LoadGuidelines(repoPath)), - AdditionalContext: buildAdditionalContextSection(opts.additionalContext), - } + ctx := b.newPromptBuildContext(repoPath, agentName, reviewType, opts.minSeverity, "range", defaultOptionalSections(repoPath, opts.additionalContext)) // Get previous reviews from before the range start if contextCount > 0 && b.db != nil { @@ -626,13 +822,13 @@ func (b *Builder) buildRangePrompt(repoPath, rangeRef string, repoID int64, cont if err == nil { contexts, err := b.getPreviousReviewContexts(repoPath, startSHA, contextCount) if err == nil && len(contexts) > 0 { - optional.PreviousReviews = orderedPreviousReviewViews(contexts) + ctx.optional.PreviousReviews = orderedPreviousReviewViews(contexts) } } } // Include previous review attempts for this same range (for re-reviews) - optional.PreviousAttempts = previousAttemptViewsFromContexts(b.previousAttemptContexts(rangeRef)) + ctx.optional.PreviousAttempts = previousAttemptViewsFromContexts(b.previousAttemptContexts(rangeRef)) // Get commits in range commits, err := git.GetRangeCommits(repoPath, rangeRef) @@ -640,8 +836,6 @@ func (b *Builder) buildRangePrompt(repoPath, rangeRef string, repoID int64, cont return "", fmt.Errorf("get range commits: %w", err) } - optional.InRangeReviews = inRangeReviewViews(b.lookupReviewContexts(commits, true)) - entries := make([]commitRangeEntryView, 0, len(commits)) for _, commitSHA := range commits { short := git.ShortSHA(commitSHA) @@ -671,7 +865,7 @@ func (b *Builder) buildRangePrompt(repoPath, rangeRef string, repoID int64, cont } excludes := b.resolveExcludes(repoPath, reviewType) - bodyLimit := max(0, promptCap-len(requiredPrefix)) + bodyLimit := max(0, ctx.promptCap-len(ctx.requiredPrefix)) diffLimit := max(0, bodyLimit-len(currentRequiredText)-len(currentOverflowText)-len(emptyDiffBlock)) diff, truncated, err := git.GetRangeDiffLimited(repoPath, rangeRef, diffLimit, excludes...) if err != nil { @@ -680,21 +874,49 @@ func (b *Builder) buildRangePrompt(repoPath, rangeRef string, repoID int64, cont diffView := diffSectionView{Heading: "### Combined Diff"} if truncated { - if opts.diffFilePath == "" && opts.requireDiffFile { - return "", ErrDiffTruncatedNoFile + if opts.diffFilePath != "" || opts.requireDiffFile { + if opts.diffFilePath == "" && opts.requireDiffFile { + return "", ErrDiffTruncatedNoFile + } + optionalPrefix, err := renderOptionalSectionsPrefix(ctx.optional) + if err != nil { + return "", err + } + return buildPromptPreservingCurrentSection(ctx.requiredPrefix, optionalPrefix, currentRequiredText, currentOverflowText, ctx.promptCap, diffFileFallbackVariants("### Combined Diff", opts.diffFilePath)...), nil } - optionalPrefix, err := renderOptionalSectionsPrefix(optional) - if err != nil { - return "", err + pathspecArgs := safeForMarkdown(git.FormatExcludeArgs(excludes)) + if isCodexReviewAgent(agentName) { + variants := codexRangeInspectionFallbackVariants(rangeRef, pathspecArgs) + selectedCtx, err := selectRichestRangePromptView(bodyLimit, templateContextFromRangeView(rangePromptView{ + Optional: ctx.optional, + Current: currentView, + }), variants) + if err != nil { + return "", err + } + if selectedCtx.Review != nil { + ctx.optional = optionalSectionsView{ + ProjectGuidelines: buildProjectGuidelinesSectionView(selectedCtx.Review.Optional.ProjectGuidelinesBody()), + AdditionalContext: selectedCtx.Review.Optional.AdditionalContext, + PreviousReviews: previousReviewViewsFromTemplateContext(selectedCtx.Review.Optional.PreviousReviews), + PreviousAttempts: reviewAttemptViewsFromTemplateContext(selectedCtx.Review.Optional.PreviousAttempts), + } + if selectedCtx.Review.Subject.Range != nil { + entries := make([]commitRangeEntryView, 0, len(selectedCtx.Review.Subject.Range.Entries)) + for _, entry := range selectedCtx.Review.Subject.Range.Entries { + entries = append(entries, commitRangeEntryView(entry)) + } + currentView = commitRangeSectionView{Count: selectedCtx.Review.Subject.Range.Count, Entries: entries} + } + diffView = diffSectionView{Heading: selectedCtx.Review.Diff.Heading, Body: selectedCtx.Review.Diff.Body, Fallback: selectedCtx.Review.Fallback.Rendered()} + } + } else { + fallback, err := renderGenericRangeFallback(renderShellCommand("git", "diff", rangeRef)) + if err != nil { + return "", err + } + diffView.Fallback = fallback } - return buildPromptPreservingCurrentSection( - requiredPrefix, - optionalPrefix, - currentRequiredText, - currentOverflowText, - promptCap, - diffFileFallbackVariants("### Combined Diff", opts.diffFilePath)..., - ), nil } else { inlineDiff, err := renderInlineDiff(diff) if err != nil { @@ -703,18 +925,18 @@ func (b *Builder) buildRangePrompt(repoPath, rangeRef string, repoID int64, cont diffView.Body = inlineDiff } - body, err := fitRangePrompt( + body, err := fitRangePromptContext( bodyLimit, - rangePromptView{ - Optional: optional, + templateContextFromRangeView(rangePromptView{ + Optional: ctx.optional, Current: currentView, Diff: diffView, - }, + }), ) if err != nil { return "", err } - return requiredPrefix + body, nil + return ctx.requiredPrefix + body, nil } func buildProjectGuidelinesSectionView(guidelines string) *markdownSectionView { @@ -736,8 +958,8 @@ func buildAdditionalContextSection(additionalContext string) string { return trimmed + "\n\n" } -func orderedPreviousReviewViews(contexts []ReviewContext) []previousReviewView { - ordered := make([]ReviewContext, 0, len(contexts)) +func orderedPreviousReviewViews(contexts []HistoricalReviewContext) []previousReviewView { + ordered := make([]HistoricalReviewContext, 0, len(contexts)) for i := len(contexts) - 1; i >= 0; i-- { ordered = append(ordered, contexts[i]) } @@ -747,13 +969,6 @@ func orderedPreviousReviewViews(contexts []ReviewContext) []previousReviewView { // LoadGuidelines loads review guidelines from the repo's default // branch, falling back to filesystem config when the default branch // has no .roborev.toml. -func (b *Builder) writeProjectGuidelines(sb *strings.Builder, guidelines string) { - body, err := renderOptionalSectionsPrefix(optionalSectionsView{ProjectGuidelines: buildProjectGuidelinesSectionView(guidelines)}) - if err == nil { - sb.WriteString(body) - } -} - func LoadGuidelines(repoPath string) string { // Load review guidelines from the default branch (origin/main, // origin/master, etc.). Branch-specific guidelines are intentionally @@ -805,38 +1020,37 @@ func (b *Builder) previousAttemptContexts(gitRef string) []reviewAttemptContext return attempts } -func (b *Builder) lookupReviewContexts(shas []string, skipMissing bool) []ReviewContext { - if b.db == nil { - return nil +// getPreviousReviewContexts gets the N commits before the target and looks up their reviews and responses +func (b *Builder) getPreviousReviewContexts(repoPath, sha string, count int) ([]HistoricalReviewContext, error) { + // Get parent commits from git + parentSHAs, err := git.GetParentCommits(repoPath, sha, count) + if err != nil { + return nil, fmt.Errorf("get parent commits: %w", err) } - var contexts []ReviewContext - for _, sha := range shas { - review, err := b.db.GetReviewByCommitSHA(sha) - if err != nil { - if skipMissing { - continue - } - contexts = append(contexts, ReviewContext{SHA: sha}) - continue - } - ctx := ReviewContext{SHA: sha, Review: review} - if review.JobID > 0 { - if responses, err := b.db.GetCommentsForJob(review.JobID); err == nil { - ctx.Responses = responses + + var contexts []HistoricalReviewContext + for _, parentSHA := range parentSHAs { + ctx := HistoricalReviewContext{SHA: parentSHA} + + // Try to look up review for this commit + review, err := b.db.GetReviewByCommitSHA(parentSHA) + if err == nil { + ctx.Review = review + + // Also fetch comments for this review's job + if review.JobID > 0 { + responses, err := b.db.GetCommentsForJob(review.JobID) + if err == nil { + ctx.Responses = responses + } } } + // If no review found, ctx.Review stays nil + contexts = append(contexts, ctx) } - return contexts -} -// getPreviousReviewContexts gets the N commits before the target and looks up their reviews and responses -func (b *Builder) getPreviousReviewContexts(repoPath, sha string, count int) ([]ReviewContext, error) { - parentSHAs, err := git.GetParentCommits(repoPath, sha, count) - if err != nil { - return nil, fmt.Errorf("get parent commits: %w", err) - } - return b.lookupReviewContexts(parentSHAs, false), nil + return contexts, nil } // BuildSimple constructs a simpler prompt without database context @@ -845,7 +1059,6 @@ func BuildSimple(repoPath, sha, agentName string) (string, error) { return b.Build(repoPath, sha, 0, 0, agentName, "", "") } -// PreviousAttemptsHeader introduces previous addressing attempts section. const PreviousAttemptsHeader = ` ## Previous Addressing Attempts @@ -855,7 +1068,6 @@ Be pragmatic - if previous attempts were rejected for being too minor, make more If they were rejected for being over-engineered, keep it simpler. ` -// UserCommentsHeader introduces user-authored comments on a review. const UserCommentsHeader = `## User Comments The following comments were left by the developer on this review. @@ -864,14 +1076,10 @@ positives, provide additional context, or request specific approaches. ` -// IsToolResponse returns true when the response was left by an automated -// tool (roborev-fix, roborev-refine, etc.) rather than a human user. func IsToolResponse(r storage.Response) bool { return strings.HasPrefix(r.Responder, "roborev-") } -// SplitResponses partitions responses into tool-generated attempts and -// user-authored comments based on the Responder field. func SplitResponses(responses []storage.Response) (toolAttempts, userComments []storage.Response) { for _, r := range responses { if IsToolResponse(r) { @@ -883,7 +1091,6 @@ func SplitResponses(responses []storage.Response) (toolAttempts, userComments [] return } -// FormatToolAttempts renders automated tool responses into a prompt section. func FormatToolAttempts(attempts []storage.Response) string { if len(attempts) == 0 { return "" @@ -899,7 +1106,6 @@ func FormatToolAttempts(attempts []storage.Response) string { return sb.String() } -// FormatUserComments renders user-authored comments into a prompt section. func FormatUserComments(comments []storage.Response) string { if len(comments) == 0 { return "" @@ -927,26 +1133,17 @@ func (b *Builder) BuildAddressPrompt(repoPath string, review *storage.Review, pr } if len(previousAttempts) > 0 { - toolAttempts, userComments := SplitResponses(previousAttempts) - if len(toolAttempts) > 0 { - view.ToolAttempts = make([]addressAttemptView, 0, len(toolAttempts)) - for _, attempt := range toolAttempts { - when := "" - if !attempt.CreatedAt.IsZero() { - when = attempt.CreatedAt.Format("2006-01-02 15:04") - } - view.ToolAttempts = append(view.ToolAttempts, addressAttemptView{Responder: attempt.Responder, Response: attempt.Response, When: when}) - } - } - if len(userComments) > 0 { - view.UserComments = make([]addressAttemptView, 0, len(userComments)) - for _, comment := range userComments { - when := "" - if !comment.CreatedAt.IsZero() { - when = comment.CreatedAt.Format("2006-01-02 15:04") - } - view.UserComments = append(view.UserComments, addressAttemptView{Responder: comment.Responder, Response: comment.Response, When: when}) + view.PreviousAttempts = make([]addressAttemptView, 0, len(previousAttempts)) + for _, attempt := range previousAttempts { + when := "" + if !attempt.CreatedAt.IsZero() { + when = attempt.CreatedAt.Format("2006-01-02 15:04") } + view.PreviousAttempts = append(view.PreviousAttempts, addressAttemptView{ + Responder: attempt.Responder, + Response: attempt.Response, + When: when, + }) } } diff --git a/internal/prompt/prompt_body_templates.go b/internal/prompt/prompt_body_templates.go index 3904930f..01f0a833 100644 --- a/internal/prompt/prompt_body_templates.go +++ b/internal/prompt/prompt_body_templates.go @@ -10,59 +10,23 @@ import ( "github.com/roborev-dev/roborev/internal/storage" ) -type markdownSectionView struct { - Heading string - Body string -} +type markdownSectionView = MarkdownSection -type reviewCommentView struct { - Responder string - Response string -} +type reviewCommentView = ReviewCommentTemplateContext -type previousReviewView struct { - Commit string - Output string - Comments []reviewCommentView - Available bool -} +type previousReviewView = PreviousReviewTemplateContext -type reviewAttemptView struct { - Label string - Agent string - When string - Output string - Comments []reviewCommentView -} +type reviewAttemptView = ReviewAttemptTemplateContext -type optionalSectionsView struct { - ProjectGuidelines *markdownSectionView - AdditionalContext string - PreviousReviews []previousReviewView - InRangeReviews []inRangeReviewView - PreviousAttempts []reviewAttemptView -} +type optionalSectionsView = ReviewOptionalContext -type currentCommitSectionView struct { - Commit string - Subject string - Author string - Message string -} +type currentCommitSectionView = SingleSubjectContext -type commitRangeEntryView struct { - Commit string - Subject string -} +type commitRangeEntryView = RangeEntryContext -type commitRangeSectionView struct { - Count int - Entries []commitRangeEntryView -} +type commitRangeSectionView = RangeSubjectContext -type dirtyChangesSectionView struct { - Description string -} +type dirtyChangesSectionView = DirtySubjectContext type diffSectionView struct { Heading string @@ -88,24 +52,11 @@ type dirtyPromptView struct { Diff diffSectionView } -type inRangeReviewView struct { - Commit string - Agent string - Verdict string - Output string - Comments []reviewCommentView -} - -type addressAttemptView struct { - Responder string - Response string - When string -} +type addressAttemptView = AddressAttemptTemplateContext type addressPromptView struct { ProjectGuidelines *markdownSectionView - ToolAttempts []addressAttemptView - UserComments []addressAttemptView + PreviousAttempts []addressAttemptView SeverityFilter string ReviewFindings string OriginalDiff string @@ -117,10 +68,7 @@ type reviewAttemptContext struct { Responses []storage.Response } -type systemPromptView struct { - NoSkillsInstruction string - CurrentDate string -} +type systemPromptView = SystemTemplateContext type inlineDiffView struct { Body string @@ -135,24 +83,191 @@ var promptTemplates = template.Must(template.New("prompt-templates").ParseFS( "templates/*.md.gotmpl", )) +func markdownSectionFromView(view *markdownSectionView) *MarkdownSection { + if view == nil { + return nil + } + return &MarkdownSection{Heading: view.Heading, Body: view.Body} +} + +func reviewCommentsFromView(views []reviewCommentView) []ReviewCommentTemplateContext { + comments := make([]ReviewCommentTemplateContext, 0, len(views)) + for _, view := range views { + comments = append(comments, ReviewCommentTemplateContext(view)) + } + return comments +} + +func previousReviewsFromView(views []previousReviewView) []PreviousReviewTemplateContext { + reviews := make([]PreviousReviewTemplateContext, 0, len(views)) + for _, view := range views { + reviews = append(reviews, PreviousReviewTemplateContext{ + Commit: view.Commit, + Output: view.Output, + Comments: reviewCommentsFromView(view.Comments), + Available: view.Available, + }) + } + return reviews +} + +func reviewAttemptsFromView(views []reviewAttemptView) []ReviewAttemptTemplateContext { + attempts := make([]ReviewAttemptTemplateContext, 0, len(views)) + for _, view := range views { + attempts = append(attempts, ReviewAttemptTemplateContext{ + Label: view.Label, + Agent: view.Agent, + When: view.When, + Output: view.Output, + Comments: reviewCommentsFromView(view.Comments), + }) + } + return attempts +} + +func reviewOptionalContextFromView(view optionalSectionsView) ReviewOptionalContext { + return ReviewOptionalContext{ + ProjectGuidelines: markdownSectionFromView(view.ProjectGuidelines), + AdditionalContext: view.AdditionalContext, + PreviousReviews: previousReviewsFromView(view.PreviousReviews), + PreviousAttempts: reviewAttemptsFromView(view.PreviousAttempts), + } +} + +type commitInspectionFallbackView struct { + SHA string + StatCmd string + DiffCmd string + FilesCmd string + ShowPathCmd string +} + +type rangeInspectionFallbackView struct { + RangeRef string + LogCmd string + StatCmd string + DiffCmd string + FilesCmd string + ViewCmd string +} + +type genericDiffFallbackView struct { + ViewCmd string +} + +func fallbackContextFromDiffSection(view diffSectionView) FallbackContext { + if view.Fallback == "" { + return FallbackContext{} + } + return FallbackContext{Mode: FallbackModeGeneric, Text: view.Fallback} +} + +func templateContextFromSingleView(view singlePromptView) TemplateContext { + return TemplateContext{ + Review: &ReviewTemplateContext{ + Kind: ReviewKindSingle, + Optional: reviewOptionalContextFromView(view.Optional), + Subject: SubjectContext{Single: &SingleSubjectContext{ + Commit: view.Current.Commit, + Subject: view.Current.Subject, + Author: view.Current.Author, + Message: view.Current.Message, + }}, + Diff: DiffContext{Heading: view.Diff.Heading, Body: view.Diff.Body}, + Fallback: fallbackContextFromDiffSection(view.Diff), + }, + } +} + +func templateContextFromRangeView(view rangePromptView) TemplateContext { + entries := make([]RangeEntryContext, 0, len(view.Current.Entries)) + for _, entry := range view.Current.Entries { + entries = append(entries, RangeEntryContext(entry)) + } + return TemplateContext{ + Review: &ReviewTemplateContext{ + Kind: ReviewKindRange, + Optional: reviewOptionalContextFromView(view.Optional), + Subject: SubjectContext{Range: &RangeSubjectContext{Count: view.Current.Count, Entries: entries}}, + Diff: DiffContext{Heading: view.Diff.Heading, Body: view.Diff.Body}, + Fallback: fallbackContextFromDiffSection(view.Diff), + }, + } +} + +func templateContextFromDirtyView(view dirtyPromptView) TemplateContext { + fallback := fallbackContextFromDiffSection(view.Diff) + if fallback.Text != "" { + fallback.Mode = FallbackModeDirty + fallback.Dirty = &DirtyFallbackContext{Body: fallback.Text} + fallback.Text = "" + } + return TemplateContext{ + Review: &ReviewTemplateContext{ + Kind: ReviewKindDirty, + Optional: reviewOptionalContextFromView(view.Optional), + Subject: SubjectContext{Dirty: &DirtySubjectContext{Description: view.Current.Description}}, + Diff: DiffContext{Heading: view.Diff.Heading, Body: view.Diff.Body}, + Fallback: fallback, + }, + } +} + +func templateContextFromAddressView(view addressPromptView) TemplateContext { + attempts := make([]AddressAttemptTemplateContext, 0, len(view.PreviousAttempts)) + for _, attempt := range view.PreviousAttempts { + attempts = append(attempts, AddressAttemptTemplateContext(attempt)) + } + return TemplateContext{ + Address: &AddressTemplateContext{ + ProjectGuidelines: markdownSectionFromView(view.ProjectGuidelines), + PreviousAttempts: attempts, + SeverityFilter: view.SeverityFilter, + ReviewFindings: view.ReviewFindings, + OriginalDiff: view.OriginalDiff, + JobID: view.JobID, + }, + } +} + +func templateContextFromSystemView(view systemPromptView) TemplateContext { + return TemplateContext{System: &SystemTemplateContext{NoSkillsInstruction: view.NoSkillsInstruction, CurrentDate: view.CurrentDate}} +} + +func renderSinglePromptContext(ctx TemplateContext) (string, error) { + return executePromptTemplate("assembled_single.md.gotmpl", ctx) +} + +func renderRangePromptContext(ctx TemplateContext) (string, error) { + return executePromptTemplate("assembled_range.md.gotmpl", ctx) +} + +func renderDirtyPromptContext(ctx TemplateContext) (string, error) { + return executePromptTemplate("assembled_dirty.md.gotmpl", ctx) +} + +func renderAddressPromptContext(ctx TemplateContext) (string, error) { + return executePromptTemplate("assembled_address.md.gotmpl", ctx) +} + func renderSinglePrompt(view singlePromptView) (string, error) { - return executePromptTemplate("assembled_single.md.gotmpl", view) + return renderSinglePromptContext(templateContextFromSingleView(view)) } func renderRangePrompt(view rangePromptView) (string, error) { - return executePromptTemplate("assembled_range.md.gotmpl", view) + return renderRangePromptContext(templateContextFromRangeView(view)) } func renderDirtyPrompt(view dirtyPromptView) (string, error) { - return executePromptTemplate("assembled_dirty.md.gotmpl", view) + return renderDirtyPromptContext(templateContextFromDirtyView(view)) } func renderAddressPrompt(view addressPromptView) (string, error) { - return executePromptTemplate("assembled_address.md.gotmpl", view) + return renderAddressPromptContext(templateContextFromAddressView(view)) } -func fitSinglePrompt(limit int, view singlePromptView) (string, error) { - body, err := renderSinglePrompt(view) +func fitSinglePromptContext(limit int, ctx TemplateContext) (string, error) { + body, err := renderSinglePromptContext(ctx) if err != nil { return "", err } @@ -160,8 +275,8 @@ func fitSinglePrompt(limit int, view singlePromptView) (string, error) { return body, nil } - for trimOptionalSections(&view.Optional) { - body, err = renderSinglePrompt(view) + for ctx.Review != nil && ctx.Review.Optional.TrimNext() { + body, err = renderSinglePromptContext(ctx) if err != nil { return "", err } @@ -170,9 +285,8 @@ func fitSinglePrompt(limit int, view singlePromptView) (string, error) { } } - if view.Current.Message != "" { - view.Current.Message = "" - body, err = renderSinglePrompt(view) + if ctx.Review != nil && ctx.Review.Subject.TrimSingleMessage() { + body, err = renderSinglePromptContext(ctx) if err != nil { return "", err } @@ -181,9 +295,8 @@ func fitSinglePrompt(limit int, view singlePromptView) (string, error) { } } - if view.Current.Author != "" { - view.Current.Author = "" - body, err = renderSinglePrompt(view) + if ctx.Review != nil && ctx.Review.Subject.TrimSingleAuthor() { + body, err = renderSinglePromptContext(ctx) if err != nil { return "", err } @@ -192,10 +305,10 @@ func fitSinglePrompt(limit int, view singlePromptView) (string, error) { } } - for len(body) > limit && view.Current.Subject != "" { + for ctx.Review != nil && ctx.Review.Subject.Single != nil && len(body) > limit && ctx.Review.Subject.Single.Subject != "" { overflow := len(body) - limit - view.Current.Subject = truncateUTF8(view.Current.Subject, max(0, len(view.Current.Subject)-overflow)) - body, err = renderSinglePrompt(view) + ctx.Review.Subject.TrimSingleSubjectTo(max(0, len(ctx.Review.Subject.Single.Subject)-overflow)) + body, err = renderSinglePromptContext(ctx) if err != nil { return "", err } @@ -204,67 +317,61 @@ func fitSinglePrompt(limit int, view singlePromptView) (string, error) { return hardCapPrompt(body, limit), nil } -func fitRangePrompt(limit int, view rangePromptView) (string, error) { - _, body, err := trimRangePromptView(limit, view) +func fitSinglePrompt(limit int, view singlePromptView) (string, error) { + return fitSinglePromptContext(limit, templateContextFromSingleView(view)) +} + +func fitRangePromptContext(limit int, ctx TemplateContext) (string, error) { + _, body, err := trimRangePromptContext(limit, ctx) if err != nil { return "", err } return hardCapPrompt(body, limit), nil } -func cloneCommitRangeSectionView(view commitRangeSectionView) commitRangeSectionView { - cloned := view - if len(view.Entries) == 0 { - return cloned - } - cloned.Entries = append([]commitRangeEntryView(nil), view.Entries...) - return cloned +func fitRangePrompt(limit int, view rangePromptView) (string, error) { + return fitRangePromptContext(limit, templateContextFromRangeView(view)) } -func trimRangePromptView(limit int, view rangePromptView) (rangePromptView, string, error) { - view.Current = cloneCommitRangeSectionView(view.Current) - body, err := renderRangePrompt(view) +func trimRangePromptContext(limit int, ctx TemplateContext) (TemplateContext, string, error) { + ctx = ctx.Clone() + body, err := renderRangePromptContext(ctx) if err != nil { - return rangePromptView{}, "", err + return TemplateContext{}, "", err } if len(body) <= limit { - return view, body, nil + return ctx, body, nil } - for trimOptionalSections(&view.Optional) { - body, err = renderRangePrompt(view) + for ctx.Review != nil && ctx.Review.Optional.TrimNext() { + body, err = renderRangePromptContext(ctx) if err != nil { - return rangePromptView{}, "", err + return TemplateContext{}, "", err } if len(body) <= limit { - return view, body, nil + return ctx, body, nil } } - for i := len(view.Current.Entries) - 1; i >= 0 && len(body) > limit; i-- { - if view.Current.Entries[i].Subject == "" { - continue - } - view.Current.Entries[i].Subject = "" - body, err = renderRangePrompt(view) + for ctx.Review != nil && len(body) > limit && ctx.Review.Subject.BlankNextRangeSubject() { + body, err = renderRangePromptContext(ctx) if err != nil { - return rangePromptView{}, "", err + return TemplateContext{}, "", err } } - for len(view.Current.Entries) > 0 && len(body) > limit { - view.Current.Entries = view.Current.Entries[:len(view.Current.Entries)-1] - body, err = renderRangePrompt(view) + for ctx.Review != nil && len(body) > limit && ctx.Review.Subject.DropLastRangeEntry() { + body, err = renderRangePromptContext(ctx) if err != nil { - return rangePromptView{}, "", err + return TemplateContext{}, "", err } } - return view, body, nil + return ctx, body, nil } -func fitDirtyPrompt(limit int, view dirtyPromptView) (string, error) { - body, err := renderDirtyPrompt(view) +func fitDirtyPromptContext(limit int, ctx TemplateContext) (string, error) { + body, err := renderDirtyPromptContext(ctx) if err != nil { return "", err } @@ -272,8 +379,8 @@ func fitDirtyPrompt(limit int, view dirtyPromptView) (string, error) { return body, nil } - for trimOptionalSections(&view.Optional) { - body, err = renderDirtyPrompt(view) + for ctx.Review != nil && ctx.Review.Optional.TrimNext() { + body, err = renderDirtyPromptContext(ctx) if err != nil { return "", err } @@ -285,24 +392,27 @@ func fitDirtyPrompt(limit int, view dirtyPromptView) (string, error) { return hardCapPrompt(body, limit), nil } +func fitDirtyPrompt(limit int, view dirtyPromptView) (string, error) { + return fitDirtyPromptContext(limit, templateContextFromDirtyView(view)) +} + func trimOptionalSections(view *optionalSectionsView) bool { - switch { - case len(view.PreviousAttempts) > 0: - view.PreviousAttempts = nil - case len(view.PreviousReviews) > 0: - view.PreviousReviews = nil - case view.AdditionalContext != "": - view.AdditionalContext = "" - case view.ProjectGuidelines != nil: - view.ProjectGuidelines = nil - default: + if view == nil { return false } + ctx := reviewOptionalContextFromView(*view) + if !ctx.TrimNext() { + return false + } + view.ProjectGuidelines = buildProjectGuidelinesSectionView(ctx.ProjectGuidelinesBody()) + view.AdditionalContext = ctx.AdditionalContext + view.PreviousReviews = previousReviewViewsFromTemplateContext(ctx.PreviousReviews) + view.PreviousAttempts = reviewAttemptViewsFromTemplateContext(ctx.PreviousAttempts) return true } func renderSystemPrompt(name string, view systemPromptView) (string, error) { - return executePromptTemplate(name, view) + return executePromptTemplate(name, templateContextFromSystemView(view)) } func renderAddressPromptFromSections(view addressPromptView) (string, error) { @@ -346,7 +456,11 @@ func renderDirtyChangesSection(view dirtyChangesSectionView) (string, error) { } func renderDiffBlock(view diffSectionView) (string, error) { - return executePromptTemplate("diff_block", view) + ctx := ReviewTemplateContext{ + Diff: DiffContext{Heading: view.Heading, Body: view.Body}, + Fallback: fallbackContextFromDiffSection(view), + } + return executePromptTemplate("diff_block", ctx) } func renderInlineDiff(body string) (string, error) { @@ -356,6 +470,22 @@ func renderInlineDiff(body string) (string, error) { return executePromptTemplate("inline_diff", inlineDiffView{Body: body}) } +func renderCommitInspectionFallback(name string, view commitInspectionFallbackView) (string, error) { + return executePromptTemplate(name, view) +} + +func renderRangeInspectionFallback(name string, view rangeInspectionFallbackView) (string, error) { + return executePromptTemplate(name, view) +} + +func renderGenericCommitFallback(viewCmd string) (string, error) { + return executePromptTemplate("generic_commit_fallback", genericDiffFallbackView{ViewCmd: viewCmd}) +} + +func renderGenericRangeFallback(viewCmd string) (string, error) { + return executePromptTemplate("generic_range_fallback", genericDiffFallbackView{ViewCmd: viewCmd}) +} + func renderDirtyTruncatedDiffFallback(body string) (string, error) { if body != "" && !strings.HasSuffix(body, "\n") { body += "\n" @@ -363,18 +493,14 @@ func renderDirtyTruncatedDiffFallback(body string) (string, error) { return executePromptTemplate("dirty_truncated_diff_fallback", dirtyTruncatedDiffFallbackView{Body: body}) } -func previousReviewViews(contexts []ReviewContext) []previousReviewView { +func previousReviewViewsFromTemplateContext(contexts []PreviousReviewTemplateContext) []previousReviewView { views := make([]previousReviewView, 0, len(contexts)) for _, ctx := range contexts { - view := previousReviewView{Commit: git.ShortSHA(ctx.SHA)} - if ctx.Review != nil { - view.Available = true - view.Output = ctx.Review.Output - } - if len(ctx.Responses) > 0 { - view.Comments = make([]reviewCommentView, 0, len(ctx.Responses)) - for _, resp := range ctx.Responses { - view.Comments = append(view.Comments, reviewCommentView{Responder: resp.Responder, Response: resp.Response}) + view := previousReviewView{Commit: ctx.Commit, Available: ctx.Available, Output: ctx.Output} + if len(ctx.Comments) > 0 { + view.Comments = make([]reviewCommentView, 0, len(ctx.Comments)) + for _, comment := range ctx.Comments { + view.Comments = append(view.Comments, reviewCommentView(comment)) } } views = append(views, view) @@ -382,29 +508,13 @@ func previousReviewViews(contexts []ReviewContext) []previousReviewView { return views } -func renderPreviousReviewsFromContexts(contexts []ReviewContext) (string, error) { - return renderOptionalSectionsFromView(optionalSectionsView{PreviousReviews: previousReviewViews(contexts)}) -} - -func inRangeReviewViews(contexts []ReviewContext) []inRangeReviewView { - views := make([]inRangeReviewView, 0, len(contexts)) +func previousReviewViews(contexts []HistoricalReviewContext) []previousReviewView { + views := make([]previousReviewView, 0, len(contexts)) for _, ctx := range contexts { - if ctx.Review == nil { - continue - } - verdict := storage.ParseVerdict(ctx.Review.Output) - verdictLabel := "unknown" - switch verdict { - case "P": - verdictLabel = "passed" - case "F": - verdictLabel = "failed" - } - view := inRangeReviewView{ - Commit: git.ShortSHA(ctx.SHA), - Agent: ctx.Review.Agent, - Verdict: verdictLabel, - Output: ctx.Review.Output, + view := previousReviewView{Commit: git.ShortSHA(ctx.SHA)} + if ctx.Review != nil { + view.Available = true + view.Output = ctx.Review.Output } if len(ctx.Responses) > 0 { view.Comments = make([]reviewCommentView, 0, len(ctx.Responses)) @@ -417,6 +527,10 @@ func inRangeReviewViews(contexts []ReviewContext) []inRangeReviewView { return views } +func renderPreviousReviewsFromContexts(contexts []HistoricalReviewContext) (string, error) { + return renderOptionalSectionsFromView(optionalSectionsView{PreviousReviews: previousReviewViews(contexts)}) +} + func reviewAttemptViews(reviews []storage.Review) []reviewAttemptView { views := make([]reviewAttemptView, 0, len(reviews)) for i, review := range reviews { @@ -438,6 +552,21 @@ func renderPreviousAttemptsFromReviews(reviews []storage.Review) (string, error) return renderOptionalSectionsFromView(optionalSectionsView{PreviousAttempts: reviewAttemptViews(reviews)}) } +func reviewAttemptViewsFromTemplateContext(attempts []ReviewAttemptTemplateContext) []reviewAttemptView { + views := make([]reviewAttemptView, 0, len(attempts)) + for _, attempt := range attempts { + view := reviewAttemptView{Label: attempt.Label, Agent: attempt.Agent, When: attempt.When, Output: attempt.Output} + if len(attempt.Comments) > 0 { + view.Comments = make([]reviewCommentView, 0, len(attempt.Comments)) + for _, comment := range attempt.Comments { + view.Comments = append(view.Comments, reviewCommentView(comment)) + } + } + views = append(views, view) + } + return views +} + func previousAttemptViewsFromContexts(attempts []reviewAttemptContext) []reviewAttemptView { views := make([]reviewAttemptView, 0, len(attempts)) for i, attempt := range attempts { diff --git a/internal/prompt/prompt_body_templates_test.go b/internal/prompt/prompt_body_templates_test.go index 8013f30a..b3591f33 100644 --- a/internal/prompt/prompt_body_templates_test.go +++ b/internal/prompt/prompt_body_templates_test.go @@ -119,11 +119,11 @@ func TestRenderAddressPromptUsesNestedSections(t *testing.T) { Heading: "## Project Guidelines", Body: "Keep it simple.", }, - ToolAttempts: []addressAttemptView{{Responder: "developer", Response: "Tried a narrow fix", When: "2026-04-04 12:00"}}, - SeverityFilter: "Only address medium and higher findings.\n\n", - ReviewFindings: "- medium: do the thing", - OriginalDiff: "diff --git a/a b/a\n+line\n", - JobID: 42, + PreviousAttempts: []addressAttemptView{{Responder: "developer", Response: "Tried a narrow fix", When: "2026-04-04 12:00"}}, + SeverityFilter: "Only address medium and higher findings.\n\n", + ReviewFindings: "- medium: do the thing", + OriginalDiff: "diff --git a/a b/a\n+line\n", + JobID: 42, } body, err := renderAddressPrompt(view) @@ -235,8 +235,66 @@ func TestBuildProjectGuidelinesSectionViewTrimsAndFormats(t *testing.T) { assert.Equal(t, "Prefer composition over inheritance.", section.Body) } -func TestPreviousReviewViewsPreserveChronologicalOrder(t *testing.T) { - views := previousReviewViews([]ReviewContext{ +func TestReviewOptionalContextTrimNextPreservesPriority(t *testing.T) { + ctx := ReviewOptionalContext{ + ProjectGuidelines: &MarkdownSection{Heading: "## Project Guidelines", Body: "Keep it simple."}, + AdditionalContext: "## Pull Request Discussion\n\ncontext\n\n", + PreviousReviews: []PreviousReviewTemplateContext{{Commit: "abc1234", Output: "review", Available: true}}, + PreviousAttempts: []ReviewAttemptTemplateContext{{Label: "Review Attempt 1", Output: "attempt"}}, + } + + require.True(t, ctx.TrimNext()) + assert.Empty(t, ctx.PreviousAttempts) + require.True(t, ctx.TrimNext()) + assert.Empty(t, ctx.PreviousReviews) + require.True(t, ctx.TrimNext()) + assert.Empty(t, ctx.AdditionalContext) + require.True(t, ctx.TrimNext()) + assert.Nil(t, ctx.ProjectGuidelines) + assert.False(t, ctx.TrimNext()) +} + +func TestTemplateContextCloneIsolatesNestedState(t *testing.T) { + ctx := TemplateContext{ + Review: &ReviewTemplateContext{ + Optional: ReviewOptionalContext{ + ProjectGuidelines: &MarkdownSection{Heading: "## Project Guidelines", Body: "Keep it simple."}, + PreviousAttempts: []ReviewAttemptTemplateContext{{Label: "Review Attempt 1", Output: "attempt"}}, + }, + Subject: SubjectContext{ + Range: &RangeSubjectContext{Count: 2, Entries: []RangeEntryContext{{Commit: "abc1234", Subject: "first"}, {Commit: "def5678", Subject: "second"}}}, + }, + Fallback: FallbackContext{Mode: FallbackModeRange, Range: &RangeFallbackContext{DiffCmd: "git diff"}}, + }, + } + + cloned := ctx.Clone() + require.NotNil(t, cloned.Review) + require.True(t, cloned.Review.Optional.TrimNext()) + require.True(t, cloned.Review.Subject.BlankNextRangeSubject()) + cloned.Review.Fallback.Range.DiffCmd = "git diff --stat" + + require.NotNil(t, ctx.Review.Optional.ProjectGuidelines) + require.Len(t, ctx.Review.Optional.PreviousAttempts, 1) + require.NotNil(t, ctx.Review.Subject.Range) + assert.Equal(t, "second", ctx.Review.Subject.Range.Entries[1].Subject) + assert.Equal(t, "git diff", ctx.Review.Fallback.Range.DiffCmd) +} + +func TestTemplateContextSubjectRangeTrimmingHelpers(t *testing.T) { + ctx := SubjectContext{Range: &RangeSubjectContext{Count: 2, Entries: []RangeEntryContext{{Commit: "abc1234", Subject: "first"}, {Commit: "def5678", Subject: "second"}}}} + + require.True(t, ctx.BlankNextRangeSubject()) + require.NotNil(t, ctx.Range) + assert.Empty(t, ctx.Range.Entries[1].Subject) + assert.Equal(t, "first", ctx.Range.Entries[0].Subject) + require.True(t, ctx.DropLastRangeEntry()) + require.Len(t, ctx.Range.Entries, 1) + assert.Equal(t, "abc1234", ctx.Range.Entries[0].Commit) +} + +func TestHistoricalReviewContextPreviousReviewViewsPreserveChronologicalOrder(t *testing.T) { + views := previousReviewViews([]HistoricalReviewContext{ {SHA: "bbbbbbb", Review: &storage.Review{Output: "second"}}, {SHA: "aaaaaaa", Review: &storage.Review{Output: "first"}}, }) @@ -246,7 +304,7 @@ func TestPreviousReviewViewsPreserveChronologicalOrder(t *testing.T) { } func TestRenderPreviousReviewsFromContexts(t *testing.T) { - body, err := renderPreviousReviewsFromContexts([]ReviewContext{ + body, err := renderPreviousReviewsFromContexts([]HistoricalReviewContext{ { SHA: "abc1234", Review: &storage.Review{Output: "Found a bug"}, diff --git a/internal/prompt/prompt_test.go b/internal/prompt/prompt_test.go index 8b23fd54..7fe87dc1 100644 --- a/internal/prompt/prompt_test.go +++ b/internal/prompt/prompt_test.go @@ -6,7 +6,6 @@ import ( "path/filepath" "strings" "testing" - "time" "unicode/utf8" "github.com/roborev-dev/roborev/internal/config" @@ -70,6 +69,48 @@ func TestBuildPromptWithAdditionalContext(t *testing.T) { assertContains(t, prompt, "Most recent human comment first.", "Prompt should contain additional context body") } +func TestBuildPromptWithAdditionalContextAndPreviousAttemptsPreservesSectionOrder(t *testing.T) { + repoPath, commits := setupTestRepo(t) + db, repoID := setupDBWithCommits(t, repoPath, commits) + + testutil.CreateCompletedReview(t, db, repoID, commits[5], "test", "First review") + + builder := NewBuilder(db) + prompt, err := builder.BuildWithAdditionalContext( + repoPath, + commits[5], + repoID, + 0, + "claude-code", + "", + "", + "## Pull Request Discussion\n\nNewest comment first.\n", + ) + require.NoError(t, err) + + additionalContextPos := strings.Index(prompt, "## Pull Request Discussion") + previousAttemptsPos := strings.Index(prompt, "## Previous Review Attempts") + currentCommitPos := strings.Index(prompt, "## Current Commit") + + require.NotEqual(t, -1, additionalContextPos) + require.NotEqual(t, -1, previousAttemptsPos) + require.NotEqual(t, -1, currentCommitPos) + assert.Less(t, additionalContextPos, previousAttemptsPos) + assert.Less(t, previousAttemptsPos, currentCommitPos) +} + +func TestOrderedPreviousReviewViewsRendersOldestFirst(t *testing.T) { + views := orderedPreviousReviewViews([]HistoricalReviewContext{ + {SHA: "ccccccc", Review: &storage.Review{Output: "newest"}}, + {SHA: "bbbbbbb", Review: &storage.Review{Output: "middle"}}, + {SHA: "aaaaaaa", Review: &storage.Review{Output: "oldest"}}, + }) + require.Len(t, views, 3) + assert.Equal(t, "aaaaaaa", views[0].Commit) + assert.Equal(t, "bbbbbbb", views[1].Commit) + assert.Equal(t, "ccccccc", views[2].Commit) +} + func TestBuildPromptWithPreviousReviews(t *testing.T) { repoPath, commits := setupTestRepo(t) @@ -498,7 +539,8 @@ func TestBuildWithDiffFileNonCodexUsesDiffFile(t *testing.T) { } func TestBuildWithDiffFileSmallDiffInlineIgnoresFile(t *testing.T) { - repoPath, sha := setupSmallDiffRepo(t) + repoPath, commits := setupTestRepo(t) + sha := commits[len(commits)-1] b := NewBuilder(nil) diffFile := filepath.Join(repoPath, ".roborev-review-42.diff") @@ -509,19 +551,70 @@ func TestBuildWithDiffFileSmallDiffInlineIgnoresFile(t *testing.T) { assertNotContains(t, prompt, diffFile, "diff file should not be referenced when diff fits inline") } -func shortestDiffFileFallback(heading string) string { - // Use a representative path for size estimation in cap tests. - variants := diffFileFallbackVariants(heading, "/tmp/roborev-review-0.diff") - return variants[len(variants)-1] +func TestBuildPromptCodexOversizedDiffProvidesGitInspectionInstructions(t *testing.T) { + repoPath, sha := setupLargeDiffRepo(t) + + b := NewBuilder(nil) + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") + require.NoError(t, err, "Build failed: %v", err) + + assertContains(t, prompt, "(Diff too large to include inline)", "expected oversized diff marker") + assertContains(t, prompt, "inspect the commit locally with read-only git commands", "expected Codex git inspection guidance") + assertContains(t, prompt, "git show --stat --summary "+sha, "expected commit stat command") + assertContains(t, prompt, "git show --format=medium --unified=80 "+sha, "expected full commit diff command") + assertContains(t, prompt, "git diff-tree --no-commit-id --name-only -r "+sha, "expected touched files command") + assertNotContains(t, prompt, "View with:", "Codex fallback should not use the weak generic hint") +} + +func TestBuildRangePromptCodexOversizedDiffProvidesGitInspectionInstructions(t *testing.T) { + repoPath, sha := setupLargeDiffRepo(t) + rangeRef := sha + "~1.." + sha + + b := NewBuilder(nil) + prompt, err := b.Build(repoPath, rangeRef, 0, 0, "codex", "", "") + require.NoError(t, err, "Build failed: %v", err) + + assertContains(t, prompt, "(Diff too large to include inline)", "expected oversized diff marker") + assertContains(t, prompt, "inspect the commit range locally with read-only git commands", "expected Codex range inspection guidance") + assertContains(t, prompt, "git log --oneline "+rangeRef, "expected range commit list command") + assertContains(t, prompt, "git diff --stat "+rangeRef, "expected range stat command") + assertContains(t, prompt, "git diff --unified=80 "+rangeRef, "expected full range diff command") + assertContains(t, prompt, "git diff --name-only "+rangeRef, "expected touched files command") + assertNotContains(t, prompt, "View with:", "Codex fallback should not use the weak generic hint") +} + +func codexCommitFallback(sha string) string { + return mustRenderPromptTestDiffBlock(codexCommitInspectionFallbackVariants(sha, nil)[0]) +} + +func shortestCodexCommitFallback(sha string) string { + variants := codexCommitInspectionFallbackVariants(sha, nil) + return mustRenderPromptTestDiffBlock(variants[len(variants)-1]) +} + +func shortestCodexRangeFallback(rangeRef string) string { + variants := codexRangeInspectionFallbackVariants(rangeRef, nil) + return mustRenderPromptTestDiffBlock(variants[len(variants)-1]) +} + +func mustRenderPromptTestDiffBlock(view diffSectionView) string { + block, err := renderDiffBlock(view) + if err != nil { + panic(err) + } + return block } func singleCommitPromptPrefixLen(t *testing.T, repoPath, sha string) int { t.Helper() var sb strings.Builder - b := NewBuilder(nil) sb.WriteString(GetSystemPrompt("codex", "review")) sb.WriteString("\n") - b.writeProjectGuidelines(&sb, LoadGuidelines(repoPath)) + guidelines, err := renderOptionalSectionsPrefix(optionalSectionsView{ + ProjectGuidelines: buildProjectGuidelinesSectionView(LoadGuidelines(repoPath)), + }) + require.NoError(t, err) + sb.WriteString(guidelines) info, err := gitpkg.GetCommitInfo(repoPath, sha) require.NoError(t, err, "GetCommitInfo failed: %v", err) @@ -541,10 +634,13 @@ func singleCommitPromptPrefixLen(t *testing.T, repoPath, sha string) int { func rangePromptPrefixLen(t *testing.T, repoPath, rangeRef string) int { t.Helper() var sb strings.Builder - b := NewBuilder(nil) sb.WriteString(GetSystemPrompt("codex", "range")) sb.WriteString("\n") - b.writeProjectGuidelines(&sb, LoadGuidelines(repoPath)) + guidelines, err := renderOptionalSectionsPrefix(optionalSectionsView{ + ProjectGuidelines: buildProjectGuidelinesSectionView(LoadGuidelines(repoPath)), + }) + require.NoError(t, err) + sb.WriteString(guidelines) commits, err := gitpkg.GetRangeCommits(repoPath, rangeRef) require.NoError(t, err, "GetRangeCommits failed: %v", err) @@ -596,52 +692,54 @@ func rangeNearCapRepo(t *testing.T, remainingBudget int) (string, string) { } func TestBuildPromptCodexOversizedDiffStaysWithinMaxPromptSize(t *testing.T) { - setupLargeDiffRepoWithGuidelines(t, 1) - remainingBudget := len(shortestDiffFileFallback("### Diff")) + _, probeSHA := setupLargeDiffRepoWithGuidelines(t, 1) + remainingBudget := len(shortestCodexCommitFallback(probeSHA)) repoPath, sha := singleCommitNearCapRepo(t, remainingBudget) b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) - assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected final prompt to stay within the prompt cap") - assertContains(t, prompt, "Diff too large", "expected truncation note") + assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected final Codex prompt to stay within the prompt cap") + assertContains(t, prompt, "git show", "expected fallback to retain local git inspection guidance") + assert.NotContains(t, prompt, codexCommitFallback(sha), "expected the full fallback to be downgraded when it would overflow") } func TestBuildRangePromptCodexOversizedDiffStaysWithinMaxPromptSize(t *testing.T) { - setupLargeDiffRepoWithGuidelines(t, 1) - remainingBudget := len(shortestDiffFileFallback("### Combined Diff")) + _, probeSHA := setupLargeDiffRepoWithGuidelines(t, 1) + probeRangeRef := probeSHA + "~1.." + probeSHA + remainingBudget := len(shortestCodexRangeFallback(probeRangeRef)) repoPath, sha := rangeNearCapRepo(t, remainingBudget) rangeRef := sha + "~1.." + sha b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, rangeRef, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, rangeRef, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) - assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected final range prompt to stay within the prompt cap") - assertContains(t, prompt, "Diff too large", "expected truncation note") + assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected final Codex range prompt to stay within the prompt cap") + assertContains(t, prompt, "git diff", "expected fallback to retain local git inspection guidance") } func TestBuildPromptCodexOversizedDiffTrimsPrefixToFitShortestFallback(t *testing.T) { - setupLargeDiffRepoWithGuidelines(t, 1) - shortestFallback := shortestDiffFileFallback("### Diff") + _, probeSHA := setupLargeDiffRepoWithGuidelines(t, 1) + shortestFallback := shortestCodexCommitFallback(probeSHA) repoPath, sha := singleCommitNearCapRepo(t, len(shortestFallback)-1) b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected final Codex prompt to stay within the prompt cap") assertContains(t, prompt, "Diff too large", "expected a diff-omitted marker even when the prefix consumed the budget") - assert.Contains(t, prompt, shortestDiffFileFallback("### Diff"), "expected the shortest fallback to be preserved by trimming earlier context") + assert.Contains(t, prompt, shortestCodexCommitFallback(sha), "expected the shortest fallback to be preserved by trimming earlier context") } func TestBuildPromptCodexOversizedDiffKeepsCurrentCommitMetadataWhenTrimming(t *testing.T) { repoPath, sha := singleCommitNearCapRepo(t, 1) - shortestFallback := shortestDiffFileFallback("### Diff") + shortestFallback := shortestCodexCommitFallback(sha) b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected final Codex prompt to stay within the prompt cap") @@ -654,11 +752,11 @@ func TestBuildPromptCodexOversizedDiffWithLargeCommitBodyStaysWithinMaxPromptSiz repoPath, sha := setupLargeCommitBodyRepo(t, defaultPromptCap) b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected large commit metadata to still stay within the prompt cap") - assert.Contains(t, prompt, shortestDiffFileFallback("### Diff"), "expected the shortest fallback to remain present when commit metadata is oversized") + assert.Contains(t, prompt, shortestCodexCommitFallback(sha), "expected the shortest fallback to remain present when commit metadata is oversized") assertContains(t, prompt, "## Current Commit", "expected the current commit section header to remain intact") assertContains(t, prompt, "**Subject:** large change", "expected the current commit subject to remain intact") } @@ -667,11 +765,11 @@ func TestBuildPromptCodexOversizedDiffWithLargeCommitSubjectStaysWithinMaxPrompt repoPath, sha := setupLargeCommitSubjectRepo(t, defaultPromptCap) b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected large commit metadata to still stay within the prompt cap") - assert.Contains(t, prompt, shortestDiffFileFallback("### Diff"), "expected the shortest fallback to remain present when commit subject metadata is oversized") + assert.Contains(t, prompt, shortestCodexCommitFallback(sha), "expected the shortest fallback to remain present when commit subject metadata is oversized") assertContains(t, prompt, "## Current Commit", "expected the current commit section header to remain intact") } @@ -679,11 +777,11 @@ func TestBuildPromptCodexOversizedDiffPrioritizesSubjectOverAuthor(t *testing.T) repoPath, sha := setupLargeCommitAuthorRepo(t, defaultPromptCap) b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected large commit metadata to still stay within the prompt cap") - assert.Contains(t, prompt, shortestDiffFileFallback("### Diff"), "expected the shortest fallback to remain present when commit author metadata is oversized") + assert.Contains(t, prompt, shortestCodexCommitFallback(sha), "expected the shortest fallback to remain present when commit author metadata is oversized") assertContains(t, prompt, "## Current Commit", "expected the current commit section header to remain intact") assertContains(t, prompt, "**Subject:** large change", "expected the subject line to survive before the author line") } @@ -702,31 +800,30 @@ func TestSetupLargeCommitAuthorRepoIgnoresIdentityEnv(t *testing.T) { } func TestBuildRangePromptCodexOversizedDiffTrimsPrefixToFitShortestFallback(t *testing.T) { - setupLargeDiffRepoWithGuidelines(t, 1) - shortestFallback := shortestDiffFileFallback("### Combined Diff") + _, probeSHA := setupLargeDiffRepoWithGuidelines(t, 1) + probeRangeRef := probeSHA + "~1.." + probeSHA + shortestFallback := shortestCodexRangeFallback(probeRangeRef) repoPath, sha := rangeNearCapRepo(t, len(shortestFallback)-1) rangeRef := sha + "~1.." + sha b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, rangeRef, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, rangeRef, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected final Codex range prompt to stay within the prompt cap") assertContains(t, prompt, "Diff too large", "expected a diff-omitted marker even when the prefix consumed the budget") - assert.Contains(t, prompt, shortestDiffFileFallback("### Combined Diff"), "expected the shortest range fallback to be preserved by trimming earlier context") + assertContains(t, prompt, "git diff", "expected Codex range fallback guidance to remain after trimming earlier context") } func TestBuildRangePromptCodexOversizedDiffKeepsCurrentRangeMetadataWhenTrimming(t *testing.T) { repoPath, sha := rangeNearCapRepo(t, 1) rangeRef := sha + "~1.." + sha - shortestFallback := shortestDiffFileFallback("### Combined Diff") - b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, rangeRef, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, rangeRef, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected final Codex range prompt to stay within the prompt cap") - assert.Contains(t, prompt, shortestFallback, "expected the shortest range fallback to be preserved after trimming earlier context") + assertContains(t, prompt, "git diff", "expected Codex range fallback guidance to remain after trimming earlier context") assertContains(t, prompt, "## Commit Range", "expected the commit range section header to remain intact") assertContains(t, prompt, "- "+gitpkg.ShortSHA(sha)+" large change", "expected the current range entry to remain intact") } @@ -735,22 +832,112 @@ func TestBuildRangePromptCodexOversizedDiffWithLargeRangeMetadataStaysWithinMaxP repoPath, rangeRef := setupLargeRangeMetadataRepo(t, 80, 4096) b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, rangeRef, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, rangeRef, 0, 0, "codex", "", "") require.NoError(t, err, "Build failed: %v", err) assert.LessOrEqual(t, len(prompt), defaultPromptCap, "expected large range metadata to still stay within the prompt cap") - assert.Contains(t, prompt, shortestDiffFileFallback("### Combined Diff"), "expected the shortest range fallback to remain present when range metadata is oversized") + assertContains(t, prompt, "git diff", "expected Codex range fallback guidance to remain present when range metadata is oversized") assertContains(t, prompt, "## Commit Range", "expected the commit range section header to remain intact") assertContains(t, prompt, "Reviewing 80 commits:", "expected the range summary to remain intact") } +func TestSelectRichestRangePromptViewPrefersRicherVariantOnEqualMetadataLoss(t *testing.T) { + view := rangePromptView{ + Current: commitRangeSectionView{Count: 3, Entries: []commitRangeEntryView{ + {Commit: "abc1234", Subject: strings.Repeat("s", 200)}, + {Commit: "def5678", Subject: strings.Repeat("s", 200)}, + {Commit: "ghi9012", Subject: strings.Repeat("s", 200)}, + }}, + } + variants := []diffSectionView{ + {Heading: "### Combined Diff", Fallback: strings.Repeat("richer guidance\n", 8)}, + {Heading: "### Combined Diff", Fallback: "short guidance\n"}, + } + trimmedCurrent := commitRangeSectionView{Count: 3, Entries: []commitRangeEntryView{ + {Commit: "abc1234", Subject: strings.Repeat("s", 200)}, + {Commit: "def5678", Subject: strings.Repeat("s", 200)}, + {Commit: "ghi9012"}, + }} + richerBody, err := renderRangePrompt(rangePromptView{Current: trimmedCurrent, Diff: variants[0]}) + require.NoError(t, err) + fullShorterBody, err := renderRangePrompt(rangePromptView{Current: view.Current, Diff: variants[1]}) + require.NoError(t, err) + require.LessOrEqual(t, len(richerBody), len(fullShorterBody), + "test setup must allow the richer variant once both candidates lose the same amount of metadata") + require.Greater(t, len(fullShorterBody), len(richerBody), + "test setup must still require metadata trimming before the richer variant fits") + + selected, err := selectRichestRangePromptView(len(richerBody), templateContextFromRangeView(view), variants) + require.NoError(t, err) + require.NotNil(t, selected.Review) + require.NotNil(t, selected.Review.Subject.Range) + + assert.Equal(t, variants[0].Fallback, selected.Review.Fallback.Rendered()) + assert.Equal(t, []RangeEntryContext{{Commit: "abc1234", Subject: strings.Repeat("s", 200)}, {Commit: "def5678", Subject: strings.Repeat("s", 200)}, {Commit: "ghi9012"}}, selected.Review.Subject.Range.Entries) +} + +func TestSelectRichestRangePromptViewPrefersSummaryWhenRicherNeedsMoreTrimming(t *testing.T) { + view := rangePromptView{ + Current: commitRangeSectionView{Count: 2, Entries: []commitRangeEntryView{ + {Commit: "abc1234", Subject: strings.Repeat("s", 200)}, + {Commit: "def5678", Subject: strings.Repeat("s", 200)}, + }}, + } + variants := []diffSectionView{ + {Heading: "### Combined Diff", Fallback: strings.Repeat("richer guidance\n", 8)}, + {Heading: "### Combined Diff", Fallback: "short guidance\n"}, + } + shorterBody, err := renderRangePrompt(rangePromptView{Current: view.Current, Diff: variants[1]}) + require.NoError(t, err) + trimmedRicherCurrent := commitRangeSectionView{Count: 2, Entries: []commitRangeEntryView{ + {Commit: "abc1234", Subject: strings.Repeat("s", 200)}, + {Commit: "def5678"}, + }} + trimmedRicherBody, err := renderRangePrompt(rangePromptView{Current: trimmedRicherCurrent, Diff: variants[0]}) + require.NoError(t, err) + require.LessOrEqual(t, len(trimmedRicherBody), len(shorterBody), + "test setup must allow the richer variant only after trimming more metadata than the shorter variant needs") + + selected, err := selectRichestRangePromptView(len(shorterBody), templateContextFromRangeView(view), variants) + require.NoError(t, err) + require.NotNil(t, selected.Review) + require.NotNil(t, selected.Review.Subject.Range) + + assert.Equal(t, variants[1].Fallback, selected.Review.Fallback.Rendered()) + assert.Equal(t, []RangeEntryContext{{Commit: "abc1234", Subject: strings.Repeat("s", 200)}, {Commit: "def5678", Subject: strings.Repeat("s", 200)}}, selected.Review.Subject.Range.Entries) +} + +func TestSelectRichestRangePromptViewPrefersOptionalContextWhenRicherNeedsMoreTrimming(t *testing.T) { + view := rangePromptView{ + Optional: optionalSectionsView{AdditionalContext: strings.Repeat("context\n", 24)}, + Current: commitRangeSectionView{Count: 1, Entries: []commitRangeEntryView{{Commit: "abc1234", Subject: "summary"}}}, + } + variants := []diffSectionView{ + {Heading: "### Combined Diff", Fallback: strings.Repeat("richer guidance\n", 8)}, + {Heading: "### Combined Diff", Fallback: "short guidance\n"}, + } + shorterBody, err := renderRangePrompt(rangePromptView{Optional: view.Optional, Current: view.Current, Diff: variants[1]}) + require.NoError(t, err) + trimmedRicherBody, err := renderRangePrompt(rangePromptView{Current: view.Current, Diff: variants[0]}) + require.NoError(t, err) + require.LessOrEqual(t, len(trimmedRicherBody), len(shorterBody), + "test setup must allow the richer variant only after trimming more optional context than the shorter variant needs") + + selected, err := selectRichestRangePromptView(len(shorterBody), templateContextFromRangeView(view), variants) + require.NoError(t, err) + require.NotNil(t, selected.Review) + + assert.Equal(t, variants[1].Fallback, selected.Review.Fallback.Rendered()) + assert.Equal(t, view.Optional.AdditionalContext, selected.Review.Optional.AdditionalContext) +} + func TestBuildPromptNonCodexSmallCapStaysWithinCap(t *testing.T) { repoPath, sha := setupLargeDiffRepoWithGuidelines(t, 5000) cap := 10000 cfg := &config.Config{DefaultMaxPromptSize: cap} b := NewBuilderWithConfig(nil, cfg) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "claude-code", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, sha, 0, 0, "claude-code", "", "") require.NoError(t, err) assert.LessOrEqual(t, len(prompt), cap, @@ -766,7 +953,7 @@ func TestBuildRangePromptNonCodexSmallCapStaysWithinCap(t *testing.T) { cfg := &config.Config{DefaultMaxPromptSize: cap} b := NewBuilderWithConfig(nil, cfg) - prompt, err := b.BuildWithDiffFile(repoPath, rangeRef, 0, 0, "claude-code", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, rangeRef, 0, 0, "claude-code", "", "") require.NoError(t, err) assert.LessOrEqual(t, len(prompt), cap, @@ -787,6 +974,41 @@ func TestBuildDirtySmallCapStaysWithinCap(t *testing.T) { assert.LessOrEqual(t, len(prompt), cap, "dirty prompt should stay within configured cap") + + repoPath = t.TempDir() + guidelines := strings.Repeat("guideline ", 512) + require.NoError(t, os.WriteFile( + filepath.Join(repoPath, ".roborev.toml"), + []byte("review_guidelines = \""+guidelines+"\"\n"), + 0o644, + )) + + diff = strings.Repeat("+ keep full diff\n", 8) + var diffSection strings.Builder + diffSection.WriteString("### Diff\n\n") + diffSection.WriteString("```diff\n") + diffSection.WriteString(diff) + if !strings.HasSuffix(diff, "\n") { + diffSection.WriteString("\n") + } + diffSection.WriteString("```\n") + + cap = len(GetSystemPrompt("claude-code", "dirty")+"\n") + + len("## Uncommitted Changes\n\nThe following changes have not yet been committed.\n\n") + + diffSection.Len() + 32 + b = NewBuilderWithConfig(nil, &config.Config{DefaultMaxPromptSize: cap}) + + prompt, err = b.BuildDirty(repoPath, diff, 0, 0, "claude-code", "", "") + require.NoError(t, err) + + assert.LessOrEqual(t, len(prompt), cap, + "dirty prompt should stay within configured cap") + assertContains(t, prompt, diff, + "expected the full dirty diff to remain inline after trimming optional context") + assertNotContains(t, prompt, "(Diff too large to include in full)", + "dirty prompt should trim optional context before falling back to a truncated diff") + assertNotContains(t, prompt, guidelines, + "expected oversized optional context to be trimmed") } func TestResolveMaxPromptSizeWithoutConfigUsesConfigDefault(t *testing.T) { @@ -803,14 +1025,13 @@ func TestBuildPromptHonorsDefaultPromptCap(t *testing.T) { legacyCfg := &config.Config{DefaultMaxPromptSize: MaxPromptSize} legacyB := NewBuilderWithConfig(nil, legacyCfg) - dummyDiff := "/tmp/roborev-review-0.diff" - legacyPrompt, err := legacyB.BuildWithDiffFile(repoPath, sha, 0, 0, "claude-code", "", "", dummyDiff) + legacyPrompt, err := legacyB.Build(repoPath, sha, 0, 0, "claude-code", "", "") require.NoError(t, err) require.Greater(t, len(legacyPrompt), config.DefaultMaxPromptSize, "precondition: prompt with legacy cap should exceed the 200KB default") b := NewBuilder(nil) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "claude-code", "", "", dummyDiff) + prompt, err := b.Build(repoPath, sha, 0, 0, "claude-code", "", "") require.NoError(t, err) assert.LessOrEqual(t, len(prompt), config.DefaultMaxPromptSize, @@ -858,6 +1079,165 @@ func TestBuildPromptLargeGuidelinesPrefersDiffOverContext(t *testing.T) { "small diff should be inlined after trimming guidelines") } +func TestBuildDirtyTruncatedFallbackPreservesClosingFenceAtTightCap(t *testing.T) { + repoPath := t.TempDir() + diff := strings.Repeat("+ keep this line in the truncated fallback\n", 128) + + currentSection, err := renderDirtyChangesSection(dirtyChangesSectionView{ + Description: "The following changes have not yet been committed.", + }) + require.NoError(t, err) + fallbackOnly, err := renderDirtyTruncatedDiffFallback("") + require.NoError(t, err) + fallbackBlock, err := renderDiffBlock(diffSectionView{Heading: "### Diff", Fallback: fallbackOnly}) + require.NoError(t, err) + + bodyLimit := len(currentSection) + len(fallbackBlock) + 1005 + cap := len(GetSystemPrompt("claude-code", "dirty")+"\n") + bodyLimit + b := NewBuilderWithConfig(nil, &config.Config{DefaultMaxPromptSize: cap}) + + prompt, err := b.BuildDirty(repoPath, diff, 0, 0, "claude-code", "", "") + require.NoError(t, err) + + assert.LessOrEqual(t, len(prompt), cap) + assert.Contains(t, prompt, "(Diff too large to include in full)") + assert.Contains(t, prompt, "... (truncated)\n```\n", + "dirty truncated fallback should keep the truncation marker and closing fence") +} + +func TestBuildDirtyTruncatedFallbackTrimsOptionalContextBeforeShrinkingSnippet(t *testing.T) { + repoPath := t.TempDir() + guidelines := strings.Repeat("g", 4000) + toml := `review_guidelines = """` + "\n" + guidelines + "\n" + `"""` + "\n" + require.NoError(t, os.WriteFile(filepath.Join(repoPath, ".roborev.toml"), []byte(toml), 0o644)) + + diff := strings.Repeat("+ retained after optional trim\n", 128) + currentSection, err := renderDirtyChangesSection(dirtyChangesSectionView{ + Description: "The following changes have not yet been committed.", + }) + require.NoError(t, err) + fallbackOnly, err := renderDirtyTruncatedDiffFallback("") + require.NoError(t, err) + fallbackBlock, err := renderDiffBlock(diffSectionView{Heading: "### Diff", Fallback: fallbackOnly}) + require.NoError(t, err) + + bodyLimit := len(currentSection) + len(fallbackBlock) + 1200 + cap := len(GetSystemPrompt("claude-code", "dirty")+"\n") + bodyLimit + b := NewBuilderWithConfig(nil, &config.Config{DefaultMaxPromptSize: cap}) + + prompt, err := b.BuildDirty(repoPath, diff, 0, 0, "claude-code", "", "") + require.NoError(t, err) + + assert.LessOrEqual(t, len(prompt), cap) + assert.NotContains(t, prompt, guidelines, + "oversized optional guidelines should be trimmed before collapsing the truncated diff snippet") + assert.Contains(t, prompt, "```diff\n+ retained after optional trim", + "dirty prompt should retain a truncated diff snippet after optional context is trimmed") + assert.Contains(t, prompt, "... (truncated)\n```\n") +} + +func TestBuildDirtyTruncatedFallbackContinuesTrimmingOptionalContextForSnippet(t *testing.T) { + repoPath := t.TempDir() + guidelines := strings.Repeat("g", 1800) + toml := `review_guidelines = """` + "\n" + guidelines + "\n" + `"""` + "\n" + require.NoError(t, os.WriteFile(filepath.Join(repoPath, ".roborev.toml"), []byte(toml), 0o644)) + additionalContext := "## Pull Request Discussion\n\n" + strings.Repeat("discussion\n", 40) + diff := strings.Repeat("+ retain after second trim\n", 128) + + fallbackOnly, err := renderDirtyTruncatedDiffFallback("") + require.NoError(t, err) + snippetBody := strings.Repeat("+ retain after second trim\n", 16) + "... (truncated)\n" + snippetFallback, err := renderDirtyTruncatedDiffFallback(snippetBody) + require.NoError(t, err) + requiredView := dirtyPromptView{ + Current: dirtyChangesSectionView{Description: "The following changes have not yet been committed."}, + Diff: diffSectionView{Heading: "### Diff", Fallback: snippetFallback}, + } + guidelinesView := requiredView + guidelinesView.Optional.ProjectGuidelines = buildProjectGuidelinesSectionView(guidelines) + fullOptionalEmptyView := dirtyPromptView{ + Optional: optionalSectionsView{ + ProjectGuidelines: buildProjectGuidelinesSectionView(guidelines), + AdditionalContext: buildAdditionalContextSection(additionalContext), + }, + Current: dirtyChangesSectionView{Description: "The following changes have not yet been committed."}, + Diff: diffSectionView{Heading: "### Diff", Fallback: fallbackOnly}, + } + guidelinesEmptyView := fullOptionalEmptyView + guidelinesEmptyView.Optional.AdditionalContext = "" + fullOptionalSnippetView := fullOptionalEmptyView + fullOptionalSnippetView.Diff.Fallback = snippetFallback + guidelinesSnippetView := guidelinesView + + requiredSnippetBody, err := renderDirtyPrompt(requiredView) + require.NoError(t, err) + guidelinesSnippetBody, err := renderDirtyPrompt(guidelinesSnippetView) + require.NoError(t, err) + fullOptionalEmptyBody, err := renderDirtyPrompt(fullOptionalEmptyView) + require.NoError(t, err) + guidelinesEmptyBody, err := renderDirtyPrompt(guidelinesEmptyView) + require.NoError(t, err) + fullOptionalSnippetBody, err := renderDirtyPrompt(fullOptionalSnippetView) + require.NoError(t, err) + + lowerBound := max(len(requiredSnippetBody), len(guidelinesEmptyBody)) + upperBound := min(len(guidelinesSnippetBody), len(fullOptionalEmptyBody), len(fullOptionalSnippetBody)) + require.Greater(t, upperBound, lowerBound, + "test setup must require trimming additional context for empty fallback and further optional trimming for the snippet") + + bodyLimit := lowerBound + (upperBound-lowerBound)/2 + cap := len(GetSystemPrompt("claude-code", "dirty")+"\n") + bodyLimit + b := NewBuilderWithConfig(nil, &config.Config{DefaultMaxPromptSize: cap}) + + prompt, err := b.BuildDirty(repoPath, diff, 0, 0, "claude-code", "", "") + require.NoError(t, err) + + assert.LessOrEqual(t, len(prompt), cap) + assert.NotContains(t, prompt, guidelines) + assert.NotContains(t, prompt, "discussion\n") + assert.GreaterOrEqual(t, strings.Count(prompt, "+ retain after second trim\n"), 16, + "dirty truncated fallback should keep sizing the snippet after trimming all removable optional context") +} + +func TestBuildDirtyFallbackOnlyRestoresOptionalContextWhenSnippetCannotFit(t *testing.T) { + repoPath := t.TempDir() + guidelines := "Keep it simple." + toml := `review_guidelines = """` + "\n" + guidelines + "\n" + `"""` + "\n" + require.NoError(t, os.WriteFile(filepath.Join(repoPath, ".roborev.toml"), []byte(toml), 0o644)) + diff := strings.Repeat("+ snippet cannot fit here\n", 128) + + fallbackOnly, err := renderDirtyTruncatedDiffFallback("") + require.NoError(t, err) + singleLineFallback, err := renderDirtyTruncatedDiffFallback("+ snippet cannot fit here\n... (truncated)\n") + require.NoError(t, err) + guidelinesFallbackView := dirtyPromptView{ + Optional: optionalSectionsView{ProjectGuidelines: buildProjectGuidelinesSectionView(guidelines)}, + Current: dirtyChangesSectionView{Description: "The following changes have not yet been committed."}, + Diff: diffSectionView{Heading: "### Diff", Fallback: fallbackOnly}, + } + requiredSingleLineView := dirtyPromptView{ + Current: dirtyChangesSectionView{Description: "The following changes have not yet been committed."}, + Diff: diffSectionView{Heading: "### Diff", Fallback: singleLineFallback}, + } + guidelinesFallbackBody, err := renderDirtyPrompt(guidelinesFallbackView) + require.NoError(t, err) + requiredSingleLineBody, err := renderDirtyPrompt(requiredSingleLineView) + require.NoError(t, err) + require.NotEmpty(t, requiredSingleLineBody) + + cap := len(GetSystemPrompt("claude-code", "dirty")+"\n") + len(guidelinesFallbackBody) + b := NewBuilderWithConfig(nil, &config.Config{DefaultMaxPromptSize: cap}) + + prompt, err := b.BuildDirty(repoPath, diff, 0, 0, "claude-code", "", "") + require.NoError(t, err) + + assert.LessOrEqual(t, len(prompt), cap) + assert.Contains(t, prompt, guidelines, + "fallback-only dirty prompt should restore optional context that still fits once snippet sizing fails") + assert.NotContains(t, prompt, "```diff", + "final prompt should fall back to the marker-only dirty diff when no snippet can fit") +} + func TestBuildDirtySmallCapTruncatesUTF8Safely(t *testing.T) { repoPath, _ := setupLargeDiffRepoWithGuidelines(t, 5000) diff := strings.Repeat("+ ascii line\n", 256) + strings.Repeat("+ 世界\n", 4096) @@ -878,7 +1258,7 @@ func TestBuildPromptCodexTinyCapStillStaysWithinCap(t *testing.T) { cfg := &config.Config{DefaultMaxPromptSize: cap} b := NewBuilderWithConfig(nil, cfg) - prompt, err := b.BuildWithDiffFile(repoPath, sha, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") require.NoError(t, err) assert.LessOrEqual(t, len(prompt), cap) @@ -891,13 +1271,77 @@ func TestBuildRangePromptCodexTinyCapStillStaysWithinCap(t *testing.T) { cfg := &config.Config{DefaultMaxPromptSize: cap} b := NewBuilderWithConfig(nil, cfg) - prompt, err := b.BuildWithDiffFile(repoPath, rangeRef, 0, 0, "codex", "", "", "/tmp/roborev-review-0.diff") + prompt, err := b.Build(repoPath, rangeRef, 0, 0, "codex", "", "") require.NoError(t, err) assert.LessOrEqual(t, len(prompt), cap) assert.True(t, utf8.ValidString(prompt), "range prompt should remain valid UTF-8 after hard capping") } +func TestBuildPromptCodexOversizedDiffFallbackCarriesExcludeScope(t *testing.T) { + repoPath, sha := setupLargeExcludePatternRepo(t) + cfg := &config.Config{ + DefaultMaxPromptSize: 4096, + ExcludePatterns: []string{"custom.dat"}, + } + b := NewBuilderWithConfig(nil, cfg) + + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") + require.NoError(t, err) + + assertContains(t, prompt, `:(exclude,glob)**/custom.dat`, + "expected Codex fallback commands to preserve custom exclude patterns") + assertNotContains(t, prompt, `:(exclude,glob)**/go.sum`, + "built-in lockfile excludes should not appear in fallback commands") +} + +func TestBuildPromptCodexShortestFallbackCarriesExcludeScope(t *testing.T) { + repoPath, sha := setupLargeExcludePatternRepo(t) + pathspecArgs := gitpkg.FormatExcludeArgs([]string{"custom.dat"}) + variants := codexCommitInspectionFallbackVariants(sha, pathspecArgs) + shortest := mustRenderPromptTestDiffBlock(variants[len(variants)-1]) + secondShortest := mustRenderPromptTestDiffBlock(variants[len(variants)-2]) + prefixLen := singleCommitPromptPrefixLen(t, repoPath, sha) + cap := prefixLen + len(shortest) + max(1, (len(secondShortest)-len(shortest))/2) + cfg := &config.Config{ + DefaultMaxPromptSize: cap, + ExcludePatterns: []string{"custom.dat"}, + } + b := NewBuilderWithConfig(nil, cfg) + + prompt, err := b.Build(repoPath, sha, 0, 0, "codex", "", "") + require.NoError(t, err) + + assert.LessOrEqual(t, len(prompt), cap) + assert.Contains(t, prompt, shortest, "expected tiny-cap prompt to use the shortest fallback variant") + assertContains(t, prompt, `:(exclude,glob)**/custom.dat`, + "expected shortest Codex fallback command to preserve custom exclude patterns") +} + +func TestBuildRangePromptCodexShortestFallbackCarriesExcludeScope(t *testing.T) { + repoPath, sha := setupLargeExcludePatternRepo(t) + rangeRef := sha + "~1.." + sha + pathspecArgs := gitpkg.FormatExcludeArgs([]string{"custom.dat"}) + variants := codexRangeInspectionFallbackVariants(rangeRef, pathspecArgs) + shortest := mustRenderPromptTestDiffBlock(variants[len(variants)-1]) + secondShortest := mustRenderPromptTestDiffBlock(variants[len(variants)-2]) + prefixLen := rangePromptPrefixLen(t, repoPath, rangeRef) + cap := prefixLen + len(shortest) + max(1, (len(secondShortest)-len(shortest))/2) + cfg := &config.Config{ + DefaultMaxPromptSize: cap, + ExcludePatterns: []string{"custom.dat"}, + } + b := NewBuilderWithConfig(nil, cfg) + + prompt, err := b.Build(repoPath, rangeRef, 0, 0, "codex", "", "") + require.NoError(t, err) + + assert.LessOrEqual(t, len(prompt), cap) + assertContains(t, prompt, "git diff", "expected tiny-cap range prompt to retain Codex diff guidance") + assertContains(t, prompt, `:(exclude,glob)**/custom.dat`, + "expected tiny-cap range fallback command to preserve custom exclude patterns") +} + func TestLoadGuidelines(t *testing.T) { tests := []struct { name string @@ -1172,213 +1616,22 @@ func TestBuildAddressPromptShowsFullDiff(t *testing.T) { assertContains(t, diffSection, "custom.dat", "excluded file should still be in address prompt diff") } -func TestSplitResponses(t *testing.T) { - responses := []storage.Response{ - {Responder: "roborev-fix", Response: "Fix applied"}, - {Responder: "alice", Response: "This is a false positive"}, - {Responder: "roborev-refine", Response: "Created commit abc123"}, - {Responder: "bob", Response: "Please keep this function"}, - } - - tool, user := SplitResponses(responses) - - assert.Len(t, tool, 2) - assert.Equal(t, "roborev-fix", tool[0].Responder) - assert.Equal(t, "roborev-refine", tool[1].Responder) - - assert.Len(t, user, 2) - assert.Equal(t, "alice", user[0].Responder) - assert.Equal(t, "bob", user[1].Responder) -} - -func TestSplitResponsesEmpty(t *testing.T) { - tool, user := SplitResponses(nil) - assert.Nil(t, tool) - assert.Nil(t, user) -} - -func TestFormatUserComments(t *testing.T) { - t.Run("nil returns empty", func(t *testing.T) { - assert.Empty(t, FormatUserComments(nil)) - }) - - t.Run("empty slice returns empty", func(t *testing.T) { - assert.Empty(t, FormatUserComments([]storage.Response{})) - }) - - t.Run("includes comment content", func(t *testing.T) { - comments := []storage.Response{ - {Responder: "alice", Response: "This is a false positive", CreatedAt: time.Date(2026, 3, 15, 10, 30, 0, 0, time.UTC)}, - {Responder: "bob", Response: "Please use a different approach", CreatedAt: time.Date(2026, 3, 15, 11, 0, 0, 0, time.UTC)}, - } - result := FormatUserComments(comments) - assert.Contains(t, result, "User Comments") - assert.Contains(t, result, "false positive") - assert.Contains(t, result, "different approach") - assert.Contains(t, result, "alice") - assert.Contains(t, result, "bob") - }) -} - -func TestFormatToolAttempts(t *testing.T) { - t.Run("nil returns empty", func(t *testing.T) { - assert.Empty(t, FormatToolAttempts(nil)) - }) - - t.Run("includes attempt content", func(t *testing.T) { - attempts := []storage.Response{ - {Responder: "roborev-fix", Response: "Fix applied (commit: abc123)", CreatedAt: time.Date(2026, 3, 15, 10, 0, 0, 0, time.UTC)}, - } - result := FormatToolAttempts(attempts) - assert.Contains(t, result, "Previous Addressing Attempts") - assert.Contains(t, result, "roborev-fix") - assert.Contains(t, result, "abc123") - }) -} - -func TestBuildAddressPromptSplitsResponses(t *testing.T) { - r := newTestRepo(t) - +func TestBuildAddressPromptRendersPreviousAttemptsAndOriginalDiff(t *testing.T) { + repoPath, sha := setupExcludePatternRepo(t) b := NewBuilder(nil) - responses := []storage.Response{ - {Responder: "roborev-fix", Response: "Fix applied", CreatedAt: time.Date(2026, 3, 15, 9, 0, 0, 0, time.UTC)}, - {Responder: "alice", Response: "This is a false positive", CreatedAt: time.Date(2026, 3, 15, 10, 0, 0, 0, time.UTC)}, - } review := &storage.Review{ - JobID: 1, Agent: "test", - Output: "Found bug in foo.go", - } - - p, err := b.BuildAddressPrompt(r.dir, review, responses, "") - require.NoError(t, err) - - // Tool attempts should appear under "Previous Addressing Attempts" - assert.Contains(t, p, "Previous Addressing Attempts") - assert.Contains(t, p, "roborev-fix") - - // User comments should appear under "User Comments" - assert.Contains(t, p, "User Comments") - assert.Contains(t, p, "false positive") - assert.Contains(t, p, "alice") -} - -func TestBuildRangePrompt_IncludesInRangeReviews(t *testing.T) { - r := newTestRepo(t) - - // Create initial commit (will serve as range start / merge base) - require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("base\n"), 0o644)) - r.git("add", "base.txt") - r.git("commit", "-m", "initial") - baseSHA := r.git("rev-parse", "HEAD") - - // Create two commits on the "branch" - require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("change1\n"), 0o644)) - r.git("add", "base.txt") - r.git("commit", "-m", "first feature commit") - commit1 := r.git("rev-parse", "HEAD") - - require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("change2\n"), 0o644)) - r.git("add", "base.txt") - r.git("commit", "-m", "second feature commit") - commit2 := r.git("rev-parse", "HEAD") - - // Set up DB with repo and per-commit reviews - db := testutil.OpenTestDB(t) - repo, err := db.GetOrCreateRepo(r.dir) - require.NoError(t, err) - - testutil.CreateCompletedReview(t, db, repo.ID, commit1, "test", - "Found bug: missing null check in handler\n\nVerdict: FAIL") - testutil.CreateCompletedReview(t, db, repo.ID, commit2, "test", - "No issues found.\n\nVerdict: PASS") - - // Build range prompt - rangeRef := baseSHA + ".." + commit2 - builder := NewBuilder(db) - prompt, err := builder.Build(r.dir, rangeRef, repo.ID, 0, "test", "", "") - require.NoError(t, err) - - // Should contain the in-range reviews section - assert := assert.New(t) - assert.Contains(prompt, "Per-Commit Reviews in This Range") - assert.Contains(prompt, "Do not re-raise issues") - assert.Contains(prompt, "missing null check in handler") - assert.Contains(prompt, "No issues found.") - assert.Contains(prompt, "failed") - assert.Contains(prompt, "passed") -} - -func TestBuildRangePrompt_NoInRangeReviewsWithoutDB(t *testing.T) { - r := newTestRepo(t) - - require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("base\n"), 0o644)) - r.git("add", "base.txt") - r.git("commit", "-m", "initial") - baseSHA := r.git("rev-parse", "HEAD") - - require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("change\n"), 0o644)) - r.git("add", "base.txt") - r.git("commit", "-m", "feature") - headSHA := r.git("rev-parse", "HEAD") - - // Build without DB — should not include in-range reviews section - builder := NewBuilder(nil) - prompt, err := builder.Build(r.dir, baseSHA+".."+headSHA, 0, 0, "test", "", "") - require.NoError(t, err) - - assert.NotContains(t, prompt, "Per-Commit Reviews in This Range") -} - -func TestBuildInjectsSeverityInstruction(t *testing.T) { - assert := assert.New(t) - repoPath, commits := setupTestRepo(t) - targetSHA := commits[len(commits)-1] - - builder := NewBuilder(nil) - - tests := []struct { - name string - reviewType string - minSeverity string - wantContain string - wantAbsent string - }{ - {"standard with medium", "", "medium", "Severity filter:", ""}, - {"security with high", "security", "high", "Severity filter:", ""}, - {"design with critical", "design", "critical", "Severity filter:", ""}, - {"empty severity skips injection", "", "", "", "Severity filter:"}, - {"low severity skips injection", "", "low", "", "Severity filter:"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - prompt, err := builder.Build(repoPath, targetSHA, 0, 0, "test", tt.reviewType, tt.minSeverity) - require.NoError(t, err) - if tt.wantContain != "" { - assert.Contains(prompt, tt.wantContain) - } - if tt.wantAbsent != "" { - assert.NotContains(prompt, tt.wantAbsent) - } - }) + Output: "Found issue: check custom.dat", + Job: &storage.ReviewJob{GitRef: sha}, } -} + attempts := []storage.Response{{Responder: "developer", Response: "Tried a narrow fix"}} -func TestBuildDirtyInjectsSeverityInstruction(t *testing.T) { - assert := assert.New(t) - repoPath, _ := setupTestRepo(t) - builder := NewBuilder(nil) - - diff := "diff --git a/foo.go b/foo.go\n--- a/foo.go\n+++ b/foo.go\n@@ -1 +1 @@\n-old\n+new\n" - - prompt, err := builder.BuildDirty(repoPath, diff, 0, 0, "test", "", "high") + prompt, err := b.BuildAddressPrompt(repoPath, review, attempts, "medium") require.NoError(t, err) - assert.Contains(prompt, "Severity filter:") - assert.Contains(prompt, "SEVERITY_THRESHOLD_MET") - // Empty severity: no injection - prompt2, err := builder.BuildDirty(repoPath, diff, 0, 0, "test", "", "") - require.NoError(t, err) - assert.NotContains(prompt2, "Severity filter:") + assert.Contains(t, prompt, "## Previous Addressing Attempts") + assert.Contains(t, prompt, "Tried a narrow fix") + assert.Contains(t, prompt, "## Review Findings to Address") + assert.Contains(t, prompt, "## Original Commit Diff") } diff --git a/internal/prompt/template_context.go b/internal/prompt/template_context.go new file mode 100644 index 00000000..fa10472f --- /dev/null +++ b/internal/prompt/template_context.go @@ -0,0 +1,362 @@ +package prompt + +import "slices" + +type TemplateContext struct { + Meta PromptMeta + Review *ReviewTemplateContext + Address *AddressTemplateContext + System *SystemTemplateContext +} + +func (c TemplateContext) Clone() TemplateContext { + cloned := c + if c.Review != nil { + review := c.Review.Clone() + cloned.Review = &review + } + if c.Address != nil { + address := c.Address.Clone() + cloned.Address = &address + } + if c.System != nil { + system := *c.System + cloned.System = &system + } + return cloned +} + +type PromptMeta struct { + AgentName string + PromptType string + ReviewType string +} + +type ReviewTemplateContext struct { + Kind ReviewKind + Optional ReviewOptionalContext + Subject SubjectContext + Diff DiffContext + Fallback FallbackContext +} + +func (c ReviewTemplateContext) Clone() ReviewTemplateContext { + cloned := c + cloned.Optional = c.Optional.Clone() + cloned.Subject = c.Subject.Clone() + cloned.Diff = c.Diff.Clone() + cloned.Fallback = c.Fallback.Clone() + return cloned +} + +type ReviewKind string + +const ( + ReviewKindSingle ReviewKind = "single" + ReviewKindRange ReviewKind = "range" + ReviewKindDirty ReviewKind = "dirty" +) + +type ReviewOptionalContext struct { + ProjectGuidelines *MarkdownSection + AdditionalContext string + PreviousReviews []PreviousReviewTemplateContext + PreviousAttempts []ReviewAttemptTemplateContext +} + +func (o ReviewOptionalContext) Clone() ReviewOptionalContext { + cloned := o + if o.ProjectGuidelines != nil { + section := *o.ProjectGuidelines + cloned.ProjectGuidelines = §ion + } + cloned.PreviousReviews = slices.Clone(o.PreviousReviews) + for i := range cloned.PreviousReviews { + cloned.PreviousReviews[i].Comments = slices.Clone(cloned.PreviousReviews[i].Comments) + } + cloned.PreviousAttempts = slices.Clone(o.PreviousAttempts) + for i := range cloned.PreviousAttempts { + cloned.PreviousAttempts[i].Comments = slices.Clone(cloned.PreviousAttempts[i].Comments) + } + return cloned +} + +func (o ReviewOptionalContext) IsEmpty() bool { + return o.ProjectGuidelines == nil && + o.AdditionalContext == "" && + len(o.PreviousReviews) == 0 && + len(o.PreviousAttempts) == 0 +} + +func (o ReviewOptionalContext) ProjectGuidelinesBody() string { + if o.ProjectGuidelines == nil { + return "" + } + return o.ProjectGuidelines.Body +} + +func (o *ReviewOptionalContext) TrimNext() bool { + if o == nil { + return false + } + switch { + case len(o.PreviousAttempts) > 0: + o.PreviousAttempts = nil + case len(o.PreviousReviews) > 0: + o.PreviousReviews = nil + case o.AdditionalContext != "": + o.AdditionalContext = "" + case o.ProjectGuidelines != nil: + o.ProjectGuidelines = nil + default: + return false + } + return true +} + +type SubjectContext struct { + Single *SingleSubjectContext + Range *RangeSubjectContext + Dirty *DirtySubjectContext +} + +func (s SubjectContext) Clone() SubjectContext { + cloned := s + if s.Single != nil { + single := *s.Single + cloned.Single = &single + } + if s.Range != nil { + rangeCtx := *s.Range + rangeCtx.Entries = slices.Clone(s.Range.Entries) + cloned.Range = &rangeCtx + } + if s.Dirty != nil { + dirty := *s.Dirty + cloned.Dirty = &dirty + } + return cloned +} + +func (s *SubjectContext) TrimSingleMessage() bool { + if s == nil || s.Single == nil || s.Single.Message == "" { + return false + } + s.Single.Message = "" + return true +} + +func (s *SubjectContext) TrimSingleAuthor() bool { + if s == nil || s.Single == nil || s.Single.Author == "" { + return false + } + s.Single.Author = "" + return true +} + +func (s *SubjectContext) TrimSingleSubjectTo(maxBytes int) bool { + if s == nil || s.Single == nil || s.Single.Subject == "" { + return false + } + next := truncateUTF8(s.Single.Subject, maxBytes) + if next == s.Single.Subject { + return false + } + s.Single.Subject = next + return true +} + +func (s *SubjectContext) BlankNextRangeSubject() bool { + if s == nil || s.Range == nil { + return false + } + for i := len(s.Range.Entries) - 1; i >= 0; i-- { + if s.Range.Entries[i].Subject == "" { + continue + } + s.Range.Entries[i].Subject = "" + return true + } + return false +} + +func (s *SubjectContext) DropLastRangeEntry() bool { + if s == nil || s.Range == nil || len(s.Range.Entries) == 0 { + return false + } + s.Range.Entries = s.Range.Entries[:len(s.Range.Entries)-1] + return true +} + +type SingleSubjectContext struct { + Commit string + Subject string + Author string + Message string +} + +type RangeSubjectContext struct { + Count int + Entries []RangeEntryContext +} + +type RangeEntryContext struct { + Commit string + Subject string +} + +type DirtySubjectContext struct { + Description string +} + +type DiffContext struct { + Heading string + Body string +} + +func (d DiffContext) Clone() DiffContext { + return d +} + +func (d DiffContext) HasBody() bool { + return d.Body != "" +} + +type FallbackMode string + +const ( + FallbackModeNone FallbackMode = "" + FallbackModeCommit FallbackMode = "commit" + FallbackModeRange FallbackMode = "range" + FallbackModeDirty FallbackMode = "dirty" + FallbackModeGeneric FallbackMode = "generic" +) + +type FallbackContext struct { + Mode FallbackMode + Text string + Commit *CommitFallbackContext + Range *RangeFallbackContext + Dirty *DirtyFallbackContext + Generic *GenericFallbackContext +} + +func (f FallbackContext) Clone() FallbackContext { + cloned := f + if f.Commit != nil { + commit := *f.Commit + cloned.Commit = &commit + } + if f.Range != nil { + rangeCtx := *f.Range + cloned.Range = &rangeCtx + } + if f.Dirty != nil { + dirty := *f.Dirty + cloned.Dirty = &dirty + } + if f.Generic != nil { + generic := *f.Generic + cloned.Generic = &generic + } + return cloned +} + +func (f FallbackContext) IsEmpty() bool { + return f.Mode == FallbackModeNone && f.Text == "" && f.Commit == nil && f.Range == nil && f.Dirty == nil && f.Generic == nil +} + +func (f FallbackContext) HasContent() bool { + return !f.IsEmpty() +} + +func (f FallbackContext) Rendered() string { + switch { + case f.Text != "": + return f.Text + case f.Dirty != nil: + return f.Dirty.Body + default: + return "" + } +} + +type CommitFallbackContext struct { + SHA string + StatCmd string + DiffCmd string + FilesCmd string + ShowPathCmd string +} + +type RangeFallbackContext struct { + RangeRef string + LogCmd string + StatCmd string + DiffCmd string + FilesCmd string + ViewCmd string +} + +type DirtyFallbackContext struct { + Body string +} + +type GenericFallbackContext struct { + ViewCmd string +} + +type AddressTemplateContext struct { + ProjectGuidelines *MarkdownSection + PreviousAttempts []AddressAttemptTemplateContext + SeverityFilter string + ReviewFindings string + OriginalDiff string + JobID int64 +} + +func (c AddressTemplateContext) Clone() AddressTemplateContext { + cloned := c + if c.ProjectGuidelines != nil { + section := *c.ProjectGuidelines + cloned.ProjectGuidelines = §ion + } + cloned.PreviousAttempts = slices.Clone(c.PreviousAttempts) + return cloned +} + +type SystemTemplateContext struct { + NoSkillsInstruction string + CurrentDate string +} + +type MarkdownSection struct { + Heading string + Body string +} + +type ReviewCommentTemplateContext struct { + Responder string + Response string +} + +type PreviousReviewTemplateContext struct { + Commit string + Output string + Comments []ReviewCommentTemplateContext + Available bool +} + +type ReviewAttemptTemplateContext struct { + Label string + Agent string + When string + Output string + Comments []ReviewCommentTemplateContext +} + +type AddressAttemptTemplateContext struct { + Responder string + Response string + When string +} diff --git a/internal/prompt/templates/assembled_address.md.gotmpl b/internal/prompt/templates/assembled_address.md.gotmpl index 61fe7a32..90ad81d8 100644 --- a/internal/prompt/templates/assembled_address.md.gotmpl +++ b/internal/prompt/templates/assembled_address.md.gotmpl @@ -1 +1 @@ -{{template "project_guidelines" .}}{{template "address_tool_attempts" .}}{{template "address_user_comments" .}}{{template "address_findings" .}} \ No newline at end of file +{{template "project_guidelines" .Address}}{{template "address_attempts" .Address}}{{template "address_findings" .Address}} \ No newline at end of file diff --git a/internal/prompt/templates/assembled_dirty.md.gotmpl b/internal/prompt/templates/assembled_dirty.md.gotmpl index 051fdc86..6ccfc0e9 100644 --- a/internal/prompt/templates/assembled_dirty.md.gotmpl +++ b/internal/prompt/templates/assembled_dirty.md.gotmpl @@ -1 +1 @@ -{{template "optional_sections" .Optional}}{{template "dirty_changes" .Current}}{{template "diff_block" .Diff}} \ No newline at end of file +{{template "optional_sections" .Review.Optional}}{{template "dirty_changes" .Review.Subject.Dirty}}{{template "diff_block" .Review}} \ No newline at end of file diff --git a/internal/prompt/templates/assembled_range.md.gotmpl b/internal/prompt/templates/assembled_range.md.gotmpl index 7c376a45..6d92834e 100644 --- a/internal/prompt/templates/assembled_range.md.gotmpl +++ b/internal/prompt/templates/assembled_range.md.gotmpl @@ -1 +1 @@ -{{template "optional_sections" .Optional}}{{template "commit_range" .Current}}{{template "diff_block" .Diff}} \ No newline at end of file +{{template "optional_sections" .Review.Optional}}{{template "commit_range" .Review.Subject.Range}}{{template "diff_block" .Review}} \ No newline at end of file diff --git a/internal/prompt/templates/assembled_single.md.gotmpl b/internal/prompt/templates/assembled_single.md.gotmpl index a031de71..8cbb31ad 100644 --- a/internal/prompt/templates/assembled_single.md.gotmpl +++ b/internal/prompt/templates/assembled_single.md.gotmpl @@ -1 +1 @@ -{{template "optional_sections" .Optional}}{{template "current_commit" .Current}}{{template "diff_block" .Diff}} \ No newline at end of file +{{template "optional_sections" .Review.Optional}}{{template "current_commit" .Review.Subject.Single}}{{template "diff_block" .Review}} \ No newline at end of file diff --git a/internal/prompt/templates/claude-code_review.md.gotmpl b/internal/prompt/templates/claude-code_review.md.gotmpl index 438c5ac7..f7984dc0 100644 --- a/internal/prompt/templates/claude-code_review.md.gotmpl +++ b/internal/prompt/templates/claude-code_review.md.gotmpl @@ -24,6 +24,6 @@ Do not include any front matter such as "Reviewing the diff..." or "I'm checking 4. **Regressions**: Changes that might break existing functionality. 5. **Code quality**: Duplication, overly complex logic, unclear naming. -Do not review the commit message. Focus only on the code changes in the diff.{{.NoSkillsInstruction}} +Do not review the commit message. Focus only on the code changes in the diff.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/codex_review.md.gotmpl b/internal/prompt/templates/codex_review.md.gotmpl index c80f59b4..f7984dc0 100644 --- a/internal/prompt/templates/codex_review.md.gotmpl +++ b/internal/prompt/templates/codex_review.md.gotmpl @@ -3,7 +3,6 @@ You are a code reviewer. Review the code changes shown below. Return only the final review. Do NOT narrate your process, mention files you opened, or describe intermediate checks. If you use tools while reviewing, finish all tool use before emitting the final review, and put the final review only after the last tool call. Do NOT build the project, run the test suite, or execute the code while reviewing. Base your review on the diff and static analysis only. -Do NOT search or read files outside the repository checkout, except for an explicitly provided diff file path for this review. Do not use tools like rg, find, or cat on unrelated paths outside the repo. ## Output Format @@ -25,6 +24,6 @@ Do not include any front matter such as "Reviewing the diff..." or "I'm checking 4. **Regressions**: Changes that might break existing functionality. 5. **Code quality**: Duplication, overly complex logic, unclear naming. -Do not review the commit message. Focus only on the code changes in the diff.{{.NoSkillsInstruction}} +Do not review the commit message. Focus only on the code changes in the diff.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/default_address.md.gotmpl b/internal/prompt/templates/default_address.md.gotmpl index 9e795b74..ea99471e 100644 --- a/internal/prompt/templates/default_address.md.gotmpl +++ b/internal/prompt/templates/default_address.md.gotmpl @@ -24,6 +24,6 @@ Changes: - ... -Keep the summary concise (under 10 bullet points). Put the most important changes first.{{.NoSkillsInstruction}} +Keep the summary concise (under 10 bullet points). Put the most important changes first.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/default_design_review.md.gotmpl b/internal/prompt/templates/default_design_review.md.gotmpl index b7a0e817..fac1b8ed 100644 --- a/internal/prompt/templates/default_design_review.md.gotmpl +++ b/internal/prompt/templates/default_design_review.md.gotmpl @@ -20,6 +20,6 @@ After reviewing, provide: 4. Any missing considerations not covered by the design 5. A verdict: Pass or Fail with brief justification -If you find no issues, state "No issues found." after the summary.{{.NoSkillsInstruction}} +If you find no issues, state "No issues found." after the summary.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/default_dirty.md.gotmpl b/internal/prompt/templates/default_dirty.md.gotmpl index 9cf1607e..bfa2bb63 100644 --- a/internal/prompt/templates/default_dirty.md.gotmpl +++ b/internal/prompt/templates/default_dirty.md.gotmpl @@ -14,6 +14,6 @@ After reviewing, provide: - File and line reference where possible - A brief explanation of the problem and suggested fix -If you find no issues, state "No issues found." after the summary.{{.NoSkillsInstruction}} +If you find no issues, state "No issues found." after the summary.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/default_range.md.gotmpl b/internal/prompt/templates/default_range.md.gotmpl index 3dc42947..e28714c6 100644 --- a/internal/prompt/templates/default_range.md.gotmpl +++ b/internal/prompt/templates/default_range.md.gotmpl @@ -16,6 +16,6 @@ After reviewing, provide: - File and line reference where possible - A brief explanation of the problem and suggested fix -If you find no issues, state "No issues found." after the summary.{{.NoSkillsInstruction}} +If you find no issues, state "No issues found." after the summary.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/default_review.md.gotmpl b/internal/prompt/templates/default_review.md.gotmpl index 008b9faa..3359ce30 100644 --- a/internal/prompt/templates/default_review.md.gotmpl +++ b/internal/prompt/templates/default_review.md.gotmpl @@ -16,6 +16,6 @@ After reviewing, provide: - File and line reference where possible - A brief explanation of the problem and suggested fix -If you find no issues, state "No issues found." after the summary.{{.NoSkillsInstruction}} +If you find no issues, state "No issues found." after the summary.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/default_security.md.gotmpl b/internal/prompt/templates/default_security.md.gotmpl index 71d54e8c..1a78e767 100644 --- a/internal/prompt/templates/default_security.md.gotmpl +++ b/internal/prompt/templates/default_security.md.gotmpl @@ -18,6 +18,6 @@ For each finding, provide: - Suggested remediation If you find no security issues, state "No issues found." after the summary. -Do not report code quality or style issues unless they have security implications.{{.NoSkillsInstruction}} +Do not report code quality or style issues unless they have security implications.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/gemini_review.md.gotmpl b/internal/prompt/templates/gemini_review.md.gotmpl index 53a8a10a..47816269 100644 --- a/internal/prompt/templates/gemini_review.md.gotmpl +++ b/internal/prompt/templates/gemini_review.md.gotmpl @@ -23,6 +23,6 @@ Do NOT build the project, run the test suite, or execute the code while reviewin 4. **Regressions**: Changes that might break existing functionality. 5. **Code quality**: Duplication, overly complex logic, unclear naming. -Do not review the commit message. Focus ONLY on the code changes in the diff.{{.NoSkillsInstruction}} +Do not review the commit message. Focus ONLY on the code changes in the diff.{{.System.NoSkillsInstruction}} -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/gemini_run.md.gotmpl b/internal/prompt/templates/gemini_run.md.gotmpl index 1d27d0b2..9da099bb 100644 --- a/internal/prompt/templates/gemini_run.md.gotmpl +++ b/internal/prompt/templates/gemini_run.md.gotmpl @@ -7,5 +7,5 @@ Focus on: - Providing code examples when helpful - Being practical and actionable -Current date: {{.CurrentDate}} (UTC) +Current date: {{.System.CurrentDate}} (UTC) diff --git a/internal/prompt/templates/prompt_sections.md.gotmpl b/internal/prompt/templates/prompt_sections.md.gotmpl index 16b78dfa..0cfa2a49 100644 --- a/internal/prompt/templates/prompt_sections.md.gotmpl +++ b/internal/prompt/templates/prompt_sections.md.gotmpl @@ -23,20 +23,6 @@ or provide context that affects how you should evaluate similar code in the curr {{range .PreviousReviews}}--- Review for commit {{.Commit}} --- {{if .Available}}{{.Output}}{{else}}No review available.{{end}} -{{template "review_comments" .Comments}} -{{end}}{{end}} -{{end}} -{{define "in_range_reviews"}}{{if .InRangeReviews}}## Per-Commit Reviews in This Range - -The following commits in this range have already been individually reviewed. -Issues found in earlier commits may have been fixed by later commits in the range. - -Do not re-raise issues identified below unless they persist in the final code. -Focus on cross-commit interactions and problems not caught by per-commit reviews. - -{{range .InRangeReviews}}--- Commit {{.Commit}} ({{.Agent}}, {{.Verdict}}) --- -{{.Output}} - {{template "review_comments" .Comments}} {{end}}{{end}} {{end}} @@ -54,7 +40,7 @@ responses from developers. Use this context to: {{template "review_comments" .Comments}} {{end}}{{end}} {{end}} -{{define "optional_sections"}}{{template "project_guidelines" .}}{{template "additional_context" .}}{{template "previous_reviews" .}}{{template "in_range_reviews" .}}{{template "previous_review_attempts" .}}{{end}} +{{define "optional_sections"}}{{template "project_guidelines" .}}{{template "additional_context" .}}{{template "previous_reviews" .}}{{template "previous_review_attempts" .}}{{end}} {{define "current_commit_required"}}## Current Commit **Commit:** {{.Commit}} @@ -82,9 +68,9 @@ Reviewing {{if gt .Count 0}}{{.Count}}{{else}}{{len .Entries}}{{end}} commits: {{if .Description}}{{.Description}}{{else}}The following changes have not yet been committed.{{end}} {{end}} -{{define "diff_block"}}{{if .Heading}}{{.Heading}}{{else}}### Diff{{end}} +{{define "diff_block"}}{{if .Diff.Heading}}{{.Diff.Heading}}{{else}}### Diff{{end}} -{{if .Fallback}}{{.Fallback}}{{else}}{{.Body}}{{end}}{{end}} +{{if .Fallback.HasContent}}{{.Fallback.Rendered}}{{else}}{{.Diff.Body}}{{end}}{{end}} {{define "inline_diff"}}```diff {{.Body}}``` {{end}} @@ -146,19 +132,9 @@ View with: {{.ViewCmd}} {{if .Body}}```diff {{.Body}}``` {{end}}{{end}} -{{define "address_tool_attempts"}}{{if .ToolAttempts}}## Previous Addressing Attempts - -{{range .ToolAttempts}}--- Attempt by {{.Responder}}{{if .When}} at {{.When}}{{end}} --- -{{.Response}} - -{{end}}{{end}}{{end}} -{{define "address_user_comments"}}{{if .UserComments}}## User Comments - -The following comments were left by the developer on this review. -Take them into account when applying fixes — they may flag false -positives, provide additional context, or request specific approaches. +{{define "address_attempts"}}{{if .PreviousAttempts}}## Previous Addressing Attempts -{{range .UserComments}}**{{.Responder}}**{{if .When}} ({{.When}}){{end}}: +{{range .PreviousAttempts}}--- Attempt by {{.Responder}}{{if .When}} at {{.When}}{{end}} --- {{.Response}} {{end}}{{end}}{{end}} diff --git a/internal/prompt/test_helpers_test.go b/internal/prompt/test_helpers_test.go index 0e97c7e2..b20faae8 100644 --- a/internal/prompt/test_helpers_test.go +++ b/internal/prompt/test_helpers_test.go @@ -97,7 +97,7 @@ func setupTestRepo(t *testing.T) (string, []string) { return r.dir, commits } -func setupSmallDiffRepo(t *testing.T) (string, string) { +func setupLargeDiffRepo(t *testing.T) (string, string) { t.Helper() r := newTestRepo(t) @@ -108,17 +108,26 @@ func setupSmallDiffRepo(t *testing.T) (string, string) { r.git("add", "base.txt") r.git("commit", "-m", "initial") + var content strings.Builder + for range 20000 { + content.WriteString("line ") + content.WriteString(strings.Repeat("x", 20)) + content.WriteString(" ") + content.WriteString(strings.Repeat("y", 20)) + content.WriteString("\n") + } + require.NoError(t, os.WriteFile( - filepath.Join(r.dir, "small.txt"), - []byte("hello world\n"), 0o644, + filepath.Join(r.dir, "large.txt"), + []byte(content.String()), 0o644, )) - r.git("add", "small.txt") - r.git("commit", "-m", "small change") + r.git("add", "large.txt") + r.git("commit", "-m", "large change") return r.dir, r.git("rev-parse", "HEAD") } -func setupLargeDiffRepo(t *testing.T) (string, string) { +func setupLargeExcludePatternRepo(t *testing.T) (string, string) { t.Helper() r := newTestRepo(t) @@ -142,7 +151,11 @@ func setupLargeDiffRepo(t *testing.T) (string, string) { filepath.Join(r.dir, "large.txt"), []byte(content.String()), 0o644, )) - r.git("add", "large.txt") + require.NoError(t, os.WriteFile( + filepath.Join(r.dir, "custom.dat"), + []byte(content.String()), 0o644, + )) + r.git("add", "large.txt", "custom.dat") r.git("commit", "-m", "large change") return r.dir, r.git("rev-parse", "HEAD")