feat: add source_type provenance flag to SecurityEvent#24
Conversation
Added SourceType enum and source_type field. Closes OWASP#13
vgudur-dev
left a comment
There was a problem hiding this comment.
Thanks for the contribution @hesam-oxe — structured provenance on SecurityEvent is the right direction. A few things to address before this is merge-ready:
Blocking
- Rebase onto current
main. This was opened against22bbde0;66faa7f(PR #25 — the classification system) has landed since and modifies the same_emitsurface. GitHub already showsmergeable_state: dirty. - Reconcile with the classification work. After #25,
MemoryGuard.write()already acceptscls: MemoryClassfor entry provenance and a free-textsource: strfor the actor. Addingsource_type: SourceTypeas a third parallel concept makes the API confusing. See the inline thread onevents.py:26andguard.py:116— pick one shape and refactor. - No tests. New public enum, new keyword-only parameter on
write(), new field in the SIEM-boundSecurityEvent.to_dict()— none of it is exercised bytests/. At minimum:write(..., source_type=SourceType.TOOL_OUTPUT)produces an event whoseto_dict()["source_type"] == "tool_output".- Omitting
source_typedefaults to"unknown"(back-compat). - The detector-triggered paths (block / quarantine / redact / allow-with-findings) preserve the caller's
source_type.
- Semantic correctness on read/delete/rollback — see inline at
guard.py:203. Hard-codingSourceType.SYSTEMon every non-write emit conflates "event provenance" with "event-emitter identity."
Non-blocking nits
- Missing trailing newlines on both files (
\ No newline at end of filein the diff). def write(...)exceeds the project's 100-char ruff limit.SourceTypeshould be re-exported fromagent_memory_guard.__init__alongsideAction/Severity.
Good problem to solve, but worth aligning with MemoryClass / Source before we expand the surface.
Generated by Claude Code
| QUARANTINE = "quarantine" | ||
|
|
||
|
|
||
| class SourceType(str, Enum): |
There was a problem hiding this comment.
Heads up — this overlaps with concepts that landed in #25 (the classification system merged just after this PR was opened). After the merge, MemoryGuard.write() already takes a cls: MemoryClass argument that encodes provenance for the entry itself (TOOL_OBSERVATION, RETRIEVED_FACT, VERIFIED_PREFERENCE, POLICY, …), and there's a separate free-text source: str = "agent" kwarg that's stored in event metadata.
So once you rebase you'll have three parallel provenance handles:
| field | type | purpose |
|---|---|---|
source: str |
free text | event metadata only |
source_type: SourceType |
this PR | event metadata only |
cls: MemoryClass |
enum, attached to the entry | provenance of the stored value |
Worth reconciling before merging — otherwise the public API has two near-synonyms (source / source_type) plus a third orthogonal concept (cls). Two reasonable shapes:
- Replace
source: strwith a structuredSource(role=SourceRole.TOOL, principal="search_tool_v1")and dropsource_type— the role is the provenance. - Keep
source_type, renamesource→source_principalfor clarity, and document the relationship toMemoryClassin the docstring (event-emitter vs entry-content).
Either is fine; the current shape is the only one that's clearly wrong.
Generated by Claude Code
| key: str | ||
| message: str | ||
| operation: str = "write" | ||
| source_type: SourceType = SourceType.UNKNOWN |
There was a problem hiding this comment.
Defaulting to UNKNOWN is pragmatic for back-compat, but for a security-focused field it lets callers silently neglect provenance forever. Two follow-ups worth filing (not blocking this PR):
- a one-shot debug-level log the first time the guard sees an
UNKNOWNwrite per process, so operators notice unset provenance; - a strict-mode policy flag (
Policy(require_source_type=True)) that rejectsUNKNOWNwrites when enabled.
Generated by Claude Code
| } | ||
|
|
||
|
|
||
| __all__ = ["Action", "SecurityEvent", "Severity", "SourceType"] No newline at end of file |
There was a problem hiding this comment.
Two nits on this line:
- Missing trailing newline (
\ No newline at end of filein the diff). Same forguard.py. Most linters and POSIX tools assume a final newline; please add one. - Once
SourceTypeis exported here, also re-export it from the package root insrc/agent_memory_guard/__init__.pyso consumers can writefrom agent_memory_guard import SourceTyperather than importing from a private path. Same pattern as the existingAction/Severityexports.
Generated by Claude Code
| return drifted | ||
|
|
||
| def write(self, key: str, value: Any, *, source: str = "agent") -> Action: | ||
| def write(self, key: str, value: Any, *, source: str = "agent", source_type: SourceType = SourceType.UNKNOWN) -> Action: |
There was a problem hiding this comment.
Two issues on this signature:
-
Line length. The project sets
line-length = 100inpyproject.toml([tool.ruff]); this line is well over that. Please wrap:def write( self, key: str, value: Any, *, source: str = "agent", source_type: SourceType = SourceType.UNKNOWN, ) -> Action:
-
Reconcile with
cls=(added in feat(classification): provenance-based memory classes and promotion rules #25). After the rebase,write()will already acceptcls: MemoryClass | str | None = Noneplus the existingsource: str = "agent". Addingsource_type: SourceTypehere makes three parameters that all describe "where this came from." See the larger discussion onevents.py:26. Decision needed before this is merge-ready.
Generated by Claude Code
| key=key, | ||
| message="Integrity verification failed on read", | ||
| metadata={"expected": exc.expected, "actual": exc.actual}, | ||
| source_type=SourceType.SYSTEM, |
There was a problem hiding this comment.
Semantic issue (applies to every source_type=SourceType.SYSTEM you set on a non-write code path — integrity failure on read, read block, read redact, read allow-with-findings, delete block, rollback).
SourceType is documented as "Provenance of a memory write — where the write came from." A read isn't a write, and an integrity failure on a read isn't sourced from "the system" — the offending data was sourced from whoever previously wrote it (or from out-of-band tampering, which is the whole reason we're flagging it). Forcing SYSTEM on these paths conflates two ideas:
- where the event-relevant data originated (what the field is documented to mean), versus
- who emitted this event (the guard itself, always).
Three ways to fix, pick one:
- Leave reads/deletes/rollbacks as
UNKNOWN— the field genuinely doesn't apply to those operations. - For integrity failures specifically, attach the recorded source_type of the prior write (you'd need to store it alongside the SHA-256 baseline in
IntegrityRegistry— small but useful change). For the rest, default toUNKNOWN. - Rename the field to something like
event_sourceand update the docstring to cover both semantics.
Whichever you pick, please also remove the hard-coded SourceType.SYSTEM from delete() and rollback() — neither is "system-sourced" in any useful sense.
Generated by Claude Code
|
@lefarcen Rebased on latest main — conflicts should be resolved. |
|
The The current integrity model — SHA-256 baselines on immutable keys — closes the in-band tampering surface. It doesn't close the operator-trust gap: the baselines and SecurityEvents are produced and held by the same infrastructure. A regulator or auditor who needs to verify a memory write independently can't do it without access to the operator's system.
The receipt is independently verifiable — any third party confirms it without operator access. Integration shape for GuardedMemory (from the LlamaIndex thread where Gogani / Nobulex outlined this for ASI06): action_ref = sha256(jcs({
"agent_id": agent_id,
"action_type": "memory_write",
"scope": sha256(jcs({"content": message.content})),
"timestamp_ms": int(time.time() * 1000)
}))
memory_event.metadata["action_ref"] = action_refThe Spec + conformance fixtures (5 language implementations, byte-verified): Happy to help wire the integration if useful. |
…newline, re-export SourceType
|
@vgudur-dev Really appreciate the thorough review — the overlap analysis Addressed the actionable items: On the MemoryClass reconciliation — I agree there's overlap that should The strict-mode policy flag idea is also worth a follow-up. 🚀 |
|
@hesam-oxe — heads up, this now has merge conflicts after we merged PR #39 (docstrings). Could you rebase against Happy to help if you run into issues with the rebase. This is a high-priority feature for v0.4.0. |
Added source_type provenance flag to SecurityEvent and MemoryGuard.
Changes
events.py— AddedSourceTypeenum with USER_INPUT, TOOL_OUTPUT,MODEL_INFERENCE, SYSTEM, UNKNOWN
events.py— Addedsource_typefield toSecurityEventguard.py— Addedsource_typeparameter toMemoryGuard.write()guard.py— All_emitcalls now passsource_typeSource Types
USER_INPUT— direct from userTOOL_OUTPUT— tool returned thisMODEL_INFERENCE— model derived from contextSYSTEM— config / startup / internal opsUNKNOWN— default for back-compatBackward Compatibility
Defaults to
UNKNOWN— existing integrations continue to work.Closes #13