From f792850a35d4668ca88a6a798ebc7bf6270ee3bb Mon Sep 17 00:00:00 2001 From: weiqingy Date: Wed, 15 Jul 2026 23:04:54 -0700 Subject: [PATCH] [infra] Add change-type review guides with runtime/ template Add a change-type review guide index to code_review.md, and the first per-type guide as a template: runtime/ state and recovery. Each guide narrows the full review passes to the concerns that matter most for one change type and links examples from past reviews, so the detail loads on demand and the main guide stays concise. The runtime/ guide draws its two examples from real review threads (#874, #880). Remaining change types are listed as planned and will follow one small PR each once this shape is settled. Exempt review-guides/ from Apache RAT, matching the existing AGENTS.md and code_review.md exemptions. --- code_review.md | 15 ++++++++++++ review-guides/runtime-state-recovery.md | 31 +++++++++++++++++++++++++ tools/.rat-excludes | 3 ++- 3 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 review-guides/runtime-state-recovery.md diff --git a/code_review.md b/code_review.md index b99d41d17..f78e7bd6b 100644 --- a/code_review.md +++ b/code_review.md @@ -30,6 +30,21 @@ Keep these findings separate. A design-focused comment does not need to prove an immediate runtime bug, but it must explain the concrete complexity, ambiguity, or maintenance risk introduced by the change. +## Change-Type Review Guides + +For common change types, start from the focused guide below as an on-ramp, then +apply the full passes in this document. Each guide narrows the checklist to the +concerns that matter most for that area and links examples from past reviews. +Guides load on demand, so the general passes here stay short. + +| Change type | Focus | Guide | +|---|---|---| +| `runtime/` state and recovery | serde and replay type fidelity, real failure-path tests | [review-guides/runtime-state-recovery.md](review-guides/runtime-state-recovery.md) | +| Python-Java bridge | cross-language parity, type mapping across Pemja | planned | +| `api/` contract | API shape, compatibility policy, deprecation | planned | +| `dist` and dependency | shading, LICENSE and NOTICE, dist registration | planned | +| docs-only | facts match their source of truth | planned | + ## Required Review Passes 1. Understand the issue or feature goal. diff --git a/review-guides/runtime-state-recovery.md b/review-guides/runtime-state-recovery.md new file mode 100644 index 000000000..f03c53541 --- /dev/null +++ b/review-guides/runtime-state-recovery.md @@ -0,0 +1,31 @@ +# Review Guide: runtime/ State and Recovery + +Load this guide when a PR changes runtime state, checkpointing, serialization of +stored values, action-state handling, or async, mailbox, and recovery paths. It +narrows the full passes in `code_review.md` to the ones that matter most for this +area; the general passes still apply. + +## Focused checklist + +- Trace every stored value through its full serialize, checkpoint, and restore + round-trip. Confirm the restored value keeps its original type instead of a + lossy or re-encoded form, such as a `byte[]` that returns as a base64 `String`. +- Confirm a serde envelope or version tag scopes the value the way recovery + needs it, not only the way the happy-path write needs it. +- Under the beta policy, question serde or state compatibility flags that only + exist to read old formats. Prefer removing them unless they carry a concrete + obligation. +- For a bugfix, check the failure actually stops reaching the old path after the + fix, not just in the one code path the PR touched. +- Test the real runtime failure path through the operator, mailbox, or action + task, not a helper in isolation. Make failure-injection fixtures deterministic + so the regression test exercises the bug on every run. +- Cover async, retry, delayed-callback, and recovery paths when the change + affects them. + +## Examples from past reviews + +| Case | Pass it exercises | Review | +|---|---|---| +| A versioned serde envelope for stored `MemoryUpdate` values carried an `isEnvelope` backward-compatibility flag. The review asked whether that compatibility path was essential or should be dropped under the beta policy. | Whether a serde compatibility channel must exist, or only keeps an old format alive. | [#874](https://github.com/apache/flink-agents/pull/874#discussion_r3541046130) | +| A hotfix propagated a swallowed action-task `Error` out of the mailbox instead of losing it in a `Future`. The review kept the linkage-error regression fixture deterministic, since `System.nanoTime()` may return a value that skips the injected failure, so the test exercises the real failure path on every run. | Testing the actual runtime failure path with a deterministic fixture. | [#880](https://github.com/apache/flink-agents/pull/880#discussion_r3540383941) | diff --git a/tools/.rat-excludes b/tools/.rat-excludes index e58064b5f..ecc6d4b24 100644 --- a/tools/.rat-excludes +++ b/tools/.rat-excludes @@ -24,4 +24,5 @@ licenses/* skills/* .*\.yaml$ AGENTS.md -code_review.md \ No newline at end of file +code_review.md +review-guides/* \ No newline at end of file