Skip to content

[codex] fix Synapse v1 follow-up review findings#19

Merged
snissn merged 1 commit into
mainfrom
codex/synapse-v1-review-fixes
Jun 12, 2026
Merged

[codex] fix Synapse v1 follow-up review findings#19
snissn merged 1 commit into
mainfrom
codex/synapse-v1-review-fixes

Conversation

@snissn

@snissn snissn commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up to the Synapse SDK v1 upgrade in #17. This PR fixes issues found in a deeper review of the v1 command mappings.

Findings Fixed

  • Corrects wallet summary so monthlyAccountRate is backed by Synapse v1's lockupRatePerMonth instead of the reserve amount totalRateBasedLockup.
  • Preserves the reserve value under a correctly named rateBasedLockup output field.
  • Exposes high-level upload completion state from UploadResult via requestedCopies, complete, and status: partially_uploaded when Synapse reports fewer committed copies than requested.
  • Serializes FailedAttempt.error directly for upload copy failures, avoiding [object Object] diagnostics when Synapse returns structured failed-attempt records.

Root Cause

The v1 upgrade adapted compile-time API changes but left two semantic mismatches: a lockup balance was still labeled as a monthly rate, and the high-level upload command discarded Synapse v1's explicit partial-completion signal.

Mergeability Evidence

  • Scope: cli/src/commands/upload.ts, cli/src/commands/wallet/summary.ts, and focused command tests.
  • Linked work: follow-up to Upgrade foc-cli to Synapse SDK v1 #17 / Synapse SDK v1 issue graph Upgrade foc-cli to Synapse SDK v1 #12-Run calibration smoke validation for Synapse SDK v1 foc-cli #16.
  • Internal deep review: no remaining correctness, conflict, or performance blockers found; this is user-facing command mapping and test coverage only, not a hot path or storage format change.
  • Latest head: ae22f5ba3e02e278d743c3638d751e96e6cd6688.
  • CI: GitHub Actions CLI passed on the latest head.
  • AI review: @codex review, @copilot review, and @coderabbitai review were requested after the PR was mature; no review comments or unresolved threads were returned before merge.
  • Known caveats: none.

Validation

  • bun test
  • bunx tsc --noEmit
  • bunx biome check src tests
  • bun run build
  • git diff --check

@snissn snissn marked this pull request as ready for review June 12, 2026 23:24
@snissn

snissn commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

@codex review

@snissn

snissn commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

@copilot review

@snissn

snissn commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@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: ae22f5ba3e

ℹ️ 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 +63 to +64
requestedCopies: 0,
complete: true,

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 Keep reset upload mock in sync

Because resetCommandMocks() is called at module load and before each test, the new successful-upload defaults here are overwritten by the reset implementation below, which still returns no requestedCopies or complete. Any test that relies on the shared upload mock after reset will now exercise undefined for those fields and report partially_uploaded, so update the reset mock to include the same fields.

Useful? React with 👍 / 👎.

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