docs: clarify success envelope contract — judge success by ok, not code#1730
Conversation
📝 WalkthroughWalkthroughDocumentation updates define the JSON output contract for ChangesJSON output contract documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 240-246: The README JSON example is using a plain json fence even
though the snippet contains // annotations, which makes the sample invalid JSON.
Update the documentation block to use a jsonc fence or move the explanatory
comments outside the fenced block, keeping the success/error examples associated
with the contract while making the sample copy-paste valid.
In `@README.zh.md`:
- Around line 241-247: The README.zh.md example block is labeled as JSON but
contains inline `//` annotations, which makes it invalid JSON. Update the
documentation sample around the JSON response examples to use a `jsonc` fence or
move the explanatory comments outside the fenced block, keeping the `ok`,
`identity`, `data`, and `error` examples valid and copy-pasteable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25e3b6ee-d105-43ec-ae9f-9cbbcf11efd6
📒 Files selected for processing (5)
README.mdREADME.zh.mderrs/ERROR_CONTRACT.mdskills/lark-shared/SKILL.mdskills/lark-task/references/lark-task-create.md
The stdout success envelope is {ok, identity, data, meta?} and carries no
top-level code/msg field, but wrappers following the raw OpenAPI convention
test code == 0 and misclassify every successful call as a failure. Around
write commands this defeats caller-side idempotency and causes duplicate
creates (see larksuite#1689).
Document the contract in ERROR_CONTRACT.md (success-envelope counterpart to
the error wire format), both READMEs (JSON Output Contract section), the
lark-shared skill (shared prerequisite for all skills), and add a success
response example to lark-task-create.md.
Closes larksuite#1689
d46e961 to
54c844c
Compare
|
Both findings were already addressed in 54c844c (the current head, force-pushed shortly after the review ran on d46e961): the @coderabbitai resolve |
|
✅ Action performedComments resolved. Approval is disabled; enable |
PR Quality SummaryCI did not complete successfully. Use the failed check links below to decide whether this PR needs a code change or a rerun. Failed checksdeterministic-gate
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@54c844c29f301e879c73ca9995e1369045ecead6🧩 Skill updatenpx skills add spacegeologist/cli#docs/success-envelope-contract -y -g |
Summary
The stdout success envelope is
{ok, identity, data, meta?}and carries no top-levelcode/msgfield, but the docs never state this. Wrappers following the raw OpenAPI convention ({"code": 0, "msg": "ok"}) testcode == 0, misclassify every successful call as a failure, and around write commands this defeats caller-side idempotency and causes duplicate creates — exactly the incident described in #1689.Changes
errs/ERROR_CONTRACT.md: add a Success envelope (stdout) subsection next to the error wire format, stating the shape, thatcodeexists only insideerror(invariant 4), and whycode == 0checks are unsafe.README.md/README.zh.md: add a JSON Output Contract section under Advanced Usage showing both envelopes side by side and theok == truerule.skills/lark-shared/SKILL.md: add a JSON 输出契约 section, since every skill reference lists lark-shared as a prerequisite — this covers all write-type skills in one place.skills/lark-task/references/lark-task-create.md: add a concrete success response example (data.guid/data.url, matchingshortcuts/task/shortcuts.goCreateTask output) and change the "Report the result" step to judge success byok.Docs-only; no behavior change. The issue's optional code-side suggestion (adding
code: 0orstatusto the success envelope) is left out deliberately — the maintainer reply on #1689 scoped this to documentation.Test Plan
make unit-testunaffected (no Go changes)internal/output/envelope.go(Envelopestruct) andshortcuts/task/shortcuts.go(CreateTaskoutputs{guid, url})Related Issues
Summary by CodeRabbit
ok == true(and/or exit code0), warning thatcode == 0is unreliable.errorand its upstream numericcodeappear, and linked to the full error taxonomy.ok == trueand added a success JSON example.