Skip to content

Add a rule to match the upstream framework's behavior to AGENTS.md - #272

Open
campoy wants to merge 1 commit into
mainfrom
agents-md/match-upstream-behavior
Open

campoy wants to merge 1 commit into
mainfrom
agents-md/match-upstream-behavior

Conversation

@campoy

@campoy campoy commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

What was changed

Adds a "Plugin behavior" section to AGENTS.md with 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.model as overriding every agent's model, but its selectModel code, 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

  1. No issue.
  2. How was this tested: scripts/ci/check_conventions.py passes.
  3. Any docs updates needed? This is the docs update.

@campoy
campoy requested a review from a team as a code owner October 29, 2026 16:19

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, with README.md:730 advertising '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:157 SandboxConfigurationError, :207 UnsafeSessionError, typescript/google-adk/README.md:725 GoogleAdkUnsupported), drops silently (#270's runner.ts:268-276 whitelists run options, so a caller's signal is ignored with no warning; python/langgraph/README.md:303 leaves runtime.store None with a one-time warning) and substitutes (session.ts:10-14 WorkflowSafeMemorySession, 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.23 range. 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) and python/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, as python/google_genai/src/temporalio/google_genai/_compat.py:4-5 already states for itself.
  • P2: 'what its code does and its tests pin' names no version. typescript/google-adk/src/__tests__/helpers.ts:39-42 says the @google/adk >=2.0.0 <2.4.0 range 'spans releases whose behavior differs' and pins each release's behavior (adkAtLeast branches in hitl.test.ts:400,430,442); 9 of 17 plugins declare multi-minor ranges; the TS plugin locks @openai/agents-core 0.14.3 against 0.21.0 on upstream main, Python 0.22.0 against 0.24.0 on PyPI; installed packages ship no tests, and exclude-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-core 0.14.3, dist/run.d.ts:60-65 says RunConfig.model 'will override the model set on every agent' while dist/runner/modelSettings.d.ts:5-9 and the code give the agent precedence. Whether in-package JSDoc and .d.ts contracts 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:318 and docs running-agents.mdx:91 versus selectModel and its named test); the Python SDK's get_model (turn_preparation.py:136-139) makes run_config.model win; #270 cites no upstream issue, while #178 cited #5414 before working around. If upstream fixes selectModel toward its docs, make sync-latest picks 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.0 installs temporalio/contrib/openai_agents/ into the plugin's own venv (its historical upstream); typescript/README.md:40 and typescript/_shared/eslint.config.mjs:1 use 'upstream' for sdk-typescript and AGENTS.md:166 for Actions jobs; python/deepagents sits on deepagents, langchain, langgraph and langsmith, with _plugin.py:188-189 deferring to temporalio.langgraph's mechanism. Name the referent (the framework the plugin is named for, or a framework field 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-2430 pins RunConfig(model=...) winning over the agent's model (upstream turn_preparation.py:136-143); #270's src/__tests__/workflows/openai-agents.ts:640-642 pins 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-26 never writes ADK's global model registry because upstream documents it as opt-in and application-owned; go/googleadk/registry_internal_test.go:59-76 records 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.

Comment thread AGENTS.md

## 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread AGENTS.md

## 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.'

Comment thread AGENTS.md

## 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread AGENTS.md

## 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread AGENTS.md
- 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants