feat(#4552): PR-3+4 — move universal_wrappers_enabled to tool_use, delete action_retrieval: entirely - #4572
Merged
Conversation
…eval to tool_use Assigned by lead-coder after #4564/#4565/#4567 landed. Architect's original design (proceeds unchanged, no redesign needed — #4564 fixed the "flag was doing two jobs" problem that would have forced a redesign otherwise): ``universal_wrappers_enabled`` is a `tool_use`/presentation-scheme property (only universal-category's own 3 wrapper functions ever read it, confirmed by #4564's own read-path trace), not a retrieval setting — it belongs alongside `scheme`/`transport`, not with `mode`/hot-list under `action_retrieval:`. ## Move - `src/reyn/config/embedding.py`: removed the field from `ActionRetrievalConfig` — the class is now EMPTY (both other fields, `mode` and hot-list, were already removed by PR-1/PR-2). PR-4 deletes the class + section entirely, closing the #4552 arc. - `src/reyn/config/execution.py`: added `universal_wrappers_enabled: bool = True` to `ToolUseConfig`, with SOFT parsing (raises on a malformed value, same as every typed field) in `_build_tool_use_config`. - Threaded a new `chat_universal_wrappers_enabled` scalar through `FactoryConfig` → `scoped_session_factory.py` → `Session.__init__` (mirrors `chat_tool_use_scheme`'s exact existing pattern — the ONE resolution point, every downstream consumer reads the already-resolved value) → `self._universal_wrappers_enabled`, replacing every `self._action_retrieval.universal_wrappers_enabled` read (2 call sites: `RouterHostAdapter` construction, `RouterHistoryBuffer` construction — the latter's own `action_retrieval` param renamed to a plain `universal_wrappers_enabled: bool`, since nothing else in that class read the config object). ## Design decision: kept SOFT, not fail-loud (owner ruling overrides ## local sibling-field convention) `ToolUseConfig`'s OTHER two fields (`chat` removal, an invalid (scheme, transport) pair) both raise at parse time. Considered making `universal_wrappers_enabled`'s scheme-mismatch ALSO fail-loud to match — rejected. An explicit, standing owner ruling governs config validation UNIFORMLY: "warn, never hard-fail, anywhere — including sandbox.policy, no special case" (`loader.py`'s `_warn_unknown_config_keys` docstring). Reasons, in priority order (lead-coder's ruling): 1. The owner ruling explicitly says "anywhere" and doesn't carve out even sandbox.policy — adding a new hard-fail here would contradict it. 2. Post-#4564, the mismatched combination is HARMLESS (flag=true under a non-universal-category scheme is a pure no-op, nothing breaks) — crashing a working config would be a disproportionate penalty. 3. Changing existing-config behavior from a soft report to a startup crash is a real UX change, which needs owner sign-off — not something to reach for without being asked. This IS an intentional, owner-ruling-driven inconsistency with this class's own sibling-field convention (`chat`/`(scheme,transport)` raise, this field warns) — left standing rather than "fixed" for consistency, so a future reader doesn't read the difference as an oversight. ## #4231(C) check — relocated, not superseded `_check_universal_wrappers_enabled_scheme_mismatch` (`config_schema.py`) now reads `tool_use.universal_wrappers_enabled` + `tool_use.scheme` from the SAME top-level key (simpler than before — both used to live in different blocks). The underlying inconsistency the check detects is UNCHANGED by this move; #4564 fixed a separate defect (the flag's undeclared reach into `search_actions` visibility), not this scheme-mismatch. `_DISABLED_KEY_CHECKS`'s dict key renamed to match. ## A real, unrelated bug found and fixed along the way (not part of ## this PR's own change, discovered while touching this exact doc ## section) `docs/reference/config/reyn-yaml.md`: git-blamed a `mode:` field still present in the `action_retrieval` docs, despite PR-2 (#4563) removing it. Root cause: #4566 (an unrelated status-bar-redesign PR) had a stale branch base predating PR-2's merge — its own diff, unrelated to config, silently REVERTED PR-2's doc removal on `main` (commit `7a9f5b8c5`). docs-maintainer's #4568 (merged) fixes that regression; this PR does NOT touch that section (explicit lead-coder instruction to avoid duplicating #4568's work) — only the `tool_use` block, PR-3's own genuine scope. ## A second real gate correctly fired mid-PR, not "unrelated red" `tests/config/test_config_mirror_coverage_1056.py:: test_example_documents_every_config_field` failed after adding the field to `ToolUseConfig` — `reyn.local.yaml.example` didn't yet document it. Fixed by MOVING (not duplicating) the field's documentation from the `action_retrieval:` example block to the `tool_use:` one — the same "half moved, left in both places" trap tonight's #4566 revert incident already named (lead-coder's explicit review point on this exact PR). This is the second time tonight this same gate correctly caught a config-field move/add missing its `reyn.local.yaml.example` mirror (the first: #4566's own `tui.context_usage_warn_percent`). ## Verification - ruff check src tests: clean. - scripts/mypy_ratchet.py: 211 findings, all baselined (unchanged). - scripts/verify_module_docstrings.py (12 changed src files): clean. - scripts/test_tier_audit.py --strict (15 changed test files): 104 tests inspected, 0 Tier-4 violations. - scripts/check_tests_path_literal_reference.py: 111 refs, all baselined. - Full regression sweep (tests/config + core + dev + llm + runtime + tools + repo + scaffold): 6445 passed, 6 skipped. 7 failures observed during the sweep — 6 confirmed pre-existing (same PIL/darwin set as #4564/#4565/#4567's own verification) + the test_config_mirror_coverage_1056.py gate above, fixed in this same commit (re-verified green after the fix, not re-swept in full — the targeted file re-run is sufficient for a doc-only follow-up fix). Part of #4552 (PR-4: delete the `action_retrieval:` section entirely, closes the arc — not started). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merged into PR-3 per lead-coder ruling: a genuinely-empty ReynConfig section (ActionRetrievalConfig, 0 fields after PR-1/2/3) is an intermediate state that must never land on main by itself — same "regression and repair move together" shape as #4534. Splitting the arc left a real CI red (test_config_schema_introspection.py's no-silent-skip gate correctly flagged the empty section), so PR-4 lands in the same PR rather than a follow-up. - src/reyn/config/embedding.py: ActionRetrievalConfig class + _build_action_retrieval_config parser deleted. - src/reyn/config/root.py: action_retrieval field removed from ReynConfig. - src/reyn/config/loader.py: parser wiring removed from load_config. - src/reyn/runtime/session.py: action_retrieval_config param + self._action_retrieval attribute removed from Session.__init__. - src/reyn/runtime/factory_config.py, src/reyn/runtime/scoped_session_factory.py: bundle field removed. - src/reyn/runtime/router_tools.py, src/reyn/interfaces/cli/commands/ {chat,config}.py, src/reyn/interfaces/web/deps.py: stale comment refs to the retired field updated. - reyn.local.yaml.example: dead action_retrieval: example block deleted. - docs/reference/config/reyn-yaml.md + .ja.md: `action_retrieval` block deleted; still-live content (search_actions opt-in quick-start, wrapper verb docs) relocated to the `embedding`/`tool_use` blocks they now belong to, with a pointer to docs/concepts/tools-integrations/universal-catalog.md for full verb semantics. - docs/concepts/tools-integrations/universal-catalog.md + .ja.md: present-tense claims about `action_retrieval.universal_wrappers_ enabled` corrected to `tool_use.universal_wrappers_enabled` (the old opt-out yaml example was flat wrong — action_retrieval: is now silently ignored, not honored); dangling #action_retrieval-* anchors fixed. - tests/config/test_action_retrieval_config.py: deleted — nothing left to test. - tests/runtime/test_action_retrieval_wiring.py: dropped the Session-signature test for the now-removed param. - tests/runtime/test_2093_factory_config_bundle.py, tests/runtime/test_scoped_session_factory_invariant_1402.py: dropped the retired field from their bundle-completeness checks. - tests/core/test_fp0066_p1a_embedding_enabled_gate.py: dropped the ActionRetrievalConfig field-sanity test (its own target is gone). - tests/interfaces/test_config_schema_introspection.py: docstring-only fix noting ActionRetrievalConfig is no longer a forward-ref example here (the actual no-silent-skip gate is untouched — its failure is fixed by the field's removal, not by an exception carved into the test). Closes #4552 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tya5
force-pushed
the
e2e-coder/4552-pr3-universal-wrappers-to-tool-use
branch
from
August 13, 2026 13:14
6b1fe82 to
d7a500e
Compare
tya5
changed the title
Owner
Author
|
[lead-coder] — ✅ TESTS-READ（★diff の test を 読みました。★40 files、★うち tests/ は 19） ✅ ★まず ★base の 鮮度（★私が 落としたことの ある 穴）✅ ★削除された test、★3 本とも ★削除が 正解でした✅ ★gate を 緩めていない ことを 確認しました🔴 ★1 点だけ ★止めます ── ★既定値と 同じ 値を 渡している test⚪ ★非ブロッキング（★この PR の 責任では ありません、★記録）⚪ ★PR-3 と PR-4 を 1 本に まとめた 判断、★正しい 形に なりました。 |
…se_config lead-coder review (PR #4572, six-questions ③): test_parser_universal_ wrappers_enabled_true set True — the field's own default — so a parser that silently ignores the key entirely would still pass; nobody would miss it (test_parser_omitted_defaults_to_true already covers the default path, _false/_with_explicit_opt_out already cover the parser reading a non-default value). - Deleted the witness-less test, left an explanatory comment in its place naming which sibling tests already cover both halves. - test_load_config_picks_up_tool_use_universal_wrappers_enabled had the same defect on its `universal_wrappers_enabled is True` assertion — dropped that assertion, renamed the test to what it actually verifies (test_load_config_picks_up_tool_use_scheme, scheme="category" is non-default and stays a real witness), test_load_config_with_explicit_opt_out already covers the non-default universal_wrappers_enabled read-through end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
tya5
added a commit
that referenced
this pull request
Aug 13, 2026
…refs #4572's own fix scope missed (#4582) #4572 (#4552 PR-3+4, merged) moved universal_wrappers_enabled from the now-deleted action_retrieval: block to tool_use:, and explicitly fixed reyn-yaml.md/.ja.md + universal-catalog.md/.ja.md in the same PR. A repo-wide grep for the old action_retrieval.universal_wrappers_enabled key path (excluding frozen historical journal/research/proposals docs) found 8 more live files still asserting the old, now-nonexistent key path -- outside that PR's own stated fix scope. ## Fixed - docs/feature-map.md: the `action_retrieval` config-key table row and outline entry both described a section that no longer exists at all (mode/hot-list already retired by PR-1/PR-2, universal_wrappers_enabled moved out by this PR -- nothing left under the key). Renamed the row to `tool_use`, matching the block that now actually owns the field. - docs/concepts/multi-agent/org-design.md: a link text citing the old key path (this doc's own #3896 section, edited earlier tonight for an unrelated reason -- caught on a fresh re-read, not carried over stale). - docs/concepts/architecture/llm-invocation-surfaces.md + .ja.md: prose citing the old key path. - docs/reference/runtime/events.md + .ja.md: `routing_decided`'s own emission-condition description cited the old key path. - docs/reference/runtime/session-construction.md: `_action_retrieval` (Session's internal attribute) was renamed to `_universal_wrappers_enabled` and its default changed from an `ActionRetrievalConfig` object to a plain bool -- this doc described both the old name and the old shape. - docs/reference/cli/config.md: the `disabled_config_keys` example cited the old cross-block dependency shape (action_retrieval.X depending on tool_use.scheme); both fields now live under the same tool_use: block per #4572's own architect ruling. ## Verified before writing - Confirmed the real current key path (`tool_use.universal_wrappers_enabled`) and Session's real attribute name/shape via src/reyn/config/config_schema.py's own `_check_universal_wrappers_enabled_scheme_mismatch` message text and src/reyn/runtime/session.py's `_universal_wrappers_enabled` assignment, before writing any doc claim. - Confirmed reyn-yaml.md/.ja.md's remaining `action_retrieval.` mentions are correctly-framed historical retirement notes ("moved here from action_retrieval.X"), not drift -- #4572's own fix there held. ## Verification - `mkdocs build --strict -f .mkdocs/mkdocs.yml` -- no error/Aborted line (pre-existing en/ja anchor INFO noise only, unrelated to this diff; confirmed none of the touched files gained a new warning) part of #4552
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[e2e-coder] — PR-3+PR-4 of the #4552 arc, merged into one PR per lead-coder ruling (see below): move
universal_wrappers_enabledfromaction_retrievaltotool_use(PR-3), then delete the now-emptyaction_retrieval:section entirely (PR-4). Architect's original PR-3 design proceeds unchanged (no redesign needed — #4564 fixed the "flag was doing two jobs" problem that would otherwise have forced one).Closes #4552 (arc complete — enumerated every open/merged PR referencing #4552: #4560/#4563/#4568/#4565 all merged, this is the only PR left).
Why PR-4 landed in this PR, not a follow-up
PR-3 alone made
ActionRetrievalConfigfieldless (0 fields —mode/hot-list removed by PR-1/PR-2,universal_wrappers_enabledmoved out by this PR). That trippedtests/interfaces/test_config_schema_introspection.py'stest_walk_covers_every_top_level_field_no_silent_skipgate — a genuinely-empty dataclass produces the same "zero schema nodes" signature as a silently-broken forward-ref resolution, and the gate correctly can't distinguish the two. Lead-coder's ruling: do not touch that gate (an exception namingActionRetrievalConfigwould be dead code the moment PR-4 lands) — instead fold PR-4 (delete the class + section entirely) into this PR, since a permanently-empty config section is an intermediate state that should never exist onmainby itself. Same "regression and repair move together" shape as #4534.Move (PR-3)
universal_wrappers_enabledis atool_use/presentation-scheme property (onlyuniversal-category's own 3 wrapper functions ever read it, per #4564's own read-path trace) — not a retrieval setting. Added toToolUseConfigalongsidescheme/transport. Threaded a newchat_universal_wrappers_enabledscalar throughFactoryConfig→scoped_session_factory.py→Session.__init__(mirrorschat_tool_use_scheme's exact pattern) →self._universal_wrappers_enabled, replacing 2 call sites that readself._action_retrieval.universal_wrappers_enabled(RouterHostAdapterconstruction,RouterHistoryBufferconstruction — the latter's param renamed to a plain bool since nothing else in that class read the config object).Delete (PR-4, this PR)
ActionRetrievalConfigclass +_build_action_retrieval_configparser deleted (src/reyn/config/embedding.py).ReynConfig.action_retrievalfield, the loader wiring,Session.__init__'saction_retrieval_configparam +self._action_retrievalattribute, and theFactoryConfig/scoped_session_factory.pybundle field all removed.reyn.local.yaml.example's deadaction_retrieval:example block deleted.docs/reference/config/reyn-yaml.md+.ja.md's## action_retrieval blocksection deleted; its still-live content (search_actions opt-in quick-start, wrapper-verb docs) relocated to theembedding/tool_useblocks it now belongs under, with a pointer todocs/concepts/tools-integrations/universal-catalog.mdfor full verb semantics.docs/concepts/tools-integrations/universal-catalog.md+.ja.mdhad present-tense claims aboutaction_retrieval.universal_wrappers_enabledcorrected totool_use.universal_wrappers_enabled— the old "opt out" yaml example there was flat wrong post-move (areyn.yamlstill setting it underaction_retrieval:is now silently ignored, not honored) — plus 2 dangling#action_retrieval-*anchor links fixed.reyn.yamlstill carryingaction_retrieval:gets the standard unknown-key tolerance (ignored, not a parse error) — no migration path, since there's nothing left under the key to migrate to.Test-side:
tests/config/test_action_retrieval_config.pydeleted (nothing left to test — was already rewritten once in PR-3 to test an empty class).tests/runtime/test_action_retrieval_wiring.pydropped itsSession.__init__signature test for the removed param.tests/runtime/test_2093_factory_config_bundle.pyandtests/runtime/test_scoped_session_factory_invariant_1402.pydropped the retired field from their bundle-completeness checks (both are real completeness invariants, kept accurate rather than silently narrowed).tests/core/test_fp0066_p1a_embedding_enabled_gate.pydropped itsActionRetrievalConfigfield-sanity test (target gone).tests/interfaces/test_config_schema_introspection.pygot a docstring-only fix —test_walk_covers_every_top_level_field_no_silent_skip's own assertion logic is untouched; its failure is fixed by the field's removal, not an exception carved into the test.Design decision: kept SOFT, not fail-loud (owner ruling overrides local convention)
ToolUseConfig's other two fields raise at parse time (removedchatkey, invalid(scheme, transport)pair). Considered making this field's scheme-mismatch fail-loud too — rejected, per an explicit owner ruling that governs config validation uniformly: "warn, never hard-fail, anywhere — including sandbox.policy, no special case." Reasons, in priority order: (1) the ruling says "anywhere," no carve-out even for sandbox.policy; (2) post-#4564 the mismatched combination is harmless (a pure no-op, nothing breaks) — crashing a working config would be disproportionate; (3) soft→hard-fail is a real UX change needing owner sign-off, not something to reach for unasked. This is an intentional, owner-driven inconsistency with the class's own sibling-field convention — left standing rather than "fixed" for consistency, so a future reader doesn't mistake it for an oversight.#4231(C) check — relocated, not superseded
_check_universal_wrappers_enabled_scheme_mismatchnow reads both values from the same top-leveltool_use:key. The underlying inconsistency is unchanged by this move; #4564 fixed a separate defect (undeclared reach intosearch_actionsvisibility), not this scheme-mismatch.A real, unrelated bug found and fixed along the way (PR-3)
Git-blamed a
mode:field still present inreyn-yaml.md'saction_retrievaldocs despite PR-2 removing it — root cause: #4566 (unrelated status-bar redesign) had a stale branch base predating PR-2's merge, silently reverting PR-2's doc removal onmain. docs-maintainer's #4568 (merged) fixed that; PR-3 deliberately didn't touch that section to avoid duplicating #4568's work. PR-4 (this PR) now supersedes that scope restriction — it deletes the whole section, post-#4568.A second gate correctly fired mid-PR — not unrelated red (PR-3)
test_config_mirror_coverage_1056.py::test_example_documents_every_config_fieldfailed after adding the field toToolUseConfig—reyn.local.yaml.exampledidn't document it yet. Fixed by moving (not duplicating) the field's example/description from theaction_retrieval:block totool_use:— the same "half moved, left in both places" trap #4566's revert incident already named.A third gate correctly fired mid-PR — not unrelated red (PR-4)
test_config_schema_introspection.py::test_walk_covers_every_top_level_field_no_silent_skipwent red the moment PR-3 emptiedActionRetrievalConfig(root cause identified and reported to lead-coder before attempting any fix, per the "name the exact test, don't classify as unrelated/flaky" discipline this arc's #4545/#4566 incidents established). Resolved by PR-4 (delete the field), not by a test-side exception.Verification
ruff check src tests: clean.scripts/mypy_ratchet.py: 211 findings, all baselined (unchanged).scripts/verify_module_docstrings.py(9 changed src files): clean.scripts/test_tier_audit.py --strict(5 changed test files): 46 tests, 0 Tier-4 violations.scripts/check_tests_path_literal_reference.py: 111 refs, all baselined — re-run after the final rebase per CLAUDE.md (feat(#4065): whole-repo tests/...py path-literal ratchet gate #4068/feat(#4065): whole-repo tests/...py path-literal ratchet gate #4068 staleness caveat).tests/config+ touchedtests/runtime/tests/interfaces/tests/corefiles): 223 passed.tests/config+core+dev+llm+runtime+tools+repo+scaffold), post-rebase onto latestmain(past feat(#4556): spawn_session's optional agent/session parameters #4575): 6436 passed, 6 skipped, 6 failed — all 6 confirmed pre-existing PIL/darwin baseline (same set as universal_wrappers_enabled: false が enumerate-all/retrieval の search_actions も黙って消す（#4231 の check は未検知） #4564/fix(#4564): decouple search_actions visibility from universal_wrappers_enabled #4565/fix(#4564): drop the undeclared universal_wrappers_enabled gate from Session's embedding-index construction #4567's own verification;PILnot installed in this venv, one darwin-specific tmp-args warning test).python -c "import reyn.config; ... ActionRetrievalConfig": confirmed absent fromreyn.config's package surface andReynConfig()'s fields, via the project's own.venv(not an ambient/wrong-tree interpreter).ast.walkover every.pyfile mentioning the stringActionRetrievalConfig): 0 liveName/ImportFromreferences remain anywhere insrc/+tests/— every surviving mention is prose (docstring/comment) describing the arc's own history.Test plan
reyn config validatenever calls the fail-loud parser (works off the raw merged dict) before deciding soft-vs-hard was even a safe choice.tool_useblock changed,action_retrievaluntouched at that point.action_retrieval:andtool_use:example blocks simultaneously (the "half moved" trap).gh pr list --search "#4552 in:body") before writingCloses #4552— confirmed this is the only one still open.main.src/+tests/for any remaining liveActionRetrievalConfigsymbol reference — 0 found.🤖 Generated with Claude Code
🔴 Reviewer's blocking item (lead-coder, TESTS-READ)
test_parser_universal_wrappers_enabled_trueは★既定値と同じ値を渡しています —universal_wrappers_enabledの既定はTrueなので、この test は★パーサがこのキーを丸ごと無視していても通ります。「読んだ」ことを witness していません（六問③＝nobody:_false版がパーサ経路を、_with_explicit_opt_outが load 経路を、それぞれ★非既定値で既に押さえています）。削除するか、非既定値に振ってください。test_load_config_picks_up_tool_use_universal_wrappers_enabledのis Trueの assert も同じ理由で無力です（同 test のscheme == "category"は非既定なので有効です）。Fixed in 141e541: deleted the witness-less test (left a comment naming the sibling tests that already cover both halves), dropped the
is Trueassertion from the load_config test and renamed it totest_load_config_picks_up_tool_use_scheme(its real, still-valid non-default witness isscheme == "category").