feat: architecture conformance checks in CI and metrics feedback loop
- cloud/arch_conformance.py: contract-version (+1 only, approved change record), forbidden-directory (own dir only, .. and symlink escapes resolved), forbidden-import layer bans audited against the baseline (foundation/execution/driver, server sqlite3 Row adapters deliberately allowed), G2 frozen-stack swap guard; skill-project one-call check. - cloud/metrics.py: first-pass/defect/rework rates over terminal tasks, unknown rates as None (never 0), #212 dual-column per-task cost (total None when incomplete), pure feedback rules (tighten/route-with-extra-audit/recalibrate, auto_apply False), golden-set gate: unapproved sample set is never decision-ready. - GET /metrics-report (authenticated, member-scoped, same as /metrics): attempts/defects are None until an attempt ledger exists, so rates stay unknown rather than fabricated. - Tests: 29 arch + 15 metrics + 3 endpoint, each check with positive and negative cases; repo-level scan pins zero layer violations on the baseline so a future violation fails CI via the existing host-verify discover run (no workflow change).
This commit is contained in:
host committed
1 parent
382ec6d62e
commit
dba6536ac2
7 files changed
+1201
No files matched your search
@@ -0,0 +1,299 @@
|
||||
"""Architecture-conformance checks for the control plane and for
|
||||
skill-driven projects (software-engineering skill, G2 frozen artefacts).
|
||||
|
||||
Zero third-party dependencies; standard library only. This module
|
||||
imports no ``cloud.*`` module, so it can never violate the layer bans
|
||||
it enforces, and it can be reused verbatim against a skill project's
|
||||
own directory tree (see ``check_skill_project``).
|
||||
|
||||
The four checks, each with a stated rule and a stated reason:
|
||||
|
||||
1. Contract version (``check_contract_version``). Rule mirrors the
|
||||
skill's contract-freeze change protocol: after freeze, a version
|
||||
change is allowed only with a coordinator-approved change record
|
||||
that names backward compatibility, the affected clients, and (for
|
||||
a breaking change) a regression task per affected client; the
|
||||
version must advance by exactly one. Reason: a silent contract
|
||||
drift is how parallel client threads diverge; the protocol is the
|
||||
only sanctioned path, so the check enforces the protocol instead
|
||||
of merely documenting it.
|
||||
|
||||
2. Forbidden directories (``is_within`` / ``check_task_writes``).
|
||||
Rule reuses the two directory definitions that already exist and
|
||||
invents no third one: on the control plane a task's own directory
|
||||
is its tenant work directory (``worker._tenant_dirs`` ``work``);
|
||||
in a skill project a client thread's own directory is its
|
||||
``apps/<client>/`` tree. Any write outside the own directory --
|
||||
including ``..`` escapes and symlink escapes, both resolved before
|
||||
comparison -- is a violation. Reason: cross-directory writes are
|
||||
how one tenant/client corrupts another's worktree.
|
||||
|
||||
3. Forbidden imports (``check_layer_imports``). Bans were chosen by
|
||||
auditing the current tree, not by wishful layering; every ban
|
||||
below passes on the baseline, and the repo-level test pins that:
|
||||
|
||||
- Foundation modules (``db``, ``models``, ``queue``,
|
||||
``logging_config``) must not import orchestration/service
|
||||
modules (``server``, ``worker``, ``pr_exec``, ``webhooks``,
|
||||
``autoscaler``, ``cost``). Reason: the foundation is imported
|
||||
by everything; an upward import is a cycle waiting to happen
|
||||
and drags service state into pure helpers.
|
||||
- The execution module (``sandbox``) must not import persistence
|
||||
or orchestration (``db``, ``server``, ``worker``, ``cost``,
|
||||
``auth``, ``pr``, ``pr_exec``, ``webhooks``). Reason: executors
|
||||
must stay swappable and testable with fake runners; decisions
|
||||
about mounts and networks arrive as arguments, not by reading
|
||||
the database.
|
||||
- No cloud module except the driver-adjacent ones (``db``,
|
||||
``backup``, ``migrations``) may open a database connection
|
||||
directly (``sqlite3.connect`` or any ``psycopg*`` import).
|
||||
Service modules must obtain connections through
|
||||
``cloud.db.connect`` or a Store. Reason: the backend switch
|
||||
(sqlite/postgres) lives in exactly one place; a direct driver
|
||||
call bypasses it. Deliberately NOT banned: ``server.py``'s use
|
||||
of ``sqlite3.Row`` / ``sqlite3.IntegrityError`` as type adapters
|
||||
on connections obtained from ``cloud.db`` -- that is adaptation,
|
||||
not a bypass, and banning it would fail the baseline for no
|
||||
safety gain. The test asserts the narrow form (no ``connect``
|
||||
call, no driver import) rather than the broad form.
|
||||
|
||||
Only static ``ast`` imports are seen; dynamic imports
|
||||
(``importlib``) are out of scope and are noted, not claimed.
|
||||
|
||||
4. Frozen stack (``check_frozen_stack``). Rule mirrors the skill's
|
||||
G2 rule "frozen stack must not be swapped mid-flight": a client
|
||||
whose stack entry changes, or disappears, after freeze fails
|
||||
unless an approved change record covers it; adding a brand-new
|
||||
client (no frozen entry) is an addition, not a swap, and passes.
|
||||
Reason: the shared core is built against the frozen table; a
|
||||
mid-flight swap silently invalidates it.
|
||||
|
||||
All checkers return plain data (dicts/lists), never raise for a
|
||||
violation, so CI can report every violation in one run instead of
|
||||
stopping at the first.
|
||||
"""
|
||||
|
||||
import ast
|
||||
import os
|
||||
from pathlib import Path
|
||||
|
||||
FOUNDATION_MODULES = frozenset({"db", "models", "queue", "logging_config"})
|
||||
ORCHESTRATION_MODULES = frozenset(
|
||||
{"server", "worker", "pr_exec", "webhooks", "autoscaler", "cost"})
|
||||
EXECUTION_MODULES = frozenset({"sandbox"})
|
||||
EXECUTION_BANNED = frozenset(
|
||||
{"db", "server", "worker", "cost", "auth", "pr", "pr_exec", "webhooks"})
|
||||
DRIVER_ADJACENT_MODULES = frozenset({"db", "backup", "migrations"})
|
||||
|
||||
|
||||
# ---------------------------------------------------------------- contract
|
||||
|
||||
def parse_version(text):
|
||||
"""Parse ``v1`` / ``1`` to int; anything else is None (unparseable
|
||||
is a failure, never silently treated as the frozen version)."""
|
||||
if text is None:
|
||||
return None
|
||||
text = str(text).strip()
|
||||
if text.startswith("v"):
|
||||
text = text[1:]
|
||||
if text.isdigit():
|
||||
return int(text)
|
||||
return None
|
||||
|
||||
|
||||
def _change_record_problems(change, breaking):
|
||||
"""Missing pieces of a contract-change record, per the protocol."""
|
||||
problems = []
|
||||
if not isinstance(change, dict):
|
||||
return ["change record missing (frozen contract changed without "
|
||||
"a coordinator-approved change record)"]
|
||||
if not change.get("approved"):
|
||||
problems.append("change not coordinator-approved")
|
||||
if change.get("backward_compatible") is None:
|
||||
problems.append("backward compatibility not declared")
|
||||
affected = change.get("affected_clients")
|
||||
if not affected or not isinstance(affected, list):
|
||||
problems.append("affected clients not listed")
|
||||
if breaking and not change.get("regression_task"):
|
||||
problems.append("breaking change without a regression task")
|
||||
return problems
|
||||
|
||||
|
||||
def check_contract_version(frozen, current, change=None):
|
||||
"""Return ``{"ok": bool, "reasons": [...]}``.
|
||||
|
||||
Equal versions pass regardless of ``change``. A different version
|
||||
passes only when it is exactly frozen + 1 and ``change`` satisfies
|
||||
the protocol; a breaking change is one where
|
||||
``backward_compatible`` is False.
|
||||
"""
|
||||
frozen_n = parse_version(frozen)
|
||||
current_n = parse_version(current)
|
||||
if frozen_n is None or current_n is None:
|
||||
return {"ok": False,
|
||||
"reasons": ["unparseable contract version: frozen=%r "
|
||||
"current=%r" % (frozen, current)]}
|
||||
if current_n == frozen_n:
|
||||
return {"ok": True, "reasons": []}
|
||||
reasons = []
|
||||
if current_n != frozen_n + 1:
|
||||
reasons.append("contract version must advance by exactly one "
|
||||
"(frozen v%d -> v%d, not v%d)"
|
||||
% (frozen_n, frozen_n + 1, current_n))
|
||||
breaking = isinstance(change, dict) and \
|
||||
change.get("backward_compatible") is False
|
||||
reasons.extend(_change_record_problems(change, breaking))
|
||||
return {"ok": not reasons, "reasons": reasons}
|
||||
|
||||
|
||||
# ------------------------------------------------------------- frozen stack
|
||||
|
||||
def check_frozen_stack(frozen, current, change=None):
|
||||
"""``frozen``/``current`` map client -> stack string.
|
||||
|
||||
Changed or removed entries fail without an approved change record
|
||||
(same shape as the contract-change record, minus the version
|
||||
rules); added clients pass as additions.
|
||||
"""
|
||||
frozen = frozen or {}
|
||||
current = current or {}
|
||||
reasons = []
|
||||
swapped = sorted(c for c in frozen
|
||||
if c not in current or current[c] != frozen[c])
|
||||
if not swapped:
|
||||
return {"ok": True, "reasons": [], "swapped": []}
|
||||
problems = _change_record_problems(change, breaking=False)
|
||||
if problems:
|
||||
reasons.append("frozen stack changed for %s without an approved "
|
||||
"change record: %s" % (swapped, "; ".join(problems)))
|
||||
return {"ok": not reasons, "reasons": reasons, "swapped": swapped}
|
||||
|
||||
|
||||
# ------------------------------------------------------------ directories
|
||||
|
||||
def is_within(root, candidate):
|
||||
"""True iff ``candidate`` resolves inside ``root`` (symlinks and
|
||||
``..`` resolved first, so both escape routes are caught)."""
|
||||
try:
|
||||
root_p = Path(root).resolve()
|
||||
cand_p = Path(candidate).resolve()
|
||||
except (OSError, RuntimeError):
|
||||
return False
|
||||
return cand_p == root_p or root_p in cand_p.parents
|
||||
|
||||
|
||||
def check_task_writes(own_dir, paths):
|
||||
"""Return the subset of ``paths`` written outside ``own_dir``."""
|
||||
return [p for p in paths if not is_within(own_dir, p)]
|
||||
|
||||
|
||||
# ----------------------------------------------------------------- imports
|
||||
|
||||
def scan_imports(source):
|
||||
"""Static import scan of one module's source.
|
||||
|
||||
Returns ``{"cloud": set, "driver_connect": bool, "driver_import": bool}``:
|
||||
``cloud`` holds the ``cloud.<name>`` top-level names imported;
|
||||
``driver_connect`` is True on any ``sqlite3.connect`` call;
|
||||
``driver_import`` is True on any ``psycopg*`` import. A bare
|
||||
``import sqlite3`` (Row/exception adapters) sets neither flag.
|
||||
"""
|
||||
cloud_imports = set()
|
||||
driver_connect = False
|
||||
driver_import = False
|
||||
try:
|
||||
tree = ast.parse(source)
|
||||
except SyntaxError:
|
||||
return {"cloud": cloud_imports,
|
||||
"driver_connect": driver_connect,
|
||||
"driver_import": driver_import,
|
||||
"parse_error": True}
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.Import):
|
||||
for alias in node.names:
|
||||
if alias.name == "sqlite3":
|
||||
continue
|
||||
if alias.name.startswith("psycopg"):
|
||||
driver_import = True
|
||||
if alias.name == "cloud" or alias.name.startswith("cloud."):
|
||||
parts = alias.name.split(".")
|
||||
if len(parts) > 1:
|
||||
cloud_imports.add(parts[1])
|
||||
elif isinstance(node, ast.ImportFrom):
|
||||
if node.module and node.module.startswith("psycopg"):
|
||||
driver_import = True
|
||||
if node.module == "cloud" or (node.module or "").startswith("cloud."):
|
||||
if node.module == "cloud":
|
||||
for alias in node.names:
|
||||
cloud_imports.add(alias.name)
|
||||
else:
|
||||
cloud_imports.add(node.module.split(".")[1])
|
||||
elif isinstance(node, ast.Call):
|
||||
func = node.func
|
||||
if isinstance(func, ast.Attribute) and func.attr == "connect":
|
||||
value = func.value
|
||||
if isinstance(value, ast.Name) and value.id == "sqlite3":
|
||||
driver_connect = True
|
||||
return {"cloud": cloud_imports,
|
||||
"driver_connect": driver_connect,
|
||||
"driver_import": driver_import}
|
||||
|
||||
|
||||
def check_layer_imports(sources):
|
||||
"""``sources`` maps module name -> source text. Returns a list of
|
||||
violation dicts; empty means the layer rules hold."""
|
||||
violations = []
|
||||
for module, source in sorted(sources.items()):
|
||||
scanned = scan_imports(source)
|
||||
if scanned.get("parse_error"):
|
||||
violations.append({"module": module, "rule": "parse",
|
||||
"detail": "source does not parse; "
|
||||
"imports cannot be verified"})
|
||||
continue
|
||||
if module in FOUNDATION_MODULES:
|
||||
bad = scanned["cloud"] & ORCHESTRATION_MODULES
|
||||
if bad:
|
||||
violations.append({"module": module,
|
||||
"rule": "foundation_must_not_import_orchestration",
|
||||
"detail": sorted(bad)})
|
||||
if module in EXECUTION_MODULES:
|
||||
bad = scanned["cloud"] & EXECUTION_BANNED
|
||||
if bad:
|
||||
violations.append({"module": module,
|
||||
"rule": "execution_must_not_import_persistence_or_orchestration",
|
||||
"detail": sorted(bad)})
|
||||
if module not in DRIVER_ADJACENT_MODULES:
|
||||
if scanned["driver_connect"] or scanned["driver_import"]:
|
||||
violations.append({"module": module,
|
||||
"rule": "service_must_not_open_driver_connection_directly",
|
||||
"detail": "use cloud.db.connect or a Store"})
|
||||
return violations
|
||||
|
||||
|
||||
def scan_cloud_package(repo_root):
|
||||
"""Run ``check_layer_imports`` over every ``cloud/*.py`` in a repo
|
||||
checkout (tests and caches excluded). This is the repo-level test
|
||||
that makes a layer violation fail CI."""
|
||||
cloud_dir = Path(repo_root) / "cloud"
|
||||
sources = {}
|
||||
if cloud_dir.is_dir():
|
||||
for path in sorted(cloud_dir.glob("*.py")):
|
||||
sources[path.stem] = path.read_text(encoding="utf-8")
|
||||
return check_layer_imports(sources)
|
||||
|
||||
|
||||
def check_skill_project(project_root, frozen_version, current_version,
|
||||
frozen_stack, current_stack, own_dir, writes,
|
||||
change=None):
|
||||
"""One-call conformance for a skill project: contract version,
|
||||
frozen stack, and directory writes together. Returns a dict with
|
||||
one entry per check plus an overall ``ok``."""
|
||||
contract = check_contract_version(frozen_version, current_version,
|
||||
change=change)
|
||||
stack = check_frozen_stack(frozen_stack, current_stack, change=change)
|
||||
outside = check_task_writes(own_dir, writes)
|
||||
directory = {"ok": not outside,
|
||||
"reasons": (["writes outside own directory: %s" % outside]
|
||||
if outside else [])}
|
||||
return {"ok": contract["ok"] and stack["ok"] and directory["ok"],
|
||||
"contract": contract, "stack": stack, "directory": directory}
|
||||
Reference in new issue
Block a user