Raise code quality toward all-5 rubric through focused cleanup and guardrails #444

New Issue

2026-07-20T22:50:54-04:00

didericis commented

2026-07-20 22:50:54 -04:00

The repo is in good shape overall: strong test coverage, explicit PRDs/ADRs, fail-closed security posture, and careful token handling. The main quality gap is not lack of care; it is architectural accretion. Some runtime modules and cross-backend flows are absorbing too much feature pressure, which makes future changes riskier than they need to be.

This issue tracks a general maintenance push to move the codebase toward an all-5s quality target across architecture, readability, resilience, testability, and SecOps.

Current Concerns

Some modules are too large and mix multiple domains, especially egress policy parsing/matching/DLP behavior.
Backend boundaries leak in a few places, such as generic CLI code depending on Docker-specific utilities/types.
Some failure paths collapse different states into the same result, making operator-visible behavior harder to diagnose.
Import and module-size guardrails are mostly social conventions rather than automated checks.
Broad exception handling is sometimes appropriate, but should consistently produce typed outcomes or contextual logging in user-visible paths.

Proposed Work

Harden reconciliation behavior:
- Distinguish backend enumeration failure from zero live bottles.
- Skip reconciliation when live-container enumeration fails.
- Validate /reconcile inputs, especially grace_seconds.
Add architecture guardrails:
- Prevent generic bot_bottle.cli.* modules from importing backend-specific internals except in explicitly allowed files.
- Consider a lightweight module-size tracking gate for runtime modules.
Decompose large runtime modules:
- Split egress_addon_core.py into focused modules for policy schema parsing, route matching, DLP scanning, and policy/context resolution.
- Keep compatibility exports until callers are migrated.
Improve backend preparation boundaries:
- Extract shared preparation logic from the backend base class into focused planner/helper objects.
- Type common launch/preparation interfaces instead of relying on backend-specific plan shapes.
Improve resilience and diagnostics:
- Replace silent optional failures in operator-visible paths with typed optional-display-failed results or debug/context logging.
- Keep fail-closed behavior, but make the reason observable when safe.

Acceptance Criteria

Targeted cleanup PRs land without broad rewrites.
Existing unit/integration tests remain green.
New tests cover reconciliation failure modes and input validation.
Generic CLI code no longer depends on Docker-only internals unless explicitly exempted.
Large-module decomposition preserves existing public behavior.
Security behavior remains fail-closed and secrets are never logged, persisted, or exposed.

Non-Goals

Rewrite the project.
Add new runtime dependencies.
Change backend behavior beyond the specific cleanup being performed.
Lower or bypass existing security posture.

The repo is in good shape overall: strong test coverage, explicit PRDs/ADRs, fail-closed security posture, and careful token handling. The main quality gap is not lack of care; it is architectural accretion. Some runtime modules and cross-backend flows are absorbing too much feature pressure, which makes future changes riskier than they need to be. This issue tracks a general maintenance push to move the codebase toward an all-5s quality target across architecture, readability, resilience, testability, and SecOps. ## Current Concerns - Some modules are too large and mix multiple domains, especially egress policy parsing/matching/DLP behavior. - Backend boundaries leak in a few places, such as generic CLI code depending on Docker-specific utilities/types. - Some failure paths collapse different states into the same result, making operator-visible behavior harder to diagnose. - Import and module-size guardrails are mostly social conventions rather than automated checks. - Broad exception handling is sometimes appropriate, but should consistently produce typed outcomes or contextual logging in user-visible paths. ## Proposed Work - Harden reconciliation behavior: - Distinguish backend enumeration failure from zero live bottles. - Skip reconciliation when live-container enumeration fails. - Validate /reconcile inputs, especially grace_seconds. - Add architecture guardrails: - Prevent generic bot_bottle.cli.* modules from importing backend-specific internals except in explicitly allowed files. - Consider a lightweight module-size tracking gate for runtime modules. - Decompose large runtime modules: - Split egress_addon_core.py into focused modules for policy schema parsing, route matching, DLP scanning, and policy/context resolution. - Keep compatibility exports until callers are migrated. - Improve backend preparation boundaries: - Extract shared preparation logic from the backend base class into focused planner/helper objects. - Type common launch/preparation interfaces instead of relying on backend-specific plan shapes. - Improve resilience and diagnostics: - Replace silent optional failures in operator-visible paths with typed optional-display-failed results or debug/context logging. - Keep fail-closed behavior, but make the reason observable when safe. ## Acceptance Criteria - Targeted cleanup PRs land without broad rewrites. - Existing unit/integration tests remain green. - New tests cover reconciliation failure modes and input validation. - Generic CLI code no longer depends on Docker-only internals unless explicitly exempted. - Large-module decomposition preserves existing public behavior. - Security behavior remains fail-closed and secrets are never logged, persisted, or exposed. ## Non-Goals - Rewrite the project. - Add new runtime dependencies. - Change backend behavior beyond the specific cleanup being performed. - Lower or bypass existing security posture.

gitea-actions bot added the Status/Needs Triage label 2026-07-20 22:51:14 -04:00

didericis added

Priority

Low

4

and removed Status/Needs Triage labels 2026-07-20 23:45:08 -04:00

gitea-actions bot added the Status/Needs Triage label 2026-07-20 23:45:15 -04:00

didericis added the Kind/Enhancement label 2026-07-20 23:45:20 -04:00

didericis removed the Status/Needs Triage label 2026-07-20 23:54:54 -04:00

~~didericis referenced this issue 2026-07-24 13:03:22 -04:00~~

refactor: repo reorganization for clearer separation of concerns #477

didericis-claude referenced this issue

2026-07-24 13:34:58 -04:00

refactor: repo reorganization for clearer separation of concerns #477

didericis-claude commented

2026-07-24 19:06:21 -04:00

Refactor landed on #477 — repo reorganization for enforceable separation of concerns

Behavior-preserving groundwork for this cleanup push: no functional change, full unit suite green throughout (2243). What the branch does, by theme:

Concerns now have one obvious home (packages by domain)

bot_bottle.store — the SQLite family (DbStore base, migrations, the audit/config/queue stores, StoreManager) pulled out of the package root.
bot_bottle.gateway (the data plane) — split into per-service subpackages egress/, supervisor/, git_gate/, plus bootstrap (the PID-1 daemon manager, renamed from gateway_init; its inner _Supervisor → _DaemonManager) and the egress entrypoint. The Gateway ABC stays as the backend-neutral lifecycle contract; DockerGateway moved to backend/docker/.
bot_bottle.manifest — consolidated the manifest_* modules into a package (Manifest/ManifestIndex → manifest.index).
bot_bottle.egress / bot_bottle.git_gate — turned into concrete service classes (Egress, GitGate) in their own packages, with neutral DTO tiers (plan.py) importable by any layer.
bot_bottle.supervisor (neutral types/plan) + orchestrator.supervisor (the Supervisor service the orchestrator owns) — split the supervise plane by tier so the data plane imports only neutral types.
backend/ — split the heavy __init__ into base (contract ABCs) + selection (registry) and thinned the per-backend inits; folded run_docker into backend/docker/util (dropped the docker_cmd shim); OrchestratorService → DockerInfraService in the docker backend.
orchestrator/ — control_plane.py → server.py; registry.py → store/registry_store.py; the single-file supervisor/ package collapsed back to supervisor.py; per-service store managers.
cli/commands/ — the twelve subcommand handlers grouped into a package; help promoted to a first-class command; the dispatcher main() moved to __main__, the registry to commands/__init__; trimmed the _common junk drawer down to a constants leaf.

One name per thing

Unified control_plane / control_auth → orchestrator across all code — modules, classes, identifiers, env vars (BOT_BOTTLE_ORCHESTRATOR_TOKEN / _AUTH_JWT), the wire header, and the on-disk token filename. The plane vocabulary ("control plane", "data plane") is now reserved for prose and the security argument, not for identifiers. The forthcoming host-side control server (#468) is named as its own tier — never a second "control plane".

Import cost

Converted the remaining heavy __init__ modules to lazy __getattr__ facades / thin registries: backend 32→2, orchestrator 27→2, cli.commands 78→23 (plus manifest / egress / git_gate). This also removed a real drag — the CLI's migration gate was transitively pulling the whole docker backend and an HTTP server on every command.

Dead code / latent-bug cleanup

Dropped the dead standalone-gateway path (--gateway, Orchestrator.ensure_gateway, the gateway ctor arg) and fixed the latent shared-CA read it surfaced (it targeted a container name the consolidated flow never creates → now reads bot-bottle-infra).
Removed obsolete supervise queue helpers and dead flat-fallback shims; trimmed dead constants (REPO_DIR) and pointless re-exports from the CLI.

Next on this branch: the substantive backend-auth uniformity work (#476), now that each concern has a single home to enforce it in.

## Refactor landed on #477 — repo reorganization for enforceable separation of concerns Behavior-preserving groundwork for this cleanup push: no functional change, full unit suite green throughout (2243). What the branch does, by theme: ### Concerns now have one obvious home (packages by domain) - **`bot_bottle.store`** — the SQLite family (`DbStore` base, `migrations`, the `audit`/`config`/`queue` stores, `StoreManager`) pulled out of the package root. - **`bot_bottle.gateway`** (the data plane) — split into per-service subpackages `egress/`, `supervisor/`, `git_gate/`, plus `bootstrap` (the PID-1 daemon manager, renamed from `gateway_init`; its inner `_Supervisor` → `_DaemonManager`) and the egress entrypoint. The `Gateway` ABC stays as the backend-neutral lifecycle contract; `DockerGateway` moved to `backend/docker/`. - **`bot_bottle.manifest`** — consolidated the `manifest_*` modules into a package (`Manifest`/`ManifestIndex` → `manifest.index`). - **`bot_bottle.egress` / `bot_bottle.git_gate`** — turned into concrete service classes (`Egress`, `GitGate`) in their own packages, with neutral DTO tiers (`plan.py`) importable by any layer. - **`bot_bottle.supervisor`** (neutral types/plan) + **`orchestrator.supervisor`** (the `Supervisor` service the orchestrator owns) — split the supervise plane by tier so the data plane imports only neutral types. - **`backend/`** — split the heavy `__init__` into `base` (contract ABCs) + `selection` (registry) and thinned the per-backend inits; folded `run_docker` into `backend/docker/util` (dropped the `docker_cmd` shim); `OrchestratorService` → `DockerInfraService` in the docker backend. - **`orchestrator/`** — `control_plane.py` → `server.py`; `registry.py` → `store/registry_store.py`; the single-file `supervisor/` package collapsed back to `supervisor.py`; per-service store managers. - **`cli/commands/`** — the twelve subcommand handlers grouped into a package; `help` promoted to a first-class command; the dispatcher `main()` moved to `__main__`, the registry to `commands/__init__`; trimmed the `_common` junk drawer down to a `constants` leaf. ### One name per thing Unified **`control_plane` / `control_auth` → `orchestrator`** across all *code* — modules, classes, identifiers, env vars (`BOT_BOTTLE_ORCHESTRATOR_TOKEN` / `_AUTH_JWT`), the wire header, and the on-disk token filename. The plane vocabulary ("control plane", "data plane") is now reserved for prose and the security argument, not for identifiers. The forthcoming host-side control server (#468) is named as its own tier — never a second "control plane". ### Import cost Converted the remaining heavy `__init__` modules to lazy `__getattr__` facades / thin registries: `backend` 32→2, `orchestrator` 27→2, `cli.commands` 78→23 (plus `manifest` / `egress` / `git_gate`). This also removed a real drag — the CLI's migration gate was transitively pulling the whole docker backend **and** an HTTP server on every command. ### Dead code / latent-bug cleanup - Dropped the dead standalone-gateway path (`--gateway`, `Orchestrator.ensure_gateway`, the `gateway` ctor arg) and fixed the latent shared-CA read it surfaced (it targeted a container name the consolidated flow never creates → now reads `bot-bottle-infra`). - Removed obsolete supervise queue helpers and dead flat-fallback shims; trimmed dead constants (`REPO_DIR`) and pointless re-exports from the CLI. **Next on this branch:** the substantive backend-auth uniformity work (#476), now that each concern has a single home to enforce it in.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: didericis/bot-bottle#444