Stop reporting truncated diff and stream output as if it were complete - #7
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes structural diff and content-stream dump output "honest" by preventing the CLI and UI from presenting bounded/truncated results as complete or identical.
Changes:
- Add explicit depth-cap truncation marking in the diff tree (
DiffNode.truncated) and an additive summary counter (summary.truncatedSubtrees), and treat any truncation as "not identical" (exit 1 / no identical banner). - Fix
dump stream --page Nfor multi-stream/Contentsarrays by concatenating all referenced streams (whitespace-joined) and tokenizing as one program. - Add acceptance + unit + component tests and document new fixtures to prevent regressions.
Reviewed changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/14-3-no-silent-truncation/multistream_test.go | Black-box CLI tests ensuring multi-stream page content is not silently partial across --json, --ops, and --raw. |
| tests/14-3-no-silent-truncation/helpers_test.go | Shared harness for building/running the CLI and parsing JSON output for Story 14.3 acceptance tests. |
| tests/14-3-no-silent-truncation/go.mod | Separate Go test module for runtime-red acceptance suite. |
| tests/14-3-no-silent-truncation/diff_truncation_test.go | Black-box CLI tests ensuring depth-capped diffs do not claim "identical" and expose truncation in text/JSON. |
| testdata/correctness/README.md | Documents the new fixture PDFs and the specific failure modes they exercise. |
| internal/pdfcore/stream.go | Adds multi-stream /Contents enumeration + GetPageContentStream concatenation and keeps legacy first-stream node ID API. |
| internal/pdfcore/stream_test.go | Unit tests for /Contents enumeration behavior including degenerate array elements. |
| internal/pdfcore/model.go | Clarifies ContentStreamData semantics for multi-stream pages. |
| internal/pdfcore/diff.go | Adds truncation markers/counting, reconciliation to avoid false positives, and depth-cap aware delta tallying. |
| internal/pdfcore/diff_test.go | Unit tests for depth-cap truncation counting and shared-object reconciliation regression. |
| frontend/src/components/DiffView.tsx | UI mirrors backend identical logic by considering truncatedSubtrees and displays truncation notes/tags. |
| frontend/src/components/DiffView.truncation.test.tsx | Component tests verifying the "identical" banner is suppressed and truncation is surfaced when bounded. |
| cmd/cli/cmd_stream.go | CLI now uses GetPageContentStream for page mode and factors "no /Contents" rendering per output surface. |
| cmd/cli/cmd_diff.go | CLI identical verdict and plain output now account for TruncatedSubtrees and surface truncation notes/tags. |
Comments suppressed due to low confidence (2)
internal/pdfcore/stream.go:113
- This inline comment says it surfaces "the first stream's" decode/type failure, but the function actually surfaces the first failing stream encountered while iterating (which may be stream 2+). Clarify to avoid confusion when debugging multi-stream pages.
if cs.Error != "" {
// Surface the first stream's decode/type failure verbatim.
return &ContentStreamData{NodeID: id, Error: cs.Error}, nil
frontend/src/components/DiffView.truncation.test.tsx:54
- This comment says truncatedSubtrees is cast through unknown because DiffSummaryData does not declare it, but DiffSummaryData now includes truncatedSubtrees. Remove or reword this comment to match the current types.
// Additive field surfaced by the Go DiffSummary (AC2). Cast through unknown
// because DiffSummaryData does not declare it yet (red-phase seam).
truncatedSubtrees: 1,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/pdfcore/stream.go:57
- In pageContentStreamNodeIDs, the pre-check
if _, ok := v[0].(IndirectRef); !okrejects a /Contents array whose first element is PDF null (decoded as Go nil), even though the loop below explicitly treats nil elements as valid and skips them. Removing this guard also lets the more-informative loop error message (with index/type) handle non-ref elements consistently.
if _, ok := v[0].(pdfcpu_types.IndirectRef); !ok {
return nil, fmt.Errorf("contents array element is not an indirect reference")
}
tests/14-3-no-silent-truncation/multistream_test.go:94
- This test's header comment says the floor path is a "DISTINCT trailing meta record", but the test does not assert the record is trailing (it accepts the meta record anywhere in the NDJSON). Either enforce trailing-ness or adjust the comment to avoid promising a stricter contract than the test checks.
// records from BOTH streams, or (floor) a DISTINCT trailing meta record
// carrying the truncation state (streamCount, no "op" key) that rides the
// NDJSON without breaching Story 14-1's one-object-per-operator contract. RED
unidoc-alip
left a comment
unidoc-alip
left a comment
There was a problem hiding this comment.
The core of this is right, and the two failure modes it closes are real ones: an inverted diff verdict at exit 0, and a partial content stream presented as a whole page. The reconciliation pass is the right instinct too - the false-positive direction is the easy mistake here and you avoided it.
One gap blocks: the Go side got diffNodeHasDelta updated so a truncated-but-unchanged node still surfaces in the delta, but the frontend's hasDelta did not get the same treatment. On a PDF whose only anomaly is a depth-capped subtree, nothing auto-expands, so the [truncated: depth cap] row added at DiffView.tsx:347 is unreachable. The new component test passes anyway because it matches /truncat/ against the whole container, which the summary note already satisfies - so the branch it claims to cover is untested.
Also worth resolving before merge: GetPageContentStream is never bound into pdfservice, so the GUI's "go to page" path still lands on stream 1 alone - an undisclosed remaining instance of the bug this PR closes. Plus inconsistent /Contents null handling between index 0 and later indices, and reconcileTruncation keying off a set populated on walk entry rather than on completed full walk. Security review found nothing.
…achable Frontend hasDelta now treats node.truncated as a delta (mirroring the Go diffNodeHasDelta), so a PDF whose only anomaly is a depth-capped subtree auto-expands to the [truncated: depth cap] row instead of burying it under an unexpanded ancestor. Tighten the component test to assert the per-node row marker + node path, not just the summary note (PR review, alip must-fix).
…tion naming Drop the pageContentStreamNodeIDs v[0] guard so a null is skipped at any index (the loop already errors on non-null non-refs with index+type); add a [null ref] case. Rename reconcileTruncation's walkedPairs -> enteredPairs and document that the set is populated on diff ENTRY, not full-walk completion, with the argument for why the truncation count still cannot wrongly reach zero; pin it with TestReconcileTruncation_EntryNotCompletion (PR review, alip should-fix).
…GUI gap Remove the now-pointless strings.Builder on the empty-stream plain path and the stray blank line left by the --ops meta removal. Document the --raw multi-stream concatenation contract change and the remaining not-handled cases (indirect-ref -to-array /Contents; GUI Go-to-Page still shows stream 1) in the fixture README (PR review, alip nits + should-fix disclosure).
|
Thanks @unidoc-alip - pushed a0bc0e0..f30ac38 addressing the round. Fixed
Deferred (disclosed, not closed)
Gate green: |
Two spots where we were quietly handing back a partial answer and letting it pass for the whole thing.
The diff depth cap. The walker stops recursing at 32 levels and compares whatever it finds down there by a one-line summary instead. If the two summaries happened to match, the node came back
unchanged, the delta tally stayed at zero, and the CLI printed "Documents are structurally identical." and exited 0. That's not a degraded answer, it's an inverted one, and 32 levels isn't as deep as it sounds - tagged structure trees, nested OCG hierarchies and form field trees blow past it routinely. Now a node cut at the cap carries aTruncatedflag, the summary counts how many subtrees were affected, and if that count is non-zero we don't claim identical and we exit 1 with a line saying how many subtrees we couldn't rule out. Exit 0 now only ever means we walked the whole thing.The annoying part was not over-shooting in the other direction. My first pass flagged every depth-cap hit, which double-counted objects reachable both on a short path and again below the cap, and that flipped genuinely identical files to exit 1 - same crime, opposite direction. There's a reconciliation pass after the walk that clears the flag on any pair that did get fully compared somewhere else. The regression test for it goes through the deep-first key order on purpose, since just reordering the checks looks like it fixes it and doesn't.
Multi-stream page content. When a page's
/Contentsis an array,dump stream --page Nwas decoding element zero and calling it the page content. Incremental-update writers split content across streams all the time, so you'd get aqwith noQand no hint anything was missing. It concatenates the whole array now, newline-joined so tokens can't fuse across a boundary, tokenized as one program. I first shipped this as a truncation marker instead, because I assumed real concatenation would dragGetPageContentStreamNodeIDand everything bound to it along with it - turned out a separate method next to it costs nothing, so the marker machinery came back out again in the last commit.Still not handled:
/Contentsas an indirect ref to an array. pdfcpu doesn't pre-dereference it, so you getnode is not a stream objectand exit 2, which is at least loud about failing rather than showing you half a page. Left it alone and wrote it down.The diff view got the same treatment so it can't put up a false "identical" banner either.
New fixtures are
deep-change-a.pdf/deep-change-b.pdf(one scalar differing well below the cap) andmulti-content-stream.pdf. Checked before pushing:go vetclean,golangci-lint0 issues,go test -race ./...all packages ok, the per-suite modules for this work plus the structural diff and stream/op suites ok,tscclean, 835 frontend tests passing.