Restructure aai_cli into a layered package (commands > app > ui > core)#156
Merged
Merged
Conversation
The flat package root mixed five concerns and forced .importlinter contract 1
to hand-maintain a 47-module `forbidden` list that the comments admitted had
"silently drifted". Replace it with a declarative `layers` contract by giving
each layer its own package:
- app/ orchestration / shared run-logic (context, transcribe_*, init_exec,
setup_exec, doctor_checks, coding_agent, mediafile)
- ui/ Rich rendering (output, render, theme, steps, follow, help_text,
typer_patches, update_check)
- core/ the Rich-free library layer (client, config, environments, errors,
llm, telemetry, debuglog, and the leaf utilities)
The CLI framework glue (main, command_registry, help_panels, options) stays at
the package root above the command layer, and the vertical feature slices
(agent, tts, streaming, code_gen, init, auth, onboard) stay put — they mix
protocol + rendering internally, so a `forbidden` edge (contract 2) keeps them
off the command layer instead.
Each layer is a single package, so intra-layer imports stay free and only the
cross-layer direction is enforced. Contracts collapse from two drift-prone
enumerated lists (47 + 15 modules) to a 4-line `layers` contract plus a short
feature-slice list; "no Rich below the UI layer" stays explicit (contract 4)
since Rich is external. Rewrote test_importlinter_coverage to partition the
filesystem against the new contracts, added a test for the hidden _update-check
command (its moved import was the one uncovered changed line), and updated the
architecture guide. No behavior change.
Two follow-ups the check.sh gates pointed at, after the layered-package move.
core/env.py — collapse the TID251 env allowlist (11 modules → 1)
The ruff banned-api (TID251) per-file allowlist hand-enumerated ~11 modules
permitted raw os.environ — the same drift-prone "list of file exceptions" the
import-linter contract used to be. Introduce core/env.py as the single
allowlisted chokepoint (get / child_env / force_color / disable_color) and route
every other module's environment access through it; callers keep ownership of
their variable *names* (config.ENV_API_KEY, telemetry.ENV_DISABLED, …), only the
raw os.environ touch moves. The env side of the allowlist is now one structural
entry. The subprocess side stays per-module: those spawns are genuinely diverse
(sync-capture, long-lived Popen with pipes, detached children), so funnelling
them through one module would just re-export all of subprocess — documented as
such in pyproject.
app/transcribe/ — promote the transcribe_* cluster to a subpackage
The five transcribe_* siblings in app/ (spawned by the 500-line file-length gate
splitting transcribe_exec) had earned a folder by the repo's own
flat-file-then-promote convention. Move them to app/transcribe/{run,render,batch,
sources,validate}.py (gitignore gets an exception, like the templates one).
Adds tests/test_env.py, updates the architecture guide and the banned-api
messages. No behavior change; full gate green.
Every other command module is named after its command (account.py, keys.py, transcribe.py, …); config was the lone `_cmd` holdover. There was no collision to avoid — the package boundary already distinguishes aai_cli.commands.config from aai_cli.core.config, the command name comes from SPEC.group_name, and the registry discovers modules dynamically (nothing imported the file by name). Pure rename, no content change.
…helpers - Empty the four layer/grouping __init__.py (core, ui, app, app/transcribe) to match the agent/tts/streaming precedent — a no-op `from __future__` import in an otherwise-empty package init isn't meaningful. - Rename tests/setup_helpers.py -> _setup_helpers.py and tests/replay_fixtures.py -> _replay_fixtures.py so every non-test support module in tests/ shares the underscore-prefix convention (pytest won't collect them as test modules); update the three importers and the two doc references. No behavior change.
Aikido's SAST flags sidecar.write_text() in app/transcribe/batch.py as a path-traversal MEDIUM. It's a false positive: the sidecar path is derived from the user's own CLI source argument and written next to that source by design (it's the resume marker) — a local CLI has no attacker-controlled path input, and CodeQL's path-injection queries agree. Add a justification comment and an inline `# nosemgrep` rather than a behavior-changing path "sanitization" that would break resume. (The finding only surfaced because moving transcribe_batch.py -> transcribe/ batch.py reset its diff age; it was pre-existing on main.)
…irectory-structure-72ssi9
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
2 participants
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.
The flat package root mixed five concerns and forced .importlinter contract 1
to hand-maintain a 47-module
forbiddenlist that the comments admitted had"silently drifted". Replace it with a declarative
layerscontract by givingeach layer its own package:
setup_exec, doctor_checks, coding_agent, mediafile)
typer_patches, update_check)
llm, telemetry, debuglog, and the leaf utilities)
The CLI framework glue (main, command_registry, help_panels, options) stays at
the package root above the command layer, and the vertical feature slices
(agent, tts, streaming, code_gen, init, auth, onboard) stay put — they mix
protocol + rendering internally, so a
forbiddenedge (contract 2) keeps themoff the command layer instead.
Each layer is a single package, so intra-layer imports stay free and only the
cross-layer direction is enforced. Contracts collapse from two drift-prone
enumerated lists (47 + 15 modules) to a 4-line
layerscontract plus a shortfeature-slice list; "no Rich below the UI layer" stays explicit (contract 4)
since Rich is external. Rewrote test_importlinter_coverage to partition the
filesystem against the new contracts, added a test for the hidden _update-check
command (its moved import was the one uncovered changed line), and updated the
architecture guide. No behavior change.