From 0fbbda94877989cfab9f647eed153354aec587a2 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:54:49 -0500 Subject: [PATCH] Guard every tracked events.py against PII, not just Run and Actor test_run_events_carry_no_pii.py and test_actor_events_carry_no_pii.py each hardcoded one file, so a new event anywhere else carrying a 2-BM directory path (which embeds a surname and proposal number) or other PII-shaped field shipped with zero objection. Replace both with test_events_carry_no_pii.py, which discovers every events.py via tracked_python_files() and checks it against a shared deny-list, plus an Actor-only extension for name/display_name (globalizing those two terms flags 28 ordinary entity-label fields elsewhere with no real PII among them, so they stay scoped to Actor's own file). Co-Authored-By: Claude Sonnet 5 --- .../ports/event_activity_trail.py | 17 +- .../test_actor_events_carry_no_pii.py | 123 ---------- .../architecture/test_events_carry_no_pii.py | 217 ++++++++++++++++++ .../tests/architecture/test_no_em_dashes.py | 1 - .../test_run_events_carry_no_pii.py | 152 ------------ 5 files changed, 226 insertions(+), 284 deletions(-) delete mode 100644 apps/api/tests/architecture/test_actor_events_carry_no_pii.py create mode 100644 apps/api/tests/architecture/test_events_carry_no_pii.py delete mode 100644 apps/api/tests/architecture/test_run_events_carry_no_pii.py diff --git a/apps/api/src/cora/infrastructure/ports/event_activity_trail.py b/apps/api/src/cora/infrastructure/ports/event_activity_trail.py index 26dd1edd959..cc4800a442c 100644 --- a/apps/api/src/cora/infrastructure/ports/event_activity_trail.py +++ b/apps/api/src/cora/infrastructure/ports/event_activity_trail.py @@ -11,17 +11,18 @@ Ships `event_id`, `stream_type`, `stream_id`, `event_type`, `occurred_at`, `recorded_at`, `correlation_id`, `causation_id` and `cause_occurred_at` only. -NEVER `payload`. `test_run_events_carry_no_pii.py` (and its Access-BC sibling) are -the only two fitness tests that guard event field names against personal -data, and they cover exactly two of the twenty-five stream types this port's -data spans; shipping raw payloads across every BC would carry that guarantee -somewhere it does not hold. A lane needs to know THAT something happened and -WHAT KIND, never the values inside it. +NEVER `payload`. `test_events_carry_no_pii.py` guards event field names +against personal data across every tracked aggregate `events.py`, but the +guard is a field-name deny-list, not a semantic content check: an unlisted +future field name would still slip through on a file the guard already +covers. Shipping raw payloads across every BC would carry that gap here too. +A lane needs to know THAT something happened and WHAT KIND, never the +values inside it. The three relationship columns do not weaken that. They are opaque identifiers and one timestamp drawn from the envelope, never from -`payload`, so no BC's field names ride out on them and the guarantee the two -fitness tests actually make is unchanged. They answer "which events belong to +`payload`, so no BC's field names ride out on them and the guarantee the +fitness test actually makes is unchanged. They answer "which events belong to one operator action" and "which event caused this one", both of which are structure, not content. Anything requiring a VALUE from inside an event still has to come from a domain-specific read, not from here. diff --git a/apps/api/tests/architecture/test_actor_events_carry_no_pii.py b/apps/api/tests/architecture/test_actor_events_carry_no_pii.py deleted file mode 100644 index 9414d5e1057..00000000000 --- a/apps/api/tests/architecture/test_actor_events_carry_no_pii.py +++ /dev/null @@ -1,123 +0,0 @@ -"""Actor event payloads carry NO PII. PII lives in the -`actor_profile` table per [[project_pii_vault]] + -[[project_pii_vault_implementation_design]]. - -Fitness function: AST-walks -`cora/access/aggregates/actor/events.py` and rejects any -dataclass field on an Actor event whose name appears in the -PII deny-list (`name`, `display_name`, `email`, `phone`, -`orcid`, `affiliation`). - -Why this lives separately from the existing payload-immutability -and from_stored-coverage fitness tests: those tests pin -structural invariants on the event union. This one pins a -DOMAIN invariant — Actor events specifically must never carry -identifying personal data, so a future field rename or addition -that reintroduces `name` (or `email` / `phone` / etc.) fails -the build instead of silently re-broadening the audit-event PII -surface. - -The deny-list mirrors the PII fields the design memo locks for -future actor_profile columns; widen it whenever a new identifying -column lands on the vault. -""" - -import ast - -import pytest - -from tests.architecture.conftest import CORA_ROOT - -_EVENTS_FILE = CORA_ROOT / "access" / "aggregates" / "actor" / "events.py" - -# Any dataclass field on an Actor* event whose annotation target -# name matches one of these strings counts as a violation. Names are -# matched case-sensitively (Python identifier convention) and treat -# `display_name` and `name` separately so callers can re-introduce -# the longer-form synonym later without unblocking the shorter-form -# field unintentionally. -_PII_FIELD_NAMES = frozenset( - { - "name", - "display_name", - "email", - "phone", - "orcid", - "affiliation", - } -) - - -def _actor_event_pii_field_violations() -> list[str]: - tree = ast.parse(_EVENTS_FILE.read_text()) - violations: list[str] = [] - for node in tree.body: - if not isinstance(node, ast.ClassDef): - continue - if not node.name.startswith("Actor"): - continue - for stmt in node.body: - if not isinstance(stmt, ast.AnnAssign): - continue - target = stmt.target - if isinstance(target, ast.Name) and target.id in _PII_FIELD_NAMES: - violations.append(f"line {stmt.lineno}: {node.name}.{target.id}") - return violations - - -@pytest.mark.architecture -def test_actor_event_payloads_carry_no_pii() -> None: - """Pin: dataclass fields named like PII never land on Actor* events. - - PII lives in the `actor_profile` vault accessed via - `ProfileStore`; events carry only `actor_id` references plus - audit-relevant primitives (kind, occurred_at). - A regression here usually means someone re-added `name` or - introduced an email / phone / etc. field on an event; move the - field to actor_profile (and update the vault schema) instead. - """ - violations = _actor_event_pii_field_violations() - assert not violations, ( - "Actor event payloads must carry NO PII; move identifying fields to " - "actor_profile via ProfileStore (see project_pii_vault):\n " + "\n ".join(violations) - ) - - -@pytest.mark.architecture -def test_actor_events_file_is_present() -> None: - """Sanity: the events.py file must exist; the file move below - the aggregate folder would silently make the PII-deny scan a - no-op without this guard.""" - msg = f"Expected Actor events file at {_EVENTS_FILE}" - assert _EVENTS_FILE.exists(), msg - - -@pytest.mark.architecture -def test_pii_deny_list_actually_finds_violations_when_seeded() -> None: - """Meta-test: confirm the AST walker would flag a seeded - violation. Guards against the silent-pass failure mode (for example - a future refactor moves the event classes to a sub-module and - the walker quietly stops seeing them). Builds an ephemeral - in-memory ast.Module mimicking the events file with a single - deliberately-bad Actor class, and asserts the walker would - report it. - """ - seed_source = ( - "from dataclasses import dataclass\n" - "@dataclass\n" - "class ActorRegisteredV2:\n" - " actor_id: int\n" - " name: str # PII violation seeded by the meta-test\n" - ) - tree = ast.parse(seed_source) - violations: list[str] = [] - for node in tree.body: - if not isinstance(node, ast.ClassDef) or not node.name.startswith("Actor"): - continue - for stmt in node.body: - if not isinstance(stmt, ast.AnnAssign): - continue - target = stmt.target - if isinstance(target, ast.Name) and target.id in _PII_FIELD_NAMES: - violations.append(f"line {stmt.lineno}: {node.name}.{target.id}") - assert violations, "seeded `name` field must be flagged by the deny-list walker" diff --git a/apps/api/tests/architecture/test_events_carry_no_pii.py b/apps/api/tests/architecture/test_events_carry_no_pii.py new file mode 100644 index 00000000000..89f8af10c3b --- /dev/null +++ b/apps/api/tests/architecture/test_events_carry_no_pii.py @@ -0,0 +1,217 @@ +"""Event payloads carry NO PII, across every tracked aggregate `events.py`. + +Fitness function: AST-walks every git-tracked `events.py` under +`src/cora` (discovered via `tracked_python_files()`, never `glob()` / +`iterdir()` -- see conftest.py's module docstring for why a filesystem +scan would see a different file set than pre-commit does) and rejects +any dataclass field on any class in that file whose name appears in +the applicable PII deny-list. + +Supersedes `test_run_events_carry_no_pii.py` and +`test_actor_events_carry_no_pii.py`. Those two hardcoded exactly one +file each (`cora/run/aggregates/run/events.py` and +`cora/access/aggregates/actor/events.py`); every other bounded +context's `events.py` was scanned by nothing. A new event anywhere +else carrying `observed_path` (or any other deny-listed field) would +have shipped with zero objection -- 2-BM's directory layout embeds a +surname and a proposal number, so a path-shaped field is personal +data wherever it lands, not only on the Run stream. + +Deliberately scans every class in every file, NOT only classes whose +name matches the aggregate (mirrors the original Run test's own +rationale): `cora/run/aggregates/run/events.py` defines +`CautionAcknowledgement`, `DecisionDebriefRequested` and +`HoldClaimReleased`, real Run-stream events that don't carry the +`Run` prefix, and a name-prefix filter would have silently exempted +them. Dropping the original Actor test's `Actor*`-prefix filter +changes nothing there today (every class in that file already carries +the `Actor` prefix); it only removes a foot-gun for tomorrow. + +## Why `name` / `display_name` are scoped to Actor, not global + +Actor's original deny-list included bare `name` and `display_name` +because on that aggregate they hold a person's name. Unioning them +into the deny-list applied to every tracked `events.py` is unsound: +`name` is also the ordinary field almost every OTHER aggregate in +this codebase uses for its own entity's label. Scanning all 43 +tracked `events.py` files with `name` / `display_name` in a +codebase-wide deny-list flags 28 fields across 15 files -- every one +of them an equipment, recipe, agent, campaign, dataset, procedure, +sample, or Trust-zone label, never a person's name. That count +includes `RunStarted.name` (the run's own label, e.g. the Bluesky +start-document precedent cited in that class's docstring), present in +the very file whose hand-curated deny-list already omits bare `name` +for exactly this reason. So `name` and `display_name` stay scoped to +`_ACTOR_EVENTS_FILE` via `_deny_list_for`, while every other deny term +from both original lists applies to every tracked `events.py` file +without exception. That is a widening for both originally-covered +files: Run's file is now also checked for `email` / `phone` / `orcid` +/ `affiliation` (Actor's terms; never observed on Run's file), and +Actor's file is now also checked for `path` / `directory` / `surname` +/ `proposal_number` / etc. (Run's terms; never observed on Actor's +file either). + +See `cora.run.aggregates.run.experiment_identity`'s module docstring +for the slice 14a proposal/ESAF-number argument, and +[[project_pii_vault]] / [[project_pii_vault_implementation_design]] +for the Actor vault. +""" + +import ast +from pathlib import Path + +import pytest + +from tests.architecture.conftest import BCS, CORA_ROOT, tracked_python_files + +_ACTOR_EVENTS_FILE = CORA_ROOT / "access" / "aggregates" / "actor" / "events.py" +_RUN_EVENTS_FILE = CORA_ROOT / "run" / "aggregates" / "run" / "events.py" + +# Applies to every tracked events.py file without exception: none of these +# terms have ever matched a legitimate, non-personal field name anywhere in +# the codebase (see module docstring). +_GLOBAL_PII_FIELD_NAMES = frozenset( + { + "observed_path", + "capture_path", + "full_file_name", + "path", + "directory", + "file_path", + "surname", + "proposal_number", + "esaf_number", + "esaf_doi_number", + "user_name", + "user_last_name", + "user_badge", + "user_email", + "user_institution", + "email", + "phone", + "orcid", + "affiliation", + } +) + +# Only meaningful as PII on Actor's own events: everywhere else in this +# codebase `name` / `display_name` is the ordinary label field of a +# non-person entity (see module docstring for the 28-hit false-positive +# count that justifies keeping this scoped rather than global). +_ACTOR_ONLY_PII_FIELD_NAMES = frozenset({"name", "display_name"}) + + +def _tracked_events_files() -> tuple[Path, ...]: + """Every git-tracked `events.py` under `src/cora`, sorted for a + deterministic (and readable) failure ordering.""" + return tuple(sorted(p for p in tracked_python_files() if p.name == "events.py")) + + +def _deny_list_for(events_file: Path) -> frozenset[str]: + if events_file == _ACTOR_EVENTS_FILE: + return _GLOBAL_PII_FIELD_NAMES | _ACTOR_ONLY_PII_FIELD_NAMES + return _GLOBAL_PII_FIELD_NAMES + + +def _pii_field_violations(source_path: Path, deny_list: frozenset[str]) -> list[str]: + """AST-walk every class in `source_path`'s dataclass fields for a + deny-list hit. Takes both the path and the deny list as arguments + (never a hardcoded global) so the seeded-violation meta-tests below + can call this SAME function against synthetic input, rather than + maintaining a second copy of the walk that could silently drift + from what actually runs. + """ + tree = ast.parse(source_path.read_text()) + violations: list[str] = [] + for node in tree.body: + if not isinstance(node, ast.ClassDef): + continue + for stmt in node.body: + if not isinstance(stmt, ast.AnnAssign): + continue + target = stmt.target + if isinstance(target, ast.Name) and target.id in deny_list: + violations.append(f"line {stmt.lineno}: {node.name}.{target.id}") + return violations + + +@pytest.mark.architecture +def test_event_payloads_carry_no_pii() -> None: + """Pin: no dataclass field on any class in any tracked `events.py` + is named like PII, or (Actor's file specifically) like a person's + name. + + A regression here usually means someone tried to carry a resolved + path, a raw `User*` PV, or an actor identity field onto an event + for convenience; move it to the appropriate vault instead + (`run_capture_path` / `run_experiment_identity` via their stores + for Run, `actor_profile` via `ProfileStore` for Actor). + """ + violations: list[str] = [] + for events_file in _tracked_events_files(): + rel = events_file.relative_to(CORA_ROOT) + for hit in _pii_field_violations(events_file, _deny_list_for(events_file)): + violations.append(f"{rel} {hit}") + assert not violations, ( + "Event payloads must carry NO PII; move identifying fields to the " + "appropriate vault (see this test module's docstring):\n " + "\n ".join(violations) + ) + + +@pytest.mark.architecture +def test_run_and_actor_events_files_are_present() -> None: + """Sanity: the two originally-hardcoded files must still exist at + their expected paths. A silent move would otherwise just drop out + of the `events.py` filter with no other signal.""" + assert _RUN_EVENTS_FILE.exists(), f"Expected Run events file at {_RUN_EVENTS_FILE}" + assert _ACTOR_EVENTS_FILE.exists(), f"Expected Actor events file at {_ACTOR_EVENTS_FILE}" + + +@pytest.mark.architecture +def test_pii_scan_discovers_events_file_for_every_bounded_context() -> None: + """Sanity: every bounded context in `BCS` contributes at least one + discovered `events.py`. Guards the discovery mechanism itself: a + `tracked_python_files()` regression (or a filter bug above) that + silently returned an empty or partial set would make the main test + pass trivially, over zero files. + """ + discovered = _tracked_events_files() + covered_bcs = {events_file.relative_to(CORA_ROOT).parts[0] for events_file in discovered} + missing = set(BCS) - covered_bcs + assert not missing, f"No tracked events.py discovered for bounded context(s): {sorted(missing)}" + + +@pytest.mark.architecture +def test_pii_deny_list_actually_finds_violations_when_seeded(tmp_path: Path) -> None: + """Meta-test: confirm the ACTUAL production walker + (`_pii_field_violations`, not a re-implemented copy) flags a + seeded violation on a class with an unrelated name. Guards against + a future refactor moving event classes to a sub-module (the walker + quietly stops seeing them) or reintroducing a name-prefix filter + (the walker stops seeing non-prefixed events), the exact shape + `CautionAcknowledgement` / `DecisionDebriefRequested` / + `HoldClaimReleased` already have in the real Run file. + """ + seed_file = tmp_path / "seed_events.py" + seed_file.write_text( + "from dataclasses import dataclass\n" + "@dataclass\n" + "class SomeUnrelatedEvent:\n" + " run_id: int\n" + " observed_path: str # PII violation seeded by the meta-test\n" + ) + violations = _pii_field_violations(seed_file, _GLOBAL_PII_FIELD_NAMES) + assert violations, "seeded `observed_path` field must be flagged by the deny-list walker" + + +@pytest.mark.architecture +def test_actor_only_pii_terms_scoped_to_actor_events_file() -> None: + """Pin: `name` / `display_name` apply only when scanning Actor's + events file, never as part of the deny-list every other tracked + `events.py` is checked against (see module docstring for why: both + terms are the ordinary entity-label field on every other aggregate + in this codebase).""" + actor_deny_list = _deny_list_for(_ACTOR_EVENTS_FILE) + other_deny_list = _deny_list_for(_RUN_EVENTS_FILE) + assert actor_deny_list >= _ACTOR_ONLY_PII_FIELD_NAMES + assert not (_ACTOR_ONLY_PII_FIELD_NAMES & other_deny_list) diff --git a/apps/api/tests/architecture/test_no_em_dashes.py b/apps/api/tests/architecture/test_no_em_dashes.py index cc05e0c1f96..13cb478ae84 100644 --- a/apps/api/tests/architecture/test_no_em_dashes.py +++ b/apps/api/tests/architecture/test_no_em_dashes.py @@ -376,7 +376,6 @@ "src/cora/trust/features/list_permissions/route.py", "tests/_strategies.py", "tests/architecture/conftest.py", - "tests/architecture/test_actor_events_carry_no_pii.py", "tests/architecture/test_actor_kind_sync.py", "tests/architecture/test_auth_principal_kind_sync.py", "tests/architecture/test_caution_invariants_module.py", diff --git a/apps/api/tests/architecture/test_run_events_carry_no_pii.py b/apps/api/tests/architecture/test_run_events_carry_no_pii.py deleted file mode 100644 index fcb769b9b44..00000000000 --- a/apps/api/tests/architecture/test_run_events_carry_no_pii.py +++ /dev/null @@ -1,152 +0,0 @@ -"""Run event payloads carry NO PII. The observed capture path lives in -the `run_capture_path` table per memory/project_witnessed_run_prelive_slices.md -slice 13 (mirroring `actor_profile` / [[project_pii_vault]]). - -Fitness function: AST-walks `cora/run/aggregates/run/events.py` and -rejects any dataclass field on ANY class in that file whose name -appears in the PII deny-list. Mirrors `test_actor_events_carry_no_pii.py`'s -mechanism (main check + file-presence guard + seeded-violation -meta-test); see that file for the full rationale on why this lives -separately from structural fitness tests. - -Deliberately scans every class in the file, NOT only classes whose name -starts with "Run": this module also defines `CautionAcknowledgement`, -`DecisionDebriefRequested`, and `HoldClaimReleased`, real Run-stream -events that don't carry the `Run` prefix. A name-prefix filter would -have silently exempted them. - -The deny-list covers slice 13's own field (`observed_path`, -`capture_path`, plus the wire-level `full_file_name`), slice 14a's -proposal/ESAF/ESAF-DOI fields (`proposal_number`, `esaf_number`, -`esaf_doi_number` -- vaulted, never harvested onto an event: see -`cora.run.aggregates.run.experiment_identity`'s module docstring for -the full argument), and the `User*` PVs slice 14b already named as -blocked (`project_witnessed_run_prelive_slices.md`): a directory/proposal -composition embeds a surname, and those PVs carry a name, badge, and -email directly. Widen it whenever a new identifying field is found on -the substrate. -""" - -import ast -from pathlib import Path - -import pytest - -from tests.architecture.conftest import CORA_ROOT - -_EVENTS_FILE = CORA_ROOT / "run" / "aggregates" / "run" / "events.py" - -# Any dataclass field anywhere in the events file whose annotation -# target name matches one of these strings counts as a violation. -_PII_FIELD_NAMES = frozenset( - { - "observed_path", - "capture_path", - "full_file_name", - "path", - "directory", - "file_path", - "surname", - "proposal_number", - "esaf_number", - "esaf_doi_number", - "user_name", - "user_last_name", - "user_badge", - "user_email", - "user_institution", - } -) - - -def _pii_field_violations(source_path: Path) -> list[str]: - """AST-walk every class in `source_path`'s dataclass fields for a - PII deny-list hit. Takes a path (not a hardcoded file) so the - seeded-violation meta-test below can call this SAME function - against a temp file, rather than maintaining a second copy of the - walk that could silently drift from what actually runs. - """ - tree = ast.parse(source_path.read_text()) - violations: list[str] = [] - for node in tree.body: - if not isinstance(node, ast.ClassDef): - continue - for stmt in node.body: - if not isinstance(stmt, ast.AnnAssign): - continue - target = stmt.target - if isinstance(target, ast.Name) and target.id in _PII_FIELD_NAMES: - violations.append(f"line {stmt.lineno}: {node.name}.{target.id}") - return violations - - -@pytest.mark.architecture -def test_run_event_payloads_carry_no_pii() -> None: - """Pin: dataclass fields named like PII, PLUS the slice 14a - proposal/ESAF/ESAF-DOI fields, never land on any event class in - `cora/run/aggregates/run/events.py`. - - The observed capture path is personal data (2-BM's directory layout - embeds a surname and a proposal number); it lives in the - `run_capture_path` vault accessed via `CapturePathStore`, never on - `RunCompleted` / `RunAborted` or any other event. A regression here - usually means someone tried to carry the resolved path (or a raw - `User*` PV) onto an event for convenience; move it to the vault - instead. - - `proposal_number` / `esaf_number` / `esaf_doi_number` are NOT personal - data (institutional identifiers for a funded experiment), so their - presence here widens this test's scope past pure PII: it also - enforces slice 14a's own decision that a value auto-harvested off - an unauthenticated channel, with no operator gesture behind it, - must never ride an immutable, INSERT-only event regardless of - whether it identifies a person. See - `cora.run.aggregates.run.experiment_identity`'s module docstring - for the full argument. A regression here usually means someone - tried to carry one of these three values onto `RunStarted` for - convenience; move it to `run_experiment_identity` via - `ExperimentIdentityStore` instead. - """ - violations = _pii_field_violations(_EVENTS_FILE) - assert not violations, ( - "Run event payloads must carry NO PII and none of the slice 14a " - "experiment-identity fields; move identifying fields to " - "run_capture_path / run_experiment_identity via their stores (see " - "memory/project_witnessed_run_prelive_slices.md, slices 13 and " - "14a):\n " + "\n ".join(violations) - ) - - -@pytest.mark.architecture -def test_run_events_file_is_present() -> None: - """Sanity: the events.py file must exist; the file move below the - aggregate folder would silently make the PII-deny scan a no-op - without this guard.""" - msg = f"Expected Run events file at {_EVENTS_FILE}" - assert _EVENTS_FILE.exists(), msg - - -@pytest.mark.architecture -def test_pii_deny_list_actually_finds_violations_when_seeded(tmp_path: Path) -> None: - """Meta-test: confirm the ACTUAL production walker (`_pii_field_violations`, - not a re-implemented copy) flags a seeded violation, on a class with - no `Run` prefix -- the exact shape `CautionAcknowledgement` / - `DecisionDebriefRequested` / `HoldClaimReleased` already have in the - real file, which a name-prefix filter would have missed. - - Guards against two silent-pass failure modes at once: a future - refactor moving the event classes to a sub-module (the walker - quietly stops seeing them), and a future re-introduction of a - name-prefix filter (the walker stops seeing non-`Run`-prefixed - events). - """ - seed_file = tmp_path / "seed_events.py" - seed_file.write_text( - "from dataclasses import dataclass\n" - "@dataclass\n" - "class HoldClaimReleased:\n" - " run_id: int\n" - " observed_path: str # PII violation seeded by the meta-test\n" - ) - violations = _pii_field_violations(seed_file) - assert violations, "seeded `observed_path` field must be flagged by the deny-list walker"