348 lines
20 KiB
Markdown
348 lines
20 KiB
Markdown
# Correction Repair Review Remediation
|
||
|
||
**Updated:** 2026-11-11
|
||
|
||
**Status:** 3 of nine findings resolved. Findings refer to implementation revision
|
||
`aa8073c`. See the [implementation plan](implementation.md).
|
||
|
||
## Review Result
|
||
|
||
The intended processing architecture is present: module-owned repair prompts,
|
||
validator-owned findings, full original exchanges with independent proposal
|
||
branches, ConfigKit-resolved budgets and profiles, and PromptKit-owned rendering,
|
||
execution, retries, structural repair, and backend admission. Revisions undergo
|
||
fresh validation and enter deterministic application under the existing conflict
|
||
checks and stage barrier. Ordinary reports summarize branches; debug artifacts
|
||
retain detailed evidence.
|
||
|
||
The review found gaps in outcome attribution, retained diagnostic evidence,
|
||
replay of configured cache settings, and protected-alias feedback. The
|
||
algorithmic audit found repeated section scans, quadratic proposal bookkeeping,
|
||
and debug-only history retained during ordinary runs. These are local
|
||
implementation costs. The plan deliberately calls for full conversation
|
||
replay and fresh validation of each revision.
|
||
|
||
No additional strengthening of invariants, contracts, prohibitions, or guarantees
|
||
was identified in the reviewed implementation. In particular, the new semantic
|
||
budget has no additional maximum, zero disables extra feedback and repair,
|
||
revisions may change their target within the original section, empty replacements
|
||
and whitespace spans remain supported, and provider request limits are not
|
||
replaced by a new local context-size restriction.
|
||
|
||
## Findings
|
||
|
||
### R1 — Preserve Defect Attribution for Revised Proposals (P2)
|
||
|
||
**Completed.** Revision defects now appear on the originating repair recovery. Subprocess tests cover exhaustion, later approval, and ordinary rejection.
|
||
|
||
**Affected plan:** Stage 6, item 5; Stage 7, items 1–2.
|
||
|
||
**Evidence:** In [repairCandidates](../../internal/framework/runner/repair.go),
|
||
lines 178–191 retain revalidation decisions, rejections, and operations, but drop
|
||
`validated.filtered`. `RepairAttempt` has no corresponding defect data, and
|
||
`repairOperations` (lines 230–249) consequently emits no defect attribution for
|
||
that revision. `mergeFilteredProposals` in
|
||
[runner.go](../../internal/framework/runner/runner.go) attributes original
|
||
proposal defects only; it cannot recover the discarded revision evidence.
|
||
|
||
**Reproduced behavior:** Begin with one segment, `cat`, and an initially
|
||
low-confidence proposal to replace it with `Cat`. Supply a structurally valid
|
||
repair with `proposal_ref: 1`, `action: revise`, and correction
|
||
`{id: 99, original_text: cat, corrected_text: Cat, confidence: 0.99}`.
|
||
Revalidation correctly rejects the out-of-section target with
|
||
`invalid_target_segment_id`, and the branch exhausts its one-attempt budget.
|
||
However, its sole `correction_repair` operation is recorded as successful with
|
||
no failure reason, filtered defect, or recovery reason. The defect is visible
|
||
only in retained validation/debug evidence; ordinary operation reporting cannot
|
||
attribute it to the repair response. This also loses the defect's operation
|
||
attribution when a later attempt succeeds.
|
||
|
||
**Required change:** Carry the defective revision's evidence into its originating
|
||
repair attempt and operation, following the plan's distinction between original
|
||
proposal filtering and defects introduced by a repair response. Preserve the
|
||
existing treatment of ordinary editorial rejection, low confidence, withdrawal,
|
||
and application conflicts; those are not automatically operational errors.
|
||
Keep original proposal-response counts separate and avoid attributing the later
|
||
defect to the original response or counting it twice.
|
||
|
||
**Acceptance:** Add a regression covering a schema-valid revision with an invalid
|
||
target or missing source span, both at budget exhaustion and before a later
|
||
successful repair. Verify that the specific defect remains attributable to its
|
||
repair operation in ordinary reporting, while proposal-response totals, semantic
|
||
attempt counts, and the final branch disposition remain correct. Include an
|
||
ordinary rejected revision to protect the existing distinction from response
|
||
defects.
|
||
|
||
### R2 — Retain Application Skip Reasons in Repaired Ledger Entries (P2)
|
||
|
||
**Completed.** The debug ledger joins the application skip reason and message by stage and proposal. Ledger and overlap tests pass.
|
||
|
||
**Affected plan:** Stage 6, item 5; Stage 8, item 3.
|
||
|
||
**Evidence:** [BuildCorrectionLedger](../../internal/framework/processreport/correction_ledger.go)
|
||
skips every repaired proposal in the `module.SkippedChanges` loop (lines 131–134).
|
||
Its replacement repair-row construction (lines 194–209) copies the branch
|
||
disposition and attempt history, but never joins the actual skipped application
|
||
result. `DispositionReasonCode` and `DispositionMessage` are therefore empty for
|
||
`approved_skipped` repair rows, although they are retained for unrepaired skipped
|
||
proposals.
|
||
|
||
**Reproduced behavior:** An earlier accepted proposal changes `cat` to `dog`.
|
||
A later repair validates a revision of that same original span to `fox`, then
|
||
correctly skips at application because the earlier edit removed its source span.
|
||
The runner retains the actual `SkippedChange`, but the repaired proposal's debug
|
||
ledger row retains only `approved_skipped` and its pre-application decisions.
|
||
A focused ledger probe confirms that a supplied skip reason and message disappear
|
||
from the serialized entry. Repair interaction artifacts are written before
|
||
application, so they cannot supply the missing application result.
|
||
|
||
**Required change:** Join the final skipped change to the repaired ledger entry
|
||
using the existing stage-instance/proposal identity, and populate the existing
|
||
disposition reason and message fields. Preserve one final row per repaired
|
||
proposal, its original proposal data, and its revision history. The compact
|
||
ordinary report can retain its existing fields.
|
||
|
||
**Acceptance:** Extend the existing overlapping-edit runner/ledger coverage so a
|
||
validated but skipped revision retains the actual application reason and message
|
||
in its single final debug row. Verify that it remains `approved_skipped`, has no
|
||
applied-edit fields, and does not create a duplicate ledger row or an additional
|
||
operational error.
|
||
|
||
### R3 — Preserve Cache Settings When Replaying Repair Turns (P3)
|
||
|
||
**Completed.** Replay retains each rendered instruction cache setting. The two-turn adapter test verifies the outbound role, text, and cache metadata.
|
||
|
||
**Affected plan:** Stage 3, items 3–5.
|
||
|
||
**Evidence:** [The adapter](../../internal/framework/llm/adapter.go#L117) appends
|
||
the current repair instruction as native PromptKit messages. It retains that
|
||
instruction for the next attempt through `convertMessages`, which copies only
|
||
role and content. On the next attempt the adapter reconstructs those prior
|
||
turns without their `CacheControl`. Operator prompt replacements already
|
||
support PromptKit's per-message `cache_control` field.
|
||
|
||
**Reproduced behavior:** Override the grammar repair prompt with a user message
|
||
using `cache_control: {type: ephemeral, ttl: 1h}`, then execute two repair
|
||
turns on the same branch. The first call sends the instruction as a cached text
|
||
block. The second call sends the same earlier instruction as a plain string,
|
||
while the new instruction still carries the cache settings. The transport
|
||
confirmed this difference with a local fixture.
|
||
|
||
**Required change:** Preserve native cache metadata for rendered instructions
|
||
carried into later turns. Keep its conversion in the adapter boundary and leave
|
||
wire encoding to PromptKit.
|
||
|
||
**Acceptance:** Extend the two-turn adapter test with a cache-controlled
|
||
operator repair prompt. Verify that the previous instruction retains its
|
||
cache settings, text, and role when replayed. The test should check the
|
||
outbound request without depending on provider caching behavior.
|
||
|
||
### R4 — Reuse Replacement Previews Across Feedback Checks (P3)
|
||
|
||
**Affected plan:** Stage 5, items 1–2.
|
||
|
||
**Evidence:** [feedbackCandidates](../../internal/framework/runner/feedback.go#L30)
|
||
calls `feedbackPreview` for every candidate at every validator, including
|
||
shape and confidence checks that never use it. Each call scans the section for
|
||
the target, then builds the replacement preview. The
|
||
[runner](../../internal/framework/runner/runner.go#L744) previews the candidate
|
||
again for ambiguity, and [validator request construction](../../internal/framework/validators/llm_validators.go#L269)
|
||
rebuilds the preview for the LLM payload. The default positive repair budget
|
||
adds this work even when all proposals pass the initial chain.
|
||
|
||
**Complexity:** For a section with `S` segments and `M` candidate versions
|
||
(initial proposals and revisions), the feedback selector alone can perform
|
||
`O(V × M × S)` target lookup work across `V` validators. Each singleton
|
||
revalidation also rebuilds section-wide indexes inside presence, nonempty-result,
|
||
and semantic validators. With a shared section index and one preview per
|
||
version that needs it, lookup and candidate traversal take expected
|
||
`O(S + V × M)` time, plus the actual text and payload work. That removes the
|
||
quadratic lookup term when proposals grow with section size.
|
||
|
||
**Measured behavior:** An offline benchmark with 100 segments and 100 valid
|
||
proposals exercised the same five deterministic validators at budgets zero
|
||
and one, with identical approvals and no repairs. On the review machine,
|
||
validation rose from about 0.28 to 0.50 ms and allocated bytes from about 0.40 to
|
||
0.68 MB per section. With 1,000 segments and proposals, the same probe rose from
|
||
about 4.2 to 8.0 ms and from 5.4 to 7.8 MB. These measurements isolate local
|
||
validation work.
|
||
|
||
**Required change:** Reuse the existing replacement preview across applicability,
|
||
ambiguity, and validation payload construction for a given candidate version
|
||
where the rules use the same before/after text. Reuse a section target index and
|
||
compute previews only for checks that need them. Recompute the preview for a
|
||
revision, and preserve the separate application check against the current text.
|
||
Pass the same read-only section lookup into singleton revalidation so validators
|
||
can use the original target and its neighbors without rebuilding their indexes.
|
||
|
||
**Acceptance:** Keep the existing independent-feedback and zero-budget coverage
|
||
passing, including ambiguous spans, blank results, invalid confidence, and
|
||
protection exceptions. Re-run a focused offline benchmark to verify reduced
|
||
scanning and allocation in the feedback path.
|
||
|
||
### R5 — Index Initial Findings and Compact Filtering Once (P3)
|
||
|
||
**Affected plan:** Stage 6, items 1 and 5.
|
||
|
||
**Evidence:** [repairCandidates](../../internal/framework/runner/repair.go#L59)
|
||
calls `actionable` with the full initial rejection list for every proposal.
|
||
Each eligible proposal then calls `repairFindings` over the same full list.
|
||
Every successful revision also walks and compacts the original filtered-defect
|
||
slice. [Findings extraction](../../internal/framework/runner/feedback.go#L72)
|
||
already appends in validator order.
|
||
|
||
**Complexity:** With `P` original proposals and `D` initial rejection
|
||
records, branch selection and initial feedback can take `Θ(P × D)` time.
|
||
If a validation outage leaves one unavailable decision per proposal, this
|
||
already takes quadratic time before the runner determines no repairs can start.
|
||
With `Q` approved repairs and `F` original filtering records, repeated
|
||
compaction can add `Θ(Q × F)` work in the worst case. Both products can be
|
||
quadratic in proposal count even with a fixed validator chain.
|
||
|
||
**Refactoring:** Group initial rejections by proposal index in a single pass.
|
||
Walk the proposals in their existing order and read only each proposal's
|
||
findings. Record the approved replacements in a set and filter the original
|
||
defect list once when returning the section result, including partial results
|
||
on error. The combined bookkeeping then takes expected `O(P + D + F)` time
|
||
apart from the individual repair calls and revalidations.
|
||
|
||
**Acceptance:** Preserve validator order within findings, proposal order
|
||
across branches, no-effect and unavailable-only handling, and original
|
||
filtering evidence for candidates that remain excluded.
|
||
|
||
### R6 — Index Application Results for Repair Outcomes (P3)
|
||
|
||
**Affected plan:** Stage 6, item 5, and Stage 7, item 3.
|
||
|
||
**Evidence:** After applying the stage, [Runner.Run](../../internal/framework/runner/runner.go#L244)
|
||
scans the stage's entire applied-change slice for each approved repair. A
|
||
repair that was skipped scans the whole slice.
|
||
|
||
**Complexity:** With `B` repair branches, `Q` approved repairs, and `A`
|
||
applied changes in the stage, outcome classification costs
|
||
`O(B + Q × A)` beyond walking the work units. It can be quadratic even when
|
||
every approved repair is applied and the matches occur at successive positions.
|
||
|
||
**Refactoring:** Index the application results once by the existing stage-local
|
||
proposal index, then look up each approved repair. This takes expected
|
||
`O(A + B)` time and `O(A)` auxiliary space for an applied-index set. An
|
||
index carrying skip details could also support the evidence join in R2.
|
||
|
||
**Acceptance:** Keep disposition based on actual application. Cover a stage
|
||
with an ordinary approval, an applied revision, and an approved revision
|
||
skipped by the existing conflict checks.
|
||
|
||
### R7 — Keep Detailed Repair History in Debug Runs (P3)
|
||
|
||
**Affected plan:** Stage 7, item 3, and Stage 8, items 2–3.
|
||
|
||
**Evidence:** [repairCandidates](../../internal/framework/runner/repair.go#L178)
|
||
stores the revision, decisions, rejections, and operation slice on every
|
||
attempt, even when debug capture is disabled. The run retains those attempts in
|
||
its [work units](../../internal/framework/runner/runner.go#L597). The
|
||
[ordinary report](../../internal/framework/processreport/report.go#L126)
|
||
needs a branch's proposal index, attempt count, and disposition. It counts
|
||
executions from the work unit's separate operation list. The debug ledger reads
|
||
the revision, decisions, and artifact references. Rejections are read during
|
||
the interaction write, and the saved attempt-level operation slice has no
|
||
consumer.
|
||
|
||
**Complexity:** For `K` attempts, the runner retains `O(K + H)` additional
|
||
history across the run, where `H` is the size of the retained revision text and
|
||
validator records. With debug off, that history can be reduced to
|
||
`O(B)` branch summaries for `B` branches after ordinary operation facts
|
||
have been emitted. Those operation facts still scale with the number of calls.
|
||
The conversation and findings needed for the next attempt remain branch-local
|
||
working state.
|
||
|
||
**Refactoring:** Keep compact outcome fields separate from optional ledger
|
||
history. Retain detailed records for the ledger when debug capture is active.
|
||
Use rejections locally for feedback and artifact writes and keep one copy of
|
||
operation facts for ordinary accounting. Pass the final revision through the
|
||
existing approved-candidate path for application.
|
||
|
||
**Acceptance:** Preserve counts and compact outcomes with debug off, including
|
||
when ordinary reporting is disabled. Debug runs retain per-attempt decisions
|
||
and available artifact paths on success and interruption.
|
||
|
||
### R8 — Select a Protected-Spelling Finding in One Pass (P3)
|
||
|
||
**Affected plan:** Stage 5, item 4.
|
||
|
||
**Evidence:** The [protection checks](../../internal/framework/validators/protected_terms.go#L98)
|
||
collect every violating spelling into a slice, sort it, and read only the first
|
||
value. This code runs during validation and through the application-time
|
||
protection guard.
|
||
|
||
**Complexity:** For `K` reported violations, selection uses up to
|
||
`O(K log K)` string comparisons and `O(K)` extra storage. A running
|
||
lexical minimum needs `O(K)` comparisons and constant auxiliary space.
|
||
The matching and identity checks still do their existing work.
|
||
|
||
**Refactoring:** Select the same deterministic representative as violations
|
||
are detected, using the evidence corrected in R9. Preserve the precedence
|
||
of identity loss over capitalization and the existing rejection rules.
|
||
|
||
### R9 — Name the Affected Protected Alias (P3)
|
||
|
||
**Affected plan:** Stage 5, item 4.
|
||
|
||
**Evidence:** When an identity loses a mention, the
|
||
[identity-preservation check](../../internal/framework/validators/protected_terms.go#L98)
|
||
uses the first prior occurrence's spelling for its finding. An identity can
|
||
have multiple protected aliases. The first spelling can still be present
|
||
after the edit.
|
||
|
||
**Reproduced behavior:** Protect both `Mira` and `Captain Mira` under one
|
||
identity. In a glossary stage, replace `Captain Mira` with `the captain` in
|
||
`Mira spoke to Captain Mira`. The validator correctly rejects the edit but
|
||
says it may alter `Mira`, the spelling that stayed. The same misleading
|
||
message becomes repair feedback.
|
||
|
||
**Refactoring:** Derive a specific spelling from before/after occurrence
|
||
counts for the identity that lost a mention, or describe the identity loss
|
||
when a spelling cannot be identified reliably. Counting spelling deficits
|
||
can be done in expected linear time in matched occurrences. Preserve the
|
||
existing identity-level protection decision and spoken-word repetition
|
||
exception.
|
||
|
||
**Acceptance:** Exercise two aliases of one identity. The finding identifies
|
||
the removed alias or the affected identity without attributing the edit to
|
||
the unchanged spelling.
|
||
|
||
## Stage Review Coverage
|
||
|
||
| Stage | Evidence reviewed | Result |
|
||
| --- | --- | --- |
|
||
| 1 — Response and native composition | Repair schema, typed decode, correspondence, native multi-turn test | Response processing scales with response size. |
|
||
| 2 — Module prompts | Four registered definitions, shared contract, rendering and operator replacements | Per-request feedback serialization scales with supplied content. |
|
||
| 3 — Exchange and adapter | Shared section exchange, prepared prefix, branch history, native failure handling | R3. Cost grows with the sum of the full requests sent, as specified in the plan. |
|
||
| 4 — Configuration and readiness | Root/stage resolution, zero and positive budgets, profile readiness, role creation | A fixed number of role selections per stage. Readiness deduplicates profile inspections. |
|
||
| 5 — Findings | Applicability, previews, cardinality, protection, semantic input and batching | R4, R8, R9. Decision cardinality uses indexed checks. |
|
||
| 6 — Branch execution and application | Eligibility, versioned revalidation, defect attribution, application outcomes and cancellation | R1, R4–R6. Application evidence feeds R2. |
|
||
| 7 — Ordinary outcomes | Operation aggregation and sorting, report-disabled summaries, compact branch rows | R1. New presentation work scans and sorts the recorded facts. |
|
||
| 8 — Debug and ledger | Attempt retention, optional artifacts, redaction, indexed lineage and interrupted joins | R2, R7. The ledger joins decisions and branches by identity. |
|
||
| 9 — Assembled command and docs | Controlled-provider subprocess flows, cancellation, output selection, documentation and preview | Existing tests cover the principal execution paths. The recorded findings identify the gaps. |
|
||
|
||
## Validation Performed
|
||
|
||
- The fresh `go test -count=1 ./...` run passed.
|
||
- Fresh race runs passed for the runner, LLM adapter, process reporting, and CLI.
|
||
- The CLI build passed.
|
||
- The repair-preview command in [the example guide](../../examples/README.md)
|
||
successfully rendered the grammar repair instruction and schema without a
|
||
provider call. Maintained-example integration tests also passed in the full
|
||
suite.
|
||
- The implementation documentation passed its link review. The follow-up
|
||
checked the links in this roadmap and ran `git diff --check`.
|
||
- Temporary Go overlays reproduced the defects in R1–R3 and measured the
|
||
validation overhead in R4 against the committed implementation.
|
||
- The algorithmic audit derived scaling from the current source and its
|
||
callers. A temporary Go overlay reproduced the wrong-alias finding in R9.
|
||
- Fresh tests passed for runner, validators, process reporting, LLM adapter,
|
||
preflight, response schemas, proposal generation, configuration, and prompts
|
||
during the algorithmic audit.
|
||
|
||
Resolve the findings with focused checks at their owning boundaries. Retain the
|
||
current architecture and accepted operator choices.
|