docs: improve agent guidance#2959
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
94bf130 to
6be27c3
Compare
6be27c3 to
5df8b71
Compare
5df8b71 to
93a4e00
Compare
| Public/internal marking is a cross-SDK rule — see the [root AGENTS.md](../AGENTS.md). The Python-idiomatic form: | ||
|
|
||
| ### Error Handling | ||
| - **Every public package declares its surface with an explicit `__all__`** in its `__init__.py` (entries listed alphabetically). Internal (`_`-prefixed) and namespace-only packages may omit it. Don't lean on the redundant `import X as X` re-export alias in place of `__all__` for a public package. |
There was a problem hiding this comment.
Issue: The parenthetical "(entries listed alphabetically)" doesn't match the codebase — and notably not even the file this section points agents to as the reference (models/__init__.py), which lists submodules then classes:
__all__ = ["bedrock", "model", "BaseModelConfig", "BedrockModel", "CacheConfig", "CacheToolsConfig", "Model"]Same for strands/__init__.py, agent/__init__.py, and tools/__init__.py — none are alphabetized.
Suggestion: Since this PR's whole purpose is to make the guidance match what the code actually does, either drop the "(entries listed alphabetically)" claim or restate the real convention (e.g. modules first, then public symbols). As written, an agent following this would "fix" every __all__ in the repo and produce noisy, wrong diffs.
|
Issue: The PR description leads with two changes that aren't in this diff. The "New Suggestion: Either include those TESTING.md changes (if they were meant to ship here) or trim the description to match the actual scope. As-is, a reviewer expecting a new file and a path fix will go looking for changes that aren't here. This matters because the description is the contract reviewers check against. |
|
Assessment: Comment (approve-leaning) Documentation-only restructure of the three Two items to address
Neither is blocking; nice cleanup overall. |
Description
AGENTS.mdis supposed to steer coding agents toward house conventions, but an audit of both SDKs against their own guidance turned up three structural problems: the highest-traffic patterns (adding a model provider, the streaming contract) weren't documented at all; several judgment rules lived only in side docs an agent readingAGENTS.mdnever opens; and the two sub-SDK guides had drifted into different shapes with duplicated, occasionally-contradictory rules. This PR restructures all threeAGENTS.mdfiles around a shared skeleton, moves genuinely cross-SDK rules into the root so they can't diverge, documents the missing high-traffic patterns, and trims prose that merely restates what the linters already enforce.It is documentation-only — no source or behavior changes. The companion Python
docs/TESTING.md(and thetest/integ/path fix) landed separately in #2961, which this builds on.Single source of truth for shared rules. Plugin naming, cross-SDK literal parity, public/internal API marking, the logging format, evergreen comments, and directory-naming parity now live once in the root
AGENTS.md; both sub-guides point to it instead of carrying their own (previously diverging) copies.Shared skeleton. Both sub-guides now follow the same section order (Overview → Directory Structure → Workflow → Coding Patterns → Adding a Model Provider → Async & Streaming → Testing → Quick Rules → …), so they're mechanically diffable and a rule present in one but missing in the other is visible at a glance. The
Things to Do/Things NOT to Dolists collapse into a single Quick Rules block of only the un-lintable rules, each tagged "enforced by review."Signal over linter noise. Each guide now states up front that ruff/mypy/eslint/prettier already enforce formatting, import order, line length, and type-annotation coverage, and spends its words on what those tools can't check, rather than restating them.
New guidance
…Plugin); cross-SDK parity (identifiers re-cased; single-word literals byte-identical; multi-word literalssnake_case↔camelCase; wire field names kept verbatim; hook-event names shared); public-vs-internal marking; structured-logging format; evergreen comments (incl. tests + the deprecated/legacy nuance); directory & file naming parity.validate_config_keys/resolveConfigMetadata, the base contract to implement, flat-file-vs-subdirectory threshold).*beforetool_choice, async-first + sync-facade duality, 3.10 floor, cooperativethreading.Eventcancellation; TS's async-only model andAbortSignalcancellation.raise Exception(...).event/index/error, nevere/i/x).TypedDictfor wire/config shapes,@dataclassfor runtime objects, pydantic only for model-read/write schemas, noNamedTuple.X | NoneoverOptional[X]; scoped# type: ignore[code]over bare ignores.__all__; lazy provider loading via__getattr__.docs/STYLE_GUIDE.md" (Protocol-over-Callable,tool_nameover string literals); "before adding a hook event, readdocs/HOOKS.md" (event-naming contract).Raises:docstring required when a public function raises as part of its contract.interfacevstyperule (object shapes asinterface+extends;typefor unions/intersections/etc.).prop?: Toptional-property form underexactOptionalPropertyTypes; named-exports-only / noexport default/ kebab-case filenames /@internal;@throwsTSDoc tag; a%s/%dprintf-in-logs ban; plus a Product Overview and Directory Structure section (the TS guide had neither).ls …for the full list" so an agent doesn't treat the abbreviated tree as a complete inventory.Removed / trimmed guidance
Things to Do/Things NOT to Doremoved as standalone lists, folded into the per-guide Quick Rules block.Changed guidance
tool_use→toolUse). Now: single-word identical, multi-word camelCased in TS, wire names verbatim.Related Issues
None — maintenance change from a convention audit of both SDKs. Builds on #2961.
Documentation PR
Not applicable — these are contributor-facing
AGENTS.mdfiles, not user-facing docs undersite/.Type of Change
Documentation update
Testing
Documentation-only. Verified every path and symbol cited in the new guidance exists in the tree (provider skeletons,
validate_config_keys/resolveConfigMetadata,STOP_REASON_MAP/snakeToCamel, the referenceddocs/links and directory names), that both sub-guides share the same section skeleton, and that an adversarial pre/post comparison found no convention dropped in the restructure (four directives initially lost were restored).hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.