docs(#669): verify API contracts per code path in code skill#1231
docs(#669): verify API contracts per code path in code skill#1231ifireball wants to merge 2 commits into
Conversation
Add step 8 planning guidance so the code agent checks external API requirements for each code path before removing or emptying parameters. Regression test locks the guidance into the scaffold. Closes fullsend-ai#669 Co-authored-by: Cursor <[email protected]>
Site previewPreview: https://35169dc3-site.fullsend-ai.workers.dev Commit: |
ifireball
changed the title
ReviewFindingsNo findings. Previous runReviewFindingsNo findings. Previous run (2)ReviewFindingsNo findings. Previous run (3)ReviewFindingsNo findings. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
ready-for-merge
Co-authored-by: Cursor <[email protected]>
fullsend-ai-review
Bot
added
ready-for-merge
ifireball
changed the title
fullsend-ai-review
Bot
added
ready-for-merge
ifireball
force-pushed
the
agent/669-api-contract-guidance
branch
from
4bdfb25 to
96decb3
Compare
ifireball
changed the title
fullsend-ai-review
Bot
added
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad (10 agents) — 2 findings requiring changes:
- CRITICAL — Merge conflict: main already has step 8 ("Search for old literal values"). Rebase needed, renumber to step 9.
- MEDIUM — Guidance scope ("removes or empties") is narrower than the bug class and the code-review skill's equivalent wording ("modifies"). Consider broadening.
3 additional LOW findings (test substring assertions, "or" ambiguity, empty retrigger commit) noted but not blocking.
| 8. **Verify API contracts per code path** — if the fix removes or empties | ||
| a parameter sent to an external API, check the API documentation or | ||
| test each code path that uses the function. Different operations | ||
| (e.g., approve vs request-changes) often have different required fields. |
There was a problem hiding this comment.
Merge conflict — duplicate step 8 numbering
The PR branch predates commit 4f39be1 on main which added step 8 ("Search for old literal values when changing constants or defaults") at this same position. The PR is currently in a CONFLICTING merge state with two items numbered 8.
A rebase onto current main is needed, and this item should be renumbered to step 9. The ordering (risk assessment → literal value search → API contract verification) reads naturally.
Flagged by 4/10 review agents
| changed behavior needs updated tests. | ||
| 7. **Assess risk** — will this change affect other callers? Does it change a | ||
| public interface? Could it break downstream consumers? | ||
| 8. **Verify API contracts per code path** — if the fix removes or empties |
There was a problem hiding this comment.
Guidance scope narrower than the bug class it targets
The trigger "removes or empties" covers the PR #657 incident but the broader class of API contract violations also includes changing a parameter value or adding a default that is invalid for some code paths.
The code-review skill's equivalent guidance (line 65) already uses the broader "modifies parameters" wording. Consider aligning:
| 8. **Verify API contracts per code path** — if the fix removes or empties | |
| 8. **Verify API contracts per code path** — if the fix removes, empties, | |
| or changes a parameter sent to an external API, check the API documentation | |
| and trace each call site to verify the parameter is valid for that operation. | |
| Different operations (e.g., approve vs request-changes) often have different | |
| required fields. |
Flagged by 5/10 review agents
3 participants
Summary
TestCodeImplementationSkillAPIContractGuidanceto lock the guidance into the scaffold.Context
Retro from PR #657: the code agent removed
commentURLand used empty review bodies for all GitHub review event types. That works for APPROVE but breaks REQUEST_CHANGES and COMMENT (422). ThesubmitFormalReviewbug is already fixed inpostreview.go; this PR addresses the process gap.Test plan
go test ./internal/scaffold/... -run TestCodeImplementationSkillAPIContractGuidancemake lintCloses #669