Skip to main content

ADR-0029 — PII detection stays inside the trust boundary

  • Status: Accepted (founder-approved 2026-08-05 on #2155, comment 5190788895). Ratifies the architect decision document (comment 5162501395) as corrected by comment 5189541301. Decided, not implemented — #2155 stays open until the detector integration lands, and the prod MEMORY_EXTRACTION_ENABLED gate stays OFF.
  • Date: 2026-08-03 (proposed) · 2026-08-05 (accepted)
  • Deciders: Principal Architect (author + technical recommendation). Founder sign-off required and given — this carries two items from the human-decision escalation list: a new artifact entering the supply chain, and a change to the at-rest security posture.
  • Refs: #2155 (this decision; prod gate) · #2134 (shipped the Detector seam without an implementation, deliberately) · #2095 (quarantine-on-hit) · #2432 / PR #2440 and #2486 / PR #2489 (the zero-dependency regex hardening described below, already merged) · #2069 (AML LLD) · #2037 (PRD) · tracker #2063 · geap-eval #1189/#1197 (separate Model Armor assessment) · #1864 (platform trust root). Composes with ADR-0024 (governance surfaces) and ADR-0025 (egress posture — this ADR deliberately adds nothing to it).

Context​

Memory extraction scrubs every candidate through a detector seam — Detector.Scrub(s string) (out string, classes []string), installed process-wide by SetDetector(DetectorFromEnv()) from cmd/context-engine (internal/context/memory/redact/detector.go). #2134 shipped the seam and a regex denylist behind it (secret, ssn, cc, email, phone, ip) but deliberately no real NER/DLP implementation, because choosing one is a new-dependency decision reserved for the founder.

The review verdict on #2134 was that the regex denylist is fail-open for unknown-class PII — names, dates of birth, foreign national IDs, international phone formats. That remains true, and it is the reason this issue exists.

The evidence that changed the framing: the detector's first live hit was a false positive, and it destroyed data​

Verified directly against beta-dev agent_memory, not taken on report. Memory e26798dd-babf-4693-a48d-40f3c4bb12c6 (work_artifact, pending, created 2026-08-02 17:42:20Z, extractor_version=v1) held, verbatim:

console.log(formatTimestamp(170[redacted:phone]));        // Unix timestamp (ms)

A 13-digit Unix epoch-ms literal in a JavaScript code sample was matched as a phone number. The root cause is exact and was reproduced in a scratch harness: the phone pattern had no leading \b, so it slid into the middle of any long digit run and consumed the trailing ten digits.

(?:\+\d{1,3}[\s.-]?)?\(?\d{3}\)?[\s.\-]?\d{3}[\s.\-]?\d{4}\b
                     ^ no left boundary

Two harms, both realised:

  1. Irreversible corruption. agent_memory has no column holding the pre-redaction original — the schema carries content and content_hash and nothing else. The original digits were never stored. The row could not be repaired, only rejected and re-extracted; it was rejected through the production review.Reviewer.Reject path on 2026-08-05, blast radius verified as exactly one memory row plus one audit row.
  2. Wasted governance attention. A redaction hit forces status='pending' and can only escalate, never de-escalate (extraction/gate.go). The false positive overrode the work_artifact exemption and consumed a reviewer slot — and the reviewer was shown 170[redacted:phone], which destroys the very evidence needed to adjudicate it.

Observed precision of the quarantine-on-hit path to date: 0/1. Exactly one redaction hit has ever fired in this deployment and it was wrong. Small n, but it is 100% of the evidence available.

Both failure directions are live, and they are coupled​

The regex detector is fail-open on the classes it does not know and fail-loud-and-destructive on the classes it thinks it knows.

This is why the mutation model is a prerequisite for the detector swap rather than a parallel improvement. A better detector fires more often; under the current mutation model, firing more often means corrupting more artifacts. NER adds PERSON, LOCATION, ORG, DATE — high-frequency classes that occur constantly in legitimate engineering content (variable names, sample data, commit authors, city names in fixtures). Dropping a real NER model behind this seam without fixing mutation upgrades one corrupted artifact into many, plus a reviewer queue that swamps the review gate.

What already shipped, and what it does not settle​

Three zero-dependency regex fixes needing no decision were separated out and merged in PR #2440 (#2432), with residual acceptance pins in PR #2489 (#2486): a leading \b on the phone pattern, a Luhn post-filter plus a 13–19 digit-count check on cc, and a routable-address filter excluding loopback / RFC1918 / link-local / broadcast from ip.

Measured false positives, using the corrected figures (the original write-up's summary sentence named a residual cc false positive that its own table did not contain — 4 phone + 5 ip = 9 accounts for the whole total, so that corpus held zero cc false positives and the clause was a statement about the class presented as a measurement; corrected on #2155):

corpusbaseline+ phone \b+ routable-ip+ Luhn cc
architect (phone 4, cc 0, ip 5)9500
shipped detector_fp_test.go (phone 5, cc 4, ip 8)171240

The delta between the two is corpus size, not behaviour; both were reproduced from a single probe building all four detector configurations from the same shipped code. Two residuals were deliberately left and are recorded here so nobody "helpfully" closes them by widening a pattern:

  • +44 20 7946 0958 (international format) is a known false negative in both the old and the new pattern. Broadening the pattern to catch it is precisely the move that caused the incident above. It stays fail-open until NER lands.
  • A Luhn-valid non-card beginning 3–6 is redacted as cc and is genuinely indistinguishable from a PAN (order reference 4111111111111111 → [redacted:cc]; internal id 1234567890123452 does not fire, because the pattern anchors on [3-6]). This is an accepted residual, pinned as a tripwire asserting that it does redact.

None of this settles the decision. It reduces the destructive-FP rate on three known-shape classes; it adds no coverage for the unknown-class PII that gates production.

Decision​

D1 — Detector backing is a self-hosted NER model, running in-cluster. Managed cloud DLP is rejected as the platform default.​

A Presidio-class or distilled-transformer NER, served locally. Extraction is asynchronous, so the latency budget is genuinely generous and CPU-only inference is viable; no GPU is required at our volume.

BYOK is the deciding constraint. Not latency, not cost — those merely fail to argue the other way. We sell per-tenant model choice and per-tenant data control. A tenant who deliberately chose Anthropic or Azure BYOK specifically to avoid GCP would, under a managed-DLP default, have 100% of their session-derived content inspected by Google — silently, as a platform default, by a vendor they never chose. That is a sub-processor we would have to disclose and a promise we actively sell on that we would be breaking. Any future contributor asking "why don't we just call Cloud DLP?" should stop at this paragraph.

The supporting reasons, in descending weight: extraction is async, which removes the usual reason to prefer a managed API; the cost shape is fixed compute headroom rather than usage pricing that scales with exactly the thing we want to grow; and it is the only option that adds a capability without expanding the trust boundary — no new egress allowlist rows, no new secrets, no new sub-processor.

Supply chain. The model weights plus inference runtime are a new artifact in the supply chain, pinned by digest, vendored into the image build, and scanned by the existing Container Image Scan gate. This is the supply-chain approval the seam in #2134 was withheld for, and it is the substance of the founder sign-off recorded above.

Failure behaviour is unchanged from #2155 scope item 4: detector unavailable → fall back to the regex denylist and quarantine all hits. Never fail-open silently.

D2 — Class-tiered mutation, and it lands before any detector upgrade.​

The key observation: quarantine already contains the risk; mutation is redundant for a quarantined row and is the sole source of irreversible damage. A pending row is not recalled into an agent's context (subject to D5.2 below), so redacting it protects nothing an agent could ever have seen — it only destroys the reviewer's ability to judge.

TierClassesBehaviour
High — self-evidencing, live-credential risksecret (ghp_/xoxb-/sk-/AKIA/PEM/bearer), cc, emailMutate + quarantine. Unchanged from today. A live credential is never left at rest, even in a quarantined row.
Low — heuristic, shape-basedphone, ip, bare digit runs, and any future NER class below a confidence thresholdFlag + quarantine, do NOT mutate. Record the class on the row; leave content intact. The reviewer sees the original and decides: approve, or reject-and-redact.

Note for the implementation: the cc pattern already carries luhnValid as an accept filter post-#2440, so every cc hit is Luhn-valid by construction. The "Luhn-valid cc" qualifier in the original proposal is now satisfied by the pattern itself and must not be built a second time.

D3 — The at-rest posture change is founder-approved, and rests on two named premises.​

This is recorded explicitly because it is a security-posture change, not an architect assumption. What was accepted:

A pending row may hold un-redacted low-tier content at rest.

Justified by exactly two premises:

  • (a) Org scoping plus an already-authorised reader. The row is org-scoped, and the reviewer is a human in that same org who is already authorised to see the source session the content was extracted from. The reviewer learns nothing they could not already read.
  • (b) pending rows are never recalled into an agent's context. No agent can obtain the content by any path.

The high tier exists precisely so that live credentials are never subject to this trade.

If either premise stops holding, this ADR must be revisited before the low tier keeps shipping. Premise (a) breaks if memory review is ever delegated outside the owning org, or to a principal without access to the source session. Premise (b) breaks if any agent-reachable read path over agent_memory stops filtering status='active' — see D5.2, which is a live instance of exactly that and is a prerequisite, not a follow-up.

The rejected alternative to D2/D3 was preserving the pre-redaction original in an access-controlled store (new column or side table, L5-gated read, its own retention clock). That is strictly more machinery, more at-rest secret surface, and a new retention obligation. It remains a legitimate fallback if D3's premises cannot be held.

D4 — Sequencing, and the production gate.​

  1. Zero-dependency regex fixes — done (#2440 / #2489).
  2. D2/D3 class-tiered mutation — the at-rest call is made; implementation may proceed.
  3. D1 detector integration behind the existing seam — may not land before step 2. A detector integration PR that arrives first will not be approved.
  4. Prod MEMORY_EXTRACTION_ENABLED stays OFF until 1–3 land. Unchanged from #2155.

D5 — Two items that bind the implementation and were not settled by the approval.​

D5.1 — ssn is in neither tier, and defaults to the conservative arm. The approved tier table assigns secret/cc/email to high and phone/ip/sub-threshold-NER to low. ClassSSN is a live class (\b\d{3}-\d{2}-\d{4}\b, no checksum, therefore heuristic) and appears in neither. The approved change is a relaxation; an unlisted class simply does not receive it. So ssn mutates, as it does today — preserving current behaviour requires no new approval. Assigning it to the low tier would be a further at-rest relaxation and needs its own decision.

D5.2 — PullContext does not filter status, so premise (b) is not universally true today. Four recall paths correctly filter status = 'active' (runtime/session/warmstart.go, runtime/planmemory/recaller.go, context/assembly/memory_loader.go, context/memory/mcpsrv/pgbackend.go, backed by the partial idx_memory_active_recall). But findLatest (internal/context/memory/service.go:495), reached from the agent-callable PullContext RPC (:272), selects FROM agent_memory WHERE org_id … AND agent_id … ORDER BY updated_at DESC LIMIT 1 with no status predicate. A freshly quarantined row is by construction the most recently updated row for that agent, so PullContext returns it. findBySnapshot is unaffected (extracted rows carry no snapshot_name).

This does not change D1 or D2 — it is a pre-existing defect, and today it is masked because every quarantined row is also mutated. D3's low tier removes that masking, converting it into an agent-reachable read of deliberately un-redacted content. Adding AND status = 'active' to findLatest is therefore a prerequisite of D2/D3, and must land in or before the class-tiered mutation change.

Consequences​

Positive.

  • Uniform per-tenant behaviour. Every tenant gets the same PII floor, and no third party sees any tenant's content for detection. This decision adds nothing to a DPA, and adds no new place a BYOK tenant's data travels to. Read this bullet with the scoping note below — it is a claim about detection, not about tenant content generally.
  • The trust boundary does not move. Zero new egress allowlist rows, zero new secrets, zero new sub-processors, no GPU. ADR-0025's egress posture is untouched by this decision. Again: does not move — not is unbroken. See the scoping note.
  • Bounded cost. Fixed CPU/RAM headroom on the context-engine workload (or a small sidecar), with no per-candidate marginal cost — as opposed to usage pricing that scales with agent activity. Exact sizing is deferred to the LLD.
  • The destructive-FP class is closed at its root, not merely narrowed: a low-tier hit can no longer destroy an artifact, and the reviewer adjudicates against the original text rather than against [redacted:…].

Negative / accepted cost.

  • A pending row may hold un-redacted low-tier content at rest (D3), valid only while premises (a) and (b) hold. D5.2 is an open breach of (b) that must be closed first.
  • International-format PII stays fail-open until NER lands. +44 20 7946 0958 is a known, deliberate false negative. It is not repaired by widening the regex — that is the move that caused the incident in Context.
  • The model artifact must be maintained: digest pin, image size, scan findings, and a refresh cadence for the weights. This is real ongoing work that a managed API would have absorbed.
  • A local inference dependency on the extraction path. Mitigated by D1's fallback (regex + quarantine-all) and by extraction being async, but the workload now has a component that can be slow or absent.
  • Per-tenant detector variance is deferred behind a known seam limitation. The current seam cannot express a per-tenant detector: active atomic.Pointer[Detector] is process-global, activeDetector() takes no arguments, and Scrub(s string) has no tenant in its signature. Supporting a per-tenant choice requires reworking the seam from process-global to per-request resolution, threading tenant context through RedactContent → scrubValue → Scrub. That is a real but modest refactor, and it is deliberately not paid for now — it is built when a tenant actually asks for a managed-DLP tier, not speculatively. Recorded here so the cost is known and the seam's limit is documented rather than rediscovered.

Scope of the trust-boundary claim — the embedder already crosses it​

This ADR's boundary claim is a statement about the delta of this decision, and is scoped to PII detection. It is not a claim that the trust boundary around tenant content is currently unbroken. It is not.

Which default, at which layer — the two disagree, and both are third parties. The code default is openai: envStr("EMBEDDING_PROVIDER", "openai") (cmd/context-engine/config.go:298), and Validate() treats "" and "openai" as the always-allowed default arm. The deployed default is vertex: EMBEDDING_PROVIDER: "${EMBEDDING_PROVIDER:-vertex}" (docker-compose.dev.yml:627, project upsquad-geap-eval, model gemini-embedding-001; founder decision 2026-06-15, #1403), wired at cmd/context-engine/main.go:358. So the code default and the deployed default name two different external vendors — OpenAI and Google. This makes the point harder, not softer: there is no configuration of the shipped defaults in which tenant content stays inside our boundary, and which third party receives it depends on which layer you read.

The surface is the whole retrieval stack, not just memory. EmbeddingProvider's own doc comment (config.go:104-105) states it "selects which Embedder backs the retrieval + knowledge IngestDocument pipelines." One embedder instance is shared by four consumers in main.go: the RAG document-ingest pipeline (:401), the retrieval service (:431), memory assembly recall (:472), and the memory embed worker (:606). So the crossing covers ingested knowledge documents, memory content, and the query strings submitted at retrieval and recall time — queries are embedded too, and a query is often more sensitive than the corpus.

What is actually measured, and what is prospective. Verified on beta-dev at time of writing: agent_memory holds 69 rows, 20 active; agent_memory_embeddings holds 20 rows, all embed_model = vertex-gemini-embedding-001-1536, sent to <location>-aiplatform.googleapis.com:443 (internal/context/embedding/vertex_embedder.go:129) under a platform ADC service-account key. That is 100% of active memory content — but it is a dev-org corpus. Prod extraction is OFF, and no tenant has yet chosen Anthropic or Azure BYOK, because P0.1.3 / P0.1.4 are unbuilt. The exposure is therefore prospective, not a live breach of a customer commitment: the existing BYOK requirements (P2.2.7, SEC.2.3/2.5, P1.2.13) govern key custody, not content routing, and P2.2.5 is completion-path only. The argument is that a tenant who would choose Anthropic or Azure BYOK specifically to avoid GCP would, on today's deployed default, have their content embedded by Google anyway — and that is a commitment we would breach on the day we sell it, which is why it is recorded now rather than after.

(Stating this precisely matters: an earlier draft of this section asserted the BYOK-tenant harm as present fact. It is not, and presenting a hypothetical as a measurement is the exact defect this ADR's own Context section corrects in the "residual Luhn-valid cc false positive" that its table never contained.)

Three things follow, and they are recorded here so nobody has to rediscover them:

  1. It is not covered by a disclosed sub-processor arrangement. P0.1.4 — the DPA template and its sub-processor list (docs/UpSquad_Complete_PRD.md:800, :1947) — is an unbuilt PRD line item, not a shipped artifact. There is no sub-processor list on which either embedding vendor — Google (deployed default) or OpenAI (code default) — has been disclosed. "It's a disclosed sub-processor" is therefore not available as a defence today; it is something that would have to be made true.

  2. On this ADR's own chosen axis, the incumbent scores worse than the option rejected. The Option-B rejection leans on tenant_egress_allowlist being tenant-scoped, so managed DLP would mean a per-tenant row naming a vendor the tenant never chose — visible in the tenant's own egress surface. The Vertex embedder does not appear in tenant_egress_allowlist at all; it is a platform-internal SDK call on a platform credential, so it is invisible to the tenant. An ADR that rejects Option B for a property its own status quo violates — and violates less visibly — invites exactly the relitigation this ADR exists to prevent. Hence this note.

  3. This does not weaken D1; it strengthens it. An existing un-consented crossing — to Google or OpenAI depending on layer — is an argument against adding a second one, not a precedent licensing it. D1's BYOK reasoning stands unchanged, and is easier to honour while the count of such crossings is still small.

The embedder crossing is explicitly out of scope here and is not resolved by this ADR. It is a separate founder decision (revisiting #1403): accept-and-disclose it via P0.1.4, or close it with a zero-egress local embedder. The latter is not a config change — OpenAIEmbedder's endpoint is a hardcoded const with no base-URL override, and the realistic local model (nomic-embed-text, 768-d) cannot reach the vector(1536) column and HNSW index fixed by migration 170, so it is a schema change. It is also blocked on #2501: the two memory recall paths do not filter embed_model, so switching embedder on a populated corpus silently mis-ranks across incompatible vector spaces. Do not treat a local embedder as a cheap prerequisite of anything in this ADR.

Generalisation for whoever reads this next. The principle D1 actually establishes is about inference on tenant content — memory rows, ingested knowledge documents, and query strings alike — of which PII detection is one instance and embedding is another. Detection is being governed here because its evidence base is detection-specific and strong. Any new inference call over tenant content — reranking, classification, summarisation, enrichment — inherits D1's reasoning and needs the same BYOK question asked of it before it ships.

Security. No new external surface from this decision (see the scoping note above for the surface that already exists). Detection strictly improves relative to the regex denylist for unknown-class PII, which is the gap gating production. The one posture regression is D3, which is scoped to the low tier, bounded by its two premises, and blocked behind D5.2.

Metrics. memory_redaction_hits_total{pattern_class} gains the NER classes. The label set must stay bounded — the enum in redact/detector.go is the only permitted source of that label, and a raw match must never reach it. The existing containment guard (which iterates the detector's patterns) type-asserts *regexDetector and will therefore not see NER classes; extending it to cover the new detector is part of the integration, not an afterthought.

Alternatives rejected​

Option B — managed cloud DLP (GCP Cloud DLP / Model Armor) as the platform default. Rejected.

Every memory candidate — raw tenant session content — would leave our trust boundary to Google. Concretely: tenant_egress_allowlist is tenant-scoped, so reaching a managed DLP means seeding a row per tenant pointing at a vendor that tenant never chose, visible in the tenant's own egress surface. It also requires a GCP SA credential or workload-identity binding (a new secret surface, itself a founder item), a new sub-processor and DPA obligation, and usage pricing that scales with agent activity — the unbounded-by-default shape is the concern, not the unit price. We do have a GCP relationship, and geap-eval #1189/#1197 are separately assessing Model Armor, which lowers the integration cost; it does not lower the data-residency cost, which is what decides this. Disqualified as a default by the BYOK argument in D1.

Option C — hybrid: self-hosted as the platform default, managed DLP as an explicit per-tenant opt-in. Rejected as premature, not as wrong.

Sound in principle: the tenant consents to their own vendor. Two reasons not to build it now. First, it requires the per-tenant seam rework described under Consequences, and that cost should be paid when a tenant asks, not speculatively. Second, it creates a documented tier difference — two tenants receiving materially different PII floors — which must be an explicit, named product tier and never a silent difference, or we cannot honestly describe our own guarantees. D1 leaves C available later at a known, modest cost.

Preserving the pre-redaction original in an access-controlled store, instead of D2/D3. Strictly more machinery, more at-rest secret surface, and a new retention obligation, to reach a weaker version of the same outcome. Retained as the fallback if D3's premises fail (see D3).

Edit this page
Last updated on by upsquad-devops-engineer[bot]