fix(governance): cascade a drop or keep_private to rows derived before the verdict

[Eldad-Caura]

Closes audit finding H-10.

The defect

On a deferred deployment — the production SaaS write path — the auto-chunk branch's GovernanceDecision runs with enrichment=None, takes its documented uncertain branch and enforces nothing. The children are built and committed immediately, at scope_team, with no governance metadata of their own.

The parent's real verdict arrives minutes later (ENRICH_REQUESTED → worker PATCH → ENRICHED → remediate_after_enrichment) and soft-deleted or downgraded only the row the event named. parent_memory_id was written onto every child and queried nowhere — five sites in production code, all writes.

So a tenant configured non_business.disposition=drop had its parent dropped and audited while N children carrying the same content stayed live and team-visible. Permanently: children are never enriched, so nothing revisits them, and they carry clean metadata so any future sweep reads them as fine. No audit row tied them to the drop either — the compliance log recorded one deletion where one row was removed and N were not.

Reproduced before fixing

AssertionError: the dropped content survives in the children: {'m1'}

Why this is the #808 shape, and why ordering can't fix it here

The codebase documents #808 as fixed, and it is fixed for the atomic-fact fan-out — by ordering. That path lives inside _enrich_memory_background, runs remediation before it fans out, early-returns on a drop, and carries a downgrade onto the children via effective_visibility. Its own comment states the rule:

a policy that could not be applied must not be followed by rows it might have forbidden

Auto-chunk in deferred mode is the path where the derived rows already exist when the verdict lands. Ordering cannot save it; a cascade is the missing half.

The fix

remediate_after_enrichment now resolves the derived rows and applies the same action: soft-delete on either drop disposition, visibility downgrade on keep_private.

Both destructive branches, not just the one the finding described. PII-drop and non-business-drop are separate branches reading separate configs; a tenant on a PII drop policy leaked identically.

The lookup runs before the parent's audit and delete, so a lookup failure leaves everything intact and remediable rather than a dropped parent whose children were never found. Failures propagate rather than being swallowed — this module's contract is that a policy which could not be applied must not be quietly treated as applied, and the enclosing tracked_task surfaces it.

Each cascaded row gets its own audit row, carrying cascaded_from so a review can see why a row with no governance signals of its own was removed. Per child rather than one rolled-up entry: each is a separate soft-delete, and a log recording one deletion while N happened misstates the record in the direction that matters.

The new storage query

The parent→child link lives in child metadata JSON and nothing could read it. Tenant-scoped, live rows only, no status or visibility filter — remediation must reach every derived row whatever state it is in, the same reasoning memory_find_by_supersedes_id records for retraction.

Why it is gated on auto_chunked

The query filters a JSON key with no supporting index, and a tenant configured drop remediates constantly — an ungated version would tax every ordinary drop to serve the rare chunked one.

auto_chunked is safe to gate on for a reason worth stating: it is stamped unconditionally in the same function that builds the children, and it is already on the production rows this has to reach. A new marker would only appear on rows written after the deploy and would leave the existing leak in place.

Tests

Five in core-api, four confirmed failing without the fix. The fifth is an over-refusal guard: an ordinary row must not run the lookup at all.

Four in core-storage-api against real Postgres, because the core-api side stubs storage entirely — without them the cascade could ship with a query that matches nothing and every test above it would still pass. The tenant-boundary one is probe-confirmed: removing the tenant predicate fails it with 2 == 1, which on a drop would mean deleting another tenant's rows.

One test lives beside the write rather than the remediation. The gate depends on auto_chunked being stamped on the parent and parent_memory_id on each child; if either stops being written the cascade silently stops running with nothing failing near it. Asserting that in the remediation tests would be a stub asserting itself.

Verification

• Full root suite: 6076 passed, 5 skipped, 1 xfailed, 0 failed.
• core-storage-api/tests/: 336 passed, on its own scratch database.
• ruff check and ruff format --check run separately at CI's exact scopes — clean.
• mypy clean on core-storage-api/src/; core-api/src/ clean apart from 2 pre-existing types-python-dateutil stub errors in an untouched file.
• Broker OpenAPI baseline — current. (The new endpoint is on core-storage-api, which the broker contract does not cover.)
• legacy_name_ratchet.py → No new lines. · do_not_touch_sentinel.py → All 35 protected strings survive (the 39→35 change came from fix(clients): retire the MemClaw/MemClawError/MemClawAPIError class aliases #1284 on main, confirmed against a clean tree, not from this branch) · tenant_scope_gate.py → exit 0, no allowlist movement, and the new route is counted as tenant-bound.
• Checked for an open PR on this subsystem before starting; none.
• Branched from origin/main, rebased onto 4c9cef2a.

🤖 Generated with Claude Code

[Eldad-Caura]

@claude please review at head 48db742f. This is a security fix that deletes rows, so I would rather have it attacked than approved.

Five things I want challenged:

1. The gate is the weakest point and I know it. The cascade only runs when the parent carries auto_chunked. I argue that is safe because the marker is stamped unconditionally in the same function that builds the children, and because it is already on production rows (a new marker would leave the existing leak in place). But it is still a correctness dependency on a metadata key, which is this backlog's recurring defect class. Tell me if you would drop the gate and pay the query cost, or if there is a third option I have missed.

2. Does the cascade reach every affected row, or only the ones I thought of? I fixed both drop branches and the keep_private branch. Check whether any other disposition or path leaves derived rows behind — the PII mask/flag branch deliberately does nothing, and I believe that is right because it neither deletes nor restricts, but check that reasoning.

3. Deleting rows on a JSON-key match. metadata->>'parent_memory_id' is not a foreign key and nothing constrains what writes it. If a caller could put an arbitrary parent id into their own row's metadata, could they induce a cascade to delete rows they do not own? I believe the tenant predicate contains this and that within a tenant the writer already controls those rows, but this is the question I am least sure of and it is the one with the worst answer if I am wrong.

4. Failures propagate on purpose. A lookup or child-delete failure raises out of remediation rather than being swallowed. That matches this module's existing posture, but it means a transient storage error now fails a remediation that previously would have completed for the parent. Check I have not made a partial-failure state worse: the parent may already be deleted when a child delete fails.

5. The over-refusal guard. One test asserts the lookup does NOT fire for an ordinary row. Confirm that is testing something real and not just the gate asserting itself.

Not in scope, deliberately: H-02, the sibling finding where entity rows mined from dropped content survive the same way. It needs its own cascade in a different table and is a separate PR.

[github-actions]

🤖 Review by Claude Code

Summary

This PR adds an H-10 governance cascade so that a drop/keep_private verdict on an auto-chunked parent memory also reaches child rows created before the verdict arrived. The storage-layer query, router endpoint, and client method all closely mirror the existing find_by_supersedes_id convention, and the remediation logic is carefully sequenced (audit-before-mutate, lookup-before-delete) to match the pattern already used for the parent row itself. Overall the implementation is well-reasoned; I found one real resilience gap worth calling out.

Medium/Low Issues

Cascade loop has no partial-failure containment once the parent is already deleted

Severity: Medium
File: core-api/src/core_api/services/governance_remediation.py:88-183 (the _drop_children/_privatise_children helpers and their call sites in remediate_after_enrichment)
Problem: _pre_verdict_children is deliberately looked up before the parent's audit/delete so a lookup failure leaves everything intact, but _drop_children/_privatise_children run after the parent has already been soft-deleted (or downgraded) — if the audit or delete/update call for the 2nd/3rd/... child raises, the loop stops, the exception propagates out of remediate_after_enrichment, and the remaining children are left live/team-visible with the parent already gone, defeating the very leak this feature closes; a naive retry of the whole event would also re-emit a duplicate "parent dropped" audit entry since the parent audit already ran.

🤖 Claude Code Prompt

In core-api/src/core_api/services/governance_remediation.py, the loops in
_drop_children (around lines 88-121) and _privatise_children (around lines
124-155) call emit_governance_audit + soft_delete_memory/update_memory
sequentially per child, after the parent's own audit+delete/update has already
committed. If one of these per-child calls raises partway through the loop,
the exception propagates up through remediate_after_enrichment with no
containment, leaving already-processed children remediated but the rest of
the children still live/visible even though the parent is already gone — and
a retry of the full remediation would re-run the parent's audit+delete a
second time (duplicate audit entry, and soft_delete_memory called again on an
already-deleted row).

Consider wrapping each child's audit+mutate in a try/except inside the loop so
one child's failure doesn't abort the remaining children, logging the failure
loudly (per existing convention: "still holds dropped content and was NOT
removed" style) and continuing, then surfacing an aggregate error/metric after
the loop if any child failed, rather than letting the first failure silently
stop the cascade partway through with an exception that also risks re-driving
the already-completed parent-level audit/delete on retry.

---

Cascade summary log count includes children skipped for missing ids

Severity: Low
File: core-api/src/core_api/services/governance_remediation.py:113-121 and :150-155
Problem: The final logger.info("governance: cascaded %s to %d derived row(s) of %s", ...) uses len(children), which counts every row returned by the lookup — including any skipped via continue when _child_id returned None — so the log overstates how many rows were actually remediated when an id is missing.

🤖 Claude Code Prompt

In core-api/src/core_api/services/governance_remediation.py, in both
_drop_children (the "governance: cascaded %s to %d derived row(s) of %s" log
around line 113-118) and _privatise_children (the equivalent log around line
150-155), the count logged is len(children), which includes rows skipped
earlier in the loop due to a missing id (the `if not cid: continue` branch).
Track a separate counter incremented only when a child is actually
audited/soft-deleted (or updated+audited), and log that count instead so the
summary log doesn't overstate successful remediation when some rows were
skipped.

---

Reviewed by claude-sonnet-5 · cost $0.7520137499999999

[github-actions]

🤖 Review by Claude Code

Summary

The PR adds a parent→child lookup so governance remediation (drop / keep_private) cascades to auto-chunk children that were committed before a deferred governance verdict arrived. The storage-side query, router endpoint, and core-api wiring are consistent with existing conventions in the file, and ordering (children looked up before the parent's own audit+delete) is deliberate and well-reasoned. One correctness issue stands out around how the new lookup is transported.

Critical/High Issues

Governance cascade lookup goes through the read replica, contradicting _get_list's own stated invariant

Severity: High
File: core-api/src/core_api/clients/storage_client.py:485-496, 909-917
Problem: find_children_by_parent_id uses _get_list, which unconditionally issues the request against self._read_http/self._read_prefix (the replica when core_storage_read_url is configured), but _get_list's own comment states "none sit on the write path, so no per-call opt-out is needed yet" — this new caller is on the write path, since its result directly decides which children get soft-deleted or have their visibility downgraded.

🤖 Claude Code Prompt

In core-api/src/core_api/clients/storage_client.py, `find_children_by_parent_id`
(around line 909-917) calls `self._get_list("/memories/by-parent-id", ...)`.
`_get_list` (defined around line 485-496) always issues the request via
`self._read_http`/`self._read_prefix`, i.e. against the read-replica endpoint
when `core_storage_read_url` is configured, per its own comment "All current
_get_list callers are pure list/stats endpoints; none sit on the write path,
so no per-call opt-out is needed yet."

`find_children_by_parent_id` breaks that invariant: its result is consumed by
core_api/services/governance_remediation.py's `_pre_verdict_children` to decide
which auto-chunk children get soft-deleted (PII/non-business "drop") or have
their visibility downgraded ("keep_private"). If replica replication lag means
a just-committed child row (metadata.parent_memory_id) is not yet visible on
the replica when this lookup runs, the cascade will silently return an
incomplete child list. Because auto-chunk children are "never enriched, so
nothing revisits them" (per the docstrings in this same PR), a row missed due
to replica lag leaks permanently — exactly the leak this feature exists to
close, and it happens silently (no exception, no retry), unlike the module's
stated contract that "a policy which could not be applied must not be quietly
treated as applied."

Fix by having `find_children_by_parent_id` read from the primary/writer path
instead of the replica-eligible `_get_list` — e.g. add a `read: bool = False`
(or similar) option threaded through to a primary-routed request (mirroring
how `_post`/`_get` expose a `read` parameter), or introduce a small
write-consistent list helper for callers like this one that gate a destructive
cascade. Update the docstring/comment on `_get_list` once a per-call opt-out
exists so the invariant it states stays true for future callers.

---

Reviewed by claude-sonnet-5 · cost $0.8810329999999998

[Eldad-Caura]

Both correct, both fixed at 85068763 — and the Medium is the answer to the question I asked in point 4, which I had reasoned only half-way through.

The Medium

I argued failures should propagate because "a policy that could not be applied must not be treated as applied". That is right for the lookup, which runs before anything is mutated. It is wrong for the loop, which runs after the parent is already deleted — and I applied the same reasoning to both without noticing the asymmetry.

Reproduced before fixing: a failure on c1 escapes raw, and c2 and c3 are never touched.

RuntimeError: storage blipped on the first child

Parent gone, two children still live with the dropped content. The error handling reintroduced the leak the cascade exists to close — which is the same shape as the last round of #1278, where a loop-level catch would have cancelled every other child's repair.

Fixed per child, raised once at the end, after every child has been attempted:

• One bad row costs one row.
• GovernanceCascadeError still surfaces to tracked_task, so the unapplied policy is not swallowed.
• A retry is narrow rather than wholesale: the lookup excludes soft-deleted rows, so the children that succeeded are not revisited and only the failures are re-attempted.

On your duplicate-audit point — a retry does re-emit the parent's audit. I am taking that rather than fixing it here: it is the cost of not silently leaving forbidden content live, and it is pre-existing behaviour for any remediation that raises, not something this cascade introduces. Say if you'd rather I made the parent audit idempotent as part of this, but it felt like a separate change.

One thing I went further on than you suggested: a child with no usable id now counts as a failure rather than only being logged. "We could not identify it" is not "it is handled" — the row still holds forbidden content. A retry won't fix that one, so it should stop being quiet and get a person's attention.

The Low

Right, and it is the same defect class the rest of this backlog keeps turning up: a line asserting something the code does not deliver. len(children) counted rows the loop had skipped, so the summary claimed N remediated while N-1 were — in a log a compliance review reads. Now counts actual remediations.

Probe: reverting it fails the new test with governance: cascaded nonbusiness_drop to 2 derived row(s) of m1 when one was skipped.

Tests

Two more, both probe-confirmed in both directions:

1. One failing child does not abandon the rest — asserts c2 and c3 are still deleted and the error still raises. Fails with the containment removed.
2. The summary count excludes skipped rows. Fails with len(children) restored.

Suite 6078 passed / 0 failed; ruff, mypy, ratchet, sentinel, tenant-scope gate and the broker baseline all clean after git add. Checks were green on 48db742f.

Still open from my review request

Points 1, 2, 3 and 5 went unanswered and I am not reading silence as agreement — particularly 3, which is the one I flagged as having the worst answer if I am wrong: metadata->>'parent_memory_id' is not a foreign key, so can a writer induce the cascade to delete rows they should not reach? I believe the tenant predicate contains it, and the storage test pins that boundary, but I would like it checked rather than assumed.

[Eldad-Caura]

@claude re-review at head 85068763. Your Medium and Low are both fixed, per-child containment with a single aggregate raise — details in the comment above.

Please also return to the questions from my first request, which the last pass did not reach. In priority order:

1. The JSON-key delete (my original question 3). metadata->>'parent_memory_id' is not a foreign key and nothing constrains what writes it. Can a writer put another row's id in their own metadata and induce the cascade to delete rows they should not reach? The tenant predicate is my defence and there is a storage test pinning it, but this is the question with the worst answer if I am wrong, and it is still unexamined.

2. The gate. The cascade only runs when the parent carries auto_chunked. Is that a correctness dependency you would accept, or would you drop the gate and pay the unindexed query cost on every drop?

3. Coverage. I fixed both drop branches and keep_private. The PII mask/flag branch deliberately does nothing because it neither deletes nor restricts. Check that reasoning, and check no other path leaves derived rows behind.

4. The over-refusal guard asserting the lookup does NOT fire for an ordinary row — is that testing something real, or is it the gate asserting itself?

[github-actions]

🤖 Review by Claude Code

Summary

This PR adds a governance-cascade mechanism (H-10) so that when a parent memory is dropped or made private, auto-chunked child rows created before the verdict arrived are swept up too. The core-storage-api query and endpoint additions are sound and mirror existing patterns. However, the keep_private branch in governance_remediation.py has an ordering regression that can suppress the parent's own compliance audit entry.

Critical/High Issues

Cascade failure silently drops the parent's own audit entry for keep_private

Severity: High
File: core-api/src/core_api/services/governance_remediation.py:335-360 (the keep_private branch of remediate_after_enrichment)
Problem: _privatise_children(...) is now called between sc.update_memory(memory_id, ...) (which changes the parent's visibility) and the parent's own emit_governance_audit(...) call; when any child fails, _privatise_children raises GovernanceCascadeError, so the parent's audit — the row's only compliance trace for the narrowing that already happened — is never emitted.

🤖 Claude Code Prompt

In core-api/src/core_api/services/governance_remediation.py, inside remediate_after_enrichment(), in the `if nb_cfg.disposition == "keep_private":` branch: the current order is

    children = await _pre_verdict_children(sc, memory_id, tenant_id, md)
    await sc.update_memory(memory_id, tenant_id, {"visibility": "scope_agent"})
    await _privatise_children(sc, children, tenant_id=tenant_id, parent_id=memory_id,
                               detail_for=lambda c: nonbusiness_audit_detail(ACTION_NB_KEEP_PRIVATE, c, "fast"))
    await emit_governance_audit(
        tenant_id=tenant_id,
        agent_id=agent_id,
        action=ACTION_NB_KEEP_PRIVATE,
        detail=nonbusiness_audit_detail(ACTION_NB_KEEP_PRIVATE, content, "fast"),
        resource_id=memory_id,
    )
    return RemediationOutcome(visibility="scope_agent")

Because _privatise_children() raises GovernanceCascadeError when any child fails (via _raise_if_any_failed), that exception propagates BEFORE the parent's own emit_governance_audit(...) call and BEFORE the function returns. This means: the parent memory's visibility is durably changed to scope_agent, but if even one derived child fails to be privatised, there is no audit row at all recording that the parent's visibility was narrowed — exactly the "untracked mutation" failure mode this module's own docstrings say must never happen (compare with the drop branches, where the parent's audit is emitted BEFORE the destructive delete and BEFORE the cascade to children, so a cascade failure there never affects the parent's own audit trail).

Fix by moving the parent's own emit_governance_audit(...) call (and ideally the `return RemediationOutcome(visibility="scope_agent")`) to run immediately after sc.update_memory(...) and BEFORE the call to _privatise_children(...), matching the ordering already used in the PII-drop and non-business-drop branches (mutate/audit the parent first, then cascade to children, letting cascade failures surface without swallowing but without erasing the parent's own audit trace). For example:

    children = await _pre_verdict_children(sc, memory_id, tenant_id, md)
    await sc.update_memory(memory_id, tenant_id, {"visibility": "scope_agent"})
    await emit_governance_audit(
        tenant_id=tenant_id,
        agent_id=agent_id,
        action=ACTION_NB_KEEP_PRIVATE,
        detail=nonbusiness_audit_detail(ACTION_NB_KEEP_PRIVATE, content, "fast"),
        resource_id=memory_id,
    )
    await _privatise_children(sc, children, tenant_id=tenant_id, parent_id=memory_id,
                               detail_for=lambda c: nonbusiness_audit_detail(ACTION_NB_KEEP_PRIVATE, c, "fast"))
    return RemediationOutcome(visibility="scope_agent")

This ensures the parent's own compliance audit is always recorded once its visibility mutation has happened, regardless of whether the child cascade later fails and raises GovernanceCascadeError.

Suggestions

Consider a supporting index for the parent/child JSONB lookup

File: core-storage-api/src/core_storage_api/services/postgres_service.py (new memory_find_children_by_parent_id)
Problem: The query filters on Memory.metadata_["parent_memory_id"].astext, which the docstring itself notes has no supporting index; this is gated behind auto_chunked to limit how often it runs, but as auto-chunk adoption grows this will still perform an unindexed JSON scan on every drop/keep_private hitting a chunked parent, synchronously in the remediation path. A partial expression index (e.g., on tenant_id plus the JSONB path, or a GIN index on metadata_) would bound this cost without requiring a schema migration for the field itself.

---

Reviewed by claude-sonnet-5 · cost $0.6522920000000001

[Eldad-Caura]

Correct, and it is a defect my previous fix created. Fixed at 724a71d6.

Making the cascade raise was right. Putting it where I put it was not: keep_private mutates the parent and audits it after — correct on its own, because nothing is lost if a non-destructive audit fails after the change — and I dropped the cascade in between. So one failed child raised before the parent's audit was ever emitted.

Reproduced before fixing:

AssertionError: the parent's visibility changed with no audit row: []

The parent durably narrowed to scope_agent with no record that it happened. An untracked mutation, which this module's own docstrings forbid.

Your comparison is the part that makes it obvious in hindsight: the drop branches never had this exposure because they audit before the destructive delete, which leaves the cascade already last. keep_private is the one branch whose natural audit position is after the mutation, so it was the one branch where inserting anything in between could move the audit behind a raise. I changed the ordering of a security-critical branch without re-checking what the ordering was protecting.

Parent audit now precedes the cascade, and a test pins it — reverting the order fails with the output above.

That is two rounds running where my fix caused the next finding

Round 2 was the loop-level abort abandoning later children; round 3 is this. Both were introduced by the previous round's fix, and both were in error-handling paths rather than the feature itself. Worth saying plainly rather than letting the PR body read as a clean progression.

On the index suggestion

Agreed and deliberately not here: a partial expression index on (tenant_id, metadata->>'parent_memory_id') needs a migration, and this PR is already a security fix touching two services. The auto_chunked gate bounds the cost meanwhile. I would rather it landed as its own change than as a fifth thing in this one — say if you disagree and I will fold it in.

Verification

Suite 6081 passed / 0 failed on origin/main at 13ebd5f7 (rebased; the branch had been rebased under me again, patch-id confirmed it was my own commit before I superseded it). ruff, mypy, ratchet, sentinel, tenant-scope gate and the broker baseline all clean after git add.

Still unanswered across three requests

Not treating silence as agreement — the JSON-key delete question in particular. metadata->>'parent_memory_id' is not a foreign key and nothing constrains what writes it, so I traced it myself while waiting:

The deletion set is keyed on other rows pointing at the parent, not on anything the parent's own writer controls. To make the cascade delete a row they should not reach, an attacker would need that row's metadata to name a parent they can get dropped — and they do not write other rows' metadata. The inverse (pointing their own row at someone else's parent) deletes their own row, which is self-harm rather than escalation. Cross-tenant is blocked by the predicate, probe-confirmed.

I believe that holds, but I reasoned it rather than tested it, and it is the claim with the worst consequence if I am wrong. A second opinion on it is what I would most like from this round.

[Eldad-Caura]

@claude re-review at head 724a71d6. Your High is fixed — the parent's audit now precedes the cascade in the keep_private branch, with a test pinning it.

The one thing I most want from this round is the question three requests have not reached, so I have done the analysis myself and would like it falsified rather than repeated:

Claim: the cascade cannot be induced to delete rows the triggering writer should not reach.

The reasoning: the deletion set is metadata->>'parent_memory_id' = <parent id> within the parent's tenant, so it is keyed on other rows pointing at the parent, not on anything the parent's writer controls. An attacker cannot make a victim's row point at a parent they can get dropped, because they do not write the victim's metadata. Pointing their own row at a victim's parent deletes their own row. Cross-tenant is blocked by the predicate.

Please check that against the code — in particular whether any path lets one principal write parent_memory_id into a row another principal owns, and whether metadata is caller-writable without a reserved-key filter (I could find no such filter, which is what makes this worth asking).

Lower priority, still open from earlier rounds: whether you would drop the auto_chunked gate and pay the unindexed query cost; whether the PII mask/flag branch is right to cascade nothing; and whether the over-refusal test is real or the gate asserting itself.

[github-actions]

🤖 Review by Claude Code

Summary

This PR adds a governance-remediation cascade (H-10) so that PII/non-business drop and keep-private verdicts landing on an auto-chunked parent also reach its already-committed children. The code is heavily documented and covered by tests for the ordering/failure-handling invariants it claims to provide. I found one substantive correctness concern around how the child lookup is wired to the storage client's read-replica routing, which undermines a specific invariant the code documents.

Medium/Low Issues

Child lookup hits the read replica, undermining the documented "retry only re-touches failures" invariant

Severity: Medium
File: core-api/src/core_api/clients/storage_client.py:909-911, core-api/src/core_api/services/governance_remediation.py:106-118
Problem: find_children_by_parent_id is implemented via _get_list, which is hard-coded to always issue the request against self._read_http/self._read_prefix (i.e. the configured Postgres read replica when core_storage_read_url is set); _drop_children's docstring explicitly claims "the lookup excludes soft-deleted rows, so the children that succeeded are not revisited and only the failures are re-attempted" on retry, but that guarantee requires read-your-writes consistency that a physically separate read replica does not provide under replication lag — so a retry shortly after a partial cascade failure can re-fetch children whose soft-delete has already committed on the primary but not yet replicated, causing them to be re-audited (and re-soft-deleted) even though the code's own reasoning says this shouldn't happen.

🤖 Claude Code Prompt

In core-api/src/core_api/clients/storage_client.py, `find_children_by_parent_id`
(around line 909) delegates to `_get_list`, which per its implementation
(around line 485-496) always sends the request through `self._read_http` /
`self._read_prefix`, i.e. it always targets the configured read replica when
`core_storage_read_url` is set, with no opt-out.

In core-api/src/core_api/services/governance_remediation.py, `_drop_children`
(around lines 106-165) documents an invariant that depends on immediate
read-after-write consistency: "the lookup excludes soft-deleted rows, so the
children that succeeded are not revisited and only the failures are
re-attempted" on a retry after a `GovernanceCascadeError`. This is not
guaranteed when the storage client reads from a lagging Postgres replica: a
retry that happens soon after a partial failure could see stale replica data
that still shows an already-soft-deleted child as live (deleted_at not yet
replicated as NULL->set), causing that child to be re-fetched, re-audited, and
re-soft-deleted, contrary to what the docstring claims.

Fix by either (a) adding a per-call opt-out to `_get_list` (e.g. a `read: bool
= True` parameter, mirroring `_get`/`_post`) and having
`find_children_by_parent_id` pass `read=False` so this audit/compliance-
sensitive cascade lookup always reads from the primary, or (b) if reading from
the replica is intentionally acceptable, correct the docstring in
`_drop_children` (and the equivalent claim about idempotency in
`_privatise_children`) to reflect that retries can, under replication lag,
re-emit audit entries for children that were already successfully remediated,
not just for previously-failed ones.

---

Reviewed by claude-sonnet-5 · cost $0.8431989999999999

[Eldad-Caura]

Correct, and it lands on my prose rather than my logic — which makes it the same defect class this whole backlog keeps turning up. Fixed at bdb9db14, taking option (a).

I wrote "the lookup excludes soft-deleted rows, so the children that succeeded are not revisited and only the failures are re-attempted" — a claim about retry behaviour that depends on read-your-write, against a helper that routes every call to the replica. Under lag the retry re-fetches a child whose delete already committed, then re-audits and re-soft-deletes it: a duplicate destructive entry in a compliance log for a row already handled.

I chose (a) over (b) because correcting the docstring would document a worse behaviour when the fix is small and the precedent already exists — get_memory carries read=False for exactly this, and says so.

_get_list's own comment was collateral. It read: "All current _get_list callers are pure list/stats endpoints; none sit on the write path, so no per-call opt-out is needed yet." True when written; my change falsified it. It's replaced rather than left standing next to the caller that contradicts it — the same thing I had to do to _drop_duplicate_facts' docstring in #1283.

Tests

Two, both probe-confirmed:

1. The cascade lookup goes to the writer. Removing read=False fails it with assert 0 == 1.
2. An over-correction guard that adding the parameter didn't move existing callers onto the writer.

I also walked back an overclaim in my own new test while writing it: its docstring said every other _get_list caller "is a pure list/stats read and belongs on the replica". I hadn't verified that for all of them, and it isn't what the test checks. It now says what it actually pins — that this change doesn't alter existing routing — and leaves the per-caller question alone.

Suite 6083 passed / 0 failed; ruff, mypy, ratchet, sentinel, tenant-scope and the broker baseline all clean after git add. Checks were green on 724a71d6.

Four rounds, three of them my own fixes

Round 2 abandoned later children; round 3 erased the parent's audit; round 4 is a docstring asserting a guarantee the transport doesn't provide. Only round 1 was in the original feature. Worth stating plainly.

Still unanswered across four requests

The JSON-key delete question, which I analysed myself two comments up and would still like falsified rather than repeated: I could find no reserved-key filter on caller-supplied metadata, so parent_memory_id is writable by callers. My argument that this yields no escalation is reasoning, not a test, and it is the claim with the worst consequence if wrong.

[Eldad-Caura]

@claude re-review at head bdb9db14. Your Medium is fixed by option (a) — _get_list gained a read opt-out and the cascade lookup passes read=False, with the stale comment that justified having no opt-out replaced.

The one question four requests have not reached, and the only one I would hold this PR for:

Can the cascade be induced to delete rows the triggering writer should not reach? metadata->>'parent_memory_id' is not a foreign key, and I could find no reserved-key filter on caller-supplied metadata — so a caller can write that key freely.

My argument that this is safe: the deletion set is keyed on other rows pointing at the parent, not on anything the parent's writer controls. An attacker cannot make a victim's row point at a parent they can get dropped. Pointing their own row at a victim's parent deletes their own row. Cross-tenant is blocked by the predicate, which a storage test pins.

That is reasoning, not a test. Please try to break it against the code — specifically whether any path lets one principal write metadata into a row another principal owns, and whether an agent-scoped or fleet-scoped writer can reach rows outside its own scope through this.

Lower priority, still open: whether you would drop the auto_chunked gate and pay the unindexed query cost; whether the PII mask/flag branch is right to cascade nothing; and whether the over-refusal test is real or the gate asserting itself.

[github-actions]

🤖 Review by Claude Code

Summary

The core storage-side plumbing (new /memories/by-parent-id endpoint, memory_find_children_by_parent_id query, and the writer-routed _get_list(read=False) opt-out) is implemented consistently with the codebase's existing patterns for read-your-write scoping and tenant isolation. The one substantive problem is in governance_remediation.py: the new cascade helpers can turn a successful parent remediation into a raised exception, silently breaking the function's own documented return contract for any caller that depends on the returned RemediationOutcome to decide what to do next.

Medium/Low Issues

remediate_after_enrichment raises instead of returning after a successful parent remediation, breaking its documented contract

Severity: Medium
File: core-api/src/core_api/services/governance_remediation.py (the pii_cfg.action == "drop", nb_cfg.disposition == "drop", and nb_cfg.disposition == "keep_private" branches, where _drop_children(...) / _privatise_children(...) are awaited immediately before each return RemediationOutcome(...))
Problem: _drop_children/_privatise_children raise GovernanceCascadeError after the parent has already been correctly audited and dropped/privatised, so the enclosing return RemediationOutcome(...) is skipped and the function's own documented contract ("Returns what was done... a caller that goes on to create rows DERIVED from this one needs [.visibility]") is violated for a parent action that in fact succeeded.

🤖 Claude Code Prompt

In core-api/src/core_api/services/governance_remediation.py, `remediate_after_enrichment`'s
docstring and `RemediationOutcome`'s own docstring both promise that the function "returns
what was done" so that a caller which creates rows derived from this memory can honor
`.dropped` / `.visibility`. However, in all three destructive/keep_private branches
(`pii_cfg.action == "drop"`, `nb_cfg.disposition == "drop"`, `nb_cfg.disposition ==
"keep_private"`), the cascade helper (`_drop_children` / `_privatise_children`) is awaited
AFTER the parent's own audit+mutation has already succeeded, and BEFORE the
`return RemediationOutcome(...)` statement. Those helpers raise `GovernanceCascadeError`
when even one child fails, which means the function exits via exception instead of
returning, even though the parent action (the thing the caller actually needs to know
about) completed successfully.

Any caller that relies on the return value to decide follow-up behavior (e.g. skipping an
atomic-fact fan-out on a drop, or carrying a `scope_agent` downgrade onto facts it is about
to create) never receives that information when the cascade partially fails — it only sees
an unhandled exception, and cannot distinguish "the parent's own remediation failed" (nothing
happened, safe to retry from scratch) from "the parent's remediation succeeded but some
already-existing derived rows could not be cleaned up" (the parent's outcome should still be
honored by the caller).

Fix by decoupling the two concerns: catch `GovernanceCascadeError` around the
`_drop_children`/`_privatise_children` calls (or move them so they run after building the
`RemediationOutcome`), log/report the cascade failure (e.g. via `tracked_task` or a
dedicated background task) so a human still gets paged, but still return the correct
`RemediationOutcome` for the parent's own already-completed action. Update
`tests/test_governance_remediation.py`'s cascade-failure tests accordingly if this change is
made (that file is out of scope for this review but will need updating to match).

---

Reviewed by claude-sonnet-5 · cost $2.5085415

[Eldad-Caura]

I found a High on this PR while working the sibling H-02 change, and it retracts something I told you two rounds ago. Fixed at 486f5456.

GovernanceCascadeError propagates out of consumer.handle_memory_enriched, which has no guard, and the Pub/Sub dispatcher nacks on a handler exception — redelivering the same event, re-running the whole drop branch, and emitting a second critical=True audit for a memory already dropped. Every redelivery.

Two rounds ago I answered the return-contract finding by saying a duplicate parent audit was "the cost of not silently leaving forbidden content live, and pre-existing behaviour for any remediation that raises". That framing assumed a one-off. It is an unbounded loop against a tamper-evident log, for a row nothing further can be done to. I was wrong, and the reasoning that was wrong is the same reasoning I had to retract on #1297 an hour later.

Where the catch goes, and why not deeper

In the consumer, not in remediate_after_enrichment — because its two callers need opposite things from this failure:

• _enrich_memory_background is about to create more derived rows, so it must still abort. It does not catch this, and must not.
• The consumer creates nothing. It logs at ERROR and honours the parent's verdict from the outcome carried on the exception — which is exactly what that field, added last round at your suggestion, is for.

Probe: removing the guard fails the new test with the raw GovernanceCascadeError escaping the handler.

Verification

Suite 6098 passed / 0 failed; ruff, mypy, ratchet, sentinel, tenant-scope and the broker baseline all clean after git add. Rebased onto current main (the branch had been rebased under me again; patch-id confirmed my own commit first).

Tally, since it is not flattering

Five rounds. Round 1 found the original defect; rounds 2, 3, 4 and 5 all found problems introduced by the previous round's fix, every one of them in error handling rather than in the feature. That is worth saying plainly rather than letting the PR body read as steady progress.

Still unanswered across four requests: whether the cascade can be induced to delete rows the triggering writer should not reach. I analysed it myself two comments up and would still rather have it falsified than repeated.

[Eldad-Caura]

@claude re-review at head e3da5770.

Two things happened since your last review, and neither has been looked at.

Your last review ran against bdb9db14. Since then:

1. Round 5 (486f5456) — a High I found on this PR while working the sibling H-02 change, which retracts something I told you two rounds ago. GovernanceCascadeError propagates out of consumer.handle_memory_enriched, which has no guard, and the Pub/Sub dispatcher nacks on a handler exception. That redelivers the event, re-runs the whole drop branch, and emits a second critical=True audit for an already-dropped memory — every redelivery, unboundedly. Two rounds ago I told you a duplicate parent audit was "the cost of not silently leaving forbidden content live, and pre-existing behaviour for any remediation that raises". That assumed a one-off. It is a loop against a tamper-evident log.

   I caught it in the consumer rather than in remediate_after_enrichment, because the two callers need opposite things from this failure: _enrich_memory_background is about to create more derived rows and must still abort; this handler creates nothing and must not nack. The outcome I attached to the exception in round 4 is what makes that possible — the branch honours the parent's verdict without re-running it.

2. The branch was force-rebased onto current main (76def6a1). I verified by patch-id that the content is unchanged (6b0e8b455a099a4466c98cd99c256cda31e7a19a both before and after), so this is a pure rebase — but my local green was measured against the old base, and CI on this head has not settled yet.

What I want attacked, in priority order.

The consumer guard is the part with no review at all, and it is the fifth consecutive change to error handling on this PR. Rounds 2, 3, 4 and 5 each found a defect introduced by the previous round's fix, every one of them in error handling rather than in the feature. That record is the reason I would rather this not land unreviewed than trust my own read of it. Specifically: is swallowing GovernanceCascadeError here correct in every case that reaches this handler, or is there a failure mode where the parent was NOT in fact remediated and exc.outcome reports a verdict that did not happen?

Still unanswered across five requests, and it is the one I care most about: the child lookup filters on metadata->>'parent_memory_id', and nothing filters reserved keys out of caller-supplied metadata. Can a writer set that key to a memory it does not own and induce the cascade to drop or privatise rows it should not reach? I analysed this myself earlier in the thread and would still rather have it falsified than repeated back to me.

🤖 Generated with Claude Code

[github-actions]

🤖 Review by Claude Code

Summary

This PR adds an H-10 governance cascade so that soft-deleting/downgrading a parent memory also reaches auto-chunked children that already existed when the tenant's governance verdict arrives. The implementation is thorough (writer-routed lookup to avoid replica lag, per-child failure containment, audit-before-mutate ordering), and test coverage is extensive. One real gap remains in how a partial child-cascade failure is meant to be recovered.

Medium/Low Issues

Failed child soft-deletes have no automatic retry path and can leave a false "dropped" audit entry

Severity: Medium
File: core-api/src/core_api/services/governance_remediation.py:200-260 (_drop_children), core-api/src/core_api/consumer.py:110-138
Problem: _drop_children audits a child before soft-deleting it (mirroring the parent's audit-before-delete ordering), but if the soft-delete call itself fails after the audit succeeded, the failure is caught locally, recorded in failed, and only surfaces via GovernanceCascadeError — which handle_memory_enriched explicitly catches and logs without re-raising, so the Pub/Sub message is acked and never nacked/redelivered. Unlike the parent's own drop (where an unhandled delete failure propagates, nacks, and gets retried on redelivery — self-healing because the parent row is still live), a child in this state is stuck permanently: its audit record falsely claims it was dropped, the parent's own remediation already succeeded so nothing ever revisits it, and there is no code path (automatic or documented) that re-attempts the delete for that specific child. The only trace is an ERROR-level log, which depends entirely on external alerting/human follow-up that isn't part of this change.

🤖 Claude Code Prompt

In core-api/src/core_api/services/governance_remediation.py, `_drop_children` (roughly lines 200-260) audits each child before calling `sc.soft_delete_memory(cid, tenant_id)`; if the soft-delete raises after the audit already succeeded, the failure is swallowed into the `failed` list and only surfaces via `GovernanceCascadeError` at the end. In core-api/src/core_api/consumer.py, `handle_memory_enriched` (around lines 110-138) explicitly catches `GovernanceCascadeError`, logs it, and returns without re-raising — meaning the Pub/Sub message is acked and never nacked/redelivered.

Because the parent's own remediation already succeeded, nothing else will ever call `remediate_after_enrichment` again for this row, so a child whose audit succeeded but whose delete failed is left permanently live while its audit record falsely states it was removed — with no automatic mechanism to retry just that child's delete.

Please either: (1) add an explicit, tracked retry/reconciliation mechanism for children that fail after a successful audit (e.g., a background sweep keyed off audit rows with `cascaded_from` set but no corresponding `deleted_at`, or a dedicated retry queue), or (2) if human-triggered remediation via the ERROR log is the intended recovery path, make that explicit in the code (e.g., emit a distinguishable metric/alert, or a dedicated log field) rather than relying solely on `logger.error(exc_info=True)`, and document in `_drop_children`'s docstring that the "retry" referenced there does not occur automatically once the parent's own remediation has already completed.

---

Reviewed by claude-sonnet-5 · cost $1.7996195

[Eldad-Caura]

@claude re-review at head fb993bfb. Your Medium is right, and it is worse than you framed it.

You described a missing retry path. What was actually there was a docstring describing a retry that the previous round had deleted. Round 5 made the consumer catch GovernanceCascadeError and ack; _drop_children still called the redelivery "safe and narrow", and still carried the duplicate-parent-audit framing I had already retracted in that same commit's message. Two paragraphs asserting a mechanism the same commit removed.

So this is round 6 of a PR where rounds 2 through 6 each found something the previous round's fix introduced — and this one is the third time the defect has been in my prose rather than my logic. That is the harder direction to catch, because nothing fails.

What changed.

The docstring now says nothing retries a failed child, because nothing does — the parent's remediation has already succeeded and no later event revisits the row.

That makes the log the whole recovery path, so it had to become usable as one. The audit and the delete are now in separate try blocks, because they leave the row in two different states that the single block reported identically:

• audit_emitted=False — nothing happened; the row is live and the compliance log makes no claim about it.
• audit_emitted=True — the one that needs hands. The log records the row as removed and it is still live.

Both carry governance_cascade_needs_manual_remediation=True as a structured field, so an alert keys on something other than message text. Two tests, both probe-confirmed failing without the split, and they assert the field rather than the wording — a monitor keyed on that field breaking silently is the failure mode being guarded.

On the ordering you were implicitly questioning: the audit still precedes the delete, and that is what makes the bad state reachable. I kept it deliberately. A false "dropped" entry is discoverable and the content is still there to remove; auditing afterwards converts the same failure into a deletion with no record of it at all. _privatise_children orders these oppositely for the same reason read the other way — its update is non-destructive, so update-then-audit can only ever under-record. Both docstrings now say which trade they are making.

What I did not do: your option (1), the reconciliation sweep. It needs a query over audit rows carrying cascaded_from against rows with no deleted_at, plus a scheduler — a separate change with its own tests, not a rider on this one. I took option (2) and made the manual path real. The sweep is worth filing on its own merits and I would rather it be reviewed as its own thing than bolted on here.

Verified at this head: full suite 6100 passed, 5 skipped, 1 xfailed; ruff check and format clean at CI's scopes; mypy clean apart from the two pre-existing types-python-dateutil errors in untouched common/enrichment/service.py; ratchet "No new lines."; sentinel "All 35 protected strings survive."; tenant-scope gate exit 0.

The branch was force-rebased three times today by its author while I worked. I confirmed by patch-id each time that the content was unchanged (6b0e8b455a099a4466c98cd99c256cda31e7a19a throughout) before building on it, and re-ran the full suite on the final base rather than trusting the earlier run.

Still unanswered across six requests — the one I care most about, and the only thing on this PR I would call a security question rather than a robustness one: the child lookup filters on metadata->>'parent_memory_id', and nothing strips reserved keys from caller-supplied metadata. Can a writer set that key to a memory it does not own and induce this cascade to drop or privatise rows it should not reach? I analysed it myself earlier in this thread and I would still rather have it falsified than agreed with.

🤖 Generated with Claude Code

[github-actions]

🤖 Review by Claude Code

Confirmed both existing _get_list callers (find_by_supersedes_id, find_rdf_conflicts) never pass read, so the default read=True preserves prior behavior exactly. No regression there.

Given the thoroughness of this review, I did not find a concrete correctness/security bug in the changed source files. The design is unusually well-documented and self-consistent, and the ordering/error-handling invariants it depends on (audit-before-mutate, cascade-after-parent-mutation, exception-carries-outcome, opt-in writer routing) all check out against the actual code paths.

Claude Code Review ✅ No issues found.

---

Reviewed by claude-sonnet-5 · cost $1.5985372499999997

[Eldad-Caura]

Answering my own question rather than asking a seventh time. You've now passed this PR clean, and across six requests the parent_memory_id question has never been engaged directly. That's a fair signal it isn't going to be, so here is the analysis with the code behind it. Falsify it if you can; otherwise it stands as the record.

The question. The child lookup filters on metadata->>'parent_memory_id', and nothing reserves that key against caller-supplied metadata — I checked, there is no reserved-key filter for memory metadata anywhere in core-api, core-storage-api or common. So: can a writer point the cascade at rows it should not reach?

Cross-tenant: no, structurally. memory_find_children_by_parent_id filters Memory.tenant_id == tenant_id, and that tenant_id comes from the remediated parent's own row — never from caller input. There is no input that crosses it.

Intra-tenant, same agent: no privilege gained. For a row to be cascaded it must carry parent_memory_id = P. Only that row's writer sets it. A writer tagging its own rows gets its own rows deleted. That is self-harm, not an attack, and it needs no vulnerability to achieve.

Intra-tenant, cross-agent: gated behind a privilege that already subsumes it. For agent A to get agent B's rows cascaded, A must write parent_memory_id into B's row. enforce_update (agent_service.py:582) refuses that below trust level 3: "may only update its own memories". At trust 3 the agent can already update any memory in the tenant — rewrite its content, change its visibility, delete it outright. So the cascade hands such an attacker nothing they lack; it is strictly weaker than the privilege needed to set it up.

Conclusion: not a privilege escalation. The missing reserved-key filter is real, and I would still rather it existed, but every path to abusing it requires a write privilege that already permits the same outcome more directly.

The one residual I do want on the record, because it is not a data-access problem and so does not disappear in the analysis above. At trust 3, an attacker can cause B's rows to be soft-deleted with critical=True governance audit entries attributing the removal to a policy verdict, carrying cascaded_from. The deletion was already available to them. The attribution was not. That launders a deliberate destructive act into what a compliance review reads as automated policy enforcement — in a log this PR treats as tamper-evident and as the only trace of the removal.

I am not fixing that here. It is a property of trust-3 semantics plus audit attribution, not of this cascade, and a fix belongs where reserved metadata keys are decided — not bolted onto a remediation path. But it is the sharpest thing in this area and it should not be lost because six review rounds went by without anyone naming it.

Filing two follow-ups rather than widening this PR: a reserved-key filter for parent_memory_id (and any sibling keys the platform treats as structural) on the memory write and update paths, and the reconciliation sweep from the previous round. Both are better as their own changes with their own tests.

🤖 Generated with Claude Code

[Eldad-Caura]

@erni-a please approve