Refine the span deletion contract and implementation plan
This commit is contained in:
@@ -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).
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user