Skip to content

patch: decide its future, and fix the abort on a mid-codepoint byte offset #375

Description

@tobert

Two things landed in the same place while stating the KernelBackend::patch
batch contract in #373. The first is a concrete bug worth fixing on its own.
The second is the larger question of what patch is for, which wants
deliberate time rather than an incremental fix.

The bug: a byte offset inside a multi-byte character aborts the kernel

PatchOp::{Insert,Delete,Replace} validate only that the offset is within
content.len(). They never check that it lands on a UTF-8 character boundary.
String::insert_str, &content[a..b], drain, and replace_range all abort
on a mid-codepoint index.

Confirmed by probe, not read off the source — PatchOp::Insert { offset: 1 }
into a file beginning with é (two bytes):

thread '...' panicked at crates/kaish-kernel/src/backend/local.rs:66:25:
assertion failed: self.is_char_boundary(idx)

An embedder that computes its own byte offset takes down the kernel instead of
receiving BackendError::InvalidOperation. The file is left untouched, so the
batch contract stated in #373 still holds — but "if an operation fails, the
error returns" does not, which is why that doc was worded to promise only that
a failing operation stops the batch before the write.

The fix is a character-boundary check in the three byte-offset arms, returning
InvalidOperation with the offset and the character it split.

A smaller sibling from the same review, unverified: line: 0 is mapped to line
1 by saturating_sub(1) rather than rejected, even though the PatchOp docs
say line numbers are 1-indexed.

The larger question: how faithful should patch be?

The concern is that a partly-faithful patch is worse than none — an agent
carries expectations from patch(1), and every place kaish quietly differs is
a place those expectations produce a wrong result rather than an error.

Known divergences today:

  • No -F / --fuzz. GNU patch takes it; kaish does not. DEFAULT_FUZZ is
    hardcoded to 2 at patch.rs:423, matching GNU's default, but there is no way
    to ask for anything else — including 0.
  • max_fuzz = 0 does not mean "here or nowhere". It still searches
    file-wide for a position. There is no way to express "apply at line 42,
    refuse otherwise", which is exactly what an anchor-based edit needs. This is
    the capability gap that reopened the edit design (see the 0.15.1 slate).

Deciding this shapes the edit work: if patch becomes faithful enough to
carry anchored edits, edit needs less; if it stays a convenience, edit
carries the exact-position contract instead.

Deferred deliberately — filing so it does not get rediscovered a third time.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions