From 31979d4081938c8e881fb3a7a1285cd8b8d09395 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 25 Sep 2026 02:44:38 +0000 Subject: [PATCH] Refine the span deletion contract and implementation plan --- docs/roadmap/implementation.md | 129 ++++++++++++++++++++++----------- docs/roadmap/span-deletion.md | 60 +++++++++++---- 2 files changed, 130 insertions(+), 59 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index cc7d4f9..251470d 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -23,12 +23,18 @@ messages, following the agent policy. Do not add permission rounds between stage Aim directly at the final state. Do not add temporary compatibility aliases, feature flags, parallel implementations, or scaffolding for intermediate commits. -Keep validators in their current order and preserve module scope, literal matching, -replacement policies, fixed chunks, required validation, failure isolation, +Preserve module scope, literal matching, replacement policies, fixed chunks, +required validation, failure isolation, reporting/debug independence, and existing recovery budgets. Do not implement any of the [deferred features](future.md), automatic whitespace cleanup, a new deletion operation type, segment tombstones, or a second normalization pass. +Preserve intended safeguards and outcomes, not incidental implementation choices. +Validator ordering, internal structure, and payload construction may change where +needed to satisfy the feature contract. Current code and tests are evidence to +review, not authority to retain a conflicting restriction. Prefer reusing correct +mechanisms, but do not freeze them merely because they exist. + This replaces the completed resilience implementation plan, which remains in Git history at `a83a518:docs/roadmap/implementation.md`. Historical resilience decisions and their remediation are not instructions to rebuild or reverse that work. @@ -41,11 +47,18 @@ implementation sprint. ### Schema and library boundary -Remove only `minLength: 1` from the `corrected_text` property of +Remove `minLength: 1` from the `corrected_text` property of `internal/framework/responseschema/assets/correction_set.v1.json`. Keep the required field, string type, and all unrelated schema constraints. Empty and whitespace-only strings are structurally valid; missing, null, and non-string replacements are not. -Nonblank source-span and resulting-segment rules stay in domain validation. +Keep `original_text` required, string-valued, and at least one character long. +Whitespace-only original spans are valid at the structural boundary and must not +be rejected solely on that basis by domain checks. Replace trimmed-emptiness checks +for original spans with literal empty-string checks in `CorrectionProposal.Validate` +and `ProposalShapeValidator.Validate`, and review other consumers for the same +restriction. Do not trim either span in transit. Resulting-segment checks still use +whitespace-aware emptiness. Preserve module matching policies: `require_unique` +for grammar, spoken-word, and homophones; `replace_all` for glossary. Keep `CorrectionSetKey`, the schema asset path, ID/name/version, and existing prompt IDs/`v1` selections. This widens an internal model-response constraint without @@ -57,7 +70,7 @@ transcript encoders, configuration, and CLI selections do not change. PromptKit continues to validate the whole response and own structural repairs. Do not introduce partial JSON salvage or Audita-side regeneration. Once the schema accepts empty replacements, normal per-candidate domain filtering handles proposals -that would blank a segment. +that would blank their stage-input segment. ### Validator identity @@ -70,39 +83,43 @@ Rename the existing validator consistently: | `NonEmptyCorrectionValidator` framework type | `NonEmptyResultingSegmentValidator` | Retain the `empty_resulting_segment` rejection/skip reason, deterministic execution -class, chain position, and preview-based behavior. Do not retain an old-key alias. +class, and the resulting-segment safeguard. Reuse preview-based validation where +correct; neither its exact implementation nor chain position is frozen. Do not +retain an old-key alias. This is an intentional validator identity change in diagnostic/decision records; document it, including any affected ordinary filtered-proposal metadata. It is not a new operator-selectable setting. Historical release notes remain untouched. ### Prompt and payload semantics -All four proposal prompts must explain that an empty replacement deletes the -identified span, within the module's existing task, and that a correction leaving -a blank segment should be omitted. Prompts must not ask the model to invent padding -or replacement content to avoid an empty result. +Review all four proposal definitions and the active validator definitions +(`editorial_review`, `meaning_reversal_review`, `spoken_form_plausibility`) for +consistency. Edit only definitions needing clarification. Review the registered +but inactive `grammar_review` and `spoken_word_review` definitions too; unchanged, +consistent definitions need no edits. Do not activate additional validators as +part of this work. -Use a spoken-word example deleting `"uh "` from `"uh hello"` with `""`, producing -`"hello"`. A grammar example may delete the uniquely occurring extra comma from -`"Hello, world."` when appropriate; the representation is illustrative, not a -claim that the edit is always editorially justified. Explain that selection of -surrounding whitespace/punctuation must produce the intended literal result. -Keep the existing uniqueness requirement and module responsibilities. +Where helpful, explain that an empty replacement deletes the identified span, +whitespace-only spans can express spacing corrections, and the resulting segment +must remain nonblank. Do not instruct the model to fabricate padding or content. +A spoken-word example could delete `"uh "` from `"uh hello"`; a grammar example +could reduce two spaces to one. Examples and wording are illustrative, not frozen +requirements or promises of automatic approval. Explain surrounding whitespace +and punctuation only to the extent needed to produce the intended literal result. +Qualify uniqueness by module: glossary uses `replace_all`, while the other three +modules use `require_unique`. -Update the three active validator prompt definitions (`editorial_review`, -`meaning_reversal_review`, `spoken_form_plausibility`) to interpret an empty -replacement in the context of the full before/after segment. Empty syntax alone -must not decide approval. Meaning-reversal review remains a narrow check for -antonyms/reversals; do not turn it into a general semantic-equivalence validator. -Spoken-form plausibility retains its existing phonetic/mistranscription remit. -The registered `grammar_review` and `spoken_word_review` definitions should receive -the same deletion interpretation as their editorial-review counterpart, without -adding them to active chains or deleting them as unrelated cleanup. +Keep module responsibilities intact. Meaning-reversal review remains a narrow +check for antonyms/reversals, not a new general semantic-equivalence validator. +Spoken-form plausibility retains its phonetic/mistranscription remit. Empty syntax +alone must not determine approval or rejection. -`BuildLLMValidationRequest` already supplies `corrected_text`, -`original_segment_text`, and `corrected_segment_text`; the JSON fields are not -omitted when empty. Reuse this representation. Verify it behaviorally rather than -adding a new payload shape, synthetic sentinel, or special deletion flag. +`BuildLLMValidationRequest` already supplies explicit `corrected_text` and full +before/after segment text. Start with that representation and verify that empty +and whitespace-only spans survive it faithfully. Adjust payload construction if +needed for consistency; do not treat its current structure as an independent +requirement. No new deletion flag or synthetic sentinel is needed merely to +represent an empty string. ### Debug representation and outcomes @@ -121,11 +138,16 @@ or a generic optional-value framework. These values remain replacement spans, not full resulting-segment text. Preserve redaction and the rest of the ledger. Keep existing operation accounting. A valid approved deletion is an ordinary -applied correction. A candidate that would blank the segment is filtered with the -existing reason and contributes to the proposal operation's consolidated filtering -recovery. Ordinary editorial/protected-term rejection does not become a recovery. -Existing apply-time conflict handling and attribution remain unchanged. Structural -repairs required for unrelated defects still count normally. +applied correction. A candidate that would blank its stage-input segment is +filtered with the existing reason and contributes to the proposal operation's +consolidated filtering recovery. Ordinary editorial/protected-term rejection does +not become a recovery. Multiple proposals can target the same segment; they are +validated against stage input and applied sequentially. A proposal applicable to +that input but made unsafe by an earlier accepted edit is skipped as a conflict, +with no added model error or filtering recovery. Keep the skipped evidence. The +runner's `attributeApplyFiltering` already makes this distinction: verify and reuse +it rather than invent new classification machinery. Structural repairs required +for unrelated defects still count normally. ## Stage 1 — Align Validator Identity and Confirm Domain Semantics @@ -137,14 +159,22 @@ repairs required for unrelated defects still count normally. string searches for serialized keys. Do not globally rewrite historical docs. - Preserve existing `CorrectionProposal.Validate`, `PreviewProposalForSegment`, `ApplyProposals`, and deterministic validation behavior wherever already correct. - Verify that there is no additional nonempty-replacement restriction downstream. + Change original-span trimmed-emptiness checks as specified above, and find any + equivalent downstream restrictions. Keep empty-original rejection and + whitespace-aware resulting-segment rejection distinct. - Extend the existing proposal and validator tests only for missing meaningful coverage: empty replacement leaving a nonblank segment, whitespace-only resulting - text rejection, literal spacing, and apply-time reevaluation after an earlier edit. - For the last case, use segment `"a b"` and ordered deletions `"a" -> ""`, + text rejection, whitespace-only originals, literal spacing, and apply-time + reevaluation after an earlier edit. Cover `"hello world"` with `" " -> " "`, + an empty original, and two separate matching whitespace runs under `require_unique`. + Assert module-policy ambiguity handling without imposing uniqueness on glossary. + For apply-time reevaluation, use segment `"a b"` and ordered deletions `"a" -> ""`, `"b" -> ""`: each is previewable initially, but after the first edit the second must be skipped as `empty_resulting_segment`. The surviving text is `" b"`; - no automatic trim or segment deletion is expected. + no automatic trim or segment deletion is expected. At the runner/outcome boundary, + verify this initially applicable second proposal produces skipped debug evidence + but no model error or filtering recovery. Reuse/extend existing conflict tests; + a second full CLI scenario is unnecessary for this attribution check. - Ensure protected-term validation still rejects removal of a protected term with an empty replacement. Prefer one case in its existing test table. Retain existing metadata-preservation tests rather than duplicating all metadata assertions. @@ -154,7 +184,9 @@ repairs required for unrelated defects still count normally. **Validation** Run `go test ./internal/framework/proposals ./internal/framework/validators ./internal/validators/... ./internal/framework/runner ./internal/framework/processreport`. -Existing registry/chain tests must use the new identity and preserve order/class. +Existing registry/chain tests must use the new identity and verify the required +validation and execution class. Update incidental order assertions if a justified +ordering change preserves those safeguards. Inspect remaining old-name references, retaining only deliberately historical ones. **End state:** The accurately named domain validator and application preserve the @@ -164,8 +196,9 @@ agreed behavior. No schema or retry workaround has been introduced. **Work** -- Apply the selected schema relaxation and prompt changes. Keep unrelated prompt - instructions, hardening, variables, and response associations intact. Review the +- Apply the selected schema relaxation and selectively clarify prompts. Preserve + intended task scope, hardening, and response contracts while correcting conflicting + instructions wherever encountered within this feature. Review the rendered definitions, not just YAML source, using existing PromptKit inspection and prompt tests. - In `internal/framework/responseschema/registry_test.go`, retain the check that @@ -178,13 +211,16 @@ agreed behavior. No schema or retry workaround has been introduced. HTTP fixture. Give accepted cases a positive repair allowance and assert one provider generation, zero repairs, and preservation of the actual string. For invalid cases, use zero repair attempts to prove rejection without redundant - model calls. Existing tests already own successful repair/exhaustion behavior. + model calls. Include a whitespace-only original in an accepted case and confirm + an empty original is structurally invalid. Existing tests already own successful + repair/exhaustion behavior. - Extend the validation-request tests in `internal/framework/validators/llm_validators_test.go` to marshal a real request for deleting `"uh "` from `"uh hello"`. Assert that JSON contains the explicit empty replacement plus full before/after text, correct identity, and no immediate - rejection. A field-presence assertion is required; decoding into a plain string - alone cannot distinguish omitted from empty. + rejection. Include a whitespace-only-original spacing correction in the request + coverage and verify literal preservation. A field-presence assertion is required; + decoding into a plain string alone cannot distinguish omitted from empty. - Update canonical prompt/module guidance for the implemented semantics. Do not add exact-prose prompt snapshots or assertions for every new sentence; existing parsing/rendering/schema-association tests plus editorial review are sufficient. @@ -244,6 +280,11 @@ Cover these distinct outcomes: validators approve both. Assert corrected transcript, two applied corrections, zero model errors/recoveries, zero structural repairs with a positive repair allowance, and explicit empty proposed/applied fields in the debug ledger. +- **Whitespace-only span:** A grammar stage reduces one uniquely occurring run + of two spaces to one. Controlled validators approve it; assert the expected + text and no error/recovery solely due to whitespace. This proves schema, domain + validation, and application agree through the actual CLI. Use a compact fixture + without adding a matrix of whitespace characters or lengths. - **Partial success with filtering:** Add a proposal to delete all text from a separate filler-only segment, preserving that separate segment through fixture normalization (use sufficient time separation or explicit normalization settings). diff --git a/docs/roadmap/span-deletion.md b/docs/roadmap/span-deletion.md index e614d8b..117281b 100644 --- a/docs/roadmap/span-deletion.md +++ b/docs/roadmap/span-deletion.md @@ -39,6 +39,9 @@ The source reviewed at `0b6cd98` contains a mismatch: allows empty replacement text. The [preview/application logic](../../internal/framework/proposals/preview.go) supports literal span deletion when the resulting segment remains nonblank. +- Domain shape checks currently reject whitespace-only original spans, although + the response schema permits them and grammar prompts permit spacing cleanup. + This is another restriction to align with the target contract below. - The validator named `non_empty_corrected_text` actually checks the resulting segment, not the replacement string. Both this check and application prevent a segment from becoming empty or whitespace-only. Application changes segment @@ -53,8 +56,11 @@ of any particular historical incident without its payload and running version. ## Target Correction Contract -- `original_text` identifies an existing, nonblank literal span in the current - target segment. Preserve the existing matching and module replacement policies. +- `original_text` identifies an existing, nonempty literal span in the current + target segment. Whitespace-only spans are eligible; an empty string identifies + no source span and remains invalid. Matching and ambiguity checks still apply + under the module's replacement policy: `require_unique` for grammar, spoken-word, + and homophones; `replace_all` for glossary. - `corrected_text` remains a required string. An empty string means delete the identified span; it does not mean remove the segment record or skip validation. Missing fields and non-string values remain structurally invalid. @@ -71,8 +77,14 @@ of any particular historical incident without its payload and running version. - Preserve literal replacement behavior. Prompts should select spans and replacements that handle adjacent whitespace and punctuation appropriately; this feature does not add automatic trimming or a second normalization pass. - Whitespace-only replacements are not categorically prohibited: their validity - depends on the resulting text and the ordinary module/domain checks. + Neither side of a correction is rejected solely for containing only whitespace. + For example, reducing a uniquely identified run of two spaces to one space may + be appropriate; validity still depends on the resulting text and domain checks. + +Preserve the intended safeguards and outcomes, not every detail of the current +implementation. Validator ordering, internal structure, and payload construction +may change when needed to achieve this consistent contract. Existing behavior or +a passing test does not justify retaining a conflicting restriction. Illustrative cases: @@ -80,6 +92,7 @@ Illustrative cases: | --- | --- | --- | --- | | `uh hello` | `uh ` | empty string | Structurally valid; may produce `hello` after required validation. | | `uh hello` | `uh hello` | `hello` | Remains a valid representation of the same cleanup, subject to validation. | +| `hello world` | two spaces | one space | Eligible spacing correction, subject to unique matching and required validation. | | `uh` | `uh` | empty string | Structurally valid proposal, then filtered because the resulting segment would be blank. | | `hello world` | `hello world` | spaces only | Filtered because the resulting segment would be blank. | @@ -89,10 +102,12 @@ a successful no-change outcome. ## Prompt and Module Alignment -Proposal instructions should explicitly describe empty-string deletion and its -resulting-segment limit. Include a concise example where it helps the module's -task. Tell the model to omit a correction that would leave a blank segment rather -than fabricate replacement content or padding to satisfy a schema restriction. +Review relevant prompt definitions for consistency and edit only those needing +clarification. Where needed, explain empty-string deletion, whitespace-only spans, +and the resulting-segment limit with a concise, module-appropriate example. The +model should omit a correction that would leave a blank segment rather than invent +padding or replacement content. Consistent guidance is the goal; adding deletion +language to every prompt is not a requirement. Retain each module's existing scope: @@ -111,7 +126,8 @@ remains ordinary processing, not a defect that should be regenerated until appro The resulting-segment validator's name and documented identity should describe that responsibility accurately, rather than imply that the replacement string itself must be nonempty. Preserve the existing blank-result failure reason and -validation order while making that deliberate identity change. +safeguard while making that deliberate identity change; current chain position +and preview implementation are not independent product requirements. ## Outcomes and Reporting @@ -119,12 +135,22 @@ A legitimate deletion is an ordinary correction. Empty replacement alone must produce neither structural repair nor a recovery classification. If another problem required successful repair or filtering, retain that actual recovery. -A schema-valid proposal that would blank a segment is a defective proposal handled -through existing safe filtering. Preserve the `empty_resulting_segment` reason +A schema-valid proposal that would blank its stage-input segment is a defective +proposal handled through existing safe filtering. Preserve the +`empty_resulting_segment` reason and consolidated `proposals_filtered` recovery behavior; other usable proposals in the response continue through required validation. Ordinary editorial rejection remains debug evidence rather than a model error or filtering recovery. +A stage can propose several edits to one segment in the same response. Each edit +is evaluated against the stage input, then approved edits are applied sequentially. +If a proposal was applicable to that input but an earlier accepted edit makes it +unsafe, skip it as an apply-time conflict. Do not count that conflict as a defective +proposal, model error, or filtering recovery. For example, in `"uh um"`, deleting +`"uh "` and deleting `"um"` each leaves text individually; after the first deletion, +the second must be skipped to prevent a blank result. Retain the skipped decision +in debug evidence. The existing runner already distinguishes this situation. + Debug evidence must accurately retain the empty replacement and the corresponding validation/application outcome. Ordinary reports retain their existing counts and purpose; no new report schema, incident category, or deletion-specific counter is @@ -139,10 +165,14 @@ an error for the operation, as today. transcript while preserving segment metadata and fixed chunk membership. - A mixed response can apply an approved deletion and another approved correction; an empty replacement does not invalidate the whole response. -- A proposal that would leave a blank segment is filtered independently, retains - the existing reason, and does not prevent unrelated valid candidates from - succeeding. Application still prevents a blank result if earlier edits change - what a later proposal would do. +- A proposal that would blank its stage-input segment is filtered independently, + retains the existing reason, and does not prevent unrelated valid candidates + from succeeding. A proposal made unsafe only by an earlier accepted edit is + skipped without adding an error or filtering recovery; a blank result is still + prevented and the conflict remains visible in debug evidence. +- A uniquely matching whitespace-only original span can reach validation and + application. Empty originals remain invalid, ambiguous matches remain subject + to module policy, and whitespace-only resulting segments remain disallowed. - Protected-term and meaning-changing deletions remain subject to their normal rejection rules. No candidate bypasses required validation. - A deletion needing no recovery counts as an ordinary applied correction; actual