Skip to content

fix(split): retry page writes regardless of path words in errors - #82

Merged
haywolf merged 1 commit into
masterfrom
fix/79-followup-retry-and-tests
Nov 4, 2026
Merged

haywolf merged 1 commit into
masterfrom
fix/79-followup-retry-and-tests

Conversation

@haywolf

@haywolf haywolf commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #79 review.

  • Write retry no longer depends on error text. withRetry takes an optional isRetryable (default: the existing PDF keyword check, so read/render paths are unchanged). Page writes pass () => true, because the error message embeds the output path, and a userData path containing password/encrypted made a transient EBUSY fail without retrying.
    • Not switched to an error.code whitelist: transient Windows EPERM/EACCES (antivirus locks) could be missed, and this keeps the change minimal. ENOENT retry behaviour is already pinned by an existing test.
  • Test for the "page has no image" branch: error names the page, and no files are written.

Tests: split vitest suites 116/116 pass; the 3 new regression tests fail with the fix reverted; npm run typecheck and npm run build pass.

Note (not changed here): the read path has the same keyword issue. An EBUSY on a file named e.g. password-list.pdf is reported as a protected PDF. Worth a separate fix.

Write retries reused the PDF keyword check, so an output path containing
'password' or 'encrypted' made a transient EBUSY fail at once. withRetry
now takes an optional isRetryable (default: the PDF check); page writes
retry on any error. Also cover the 'page has no image' branch.

Follow-up to #79.
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

AI Code Review Summary

PR: #82 (fix(split): retry page writes regardless of path words in errors)
Preferred language: English

Overall Assessment

No blocking issue was detected in the reviewed diff; keep focused regression testing before merge.

Major Findings by Severity

No major issues identified from the reviewed diff.

Actionable Suggestions

  • Address the highest severity findings first and add targeted tests for changed logic.

Potential Risks

  • Potential hidden risks remain in edge cases not covered by the current diff context.

Test Suggestions

  • Add happy-path + boundary + failure-path tests for touched modules.

File-Level Coverage Notes

  • src/core/infrastructure/adapters/split/PDFSplitter.ts: File was in scope but not fully reviewed before budget limits. (coverage_incomplete)
  • src/core/infrastructure/adapters/split/tests/PDFSplitter.test.ts: File was in scope but not fully reviewed before budget limits. (coverage_incomplete)

Inline Downgraded Items (processed but not inline)

  • None

Coverage Status

  • Target files: 2
  • Covered files: 0
  • Uncovered files: 2
  • No-patch/binary covered as file-level: 0
  • Findings with unknown confidence (N/A): 0

Uncovered list:

  • src/core/infrastructure/adapters/split/PDFSplitter.ts: coverage_incomplete
  • src/core/infrastructure/adapters/split/tests/PDFSplitter.test.ts: coverage_incomplete

No-patch covered list:

  • None

Runtime/Budget

  • Rounds used: 1/4
  • Planned batches: 1
  • Executed batches: 1
  • Sub-agent runs: 1
  • Planner calls: 2
  • Reviewer calls: 2
  • Model calls: 4/64
  • Structured-output summary-only degradation: YES

Reasons:

  • planner_structured_output_failed_round_1: Structured output failed after repair: You have no permission to access this resource (api_key_source: payg) (request_id: c452aa11587444cc8b4f7aa3126682da)
  • reviewer_structured_output_failed_round_1_general: Structured output failed after repair: You have no permission to access this resource (api_key_source: payg) (request_id: ee8bd806ae97409b9b66eae679bd9700)

@haywolf
haywolf merged commit 781139d into master Nov 4, 2026
3 checks passed
@haywolf
haywolf deleted the fix/79-followup-retry-and-tests branch November 4, 2026 04:43
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.

1 participant