Skip to content

fix(odf): fail validation when package parts are unreadable - #590

Merged
productdevbook merged 2 commits into
productdevbook:mainfrom
sean0407:codex/odf-validator-missing-parts
Oct 14, 2026
Merged

productdevbook merged 2 commits into
productdevbook:mainfrom
sean0407:codex/odf-validator-missing-parts

Conversation

@sean0407

@sean0407 sean0407 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

- failed or missing ZIP parts are printed as absent and skipped
- an omitted manifest schema is not called out
+ required-part extraction errors count as failures
+ skipped manifest checks are identified in the output

Closes #566. The current catch-and-continue path can print “All parts valid.” and exit 0 after checking no required part.

Evidence

Merge Danger

Door: two-way

Blast Radius: Low

A failed extraction now makes the validation command exit non-zero instead of reporting a false pass.

Summary by CodeRabbit

  • Bug Fixes
    • ODF validation now reports package parts that cannot be read as failures instead of skipping them, and distinguishes invalid or unreadable parts from valid documents with skipped manifest checks.
  • New Features
    • When no manifest schema is provided, validation reports the manifest check as skipped.
    • The validator can load its bundled ZIP reader from an explicitly specified distribution path.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T07:09:18.899455Z 91d3fd7 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a309130d-511a-4de4-9b5e-d0f6620a953b
📥 Commits

Reviewing files that changed from the base of the PR and between 91d3fd7 and 91c7b9c.

📒 Files selected for processing: 4
  • scripts/validate-odf.mjs
  • test/odf-validation.test.ts
  • tsconfig.cli.json
  • tsconfig.json

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

📝 Walkthrough

The ODF validator now resolves an explicit distribution bundle, counts unreadable package parts as failures, reports skipped manifest checks, and distinguishes these outcomes in its final status. CLI tests cover missing parts, unreadable packages, and omitted manifest schemas.

Changes: ODF validation

Layer / File(s) Summary
Bundle resolution
scripts/validate-odf.mjs
The validator resolves an explicit --dist path to a file URL and locates the ZIP reader relative to the selected bundle.
Validation reporting and tests
scripts/validate-odf.mjs, test/odf-validation.test.ts, tsconfig.cli.json, tsconfig.json
Unreadable parts now count as failures. The validator reports skipped manifest checks and selects a final status based on failures and skipped checks. CLI tests cover missing and unreadable parts and skipped manifest checks.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: productdevbook

Merge Risk: ⚪ Minimal · up to 91c7b

The validator now fails when package parts are unreadable and reports skipped manifest checks. No actionable merge-blocking risk was found in the supplied context.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: unreadable ODF package parts now cause validation to fail.
Linked Issues check Passed The changes satisfy the coding requirements in directly linked issue #566. Required-part extraction errors now increment the failure count and produce missing or unreadable output. The validator exits…
Out of Scope Changes check Passed The reported changes stay within issue #566. The --dist ZIP-reader resolution supports the regression test and the affected extraction path. The TypeScript configuration changes include the new test…
Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

`validateOdfParts` had a single caller: the test drives the CLI through
`spawnSync` and never imports the module, so the extraction bought
indirection — eight injected parameters, including `log` — and nothing
else. The counting change is back in the loop it belongs to.

The test was a `.mjs` file, which no tsconfig project looks at, so it
sat outside `bun run test`'s typecheck. It is `.test.ts` now, listed
where CONTRIBUTING says a test needing node builtins goes.

And `extract` resolves the ZIP reader relative to `--dist` instead of
whatever sits in ../dist, which is what makes the stub bundle force the
`unzip` fallback: the missing-part run can now assert that the three
parts which *did* come out are still reported valid.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@productdevbook

Copy link
Copy Markdown
Owner

Taking this with one commit on top. The behaviour is right and matches #566: a part that will not come out of the package is a failed check, and a manifest check nobody ran says so.

Two things changed:

  • validateOdfParts had a single caller. The test drives the CLI through spawnSync and never imports the module, so the extraction bought eight injected parameters (including log) and no second caller — the counting is back in the loop in validate-odf.mjs.
  • test/odf-validation.test.mjs was outside the gate: no tsconfig project looks at .mjs, so bun run test never typechecked it. It is test/odf-validation.test.ts now and listed in tsconfig.cli.json / tsconfig.json, which is where CONTRIBUTING puts a test that needs node builtins.

While there: extract resolved the ZIP reader from a hardcoded ../dist/zip/reader.mjs regardless of --dist, so with a built dist/ present your stub bundle did not actually force the unzip fallback and every part failed for the wrong reason. It resolves relative to --dist now, which lets the missing-part case assert that content.xml, styles.xml and meta.xml are still reported valid. All three tests still fail against main.

@productdevbook
productdevbook merged commit c991ad0 into productdevbook:main Oct 14, 2026
7 of 13 checks passed
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.

ODF validator reports "All parts valid." when parts are missing, the ZIP can't be read, or the manifest check is skipped

2 participants