Skip to content

fix(sdk): keep partial suffix lines out of coarse blame - #468

Open
Jackmeson1 wants to merge 1 commit into
Ephemeral-AI-Lab:mainfrom
Jackmeson1:fix/coarse-blame-suffix
Open

Jackmeson1 wants to merge 1 commit into
Ephemeral-AI-Lab:mainfrom
Jackmeson1:fix/coarse-blame-suffix

Conversation

@Jackmeson1

Copy link
Copy Markdown

Splitting alpha beta into alpha and beta in a 2,005-line file made coarse fs.blame credit beta to the base Layer, or to an earlier Commit that wrote alpha beta. This change leaves that partial suffix line with the Commit that split it.

Cause and change

At caebd268d, engine/crates/layerfs-store-service/src/read/blame.rs:68–106 computes common byte edges. Its coarse branch at lines 209–217 maps a newer suffix line to new_line + old.lines - new.lines without checking the corresponding older line boundary.

In the reproduction, the prefix is five bytes and the suffix starts at byte 6 in both versions. That byte starts beta\n in the newer version but follows a space in the older version. New line 2 is consequently mapped to old line 1, alpha beta.

equal_edges() now checks whether the older suffix starts at byte zero or immediately after LF. If neither holds, it reduces the suffix length by one byte. That excludes a newer line starting exactly at the original suffix boundary. Later lines remain eligible because their preceding LF is inside the equal suffix in both versions.

The suffix remains byte-based; it is not rounded to a line boundary. The added check makes at most one logical one-byte range request per changed coarse comparison. Underlying content acquisition can read or decode more than one byte. No additional full-file line vector is introduced, and long-history performance was not measured.

api/docs/agent.md and core/docs/architecture/82-sdk-call-channel.md document the suffix-line eligibility rule.

A separate prefix-at-EOF defect remains: a newer unterminated last line that is a shortened older line can be accepted by the prefix branch. Its demonstrated probes and extension are reserved for a separate follow-up.

Validation

I ran the following outside the sandbox on a Colab CPU runtime: Ubuntu 24.04.5, Linux 6.6.159+ x86_64, overlayfs, Rust/Cargo 1.85.1 and cargo-nextest 0.9.150, with LAYERFS_CONSTRUCTION_WORKERS=1.

The existing Rust fixtures explicitly select Disposable persistence and invoke layerfs_api::call. Their bootstrap bypasses normal filesystem admission. These results cover the Rust API behavior, not native-addon, worker, FUSE, or supported-filesystem installation.

The new engine/crates/layerfs-api/tests/sdk_blame_coarse.rs contains eleven tests. It checks every final line's text and origin across bounded pagination.

  • Six fail on caebd268d and pass with the fix: splits at the first, middle and last line; a last-line split without final LF; xalpha cut to alpha; and a split of a line written by an earlier Commit.
  • Five controls pass on both: joining lines, adding/dropping the first line, preserving an unterminated last line, adding final LF, and the same split in a 10-line file.

Commands:

export LAYERFS_CONSTRUCTION_WORKERS=1
cargo +1.85.1 test --locked -p layerfs-api --test sdk_blame_coarse --no-run
timeout 120 cargo +1.85.1 test --locked -p layerfs-api --test sdk_blame_coarse -- --test-threads=1

filter="$(python3 -B tools/ci_tests.py filter fast)"
cargo +1.85.1 nextest run --locked -p layerfs-store-service -p layerfs-api --no-run --profile ci -E "$filter"
timeout 120 cargo +1.85.1 nextest run --locked -p layerfs-store-service -p layerfs-api --profile ci -E "$filter" --no-fail-fast
Check caebd268d plus regression tests With the suffix fix
sdk_blame_coarse 5 passed, 6 failed 11 passed
Standalone issue reproduction 1 failed 1 passed
Saved prop07 check 1 failed 1 passed
sdk_method_golden and method_refusal_order 2 passed 2 passed
Two-crate fast nextest selection 112 passed, 7 failed, 6 skipped 118 passed, 1 failed, 6 skipped

The remaining failure is layerfs-api::local_install import_installs_the_store_in_process. Both runs refuse the Colab overlayfs destination because direct Store access requires ext4, xfs or btrfs. Neither fast-lane run is wholly green.

A throwaway probe, not included in this PR, splits lines at the beginning, middle and end of approximately 1.85-MB files containing about 30,000 lines. Its three cases fail on base and pass with the fix. These files exceed both comparison bounds; this does not isolate the byte-only threshold.

Additional recorded checks:

  • cargo +1.85.1 fmt --all -- --check: passed.
  • cargo +1.85.1 clippy --locked -p layerfs-store-service -p layerfs-api --all-targets -- -D warnings: passed.
  • python3 -B tools/check_product_boundary.py --skip-sdk: passed on clean copies of base and fixed trees.
  • python3 -B tools/check_product_boundary.py: passed on a clean fixed-tree copy.
  • python3 -B -m unittest discover -s tools -p 'test_*.py': 155 tests passed. This recorded invocation lacked an explicit wall timeout.

The boundary guard also produced two path-dependent violations in unchanged files when the checkout was under /root/lfs/src; the clean-copy results above preserve that distinction.

Slow/nightly and mount selections, isolated byte-limit coverage, CRLF/UTF-8-specific cases, and long-history performance were not run. Durable: NOT_RUN — disabled by owner until explicit reauthorization.

No matching coarse-suffix report was found in the saved issue/PR search results. That search is a point-in-time check.

Production LOC: 102,200 -> 102,206 (delta +6)
engine 61,313 -> 61,319 (+6): layerfs-store-service 10,032 -> 10,038 (+6);
core 38,241 and api 2,646 unchanged.
Reference absent (retired R9c; last counted 65,417 at tree 498dd1917812).
Method: tools/production_loc.py
(blob 3341e4d0362c822b6d9a08419c432942b3a13800)
--root --json, using the caebd26 archive and that archive
with this change applied. The documentation wording edits
do not change production source. Scope includes first-party production
Rust, runtime SQL and API JavaScript; excludes tests, inline test code,
documentation, tooling, dependencies and generated artifacts.

No tests deleted.

Fixes #467

AI disclosure: this was prepared with AI assistance (Claude Opus 5.5).

🤖 Generated with Claude Code

https://claude.ai/code/session_019TTGAv6fe859ebbCLLfLHP

When changed file versions exceed blame's 2,000-line or 48-KiB comparison
bounds, the coarse path maps suffix lines by the total line-count
difference. It did not check that the corresponding older suffix byte
started a line. Splitting alpha beta into alpha and beta could therefore
credit beta to the base Layer or an earlier Commit containing alpha beta.

Check the older suffix boundary in equal_edges. If it is neither byte
zero nor immediately after LF, reduce the suffix length by one byte.
This excludes a newer line starting exactly at the original boundary;
later suffix lines retain matching preceding LF bytes in both versions.

The check adds at most one logical one-byte range request per changed
coarse comparison, with no additional full-file line vector. Underlying
object reads can be larger. Update agent.md and 82-sdk-call-channel.md
to describe inherited suffix lines, rather than a line-aligned suffix.

Keep the separately demonstrated prefix-at-EOF defect for a follow-up.

Outside-sandbox validation I ran on a Colab CPU runtime, Ubuntu 24.04.5,
Linux 6.6.159+ x86_64/overlayfs, Rust 1.85.1, nextest 0.9.150:
- sdk_blame_coarse: base 5 passed/6 failed; fixed 11 passed.
- Standalone split reproduction and saved prop07: fail before, pass after.
- sdk_method_golden and method_refusal_order: pass on both.
- Two-crate fast nextest: base 112 passed/7 failed; fixed 118 passed/1
  failed; 6 skipped on each. local_install refuses overlayfs on both.
- Scoped warning-denying Clippy and formatting pass; boundary guards
  pass on clean copies. The path-dependent guard failure is retained.
- Tool self-tests: 155 passed; that invocation lacked a wall timeout.

The Rust fixtures use Disposable Stores and bypass normal filesystem
admission. Slow/mount, native-addon/worker and long-history performance
coverage were not run. Durable remains disabled by owner. No tests deleted.

Production LOC: 102,200 -> 102,206 (delta +6)
  engine 61,313 -> 61,319 (+6): layerfs-store-service 10,032 -> 10,038 (+6);
  core 38,241 and api 2,646 unchanged.
  Reference absent (retired R9c; last counted 65,417 at tree 498dd19).
  Method: tools/production_loc.py
  (blob 3341e4d)
  --root <snapshot> --json on the caebd26 archive and that archive
  with this change applied. Documentation wording edits
  leave production source unchanged. Counts include first-party Rust,
  runtime SQL and API JavaScript, excluding tests and inline test code,
  docs, tooling, dependencies and generated artifacts.

AI disclosure: this was prepared with AI assistance (Claude Opus 5.5).

Signed-off-by: Jackmeson1 <130693642+Jackmeson1@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TTGAv6fe859ebbCLLfLHP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs.blame credits a split line to an older version in coarse comparisons

1 participant