fix(clearfolio): bound provider transport and JSON responses - #498
fix(clearfolio): bound provider transport and JSON responses#498seonghobae wants to merge 45 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
@opencode-agent Review exact current head |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
The current Clearfolio response boundary correctly bounds successful JSON bodies, but all three non-2xx paths return before consuming or cancelling the Undici-backed fetch body. Node/Undici explicitly requires fetch callers to consume or cancel response bodies rather than relying on GC, because unread bodies can reduce connection reuse and eventually stall or exhaust the pool. Fix this once at the provider-response boundary and cover submit/status/artifact rejection paths with cancellation-observable regressions; preserve the existing rule that rejected payload bytes are never parsed or surfaced.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head4f017140c1ab375e4f302bf2094a64e7b290d14b. -
Head SHA:
4f017140c1ab375e4f302bf2094a64e7b290d14b -
Workflow run: 31917786768
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (3 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (3 files)"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (3 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (3 files)"]
R3 --> V3["targeted test run"]
|
Rebuild the child from the current #493 parent tree, preserve only the bounded provider transport/JSON response slice, and retain protected adaptive-orchestrator attribution changes without widening the stack.
…arent Rebuild the artifact-origin child from the exact current #498 parent tree while preserving the child's bounded Clearfolio origin-policy delta. Shared package and changelog paths retain both the protected orchestrator attribution work and the child origin-policy coverage.
opencode-agent
Bot
added
priority: medium
|
Caution Review failedAn error occurred during the review process. Please try again later. 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. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| async function readBoundedJson(response, invalidMessage) { | ||
| const contentType = response?.headers?.get?.('content-type'); | ||
| if ( | ||
| typeof contentType !== 'string' | ||
| || contentType.split(';', 1)[0].trim().toLowerCase() !== 'application/json' | ||
| ) { | ||
| return rejectProviderResponse(response, invalidMessage); | ||
| } | ||
|
|
||
| const contentLength = response.headers.get('content-length'); | ||
| if (contentLength !== null) { | ||
| if (!/^\d+$/.test(contentLength)) return rejectProviderResponse(response, invalidMessage); | ||
| const declaredBytes = Number(contentLength); | ||
| if (!Number.isSafeInteger(declaredBytes) || declaredBytes > CLEARFOLIO_MAX_RESPONSE_BYTES) { | ||
| return rejectProviderResponse(response, invalidMessage); | ||
| } | ||
| } | ||
|
|
||
| if (!response.body || typeof response.body.getReader !== 'function') { | ||
| return rejectProviderResponse(response, invalidMessage); | ||
| } | ||
|
|
||
| const reader = response.body.getReader(); | ||
| const chunks = []; | ||
| let totalBytes = 0; | ||
| try { | ||
| for (;;) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
| if (!(value instanceof Uint8Array)) throw new Error(invalidMessage); | ||
| totalBytes += value.byteLength; | ||
| if (totalBytes > CLEARFOLIO_MAX_RESPONSE_BYTES) { | ||
| try { await reader.cancel(); } catch { /* validation remains authoritative */ } | ||
| throw new Error(invalidMessage); | ||
| } | ||
| chunks.push(value); | ||
| } | ||
| } catch (error) { | ||
| if (error?.message === invalidMessage) throw error; | ||
| throw new Error(invalidMessage); | ||
| } finally { | ||
| try { reader.releaseLock(); } catch { /* no observable effect */ } | ||
| } | ||
|
|
||
| if (totalBytes === 0) throw new Error(invalidMessage); | ||
| const bytes = new Uint8Array(totalBytes); | ||
| let offset = 0; | ||
| for (const chunk of chunks) { | ||
| bytes.set(chunk, offset); | ||
| offset += chunk.byteLength; | ||
| } | ||
|
|
||
| let text; | ||
| try { | ||
| text = new TextDecoder('utf-8', { fatal: true }).decode(bytes); | ||
| } catch { | ||
| throw new Error(invalidMessage); | ||
| } | ||
| try { | ||
| return JSON.parse(text); | ||
| } catch { | ||
| throw new Error(invalidMessage); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Byte ceiling correctly independent of Content-Length
readBoundedJson treats Content-Length as a hint only and independently counts streamed bytes, cancelling the reader past CLEARFOLIO_MAX_RESPONSE_BYTES. Dishonest or absent length headers cannot bypass the memory ceiling. Not a bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export async function submitJob(orgId, userId, document) { | ||
| const validatedDocument = validateDocument(document); | ||
| const configuration = clearfolioConfiguration(); |
There was a problem hiding this comment.
📝 Info: Validation precedence shifted ahead of config/mock
submitJob now validates the document before clearfolioConfiguration, and jobStatus/artifactUrl validate the job id first. Invalid input in unconfigured or mock mode now surfaces clearfolio document invalid/clearfolio job id invalid instead of the configuration error, and mock mode now validates documents it previously stored verbatim.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const category = ( | ||
| error?.name === ATTACHMENT_STATUS_TIMEOUT_ERROR | ||
| || error?.name === 'TimeoutError' | ||
| ) ? 'timeout' : failureCategory; |
There was a problem hiding this comment.
📝 Info: Timeout guard scoped to lookup phase
The timeout mapping in worker triggers only while failureCategory is downstream_lookup. A standard TimeoutError thrown during persistence or status validation keeps its original category. This matches the adapter, which can only emit its 15s TimeoutError during the fetch phase, and each failure still increments exactly one category.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function validateDocument(document) { | ||
| if (!isJsonRecord(document)) throw new Error('clearfolio document invalid'); | ||
| const { name, mime, bytes } = document; | ||
| if ( | ||
| typeof name !== 'string' | ||
| || name.trim().length === 0 | ||
| || name.length > MAX_DOCUMENT_NAME_LENGTH | ||
| || CONTROL_CHARACTER_PATTERN.test(name) | ||
| || typeof mime !== 'string' | ||
| || mime.length > MAX_MIME_LENGTH | ||
| || CONTROL_CHARACTER_PATTERN.test(mime) | ||
| || !(bytes instanceof Uint8Array) | ||
| || bytes.byteLength > MAX_DOCUMENT_BYTES | ||
| ) { | ||
| throw new Error('clearfolio document invalid'); | ||
| } | ||
| return { name, mime, bytes }; | ||
| } |
There was a problem hiding this comment.
📝 Info: Long or control-char filenames now rejected on upload
validateDocument rejects names over 512 chars, MIME over 255 chars, or control characters before transport. The upload path forwards file.name||'document' directly (server/app.mjs:1052), so an upload with such a filename that previously reached the provider now returns a 502. Intentional hardening, but a behavior change for edge-case filenames.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } catch { | ||
| throw new Error('clearfolio submit unavailable'); | ||
| } |
There was a problem hiding this comment.
📝 Info: Only jobStatus preserves timeout identity
submitJob and artifactUrl collapse all transport failures to generic errors, while jobStatus preserves the TimeoutError name. This asymmetry is consistent: only jobStatus feeds the refresh worker's timeout metric categorization; submit and artifact-link are not.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
📝 Info: Malformed stored job IDs counted as downstream lookup failures
validateJobId rejects a persisted job ID over 256 chars or with control characters before any request. The refresh worker only screens empty IDs as skipped, so such rows fall into the catch as a plain error and increment downstream_lookup instead of a data-quality category.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| } catch (error) { | ||
| if (request.signal.aborted && request.signal.reason?.name === 'TimeoutError') { | ||
| const unavailable = new Error('clearfolio status unavailable'); | ||
| unavailable.name = 'TimeoutError'; | ||
| throw unavailable; | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
📝 Info: Body-read timeout recovery relies on signal reason
When the 15s budget aborts an in-progress body read, readBoundedJson rethrows a fresh sanitized error, losing the TimeoutError name; the catch recovers timeout identity from request.signal.reason. A caller cancellation mid-body instead surfaces as the invalid-response error, but the worker's Promise.race discards that rejection, so refresh metrics are unaffected.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Buyer and security impact
A configured Clearfolio endpoint is still an untrusted external API. This bounded stacked slice prevents slow, redirecting, oversized, non-JSON, or rejected provider responses from consuming unbounded ScopeWeave time/memory or leaking tenant HMAC claims across redirects. It now also preserves timeout identity across the Clearfolio→attachment-refresh boundary so operator metrics distinguish provider deadline exhaustion from generic downstream lookup failure without exposing provider detail.
This is a follow-up slice of #489. It is stacked on #493 and does not close #489.
Exact current stack and scope
fix/clearfolio-production-configuration@78f8b557cd2b9cab238af72304f0ed42e1557759(fix(clearfolio): fail closed on production configuration #493);d60e281e7e2375207aa8b266f2938dfb51e65c2e;ahead_by: 39,behind_by: 0).Fresh parent→child comparison contains exactly ten child-owned files:
CHANGELOG.md;docs/deploy.md;docs/doctoring/clearfolio-provider-response-boundary.md;package.json;server/attachment_status.mjs;server/clearfolio.mjs;tests/unit/clearfolio-adapter-mock-hmac.test.mjs;tests/unit/clearfolio-provider-boundary.test.mjs;tests/unit/clearfolio-refresh-timeout.test.mjs;tests/unit/clearfolio-status-signal.test.mjs.No database, auth/session, dependency lockfile, workflow, scanner suppression, parent production-configuration doctoring, or branch-protection artifact is in the child semantic delta.
Request, resource, and timeout boundary
redirect: "error";TimeoutErroridentity is retained while its message is replaced with the fixed non-secretclearfolio status unavailablesurface;TimeoutErroras the fixedtimeoutmetric category;application/json;Review-driven timeout repair
Current review identified a real observability defect: when the Clearfolio adapter's 15-second internal budget fired before an attachment refresh timeout configured above 15 seconds,
jobStatus()collapsed the timeout to a generic error and the refresh worker counted it asdownstream_lookup.The repair was driven on the existing branch:
8faa7c279c71b5e591fe694a5152c338a2a2a5d4added a realistic provider-timeout regression before production changes. The repository's explicit test script did not yet execute that new file, so the old Server Tests success was correctly treated as a false-green test-registration gap rather than RED/GREEN evidence.6b5854836dc9852d511a5f8019e28d7e67753b61preserves the standardTimeoutErrorname while retaining the fixed sanitized status-unavailable message.35c8157b499c4df2c7d08d6b336d75cf43d4ea24maps standard downstream timeout identity to the existing low-cardinalitytimeoutmetrics bucket while preserving stale attachment state.d60e281e7e2375207aa8b266f2938dfb51e65c2ewires the new regression into bothtest:unitand the owned coverage case list so omission cannot silently green the contract again.The current
unit-and-apijob98315148974explicitly executedtests/unit/clearfolio-refresh-timeout.test.mjs; the regression passed and verified the sanitized error name/message, timeout metric increment, zero downstream-lookup increment, and stale-state preservation.Current evidence boundary
For current head
d60e281e7e2375207aa8b266f2938dfb51e65c2e:33010479245: GitHub-success;33010479942: GitHub-success;33010479277: GitHub-success;unit-and-apijob98315148974: success, including the new timeout regression and the existing Clearfolio/attachment suites;cloud-e2ein the same Server Tests run completed successfully.This Server Tests result is useful behavioral evidence, not exact-contributor-head merge authority: the job fetched and checked out synthetic merge
41bd9329f0b0d5244d12a6d0c87d401285e59956, loggingHEAD is now at 41bd932 Merge d60e281... into 78f8b557.... Repository exact-head checkout/coverage remains owned by #523, and centrally reusable SAST/Security exact-head evidence remains owner-controlled byContextualWisdomLab/.github#1222. Until those controls are protected-shipped and this unchanged branch is freshly re-evidenced, synthetic/absent/stale evidence remains non-authorizing.No current qualifying independent approval exists for
d60e281....Documentation inheritance
The child inherits #493's production-configuration authority: tenant-signed requests reject redirects and browser artifact authority is same-origin by default. This child adds bounded transport/resource and timeout-observability behavior; it does not weaken or supersede the parent configuration contract.
Remaining #489 work
This slice does not complete the Clearfolio production lifecycle. Dependent work still owns any explicit reviewed cross-origin artifact allowlist that is actually required, capability readiness, broader persistence/lifecycle controls, incident/recovery evidence, and protected integration.
Merge gate
Do not integrate this child before #493. After the parent reaches protected
develop, retarget or reconcile this bounded semantic diff against the resulting protected head and rerun all then-applicable gates. Merge or auto-merge only after the unchanged exact contributor head has terminal-passing exact-head CI/browser/owned coverage/docstring/security/dependency/supply-chain/package/provenance evidence, zero valid unresolved findings, and qualifying independent approval under live rulesets. Pending, queued, skipped-required, cancelled, absent, neutral-required, failed, stale, predecessor, synthetic-only, status-only, author-only, or model-only evidence is non-authorizing.