FileId/span integrity hardening, phase A: fix wrong-file offset binding across engine, writers, attribution, and Lua (bd-nv4p0eb1) - #482
Merged
Conversation
…bd-nv4p0eb1) Research doc: full audit of the (FileId, offsets, content) splittability bug class across q2, quarto-yaml, and quarto-source-map — verified findings, namespace map, design options A/B/C with VFS constraints and review-resolved open questions. Plan doc: phase-A work items, one strand per item (bd-itj2mjkr, bd-x113wg9v, bd-f6h40a9r, bd-t3enk8gq, bd-vmlhw7nx, bd-thagcbfq, bd-h5rfw3ao, bd-r64mj1aa, bd-fc3mf161, bd-jrq4hroi, bd-y5gpc8yv). Upstream umbrella issue: posit-dev/quarto-yaml#17. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…2mjkr) EngineExecutionStage derived the intermediate's FileId from ast_context.filenames.len(), but MetadataMergeStage grows source_context.files (via add_file_with_id, sparse hash ids) without growing filenames — so in any project render with an executable engine the remapped executed AST attributed engine-produced blocks to whatever file occupied that dense slot (typically _quarto.yml). - new_slot now comes from the SourceContext's own add_file / add_file_with_info return value. - The executed-AST remap is conditional (FileId(0) -> new_slot) instead of additive, so any non-zero id passes through untouched rather than being shifted into garbage. - Corrected both comments that claimed files/filenames stay in lock-step. TDD: stage-level test engine_blocks_attribute_to_intermediate_after_- config_registration verified red on baseline (block attributed to /project/_quarto.yml), green after. Workspace: 11147 tests pass; cargo xtask verify --skip-hub-build green. E2E: q2 render of a project fixture (_quarto.yml + jupyter cell) executes and renders clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d slot fix Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…riter (bd-f6h40a9r) The incremental writer fell back to raw start_offset()..end_offset() spans wherever preimage_in returned None — but preimage_in returns None precisely when the node's bytes are NOT in the target file (foreign provenance from include expansion, gappy Concats, synthetic nodes). The fallback then sliced original_qmd at another file's coordinates: wrong bytes copied into the rewritten document when the range fit, a panic when it didn't. Fixed all four raw-offset sites, degrading to whole-block Rewrite (re-serialization) on a preimage miss — the mechanism coarsen already supports: - KeepBefore inline assembly (assemble_inline_content): bail on miss instead of inline_source_span fallback. - assemble_recursed_container: container span and child delimiters now preimage-derived with ordering guards; bail on miss. Signature (and assemble_inline_content's) now returns Option to propagate the bail. - Block-level Verbatim coarsening: kept blocks must have an in-target preimage; foreign kept blocks become Rewrite entries. - compute_separator: original-gap reuse now requires both adjacent blocks' preimages and a well-formed gap; falls back to the standard separator otherwise. TDD: four integration tests (mid-inline in-range/out-of-range, recursed container, kept block) all verified red first — two wrong-bytes symptoms, two panics (one via compute_separator). pampa: 4304 tests pass; workspace: 11151 pass. Not browser-verified: the production surface is the hub-client save path, which the existing JSON-roundtrip integration helpers simulate; full xtask verify (hub leg) runs on the integration branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er foreign-offset fix Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bd-t3enk8gq) section.rs (standalone-caption attach) and pipe_table.rs (adjacent caption) fused the table and caption spans by pairing raw start_offset()/end_offset() values with root_file_id().unwrap_or(FileId(0)) in a hand-built Original. Three defects in one expression: raw offsets are parent-relative for Substrings (buffer offsets stamped as file-absolute — mis-anchoring the table by the parent's start under a parent_source_info parse, a public qmd::read parameter); no same-file check between table and caption; and unwrap_or(FileId(0)) mints the aliasing dummy id. Both sites now use hull_source_infos (promoted to pub(crate) in postprocess.rs), which same-file-checks via root_file_id, resolves absolute offsets via preimage_in, and degrades losslessly to combine() on mismatch. TDD: new integration module table_caption_provenance verified red first — both parent-context tests resolved the table start to buffer-relative 0 instead of absolute 1000; no-parent control unchanged. pampa: 4307 tests pass; workspace: 11154 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ll fix Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssed modules (bd-jrq4hroi) New xtask lint rule flagging SourceContext::add_file_with_id outside blessed modules — the API pairs an arbitrary FileId with arbitrary content, the exact splittable state behind bd-m6wmztln's wrong-file diagnostic spans. Everywhere else must use quarto_core::config_sources::bind_config_source (PR #478) or carry a reasoned // lint:allow(add-file-with-id) marker. - Blessed: config_sources.rs (the helper itself), metadata_merge.rs (single-path-derived triple), span_assert.rs (test helper); plus render_scripts.rs and project_resources.rs TEMPORARILY, to avoid inline-comment conflicts with in-flight PR #478 — remove after it and bd-x113wg9v land. - Inline allowances with reasons at the four verified sites the red run flagged: commands/render.rs ×2 (derived id / hash-equality guarded), project/mod.rs project_type_error (pre-merge ordering, bd-h5rfw3ao cited), theme_diagnostic.rs (candidate-matched). - Rule skips #[cfg(test)] modules and #[test] fns; 6 unit tests; documented in CLAUDE.md. Red run verified before allowances: exactly the four audited sites flagged, test-mod sites correctly skipped. cargo xtask lint now clean; xtask/quarto-core/quarto suites pass (3348 tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…guardrail Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(bd-vmlhw7nx) query_attribution skipped nodes via `file_id != 0` — a raw-usize comparison encoding "the blamed doc is slot 0" with nothing tying the constant to what the provider actually blamed. If slot assignment ever changes, the gate silently misattributes (byte-range collision into the wrong file's runs) or silently skips the whole document. - AttributionData gains `file_id`: the identity of the file the runs' byte ranges index into (serde(skip); manual Default with FileId(0)). - AttributionDataBuilder gains set_document_file_id (defaults to the primary document's dense slot, documented against ParseDocumentStage's slot-0 invariant). - Both sidecar producers thread the provider's value through. - query_attribution takes the blamed FileId (threaded through the AST walkers) and compares typed ids. TDD: 4 unit tests written against the new semantics with the gate still on literal 0 — the two fid-semantics tests verified red, the two v1-behavior controls passed; gate fixed, all green. Workspace: 11158 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ile ranges (bd-thagcbfq)
si:byte_range() discarded the resolved FileId, handing Lua bare
{start, end} — so the natural filter idiom byte_range() ->
quarto.attribution.lookup_range(r[1], r[2]) silently misattributed
include-spliced nodes (their offsets collide into the primary doc's
blame runs). Only the bundled lookup thunk guarded, via a hardcoded
fid ~= 0 that every filter author had to rediscover.
- byte_range() tables now carry file_id alongside start/end_.
- lookup_range accepts an optional third argument declaring the
range's file; when given and different from the handle's blamed file
it returns nil (compared Rust-side). Two-arg calls keep the
historical caller-asserts-primary contract.
- AttributionLookup gains blamed_file_id() (default 0, the v1
primary-doc slot); quarto-core's handle overrides it from
AttributionData::file_id (bd-vmlhw7nx's field).
- The lookup thunk passes r.file_id instead of hardcoding fid ~= 0.
TDD: both new filter tests verified red first (file_id key absent;
three-arg call silently ignored the third arg and returned a hit).
pampa+quarto-core: 7218 tests pass; workspace: 11168 pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…osure Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y5gpc8yv) parse(), parse_with_parent(), and parse_with_context() hardcoded FileId(0) for anonymous parses. SourceContext::get_file falls back to positional indexing for unmapped ids, so an anonymous XML span resolved against whatever file occupies an unrelated context's first dense slot — in q2, the primary document — rendering XML offsets against qmd text. New exported ANONYMOUS_FILE_ID = FileId(usize::MAX): resolves in no context, so anonymous spans degrade span-less, never wrong-file. Aligns with the reserved "unknown" id planned for quarto-source-map's binding-API redesign. No working spans regressed: the only production consumers (quarto-csl via pampa's citeproc filter, quarto-citeproc locales) stringify errors without span rendering today; callers that know the path can already use parse_with_file_id. TDD: anonymous_parse_does_not_alias_dense_slot_zero verified red (anonymous fid resolved to main.qmd's slot). quarto-xml: 25 tests pass; workspace: 11169 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… FileId Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The scripted edit bypassed the on-edit rustfmt hook; caught by cargo xtask verify's formatting gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lockfile drift from the v0.14.0 release (workspace crates 0.13.0 -> 0.14.0), surfaced by the first WASM verify build on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…v4p0eb1-span-hardening Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> # Conflicts: # crates/wasm-quarto-hub-client/Cargo.lock
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase A of the FileId/span integrity hardening (bd-nv4p0eb1, the systematic audit follow-up to #478): seven strands fixing every verified q2-side instance of the "splittable (FileId, offsets, content)" bug class outside the sites #478 already covers, plus a lint guardrail preventing new ones. One
--no-ffmerge per strand; every fix was TDD red-first.Assessment:
claude-notes/research/2026-08-09-fileid-span-integrity-audit.mdPlan:
claude-notes/plans/2026-08-09-fileid-span-hardening-phase-a.mdThe fixes
EngineExecutionStagederived the engine intermediate's FileId fromast_context.filenames.len(), butMetadataMergeStagegrowssource_context.fileswithout growingfilenames— so in every project render with an executable engine, engine-produced blocks were remapped onto_quarto.yml's dense slot. Now uses the idadd_file*actually returns; the executed-AST remap is conditional (FileId(0) → new_slot) instead of additive. E2E-verified with a real jupyter project render.start_offset()..end_offset()whereverpreimage_inreturnedNone— which is precisely the foreign-file/gappy-Concat case — slicingoriginal_qmdat another file's coordinates (wrong bytes, or a panic). All four sites (KeepBefore inlines, recursed containers, block-level Verbatim coarsening,compute_separator— the red run caught the last one panicking) now degrade to whole-block Rewrite.section.rs/pipe_table.rspaired raw offsets (parent-relative for Substrings) withroot_file_id().unwrap_or(FileId(0))in a hand-builtOriginal. Both now use the same-file, preimage-basedhull_source_infos(promotedpub(crate)); red tests exercised the mis-anchoring throughqmd::read's publicparent_source_infoparameter.file_id != 0.AttributionDatanow carries the blamed file'sFileId;query_attributioncompares typed ids threaded through the walkers.si:byte_range()discarded the fid, solookup_range(r[1], r[2])silently misattributed include-spliced nodes. The table now carriesfile_id;lookup_rangetakes an optional third argument checked Rust-side against the handle'sblamed_file_id(); the bundledlookupthunk passes it instead of hardcodingfid ~= 0.FileId(0), which positionally aliases the primary document in any populatedSourceContext. Now a non-aliasingANONYMOUS_FILE_ID = FileId(usize::MAX)— span-less degradation, never wrong-file.add-file-with-idxtask lint restrictingSourceContext::add_file_with_idto blessed modules; everywhere else must usebind_config_source(Bind the correct config-source file to Q-5-8/Q-5-1 diagnostic spans (bd-m6wmztln, bd-p86nlm92) #478) or carry a reasoned// lint:allow(add-file-with-id). The red run flagged exactly the four audited sites, now carrying reasoned allowances.render_scripts.rs/project_resources.rsare temporarily blessed to avoid conflicts with Bind the correct config-source file to Q-5-8/Q-5-1 diagnostic spans (bd-m6wmztln, bd-p86nlm92) #478 — to be removed once Bind the correct config-source file to Q-5-8/Q-5-1 diagnostic spans (bd-m6wmztln, bd-p86nlm92) #478 and bd-x113wg9v land.Testing
cargo xtask verifygreen at branch head (Rust + lint + fmt + hub/WASM legs).admin_collect_lifecycle; no dependency edge to these changes, green in isolation and on rerun).Relationship to other work
Independent of #478 (based on
main); the remaining audit strands (bd-x113wg9v, bd-h5rfw3ao, bd-r64mj1aa) wait for #478'sbind_config_source+extension_manifest_paths. The upstream API redesign (quarto-source-map/quarto-yaml, "Option B/C") is tracked in the research doc §6 and posit-dev/quarto-yaml#17.🤖 Generated with Claude Code