validate-architecture

name

validate-architecture

description

The Gilbert architectural rulebook — layer imports, concrete classes, business logic placement, AI prompts, multi-user isolation, plugins, slash commands, README freshness. Load BEFORE implementing any new service, integration, plugin, web route, AI tool, or significant feature so the rules guide the design. Also run as a post-change audit when the user says "check the rules", "validate", or "audit the architecture".

Gilbert Architectural Rules + Validation

Two modes:

• Design mode (before/during implementation): read the categories below and use them as constraints while writing code. Don't audit existing files — just keep the rules in mind so the new code lands compliant.
• Audit mode (after changes, or on explicit request): walk every category in order, run the listed greps/inspections, list violations with file:line, and fix them. Then run uv run pytest tests/ -x -q.

README freshness is part of the audit — stale docs are regressions, not deferred cleanup.

Keeping this skill current

This skill is the canonical rulebook — it must stay in sync with how the codebase actually works. Whenever you notice a rule that should be added, changed, or removed, propose the edit and require explicit user confirmation before writing it to this file.

Triggers to watch for:

• The user states a new architectural rule, convention, or constraint ("from now on …", "we should always …", "never do X here").
• A code change introduces a new layer, capability protocol, backend type, plugin contract, or naming convention not yet documented here.
• An existing rule has drifted: code consistently does the opposite, and the user confirms the code is right.
• You find a category here that's no longer enforced or relevant.
• A new category of mistake keeps coming up that isn't covered.

Procedure when a trigger fires:

1. State the proposed change in one sentence: which category, what rule, what's new/changed/removed, and the reason.
2. Show the exact diff you'd apply to this SKILL.md.
3. Ask the user to confirm. Do not edit this file without an explicit "yes" / "go ahead" / equivalent. Implicit signals don't count.
4. After applying, mention any downstream docs that may also need to change (e.g., CLAUDE.md, the matching walkthrough under docs/architecture/).

Don't bundle rule updates into an unrelated change. If you spot a rule drift while doing something else, surface it as a separate proposal and let the user decide whether to handle it now or defer.

How to run (audit mode)

1. Skim CLAUDE.md for any rules added since this skill was written.
2. Walk every category below in order. For each, run the listed greps / inspections, list violations with file:line, and fix them before moving on.
3. Run uv run pytest tests/ -x -q at the end to confirm nothing broke.
4. Report a final summary grouped by category: violations found, violations fixed, anything deferred (with reason).

Do not just flag issues — fix them. If a fix is genuinely out of scope, say so explicitly with reasoning.

Categories

1. Layer Import Violations (most critical)

The layer rules from CLAUDE.md:

interfaces/     ← depends on nothing
core/           ← depends on interfaces/
integrations/   ← depends on interfaces/
storage/        ← depends on interfaces/
web/            ← depends on interfaces/ and core/
app.py          ← composition root, may import anything
plugins         ← may only import gilbert.interfaces.* and own internals

Scan imports in each layer and flag:

• interfaces/ importing anything from core/, integrations/, storage/, or web/. Allowed: stdlib, third-party types, cross-references inside interfaces/.
• core/services/ importing from integrations/ (allowed only as a side-effect import gilbert.integrations.foo # noqa: F401 for backend registration) or from web/.
• integrations/ importing from core/services/, web/, or another integration module.
• storage/ importing from core/, integrations/, or web/.
• web/ importing from integrations/ or storage/ directly (must go through core/).
• Plugins importing from gilbert.core.services, gilbert.integrations, gilbert.web, or gilbert.storage. Plugins may only import from gilbert.interfaces.* and their own internal modules.

2. Concrete Class Violations

• isinstance checks against concrete service classes (e.g., isinstance(svc, ConfigurationService)). Must use the capability protocols from interfaces/ (ConfigurationReader, EventBusProvider, SchedulerProvider, …).
• Direct instantiation of backend classes (e.g., ElevenLabsTTS()) outside app.py. Must go through Backend.registered_backends().get("name").
• Direct import of concrete backends from integrations/ outside app.py or side-effect registration imports.

2b. Protocol Surface Completeness

A service that implements PART of a @runtime_checkable Protocol from interfaces/ but not the whole thing is silently filtered out by isinstance(svc, Protocol) checks. Voice-agent shipped a regression exactly this way: it declared config_namespace + config_category and set toggleable=True on its ServiceInfo, but never implemented config_params() / on_config_changed(). isinstance(svc, Configurable) returned False, the _build_categories walk in configuration.py skipped it, and the Settings → Services toggle silently never appeared even though the plugin loaded and the service registered.

Audit:

1. For every class … (Service) declaration in src/gilbert/core/services/, std-plugins/*/, local-plugins/*/: if the class defines ANY of config_namespace, config_category, config_params, on_config_changed, it MUST define ALL FOUR.

   grep -rln "def config_namespace\|def config_params\|def config_category\|on_config_changed" \
       src/gilbert/core/services/ std-plugins/*/*.py local-plugins/*/*.py 2>/dev/null \
   | while read f; do
       ns=$(grep -c "def config_namespace" "$f")
       cat=$(grep -c "def config_category" "$f")
       p=$(grep -c "def config_params" "$f")
       o=$(grep -c "on_config_changed" "$f")
       if [ "$ns" -gt 0 ] || [ "$cat" -gt 0 ] || [ "$p" -gt 0 ] || [ "$o" -gt 0 ]; then
         [ "$ns" -gt 0 ] && [ "$cat" -gt 0 ] && [ "$p" -gt 0 ] && [ "$o" -gt 0 ] || echo "PARTIAL Configurable: $f"
       fi
   done
   

2. If service_info() returns ServiceInfo(toggleable=True, …), the class MUST also satisfy the full Configurable Protocol — otherwise the toggle never appears in Settings → Services and the user can't enable the service from the UI.

Same shape applies to other partial-Protocol traps: ToolProvider (tool_provider_name + get_tools + execute_tool must all be present), WsHandlerProvider, ConfigActionProvider, etc. If you see one method of a Protocol's surface but not its siblings, that's the smell.

3. Duck-Typing and Private Access

• getattr(obj, "method", …) to probe service capabilities — must be an isinstance check against the appropriate @runtime_checkable Protocol.
• Private attribute access (obj._field) across module boundaries.
• # type: ignore comments — each is suspicious. Most can be replaced with isinstance guards, str() wrapping for numeric conversions, or if isinstance(item, dict) filtering in comprehensions.

4. Business Logic in Wrong Layer

• Web routes doing authorization logic, AI prompt construction, backend resolution, or third-party API URL building. Routes only parse, dispatch, and format.
• Shared constants/mappings defined in core/, integrations/, or web/ while used by multiple layers — they belong in interfaces/ (see interfaces/knowledge.py:EXT_TO_DOCUMENT_TYPE, interfaces/acl.py defaults).
• Multiple services advertising the same capability instead of one aggregator. When a capability has N implementations (TTS providers, presence sources, …), build one service that holds N backends internally and exposes the merged result. Don't register N services with the same capability and expect callers to combine them. Pattern model: TTSService wrapping TTSBackend.

5. Hardcoded AI Prompts

Every AI prompt MUST be a ConfigParam(multiline=True, ai_prompt=True) on the owning service with the bundled string as default, and the call site MUST read self._foo_prompt (cached in on_config_changed).

• Grep for system_prompt= and Message(role=MessageRole.SYSTEM across src/gilbert/core/services/, src/gilbert/integrations/, std-plugins/, local-plugins/. Any literal string on the right is a violation (short connection-test probes excepted).
• Grep for _DEFAULT_*PROMPT constants. Each must be the default= of a ConfigParam(ai_prompt=True) on the same service, and must NOT be referenced at the call site.
• Backend wrappers must forward ai_prompt=bp.ai_prompt when wrapping backend_config_params() into the parent service's params (ai.py, tts.py, ocr.py, vision.py, lights.py, shades.py, speaker.py, users.py, knowledge.py, thermostat.py, auth.py, doorbell.py, websearch.py, presence.py, music.py, tunnel.py).
• Dead config fields — ConfigParam(ai_prompt=True) whose value is never consumed in any AI call. Either wire it or remove it.

6. Multi-User Isolation

Services are singletons across every user and in-flight request. Per-request state stored on self races under concurrent users and leaks data between conversations.

• Grep self\._current_|self\._active_|self\._pending_ in src/gilbert/core/services/ and std-plugins/*/. For each hit, decide whether the value is service-lifetime (config, backend handles — fine) or request-scoped (user id, conversation id, active turn — must move to a ContextVar in gilbert.core.context).
• Any attribute set inside a request handler and read after an await is suspicious. Two concurrent requests interleave during the await.
• Request-scoped identity (user id, conversation id, correlation id, trace context) lives in ContextVars in gilbert.core.context. Reads through get_current_*(), writes through set_current_*() at entry points only.
• Parallel asyncio.Tasks spawned inside request handlers must pass context=contextvars.copy_context(). Grep asyncio.gather, asyncio.create_task, asyncio.Task — any instance without context= invoked in a request path needs review.
• Global locks protecting per-target resources (e.g., one _announce_lock covering every speaker) serialize unrelated work. Gate by dict[target_id, asyncio.Lock] keyed appropriately.
• Module-level mutable state (_current_session: dict = {} at module scope, populated by one request and read by another) has the same failure mode as instance attributes — treat it the same way.
• Tool handlers read caller identity from injected _user_id / _conversation_id arguments, not from self or global context.

7. Plugin-Specific Checks

• Plugin resolves dependencies via concrete imports instead of resolver.require_capability() / resolver.get_capability().
• Plugin reads config via context.config instead of implementing Configurable and using the ConfigurationReader protocol.
• Plugin accesses _private attributes on resolved services.
• Plugin Service class that provides tools does not declare slash_namespace — every tool-providing plugin Service must set a short, user-friendly namespace rather than relying on the directory-name fallback. Grep class .* \(Service\) in std-plugins/ and local-plugins/ and verify each has slash_namespace = "...".

8. Slash Command Violations

• Tools without slash_command — audit every ToolDefinition(...) in src/gilbert/core/services/, src/gilbert/integrations/, and plugin trees. Tools should set slash_command="..." unless they hit a documented exception (raw HTML/multi-line required inputs, opaque-ID-only inputs, complex structured arrays/objects, mid-AI-turn callbacks).
• Missing slash_help on tools that declare slash_command — every exposed command needs a one-line slash_help for autocomplete. Terse hint, not a duplicate of description.
• Multi-tool services that don't use slash_group — three or more slash-enabled tools that are conceptually related should collapse under a slash_group.
• Parameter order hostile to shell use — if a tool's first positional is rarely supplied (e.g., limit before query), reorder so the most-commonly-set ones come first.
• Non-identifier slash_command / slash_group values — must match [a-zA-Z][a-zA-Z0-9_\-]*. Dots are reserved for plugin namespacing; spaces for group/subcommand composition. Enforced by tests/unit/test_slash_command_uniqueness.py.
• Duplicate (slash_group, slash_command) pairs across core tools. Same leaf name under different groups (/radio stop vs /speaker stop) is allowed.

9. UI Extension Violations (frontend)

Plugin UI lives inside the plugin's own directory; core SPA never imports plugin-specific code.

• Plugin imports in frontend/src/ — grep for plugin-specific identifiers and any import .* from "@/(types|api|components)/<plugin-specific-name>". Anything that names a specific plugin (a service, component, or type the plugin owns) is a violation. Browser plugin is the canonical "done right" example — every TS file under std-plugins/browser/frontend/.
• Plugin-specific WS RPC bindings on core useWsApi — every entry in frontend/src/hooks/useWsApi.ts must be generic chat/dashboard/settings infrastructure OR a non-plugin core service (agent, notifications, mcp). Plugin RPCs (browser.*, slack.*, sonos.*, …) belong in a per-plugin <plugin>/frontend/api.ts exporting useFooApi() over the underlying rpc() from useWebSocket.
• Plugin-specific React types on core's frontend/src/types/ — same rule. BrowserCredential-style types live in <plugin>/frontend/types.ts.
• Plugin-specific panels mounted via hardcoded conditionals in core pages ({current.name === "Browser" ? <BrowserCredentialsPanel /> : null}). Replace with <PluginPanelSlot slot="..."> and register the component via Plugin.ui_panels() + a side-effect <plugin>/frontend/panels.ts calling registerPanel.
• Core pages without extension slots in obvious places — header (header.widgets, header.user-menu), dashboard (dashboard.top/bottom), per-page sidebars and toolbars. New core pages should drop a <PluginPanelSlot slot="<page-name>.toolbar"> even if no plugin uses it yet. Empty slots cost nothing.
• Plugin.ui_panels() / ui_routes() declarations with panel_id not registered in any <plugin>/frontend/panels.ts — silent dead code. Either remove the backend declaration or add the registration.
• UI elements that depend on a feature but don't declare requires_capability — anything visible in the UI (nav items, dashboard cards, routes, menu entries, settings pages) whose backing service may be absent MUST carry a runtime gate. The user shouldn't see "Cameras" in the navbar if no camera service is running. Sample: core/services/web_api.py dashboard-group entries take a "requires_capability": "<name>" field; nav items in frontend/src/components/layout/nav-shared.ts and route definitions in frontend/src/App.tsx should hide entries whose capability isn't advertised. Audit: grep dashboard-group / nav / route definitions for feature names and confirm each carries requires_capability (or a frontend-side useHasCapability(...) gate).

10. AI Backend Visibility

Only AIService, AI profiles, and the chat UI know about AI backends. No service caller should reference backend names, model IDs, or backend classes directly. Audit consumer services for hardcoded model strings ("claude-...", "gpt-...") or imports of concrete AI backend classes.

11. Capability Wiring (declared vs consumed)

Every string passed to resolver.get_capability("…") / resolver.require_capability("…") / resolver.get_all("…") must appear in some service's ServiceInfo.capabilities=frozenset({…}). A consumer asking for "ai" when the service advertises "ai_chat" silently returns None — the calling code's isinstance gate falls through to "service unavailable" and the feature only breaks when the dead code path runs.

python3 .claude/skills/validate-architecture/check_capabilities.py

Anything reported is a bug — fix at the call site (consumer asking for the wrong name) or at the declaration (service forgot to advertise the cap the consumer needs). Lookups through a variable (get_capability(self._cap_name)) can't be checked statically and are skipped. The inverse direction (capabilities declared but never consumed) is not flagged: too many caps are identification-only (plugin slugs, framework-iterated ws_handlers / ai_tools, frontend RPC requires_capability fields).

12. Cross-Service Integration (capability discovery, not hardcoded references)

When service A needs data, prose, or behavior from service B, the integration must go through a capability protocol that any service can advertise — never a hardcoded import or stored reference to a specific consumer-known service. The greeting service refactor (docs/plans/2026-05-23-greeting-context-providers.md) is the canonical example: weather, news, and health no longer live as hardcoded _weather / _feeds / _health fields on GreetingService; instead each contributor implements GreetingContextProvider and the greeting service discovers them with resolver.get_all("greeting_context").

Anti-patterns to flag:

• Hardcoded consumer-side references on self — fields like self._weather: WeatherProvider | None, self._feeds: FeedsProvider | None, populated at startup from a specific capability lookup and used directly thereafter. Adding a new contributor requires editing the consumer. Replace with a single capability protocol that all contributors implement; consumer iterates resolver.get_all(<cap>).
• isinstance(svc, ConsumerSpecificProvider) lists in one consumer that hardcode N protocols, one per known integration. Same anti-pattern. If three+ services contribute to the same outcome (greeting context, dashboard cards, search sources, briefing sources, …), define ONE shared capability protocol they all implement.
• Per-contributor toggle flags on the consumer (include_weather: bool, include_briefing: bool, …) that gate hardcoded references. Replace with a single discovered list (enabled_<cap>_providers: list[str]) so adding a new contributor is purely additive on the consumer side.
• Consumer-side format templates for foreign data (e.g., a weather_hint_template living on the greeting service, owning the layout of weather data). Move the template onto the owning service (WeatherService owns the weather hint), or accept rendered prose from the contributor — never let the consumer hold layout/format knowledge for another service's data.
• Consumer-side per-source fetch helpers (_build_weather_blurb, _fetch_health_brief, _maybe_briefing_text on the consumer). Each one is a hidden coupling; move into the owning service behind the capability protocol.

How to audit:

• Grep src/gilbert/core/services/<consumer>.py for self._<name>: fields naming a specific other service or its protocol. Each is suspect — if there are 2+ named-specific consumer references in one service, that's the smell.
• Cross-check the file's resolver.get_capability("<name>") / resolver.require_capability(...) calls. If 3+ different names are looked up and stored as instance fields, propose a capability-protocol unification.
• Check for matching toggle/format/template configs on the consumer that name another service's domain (e.g., weather_* on greeting service).

13. Documentation Freshness

These docs are product documentation, not advisory notes. Drift is a regression to fix in the same change that caused it.

• README.md (Gilbert root) — overview, integration table, plugin system summary, getting started, configuration, dev commands. Update when configuration, startup, bundled integrations, or plugin directory structure change.
• std-plugins/README.md — canonical inventory of every plugin (table + per-plugin detail with config keys, deps, slash commands). Update in the same commit that adds/removes/renames a plugin or changes its config_params().
• std-plugins/CLAUDE.md — plugin development conventions. Update when plugin layout rules, test conventions, or workspace wiring change.
• CLAUDE.md (root) — the architecture reference. Update when layer rules, capability protocols, or the plugin contract change.

Freshness pass: after changes, grep affected READMEs for strings that may have been invalidated (plugin names, config keys, commands, paths) and update them.

Final step

Run uv run pytest tests/ -x -q. If anything fails, fix it before declaring the audit complete.