avoid panics, align to srf

build.zig.zon

@@ -5,8 +5,8 @@
     .minimum_zig_version = "0.16.0",
     .dependencies = .{
         .srf = .{
-            .url = "git+https://git.lerch.org/lobo/srf#08f06df810684f8d32ad0ea5a8f2b3ee61733d2c",
-            .hash = "srf-0.0.0-qZj571YvAgCNHXtzQCOdoAKNzjhLp5o_Ap-GMFmO5-CW",
+            .url = "git+https://git.lerch.org/lobo/srf.git#7f3a43e838fc472bdb1af6c1b692f2255dd19f48",
+            .hash = "srf-0.0.0-qZj57-17AgAXn2at0WH1xV75oJhr3vTFeU5FxU7kAhtf",
         },
     },
     .paths = .{

nix/package.nix

@@ -22,7 +22,7 @@ stdenv.mkDerivation (finalAttrs: {
   zigDeps = zig.fetchDeps {
     inherit (finalAttrs) pname version src;
     fetchAll = true;
-    hash = "sha256-hS69otO9YY1acs+Vn8IdFj+mV8QjoeDVtxtsCrXD3RY=";
+    hash = "sha256-0A2IlywPDkgX8+oiZ1Xd2zF0o07T4moBRngddwl1GBk=";
   };
 
   postConfigure = ''

src/analysis.zig

@@ -192,20 +192,13 @@ pub fn analyze(
     return sink.items.toOwnedSlice(allocator);
 }
 
-/// Reports length-prefix declarations that srf does not usefully report itself.
+/// Reports length prefixes that run past the end of the file.
 ///
-/// srf covers the compact-format case as of 08f06df: a value that does not end on
-/// the delimiter gets a diagnostic and a fatal `ParseFailed`. We stay out of its
-/// way there, because two squiggles for one defect is worse than one.
-///
-/// What is left to us:
-///
-///   * **Long format overruns.** `srf.zig:848` takes the `field_delimiter == '\n'`
-///     branch and calls `nextLine()` unconditionally, discarding whatever was left
-///     of the line. No diagnostic, no failure, no log: `k:3:hello` silently
-///     becomes `hel`. Silent data loss is the worst kind, so we report it.
-///   * **Declarations past the end of the file.** srf fails with a bare
-///     `EndOfStream` and no location at all.
+/// This used to cover short prefixes too, but srf reports those itself as of
+/// 4bec474, in both formats, and with a better message: it suggests the corrected
+/// byte count. Duplicating it would put two squiggles on one defect, so the only
+/// case left to us is a declaration longer than the remaining document, which srf
+/// still fails on with no location at all.
 fn appendLengthProblems(
     allocator: std.mem.Allocator,
     sink: *Sink,
@@ -219,49 +212,27 @@ fn appendLengthProblems(
             else => continue,
         };
         const problem = located.field.length_problem orelse continue;
-
-        // srf already diagnoses a compact-format overrun, and does it fatally.
-        if (problem == .overrun and !located.field.long_mode) continue;
-
-        const message = switch (problem) {
-            .truncated => |t| try std.fmt.allocPrint(
-                allocator,
-                "length prefix declares {d} bytes but only {d} remain in the file",
-                .{ t.declared, t.available },
-            ),
-            .overrun => |o| try std.fmt.allocPrint(
-                allocator,
-                "length prefix declares {d} bytes, so srf will silently discard the {d} byte(s) highlighted here",
-                .{ o.declared, o.surplus },
-            ),
+        const truncated = switch (problem) {
+            .truncated => |t| t,
+            // srf's own diagnostic is better than anything we would write here.
+            .overrun => continue,
         };
+
+        const message = try std.fmt.allocPrint(
+            allocator,
+            "length prefix declares {d} bytes but only {d} remain in the file",
+            .{ truncated.declared, truncated.available },
+        );
         errdefer allocator.free(message);
 
         sink.items.append(allocator, .{
-            // Underline what is wrong. For an overrun that is the surplus text
-            // past the declared end, not the bytes that do fit: highlighting the
-            // part srf keeps would point at the one bit that is correct.
-            .range = switch (problem) {
-                .truncated => positions.rangeFromSpan(
-                    text,
-                    located.value_span.start,
-                    located.value_span.end,
-                    encoding,
-                ),
-                .overrun => |o| positions.rangeFromSpan(
-                    text,
-                    o.surplus_start,
-                    o.surplus_start + o.surplus,
-                    encoding,
-                ),
-            },
-            // A long-format overrun still parses, so the cost is losing data
-            // rather than failing to read the file. A truncated declaration stops
-            // the parse outright.
-            .severity = switch (problem) {
-                .truncated => .err,
-                .overrun => .warning,
-            },
+            .range = positions.rangeFromSpan(
+                text,
+                located.value_span.start,
+                located.value_span.end,
+                encoding,
+            ),
+            .severity = .err,
             .message = message,
         }) catch {
             allocator.free(message);
@@ -588,16 +559,19 @@ test "an error reported at end of line underlines the whole line" {
     try testing.expectEqual(@as(u32, "broken line here".len), range.end.character);
 }
 
-test "a long-format length overrun is reported, which srf does not do" {
-    // srf keeps "hel" and drops "lo" without a word. That is silent data loss,
-    // so we report it ourselves.
-    const diags = try analyzeForTest("#!srfv1\n#!long\nk:3:hello\n");
-    defer freeDiagnostics(testing.allocator, diags);
-    try testing.expectEqual(@as(usize, 1), diags.len);
-    try testing.expectEqual(Severity.warning, diags[0].severity);
-    try testing.expect(std.mem.indexOf(u8, diags[0].message, "declares 3 bytes") != null);
-    try testing.expect(std.mem.indexOf(u8, diags[0].message, "silently discard the 2 byte(s)") != null);
-    try testing.expectEqual(@as(u32, 2), diags[0].range.start.line);
+test "a long-format overrun is left to srf, which reports it as of 4bec474" {
+    // srf used to keep "hel" and drop "lo" silently. It now reports it and
+    // suggests the corrected count, so we add nothing.
+    const ours = try lengthDiagnosticsForTest("#!srfv1\n#!long\nk:3:hello\n");
+    defer freeDiagnostics(testing.allocator, ours);
+    try testing.expectEqual(@as(usize, 0), ours.len);
+
+    const all = try analyzeForTest("#!srfv1\n#!long\nk:3:hello\n");
+    defer freeDiagnostics(testing.allocator, all);
+    try testing.expectEqual(@as(usize, 1), all.len);
+    try testing.expect(std.mem.indexOf(u8, all[0].message, "additional bytes after field value") != null);
+    try testing.expect(std.mem.indexOf(u8, all[0].message, "restated as 5") != null);
+    try testing.expectEqual(@as(u32, 2), all[0].range.start.line);
 }
 
 test "a length prefix past the end of the file is an error" {
@@ -608,22 +582,32 @@ test "a length prefix past the end of the file is an error" {
     try testing.expect(std.mem.indexOf(u8, diags[0].message, "declares 99 bytes but only 6 remain") != null);
 }
 
-test "a compact-format overrun is left to srf, which now reports it" {
-    // srf 08f06df diagnoses this fatally, so we add nothing: one defect, one
-    // diagnostic.
-    const ours = try lengthDiagnosticsForTest("#!srfv1\nk:2:abc\n");
+test "a compact-format overrun is left to srf, which reports it" {
+    // srf recovers by jumping to the next delimiter and reports once (c5e83f7).
+    const ours = try lengthDiagnosticsForTest("#!srfv1\nk:3:hello,foo::bar\n");
     defer freeDiagnostics(testing.allocator, ours);
     try testing.expectEqual(@as(usize, 0), ours.len);
 
-    const all = try analyzeForTest("#!srfv1\nk:2:abc\n");
+    const all = try analyzeForTest("#!srfv1\nk:3:hello,foo::bar\n");
     defer freeDiagnostics(testing.allocator, all);
     try testing.expectEqual(@as(usize, 1), all.len);
     try testing.expectEqual(Severity.err, all[0].severity);
-    // srf's own wording, located on the offending line.
-    try testing.expect(std.mem.indexOf(u8, all[0].message, "reset line for next item") != null);
     try testing.expectEqual(@as(u32, 1), all[0].range.start.line);
+    try testing.expect(std.mem.indexOf(u8, all[0].message, "2 additional bytes") != null);
+    try testing.expect(std.mem.indexOf(u8, all[0].message, "restated as 5") != null);
 }
 
+test "a compact overrun with no following delimiter still names the real problem" {
+    // Recovery needs a delimiter later on the line to jump to. Without one the
+    // leftover bytes are parsed as a field, so srf adds a second, less useful
+    // "no type data or value after key". The primary diagnostic is still first
+    // and still correct, which is what an editor shows on the line.
+    const all = try analyzeForTest("#!srfv1\nk:2:abc\n");
+    defer freeDiagnostics(testing.allocator, all);
+    try testing.expect(all.len >= 1);
+    try testing.expect(std.mem.indexOf(u8, all[0].message, "1 additional bytes") != null);
+    try testing.expect(std.mem.indexOf(u8, all[0].message, "restated as 3") != null);
+}
 test "a correct length prefix produces no diagnostic" {
     const diags = try analyzeForTest("#!srfv1\n#!long\nk:5:hello\n");
     defer freeDiagnostics(testing.allocator, diags);
@@ -642,18 +626,6 @@ test "a multi-line length-prefixed value produces no diagnostic" {
     try testing.expectEqual(@as(usize, 0), diags.len);
 }
 
-test "an overrun diagnostic underlines the surplus, not the bytes that fit" {
-    // "k:3:hello": the value starts at character 4 and declares 3 bytes, so the
-    // surplus "lo" runs from character 7 to 9. Highlighting 4..7 would point at
-    // the one part of the line that is correct.
-    const text = "#!srfv1\n#!long\nk:3:hello\n";
-    const diags = try analyzeForTest(text);
-    defer freeDiagnostics(testing.allocator, diags);
-    try testing.expectEqual(@as(usize, 1), diags.len);
-    try testing.expectEqual(@as(u32, 7), diags[0].range.start.character);
-    try testing.expectEqual(@as(u32, 9), diags[0].range.end.character);
-}
-
 test "a truncation diagnostic underlines the value it does have" {
     const diags = try lengthDiagnosticsForTest("#!srfv1\n#!long\nk:99:hello\n");
     defer freeDiagnostics(testing.allocator, diags);
@@ -661,8 +633,9 @@ test "a truncation diagnostic underlines the value it does have" {
 }
 
 test "length diagnostics coexist with srf's own diagnostics" {
-    // Duplicate magic header (srf's finding) plus a bad length prefix (ours).
-    const diags = try analyzeForTest("#!srfv1\n#!srfv1\n#!long\nk:3:hello\n");
+    // Duplicate magic header (srf's finding) plus a truncated length prefix
+    // (ours, since srf fails on that one without a location).
+    const diags = try analyzeForTest("#!srfv1\n#!srfv1\n#!long\nk:99:hello\n");
     defer freeDiagnostics(testing.allocator, diags);
     var from_srf = false;
     var from_us = false;
@@ -674,8 +647,8 @@ test "length diagnostics coexist with srf's own diagnostics" {
     try testing.expect(from_us);
 }
 
-test "a compact overrun no longer produces the unlocated fallback" {
-    // srf now fails fatally *with* a diagnostic, so the generic
+test "a compact overrun does not produce the unlocated fallback" {
+    // srf fails fatally *with* a diagnostic, so the generic
     // "reported no location" message must not appear alongside it.
     const diags = try analyzeForTest("#!srfv1\nk:2:abc\n");
     defer freeDiagnostics(testing.allocator, diags);
@@ -874,7 +847,7 @@ test "an unrecognised hint is left to srf, not counted as a type" {
 }
 
 test "type warnings coexist with length warnings without duplication" {
-    const text = "#!srfv1\n#!long\nk:num:5\n\nk::abc\n\nbio:3:hello\n";
+    const text = "#!srfv1\n#!long\nk:num:5\n\nk::abc\n\nbio:99:hello\n";
     const diags = try analyzeForTest(text);
     defer freeDiagnostics(testing.allocator, diags);
     var type_warnings: usize = 0;
@@ -896,3 +869,163 @@ test "a consistent document produces no warnings at all" {
     defer freeDiagnostics(testing.allocator, diags);
     try testing.expectEqual(@as(usize, 0), diags.len);
 }
+
+test "whitespace after a length-prefixed value does not crash the parser" {
+    // srf panicked here before e2b4995: an all-whitespace remainder made
+    // `checkShortPrefix` index an empty slice. Driving the real parser from our
+    // own suite means a regression upstream shows up here rather than in an
+    // editor.
+    for ([_][]const u8{
+        "#!srfv1\n#!long\nk:5:hello \n",
+        "#!srfv1\n#!long\nk:5:hello\t\n",
+        "#!srfv1\n#!long\nk:5:hello   \n",
+        "#!srfv1\n#!long\nk:5:hello \t \n",
+    }) |text| {
+        const diags = try analyzeForTest(text);
+        defer freeDiagnostics(testing.allocator, diags);
+        // Trailing whitespace is acceptable in long format, same as a comment.
+        try testing.expectEqual(@as(usize, 0), diags.len);
+    }
+}
+
+test "a comment after a length-prefixed value is accepted" {
+    for ([_][]const u8{
+        "#!srfv1\n#!long\nk:5:hello # a note\n",
+        "#!srfv1\n#!long\nk:5:hello  #tight\n",
+        "#!srfv1\n#!long\nk:5:hello\t# tabbed\n",
+    }) |text| {
+        const diags = try analyzeForTest(text);
+        defer freeDiagnostics(testing.allocator, diags);
+        try testing.expectEqual(@as(usize, 0), diags.len);
+    }
+}
+
+test "compact format does not allow trailing whitespace after a length prefix" {
+    // No comment allowance there: the next byte must be a comma or the line ends.
+    // srf reports it, so we add nothing of our own.
+    const ours = try lengthDiagnosticsForTest("#!srfv1\nk:5:hello \n");
+    defer freeDiagnostics(testing.allocator, ours);
+    try testing.expectEqual(@as(usize, 0), ours.len);
+
+    const all = try analyzeForTest("#!srfv1\nk:5:hello \n");
+    defer freeDiagnostics(testing.allocator, all);
+    try testing.expect(all.len > 0);
+    try testing.expect(std.mem.indexOf(u8, all[0].message, "additional bytes") != null);
+}
+
+test "a bad length prefix followed by more lines does not crash the parser" {
+    // Three separate srf panics have lived in this area: an all-whitespace
+    // remainder indexing an empty slice, and two from `partial_line_column`
+    // drifting out of sync with `current_line` during intra-record recovery. All
+    // of them needed a *second* line to show up, which single-record tests never
+    // supplied. These drive the real parser over multi-line documents so a
+    // regression upstream surfaces here rather than by killing the server in
+    // someone's editor.
+    for ([_][]const u8{
+        // compact, short prefix with a delimiter to recover to
+        "#!srfv1\nk:3:hello,foo::bar\nz::a\n",
+        "#!srfv1\nk:3:hello,foo::bar\nz::a,y::b\n",
+        // compact, short prefix with nothing to recover to
+        "#!srfv1\nk:2:abc\nz::a\n",
+        "#!srfv1\nk:5:hello \nz::a\n",
+        // several bad prefixes in a row
+        "#!srfv1\nk:2:abc\nj:3:hello,x::y\nm:1:zz\nn::ok\n",
+        // long format equivalents
+        "#!srfv1\n#!long\nk:3:hello\nz::a\n",
+        "#!srfv1\n#!long\nk:5:hello \nz::a\n",
+        "#!srfv1\n#!long\nk:5:hello # c\nz::a\n",
+        "#!srfv1\n#!long\nbio:5:foo\nbar\nz::a\n",
+        "#!srfv1\n#!long\nk:99:hello\n",
+    }) |text| {
+        const diags = try analyzeForTest(text);
+        freeDiagnostics(testing.allocator, diags);
+    }
+}
+
+test "a recovered short prefix reports once when the rest of the line is valid" {
+    const diags = try analyzeForTest("#!srfv1\nk:3:hello,foo::bar\nz::a,y::b\n");
+    defer freeDiagnostics(testing.allocator, diags);
+    try testing.expectEqual(@as(usize, 1), diags.len);
+    try testing.expectEqual(@as(u32, 1), diags[0].range.start.line);
+    try testing.expect(std.mem.indexOf(u8, diags[0].message, "restated as 5") != null);
+}
+
+test "each bad length prefix in a document is reported once, on its own line" {
+    const diags = try analyzeForTest("#!srfv1\nk:2:abc\nj:3:hello,x::y\nm:1:zz\nn::ok\n");
+    defer freeDiagnostics(testing.allocator, diags);
+    try testing.expectEqual(@as(usize, 3), diags.len);
+    for (diags, 1..) |d, line| {
+        try testing.expectEqual(@as(u32, @intCast(line)), d.range.start.line);
+        try testing.expect(std.mem.indexOf(u8, d.message, "additional bytes") != null);
+    }
+}
+
+test "a multi-line bad length prefix followed by more lines does not crash" {
+    // Round four of the same family: `partial_line_column` and `current_line`
+    // drifting apart during recovery from a miskeyed length prefix. Every one of
+    // these needed a following line, and several needed a comma on it. Driving the
+    // real parser here means a regression upstream fails our build instead of
+    // taking the server down mid-edit.
+    for ([_][]const u8{
+        // underflow that pulls bytes from the next line
+        "#!srfv1\nbio:5:foo\n# comment\nfinal::v\n",
+        "#!srfv1\nbio:5:foo\n  indented::v\nfinal::v\n",
+        "#!srfv1\nbio:5:foo\n# com,ment\nfinal::v\n",
+        "#!srfv1\nbio:5:foo\nab\nfinal::v\n",
+        // deep underflows spanning several lines
+        "#!srfv1\nbio:2:foo\nbar\nbaz\n",
+        "#!srfv1\nbio:1:foo\nbar\n# c\n",
+        "#!srfv1\nbio:9:foo\nbar\nbaz\nqux\n",
+        "#!srfv1\nbio:4:a\nb\nc\nd\ne\n",
+        "#!srfv1\n#!long\nbio:2:foo\nbar\nbaz\n",
+        // exact lengths that consume into the following line
+        "#!srfv1\nbio:7:foo\nbar\nfinal::v\n",
+        "#!srfv1\nbio:7:foo\nbar,baz\nfinal::v\n",
+        "#!srfv1\nbio:7:foo\nz::a\nfinal::v\n",
+    }) |text| {
+        const diags = try analyzeForTest(text);
+        freeDiagnostics(testing.allocator, diags);
+    }
+}
+
+test "multi-line recovery from a bad length prefix yields the rest of the record" {
+    // srf recovers by skipping the surplus bytes and the delimiter after them,
+    // landing where the corrected document would (srf 7f3a43e). These assert the
+    // recovered fields, not merely that nothing crashed: four separate panics have
+    // lived in this path, and "does not crash" would have passed for several of
+    // the wrong answers along the way.
+    const cases = [_]struct { text: []const u8, want: []const []const u8 }{
+        // Declares 7, so bio is "foo\nz::" and the surplus is "a".
+        .{
+            .text = "#!srfv1\nbio:7:foo\nz::a,y::b\nfinal::v\n",
+            .want = &.{ "bio", "y", "final" },
+        },
+        // Declares 12, so bio is "foo\nz:5:a,b," and the surplus is "c".
+        .{
+            .text = "#!srfv1\nbio:12:foo\nz:5:a,b,c,q::x\nfinal::v\n",
+            .want = &.{ "bio", "q", "final" },
+        },
+        // Declares 10, so foo is "bar\nbaz,qu" and the surplus is "x".
+        .{
+            .text = "#!srfv1\nfoo:10:bar\nbaz,qux,key::val\n",
+            .want = &.{ "foo", "key" },
+        },
+    };
+
+    for (cases) |case| {
+        var it = document.items(case.text);
+        var keys: std.ArrayList([]const u8) = .empty;
+        defer keys.deinit(testing.allocator);
+        while (it.next()) |item| switch (item) {
+            .field => |f| try keys.append(testing.allocator, f.field.key),
+            else => {},
+        };
+        try testing.expectEqual(case.want.len, keys.items.len);
+        for (case.want, keys.items) |w, g| try testing.expectEqualStrings(w, g);
+
+        // And the parser agrees it is broken, exactly once.
+        const diags = try analyzeForTest(case.text);
+        defer freeDiagnostics(testing.allocator, diags);
+        try testing.expectEqual(@as(usize, 1), diags.len);
+    }
+}

src/document.zig

@@ -367,12 +367,31 @@ const Scanner = struct {
             // cannot run us off the end of the document.
             const end = @min(value_start + declared, self.text.len);
             const new_line_end = self.lineEnd(end);
-            const comma_follows = !self.long_mode and end < new_line_end and self.text[end] == ',';
+            const problem = self.lengthProblem(declared, value_start);
+
+            // Where the next field starts. When the count lands on a delimiter
+            // this is simply past it. When it does not, srf recovers by skipping
+            // the surplus bytes *and* the delimiter after them, so mirror that:
+            // otherwise hover would find no field where the parser reports one.
+            const next_field: ?usize = if (self.long_mode)
+                // srf discards the remainder of the line in long format, so
+                // nothing further on it is addressable.
+                null
+            else if (end < new_line_end and self.text[end] == ',')
+                end + 1
+            else if (problem != null)
+                if (std.mem.indexOfScalarPos(u8, self.text[0..new_line_end], end, ',')) |comma|
+                    comma + 1
+                else
+                    null
+            else
+                null;
+
             return .{
                 .end = end,
-                .next_field = if (comma_follows) end + 1 else null,
+                .next_field = next_field,
                 .line_end = new_line_end,
-                .length_problem = self.lengthProblem(declared, value_start),
+                .length_problem = problem,
             };
         }
 
@@ -419,17 +438,39 @@ const Scanner = struct {
         // End of file terminates a field just as well as a newline does.
         if (after >= self.text.len) return null;
 
-        const terminator = self.text[after];
-        if (terminator == '\n') return null;
-        if (!self.long_mode and terminator == ',') return null;
+        if (self.text[after] == '\n') return null;
+        if (!self.long_mode) {
+            if (self.text[after] == ',') return null;
+        } else {
+            // Long format tolerates whitespace, and a comment, after the value.
+            // Mirrors srf's `checkShortPrefix`, so hover cannot flag something
+            // the parser is happy with.
+            var i = after;
+            while (i < self.text.len and isSpace(self.text[i])) i += 1;
+            if (i >= self.text.len or self.text[i] == '\n' or self.text[i] == '#') return null;
+        }
 
         // Measured from the declared end, not from the start of the value: a
         // multi-line length-prefixed value can put the surplus on a later line,
         // and a range built from the first line's end would come out inverted.
+        //
+        // How far the surplus runs has to match how srf counts it, or hover ends
+        // up suggesting a different corrected length than the diagnostic does. srf
+        // stops at the next delimiter in compact format (`extra_bytes` in
+        // `checkShortPrefix`) and runs to end of line in long format, where a
+        // comma is ordinary text.
+        const line_end = self.lineEnd(after);
+        const surplus_end = if (self.long_mode)
+            line_end
+        else if (std.mem.indexOfScalarPos(u8, self.text[0..line_end], after, ',')) |comma|
+            comma
+        else
+            line_end;
+
         return .{ .overrun = .{
             .declared = declared,
             .surplus_start = after,
-            .surplus = self.lineEnd(after) - after,
+            .surplus = surplus_end - after,
         } };
     }
 
@@ -738,8 +779,34 @@ test "in long format a trailing comma after the declared bytes is an overrun" {
     try testing.expectEqual(@as(usize, 1), problem.overrun.surplus);
 }
 
-test "trailing whitespace after the declared bytes is an overrun" {
-    const problem = problemAt("#!srfv1\n#!long\nk:5:hello \n", "hello").?;
+test "in long format, trailing whitespace after the declared bytes is accepted" {
+    // srf's checkShortPrefix trims whitespace before deciding, so flagging this
+    // would contradict the parser.
+    try testing.expectEqual(
+        @as(?LengthProblem, null),
+        problemAt("#!srfv1\n#!long\nk:5:hello \n", "hello"),
+    );
+    try testing.expectEqual(
+        @as(?LengthProblem, null),
+        problemAt("#!srfv1\n#!long\nk:5:hello\t\n", "hello"),
+    );
+}
+
+test "in long format, a trailing comment after the declared bytes is accepted" {
+    try testing.expectEqual(
+        @as(?LengthProblem, null),
+        problemAt("#!srfv1\n#!long\nk:5:hello # a note\n", "hello"),
+    );
+    try testing.expectEqual(
+        @as(?LengthProblem, null),
+        problemAt("#!srfv1\n#!long\nk:5:hello   # spaced\n", "hello"),
+    );
+}
+
+test "in compact format, trailing whitespace is still an overrun" {
+    // Compact format has no comment-after-value allowance: the next byte must be
+    // a comma or the line must end.
+    const problem = problemAt("#!srfv1\nk:5:hello \n", "hello").?;
     try testing.expectEqual(@as(usize, 5), problem.overrun.declared);
 }
 
@@ -870,3 +937,26 @@ test "value kinds describe themselves for a message" {
     try testing.expectEqualStrings("`bool`", ValueKind.boolean.describe());
     try testing.expectEqualStrings("`binary`", ValueKind.bytes.describe());
 }
+
+test "in compact format the surplus stops at the next delimiter, as srf counts it" {
+    // srf's `checkShortPrefix` reports `indexOfScalar(past_val, ',')` bytes, so
+    // measuring to end of line here would make hover suggest a different
+    // corrected length than the diagnostic does.
+    const problem = problemAt("#!srfv1\nim_worth:4:23,000,000,000,really:bool:false\n", "im_worth").?;
+    try testing.expectEqual(@as(usize, 4), problem.overrun.declared);
+    // "23,0" then "00" then a comma: two surplus bytes, so 4 + 2 = 6.
+    try testing.expectEqual(@as(usize, 2), problem.overrun.surplus);
+}
+
+test "in compact format a surplus with no delimiter runs to end of line" {
+    const problem = problemAt("#!srfv1\nk:2:abcdef\n", "k:2:").?;
+    try testing.expectEqual(@as(usize, 4), problem.overrun.surplus);
+}
+
+test "in long format the surplus runs to end of line, commas included" {
+    // Long format has no comma delimiter, so a comma is ordinary text and srf
+    // counts the whole remainder.
+    const problem = problemAt("#!srfv1\n#!long\nim_worth:4:23,000,000\n", "im_worth").?;
+    // Value is "23,0"; the remainder "00,000" is six bytes, commas and all.
+    try testing.expectEqual(@as(usize, 6), problem.overrun.surplus);
+}

src/hover.zig

@@ -308,22 +308,22 @@ fn writeLengthProblem(
             "**Length mismatch:** declares {d} byte{s} but only {d} remain in the file.\n\n",
             .{ t.declared, plural(t.declared), t.available },
         ),
+        // srf reports this one itself, fatally, and suggests the corrected count.
+        // Hover is still where the detail belongs, so state the byte arithmetic
+        // and the rule rather than just repeating that something is wrong.
         .overrun => |o| {
             try w.print(
-                "**Length mismatch:** declares {d} byte{s}, but the value does not end there",
-                .{ o.declared, plural(o.declared) },
+                "**Length mismatch:** declares {d} byte{s}, but {d} more byte{s} follow before the end of the field.\n\n",
+                .{ o.declared, plural(o.declared), o.surplus, plural(o.surplus) },
             );
+            try w.print("Restating the length as {d} would cover them. ", .{o.declared + o.surplus});
             if (f.long_mode) {
-                try w.print(
-                    ": {d} more byte{s} follow.\n\nsrf keeps the first {d} and discards the rest without reporting it.\n\n",
-                    .{ o.surplus, plural(o.surplus), o.declared },
+                try w.writeAll(
+                    "A length-prefixed value must be followed by end of line, optionally after whitespace or a comment.\n\n",
                 );
             } else {
-                // srf reports this one itself, fatally. Hover is still where the
-                // detail belongs, so explain the rule rather than just repeating
-                // that something is wrong.
                 try w.writeAll(
-                    ". A length-prefixed value must be followed by a comma or end of line, so srf rejects the document.\n\n",
+                    "A length-prefixed value must be followed by a comma or end of line.\n\n",
                 );
             }
         },
@@ -703,11 +703,11 @@ test "looksLikeText accepts text and rejects control bytes" {
     try testing.expect(!looksLikeText("bad\x07bell"));
 }
 
-test "an overrun length in long format explains srf's silent truncation" {
+test "an overrun length in long format suggests the corrected count" {
     const text = "#!srfv1\n#!long\nk:3:hello\n";
-    try expectHoverContains(text, "k:3:", "declares 3 bytes, but the value does not end there");
-    try expectHoverContains(text, "k:3:", "2 more bytes follow");
-    try expectHoverContains(text, "k:3:", "discards the rest without reporting it");
+    try expectHoverContains(text, "k:3:", "declares 3 bytes, but 2 more bytes follow");
+    try expectHoverContains(text, "k:3:", "Restating the length as 5");
+    try expectHoverContains(text, "k:3:", "optionally after whitespace or a comment");
 }
 
 test "a truncated length reports what is actually left" {
@@ -718,8 +718,13 @@ test "a truncated length reports what is actually left" {
 test "an overrun length in compact format explains the terminator rule" {
     const text = "#!srfv1\nk:2:abc\n";
     try expectHoverContains(text, "k:2:", "followed by a comma or end of line");
-    // Hover explains it even though the diagnostic for it comes from srf.
-    try expectHoverContains(text, "k:2:", "srf rejects the document");
+    try expectHoverContains(text, "k:2:", "Restating the length as 3");
+}
+
+test "a trailing comment in long format is not reported as a mismatch" {
+    const markdown = try hoverOn("#!srfv1\n#!long\nk:5:hello # fine\n", "hello");
+    defer testing.allocator.free(markdown);
+    try testing.expect(std.mem.indexOf(u8, markdown, "Length mismatch") == null);
 }
 
 test "a correct length prefix reports no mismatch" {