weiqingy commented on code in PR #961: URL: https://github.com/apache/flink-agents/pull/961#discussion_r3755202477
########## review-guides/api-contract.md: ########## @@ -0,0 +1,72 @@ +# Review Guide: api/ Contract + +Load this guide when a PR changes a public API surface: a signature or type in +`api/`, a new resource implementation, a config option, a YAML-visible name, or +anything a user's agent code calls. 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 + +- When a PR adds a public resource implementation, check that its short YAML + alias landed in both alias tables and the doc table. Nothing fails when all + three are skipped: each loader passes an unrecognized name through unchanged, + so the class stays reachable by fully-qualified name and no test notices the + omission. +- Regenerate the cross-language snapshots on both sides in the same change when + a field on a built-in event or on the agent plan is added, renamed, or + retyped. Each language pins its own serialization against its own committed + file, so refreshing one side leaves the other side's stability test failing. A + field added in only one language is caught by nothing, because the payload is + a free-form attribute map on the read side. +- Check that a new public config option landed on both languages' sides. A + Java-only option passes every fast CI job: the bidirectional parity check runs + only in the slow cross-language lane, and the in-tree guard is a hardcoded + count on the Python side. +- Treat the public base classes users extend as source-compatibility boundaries. + A new abstract method breaks every implementation, including ones outside this + repo, while a defaulted overload plus a capability probe does not. The Python Review Comment: Good point, and it turns out not to be hypothetical. `BaseChatModelConnection.chat(List, List, Map, Object)` is that four-argument default, and it throws rather than forwarding a schema it cannot translate. The bullet now reads: > Compiling is not the same as honoring the new argument: a default that forwards to the older signature drops it in silence, so check that the default rejects what it cannot honor, and that an override gating it behind a capability probe leaves a fallback in force or fails, rather than silently doing neither. I could not find a way to express the caller half here. `supportsNativeStructuredOutput` is `protected`, and nothing calls the four-argument overload in production, so I aimed the clause at the overrides, where the probe actually gets consulted. Not a defect report, since no schema reaches those branches today. Does that match what you had in mind, or would you rather it name the caller case and point at #912? On the wording, I made it a disjunction rather than a flat "must fail", since `BaseChatModelConnection.java:59-61` asks an unrecognized model to report `false` and degrade to the fallback. Is that the boundary you were drawing? ########## review-guides/api-contract.md: ########## @@ -0,0 +1,72 @@ +# Review Guide: api/ Contract + +Load this guide when a PR changes a public API surface: a signature or type in +`api/`, a new resource implementation, a config option, a YAML-visible name, or +anything a user's agent code calls. 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 + +- When a PR adds a public resource implementation, check that its short YAML + alias landed in both alias tables and the doc table. Nothing fails when all + three are skipped: each loader passes an unrecognized name through unchanged, + so the class stays reachable by fully-qualified name and no test notices the + omission. +- Regenerate the cross-language snapshots on both sides in the same change when + a field on a built-in event or on the agent plan is added, renamed, or + retyped. Each language pins its own serialization against its own committed + file, so refreshing one side leaves the other side's stability test failing. A + field added in only one language is caught by nothing, because the payload is + a free-form attribute map on the read side. +- Check that a new public config option landed on both languages' sides. A + Java-only option passes every fast CI job: the bidirectional parity check runs + only in the slow cross-language lane, and the in-tree guard is a hardcoded + count on the Python side. +- Treat the public base classes users extend as source-compatibility boundaries. + A new abstract method breaks every implementation, including ones outside this + repo, while a defaulted overload plus a capability probe does not. The Python + guard that catches a mis-declared override only sees connections whose module + is imported by hand at the top of the test. +- Check that a removal is complete rather than asking whether to deprecate. + There is no deprecation mechanism in this repo, so an API is either kept or + deleted outright. Review Comment: Agreed, and it was contradicting this guide's own #756 row, which frames the same question as a judgment. Reworded to: > There is no deprecation mechanism in this repo, so an API is either kept or deleted outright. Under the beta policy, prefer deleting unless a concrete compatibility obligation requires keeping it. I kept the missing-mechanism sentence since that is what rules out a third option, and moved the preference onto the beta policy, where `code_review.md` already puts it. Does that split read the way you meant? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
