Repository navigation
docs(runtime): define safe recovery for PDF rejections - #4818
Conversation
Generated-by: OpenAI Codex Signed-off-by: seekskyworld <djh1813553759@gmail.com>
6709337 to
08dd40d
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 08dd40da713c69bc478640456a66d59e5007f1de (OPEN). Technical GO — one P3 below. Docs-only, no behavior change; it records an architecture decision, and whether to adopt that decision is a human call (no approve from me).
What was verified
All five checkable factual claims about current code state hold: the classifier's five diagnostic fields (provider-error-classification.ts:129-135), the per-wire parsed-error-shape table (matching the comments at :95-99 almost verbatim), no PDF-specific structured fields (only MIME-sniffing hits in attachments.ts), zero PDF retry/fallback hits across packages/runtime/src/, and the three linked issues existing in matching states (#3285, #3164, #3284 OPEN). Normative "admission criteria for future implementation" content cannot be falsified by code; only its statements about current state were checked. English-only is fine for docs/architecture/ (mixed conventions there, not counted as a defect).
P3 — new doc missing from the docs/README.md index
docs/README.md:59-70 maintains an architecture-doc list (several with bilingual links). This PR adds the file without an index entry — not a broken link (:71 has a Backend architecture chapters → ./architecture/ catch-all), just undiscoverable from the index. Not every file under the directory is indexed (soft convention); suggest one line.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移。当前 head 是 08dd40d,未关闭。五条事实全中,技术上无阻断问题,一条小的索引提醒。架构决定是否采纳由人类定。
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
|
Addressed the documentation follow-up in commit |
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Design doc concluding that Maka must not auto-retry a PDF-free request after a provider rejects a PDF input, with a five-fact admission contract for any future fallback (structured named rejection, pre-dispatch proof, independent attempt identity, per-request latch, bounded redacted explanation). The provider-evidence audit table (OpenAI Chat / Responses / Anthropic Messages all expose only generic code/type/message/HTTP status) matches the real boundary in provider-error-classification.ts, and the reasoning is first-principles sound: HTTP status and wording cannot prove no billable work or tool effect occurred, so inferring a PDF-specific rejection from them could duplicate work or corrupt durable history. Indexed in docs/README.md; test lane green.
Findings
No material findings. The doc correctly locates the only seam that could ever admit a fallback (the adapter owning the provider wire) and refuses to build it without evidence.
Verdict
merge-ready — well-grounded design conclusion with verification requirements for any future implementation.
|
This pull request has had no new commits for 30 days and has been marked stale. It will be closed in 7 days unless a new commit is pushed. Comments do not reset this timer: only a new commit does. If the pull request is intentionally long-lived, a maintainer can apply the |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
No P0–P2 issue was found at 4c50a5c2a4aa473aeee6bace07b49f9ec4bf36d1. This documentation-only change records a conservative future admission contract, not an implementation of PDF-free retry. The sole follow-up to the earlier reviewed head adds the missing docs index entry, fixing that P3. I rechecked the unchanged decision against the classifier, ModelAdapter, attachment handling and retry loop. The current generic error facts do not certify a PDF-specific pre-dispatch rejection; ordinary transient retry and context-overflow recovery must not be confused with removing PDF parts and retrying.
[P3] Name the existing test coverage accurately — docs/architecture/runtime-pdf-rejection-recovery.md:97–98. This head has attachment MIME tests and a steering test that preserves PDF attachment metadata but sends only an image file part to the provider. It does not have a native-PDF request/rejection suite. Replace the reference to existing native document-input tests with the actual attachment and generic provider-failure coverage, or link the intended test explicitly. The future fallback verification requirements remain appropriate; this wording should not suggest they are already covered.
All 76 selected existing tests passed (67 classifier/ModelAdapter, eight attachment tests and one steering attachment test), after a fresh dependency install, dependency patches and clean Core/Storage/MCP/Runtime builds. An independent execution checked 18 request/stream-error shapes through the actual adapter and diagnostic projection. A mock PDF request produced one ordinary terminal failure, retained its original file part and did not trigger an adapter-level PDF-free retry. These checks do not establish live-provider rejection timing or a future fallback's accounting, restart or redaction guarantees.
The head is unchanged, GitHub reports MERGEABLE, automatic merging with main bacb1caed is clean, and the exact-head test run 34082053905 succeeded. No protocol epoch or runtime behavior is changed.
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: the automated review of this exact head found no blocking (P0–P2) issues, and CI is green.
Summary
Refs #3285
Records the current runtime decision for provider PDF rejections: Maka does not perform a PDF-free retry until a provider adapter supplies structured, request-level evidence that proves the rejection occurred before provider work or tool effects. The document now includes the exact parsed error shapes accepted by the current classifier and the source paths audited.
Verification
git diff --checkpackages/runtime/src/provider-error-classification.ts,packages/runtime/src/model-adapter.ts, andpackages/core/src/attachments.tsAI use
Tool(s) and scope: OpenAI Codex authored the decision document and reviewed the linked runtime contracts.
Checklist
Does this PR entail a change in behavior?