This is an automated email from the ASF dual-hosted git repository.
wenjin272 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/flink-agents.git
The following commit(s) were added to refs/heads/main by this push:
new a356606e [docs] Clarify review policy (#890)
a356606e is described below
commit a356606ec201295add500cb895698a66f9fa0f22
Author: Wenjin Xie <[email protected]>
AuthorDate: Fri Jul 10 16:43:01 2026 +0800
[docs] Clarify review policy (#890)
Co-authored-by: Codex <[email protected]>
---
AGENTS.md | 2 ++
code_review.md | 55 +++++++++++++++++++++++++++++++++++++++++--------------
2 files changed, 43 insertions(+), 14 deletions(-)
diff --git a/AGENTS.md b/AGENTS.md
index 43d1601a..d536f8d7 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -21,6 +21,8 @@ Java targets Java 11 and is formatted by Spotless using
google-java-format AOSP
Java tests use JUnit 5 with AssertJ and Mockito. Python tests use pytest;
integration tests are marked with `integration` and can require external
services such as Ollama, OpenAI, Chroma, or mem0. Before Python-facing tests,
including cross-language suites, export `PYTHONPATH=$(python3 -c 'import
sysconfig; print(sysconfig.get_paths()["purelib"])')` or the equivalent path
from the active Python environment. Prefer focused tests near the changed
module, then run `./tools/ut.sh` or the rel [...]
+- If verification may be using stale generated, built, or installed artifacts,
rebuild with `./tools/build.sh` or document an equivalent targeted rebuild
before attributing the failure to source changes.
+
## Commit & Pull Request Guidelines
Use concise subjects with bracketed components, matching existing history,
such as `[python] Admit bytes in memory values` or `[api][java] Add event
constants`. For nontrivial behavior changes, open or link a GitHub issue before
the PR. PR titles should include relevant components like `[api]`, `[runtime]`,
`[java]`, `[python]`, or `[hotfix]`; describe the change, test evidence, and
any compatibility impact.
diff --git a/code_review.md b/code_review.md
index 9f33b3ff..b99d41d1 100644
--- a/code_review.md
+++ b/code_review.md
@@ -14,29 +14,53 @@ design, clarify contracts, or prevent old bug paths from
remaining available.
Do not keep an API, fallback, or mutable state channel only for compatibility
when it conflicts with the new design.
+## Review Lenses
+
+Do not limit the review to bug-focused correctness checks. For non-trivial
+changes, review through at least these lenses:
+
+- Bug-focused review: check whether the change can fail at runtime, produce
+ incorrect behavior, violate compatibility, miss required tests, or break
+ documented contracts.
+- Design-focused review: check whether the new or changed design is simple,
+ coherent, maintainable, and aligned with module boundaries and user-facing
+ concepts.
+
+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.
+
## Required Review Passes
1. Understand the issue or feature goal.
-2. For GitHub PRs, read existing unresolved human review threads before
+2. Verify that the implementation matches the PR's stated goal. Trace the goal
+ to the changed code, tests, docs, and runtime behavior, and call out gaps
+ where the PR only partially addresses the goal or validates an adjacent
+ behavior instead of the intended contract.
+3. For GitHub PRs, read existing unresolved human review threads before
finalizing findings.
-3. Read the diff, then trace the full runtime call path outside the diff.
-4. Search all call sites of changed public, protected, and cross-language APIs.
-5. Decide whether changed APIs should be kept, documented, deprecated, or
+4. Read the diff, then trace the full runtime call path outside the diff.
+5. Search all call sites of changed public, protected, and cross-language APIs.
+6. Decide whether changed APIs should be kept, documented, deprecated, or
deleted.
-6. Check user-facing names, docs, and error messages for implementation leaks,
+7. Run a design-focused pass for non-trivial changes. Look for unclear API
+ shape, ambiguous contracts, unnecessary lifecycle state, duplicated
+ abstractions, implementation details leaking into user-facing names or docs,
+ avoidable fallback paths, and module-boundary violations.
+8. Check user-facing names, docs, and error messages for implementation leaks,
stale comments, or ambiguous contracts.
-7. Check whether the root cause channel still exists through old APIs,
+9. Check whether the root cause channel still exists through old APIs,
defaults, fallback behavior, or mutable shared state.
-8. Compare equivalent Java, Python, and YAML-facing behavior.
-9. Check cross-language wrappers, adapters, serializers, and runtime bridge
+10. Compare equivalent Java, Python, and YAML-facing behavior.
+11. Check cross-language wrappers, adapters, serializers, and runtime bridge
code when the change crosses Java/Python boundaries.
-10. Check dependency, license, NOTICE, and shared-fixture changes when new
+12. Check dependency, license, NOTICE, and shared-fixture changes when new
libraries or generated/bundled artifacts are introduced.
-11. Verify tests cover the behavior at the level where the bug happened.
-12. Run targeted tests or clearly state why they could not be run.
-13. Separate findings into blockers, non-blocking risks, and test gaps. Include
- relevant human-raised issues in the conclusion, marked as already raised by
- a human reviewer.
+13. Verify tests cover the behavior at the level where the bug happened.
+14. Run targeted tests or clearly state why they could not be run.
+15. Separate findings into blockers, non-blocking design or compatibility
risks,
+ and test gaps. Include relevant human-raised issues in the conclusion,
+ marked as already raised by a human reviewer.
## Root-Cause Checklist
@@ -70,6 +94,9 @@ For public APIs, configuration, and common runtime extension
points, ask:
Because the project is beta, reviewers should actively recommend deleting
unsafe or misleading APIs when doing so simplifies the contract.
+Do not preserve compatibility by default. Compatibility paths must have a
+concrete obligation; otherwise prefer removing or rejecting them, especially
+when they keep old bugs or ambiguous contracts alive.
When reviewing a public API change:
