Repository navigation
[infra] Follow-up improvements to AGENTS.md and code_review.md #894
Description
Background
AGENTS.md and code_review.md were introduced in #878 and already iterated on in #890. Both PRs frame the guides as living documents meant to be refined continuously. This issue tracks a few concrete, self-contained follow-ups, so they can be discussed and picked up as small independent PRs.
I'm happy to take these on if a committer is supportive. Splitting into the two well-scoped changes below to keep each PR small and reviewable.
Proposed changes (high confidence, ready to implement)
1. Freshness check for the version facts in AGENTS.md
AGENTS.md hard-codes facts that can silently drift from their real source: Python 3.10 through 3.12, Ruff line length 88, Java 11. This is not hypothetical — the source of truth for these already lives in python/pyproject.toml and the root pom.xml, and comparable version strings elsewhere in the repo have drifted before.
Proposal: a small script under tools/, wired into the existing lint/CI lane, that asserts the versions quoted in AGENTS.md match python/pyproject.toml and pom.xml. This makes the guide un-rottable and is mechanical to review.
Shape: one [ci] / [infra] PR.
2. Worked-examples appendix for code_review.md
#890 added the "Review Lenses" section and a dedicated design-focused pass, but the passes are still described in the abstract. A short appendix that maps a few already-merged bugs to the exact pass that would catch them would make the checklist more tangible without changing any of its policy.
Candidate cases, all from merged PRs in this repo:
- Durable-serde type loss where a
byte[]value returned as a base64Stringafter replay ([runtime] Binary serde for MemoryUpdate values via versioned Kryo envelope #874) — illustrates "does the root cause still exist through the serialization path?". - A compatibility fallback that could fail during Pemja init, removed in favor of explicit config ([python] Remove PyFlink gateway fallback from AgentConfigOptions #867) — illustrates the beta "don't preserve compatibility by default" policy.
- A memory-value contract that rejected a safe
bytestype ([python] Admit bytes in the Python memory value contract #846) — illustrates the Java/Python parity and reject-at-boundary passes. - An
Errorswallowed in aFutureinstead of failing the task ([hotfix] Propagate action task Errors from mailbox #880) — illustrates "test the actual runtime path where the bug occurred".
Each case would be 3-5 lines: the symptom, the pass/lens that catches it, and the fix pattern.
Shape: one docs-only PR appended to code_review.md.
Items worth discussing before implementing
These are more opinionated, so raising them here first rather than opening a PR:
- A change-type review matrix (e.g.
api/contract changes,runtime/state or recovery changes, Python-Java bridge changes,dist/dependency changes, docs-only changes) mapping each PR type to the passes that matter most, as an on-ramp into the existing full checklist. - Whether module-scoped
AGENTS.mdfiles (e.g. underpython/,runtime/python/,dist/) would help, given how differently those sub-domains behave. - Whether
CLAUDE.mdandAGENTS.mdshould converge toward one source of truth with a thin pointer from the other, to avoid the two guides drifting.
Feedback on scope and priority welcome.
Activity
weiqingy commentedon Jul 10, 2026
@wenjin272 — following up on the AGENTS.md / code_review.md iteration you've been driving (#878, #890). I've drafted the two follow-ups above and verified both locally. Before opening PRs I wanted to check the direction is welcome, per the issue-first guideline.
1. Freshness check — a small dependency-free
tools/check-agents-md.pywired into thelintCI job. It derives the Python range, Ruff line length, and Java version frompython/pyproject.tomlandpom.xmland assertsAGENTS.mdstill quotes them, so the guide can't silently drift. Verified green on currentmain, and red when I simulated a Python-3.11 or Java-17 bump.2. Worked-examples appendix — a short
## Worked Examplessection incode_review.mdmapping four merged bugs (#874, #867, #846, #880) to the specific passes that surface them, to ground the checklist in concrete cases.Happy to open these as PRs once we're aligned on the direction, or to adjust scope/shape first if you'd prefer.
wenjin272 commentedon Jul 13, 2026
@weiqingy Overall, I agree with your point of view.
AGENTS.md.code_review.mdconcise to avoid consuming excessive context window space.Regarding other items worth discussing:
This seems very meaningful as it makes the injected context more focused and concise.
This feels reasonable personally, but we might want to observe if other repos have similar practices.
Could we use a symlink to point CLAUDE.md directly to AGENTS.md?
weiqingy commentedon Jul 14, 2026
Thanks for the review @wenjin272 !
1. Freshness check
Opening this as a PR now. I rebased it onto current
mainand re-verified: green today, and it fails correctly when I simulate each of the three bumps independently (Python upper bound, Ruff line length, Java version), so it isn't passing vacuously.2. Worked examples — both concerns land
On conciseness: my draft appended 36 lines of prose to a 175-line file — roughly 20% growth on a file whose entire job is to be injected into a context window. That's the wrong trade, and I've reshaped it into a table, one row per case (~6 lines instead of 36).
On representativeness: rather than argue for my picks, I tried to falsify them by mapping the four cases onto the change-type axis from your next point. They only cover two of the five buckets —
runtime/state and recovery (#874, #880) and the Python-Java bridge (#846, #867), with nothing forapi/contract changes,dist/dependency changes, or docs-only changes. So they are not representative: they cluster where I happen to have been reading recently. The fix is a selection rule rather than my taste — one case per change type, drawn from merged PRs.Which suggests folding this item and the matrix into a single change. You liked the matrix because it makes the injected context more focused; worked examples as a separate section cut directly against that. But as the evidence column of the matrix, they cost almost nothing:
runtime/state & recoveryapi/contractdist/ dependencyOne table instead of two sections: each pass earns its place by pointing at a bug it actually caught, the doc gets shorter and more concrete at the same time, and the gaps are visible rather than hidden. If you would rather keep them separate I am happy to do that — but this way the conciseness concern and the matrix pull in the same direction instead of against each other.
3. Module-scoped
AGENTS.md— Flink itself is the prior artThe standard is explicit that nesting is the intended monorepo pattern: "Place another AGENTS.md inside each package. Agents automatically read the nearest file in the directory tree, so the closest one takes precedence and every subproject can ship tailored instructions" (https://agents.md/).
More usefully, the practice is already established in our own neighbourhood:
flink-table/flink-table-planner/AGENTS.mdandflink-table/flink-table-runtime/AGENTS.mdalongside the root one.providers/,dev/, the UI, and the task SDK.The part that speaks to your context-window concern: nested guides are pulled in on demand when an agent touches files in that subtree, not at launch. So module-scoped files don't add to the always-on context — they subtract from it. That's the same property you liked in the matrix, which is a good sign the two ideas are pointing the same way.
One wrinkle worth knowing before we commit to it: tools disagree on how they resolve "nearest". Copilot and Cursor key off the file being edited, so
python/AGENTS.mdapplies wheneverpython/**is touched. Codex and Gemini CLI instead resolve relative to the session's working directory, so a nested file only applies if the session was started inside that directory.apache/flinkhedges against this by having its rootAGENTS.mdlink to the module files explicitly, so tools that only read the root still find them. If we do this, that seems worth copying.4.
CLAUDE.mdandAGENTS.md— a correction to my own framing, then yes to the symlinkI raised this as "avoid the two guides drifting", but that premise was wrong: there is no
CLAUDE.mdin this repo and there never has been. Nothing is drifting today, so the real question is whether to add a pointer at all.It does need one — Claude Code reads
CLAUDE.md, notAGENTS.md, so today it picks up nothing from our guide. And your symlink instinct matches what the other Apache projects settled on: apache/spark, apache/airflow, apache/superset, and apache/datafusion all check inCLAUDE.mdas a symlink toAGENTS.md. It is a tracked-symlink pattern we already use elsewhere in this repo (python/LICENSE), so it is not a new mechanism for us either.The one caveat is Windows. Creating a symlink there needs Administrator privileges or Developer Mode, and git's own docs note that with
core.symlinks=falsea symlink is checked out as "a small plain file that contains the link text" — soCLAUDE.mdsilently becomes a 9-byte file whose entire content is the stringAGENTS.md, and an agent reading it gets nothing. For that reason the documented alternative is a one-line realCLAUDE.mdcontaining@AGENTS.md, which is an import: same single source of truth, portable to Windows, and it leaves room for Claude-specific content later (https://code.claude.com/docs/en/memory).Both are one-line files and neither duplicates content, so I'm happy either way — the symlink has the Apache precedent, the import is the safer default for contributors on Windows. WDYT?
wenjin272 commentedon Jul 15, 2026
Thanks for the thoughtful follow-up, @weiqingy. Overall, I agree with this direction.
For item 2, I think we could create a separate Markdown file for each change type and reference those files from code_review.md. Each file could contain a focused checklist together with one or two examples drawn from actual human reviews. This would keep the main code_review.md concise while allowing the relevant detailed guidance to be loaded only when needed.
I also agree with the value and motivation behind items 3 and 4. Module-scoped AGENTS.md files and a single source of truth for agent instructions both seem worth pursuing.
weiqingy commentedon Jul 16, 2026
Sounds good, @wenjin272 — the per-change-type files are the right call; they keep
code_review.mda thin index while a type's detailed checklist only loads when an agent touches that kind of change.I'll open a first PR with the change-type matrix as an index in
code_review.mdplus one type —runtime/state & recovery — fully worked as the template, with its examples linked to the real review threads that caught them (#874, #880). Once the shape looks right we can fill in the remaining types one small PR each.I'll put the per-type files under
review-guides/(happy to move them under.github/if you'd rather keep the root uncluttered).Separately, item 4 is up as #910 — a one-line
CLAUDE.mdimportingAGENTS.mdvia@AGENTS.mdrather than a symlink (a checked-in symlink degrades to a plain text file on Windows undercore.symlinks=false). And I'll sequence module-scopedAGENTS.md(item 3) after this lands, since both rely on the same load-nearest-file-on-demand behaviour.wenjin272 commentedon Jul 25, 2026
@weiqingy I’m wondering whether we should complement the reviewer-side guidance with some developer-side guidance for AI-assisted PRs.
For a non-trivial PR whose implementation is largely AI-assisted, the author could provide a concrete Implementation Description, either in the PR description or as a linked document. It should go beyond a high-level design and cover the runtime flow, behavioral contracts, key decisions, failure behavior, compatibility impact, and how tests map to those contracts.
This could support a two-stage review process:
One possible way to introduce this would be to add a short How to Contribute section to
AGENTS.md, link to something likecontribution-guides/ai-assisted-pr.md, and add an Implementation Description section to the PR template.The goal would be to keep design and architectural decisions under human review, while letting coding agents handle more of the implementation-level verification. Do you think this direction would be useful and feasible for the project?
I think we could first try a lightweight experiment before promoting this as a community-wide practice.
For PRs opened by the two of us, especially non-trivial or heavily AI-assisted ones, we could voluntarily include a concrete Implementation Description in the PR body or a linked document. It would describe the runtime flow, behavioral contracts, key decisions, failure behavior, compatibility impact, and the mapping between those contracts and tests.
Reviewers could first review this description at the design and architecture level, then use a coding agent to verify that the implementation and tests match it. This may help keep architectural decisions under human review while delegating more implementation-level verification to AI.
After trying this on a few PRs, we could evaluate whether it actually reduces review effort, what level of detail is useful, and how much authoring overhead it introduces. If the experiment works well, we could then consider documenting it in
AGENTS.md, adding a contribution guide, or updating the PR template. Would you be interested in trying this?weiqingy commentedon Jul 27, 2026
@wenjin272 Yes, I'd like to try this. Happy to start on my side right away.
On the direction first: this is the developer-side mirror of what we just landed on the reviewer side, which I think is the strongest argument for it.
code_review.mdplus the per-type guides tell an agent what to look for in someone else's change. Nothing today tells an author's agent what it owes a reviewer. Writing the contract down once serves both stages, so the cost is amortized rather than added.I already write something close to this for my own non-trivial PRs, as a private working note. So on your authoring-overhead question I can share a real data point from my end: for a PR that already has a design behind it, the Implementation Description is mostly a derivation, not new authoring. Where I expect the overhead to actually show up is the test-to-contract mapping, which is the part I currently do implicitly.
First candidate. I have a non-trivial, AI-assisted PR coming up: native structured output for the Azure OpenAI chat model (#280, following #843 and #919). It has a real behavioral contract (when the native path applies versus the prompt-based fallback), a compatibility story, and a failure mode worth stating explicitly. I'll write the Implementation Description for it and we can use it as the first sample. If that turns out to be too small a case, I'll do the same on the next one.
One question about stage 2. If the same agent that wrote the implementation also authors the description, and the description can still be edited after stage 1, then stage 2 verifies the code against a document that was reshaped to match the code. The check then passes by construction and tells us nothing. A cheap guard would be to have stage 2 verify against the version accepted in stage 1, so any later edit to the description shows up as its own reviewable delta instead of silently becoming the new baseline. Does that match how you were picturing it, or were you thinking of the description as a living document that tracks the PR?
Where the description lives. Length probably decides this, and how long these actually run is one of the things the experiment should tell us. My guess is that a full description for a non-trivial change is long enough to swamp the PR body and bury the template sections under it. There's also a versioning problem that ties back to the point above: if stage 2 verifies against what stage 1 accepted, the description has to be pinnable, and an edit to a PR body leaves no visible trace. So I'd suggest a short contract-level summary in the PR body, enough for a reviewer to start stage 1, with the full description as a PR comment that we amend by posting a new comment rather than editing. That gives the stage-2 agent a permalink to exactly the version humans accepted, and costs the repo nothing. A per-PR document committed to the tree is the other way to get versioning, but it leaves a permanent file whose only reader is one review (and going by #911, new markdown also needs a
tools/.rat-excludesentry beforecheck-license.shpasses). If descriptions outgrow what a comment carries comfortably, that's the signal to move to a committed document, and by then we'll know what size we're dealing with.One thing worth measuring. Alongside your three criteria, I'd like to track whether writing the description catches anything before review. Stating the failure behavior and the test-to-contract mapping explicitly is where I'd expect gaps to surface. If that holds, the description pays for itself even on a PR where it saves the reviewer nothing.
On the rest of the issue: now that #911 is in, I'll pick up the remaining change-type guides (
api/, the Python-Java bridge,dist, docs-only) next, and module-scopedAGENTS.mdafter that. This experiment is independent of both, so it doesn't need to hold either up.wenjin272 commentedon Jul 27, 2026
Thanks, @weiqingy. The Azure OpenAI structured-output change sounds like a good first candidate.
I think your baseline concern is valid for specification-driven development, where humans approve a design before implementation and the code must be verified against that fixed specification.
My proposal is slightly different. The Implementation Description is an as-built description of the final code, intended to make review easier:
In this model, updating the description as the implementation evolves is expected, so fixing a Stage 1 baseline is not essential. If an individual contributor chooses to follow an SDD workflow for a particular PR, pinning the human-approved specification would make sense in that case. However, this would remain an optional development practice rather than a community requirement.
Regarding the issue of the description's position, I agree with your view.
weiqingy commentedon Jul 28, 2026
@wenjin272 That distinction resolves it, thanks. I was reading stage 2 as a one-directional check, and against an as-built description that check really is vacuous. The second half of your stage 2, whether the description omits any important behavior, is the part I was missing, and it changes the shape entirely: the description isn't a spec the code has to meet, it's a summary whose completeness the agent certifies so a human can review the summary instead of the diff. Nothing to pin in that model. Agreed as well that pinning only makes sense as an optional practice for a contributor running a spec-driven workflow, not something the project should require.
One thing I'd suggest we watch during the experiment. The omission check is carrying most of the weight now, and it's the harder of the two halves. Verifying a stated claim against the code is bounded work, but deciding what the description should have mentioned is close to the full review problem, and it asks the checking agent to notice something the authoring agent already missed once. So if we run a few PRs and the omission check comes back empty every time, I'd read that as a signal about the check rather than evidence the descriptions were complete. Might be worth logging omissions found as its own number alongside your three criteria.
One concrete way to help that check: treat the six fields you listed (runtime flow, behavioral contracts, key decisions, failure behavior, compatibility impact, test-to-contract mapping) as required headings, with an empty one stated explicitly rather than dropped. It bounds part of the omission problem, since "does the compatibility section name what breaks" is a much cheaper question than "is anything missing at all". It also counters the failure I'd expect to be most common: an as-built description written by the same agent that wrote the code tends to describe what that agent deliberated about, so decisions it made reflexively never reach the page. Fixed headings force a sweep across categories instead of free recall. A section that says "Compatibility impact: none" can still be wrong, which is what the diff-side half of stage 2 is for, but a missing category at least becomes visible instead of invisible.
I'll write the first description with the Azure OpenAI structured-output PR and we can calibrate from there.