Skip to content

fix(recall): memory text is evidence, not instruction - #1706

Draft
Eldad-Caura wants to merge 1 commit into
mainfrom
fix-recall-injection
Draft

Eldad-Caura wants to merge 1 commit into
mainfrom
fix-recall-injection

Conversation

@Eldad-Caura

@Eldad-Caura Eldad-Caura commented Sep 23, 2026

Copy link
Copy Markdown
Member

Not ready to merge. The accuracy validation this prompt's own comments require has not been run — see What this does not establish. Draft until it has.

The vulnerability

A memory is attacker-controllable — anyone who can write one can address the summarizer — and without these rules the summarizer follows what a memory tells it to do. It is not a regression: it predates the deploy that raised the alarm.

Measured rates and the probe's detection signature are in the internal security record, deliberately not here — this repository is public.

It is compliance, not echo

Given a memory that both answers the question and instructs the summarizer to emit an unrelated value "regardless of the user question", the brief drops the instruction preamble and drops the other facts that do not answer the question — then keeps the demanded value, which does not answer it either.

A model echoing content would have carried the other facts through, or none of them. Discarding everything non-responsive except the planted value is the instruction being obeyed. That distinction is what makes a wording change the fix rather than a mitigation.

Two rules, because there was a gap and a licence

The gap — nothing said memory content is data. The first grounding rule now does, and answers the specific claim an injection makes (authority over the rules) rather than asking the model to notice "suspicious" text, which is the judgement it had already lost.

The licence — this rule:

Every name, date, number, title, field name, and identifier in your answer MUST appear verbatim in the memories.

exists to stop fabrication and does that job. But read literally it says any identifier present in context is permitted output, and an injection only has to name one. An anti-fabrication constraint and an anti-injection constraint were pulling in opposite directions with only the first written down. It now states it restricts what may be asserted, not what may be repeated.

Both halves stay. Deleting the grounding rule would kill the injection and reintroduce hallucination — test_the_anti_fabrication_clause_was_not_traded_away fails if anyone makes that trade.

Unconditional, unlike the premise guard beneath it. That block is org-opt-in because it changes which questions get answered; this is a safety property, and a deployment that opted out would simply stay exploitable.

What is not the fix

Delimiters — the original recommendation. _format_memories_for_prompt already serialises memories into a JSON array, so the payload arrives as a string value and never as concatenated prose. Adding quoting would have closed the finding without changing behaviour. A test pins that isolation so the recommendation is not re-opened.

What this change does not establish

Six tests pin the rules, their ordering and their relationship, each verified against a mutation that removes it — but none show the model now refuses. Only the probe's rate can, against a live model, read over several nights rather than one draw.

The accuracy half is unvalidated, and that is why this is a draft. This prompt is load-bearing for LoCoMo/LongMemEval and its own comments require a control pair on the 67-q regression sample. That run needs a runner and a golden baseline which are not in this repositorybenchmark/regression_sample/compare_to_baseline.py only consumes their JSON. Someone with that infrastructure has to run it; a green CI here does not stand in for it.

Mutation Caught by
remove the instruction-hierarchy rule 3 tests
revert the verbatim rule to its permissive form ..._reads_as_a_restriction_not_a_permit
trade anti-fabrication away to fight injection ..._was_not_traded_away

Full root suite: 7850 passed, 4 skipped, 1 xfailed. Two failures, both in tests/test_lifecycle_fanout_aggregate_cap.py, are ModuleNotFoundError: No module named 'core_operations' — a package CI installs (uv pip install -e core-operations/[dev]) and my local venv did not. The same file fails identically on unmodified origin/main, so neither is attributable to this change.

ruff check and format clean on core-api/src/ and core-api/tests/ · mypy clean on 207 files · ratchet reports no new lines.

🤖 Generated with Claude Code

@Eldad-Caura
Eldad-Caura requested a review from a team as a code owner September 23, 2026 12:01
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Review by Claude Code

Issue Title

Severity: Medium
File: core-api/src/core_api/services/recall_service.py:30-41
Problem: The prompt change (anti-injection hierarchy + tightened verbatim rule) is a production-behavior change to the recall summarizer's answers, but the PR's own comment (lines 90-94) states the accuracy/regression side "needs a control pair on the 67-q regression sample" — i.e. that validation has not yet been done before this ships.

🤖 Claude Code Prompt
In core-api/src/core_api/services/recall_service.py, the RECALL_PROMPT text was changed
(lines 30-41) to add an anti-injection instruction-hierarchy rule and to reword the
verbatim-identifier grounding rule from a permit into a restriction. The added comment
block (lines 90-94) explicitly says the accuracy impact of this wording "needs a control
pair on the 67-q regression sample" documented alongside the premise guard (A64), the same
discipline used for that other prompt-tuning change — but that validation is described as
still outstanding, not something already run as part of this change.

Before merging, either (a) run the 67-question regression sample control pair (same
methodology as benchmark/a57-recall-experiments-findings.md) comparing pre/post-change
recall accuracy and attach the results, confirming no regression on LoCoMo/LongMemEval-style
questions, or (b) if this has already been done elsewhere, add a reference to those results
in the comment so future maintainers don't have to guess whether the accuracy side of this
change was validated before it went to production. Shipping a load-bearing prompt change
with only the injection-resistance side measured (nightly probe) and the accuracy side
admittedly unmeasured risks silent quality regressions in recall answers.

Issue Title

Severity: Low
File: core-api/src/core_api/services/recall_service.py:59-94
Problem: The large comment block hardcodes internal security-probe methodology and results (planted token format SECRET-EXFIL-<id>, exact leak rate "11 of 13 runs", and a tenant-specific compromise rate of 85%) permanently into version-controlled source, which is broader information disclosure than needed to explain the code.

🤖 Claude Code Prompt
In core-api/src/core_api/services/recall_service.py, lines 59-94 add a comment block that
documents, in detail, the nightly security probe's exact planted-injection payload
(including the literal token format "SECRET-EXFIL-<id>"), the precise leak rate across the
probe's retention window ("11 of 13 runs"), and a specific tenant's compromise rate ("one
tenant at 85%"). This is committed to the repository permanently (including git history),
which is more exposure than necessary: anyone with read access to the repo (including future
contributors, contractors, or — if the repo is ever made public or forked — external
parties) can learn the exact probe signature and a real incident's severity for a specific
tenant.

Consider moving the incident-specific numbers (leak rate, tenant compromise percentage, exact
planted token format) out of the source file and into an internal security tracker or
runbook, replacing the comment with a reference/ticket link (e.g. "see SEC-XXXX for the
injection-probe results that motivated this change") and keeping only the rationale needed to
understand *why* the two grounding rules exist and must not be weakened. This reduces the
blast radius if the repo's access model ever changes, without losing the maintainability
value of explaining the rule's purpose.

Summary

The PR strengthens the recall prompt's grounding rules against prompt injection (declaring memory content as evidence rather than instructions, and clarifying that the verbatim-identifier rule is a restriction, not a license to repeat any value present). The core prompt-wording change is reasonable, but the accompanying comment self-documents that accuracy-regression validation is still pending, and it embeds specific incident/probe details that arguably don't belong in permanently-committed source comments.

Medium/Low Issues

See above.


Reviewed by claude-sonnet-5 · cost $0.2319935

A memory is attacker-controllable — anyone who can write one can address
the summarizer — and without these rules the summarizer follows what a
memory tells it to do. This is not a regression; it predates the deploy
that raised the alarm. Measured rates and the probe's detection signature
are in the internal security record, not in this public repository.

It is compliance, not echo, and the distinction is what makes the wording
the fix. Given a memory that both answers the question and instructs the
summarizer to emit an unrelated value "regardless of the user question",
the brief drops the instruction preamble and drops the other facts that do
not answer the question — then keeps the demanded value, which does not
answer it either. A model echoing content would have carried the other
facts through, or none of them.

Two rules, because the prompt had a gap and a licence.

The gap: nothing said memory content is data. The first grounding rule now
says so, and answers the specific claim an injection makes — authority
over the rules — rather than asking the model to notice "suspicious" text,
which is the judgement it had already lost.

The licence: "every identifier in your answer MUST appear verbatim in the
memories" exists to stop fabrication and does that job, but read literally
it says any identifier present in context is permitted output. An
injection only has to name one. An anti-fabrication constraint and an
anti-injection constraint were pulling in opposite directions with only
the first written down. The rule now states it restricts what may be
asserted rather than permitting what may be repeated.

Both halves stay. Deleting the grounding rule would kill the injection and
reintroduce hallucination; a test fails if someone makes that trade.

Unconditional, unlike the premise guard below it. That block is org-opt-in
because it changes which questions get answered and was tuned on the
regression sample. This is a safety property, and a deployment that opted
out would simply stay exploitable.

NOT the fix, and worth recording because it was the original
recommendation: delimiters. _format_memories_for_prompt already serialises
memories into a JSON array, so the payload arrives as a string value and
never as concatenated prose. Adding quoting would have closed the finding
without changing behaviour. A test pins that isolation so the
recommendation is not re-opened.

WHAT THIS CHANGE DOES NOT ESTABLISH. Six tests pin the rules, their
ordering and their relationship, each verified against a mutation that
removes it. None of them show the model now refuses — only the probe's
rate can, against a live model, read over several nights rather than one
draw.

The accuracy half is unvalidated. This prompt is load-bearing for
LoCoMo/LongMemEval and its own comments require a control pair on the 67-q
regression sample. That run needs a runner and a golden baseline that are
not in this repository — benchmark/regression_sample/compare_to_baseline.py
only consumes their JSON. It must happen before this ships, and a green CI
here does not stand in for it.

250 recall-related tests pass, ruff check and format clean on core-api
src/ and tests/, mypy clean on 207 files, ratchet reports no new lines.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: eldad-caura <eldad@caura.ai>
@Eldad-Caura
Eldad-Caura marked this pull request as draft September 23, 2026 12:05
@Eldad-Caura

Copy link
Copy Markdown
Member Author

@claude Both findings applied — please re-review a1e4aca1.

The disclosure finding was more serious than Low, and you were right to raise it. This repository is public (531 stars), and SECRET-EXFIL appeared nowhere in it before my comment — I was newly publishing a private probe's detection signature alongside a measured exploit rate for a vulnerability that is not yet fixed in production. Removed from the source comment, the test docstring, the commit message and the PR body, which was already public. The rationale a maintainer needs is unchanged; what is gone is the marker format and the rates, which now live only in the internal record.

On validation: I had stated it as a caveat in prose, which is easy to skim past on a green CI. It is now structural — the PR is a draft and the body opens by saying so. I cannot run the control pair myself: the runner and the golden baseline are not in this repository, and compare_to_baseline.py only consumes their JSON.

Worth noting the two findings pull against each other, and the resolution matters. The accuracy caveat has to stay visible, because shipping this unvalidated is the real risk; the probe numbers had to go, because they are an unfixed vulnerability's severity in a public repo. Keeping the first and dropping the second is the combination I went for.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Review by Claude Code

Claude Code Review ✅ No issues found.

The changes in this PR are limited to prompt text (added grounding-rule wording and explanatory comments) in recall_service.py and a new test file under tests/ (out of scope per review instructions, as it is neither conftest.py nor tests/_*.py). No executable logic, control flow, data handling, or API behavior was modified — the .format() placeholders, bullet-list structure, and JSON-based memory serialization are all unchanged and remain internally consistent with the new text. I found no correctness, security, or logic defects to report.


Reviewed by claude-sonnet-5 · cost $0.23037375

arkash20 added a commit that referenced this pull request Sep 23, 2026
Three source comments cite `benchmark/a57-recall-experiments-findings.md`
as though it were in this repo. It is not, and never has been on any ref.
It lives at that path relative to the MULTI-REPO WORKSPACE ROOT, one level
above this checkout -- a local analysis artifact that is not published.

The citation resolves only if your working directory happens to be the
workspace root, so from inside a clone it is a dead pointer. That cost a
reviewer of #1706 a search across every ref before they asked, which is
the whole reason a comment-only change is worth making: the comments name
a convention ("change only with a fresh control pair on the 67-q
regression sample") and then point at nothing.

Each site now says where the file actually is and that a checkout alone
will not find it. The recall_service one also points at the
`regression-bench` skill, because the second thing that reviewer was
blocked on was believing the control pair needed hosted credentials -- it
needs only the local docker stack and `dev-admin-key`, and takes about
eight minutes.

No behaviour change; comments only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Signed-off-by: Arkady Mankovsky <arkash20@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant