Skip to content

fix(runtime): surface writeback no-receipt failures#244

Merged
khaliqgant merged 2 commits into
mainfrom
feat/writeback-normalized-wrapper
Jun 18, 2026
Merged

fix(runtime): surface writeback no-receipt failures#244
khaliqgant merged 2 commits into
mainfrom
feat/writeback-normalized-wrapper

Conversation

@khaliqgant

@khaliqgant khaliqgant commented Jun 18, 2026

Copy link
Copy Markdown
Member

Summary

  • wrap runtime writeJsonFile so missing Relayfile writeback receipts become first-class WritebackErrors with state: no_receipt
  • expose normalized writeback status helpers and WritebackError from runtime/client barrels
  • add runtime coverage for receipt success and no-receipt failure compatibility with WorkforceIntegrationError

Dependency

Verification

  • pnpm install --frozen-lockfile
  • pnpm --filter @agentworkforce/persona-kit build
  • pnpm --filter @agentworkforce/runtime test (94/94 pass)

Part of AgentWorkforce/cloud#2291 W6 follow-up.

@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@khaliqgant, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 37 minutes and 14 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c8958db4-1db9-47e6-bea8-18e747e22a14

📥 Commits

Reviewing files that changed from the base of the PR and between ac88488 and 672f3e0.

📒 Files selected for processing (4)
  • packages/runtime/CHANGELOG.md
  • packages/runtime/src/clients/index.test.ts
  • packages/runtime/src/clients/index.ts
  • packages/runtime/src/index.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/writeback-normalized-wrapper

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 and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 959bd0fca3

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +144 to +145
if (normalized.state !== 'succeeded') {
throw new WritebackError(normalized);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't throw for fire-and-forget writebacks

When a client intentionally sets writebackTimeoutMs: 0, adapter-core returns immediately with no receipt; mcp-workforce still relies on that mode in its GitHub tool test (packages/mcp-workforce/src/tools/integrations.test.ts:65-67) and calls this runtime helper from integrations.ts:84. This new branch normalizes that expected fire-and-forget result to no_receipt and throws, so integration.github.comment/createIssue/etc. can no longer write drafts in offline/fire-and-forget mode and the existing mcp-workforce test would fail. Preserve timeout-0 as a successful draft write or update the callers/config to stop using it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 672f3e0. The wrapper now preserves explicit writebackTimeoutMs: 0 fire-and-forget writes by returning the draft result when normalization reports no_receipt; timed waiters still throw WritebackError. Added regression coverage for both paths and verified @agentworkforce/runtime (95/95) plus @agentworkforce/mcp-workforce (25/25). The aggregate check got through lint/typecheck and later hit an unrelated timing flake in a deploy OAuth polling test; rerunning that deploy test passed.

@agent-relay-code

Copy link
Copy Markdown
Contributor

ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed.

The only production consumer is mcp-workforce/src/tools/integrations.ts (4 call sites, all github), which passes writebackTimeoutMs straight from config. Its handlers build return values via result.receipt?.id ?? '', designed to tolerate a no-receipt result. The PR's throw removes that tolerance, breaking the fire-and-forget contract documented in adapter-core.

My review is complete. Let me write up the findings.


Review Summary — PR #244 (feat/writeback-normalized-wrapper)

Purpose of PR: Wrap @relayfile/adapter-core's writeJsonFile in the runtime so a Relayfile writeback that returns no receipt becomes a first-class WritebackError, and export normalizeWritebackStatus / WritebackError / NormalizedWritebackStatus types.

CI verification (ran the canonical pipeline: install → build → lint → typecheck → test with node ≥22 + pnpm 10.17.1):

  • Build: PASS
  • Lint: PASS
  • Typecheck: PASS
  • Test: FAIL — 1 PR-caused regression + 1 unrelated environmental failure

Blocking finding (requires human judgment — left unchanged)

packages/runtime/src/clients/index.ts:135-148 — the new writeJsonFile throws WritebackError for any no-receipt result. But adapter-core documents writebackTimeoutMs: 0 as a legitimate fire-and-forget success path ("the client returns immediately without a receipt"). The wrapper conflates that intentional mode with genuine failure.

This breaks an existing, untouched package's test:

  • packages/mcp-workforce/src/tools/integrations.test.ts:55"dispatchIntegration writes a github comment draft under the Relayfile mount" now fails with WritebackError: writeback no_receipt .... The only production caller (packages/mcp-workforce/src/tools/integrations.ts:84,96,113,149) passes writebackTimeoutMs: 0 through and was built to tolerate a no-receipt result (result.receipt?.id ?? '').

I did not change this, because:

  • Making it skip the throw when writebackTimeoutMs === 0 is a fail-open/semantic decision (turning an error path back into a success path) that needs a human.
  • Editing/weakening the mcp-workforce test to make CI green would hide the regression.

Recommended human fix (not applied): Treat no_receipt as success when fire-and-forget is requested (writebackTimeoutMs === 0), or have callers opt into the strict-throw behavior. Then update the runtime contract docs and decide whether mcp-workforce's callers should propagate or swallow the error.

Non-blocking / environmental

  • packages/cli test fails with @relayburn/sdk: native binding not found for linux-x64 — a missing optional native dependency in this sandbox, unrelated to this PR; CI has the binding. Not a PR regression.

Addressed comments

  • No bot or human review comments were present in .workforce/context.json (no comments/reviews data), so there are no external findings to reconcile. The blocking regression above was discovered by running CI, not from a comment.

Advisory Notes

  • None. The blocking finding is within the PR's own scope (the new wrapper), not an unrelated cleanup.

No files were edited: the sole issue is a semantic/fail-open behavior change that must not be auto-fixed, and the only mechanical "fix" available (altering the existing test) would mask a real regression.

The PR has a failing required CI check (Test) caused by its own behavior change, so it is not ready to merge. I am not printing READY.

@khaliqgant khaliqgant merged commit c4fe7f1 into main Jun 18, 2026
3 checks passed
@khaliqgant khaliqgant deleted the feat/writeback-normalized-wrapper branch June 18, 2026 19:40
@agent-relay-code

Copy link
Copy Markdown
Contributor

ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed.

Let me analyze the 'ok' state subtlety carefully. When the real adapter exports normalizeWritebackStatus (future version), it could return state 'ok'. The runtime's writeJsonFile only treats 'succeeded' as non-throwing — 'ok' would trigger a throw. This is a forward-compatibility concern, but the adapter doesn't currently export normalizeWritebackStatus, so the fallback (which only ever returns 'succeeded' or 'no_receipt') is what runs today. This is a latent concern worth noting as advisory, not a current bug. I'll flag it as a review comment (judgment call, not auto-fixable).

I have enough to write the review. Let me verify there are no lint/format/typo issues I should mechanically fix in the diff. The diff is clean TypeScript that already compiles. Let me confirm there's no pre-existing changelog formatting issue or import-order issue.

The code is well-formatted and passes tsc. There are no mechanical fixes needed. Now let me write up the review.

Review Summary

I reviewed PR #244 (feat/writeback-normalized-wrapper) — making the runtime's writeJsonFile treat Relayfile writebacks that time out without receipts as first-class WritebackErrors.

What the PR does

The runtime previously re-exported writeJsonFile directly from @relayfile/adapter-core/vfs-client. It now wraps it: after the core write, it normalizes the result and throws WritebackError when no receipt is produced — except when writebackTimeoutMs === 0 (explicit fire-and-forget), which is preserved. It also adds normalizeWritebackStatus, a WritebackError class (with a fallback when the adapter doesn't export one), and the NormalizedWritebackStatus/State types, all re-exported from the package index.

Verification (ran the full canonical CI sequence)

  • pnpm -r build — passes (all 15 packages, including mcp-workforce which consumes writeJsonFile).
  • pnpm run typecheck incl. typecheck:examples — passes (covers review-agent, linear-shipper, notion-essay-pr, weekly-digest).
  • pnpm -r test — passes; runtime suite is 95/95 including the 3 new clients/index.test.ts tests.
  • Confirmed the installed adapter-core (0.3.59) does not export normalizeWritebackStatus/WritebackError, so the runtime's fallback paths are what execute today, and they are correctly typed via the optional-extension cast.

(An initial isolated runtime test showed 5 failures — these were purely a build-ordering artifact because persona-kit's dist wasn't built yet; they cleared once the workspace was built as CI does. Not PR-related.)

Findings

This is a deliberate behavior change (fail-loud), not a bug. Callers that don't set writebackTimeoutMs: 0 (e.g. mcp-workforce/src/tools/integrations.ts, examples/review-agent/agent.ts, examples/linear-shipper, notion-essay-pr, weekly-digest) will now throw on a writeback timeout instead of returning a result with the ?? result.path fallback. Those fallbacks become partially unreachable. This matches the PR's stated intent and changes a silent-degrade into a surfaced error (fail-closed direction — safe). I did not change any caller code; whether to add try/catch or set fire-and-forget at those call sites is a human/product decision and out of scope for this PR. Recorded below as advisory.

Did not auto-edit anything. The diff compiles, is well-formatted, and has no lint/typo/import-order issues to mechanically fix. The core logic change is semantic/safety-relevant, so per policy it gets review comments, not edits.

Addressed comments

  • No bot or human reviewer comments were present in the provided PR metadata (.workforce/context.json contains only diff/changed-files references; no review threads). Nothing to reconcile.

Advisory Notes

  • Forward-compat of the 'ok' state: NormalizedWritebackState includes 'ok', but writeJsonFile only treats 'succeeded' as success (clients/index.ts:144). If a future adapter-core exports a real normalizeWritebackStatus that returns 'ok', the runtime would throw on an otherwise-successful write. Today the fallback only ever returns 'succeeded'/'no_receipt', so this is latent, not live. Consider accepting both 'succeeded' and 'ok' as success — but that's a behavior decision for the author; left unchanged.
  • Caller migration: The new throwing semantics make the result.receipt?.x ?? result.path fallbacks in mcp-workforce/src/tools/integrations.ts and the example agents partially unreachable on timeout. If silent-degrade was intended at any of those sites, they'd need explicit writebackTimeoutMs: 0 or try/catch. This is unrelated to this PR's purpose (it doesn't touch those files) and should be a follow-up; left unchanged.

The PR is internally consistent, fully builds/typechecks/tests green across the monorepo, and moves writeback handling in the safer fail-loud direction. The remaining items above are author/product judgment calls, not blockers I can mechanically resolve. Since the decision on the 'ok'-state and caller-migration questions needs human judgment and all CI-equivalent checks pass, this is ready for a human to weigh in.

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