Build the plugin set a real site runs, and write down what shipped
The lean-build job proved the delete-a-plugin guarantee with two plugins and four hand-picked marker components. It now builds a real twelve-plugin set, and the assertions follow a rule instead of a hunch: one component from EVERY omitted plugin must be absent, one from EVERY chosen plugin must be present, and the backend tree is checked both ways. An omitted set that grows without its check growing is how a leak ships. CHANGELOG had not been touched across the whole range, so the largest security-behaviour change in the product - per-PC endpoint credentials, the shared-key refusal for enrolled PCs, server pinning, agent 1.20.1 and tray 1.7.1 - had no release record. An [Unreleased] section now carries it, written from the commits rather than from memory. CLAUDE.md and the agent quick-reference stated that ADR-016 bans storing credentials in the registry. It does not, and this fleet depends on HKLM for its collector key and server pins: the next person following that text removes them and silently breaks reporting and pinning on every un-enrolled PC. Corrected, along with two other statements that were false on this branch - the database is MySQL, not PostgreSQL, and column names are lowercase concatenated, not snake_case, which is the opposite of what the gate enforces. The wiki drafts are updated against the code as it stands, each old finding given a verdict with a file and line rather than left as an assertion from a week ago, and a new page records the ADR conformance result - including that the ADR set itself is not on this branch, so nothing here enforces it except the naming and style script. They are also scrubbed of the things that must not reach a published wiki: the site name, the internal git server, developer home paths and the assistant tooling. They now pass the repository's own publishability patterns, which is what the scrub is for.
This commit is contained in:
cproudlock
@@ -1,443 +1,405 @@
|
||||
# Architectural Findings - September 11, 2026
|
||||
|
||||
**Scope:** Full codebase analysis via Gortex semantic code intelligence
|
||||
**Severity:** 22 findings (6 critical, 6 high, 10 medium)
|
||||
**Status:** Documented for remediation planning
|
||||
**Scope:** Full codebase analysis via Gortex semantic code intelligence
|
||||
**Original severity claim:** 22 findings (6 critical, 6 high, 10 medium)
|
||||
**Status:** RE-CHECKED against branch gh-wj at 35d4fcd41 on 2026-09-18. Most of
|
||||
the original findings do not survive a read of the code. This page keeps them,
|
||||
with the verdict and the evidence, so nobody acts on a claim that was never
|
||||
true.
|
||||
|
||||
## Executive Summary
|
||||
## How to read this page (added 2026-09-18)
|
||||
|
||||
Gortex semantic analysis identified architectural issues in plugin boundaries, dependency management, and data schema consistency. Most critical: circular dependencies between plugins violate the plugin isolation model, and schema drift between SQLAlchemy models and Alembic migrations creates deployment risks.
|
||||
The September 11 analysis was written from semantic queries, not from reading
|
||||
the files it names. Eight of its claims cite code that does not exist on this
|
||||
branch: a `machines` plugin that imports `computers`, a migration that spells a
|
||||
column `last_reported_date`, a ServiceNow plugin, `MACHINE_TYPES = ['Lathe',
|
||||
'Mill', 'CNC']`, a hardcoded `['Bldg 300', 'Bldg 400']`, Vuex, an absent CSP
|
||||
header, an unhandled `requests.post`. Each of those was grepped for on
|
||||
2026-09-18 and returns nothing.
|
||||
|
||||
**Key theme:** Plugin architecture (ADR-001, ADR-008) is well-designed but not consistently enforced. Code grew organically with cross-plugin imports, shared state leakage, and implicit coupling that violates the self-contained plugin contract.
|
||||
Every finding below now carries one of four verdicts:
|
||||
|
||||
- **REFUTED** - the cited code does not exist, or the opposite is in the tree.
|
||||
- **STALE** - it was true once; the tree has since fixed it.
|
||||
- **HOLDS (narrowed)** - something real is there, but smaller or different than
|
||||
the original text claims.
|
||||
- **UNVERIFIED** - cannot be settled from this repository.
|
||||
|
||||
Effort and priority numbers in the original text were estimates with no stated
|
||||
basis. They are left where they sit for findings that still hold and struck
|
||||
where the finding does not.
|
||||
|
||||
## Critical Findings (P0)
|
||||
|
||||
### 1. Circular Dependencies Between Plugins
|
||||
|
||||
**Risk:** Plugin load order matters. Enabling/disabling plugins can break others.
|
||||
**Verdict: REFUTED as stated. HOLDS (narrowed) as a deferred-import coupling.**
|
||||
|
||||
**Evidence:**
|
||||
- `computers` plugin imports from `machines` plugin: `from plugins.machines.models import Machine`
|
||||
- `machines` plugin imports from `computers` plugin: `from plugins.computers.models import Computer`
|
||||
- `printers` plugin imports from `computers` plugin
|
||||
- `endpoints` plugin imports from `computers`, `printers`, and `usb` plugins
|
||||
**Original claim:** `computers` imports `machines` and `machines` imports
|
||||
`computers`; import errors at runtime if plugins load in the wrong order; 25+
|
||||
violations.
|
||||
|
||||
**Impact:**
|
||||
- Violates plugin isolation (ADR-001: plugins are independent extensions)
|
||||
- Disabling `computers` breaks `machines`, `printers`, `endpoints`
|
||||
- Plugin dependency chain not declared in plugin metadata
|
||||
- Import errors at runtime if plugins loaded in wrong order
|
||||
**What the code shows (2026-09-18):**
|
||||
|
||||
**Root cause:** Plugins use each other's models for relationships (e.g., Computer controls Machine).
|
||||
- `grep -rn "^from plugins\." plugins/ --include=*.py` returns **nothing**.
|
||||
There is not one module-level cross-plugin import in the tree, so there is no
|
||||
load-order failure mode and no import cycle at import time.
|
||||
- `grep -rn "from plugins.computers" plugins/machines/` returns **nothing**.
|
||||
The `machines` plugin does not reference `computers` at all, so the cycle the
|
||||
finding is built on does not exist.
|
||||
- 52 cross-plugin imports do exist, and every one of them is inside a function
|
||||
body, resolved on call. `plugins/computers/plugin.py:414` and `:1166` reach
|
||||
`plugins.machines.models` that way; `plugins/endpoints/api/routes.py:235` and
|
||||
fourteen more reach `plugins.computers.models`.
|
||||
|
||||
**Proper pattern:** Relationship via `Asset` base class
|
||||
```python
|
||||
# BAD: Direct plugin-to-plugin import
|
||||
from plugins.machines.models import Machine
|
||||
**What is actually there:** a mutual reference pair between `computers` and
|
||||
`endpoints`, both deferred:
|
||||
|
||||
# GOOD: Relationship via Asset ID, resolve at runtime
|
||||
machine_asset_id = db.Column(db.Integer, db.ForeignKey('asset.id'))
|
||||
machine = relationship('Asset', foreign_keys=[machine_asset_id])
|
||||
- `plugins/computers/api/policy_routes.py:44` imports
|
||||
`plugins.endpoints.commands.elevated_permission_for`, unguarded.
|
||||
- `plugins/endpoints/commands.py:477` imports
|
||||
`plugins.computers.services.policyresolver.effective_policy_for`, wrapped in
|
||||
`try/except` that falls back to the base allowlist (`commands.py:475-481`).
|
||||
|
||||
# In plugin manifest, declare dependency
|
||||
dependencies = ['machines'] # Explicit, not implicit
|
||||
```
|
||||
So one side of that pair degrades when its partner is absent and the other
|
||||
raises. The same asymmetry appears at
|
||||
`plugins/endpoints/api/agentauth.py:118`, where `resolve_hostname` imports
|
||||
`plugins.computers.models.Computer` with no guard: an endpoints-without-
|
||||
computers site fails there rather than returning "unknown hostname".
|
||||
|
||||
**Remediation:**
|
||||
1. Audit all `from plugins.<other>` imports (25+ violations found)
|
||||
2. Replace with Asset-based relationships or declared dependencies
|
||||
3. Enforce via linter: plugins can import from `shopdb.api`, not `plugins.*`
|
||||
|
||||
**Effort:** 60 hours (high impact, requires refactoring multiple plugins)
|
||||
**Priority:** Q1 2027 (blocks plugin catalog ADR-013 lean builds)
|
||||
**What to do instead of the original remediation:** nothing about load order.
|
||||
The open question is narrower and worth one pass: decide whether a deferred
|
||||
cross-plugin import must be guarded, and guard the unguarded ones. A linter
|
||||
rule banning `plugins.*` imports outright would break the working policy and
|
||||
enrollment paths listed above, which are the product's actual design.
|
||||
|
||||
### 2. Undeclared Import Dependencies
|
||||
|
||||
**Risk:** Plugins import from `shopdb.core` internals instead of stable `shopdb.api` surface.
|
||||
**Verdict: REFUTED.**
|
||||
|
||||
**Evidence:**
|
||||
- 18 plugins import from `shopdb.core.models` directly (not via `shopdb.api`)
|
||||
- 12 plugins import from `shopdb.core.utils` (internal helper modules)
|
||||
- 7 plugins import from `shopdb.core.services` (not part of plugin API)
|
||||
**Original claim:** 18 plugins import `shopdb.core.models`, 12 import
|
||||
`shopdb.core.utils`, 7 import `shopdb.core.services`.
|
||||
|
||||
**Impact:**
|
||||
- Plugins break when core refactors internals
|
||||
- No API versioning or stability guarantee
|
||||
- Circular imports between core and plugins
|
||||
**What the code shows:**
|
||||
`grep -rn "from shopdb\.\(core\|utils\|extensions\|services\)" plugins/
|
||||
--include=*.py` returns **0 hits**. Not one plugin deep-imports core. The only
|
||||
`shopdb.*` import in `plugins/` outside `shopdb.api` is
|
||||
`shopdb.plugins.alembic_template` from migration `env.py` files, which is the
|
||||
shared Alembic runner ADR-008 sanctions.
|
||||
|
||||
**Plugin API contract (ADR-001):** Plugins MUST import from `shopdb.api` only
|
||||
```python
|
||||
# GOOD: Stable plugin API
|
||||
from shopdb.api import db, Asset, BasePlugin, success_response
|
||||
ADR-001's rule that `shopdb.api` is the only core surface a plugin may import is
|
||||
being kept. The thing that is NOT enforced is raw SQL against core tables: see
|
||||
finding 3a below.
|
||||
|
||||
# BAD: Internal implementation details
|
||||
from shopdb.core.models.asset import Asset # Internal path
|
||||
from shopdb.core.services.search import SearchService # Not exported
|
||||
```
|
||||
### 2a. Raw SQL against core tables from a plugin (added 2026-09-18)
|
||||
|
||||
**Remediation:**
|
||||
1. Audit imports: `grep -r "from shopdb.core" plugins/`
|
||||
2. Export needed classes via `shopdb.api.__init__.py`
|
||||
3. Update plugin imports to use `shopdb.api`
|
||||
4. Add linter rule: plugins cannot import `shopdb.core.*`
|
||||
**Verdict: HOLDS (new, narrow).**
|
||||
|
||||
**Effort:** 40 hours
|
||||
**Priority:** Q4 2026 (enables core refactoring without breaking plugins)
|
||||
`plugins/computers/plugin.py:2080-2091` runs
|
||||
`db.session.execute(db.text(...))` joining core `assets`, `assetrelationships`
|
||||
and `assettypes` inside the `flask computers retype` CLI command. `Asset`,
|
||||
`AssetType`, `AssetRelationship` and `RelationshipType` are all exported from
|
||||
`shopdb.api` (`shopdb/api/__init__.py:47-48`, `:424`, `:438-439`), so no
|
||||
missing export excuses it, and `plugins/endpoints/api/routes.py:3981-3986` is
|
||||
the same pattern already corrected for `BusinessUnit`. The contract note at
|
||||
`shopdb/__init__.py:135-142` names the reason: a raw query is coupling to the
|
||||
core schema that no contract bump can catch.
|
||||
|
||||
Blast radius: one admin CLI command, no serving route. Import rule not
|
||||
literally broken. Worth fixing because it is the kind of thing that gets copied.
|
||||
|
||||
### 3. Schema Drift: Models vs Migrations
|
||||
|
||||
**Risk:** SQLAlchemy models don't match Alembic migrations. Deployed schema differs from code.
|
||||
**Verdict: REFUTED as stated. HOLDS (narrowed) to one column.**
|
||||
|
||||
**Evidence (examples):**
|
||||
- `Computer.lastreporteddate` column in model, but migration creates `last_reported_date` (snake_case mismatch)
|
||||
- `Printer.driver` column added to model but no migration exists
|
||||
- `AssetRelationship.label` column in migration but not in model
|
||||
- 14 tables with mismatched column names or missing columns
|
||||
**Original claim:** `Computer.lastreporteddate` in the model but
|
||||
`last_reported_date` in the migration; `Printer.driver` with no migration;
|
||||
`AssetRelationship.label` in a migration but not the model; 14 tables affected.
|
||||
|
||||
**Impact:**
|
||||
- Fresh install (runs migrations) has different schema than ORM expects
|
||||
- Queries fail: `Column 'lastreporteddate' does not exist in table`
|
||||
- Adding data via ORM writes to wrong column
|
||||
- Impossible to tell what production schema actually is (migrations are source of truth, but models are what dev sees)
|
||||
**What the code shows:**
|
||||
|
||||
**Root cause:**
|
||||
- Naming convention changed (ADR: lowercase concatenated, no underscores)
|
||||
- Manual migrations written with old convention
|
||||
- No validation that migrations match models
|
||||
- `grep -rn "last_reported_date" --include=*.py .` returns **nothing**. The
|
||||
model spells it `lastreporteddate` (`plugins/computers/models/computer.py:85`)
|
||||
and so does every migration.
|
||||
- `grep -rPn "sa\.Column\(\s*'[a-z]+_[a-z_]+'" plugins/*/migrations/versions/*.py
|
||||
migrations/versions/*.py` returns **nothing**. There is no snake_case column
|
||||
name in any migration in the tree.
|
||||
- There is no `shopdb/models/` directory to hold the claimed
|
||||
`AssetRelationship.label`; core models live under `shopdb/core/`.
|
||||
|
||||
**Remediation:**
|
||||
1. Generate fresh migration from current models: `flask db migrate`
|
||||
2. Compare to existing migrations, find diffs
|
||||
3. Write fixup migration to align schema with models
|
||||
4. Add test: `test_migrations_match_models()` validates ORM vs DB schema
|
||||
5. Enforce naming convention in migration template
|
||||
**The one real instance:** `plugins/notifications/models/notification.py:158`
|
||||
declares `alertscope = db.Column(db.JSON, nullable=True)` while the migration
|
||||
that creates it, `plugins/notifications/migrations/versions/0006_add_fleet_alerts.py:45`,
|
||||
creates `sa.Column('alertscope', sa.Text, nullable=True)` and its own docstring
|
||||
at line 8 calls it "the alertscope TEXT field (JSON string)". Commit f484be8a3
|
||||
fixed the two neighbouring columns to agree with 0006 and left this one. Nothing
|
||||
breaks on MySQL 8 - the SQLAlchemy JSON type serialises into TEXT and reads back
|
||||
- and consumers already normalise both shapes
|
||||
(`plugins/endpoints/api/routes.py:4011-4020`). The cost is that the model is not
|
||||
a true description of the deployed column, which is what autogenerate reads.
|
||||
|
||||
**Effort:** 80 hours (high risk, requires careful schema analysis)
|
||||
**Priority:** Q4 2026 (CRITICAL - blocks confidence in production schema)
|
||||
**Related gap that is real:** the naming gate cannot see migrations at all.
|
||||
`scripts/check-naming-and-style.sh:236` greps
|
||||
`^\s+[a-z]+_[a-z_]+\s*=\s*db\.Column`, which only matches a model attribute
|
||||
assignment; migrations write `sa.Column('name', ...)` with the name as a
|
||||
positional string. That is how the shipped build of
|
||||
`plugins/endpoints/migrations/versions/0021_client_certificates.py` created
|
||||
`client_cert_fingerprint`, `cert_issued_at` and `cert_expires_at` and passed the
|
||||
gate (`git show 5cd533239:plugins/endpoints/migrations/versions/0021_client_certificates.py`,
|
||||
lines 20-28). The tree is conformant today, because 0021 and 0022 are now
|
||||
no-ops and 0023 renamed the survivor; the gate simply would not notice the next
|
||||
one.
|
||||
|
||||
### 4. Single Points of Failure
|
||||
|
||||
**Risk:** Core services have no failover or redundancy.
|
||||
**Verdict: UNVERIFIED from this repository, and partly STALE.**
|
||||
|
||||
**Evidence:**
|
||||
- Single database instance (no replication)
|
||||
- Single Flask app instance (gunicorn workers on one server)
|
||||
- Single Docker host (no orchestration)
|
||||
- No load balancing
|
||||
Replication, load balancing and orchestration are properties of the reference site
|
||||
deployment, not of this tree, so nothing here can confirm or deny them. Two
|
||||
parts of the original text are wrong about what the repo ships:
|
||||
|
||||
**Impact:**
|
||||
- Database outage = full system outage
|
||||
- App server failure = downtime until manual restart
|
||||
- Hardware failure = data loss without recent backup
|
||||
- Restart policy is configured, not absent: `docker-compose.yml:24`, `:104` and
|
||||
`:120` set `restart: unless-stopped`, and three services carry `healthcheck:`
|
||||
blocks (`:38`, `:112`, `:219`).
|
||||
- Backups are no longer "manual mysqldump weekly, stored on the same server".
|
||||
See finding 4a.
|
||||
|
||||
**Mitigation (current):**
|
||||
- Weekly manual backups (mysqldump)
|
||||
- VM snapshots before deploys
|
||||
- Docker restart on failure (docker-compose restart policy)
|
||||
The underlying point - one database, one app host, air gap limits the options -
|
||||
is a statement about the site, and belongs in a deployment document rather than
|
||||
a code findings page. The 200+ hour estimate has no basis in anything measured.
|
||||
|
||||
**Proper solution:** High availability architecture
|
||||
- Database: MySQL master-replica replication
|
||||
- App: Multiple gunicorn instances behind nginx load balancer
|
||||
- Orchestration: Docker Swarm or Kubernetes for auto-restart
|
||||
- Monitoring: Automated health checks + alerting
|
||||
### 4a. Backups (added 2026-09-18, correcting the original text)
|
||||
|
||||
**Effort:** 200+ hours (infrastructure redesign)
|
||||
**Priority:** Q2 2027 (production risk, but air-gapped environment has limited HA options)
|
||||
**Verdict: STALE. The repo ships a backup system.**
|
||||
|
||||
**Note:** Air-gap deployment model limits cloud HA solutions. On-premise HA requires hardware investment.
|
||||
`scripts/backup/` contains `backup.sh`, `restore.sh`, `status.sh`, `watch.sh`,
|
||||
`simulate-restore.sh` and `test-restore-datarev.sh`. Retention is enforced by
|
||||
restic (`scripts/backup/backup.sh:134-141`: `--keep-daily 5 --keep-weekly 4
|
||||
--keep-monthly 6 --prune`), destinations include SMB and S3 as well as local
|
||||
(`scripts/backup/README.md`), each set carries `manifest.json` with schema
|
||||
revisions for compatibility checking, and restore runs from the Settings >
|
||||
Backups UI with a schema compatibility check. A restore-simulation harness is in
|
||||
the tree.
|
||||
|
||||
### 5. Plugin Migration Chain Validation Gaps
|
||||
|
||||
**Risk:** ADR-008 (plugin migration ownership) not enforced by tooling.
|
||||
**Verdict: STALE as written, and the credited fix is the wrong artifact.**
|
||||
|
||||
**Evidence:**
|
||||
- Manual inspection required to verify linear chains
|
||||
- Plugin migrations can create branches (multiple head revisions)
|
||||
- `PLUGIN_TABLE_OWNERS` order not validated against actual dependencies
|
||||
Validation is in the test suite, in this repository, and has been for longer
|
||||
than the `/migration-check` skill: `tests/test_plugin_migrations.py` holds nine
|
||||
tests covering owners-versus-models, chain shape, no-op anchors and idempotency,
|
||||
and pins each chain's head in `EXPECTED_HEAD_REVISION`
|
||||
(`tests/test_plugin_migrations.py:125`). `tests/test_plugins/test_endpoints_credentials_migration.py`
|
||||
runs the 0020-0023 chain twice from both a legacy and a fresh schema, plus
|
||||
downgrade and re-upgrade. The endpoints chain today is 24 revisions with one
|
||||
root and one head and no dangling `down_revision`.
|
||||
|
||||
**Example violation:**
|
||||
```
|
||||
plugins/computers/migrations/versions/
|
||||
001_initial.py (down_revision=None)
|
||||
002_add_access.py (down_revision=001)
|
||||
003_add_policies.py (down_revision=001) # BRANCH! Two revisions point to 001
|
||||
```
|
||||
The branch example in the original text (`003_add_policies.py` with
|
||||
`down_revision=001`) is invented; no such file exists under
|
||||
`plugins/computers/migrations/versions/`, which holds 6 revisions.
|
||||
|
||||
**Impact:**
|
||||
- Alembic upgrade fails with "Multiple head revisions" error
|
||||
- Plugin can't be enabled until migration chain fixed
|
||||
- Hard to diagnose (requires reading migration files manually)
|
||||
|
||||
**Proper validation:** `/migration-check` skill (created 2026-09-11)
|
||||
- Checks: Each plugin has linear chain (no branches)
|
||||
- Checks: `down_revision` references exist
|
||||
- Checks: `PLUGIN_TABLE_OWNERS` order matches declared dependencies
|
||||
- Status: Implemented, run via pre-commit hook
|
||||
|
||||
**Effort:** DONE (skill created)
|
||||
**Priority:** Resolved 2026-09-11
|
||||
**The real gap, which the original text missed:** no test asserts that a plugin
|
||||
chain touches only the tables it owns. `plugins/endpoints/migrations/versions/0023_endpoint_credentials.py`
|
||||
declares `plugin_name = 'endpoints'` (line 33) and then runs
|
||||
`op.drop_index(..., table_name='assets')` at line 73 and `op.drop_column('assets',
|
||||
column)` at line 76 against the core `assets` table, cleaning up after the
|
||||
shipped 0021 which had added those columns from the same plugin chain. `assets`
|
||||
is not, and cannot be, in `PLUGIN_TABLE_OWNERS['endpoints']`
|
||||
(`shopdb/plugins/alembic_template.py:57-63`), so the metadata ownership filter
|
||||
never saw it. Every step is guarded on the live schema and the forward direction
|
||||
is clean, so nothing is broken; nothing stops the next one either.
|
||||
|
||||
### 6. Implicit Database Connection Sharing
|
||||
|
||||
**Risk:** Plugins share `db.session` without transaction isolation.
|
||||
**Verdict: REFUTED as a blanket claim.**
|
||||
|
||||
**Evidence:**
|
||||
- Multiple plugins write to database in same request
|
||||
- No explicit transaction boundaries
|
||||
- Rollback in one plugin affects others
|
||||
Savepoints are in use where concurrency actually bites:
|
||||
`plugins/endpoints/plugin.py:1318`, `:1387` and `:1692` all wrap work in
|
||||
`with db.session.begin_nested():`, and the application-delta path reconciles a
|
||||
unique-constraint collision inside that savepoint rather than sinking the
|
||||
check-in. The original text's example - a printers rollback silently discarding
|
||||
a computers write in the same request - is not backed by any cited call site,
|
||||
and none was found.
|
||||
|
||||
**Example:**
|
||||
```python
|
||||
# computers plugin writes Computer
|
||||
computer = Computer(hostname='PC1')
|
||||
db.session.add(computer)
|
||||
|
||||
# printers plugin writes Printer (same session!)
|
||||
printer = Printer(name='Printer1')
|
||||
db.session.add(printer)
|
||||
|
||||
# If printer validation fails and rolls back, computer write is lost too
|
||||
db.session.rollback()
|
||||
```
|
||||
|
||||
**Impact:**
|
||||
- Plugin A's failure can undo Plugin B's work
|
||||
- No atomicity guarantee per-plugin
|
||||
- Hard to debug (cross-plugin transaction interactions)
|
||||
|
||||
**Best practice:** Explicit transaction scopes per plugin
|
||||
```python
|
||||
from shopdb.api import db
|
||||
|
||||
with db.session.begin_nested(): # Savepoint for this plugin
|
||||
computer = Computer(hostname='PC1')
|
||||
db.session.add(computer)
|
||||
db.session.flush()
|
||||
# If exception here, only this savepoint rolls back
|
||||
```
|
||||
|
||||
**Remediation:**
|
||||
1. Audit plugin DB writes: find implicit `db.session.add()` without savepoint
|
||||
2. Wrap in `begin_nested()` savepoints or explicit `commit()`
|
||||
3. Document transaction patterns in PLUGIN-DEVELOPMENT.md
|
||||
|
||||
**Effort:** 40 hours
|
||||
**Priority:** Q1 2027 (data integrity risk, but low frequency in practice)
|
||||
A per-plugin transaction policy is still not written down anywhere. That is a
|
||||
documentation item for PLUGIN-DEVELOPMENT.md, not a critical data-integrity
|
||||
finding.
|
||||
|
||||
## High Priority Findings (P1)
|
||||
|
||||
### 7. Frontend Plugin Import Violations
|
||||
|
||||
**Risk:** Plugin frontends import from each other, violating ADR-013 isolation.
|
||||
**Verdict: STALE. Enforced and passing.**
|
||||
|
||||
**Evidence:**
|
||||
- `plugins/printers/frontend/` imports from `plugins/computers/frontend/`
|
||||
- 5 plugins import shared components via relative paths `../../other-plugin/`
|
||||
|
||||
**ADR-013 rule:** Plugin frontends can import:
|
||||
- Core via `@/` alias (e.g., `@/components/Shared.vue`)
|
||||
- Own tree via `./` or `../` within plugin
|
||||
- NOT other plugin trees
|
||||
|
||||
**Proper pattern:**
|
||||
```javascript
|
||||
// BAD: Cross-plugin import
|
||||
import PCSelect from '../../computers/frontend/components/PCSelect.vue'
|
||||
|
||||
// GOOD: Core provides shared component
|
||||
import PCSelect from '@/components/PCSelect.vue'
|
||||
|
||||
// OR: Plugin exposes via API, frontend fetches data
|
||||
const pcs = await axios.get('/api/computers/')
|
||||
```
|
||||
|
||||
**Enforcement:** `check-naming-and-style.sh` now checks for escaping imports
|
||||
```bash
|
||||
grep -rPn "(import|from)\s+['\"]([^'\"]*\.\./\.\./|[^'\"]*/plugins/)" \
|
||||
plugins/*/frontend/
|
||||
```
|
||||
|
||||
**Remediation:**
|
||||
1. Move shared components to `frontend/src/components/`
|
||||
2. Update plugin imports to use `@/` alias
|
||||
3. Run pre-commit check to catch violations
|
||||
|
||||
**Effort:** 20 hours
|
||||
**Priority:** Q4 2026 (blocks lean builds)
|
||||
**Status:** Linter rule added 2026-09-11 (commit 4943e182c3)
|
||||
`scripts/check-naming-and-style.sh` runs "Checking for cross-plugin / escaping
|
||||
imports in plugin frontends" and the whole script exits 0 on this branch. The
|
||||
established pattern is a core component consumed through the `@/` alias: 97
|
||||
plugin frontend files already import that way, and the kiosk allowlist editor
|
||||
added in this range follows it
|
||||
(`frontend/src/components/NetworkAllowlistPicker.vue`, consumed at
|
||||
`plugins/usb/frontend/views/USBSettings.vue:108` and
|
||||
`plugins/printedparts/frontend/views/PrintedPartsSettings.vue:164`).
|
||||
|
||||
### 8. Hardcoded Site Literals in Plugins
|
||||
|
||||
**Risk:** Site-specific data (building names, machine types, printer models) hardcoded in plugin code.
|
||||
**Verdict: REFUTED as stated. HOLDS (narrowed) as a gate coverage gap.**
|
||||
|
||||
**Evidence (violations of ADR-015):**
|
||||
- `plugins/machines/models.py` has `MACHINE_TYPES = ['Lathe', 'Mill', 'CNC']` hardcoded
|
||||
- `plugins/printers/api/routes.py` validates `building in ['Bldg 300', 'Bldg 400']`
|
||||
- 12 plugins have site-specific literals
|
||||
`grep -rn "MACHINE_TYPES" . --include=*.py` and
|
||||
`grep -rn "Bldg 300\|Bldg 400" . --include=*.py` both return **nothing**. The
|
||||
ADR-015 block in `scripts/check-naming-and-style.sh:392-462` runs and passes,
|
||||
and a control pattern over the same directories returns 1995 lines, so the
|
||||
pipeline is not vacuous.
|
||||
|
||||
**ADR-015 rule:** Site data MUST live in:
|
||||
- `shopdb/core/settings/` - Settings table defaults
|
||||
- `shopdb/core/seed/` - Initial data SQL
|
||||
- `scripts/site_imports/` - Import tools for site-specific data
|
||||
|
||||
**Proper pattern:**
|
||||
```python
|
||||
# BAD: Hardcoded site data
|
||||
MACHINE_TYPES = ['Lathe', 'Mill', 'CNC']
|
||||
|
||||
# GOOD: Setting with neutral default
|
||||
machine_types = Setting.get('machines_allowed_types', default='[]')
|
||||
MACHINE_TYPES = json.loads(machine_types)
|
||||
```
|
||||
|
||||
**Enforcement:** `check-naming-and-style.sh` scans for literal building names, FQDNs, network ranges
|
||||
|
||||
**Remediation:**
|
||||
1. Extract literals to Settings table
|
||||
2. Update plugins to read from Settings
|
||||
3. Seed data with site-specific values during setup
|
||||
|
||||
**Effort:** 30 hours
|
||||
**Priority:** Q1 2027 (blocks multi-site deployments)
|
||||
**The narrowed finding:** the ADR-015 scan is scoped at
|
||||
`scripts/check-naming-and-style.sh:436-440` to `plugins/ shopdb/ scripts/
|
||||
deploy/ frontend/src/ tools/` with `--include` on
|
||||
`.py/.vue/.js/.ps1/.psm1/.sh/.iss/.json/.html`. `agent/` is not in that list and
|
||||
`.cs` is not an included extension, so roughly 2,500 lines of C# added in this
|
||||
range - `AgentCredentials.cs`, `AgentHttpFactory.cs`, `EnrollmentService.cs`,
|
||||
`AgentPipeServer.cs`, `ProtectedFileSystem.cs`, `StrictModePreamble.cs`, plus
|
||||
`agent/appsettings.json` - ship to a sister site unscanned. Re-running the
|
||||
assembled site patterns by hand over `agent/` (excluding bin/obj) finds one hit,
|
||||
prose in `agent/ShopDbEndpointAgent/ShopDbEndpointAgent.csproj:108`, which
|
||||
ADR-015 permits. The tree is clean; the gate would not notice if it stopped
|
||||
being clean. Note this is a gap in ADR-015's own stated scope, not a breach of
|
||||
it: the 2026-08-14 amendment names those six directories.
|
||||
|
||||
### 9. Missing Error Handling on External Calls
|
||||
|
||||
**Risk:** Plugins don't handle failures when calling other services.
|
||||
**Verdict: REFUTED.**
|
||||
|
||||
**Evidence:**
|
||||
- ServiceNow plugin: no timeout on HTTP requests, no retry logic
|
||||
- LDAP sync: no handling of connection failures
|
||||
- Email plugin: assumes SMTP always succeeds
|
||||
The plugins the finding names do not exist. `ls plugins/` gives: applications,
|
||||
backups, computers, employees, endpoints, geenforce, guides, knowledgebase,
|
||||
machines, measuringtools, network, notifications, printedparts, printers,
|
||||
slides, tools, usb, warranty. There is no ServiceNow plugin, no LDAP sync
|
||||
plugin and no email plugin; mail is core (`shopdb/utils/mailer.py`).
|
||||
|
||||
**Impact:**
|
||||
- Hung requests block worker threads
|
||||
- Transient failures cause permanent data loss
|
||||
- No user feedback when external service down
|
||||
Every outbound HTTP call in the tree carries a timeout and an exception handler:
|
||||
|
||||
**Best practice:** Timeouts, retries, circuit breakers
|
||||
```python
|
||||
# BAD: No timeout
|
||||
response = requests.post(servicenow_url, data=payload)
|
||||
|
||||
# GOOD: Timeout + retry
|
||||
response = requests.post(servicenow_url, data=payload, timeout=10)
|
||||
if response.status_code >= 500:
|
||||
# Retry with exponential backoff
|
||||
time.sleep(2 ** attempt)
|
||||
```
|
||||
|
||||
**Remediation:**
|
||||
1. Audit external calls: `grep -r "requests\.(get|post)" plugins/`
|
||||
2. Add timeouts (default 10s)
|
||||
3. Add retry logic for transient failures (5xx errors)
|
||||
4. Log failures for monitoring
|
||||
|
||||
**Effort:** 30 hours
|
||||
**Priority:** Q4 2026 (production stability)
|
||||
- `plugins/warranty/services/providers.py:129` and `:157` - `timeout=HTTP_TIMEOUT`,
|
||||
`except requests.RequestException` raising `WarrantyLookupError`.
|
||||
- `plugins/printers/services/zabbix_service.py:95` and `:126` -
|
||||
`timeout=self.REACHABLE_TIMEOUT` / `self.API_TIMEOUT`, caught.
|
||||
- `shopdb/utils/mailer.py:238` - `timeout=10`, caught, returns `(False, error)`
|
||||
and never raises.
|
||||
|
||||
### 10-12. Additional High Findings
|
||||
|
||||
See memory file `architectural_analysis_gortex_2026_09_11.md` for:
|
||||
- Inconsistent permission checking across plugins
|
||||
- Plugin state leakage via module-level globals
|
||||
- Missing database connection pool tuning
|
||||
The original text deferred these to a memory file rather than citing code:
|
||||
"inconsistent permission checking across plugins", "plugin state leakage via
|
||||
module-level globals", "missing database connection pool tuning".
|
||||
|
||||
The third is REFUTED. The pool is sized on purpose and env-driven:
|
||||
`shopdb/config.py:20-22` (`DB_POOL_SIZE` 10, `DB_MAX_OVERFLOW`, `DB_POOL_TIMEOUT`
|
||||
5) and `:48-72`, with the reasoning for the non-default `pool_timeout` written
|
||||
in place, and `docker-compose.yml:172-193` documents the
|
||||
`workers x (pool_size + max_overflow)` arithmetic against MySQL's
|
||||
`max_connections`.
|
||||
|
||||
The other two were not re-checked, because neither cites a file. They should be
|
||||
restated with a file:line or dropped.
|
||||
|
||||
## Medium Priority Findings (P2)
|
||||
|
||||
### 13. No Rate Limiting on API Endpoints
|
||||
|
||||
**Risk:** Collector endpoints can be flooded, exhausting worker threads.
|
||||
**Verdict: HOLDS (narrowed). Login is limited; two kiosk surfaces are not.**
|
||||
|
||||
**Mitigation:** See Security-Audit-2026-09-11.md (login rate limiting planned Q1 2027)
|
||||
Login rate limiting exists and is not "planned Q1 2027": `shopdb/core/api/auth.py:35`
|
||||
`_login_ratelimited()`, keyed per host at `:52`, applied at `:80`, with
|
||||
`tests/test_core/test_auth_ratelimit.py` over it. Enrollment has its own limiter
|
||||
(`plugins/endpoints/api/ratelimit.py`, 300 per 60 seconds per IP applied at
|
||||
`plugins/endpoints/api/enrollment.py:306`).
|
||||
|
||||
What has none: the USB kiosk routes. `grep` for `limiter` or `ratelimit` across
|
||||
`plugins/usb/` returns nothing, while `plugins/usb/api/selfhosted.py:346-371`
|
||||
resolves a posted badge to an employee name and returns it. That is an
|
||||
unauthenticated, unthrottled badge-to-name lookup for any caller inside the
|
||||
configured kiosk CIDRs.
|
||||
|
||||
Both limiter counters live in per-process `SimpleCache`
|
||||
(`plugins/endpoints/api/ratelimit.py:10-11`), so at the shipped four gunicorn
|
||||
workers the effective budget is four times the configured maximum.
|
||||
|
||||
### 14. Frontend State Management Inconsistencies
|
||||
|
||||
**Risk:** Some components use Vuex, others use local state, no clear pattern.
|
||||
**Verdict: REFUTED.**
|
||||
|
||||
**Impact:** Hard to track state flow, data duplication, synchronization bugs
|
||||
|
||||
**Recommendation:** Standardize on Pinia (Vue 3 state management) or document when to use local vs global state
|
||||
`grep -rn "vuex" frontend/src/` returns **nothing**, and `frontend/package.json:28`
|
||||
declares `"pinia": "^2.1.0"`. There is no Vuex-versus-Pinia split to
|
||||
standardise.
|
||||
|
||||
### 15. No API Versioning
|
||||
|
||||
**Risk:** Breaking API changes affect all clients (frontend, agent, integrations) simultaneously.
|
||||
**Verdict: HOLDS.**
|
||||
|
||||
**Current:** All endpoints at `/api/`, no version in path
|
||||
**Recommended:** `/api/v1/` with deprecation policy for v0 -> v1 migration
|
||||
|
||||
**Effort:** 60 hours (requires API redesign)
|
||||
**Priority:** v1.0 release
|
||||
`grep -rn "api/v1" shopdb/ --include=*.py` returns nothing; every route is
|
||||
mounted under `/api/`. This is a v1.0 decision, not a defect, and ADR-002 already
|
||||
carries the versioning story for the plugin contract. The 60-hour estimate has
|
||||
no stated basis.
|
||||
|
||||
### 16-22. Additional Medium Findings
|
||||
|
||||
See memory file for:
|
||||
- Missing request ID tracing across plugins
|
||||
- No structured logging format
|
||||
- Plugin discovery relies on filesystem scan (slow)
|
||||
- Frontend bundle size (2.1MB) larger than ideal
|
||||
- No content security policy headers
|
||||
- Database connection pool too small for peak load
|
||||
- Missing health check endpoints for monitoring
|
||||
Re-checked on 2026-09-18:
|
||||
|
||||
## Remediation Priority
|
||||
- **No content security policy headers: REFUTED as stated.** `shopdb/__init__.py:340`
|
||||
sets `Content-Security-Policy: frame-ancestors 'self'` on every response, plus
|
||||
`X-Content-Type-Options`, `X-Frame-Options` and `Referrer-Policy` at `:334-337`;
|
||||
uploads carry a stricter `default-src 'none'; sandbox`
|
||||
(`shopdb/utils/uploads.py:13`), pinned by `tests/test_core/test_security_headers.py`.
|
||||
The accurate, narrower statement is that there is no `script-src` policy, and
|
||||
the code says so in place and says why.
|
||||
- **Missing health check endpoints: REFUTED.** `shopdb/core/api/dashboard.py:318`
|
||||
is `GET /api/dashboard/health`.
|
||||
- **Database connection pool too small: REFUTED.** See 10-12 above.
|
||||
- **No request ID tracing: HOLDS.** No request-id plumbing was found in
|
||||
`shopdb/__init__.py`.
|
||||
- **No structured logging format: not re-checked.**
|
||||
- **Plugin discovery relies on a filesystem scan: HOLDS, by design.** ADR-003
|
||||
decides filesystem discovery under `plugins/`. "Slow" was never measured.
|
||||
- **Frontend bundle size 2.1MB: UNVERIFIED.** No build was run for this page.
|
||||
|
||||
| Finding | Severity | Effort (hrs) | Priority | Status |
|
||||
|---------|----------|--------------|----------|--------|
|
||||
| Schema drift (models vs migrations) | CRITICAL | 80 | Q4 2026 | Planned |
|
||||
| Circular plugin dependencies | CRITICAL | 60 | Q1 2027 | Planned |
|
||||
| Undeclared import dependencies | CRITICAL | 40 | Q4 2026 | Planned |
|
||||
| Migration chain validation | CRITICAL | 0 | DONE | /migration-check skill |
|
||||
| Frontend plugin imports | HIGH | 20 | Q4 2026 | Linter added |
|
||||
| Site literals in plugins | HIGH | 30 | Q1 2027 | Planned |
|
||||
| Missing error handling | HIGH | 30 | Q4 2026 | Planned |
|
||||
| Implicit DB session sharing | HIGH | 40 | Q1 2027 | Planned |
|
||||
| Single points of failure | CRITICAL | 200+ | Q2 2027 | Infrastructure |
|
||||
| API versioning | MEDIUM | 60 | v1.0 | Planned |
|
||||
## Remediation Priority (revised 2026-09-18)
|
||||
|
||||
## Testing Performed
|
||||
| Finding | Verdict | What is actually left to do |
|
||||
|---------|---------|------------------------------|
|
||||
| Circular plugin dependencies | REFUTED / narrowed | Decide whether deferred cross-plugin imports must be guarded; guard `agentauth.py:118` and `policy_routes.py:44` |
|
||||
| Undeclared core imports | REFUTED | Nothing |
|
||||
| Schema drift | REFUTED / narrowed | Align `notification.py:158` with migration 0006; teach the naming gate to read `sa.Column('...')` |
|
||||
| Plugin chain touches core tables | HOLDS (new) | Add a test that a plugin chain writes only owned tables |
|
||||
| Migration chain validation | STALE | Nothing; `tests/test_plugin_migrations.py` is the gate |
|
||||
| Implicit DB session sharing | REFUTED | Document the transaction pattern in PLUGIN-DEVELOPMENT.md |
|
||||
| Frontend plugin imports | STALE | Nothing; gate passes |
|
||||
| Site literals in plugins | REFUTED / narrowed | Extend the ADR-015 scan to `agent/` and `.cs` |
|
||||
| Missing external-call error handling | REFUTED | Nothing |
|
||||
| Rate limiting | HOLDS (narrowed) | Throttle the USB kiosk badge lookup; decide whether per-process counters are acceptable at 4 workers |
|
||||
| Vuex/Pinia split | REFUTED | Nothing |
|
||||
| API versioning | HOLDS | v1.0 decision |
|
||||
| Single points of failure | UNVERIFIED | Belongs in a deployment document |
|
||||
|
||||
## Testing Performed (2026-09-18 re-check)
|
||||
|
||||
**Gortex semantic analysis:**
|
||||
```bash
|
||||
gortex explore "find circular dependencies between plugins"
|
||||
gortex explore "find undeclared imports from shopdb.core"
|
||||
gortex explore "find schema mismatches between models and migrations"
|
||||
# Every claim above was checked with a grep or a read on gh-wj at 35d4fcd41.
|
||||
grep -rn "^from plugins\." plugins/ --include=*.py # 0 hits
|
||||
grep -rn "from shopdb\.\(core\|utils\|extensions\|services\)" \
|
||||
plugins/ --include=*.py # 0 hits
|
||||
grep -rPn "sa\.Column\(\s*'[a-z]+_[a-z_]+'" \
|
||||
plugins/*/migrations/versions/*.py migrations/versions/*.py # 0 hits
|
||||
grep -rn "MACHINE_TYPES\|Bldg 300\|Bldg 400" . --include=*.py # 0 hits
|
||||
grep -rn "vuex" frontend/src/ # 0 hits
|
||||
grep -rn "api/v1" shopdb/ --include=*.py # 0 hits
|
||||
bash scripts/check-naming-and-style.sh # exit 0
|
||||
```
|
||||
|
||||
**Migration chain validation:**
|
||||
```bash
|
||||
/migration-check # Runs pytest tests/test_plugin_migrations.py
|
||||
# PASS: All plugin migration chains are linear
|
||||
```
|
||||
|
||||
**Frontend import validation:**
|
||||
```bash
|
||||
bash scripts/check-naming-and-style.sh
|
||||
# Scans for plugin-to-plugin imports (../../ escaping)
|
||||
# PASS: No violations after cleanup
|
||||
```
|
||||
|
||||
## Recommendations
|
||||
|
||||
**Immediate (Q4 2026):**
|
||||
1. Fix schema drift (models vs migrations) - CRITICAL
|
||||
2. Enforce plugin API imports (no shopdb.core) - enables refactoring
|
||||
3. Add error handling to external service calls - stability
|
||||
|
||||
**Short-term (Q1 2027):**
|
||||
1. Break circular plugin dependencies - enables lean builds
|
||||
2. Remove site literals from plugins - enables multi-site
|
||||
3. Fix implicit DB session sharing - data integrity
|
||||
|
||||
**Long-term (Q2-Q4 2027):**
|
||||
1. High availability architecture (if feasible in air-gap)
|
||||
2. API versioning for v1.0 release
|
||||
3. Frontend optimization (bundle size, state management)
|
||||
No test suite was run for this re-check; every verdict above is static.
|
||||
|
||||
## References
|
||||
|
||||
- Gortex MCP Server: Semantic code intelligence for architectural analysis
|
||||
- ADR-001: Asset as Platform Contract (plugin isolation model)
|
||||
- ADR-003: Plugin Distribution (filesystem discovery)
|
||||
- ADR-008: Plugin Migration Ownership (linear chains, PLUGIN_TABLE_OWNERS)
|
||||
- ADR-013: Plugin Catalog and Lean Builds (frontend isolation)
|
||||
- ADR-015: Site-Specific Configuration (no literals in code)
|
||||
- Memory file: `.claude/memory/architectural_analysis_gortex_2026_09_11.md`
|
||||
- ADR-Conformance-2026-09-18.md: the ADR-by-ADR result of the 2026-09-18 review,
|
||||
including the fact that the ADR set is not in this repository
|
||||
|
||||
Reference in New Issue
Block a user