Restructure for concurrent agent contributions: derive registration, isolate patches, split guidance#145
Conversation
…isolate patches, split guidance Remove the shared-file hotspots that force every contribution to edit the same lists, so concurrent branches stay in disjoint files: - Convention-based command registration (aai_cli/command_registry.py): every module under aai_cli/commands/ declares its own SPEC = CommandModuleSpec(panel, order, commands, group_name); main.py discovers, validates, and orders them. Adding a command no longer edits main.py's import block, add_typer list, or _COMMAND_ORDER tuple - and the help-snapshot partition (HELP_GROUPS) is now derived from the same SPECs instead of a hand-maintained parallel dict. - Wildcard the command-independence import contract (aai_cli.commands.*), which exposed two gaps: onboard and speak were missing from the enumerated list, and onboard/sections.py imported command-internal logic from commands/doctor.py and commands/setup.py. Fix the coupling for real by extracting that logic to the core layer (aai_cli/doctor_checks.py, aai_cli/setup_exec.py); both commands and the onboarding wizard now share one implementation. - Isolate every Typer/Click/Rich override in aai_cli/typer_patches.py (help palette, no-clip tables, pipe-safe consoles, Click error formatting, completion-help trim) so a dependency upgrade that breaks a patch is fixed in one file instead of in main.py traffic. - Give the root `assembly --help` screen its own snapshot module/golden: every new command changes it, so that churn now lands in one trivially-regenerable .ambr instead of inside a command group's goldens. - Count-gate the noqa/type-ignore/no-cover and test skip/xfail/sleep escape hatches against the merge-base (same policy as Any/cast) so refactors that *move* an existing hatch don't false-positive while net-new hatches still fail. - Split AGENTS.md per directory (root invariants + aai_cli/ architecture + tests/ suite guide, with CLAUDE.md symlinks), add a "working alongside other agents" section (duplicate-work check, uv.lock policy, merge queue), and exclude the nested docs from the wheel. - Add .github/workflows/pr-overlap.yml: advisory warning when an open PR's changed files intersect another open PR's, so duplicated work is caught while both are still open. Full ./scripts/check.sh gate green (incl. diff-cover 100%, mutation gate, CodeQL). https://claude.ai/code/session_01Gb5xKopp2JGa1tU4i5oNb1
The forbidden contract (core modules must not import aai_cli.commands) enumerates its sources by hand, so nine modules - including onboard, which actually was violating until the doctor_checks/setup_exec extraction - were silently uncovered: choices, eval_hf_api, jsonshape, onboard, steps, timeparse, tts, typer_patches, update_check. List them all, and add tests/test_importlinter_coverage.py so a future top-level module fails loudly until contract 1 covers it (or is deliberately exempted alongside main/command_registry). Also kill the three mutation-gate survivors the first commit left (its new files were untracked during the pre-commit gate run, so git diff kept their lines out of mutation scope until they were committed): assert the registry dataclasses are actually frozen, and pin that doctor's profile/environment context line requires *both* keys - the existing partial-payload test set neither, which can't tell `and` from `or`. https://claude.ai/code/session_01Gb5xKopp2JGa1tU4i5oNb1
| def _copy_tree(node: Traversable, dest: Path) -> None: | ||
| dest.mkdir(parents=True, exist_ok=True) | ||
| for child in node.iterdir(): | ||
| if child.name == "__pycache__" or child.name.endswith(".pyc"): | ||
| continue | ||
| out = dest / child.name | ||
| if child.is_dir(): | ||
| _copy_tree(child, out) |
There was a problem hiding this comment.
Function _copy_tree calls itself recursively (unbounded). Replace with an iterative traversal or add a checked max-depth / visited tracking to prevent unbounded recursion.
Show fix
| def _copy_tree(node: Traversable, dest: Path) -> None: | |
| dest.mkdir(parents=True, exist_ok=True) | |
| for child in node.iterdir(): | |
| if child.name == "__pycache__" or child.name.endswith(".pyc"): | |
| continue | |
| out = dest / child.name | |
| if child.is_dir(): | |
| _copy_tree(child, out) | |
| def _copy_tree(node: Traversable, dest: Path, depth: int = 0, max_depth: int = 1000) -> None: | |
| if depth >= max_depth: | |
| return | |
| dest.mkdir(parents=True, exist_ok=True) | |
| for child in node.iterdir(): | |
| if child.name == "__pycache__" or child.name.endswith(".pyc"): | |
| continue | |
| out = dest / child.name | |
| if child.is_dir(): | |
| _copy_tree(child, out, depth + 1, max_depth) |
Details
✨ AI Reasoning
The code introduces a recursive helper that copies a Traversable tree by calling itself for each directory child. It's trying to copy a directory subtree. The implementation uses direct recursion without any counter, depth limit, or cycle/visited tracking, so a deeply nested directory (or a maliciously crafted Traversable with cycles) could cause unbounded recursion and a stack overflow (DoS). This is newly added in this PR, so it is an introduced risk. Fixing would be reasonably scoped: either convert to an explicit stack/queue (iterative traversal) or add an enforced maximum depth/visited set.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Remove the shared-file hotspots that force every contribution to edit the
same lists, so concurrent branches stay in disjoint files:
every module under aai_cli/commands/ declares its own
SPEC = CommandModuleSpec(panel, order, commands, group_name); main.py
discovers, validates, and orders them. Adding a command no longer edits
main.py's import block, add_typer list, or _COMMAND_ORDER tuple - and
the help-snapshot partition (HELP_GROUPS) is now derived from the same
SPECs instead of a hand-maintained parallel dict.
(aai_cli.commands.*), which exposed two gaps: onboard and speak were
missing from the enumerated list, and onboard/sections.py imported
command-internal logic from commands/doctor.py and commands/setup.py.
Fix the coupling for real by extracting that logic to the core layer
(aai_cli/doctor_checks.py, aai_cli/setup_exec.py); both commands and
the onboarding wizard now share one implementation.
(help palette, no-clip tables, pipe-safe consoles, Click error
formatting, completion-help trim) so a dependency upgrade that breaks
a patch is fixed in one file instead of in main.py traffic.
assembly --helpscreen its own snapshot module/golden:every new command changes it, so that churn now lands in one
trivially-regenerable .ambr instead of inside a command group's goldens.
escape hatches against the merge-base (same policy as Any/cast) so
refactors that move an existing hatch don't false-positive while
net-new hatches still fail.
alongside other agents" section (duplicate-work check, uv.lock policy,
merge queue), and exclude the nested docs from the wheel.
PR's changed files intersect another open PR's, so duplicated work is
caught while both are still open.
Full ./scripts/check.sh gate green (incl. diff-cover 100%, mutation
gate, CodeQL).
https://claude.ai/code/session_01Gb5xKopp2JGa1tU4i5oNb1