test: pin globals a rebound component impl may load (#9534)

[satyam-thakur]

Problem / Motivation

bind_component_globals in src/kiro_crew/subagent_manager/_component.py rebuilds every
function whose name ends in _impl with FunctionType(code, vars(subagent)). The function
keeps its own code but runs on subagent.py's globals. So an import at the top of the module
that defines the function does nothing for it. The name has to exist in subagent.py.

Nothing checked that. A rebound function can load a global subagent.py does not define. It
imports fine and it reads fine in review. It raises NameError only when that line runs.

Why it matters

Several of these implementations load a global inside a try: whose except Exception denies.
The NameError is swallowed. The miss then looks like a gate that refuses everything, not like
an error.

#7843 hit this twice in five days. First HOOK_EVENT_PRE_TOOL_USE, then _should_block_results.
Both times a PreToolUse security gate read as working while it denied every call without ever
consulting a hook.

What changed (motivation → approach → change)

The symptom is a silent deny. The cause is a name that resolves nowhere, and nobody looks until
the line runs.

Two fixes were on the table. Check at bind time, or check in a test. Bind time adds work to every
start and still only raises after the code has shipped. A test catches it before it ships and
costs nothing at runtime. So the change is a test.

The test walks the bytecode of every rebound *_impl, collects each global it loads including the
ones in nested bodies, and asserts the name resolves in subagent.py or in builtins. Bytecode,
not co_names: co_names also holds attribute names, which would report dozens of names that are
not globals. main is clean today, so this is a regression test and no runtime behaviour changes.
The docstring on bind_component_globals now states the consequence, so the next person adding an
import to one of those modules reads it right there.

Tests

New test/test_subagent_component_globals.py, three tests:

• test_rebound_implementations_only_load_globals_subagent_defines — the sweep. Every global a
  rebound *_impl loads resolves in subagent.py or in builtins. Goes red on both fix(agents): enforce PreToolUse deny on subagent/task-runner paths #7843 names.
• test_component_implementations_are_rebound_onto_subagent_globals — proves the sweep had
  something to sweep. If the rebind ever stops happening, the sweep would pass while checking
  nothing. This test fails instead.
• test_the_sweep_reports_a_global_subagent_does_not_define — proves the detector sees an
  unresolved name inside a nested function, so the recursion into co_consts is covered.

Runs in about 4 seconds.

Manual verification

N/A — unit coverage sufficient. There is no UI surface and no runtime behaviour change: the only
source edit is a docstring. The test leaves nothing behind — no child process, no chdir, no
writes outside pytest's own tmp dir — and the repo tree is clean after a run.

Related Issues

Fixes #9534

Pattern harvest

Rule candidate: review-prompt
Pattern: a function rebound onto another module's globals loads a name that module does not define

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[satyam-thakur]

Intent: Stop a rebound *_impl from loading a global subagent.py does not define, by proving at test time that every such name resolves. The class already shipped twice inside a try:/except Exception that turned the NameError into a silent deny (#7843), so the goal is a guard that fails loudly and early, not a runtime change.
Not a goal: Changing any runtime behaviour, altering how bind_component_globals rebinds, or fixing a live break — main is clean today, so this is a regression test plus the docstring that tells the next reader why the import has to live in subagent.py.

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of a6ee524d9f1e08756a9ebad4b16c382fe3298cae via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A recurring, evidence-backed silent-failure class (#7843, twice) pinned by a zero-runtime-cost static sweep at the single call site — sound and proportionate.

[DESIGN-REVIEWED] a6ee524

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed a6ee524d9f1e08756a9ebad4b16c382fe3298cae via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] a6ee524

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of a6ee524d9f1e08756a9ebad4b16c382fe3298cae via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The evidence is consistent: the rebind mechanism exists at exactly one site, the #7843 names live in hooks.py (not subagent.py), and admission.py:890 already carries an in-tree comment ("Imported HERE, not at module scope: bind_component_globals rebinds…") corroborating the defect class. No existing test does this sweep (grep LOAD_GLOBAL|get_instructions in tests: 0 hits doing this job; co_names appears in 2 unrelated assertions). No new runtime surface ships — one docstring plus a test file.

First-Principles-Verdict: PASS

Verify #7843 records the two silent-deny NameErrors; the tree corroborates the class only indirectly (admission.py:890's imported-inside-the-function comment).

What this change ships

Inventory (4 items) — 4 justified

Intent: stop a rebound *_impl from shipping a global that resolves nowhere and reads as a gate denying everything — an ADDITION (regression test; base is clean).

1. CI now fails when a rebound implementation loads a name subagent.py doesn't define — justified
2. A guard test fails if the rebind stops happening, so the sweep can't pass vacuously — justified
3. A self-test proves the detector sees names inside nested function bodies — justified
4. bind_component_globals docstring now states the resolve-in-namespace consequence — justified

Duplication and siblings counted: one FunctionType( rebind site in src/ (grep: 1, _component.py:32); zero existing bytecode-sweep tests. No config key, flag, or exported symbol added.

[FIRST-PRINCIPLES-REVIEWED] a6ee524

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed a6ee524d9f1e08756a9ebad4b16c382fe3298cae via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] a6ee524