From e6e3b307f8a0d40a29e79faf0dac9b8185e31106 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 11 Oct 2026 10:41:57 +0200 Subject: [PATCH] repair: say when a file had to be rebuilt to be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doc.Repaired reports whether any file this document's pages came from had to be rebuilt before it could be read, and `pdfops repair` writes the result out. -check answers the question without writing anything. ⛔ The rebuilding is not new and is not this verb's doing: go-pdfkit/reader repairs a damaged file on its own, quietly, which is the right behaviour for a reader — a caller asking for page three wants page three, not a lecture. What was missing was a way to SAY it happened and to keep the result. This closes BentoPDF's repair-pdf, which is a named entry point over exactly that. It reports on ANY source, not all. A merged document's pages come from several files, and one damaged source is the answer to "did this need repairing"; a true that depended on every source being broken would be false for every real merge. The test asserts both orders, because "any" is not "the first". ⛔ A POSITIVE control comes first: without it, "Repaired() is false" would pass on a fixture that was never broken and the whole test would measure nothing. The fixture helper fails loudly if the file it is handed has no startxref to break, which is the same guard from the other side. The scope is reader's and the README says so rather than implying QPDF's: what is recovered is a cross-reference table that is wrong, missing or points into the middle of something else — the commonest damage, and the kind where the objects are all still there. A file that did not need repairing is said out loud and is NOT an error, so that "repair everything in this directory" can be scripted. Both branches of the verb have their own save, so both have their own unwritable-output test: a healthy file and a damaged one take different paths and the first draft covered only one. 100% statement coverage on both packages with no uncovered statements, go vet and gofmt clean. Co-Authored-By: Claude Opus 5 --- README.md | 29 ++++++++++ cmd/pdfops/expose_test.go | 103 ++++++++++++++++++++++++++++++++++++ cmd/pdfops/run.go | 44 ++++++++++++++++ repaired.go | 30 +++++++++++ repaired_test.go | 108 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 314 insertions(+) create mode 100644 repaired.go create mode 100644 repaired_test.go diff --git a/README.md b/README.md index 0430ac2..011e29c 100644 --- a/README.md +++ b/README.md @@ -170,6 +170,35 @@ range, because the reversal absorbs the difference. It passed at the ceiling **and at the floor**, so it proved nothing; the negative control is what said so, and the assertion now uses `-keep-backs`, where the split is load-bearing. + +`repair` writes a file back out with its structure rebuilt, and says whether +rebuilding was needed. `-check` answers that question and writes nothing. + +```sh +pdfops repair -check suspect.pdf +pdfops repair broken.pdf fixed.pdf +``` + +⛔ **The rebuilding is not this verb's doing.** +[go-pdfkit/reader](https://github.com/go-pdfkit/reader) repairs a damaged file +on its own and quietly, which is the right behaviour for a reader — a caller +asking for page three wants page three, not a lecture. What was missing was a +way to **say** it happened and to keep the result, and that is the whole of +this verb. `Doc.Repaired()` is the same answer in Go. + +It reports on **any** source, not all: a merged document's pages come from +several files, and one damaged source is the answer to *"did this need +repairing"*. A true that depended on every source being broken would be false +for every real merge. + +⛔ And the scope is **reader's**, not QPDF's. What it recovers is a +cross-reference table that is wrong, missing, or points into the middle of +something else — the commonest damage, and the kind where the objects are all +still there. It is not a general salvage engine, and a file whose objects are +themselves truncated may still be unreadable. A file that did not need +repairing is **said out loud and is not an error**, so that "repair everything +in this directory" can be scripted. + `attachments` lists the files a document carries, and writes them out with `-to`. A name comes out of the document rather than from whoever runs the command, so it is stripped to its last element: `../escaped.txt` is written diff --git a/cmd/pdfops/expose_test.go b/cmd/pdfops/expose_test.go index b3c6459..a392286 100644 --- a/cmd/pdfops/expose_test.go +++ b/cmd/pdfops/expose_test.go @@ -1,9 +1,11 @@ package main import ( + "bytes" "fmt" "os" "path/filepath" + "regexp" "strconv" "strings" "testing" @@ -783,3 +785,104 @@ func writeDoc(t *testing.T, build func(*ops.Doc)) string { } func itoa(n int) string { return strconv.Itoa(n) } + +// repair writes a file back out with its structure rebuilt, and says whether +// rebuilding was needed. +// +// ⛔ Both answers are asserted, because a verb that said "rebuilt" whatever it +// was given would pass a test that only ever handed it a damaged file — and +// that is the direction this one would fail in, since the honest answer for a +// healthy file is the less interesting one. +func TestRepair(t *testing.T) { + dir := t.TempDir() + healthy, damaged := fixture(t, 2), damagedFixture(t) + + out := filepath.Join(dir, "healthy.pdf") + code, said, msg := exec("repair", healthy, out) + if code != 0 { + t.Fatalf("code %d: %s", code, msg) + } + if !strings.Contains(said, "nothing needed rebuilding") { + t.Errorf("a healthy file: said %q", said) + } + if got := len(contentsOf(t, out)); got != 2 { + t.Errorf("%d pages came out, want 2", got) + } + + out = filepath.Join(dir, "fixed.pdf") + code, said, msg = exec("repair", damaged, out) + if code != 0 { + t.Fatalf("code %d: %s", code, msg) + } + if !strings.Contains(said, "rebuilt") || strings.Contains(said, "nothing") { + t.Errorf("a damaged file: said %q", said) + } + if got := len(contentsOf(t, out)); got != 2 { + t.Errorf("%d pages came out of the repair, want 2", got) + } + // ⛔ And what came out is itself clean: `repair` on the result says there + // was nothing to do. A repair whose output still needs repairing is not + // one. + code, said, _ = exec("repair", "-check", out) + if code != 0 || !strings.Contains(said, "nothing needed rebuilding") { + t.Errorf("the repaired file still needs repairing: %q", said) + } +} + +func TestRepairCheck(t *testing.T) { + code, said, msg := exec("repair", "-check", damagedFixture(t)) + if code != 0 { + t.Fatalf("code %d: %s", code, msg) + } + if !strings.Contains(said, "rebuilt") { + t.Errorf("-check said %q", said) + } + // -check takes no output file, because it writes nothing. + if code, _, _ := exec("repair", "-check", damagedFixture(t), filepath.Join(t.TempDir(), "x.pdf")); code != 1 { + t.Error("-check accepted an output file") + } +} + +func TestRepairRefusals(t *testing.T) { + in := fixture(t, 1) + out := filepath.Join(t.TempDir(), "out.pdf") + if code, _, _ := exec("repair", in); code != 1 { + t.Error("no output file was accepted") + } + if code, _, _ := exec("repair", "/no/such/file.pdf", out); code != 1 { + t.Error("a missing file was accepted") + } + if code, _, _ := exec("repair", "-nonsense", in, out); code != 1 { + t.Error("an unknown flag was accepted") + } + // Both branches have their own save, so both need their own unwritable + // output: a healthy file and a damaged one take different paths through + // the verb and only one of them was covered by the first draft. + if code, _, _ := exec("repair", in, "/no/such/directory/out.pdf"); code != 1 { + t.Error("an unwritable output was accepted for a healthy file") + } + if code, _, _ := exec("repair", damagedFixture(t), "/no/such/directory/out.pdf"); code != 1 { + t.Error("an unwritable output was accepted for a damaged file") + } +} + +// damagedFixture points a file's startxref at the wrong place, which is the +// commonest way a PDF arrives damaged: the objects are all there and the table +// that says where they are is not. +func damagedFixture(t *testing.T) string { + t.Helper() + b, err := os.ReadFile(fixture(t, 2)) + if err != nil { + t.Fatal(err) + } + re := regexp.MustCompile(`startxref\s*\n\s*\d+`) + broken := re.ReplaceAll(b, []byte("startxref\n999999")) + if bytes.Equal(broken, b) { + t.Fatal("the fixture has no startxref to break, so this proves nothing") + } + path := filepath.Join(t.TempDir(), "damaged.pdf") + if err := os.WriteFile(path, broken, 0o644); err != nil { + t.Fatal(err) + } + return path +} diff --git a/cmd/pdfops/run.go b/cmd/pdfops/run.go index c545a59..67b34ae 100644 --- a/cmd/pdfops/run.go +++ b/cmd/pdfops/run.go @@ -61,6 +61,7 @@ var commands = []command{ {"info", "", "print what the file says about itself", runInfo}, {"metadata", "(-set … | -clear) ", "write what the file says about itself", runMetadata}, {"version", "-set ", "set the PDF version the file declares", runVersion}, + {"repair", "[-check] [out.pdf]", "rebuild a file whose structure is damaged, and say whether it was", runRepair}, {"strip", "[-annotations] [-bookmarks] ", "write the file without its metadata", runStrip}, {"sanitize", " ", "remove what runs rather than shows: scripts, launching, embedded files", runSanitize}, {"outline", "(-drop | -from ) ", "replace or remove the bookmarks", runOutline}, @@ -1722,3 +1723,46 @@ func runDeblank(c *context, args []string) error { fmt.Fprintln(c.out, "dropped", len(went), "page(s):", went) return nil } + +// runRepair writes a file back out with its structure rebuilt, and says +// whether rebuilding was needed. +// +// ⛔ The rebuilding is not this verb's doing: go-pdfkit/reader repairs a +// damaged file on its own, quietly, which is the right behaviour for a reader +// — a caller asking for page three wants page three. What was missing was a +// way to SAY it happened and to keep the result, which is the whole of this +// verb. It is also why -check exists: the question "is this file damaged" has +// an answer that is worth having without writing anything. +func runRepair(c *context, args []string) error { + fs := flags("repair") + check := fs.Bool("check", false, "say whether the file needed rebuilding and write nothing") + if err := fs.Parse(args); err != nil { + return err + } + want, usage := 2, " " + if *check { + want, usage = 1, "" + } + if err := wantArgs(fs, want, usage); err != nil { + return err + } + d, err := c.open(fs.Arg(0)) + if err != nil { + return err + } + if !d.Repaired() { + // ⛔ Said out loud, and not an error. A file that did not need + // repairing is the good case, and reporting it as a failure would + // make "repair everything in this directory" impossible to script. + fmt.Fprintln(c.out, "nothing needed rebuilding") + if *check { + return nil + } + return save(d, fs.Arg(1)) + } + fmt.Fprintln(c.out, "the structure was rebuilt to read it") + if *check { + return nil + } + return save(d, fs.Arg(1)) +} diff --git a/repaired.go b/repaired.go new file mode 100644 index 0000000..4976cf2 --- /dev/null +++ b/repaired.go @@ -0,0 +1,30 @@ +// Copyright (c) 2026, the go-pdfkit/ops authors +// All rights reserved. +// +// SPDX-License-Identifier: BSD-3-Clause + +package ops + +// Repaired says whether any file this document's pages came from had to be +// rebuilt before it could be read: a cross-reference table that did not match +// the objects, a trailer that was not there, an offset that pointed into the +// middle of something else. +// +// [reader] does the rebuilding on its own, and quietly, which is the right +// behaviour for a reader — a caller asking for page three wants page three, +// not a lecture. But it leaves nobody able to SAY it happened, and that is +// worth saying for two different reasons: a file that needed rebuilding should +// be written back out and kept in its rebuilt form, and a file that did not +// need it should not be presented as rescued. +// +// ⛔ ANY, not all. A merged document's pages come from several files, and one +// damaged source is the answer to "did this need repairing": a true that +// depended on every source being broken would be false for every real merge. +func (d *Doc) Repaired() bool { + for _, p := range d.pages { + if p.src != nil && p.src.Repaired() { + return true + } + } + return false +} diff --git a/repaired_test.go b/repaired_test.go new file mode 100644 index 0000000..ed5360a --- /dev/null +++ b/repaired_test.go @@ -0,0 +1,108 @@ +// Copyright (c) 2026, the go-pdfkit/ops authors +// All rights reserved. +// +// SPDX-License-Identifier: BSD-3-Clause + +package ops + +import ( + "bytes" + "regexp" + "testing" +) + +// brokenXref points a file's startxref at the wrong place, which is the +// commonest way a PDF arrives damaged: the objects are all there and the table +// that says where they are is not. +func brokenXref(t *testing.T, b []byte) []byte { + t.Helper() + re := regexp.MustCompile(`startxref\s*\n\s*\d+`) + out := re.ReplaceAll(b, []byte("startxref\n999999")) + if bytes.Equal(out, b) { + t.Fatal("the fixture has no startxref to break, so this proves nothing") + } + return out +} + +// ⛔ A POSITIVE control first. Without it, "Repaired() is false" would pass on +// a fixture that was never broken, and the whole test would be measuring +// nothing — which is what the brokenXref helper's own fatal guards against on +// the other side. +func TestRepairedIsFalseForAFileThatIsNotDamaged(t *testing.T) { + d, err := Open(simple(t, 2)) + if err != nil { + t.Fatal(err) + } + if d.Repaired() { + t.Error("a file that opens normally was reported as rebuilt") + } +} + +func TestRepairedIsTrueForAFileWhoseTableIsWrong(t *testing.T) { + d, err := Open(brokenXref(t, simple(t, 2))) + if err != nil { + t.Fatalf("a file with a wrong startxref could not be opened at all: %v", err) + } + if !d.Repaired() { + t.Error("a file whose cross-reference table was wrong was not reported as rebuilt") + } + // And it is really readable: a repair that loses the pages is not a repair. + if got := d.PageCount(); got != 2 { + t.Errorf("PageCount() = %d, want 2", got) + } + out, err := d.Bytes() + if err != nil { + t.Fatal(err) + } + // ⛔ What comes out must NOT itself need repairing. That is the whole point + // of writing it back: a rebuilt file kept in its rebuilt form. + back, err := Open(out) + if err != nil { + t.Fatal(err) + } + if back.Repaired() { + t.Error("what was written had to be rebuilt again to be read") + } +} + +// ⛔ ANY source, not all. A merged document's pages come from several files, +// and one damaged source is the answer to "did this need repairing" — a true +// that depended on every source being broken would be false for every real +// merge. +func TestOneDamagedSourceIsEnough(t *testing.T) { + good, err := Open(simple(t, 1)) + if err != nil { + t.Fatal(err) + } + bad, err := Open(brokenXref(t, simple(t, 1))) + if err != nil { + t.Fatal(err) + } + good.Append(bad) + if !good.Repaired() { + t.Error("a merge of a clean file and a damaged one was not reported as rebuilt") + } + // The other order too, because "any" is not "the first". + good2, err := Open(simple(t, 1)) + if err != nil { + t.Fatal(err) + } + bad2, err := Open(brokenXref(t, simple(t, 1))) + if err != nil { + t.Fatal(err) + } + bad2.Append(good2) + if !bad2.Repaired() { + t.Error("the damaged file first was not reported as rebuilt") + } +} + +// A document built here rather than borrowed has no source to have been +// repaired, and must not claim one. +func TestADocumentBuiltFromNothingWasNotRepaired(t *testing.T) { + d := New() + d.Blank(595, 842) + if d.Repaired() { + t.Error("a document with no source file was reported as rebuilt") + } +}