[US-1820] Rename dump plaintext to dump bytes, and reject unexpected positional arguments everywhere - #21
Conversation
Array-element scalars now carry the full value in the model, so --json is no longer limited to the clamped row label. A changed diff node whose two sides decode alike falls back to the byte-exact renderings instead of printing the same text twice. valueRaw is emitted for every hex literal and every literal carrying an escape, so its absence means what the field docs claim. The binary carve-out applies on the diff surface. The GUI row escapes and clamps once per render instead of pushing an uncapped value into title.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The functional changes are well-covered by new/updated tests, with only minor documentation-comment inconsistencies noted.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR updates the CLI and object-tree surfaces to (1) rename dump plaintext to dump bytes with a deprecation alias, and (2) consistently reject unexpected positional arguments across the command surface to avoid silently ignoring trailing flags; it also refines scalar value/valueRaw emission and diff summarization to better preserve stored-form fidelity while keeping binary carve-outs safe.
Changes:
- Add a shared positional-arity guard and apply it across CLI entry points so trailing flags or extra operands become usage errors with command-appropriate exit codes.
- Rename
dump plaintexttodump bytes, keepplaintextas a hidden alias that emits a pinned stderr deprecation notice while preserving byte-exact stdout. - Expand scalar value handling: array-element scalars now emit
valuein JSON,valueRawis emitted whenever the stored form differs from the display form, and diff summaries fall back to byte-exact renderings on decode collisions while preserving binary carve-outs.
| File | Description |
|---|---|
| tests/object-tree-scalar-values/tree_values_test.go | Updates tests for array-element value in JSON and label/value expectations. |
| tests/object-tree-scalar-values/text_decoding_test.go | Updates and expands coverage for valueRaw emission rules. |
| tests/object-tree-scalar-values/fixtures_test.go | Extends fixtures for hex-ascii, escaped literals, and signature/diff carve-out cases. |
| tests/object-tree-scalar-values/diff_values_test.go | Adds diff regression tests for decode-collision fallback and binary carve-out preservation. |
| tests/cli-views/usage_help_test.go | Updates help expectations to list dump bytes and adjust examples. |
| tests/cli-views/positional_arity_test.go | New integration suite enforcing positional arity across all CLI commands. |
| tests/cli-views/helpers_test.go | Adds runCLIIn and testdata copy helper to support dash-leading path tests. |
| tests/cli-views/dump_bytes_test.go | New integration tests for dump bytes and deprecated dump plaintext alias parity and stderr notice behavior. |
| tests/cli-views/document_views_test.go | Renames plaintext coverage to bytes and adds shared trailing-flag rejection coverage. |
| tests/cli-output-format-normalization/format_default_test.go | Updates normalization test docs to refer to dump bytes. |
| tests/cli-output-format-normalization/bytes_test.go | Renames plaintext tests to bytes and updates invocations accordingly. |
| scripts/verify-cli-output-parity.sh | Adjusts parity harness to compare baseline dump plaintext vs head dump bytes without stderr skew. |
| internal/pdfcore/tree.go | Populates Value for all scalar leaves (including array elements) and updates scalar value/raw logic usage. |
| internal/pdfcore/scalarrender.go | Replaces decode-change heuristic with rawCounterpartNeeded to decide valueRaw emission. |
| internal/pdfcore/scalarrender_test.go | Updates unit tests for rawCounterpartNeeded and diff summarization signatures. |
| internal/pdfcore/model.go | Updates TreeNode docs for value/valueRaw semantics. |
| internal/pdfcore/diff.go | Threads binary-carve-out context through diff traversal and adds decode-collision fallback logic. |
| internal/pdfcore/diff_test.go | Updates scalar-leaf diff tests for collision fallback and binary stand-in preservation. |
| frontend/src/lib/escapeDisplayValue.ts | Adds clamp-and-marker helper and render cap constant for GUI tree value rendering. |
| frontend/src/lib/escapeDisplayValue.test.ts | Adds unit tests for clamping, counting, and escape-boundary behavior. |
| frontend/src/hooks/useDocumentState.tsx | Updates TreeNode type docs to match new value/render-clamp contract. |
| frontend/src/components/TreePanel.tsx | Renders array-element values once and clamps/escapes once per render with a hard cap. |
| frontend/src/components/TreePanel.scalarValues.test.tsx | Extends GUI rendering tests for array-element rows and render-ceiling clamping. |
| docs/cli-usage.md | Documents dump bytes, alias deprecation/removal, and the "flags before file" rule. |
| cmd/cli/usage_text_test.go | New tests pinning printUsage bytes-line content and command-description alignment. |
| cmd/cli/main.go | Adds dump bytes, implements dump plaintext alias notice, and introduces requirePositionals. |
| cmd/cli/docs_consistency_test.go | New tests enforcing doc/help command-list parity and consistent alias removal version. |
| cmd/cli/doc.go | Updates CLI contract documentation for stderr shapes, exit codes, and new command spelling. |
| cmd/cli/cmd_validate.go | Uses requirePositionals for validate. |
| cmd/cli/cmd_tree.go | Adds shared usage const, uses requirePositionals, and avoids printing value twice for array elements. |
| cmd/cli/cmd_stream.go | Uses requirePositionals for stream. |
| cmd/cli/cmd_page.go | Uses requirePositionals for page. |
| cmd/cli/cmd_object.go | Adds usage const and uses requirePositionals for object. |
| cmd/cli/cmd_metadata.go | Uses requirePositionals for metadata. |
| cmd/cli/cmd_font.go | Splits ref vs file checks and uses requirePositionals for font. |
| cmd/cli/cmd_embedded.go | Uses requirePositionals for embedded. |
| cmd/cli/cmd_docview.go | Updates doc-view parser docs and uses requirePositionals. |
| cmd/cli/cmd_diff.go | Uses requirePositionals for diff with 2 operands. |
| cmd/cli/cmd_bytes.go | Renames plaintext handler to bytes and ensures alias routes through canonical resource label. |
| cmd/cli/cmd_byref.go | Uses requirePositionals for by-ref commands. |
| CHANGELOG.md | Documents the rename, positional-arity behavior change, and scalar/diff behavior updates. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
unidoc-anom
left a comment
There was a problem hiding this comment.
Verification summary for this branch. Flagging up front that this is not an approval: I opened the PR, so this is a record of what was checked, and it still needs a human reviewer to satisfy branch protection.
Both Copilot comments are addressed in a0989b7 and their threads are resolved. Both were correct, and both were comment accuracy rather than behaviour:
internal/pdfcore/model.go-ValueRawhas noomitempty, so it serializes as"", while the comment said it "is omitted". The omission happens one layer up in the CLI'streeNodeOutput, which carriesomitemptyon both fields;TreeNodedeliberately does not, because the generated Wails binding types them as required. The comment now says the field stays empty and names where the key actually gets dropped.internal/pdfcore/tree.go- the comment claimed an array element'sLabelis "the only copy a reader sees", directly above the line that now populates the full copy inValue. Stale from before array elements carried a value.
CI: green on macos-latest, ubuntu-latest and windows-latest for the head commit.
Local gates: go vet clean, golangci-lint 0 issues, go test ./... passing, scripts/test-all.sh all suites, tsc and eslint clean, vitest 960 passing in 73 files, and wails3 generate bindings -clean=true producing no change.
What review found and what was done about it. Four passes over the full diff, each of which found something:
- The decode-collision fallback bypassed the binary carve-out when a signature dict was reached twice (routine:
/AcroForm/Fields/N/Vand the page/Annots/N/V). Both summaries matched as<binary, N bytes>, the fallback fired, anddiffCompareput the raw DER on the row, unclamped on both surfaces. Fixed and pinned with a fixture that reaches one signature dict twice. - The shared arity guard silently dropped the per-command empty-path check that eight subcommands had, so
dump tree ""went from a usage error to{"error":"file not found"}, contradictingdoc.go. Fixed; the guard now rejects an empty operand at every expected position. - The
CHANGELOGAddedbullet still described two rules this branch deleted, including one that would have told a readervalueRaw's absence means decoding was a no-op - the opposite of what the code does. - Array-element rows reported two different truncation counts for the same node, 80 in the row and 2000 in the tooltip, and showed 25x less than their dictionary siblings.
The fourth pass found no correctness defects, only display consistency, which is where the review stopped.
Where a reviewer's attention is worth spending, in order:
- The positional-arity guard is the real risk, not the rename. It changes exit codes and stderr for fifteen commands including
validateanddiff. Anything scripted that relied on a trailing flag being ignored now gets a usage error.dump metadataanddump embeddedwere previously documented as still dropping a trailing flag and no longer do. cmd/cli/doc.gonow makes detailed behavioural claims about all eleven dump subcommands. Two passes verified them against the built binary, but no test gates them, so they will drift the next time a subcommand changes how it reports an error.tests/shared-text-string-decoderhad a test pinningdump objectrendering/Titleand/UFas raw hex - the contract this work deliberately overturns. It was rewritten to assert the decoded row while keeping the recoverability guarantee on the--jsonrawfield. That is a contract change to a prior change's suite.
Known limits, deliberate:
- A signature
/Contentsor/Certarray reached as an indirect object cannot be carved out; theobj:G:Nnode ID has no room for the key. - An array element that is an indirect ref to a scalar keeps the backend's 80-rune label with no tooltip, since the walker does not dereference to fill
Value. - The removal version assumes this ships in 0.5.0. It appears in the notice constant,
docs/cli-usage.mdandCHANGELOG.md; a test keeps those three consistent but cannot know the ship version. scripts/verify-cli-output-parity.shstill reports differences against dev, from the deliberatevalue/valueRawadditions. Thedump bytesrow runs the new spelling against head and the old against the baseline so the same handler is compared.
One follow-up on merge, not in this diff: annotate the 2026-08-12 entry in the issues log rather than deleting it. That gap was closed by a decision, not by being filled.

Description and links
Two user-facing changes, one small and one that reaches the whole CLI surface.
dump plaintextreads like text extraction and is a byte dump. Its help string has always said "Dump document bytes as text", so the help was never wrong - only the command word was. The triggering case: pulling the "Terms of Use" section out of a manual to settle a redistribution question.dump plaintext <file>printed%PDF-1.4, the xref table and compressed stream data; piping it throughgrep -i "licen\|copyright"printed nothing. Nothing malfunctioned.pdftotext <file> -then produced 1343 lines with the section directly greppable.The larger change came out of reviewing the first one. Go's
flagpackage stops parsing at the first non-flag argument, so anything written after the file path arrived as a spare positional and was silently dropped:A caller piping that to
jqgets plain text and a success exit. It was live on every command that takes a file.dump byteswas the worst of them - the other document-level dumps degrade to readable text, while this one degrades to a raw binary document on a byte-exact stdout - butdump treeis the one people actually script.Jira: US-1820
Technical changes
requirePositionals(fs, want, usage)incmd/cli/main.go, applied at all 15 entry points. It rejects both a wrong positional count and a present-but-empty operand.wantis a parameter rather than a fixed 1 becausedifftakes two files, and the empty check runs at every position sodiff's second operand is covered. The helper does not choose the exit code: the dump subcommands return 1,validateanddiffreturn 2 to match their operational-error code.dump plaintextresource todump bytes, withplaintextkept as a help-hidden alias. The alias writes a one-line deprecation notice to stderr before falling through to the same handler, so piped stdout stays byte-exact. It is emitted in the dispatch arm before flag parsing, so it fires on usage errors too, and it names removal in 0.6.0.cmd_plaintext.gotocmd_bytes.goand its identifiers to match (git mv, so history follows).internal/pdfcore.GetPlainTextandinternal/pdfcore/plaintext.goare deliberately not renamed - that model is shared with the desktop Plain Text panel, and renaming it ripples into the service layer and the Wails bindings for no user-facing gain.cmd/cli/doc.go's stderr contract. It previously claimed "Errors are always JSON on stderr", which was already false before this branch:parseDocViewFlagswrites a bareUsage:line, and several subcommands report plain text at exit 2. It now states the rule the code follows - an argument shape error is plain text, a rejected flag value is JSON - and names the cases that cross that line in both directions.valueso--jsonis no longer limited to the clamped row label; achangeddiff node whose two sides decode alike falls back to byte-exact renderings instead of printing identical text twice, and that fallback keeps the<binary, N bytes>stand-in so a signature reached twice cannot put raw DER on the row;valueRawis emitted for every hex literal and escaped literal, so its absence means the display form is the form on disk.[truncated: N of M]marker shape as the CLI, counted the same way per unit. Array-element rows clamp at the render ceiling like their dictionary siblings rather than showing the backend's 80-rune label.Considerations
Page-text extraction was the alternative fix for the original complaint and was declined. Poppler has two decades of layout heuristics behind its space and line-break inference, which is the part that decides extracted-prose quality and the part we would start from zero on. UniPDF does extraction but is AGPL-or-commercial and this repo is public. Shelling out to poppler puts a hard external binary into an app that ships self-contained. Demand is a single case that the right tool answered in forty seconds. And reflowed page prose is a rendering output, against the structure-focused direction. The help now points at
pdftotextinstead, which is deliberate rather than an omission.The arity guard lives in a shared helper, so it changes behaviour for commands this PR's title does not mention. Fixing four of fifteen callers and documenting the asymmetry as intentional seemed worse than fixing all of them.
Keeping the alias rather than breaking the old spelling is the ordinary reason: a public repo at 0.4.0 should not silently break a documented command. The removal version assumes this ships in 0.5.0, giving one full minor of overlap.
Two things worth recording from review. The
doc.gostderr contract took three passes to get right - the first correction replaced a false blanket claim with precise claims that were still false for subcommands the author had not surveyed, and the rule only held up once someone tried to falsify it rather than confirm it. Separately, the shared arity guard silently dropped the per-command empty-path check that eight subcommands had, sodump tree ""went from a usage error to{"error":"file not found"}; the guard now rejects an empty operand explicitly.How was this tested?
go vet ./...clean,golangci-lint run0 issuesgo test ./...passing;scripts/test-all.shall suites green, covering every per-module suite undertests/npx tsc --noEmitclean,npx eslint .clean,npx vitest run960 passed in 73 fileswails3 generate bindings -clean=trueproduces no change, which is the check that the GUI contract is untouchedtests/cli-viewswere red against a vet-clean tree before any production code was writtentests/cli-views/positional_arity_test.gois table-driven over all 16 commands, covering a valid invocation, a flag after the file, an extra file, an empty operand,diff's two-file usage line, and a genuinely dash-leading path behind--requirePositionalsto always return true fails 32 subtests; removing the empty-operand loop fails 5; dropping--from the dash-path test fails 3; reverting the diff fallback puts the raw DER back on the row and fails both new signature casesdumpsubcommands plusvalidateanddifffor the trailing-flag, extra-file and empty-operand cases, and checked the alias on a clean run, a usage error, a missing file and an EPIPE. Confirmeddump bytesanddump plaintextproduce identical stdout, that the notice never reaches stdout, and that rendered--helpcontains zero occurrences of the literal tokenplaintextwhile keeping all three "plain text" two-word instancesWhat could go wrong?
The arity guard is the real risk here, not the rename. It changes exit codes and stderr for fifteen commands, including
validateanddiff, which this PR's title does not suggest. Anything scripted that relied on a trailing flag being ignored now gets a usage error instead of output. That is the intended fix, but it is a breaking change for a caller who had adapted to the old behaviour.dump metadataanddump embeddedwere previously called out indoc.goas still dropping a trailing flag; they no longer do. If anyone wrote a workaround against that documented gap, it is gone.The binary carve-out is a key-plus-context match, so it depends on signature dictionaries carrying
/Type /Sig,/Type /DocTimeStampor a/ByteRange. A signature dictionary typed with something else and no/ByteRangewould have its DER run through the text decoder. Nothing outside the signature family carries/ByteRange, so it cannot over-carve, but it can under-carve on a shape we have not seen. Separately, a signature/Contentsor/Certarray reached as an indirect object cannot be carved out at all - theobj:G:Nnode ID has no room for the key - so the DER decodes when that value is selected as an object in its own right.The decoder inherits pdfcpu's quirks and they are now visible on six surfaces instead of one. The non-UTF-16BE fallback is Latin-1 despite the function being named
CP1252ToUTF8, so a Word-authored curly apostrophe decodes to a C1 control rather than the character a reader expects - escaped rather than silently invisible, but still not the apostrophe.HexLiteralToStringruns escape processing over already-decoded bytes, so<415C42>givesAB, notA\B. Both are pinned by tests.scripts/verify-cli-output-parity.shstill reports differences againstorigin/dev, from the deliberatevalue/valueRawadditions and decoded strings. That is the tool working. Thedump bytesrow runs the new spelling against head and the old against the baseline so the same handler is compared with no spurious stderr difference.One narrow inconsistency left in place: an array element that is an indirect ref to a scalar keeps the backend's 80-rune label with no tooltip, where its inline siblings clamp at 2000. Closing it means dereferencing in the tree walker, which the design explicitly avoids.
Screenshots/videos (if appropriate)
The rename, and what the alias does:
The argument-shape fix, before and after:
And an empty operand, which names no file and is now a usage error rather than a runtime one:
Checklist
devbranch, notmaster