docs: capture PR review learnings from #619 and #608 - #621
Conversation
Distill new, generalizable review findings from the 10 most recently merged PRs into docs/pr-review-learnings.md and mirror the categories in the AGENTS.md pointer. - §20: readers over persisted tool-output/message payloads must treat a newly-added field as absent-legacy, not malformed, or old rows regress (#619 approve_week_plan banner extractor). - §21: data-derived enforcement thresholds must exclude not-yet-synced observations (false zeros) and require the statistic actually demonstrate the gated effect (#608 personal MRV). - §19: extend the scheduling-idempotency entry with lease-expiry-by-phase and preflight TOCTOU reservation notes from #619. Other PRs in the window (#615 conventions, #616 already covered by §19, #611/#612/#609/#607/#603/#586) surfaced no new recurring gaps.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request updates review guidance with lease and reservation checks, legacy persisted-payload compatibility, data-derived enforcement thresholds, and Convex runtime-boundary topics. It also updates the review date and related checklists. ChangesReview learning guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
✨ 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 |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f70042622
ℹ️ 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".
| `qualified_for_enforcement` — i.e. the estimate becomes a hard programming gate, | ||
| not just advice. Two ways the estimate qualified on data that didn't support it: |
There was a problem hiding this comment.
In the current tree, qualified_for_enforcement does not become a hard programming gate: the estimate is only exposed through analyze_volume_strength, whose description says it is merely eligible for a separate backend gate (convex/ai/volumeStrengthTool.ts:129), while computeWeeklyVolume continues to apply the static VOLUME_LANDMARKS limits (convex/coach/periodization.ts:230-239). Describing this as an existing constraint that blocks or caps users' programming misstates the reviewed behavior and can misdirect future reviewers; frame it as a risk for any future enforcing consumer unless such a gate is added.
Useful? React with 👍 / 👎.
2 participants
Summary
docs/pr-review-learnings.md.Changes
docs/pr-review-learnings.mdschedulingFailed/deferred) to theapprove_week_planbanner extractor's shape check retroactively marked legacy stored tool outputs as malformed, re-rendering confirmed successes as "This change could not be confirmed." Preventive check: treat an absent newly-added field as legacy default, keep rejecting present-but-malformed values, and regression-test a pre-migration payload.performanceSyncComplete) and qualified on a statistic that a general downward trend also satisfies. Preventive check: exclude incomplete/unsynced units, require the statistic to actually demonstrate the gated effect, and default to advisory.reconciling/post_authorizedmay already have created a tile); and reserve the target atomically to close the preflight→external-DELETE TOCTOU window.Last revieweddate.AGENTS.md— extended the "Learning From Past Reviews" category pointer with the three new trigger areas (lease/claim expiry-by-phase, persisted-payload readers, data-derived enforcement thresholds).Other PRs in the window surfaced no new recurring gaps: #615 (one-component-per-file + title-template) was convention/stylistic; #616's scheduling-idempotency findings are already captured in §19; #611/#612/#609/#607/#603/#586 had no review threads.
Checklist
npx prettier --checkpasses on the edited filestype: description)Test Plan
Documentation-only. Verified
npx prettier --check docs/pr-review-learnings.md AGENTS.mdreports clean, and that section headers renumber contiguously through §21.Generated by Claude Code
Summary by CodeRabbit