Repository navigation
refactor(approval): split approval/manager.py into 8 private modules (pure moves, A-P1-6; needs sign-off) - #111
Merged
Merged
Conversation
Records the risk assessment for the pure-move split of teaagent/approval/manager.py into teaagent/approval/_*.py modules behind the stable facade teaagent.approval.manager (ADR-0044). Human Review gate is OPEN in the report; no approval is recorded. Code moves follow in separate commits, one per plan step. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: docs-only commit. scripts/check_high_risk_paths.py passes with the risk report staged; the three commit-msg validators pass on this message. Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
Adds contract tests for teaagent.approval.manager as the stable facade (A-P1-6, ADR-0044): the pinned __all__ list, the private names imported from the facade, identity through the lazy teaagent.approval map, teaagent.policy re-exports, and the teaagent.approval_manager alias. No existing test is changed. The deny-precedence characterisation from plan Step 0 item 4 is not added as xfail: the fix is on main and tests/test_deny_preset_terminal_over_grants.py already requires it. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: python -m pytest -q tests -k "approval or deny or policy or permission or multisig or jit or skill or symlink" -n 4 -> 1342 passed, 3 skipped (pre-existing); tests/approval -> 78 passed; ruff check and ruff format clean; mypy teaagent/approval --explicit-package-bases -> no issues; dir(teaagent.approval.manager) = 51 names Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
…alize Pure move (A-P1-6 step 1, ADR-0044). The function body is unchanged and teaagent.approval.manager re-exports it, so policy.py and every caller keep the same object. Re-exports of stdlib imports left unused in manager.py are kept for dir() parity with the facade. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: pinned tests (tests/test_shell_obfuscation_adversarial.py tests/test_remediation_p1_p2.py tests/test_hypothesis_invariants.py tests/test_approval_manager_multisig_fallback.py tests/approval tests/test_circular_imports.py tests/test_policy.py) -> 234 passed; approval subset (-k approval or deny or policy or permission or multisig or jit or skill or symlink) -> 1342 passed, 3 skipped; AST move-equivalence -> 0 findings; dir() parity 51/51; ruff check and ruff format clean; mypy teaagent/approval -> no issues; check_circular_imports, check_any_count, check_complexity, check_public_docstrings, validate_runner_invariants pass Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
…rmat Pure move (A-P1-6 step 2, ADR-0044). The function body is unchanged and teaagent.approval.manager re-exports it, so teaagent.approval and every caller keep the same object. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: pinned tests (tests/test_permission_explain.py tests/acceptance/test_policy_denial_reason_code_flow.py tests/acceptance/test_policy_as_code_flow.py tests/test_policy.py tests/approval tests/test_circular_imports.py) -> 157 passed; approval subset (-k approval or deny or policy or permission or multisig or jit or skill or symlink) -> 1342 passed, 3 skipped; AST move-equivalence -> 0 findings; dir() parity 51/51; ruff check and ruff format clean; mypy teaagent/approval -> no issues; structural checks pass Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
… approval/_permission_mode Pure move (A-P1-6 step 3, ADR-0044). Bodies are unchanged and the enum values (read-only, workspace-write, prompt, allow, danger-full-access) are not edited. teaagent.approval.manager re-exports both names, so PermissionMode keeps one class object across teaagent.approval, teaagent.policy and the facade. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: pinned tests (tests/policy/test_permission_matrix.py tests/test_full_access_gate.py tests/acceptance/test_plan_mode_read_only_flow.py tests/test_mode2_tui_chat.py tests/test_policy_routing.py tests/approval tests/test_circular_imports.py tests/test_policy.py) -> 244 passed; approval subset (-k approval or deny or policy or permission or multisig or jit or skill or symlink) -> 1342 passed, 3 skipped; AST move-equivalence -> 0 findings; dir() parity 51/51; ruff check and ruff format clean; mypy teaagent/approval -> no issues; structural checks pass Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
Pure move (A-P1-6 step 4, ADR-0044): JITApprovalState, JITApprovalManager and _JIT_APPROVAL_TIMEOUT_SECONDS move unchanged to approval/_jit.py. ApprovalManager still builds JITApprovalManager through the facade name. ruff.toml gets the same T201 (print) exemption for _jit.py that approval/manager.py already has; the moved prompt code is otherwise byte-identical. The module-level __annotations__ attribute is no longer created in manager.py, so dir() loses that one dunder; the name stays reachable as an attribute. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: pinned tests (tests/test_efx003_one_time_approval.py tests/test_approval_token_exactness.py tests/test_policy_jit.py tests/test_phase5_jit_approval_server.py tests/test_phase6_jit_server.py tests/test_approval_queue_persistence.py tests/approval tests/test_circular_imports.py tests/test_policy.py) -> 196 passed; approval subset (-k approval or deny or policy or permission or multisig or jit or skill or symlink) -> 1342 passed, 3 skipped; AST move-equivalence -> 0 findings; importability parity 51/51 (dir() only drops the lazily created __annotations__); ruff check and ruff format clean; mypy teaagent/approval -> no issues; structural checks pass Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
Pure move (A-P1-6 step 5, ADR-0044). The class body is unchanged and teaagent.approval.manager re-exports it, so the lazy teaagent.approval map and every caller keep the same class object. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: pinned tests (tests/test_deny_preset_runtime_enforcement.py tests/test_approval_token_exactness.py tests/test_auto_mode_authority_audit.py tests/approval tests/test_circular_imports.py tests/test_policy.py) -> 170 passed; approval subset (-k approval or deny or policy or permission or multisig or jit or skill or symlink) -> 1342 passed, 3 skipped; AST move-equivalence -> 0 findings; importability parity 51/51; ruff check and ruff format clean; mypy teaagent/approval -> no issues; structural checks pass Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
Pure move (A-P1-6 step 6, ADR-0044): PeerSignature, MultiSigQuorumConfig, PeerApprovalRequest and its deprecated ApprovalRequest alias, MultiSigQuorumManager, _SSH_VERIFICATION_IMPLEMENTED and _verify_ssh_signature move unchanged. The module keeps the logger name teaagent.approval_manager. The MultiSigQuorumManager instance attribute and method names are unchanged, so the instance-level monkeypatches in the fallback and policy tests still apply. ruff.toml gets the T201 (print) exemption for _multisig.py, matching manager.py. One patch target changes scope: tests/test_sec_tier1_hardening.py patches teaagent.approval_manager._verify_ssh_signature. _collect_peer_signatures now resolves that name from _multisig, so the patch no longer reaches it. The test's stale-signature path returns before signature verification, so its assertion and result are unchanged and no test is edited. Recorded here for the review gate. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: pinned tests (tests/test_approval_manager_multisig_fallback.py tests/test_sec_tier1_hardening.py tests/test_hypothesis_invariants.py tests/test_approval_coordination_backend.py tests/test_remediation_p1_p2.py tests/test_peer_approval_request_alias.py tests/test_approval_async_from_sync.py tests/test_policy_multisig_archived.py tests/test_governance_hardening.py tests/test_policy.py tests/approval tests/test_circular_imports.py) -> 259 passed; approval subset (-k approval or deny or policy or permission or multisig or jit or skill or symlink) -> 1342 passed, 3 skipped; AST move-equivalence -> 0 findings; importability parity 51/51; ruff check and ruff format clean; mypy teaagent/approval -> no issues; structural checks pass Confidence: medium Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
…l_protection Pure move (A-P1-6 step 7, ADR-0044): _PROTECTED_SKILL_PATTERNS, _CANDIDATE_SKILL_PREFIX, is_protected_skill_path and _is_skill_dev_opt_in move unchanged to approval/_skill_protection.py (DSK-P0-002, fail-closed on OSError from G-P1-11). teaagent.approval.manager re-exports all four. Deviation from plan Step 7: ApprovalManager._assert_skill_path_not_protected stays in manager.py instead of moving into a _SkillProtectionMixin. Its body reads _is_skill_dev_opt_in from the manager module, and tests/test_approval_fail_closed_oserror.py monkeypatches teaagent.approval.manager._is_skill_dev_opt_in. Moving the method would make that patch silently stop applying, so the method stays where the patch reaches it. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: pinned tests (tests/test_skill_write_quarantine.py tests/test_skill_lifecycle.py tests/test_check_high_risk_paths.py tests/test_approval_fail_closed_oserror.py tests/approval tests/test_circular_imports.py tests/test_policy.py) -> 216 passed; approval subset (-k approval or deny or policy or permission or multisig or jit or skill or symlink) -> 1342 passed, 3 skipped; AST move-equivalence -> 0 findings; importability parity 51/51; ruff check and ruff format clean; mypy teaagent/approval -> no issues; structural checks pass Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
…nt mixin Pure move (A-P1-6 step 8, ADR-0044). The containment methods and the _PATH_ARGUMENT_KEYS attribute move unchanged from ApprovalManager into _PathContainmentMixin (approval/_path_containment.py), and ApprovalManager inherits from it. Method names, staticmethod status and self semantics are unchanged. The fail-closed rules are not edited: containment first, symlink components raise SYMLINK_BLOCKED, OSError on the symlink check still denies (G-P1-11), and tenant mismatch still raises. The mixin adds three bare class-level annotations (workspace_root, tenant_id, _extra_path_keys). They have no value and create no runtime attribute; they exist so mypy can type the attributes ApprovalManager.__init__ sets. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: pinned tests (tests/test_ws3_schema_path_containment.py tests/test_approval_manager_symlink_check.py tests/test_tenant_isolation.py tests/integration/test_sec13_security_paths.py tests/test_prompt_injection_boundaries.py tests/test_governance_fuzz.py tests/approval tests/test_circular_imports.py tests/test_policy.py) -> 243 passed; approval subset (-k approval or deny or policy or permission or multisig or jit or skill or symlink) -> 1342 passed, 3 skipped; AST move-equivalence -> 0 findings (the only HEADER change is the ApprovalManager base class); importability parity 51/51; ruff check and ruff format clean; mypy teaagent/approval -> no issues; structural checks pass Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
Adds the A-P1-6 row and in-progress tally to the action register and regenerates the docs inventory, aging dashboard and OKF bundles. Repoints two line-number citations in docs/roadmap-status.md that the split moved (check_preset now in approval/_preset_scope.py; the deny raise stays in ApprovalManager.assert_allowed) and updates the G5 corpus counts to 684 Markdown files, 683 inventoried (the inventory rule: total = inventoried + 1). The god-module exemption for approval/manager.py is NOT removed: ADR-0044 is Proposed and says removal waits for its acceptance. Action: A-P1-6 Gate: owner-override (owner decision 2026-10-27: split approval/manager.py after the deny fix) Constraint: pure move; no behavior change; teaagent.approval.manager re-exports every prior name Tested: python scripts/validate_docs_consistency.py -> passed; python -m pytest -q tests/test_docs_consistency.py -> 57 passed (after the G5 row fix); check_god_modules -> OK with the exemption kept Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
review-institution G4 requires docs/reviews/<PR>-risk.md for a PR declared high-risk. This is the approval-split risk report under the PR-numbered name; the human review gate stays open. The G5 corpus count is updated. Action: A-P1-6 Gate: governance-gap (review-institution G4 risk report on a high-risk PR) Constraint: docs only; human review gate left open Tested: validate_docs_consistency.py passed; tests/test_docs_consistency.py passed Confidence: high Roadmap-Status: updated G5 corpus count (no horizon status moved) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
The split kept asyncio, concurrent.futures, hashlib, os, re, shlex, sys, time, dataclass, field, Enum and Optional in manager.py with noqa for dir() parity. No caller imports them from the facade and no test patches them there (the code that used them moved), so they are dead imports; the code-quality bot flagged them. The moved private constants stay re-exported: tests import _SSH_VERIFICATION_IMPLEMENTED via the facade and the facade-surface test pins them. Action: A-P1-6 Gate: governance-gap (review-bot finding: unused imports introduced by the split) Constraint: re-exported names unchanged; __all__ unchanged; no behavior change Tested: ruff and mypy (teaagent/approval) clean; check_circular_imports passes; approval subset 1342 passed, 3 skipped; facade-surface, migration-compatibility and the two _SSH_VERIFICATION_IMPLEMENTED tests 117 passed Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
The code-quality bot flagged the remaining parity imports. Of these, _CANDIDATE_SKILL_PREFIX, _PROTECTED_SKILL_PATTERNS and read_only_runtime_block_reason have no importer outside the approval package (the read-only acceptance test imports the gate helper from teaagent.read_only_gate), so they are removed. _JIT_APPROVAL_TIMEOUT_SECONDS (pinned by the facade-surface test) and _SSH_VERIFICATION_IMPLEMENTED (imported by two tests via teaagent.approval_manager) stay, written as explicit "name as name" re-exports instead of noqa. Action: A-P1-6 Gate: governance-gap (review-bot finding: unused imports introduced by the split) Constraint: __all__ unchanged; every name a test or caller imports from the facade still resolves Tested: ruff check, ruff format, mypy (teaagent/approval) clean; check_circular_imports passes; approval subset incl. read_only 1398 passed, 3 skipped; tests/approval + the two _SSH tests + read-only acceptance flow 130 passed Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
Brings in the merged dependency and approval-hardening PRs. Ledger conflicts resolved: the action register by unioning ID rows and recounting from the rows, roadmap-status by keeping both sides' dated entries, and generated docs regenerated. Action: A-P1-6 Gate: governance-gap (merge conflict with main on ledger files) Constraint: merge only; no code change beyond main Tested: validate_docs_consistency.py passed; tests/test_docs_consistency.py passed Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
…al-split Action: A-P1-6 Gate: governance-gap (merge conflict with main on ledger files) Constraint: merge only; no code change beyond main Tested: validate_docs_consistency.py passed; tests/test_docs_consistency.py passed Confidence: high Roadmap-Status: unchanged Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JE5Q9zqTjf7w2PzjzAC2Qq
Merged
4 tasks done
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
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.
Summary
Why
The owner decided on 2026-10-27 to split
teaagent/approval/manager.pyonce the deny-terminal fix (#107) had merged. Both preconditions are now met: #107 is merged, and so is the plan (#105). The layout follows ADR 0044 (Proposed): underscore-private modules, withteaagent.approval.manageras the stable facade.teaagent/approval/is a high-risk path, so a human reviewer signs off before merge.What (one pure move per commit)
_shell_normalize.py(_normalize_shell_arg)_denial_format.py(format_denial_message)_permission_mode.py(PermissionMode,PermissionModeEnforcer)_jit.py(JITApprovalState,JITApprovalManager, timeout)_preset_scope.py(ApprovalStoreManager)_multisig.py(PeerSignature,MultiSigQuorumConfig,PeerApprovalRequest+ alias,MultiSigQuorumManager, SSH verify)_skill_protection.py_path_containment.py(_PathContainmentMixin;ApprovalManagerinherits it)manager.py(facade +ApprovalManager)Other changes:
tests/approval/test_manager_facade_surface.pypins__all__, the private facade names, and identity throughteaagent.approval,teaagent.policyand theteaagent.approval_manageralias.roadmap-statuscitations repointed: they citedmanager.pylines that no longer exist.Evidence
dir(teaagent.approval.manager)are still importable after every step.__all__is unchanged.ast.dumpof every moved definition againstorigin/mainfound 0 blocking differences at every step. The only non-blocking one is the expected new base class in Step 8.check_any_count(2170 ≤ 2177), complexity, docstrings, runner invariants, god modules, mypy and ruff.For the reviewer
tests/test_deny_preset_terminal_over_grants.py.ApprovalManager._assert_skill_path_not_protectedstays inmanager.py.tests/test_approval_fail_closed_oserror.pymonkeypatchesteaagent.approval.manager._is_skill_dev_opt_in, and the patch would stop applying if the method moved.tests/test_sec_tier1_hardening.pypatchesteaagent.approval_manager._verify_ssh_signature, but_collect_peer_signaturesnow resolves that name in_multisig, so the patch no longer reaches it. The test's stale-signature path returns before any verification, so its result is unchanged. It is worth retargeting the patch in a follow-up.manager.pykeeps the now-unused stdlib imports and moved constants, marked# noqa: F401, sodir()parity holds.ruff.toml: T201 (print) exemptions added for_jit.pyand_multisig.py, matching the existingmanager.pyexemption.manager.pywaits for owner acceptance of ADR 0044. All modules are already under 800 lines, andcheck_god_modulespasses with the exemption removed.Action ID
Risk Class
Risk class: high
Self-Review Checklist
General (all PRs)
ruff check+ruff format --checkpassmypy teaagent/reports 0 issues (teaagent/approval)python3 scripts/run_test_tier.py --tier smokepasses (full suite run)check_root_module_count.py≤ 184check_complexity.py≤ 99High-Risk (if risk class = high)
reflective-riskreport attached (docs/reviews/approval-split-risk.md; a per-PR copy is added for G4)Documentation
Generated by Claude Code