Repository navigation
Conversation
DABH
left a comment
There was a problem hiding this comment.
Review of the new ## Plugin behavior rule (head a899be2). The diff is one heading and one bullet in AGENTS.md; nothing in scripts/ parses that file, so the findings are about what the sentence tells a contributor or coding agent to do, checked against the code on main and against #270, the PR the rule was written to settle. 16 candidate findings were each verified independently: 12 held, 3 were refuted (the CancelledFailure branch in #270 translates upstream's own throwIfAborted(), so it is not a departure; blank squash bodies do not undercut 'say in the PR' because #270 pins a named test and a README line; a one-bullet section outside the 'Do not' list is consistent with the file), and a gap sweep added the CONTRIBUTING item.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
The recurring theme: this is a two-sentence tie-breaker written for one question (code versus docs) but phrased as a general mandate, and every merged plugin already departs from upstream for reasons the sentence never names: replay compatibility, Temporal owning retries, no secrets in Event History, determinism. Read literally by a coding agent, 'Don't keep ... a plugin-specific rule that departs from it' directs the removal of those departures.
Further findings, all on line 88
- P2: the rule's cited example is unmerged. The PR body's 'The plugin follows the code' is false on main: #270 is open, and
typescript/openai-agents/src/workflow/convert-agent.ts:45(modelNameOverride ?? rawModel) and:60(agent.clone({ model: activityBackedModel })) still implement override-all, withREADME.md:730advertising 'Model override per run'. Merging #272 first makes main violate its own rule on the motivating example and invites a drive-by 'fix' that competes with #270's provider-on-runner design. State the dependency (merge after #270) or phrase the rule without encoding an unmerged decision. - P2: no default for behavior Temporal does not allow. The repo currently rejects loudly (
runner.ts:157SandboxConfigurationError,:207UnsafeSessionError,typescript/google-adk/README.md:725GoogleAdkUnsupported), drops silently (#270'srunner.ts:268-276whitelists run options, so a caller'ssignalis ignored with no warning;python/langgraph/README.md:303leavesruntime.storeNone with a one-time warning) and substitutes (session.ts:10-14WorkflowSafeMemorySession,python/deepagents/src/temporalio/deepagents/_tools.py:403-411). 'Match upstream' is satisfied by a silent no-op. Add a default: reject at run start unless a documented replay-safe substitute exists. - P2: no carve-out for upstream defects.
_invoke_model_activity.py:335-358(_align_implicit_model_settings, #178) deliberately does what the locked openai-agents 0.22.0 does not, to work around openai/openai-agents-python#5414; upstream fixed it on 2026-10-27 (#5425) outside the plugin's>=0.22,<0.23range. Under a locked-version reading the rule says delete it, and custom-provider users regress to the non-retryable HTTP 400 at:388-399; under an upstream-main reading it says keep. Same shape:typescript/openai-agents/src/workflow/agent-sink-processor.ts:46(open-span sweep because upstream skips.end()on interruption) andpython/langsmith/pyproject.toml:21(!=0.14.0,!=0.14.1). Say that a workaround for a linked upstream issue is not a departure and is removed once the fix enters the supported range, aspython/google_genai/src/temporalio/google_genai/_compat.py:4-5already states for itself. - P2: 'what its code does and its tests pin' names no version.
typescript/google-adk/src/__tests__/helpers.ts:39-42says the@google/adk >=2.0.0 <2.4.0range 'spans releases whose behavior differs' and pins each release's behavior (adkAtLeastbranches inhitl.test.ts:400,430,442); 9 of 17 plugins declare multi-minor ranges; the TS plugin locks@openai/agents-core0.14.3 against 0.21.0 on upstream main, Python 0.22.0 against 0.24.0 on PyPI; installed packages ship no tests, andexclude-newer = "2 weeks"keeps locks behind. Say which version governs (releases within the declared range, not upstream main) and that tests are evidence of that code, not a second authority. - P3: the code-versus-docs split is undefined for the motivating case. In the locked
@openai/agents-core0.14.3,dist/run.d.ts:60-65saysRunConfig.model'will override the model set on every agent' whiledist/runner/modelSettings.d.ts:5-9and the code give the agent precedence. Whether in-package JSDoc and.d.tscontracts count as 'code' or 'docs' decides #270's own question, and the disclosure trigger may never fire. - P3: no step to cite or open an upstream issue first. The JS split has existed since 0.0.1 (
run.ts:318and docsrunning-agents.mdx:91versusselectModeland its named test); the Python SDK'sget_model(turn_preparation.py:136-139) makesrun_config.modelwin; #270 cites no upstream issue, while #178 cited #5414 before working around. If upstream fixesselectModeltoward its docs,make sync-latestpicks it up and the rule directs a second flip of the plugin's own pin (test-openai-agents.ts:1113), another silent in-flight change. Require citing or opening an upstream issue and stating the in-flight effect of a flip. - P3: 'the upstream framework' has competing referents. The locked
temporalio==1.34.0installstemporalio/contrib/openai_agents/into the plugin's own venv (its historical upstream);typescript/README.md:40andtypescript/_shared/eslint.config.mjs:1use 'upstream' for sdk-typescript and AGENTS.md:166 for Actions jobs;python/deepagentssits on deepagents, langchain, langgraph and langsmith, with_plugin.py:188-189deferring totemporalio.langgraph's mechanism. Name the referent (the framework the plugin is named for, or aframeworkfield in plugin.toml) and exclude sdk-python's installed contrib modules. - P3: sibling-language parity is unaddressed.
python/openai_agents/tests/test_openai.py:2410-2430pinsRunConfig(model=...)winning over the agent's model (upstreamturn_preparation.py:136-143); #270'ssrc/__tests__/workflows/openai-agents.ts:640-642pins the opposite. Both are 'as upstream', both plugins are GA, and neither README mentions the other. Say whether sibling plugins may differ and require the README to record the precedence. - P3: documented contracts get no status.
go/googleadk/activities.go:19-26never writes ADK's global model registry because upstream documents it as opt-in and application-owned;go/googleadk/registry_internal_test.go:59-76records the panic that global registration caused. 'even where its docs say otherwise' lets that justification be challenged under 'what its code does'. Scope the docs-versus-code clause to observable runtime semantics and keep documented contracts binding.
|
|
||
| ## Plugin behavior | ||
|
|
||
| - Match the upstream framework's behavior wherever Temporal allows it: what its code does and its tests pin, even where its docs say otherwise. Don't keep or add a plugin-specific rule that departs from it, and say in the PR when upstream's docs contradict its code. |
There was a problem hiding this comment.
P1: no precedence for replay compatibility over 'Don't keep'. The only workflow.patched sites in plugin source are legacy branches whose old behavior departs from upstream and is kept solely so recorded histories replay: python/deepagents/src/temporalio/deepagents/workflow.py:104-108 ('replaying them without the cache would emit commands history does not have'), python/strands_agents/src/temporalio/strands_agents/_temporal_model.py:244, python/openai_agents/src/temporalio/openai_agents/_openai_runner.py:147-149 and :281. AGENTS.md is a coding agent's verbatim instruction set; an agent that deletes one of those branches to comply produces a nondeterminism error on every Workflow Task of an in-flight run. Every other behavior statement in this file exists for replay (line 59 Go identifiers, line 155 Activity names), and repo practice on gating is split: #204 (b11a86b) gated its refusal, while #270, this rule's own exemplar, flips runConfig.model precedence with no gate ('switches to it, without an error'). Add a sentence that behavior changes affecting recorded histories are gated or called out as breaking with in-flight and Worker Versioning guidance, and that it takes precedence over this bullet.
|
|
||
| ## Plugin behavior | ||
|
|
||
| - Match the upstream framework's behavior wherever Temporal allows it: what its code does and its tests pin, even where its docs say otherwise. Don't keep or add a plugin-specific rule that departs from it, and say in the PR when upstream's docs contradict its code. |
There was a problem hiding this comment.
P2: the carve-out qualifies only the first sentence. 'wherever Temporal allows it' attaches to 'Match the upstream framework's behavior'; the second sentence, 'Don't keep or add a plugin-specific rule that departs from it', carries no qualifier and switches noun from 'behavior' to 'rule', a word this file otherwise uses only for enforced policy (line 84 'org opengrep rule', line 335 'Python rules'). Read on its own it forbids keeping the Temporal-required departures the first sentence permits, such as typescript/openai-agents/README.md:100-101 (runConfig.model must be a string; signal unsupported), and a reader cannot tell whether 'rule' means the README statement or the code behind it (#270 changed both for the same rule). One sentence avoids the drift: 'Match the upstream framework's behavior wherever Temporal allows it, in existing plugin code as well as new: what its code does and its tests pin, even where its docs say otherwise. Say in the PR when upstream's docs contradict its code.'
|
|
||
| ## Plugin behavior | ||
|
|
||
| - Match the upstream framework's behavior wherever Temporal allows it: what its code does and its tests pin, even where its docs say otherwise. Don't keep or add a plugin-specific rule that departs from it, and say in the PR when upstream's docs contradict its code. |
There was a problem hiding this comment.
P2: 'wherever Temporal allows it' is undefined, and the departures the plugins actually make are policy, not impossibility. A literal reader can take 'allows' as 'the Workflow sandbox does not throw'. Upstream paths that run without any Temporal error and are refused anyway: python/openai_agents/src/temporalio/openai_agents/_openai_runner.py:270-278 (a provider set on the run), _trace_interceptor.py:30-31 (#186: the header is stored in Event History, so it must not hold a credential), _temporal_model_stub.py:125-133, and typescript/openai-agents/src/workflow/runner.ts:243-247 (UnsafeSessionError for upstream's MemorySession). Merged plugins also depart from upstream on grounds Temporal permits either way: _invoke_model_activity.py:406-412 defaults to AsyncOpenAI(max_retries=0) where upstream defaults to 2; python/typesafe/src/temporalio/typesafe/_plugin.py:31-36 and python/google_genai/src/temporalio/google_genai/_google_genai_plugin.py:25-42 raise on upstream-permitted retry options; go/googleadk/README.md:213-215 asks users to disable SDK retries; typescript/google-adk/src/hitl.ts:32-37 drops adk_request_credential so secrets never enter history; go/googleadk/activities.go:135-156 adds a zero-config gemini-* fallback that upstream's registry explicitly makes opt-in. A contributor restoring upstream defaults to comply gets SDK backoff multiplied by Temporal retries, secrets in Event History, or a hung graph. Enumerate the grounds (retry ownership, history as the only state store, no secrets in history, determinism) and say that refusals enforcing them are not departures.
|
|
||
| ## Plugin behavior | ||
|
|
||
| - Match the upstream framework's behavior wherever Temporal allows it: what its code does and its tests pin, even where its docs say otherwise. Don't keep or add a plugin-specific rule that departs from it, and say in the PR when upstream's docs contradict its code. |
There was a problem hiding this comment.
P2: the rule codifies #270's symptom, not its cause. #270's stated root cause is 'the plugin's copies of the framework's objects', yet nothing here mentions cloning, re-implementation or extension points, so a faithful copy that matches upstream's current code and tests satisfies every clause and drifts at the next upstream release. The GA Python plugin still does what #270 removed in TypeScript: _openai_runner.py:69 dataclasses.replace(agent), :98 re-wrapped on_invoke_handoff, :118-121 replaced tool _agent_instance, still called at :326 although a provider is installed at :294. That clone silently dropped upstream's Handoff._agent_ref (added in 0.9.0) until #85, and sdk-python #1310 was a second bug of the same class. State the mechanism: run upstream's own code paths and intervene only at Temporal seams through its extension points; never clone framework objects or depend on private fields.
| - No changelog files. `scripts/release/release_tool.py release-notes` derives notes from commit subjects touching the plugin directory since its previous tag. | ||
| - Every GitHub Action is pinned to a full commit SHA with a `# vN` comment (org opengrep rule). | ||
|
|
||
| ## Plugin behavior |
There was a problem hiding this comment.
P3: nothing prompts the 'say in the PR' disclosure. CONTRIBUTING.md:14-15 defers to the org PR template (issue, how tested, docs updates); there is no repo PULL_REQUEST_TEMPLATE.md, and the 'Pull requests' section is untouched although the PR body calls this 'the docs update'. A contributor filling the three checklist items never states the contradiction, and reviewers have no cue to ask. Add one line under 'Pull requests' pointing at this section, or fold it into checklist item 3. Separately, 'How was this tested: check_conventions.py passes' is vacuous: the script never reads AGENTS.md, and scripts/ci/detect_changes.py:72 says a root-level AGENTS.md change selects nothing.
What was changed
Adds a "Plugin behavior" section to
AGENTS.mdwith one rule: match the upstream framework's behavior wherever Temporal allows it. That means what the framework's code does and its tests pin, even where its docs say otherwise. Don't keep or add a plugin-specific rule that departs from it, and say in the PR when upstream's docs contradict its code.Why?
It came up in #270. The OpenAI Agents JS SDK documents
RunConfig.modelas overriding every agent's model, but itsselectModelcode, and the tests that pin it, let an agent's own model win. The plugin follows the code. Writing the rule down lets contributors and reviewers settle questions like this without asking each time.Checklist
scripts/ci/check_conventions.pypasses.