feat(v0.4.0): tenant context + structured audit events + idempotency (P2/P3)#83
Merged
Conversation
…store (v0.4.0 P2/P3) P2 (US2 - tenant context + structured audit events): - Add tenancy/ package: TenantContext, DEFAULT_TENANT, and a default resolve_tenant() resolver (X-Attestix-Tenant header, defaults to 'default'). Repository-level tenant scoping landed in P1; this adds the context seam. - Add audit/ package: AuditEvent dataclass + per-tenant hash chain reusing the provenance prev_hash/chain_hash/genesis pattern, and AuditEventEmitter with a default local (Repository-backed) sink + injectable external sink (FR-017). Honor FR-028: document that actor/target_id may be PII and the file sink is the plaintext self-host/dev default; production/multi-tenant must use an encrypted-at-rest Repository. P3 (US3 - idempotency keys): - Add idempotency/ package: IdempotencyStore ABC + Repository-backed default with 24h TTL and reclaim, the surface-agnostic run_idempotent() helper, and an opt-in IdempotencyMiddleware. Honor FR-029: store only a minimal representation (status + resource id + response hash), never raw key material or signed bodies. - Middleware is provided but NOT auto-mounted; a documented TODO marks the seam in api/main.py so the default app keeps exact v0.3.0 request handling (no key -> no bookkeeping, FR-022). Supporting: - config.py: add AUDIT_FILE / IDEMPOTENCY_FILE; FileRepository: add audit + idempotency collections. conftest patches the new paths into the temp dir. - pyproject: register tenancy/audit/idempotency (flat + attestix.* mirror). - Resolve the four T046 [NEEDS CLARIFICATION] items in spec.md / data-model.md (file idempotency concurrency = first-writer-wins via filelock; idempotency scope = surface-agnostic, REST-primary; no-op update emits one event; default REST tenant resolution = resolve_tenant header/default). Tests (all additive, parametrized over FileRepository + MemoryRepository): - tests/unit/test_audit_events.py: shape, one-event-per-change, chain verify + tamper detection, per-tenant chains, external sink. - tests/integration/test_tenant_isolation.py: cross-tenant isolation, default parity, legacy (no tenant_id) record reads as 'default', resolver. - tests/integration/test_idempotency.py: dedupe within TTL, conflict on payload mismatch, TTL expiry, no-key passthrough, per-tenant scoping, minimal storage, reclaim. Suite: 411 -> 454 passed, 1 skipped, 0 regressions; 91 RFC conformance benchmarks unaffected. Deferred to a P1 follow-up: per-service tenant/audit threading (T033/T034) and mounting the idempotency middleware (T040).
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Tenant Context & Resolution tenancy/context.py, tenancy/__init__.py, attestix/tenancy/__init__.py, tests/integration/test_tenant_isolation.py |
TenantContext dataclass with require_tenant() validation; case-insensitive X-Attestix-Tenant header resolution with fallback to "default"; per-tenant repository scoping with backward compatibility for legacy records. |
Audit Events Schema & Hash-Chain Emission audit/events.py, audit/emitter.py, audit/__init__.py, attestix/audit/__init__.py, tests/unit/test_audit_events.py |
Frozen AuditEvent dataclass with SHA-256 chain_hash computed from JCS-canonical JSON; compute_change_digest() hashes state transitions without exposing sensitive data; AuditEventEmitter emits exactly one event per committed operation, chains per tenant via in-process lock, persists to repository, and forwards to optional external sink. |
Idempotency Store & Deduplication Logic idempotency/store.py, tests/integration/test_idempotency.py |
IdempotencyStore ABC with TTL enforcement (24h); RepositoryIdempotencyStore implementation with opportunistic expiry deletion; run_idempotent() orchestrates reserve → execute → record, detecting fingerprint conflicts and returning minimal stored responses (status, resource_id, hash only). |
REST Middleware & Optional Mounting idempotency/middleware.py, idempotency/__init__.py, attestix/idempotency/__init__.py, api/main.py |
Opt-in IdempotencyMiddleware for Starlette with lazy imports; intercepts write methods and Idempotency-Key header; returns 409 on conflict/in-progress or 200 with minimal response on replay; includes TODO seam for future mounting. |
Storage, Config & Distribution config.py, storage/file_repository.py, tests/conftest.py, pyproject.toml |
Adds AUDIT_FILE and IDEMPOTENCY_FILE JSON paths; registers both in FileRepository._COLLECTIONS with default shapes; extends test fixture for isolation; includes both flat and namespace-mirrored Python packages. |
Documentation & Specifications docs/extensibility-v0.4.0.md, specs/001-v0.4.0-extensibility/… |
Describes v0.4.0 as five opt-in seams preserving v0.3.0 defaults; clarifies tenant scoping, audit determinism (one event per committed mutation), idempotency first-writer-wins via filelock, minimal stored response design, and mandatory encrypted-at-rest for production audit storage. Updates task tracking for completed implementations. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
- feat: structured AuditEvent emission on state changes (cloud prereq, v0.4.0) #70: Implements the same structured audit-event datatype (
AuditEventandAuditEventEmitterwith SHA-256 hash-chaining) and event emission seam, directly related at the code level.
Possibly related PRs
- VibeTensor/attestix#82: Extends the existing
storage/file_repository.pyby adding new_COLLECTIONSentries forauditandidempotency, building directly on the pluggableFileRepositorystorage seam.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 38.57% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately captures the main changes: tenant context (P2), structured audit events (P2), and idempotency (P3) as core features of the v0.4.0 extensibility layer. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feature/v0.4.0-p2-p3
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import pytest | ||
|
|
||
| from audit import AuditEvent, AuditEventEmitter, GENESIS_HASH, verify_chain | ||
| from audit.emitter import AUDIT_COLLECTION |
Deploying attestix with
|
| Latest commit: |
5b77430
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://138af36d.attestix.pages.dev |
| Branch Preview URL: | https://feature-v0-4-0-p2-p3.attestix.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_idempotency.py (1)
165-189: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a regression test for
execute()exceptions.Please add a case asserting that a failing execution does not leave the key permanently blocked as
in_progressfor the TTL window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_idempotency.py` around lines 165 - 189, Add a regression test in tests/integration/test_idempotency.py that simulates a failing execute() call and asserts the idempotency key is not left as "in_progress" for the full TTL: create a controlled idempotency execution (using the existing store_and_repo fixture or store/ repo helpers), call the code path that invokes execute() in a way that raises an exception, catch/expect that exception in the test, then call store.reclaim_expired(tenant_id="default") (or advance time / adjust created_at to simulate TTL expiry) and assert the stale key is removed (e.g., removed == 1 and the repo.list(IDEMPOTENCY_COLLECTION, id_field="key") does not contain the key). Reference execute(), store.reclaim_expired, request_fingerprint and TTL to locate the related logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@audit/emitter.py`:
- Around line 51-56: The instance-level self._lock in AuditEventEmitter does not
prevent two separate emitter instances from racing; replace it with a shared
class-level lock registry keyed by tenant (or repository) so all emitters for
the same tenant serialize chain computation. Concretely, add a class attribute
like AuditEventEmitter._tenant_locks = defaultdict(threading.Lock) (or similar)
and update all places that currently use self._lock (the initialization and the
critical sections around computing prev_hash and appending the event — the code
around lines where self._lock is set and the block at lines ~89-100) to acquire
the tenant-specific lock (e.g., with AuditEventEmitter._tenant_locks[tenant_id])
instead of the instance lock. Ensure locks are released appropriately and avoid
leaking locks for one-off tenants if needed.
- Around line 109-111: Wrap the external sink call (self._sink(event)) in a
try/except so failures from the external sink do not propagate after the local
state and audit write have committed: catch Exception around self._sink(event),
log the error with contextual info (including event id/payload) using the
emitter's logger (e.g., self._logger or the module logger), and do not re-raise;
instead either schedule a retry or enqueue the event to your outbox/retry
mechanism if one exists, but always return event to callers regardless of sink
outcome.
In `@audit/events.py`:
- Around line 194-199: In verify_chain(), the loop that converts each ev into an
AuditEvent using AuditEvent(**{k: ev[k] for k in _AUDIT_FIELDS}) can raise
KeyError/TypeError for malformed/corrupted dicts; change this so any exception
during that conversion (or if ev is not the expected type) causes verify_chain()
to return False instead of propagating the exception—wrap the conversion in a
try/except (catching KeyError/TypeError/ValueError as appropriate) and return
False on failure, then continue the existing prev_hash comparison against
expected_prev.
In `@idempotency/middleware.py`:
- Around line 131-152: call_next can raise and leave the reservation key stuck
in "in_progress"; wrap the call to call_next in a try/except/finally such that
if call_next raises you transition the reservation out of in_progress (e.g.
update self._store.update(key, {..., "status": "failed", "stored_response":
minimal_stored_response(...)} , tenant_id=tenant_id) or delete the reservation)
and ensure created_at is preserved from self._store.get(key,
tenant_id=tenant_id) (use reserved["created_at"] if present); reference the
existing symbols call_next, self._store.get, self._store.update, key, tenant_id,
and minimal_stored_response when implementing the cleanup path.
- Around line 82-88: The current fingerprint input omits query parameters and
decodes the request body lossily; update the payload passed to
request_fingerprint to include the full query (e.g. request.url.query or
request.url.query_string) and preserve body bytes losslessly (do not decode with
"replace"—encode the raw bytes into a deterministic text form such as hex or
base64). Modify the block building payload in idempotency/middleware.py (where
payload is constructed and request_fingerprint is called) to include a query
field and set body to a lossless encoded representation of the raw body bytes
before calling request_fingerprint.
- Around line 100-105: The replay branch currently returns a hardcoded
status_code=200; update it to use the original stored status so replays preserve
the original HTTP outcome by reading the status from the stored record (e.g.,
use existing.get("status_code") or existing.get("stored_response",
{}).get("status_code") with a safe fallback like 200) and pass that as the
JSONResponse status_code while still returning existing.get("stored_response")
as the content.
In `@idempotency/store.py`:
- Around line 233-262: The check-then-insert around store.get(...) and
store.put(...) is not atomic so concurrent callers can both reserve the same
key; change this to an atomic "put-if-absent" / compare-and-set operation:
implement or call a store method like put_if_absent(key, value, tenant_id=...)
(or use a store transaction/conditional write) that only inserts the in-progress
record when the key does not already exist, returning a boolean/previous value;
if the atomic insert fails because a record already exists, re-fetch the
existing record (using store.get(...)) and apply the same fingerprint/status
checks (using request_fingerprint, STATUS_IN_PROGRESS, STATUS_COMPLETED) and
raise IdempotencyConflictError or return the stored response accordingly instead
of calling execute().
- Around line 262-283: The execute() call in this block can raise and leave the
idempotency record stuck as "in_progress"; wrap the
execute()/summarize/minimal_stored_response flow in a try/except that on
exception updates the same store record (via store.update) for key/tenant_id to
transition from in_progress to a failure state (e.g. STATUS_FAILED or similar),
write a minimal_stored_response containing the error payload/metadata and
preserve the original created_at (using existing["created_at"] or
_reserved_created_at(store, key, tenant_id)), then re-raise the exception;
ensure you reference execute(), summarize, minimal_stored_response,
store.update, existing and _reserved_created_at so the cleanup runs before
propagating the error.
In `@tenancy/context.py`:
- Around line 53-58: The require_tenant() method currently validates
self.tenant_id using strip() but returns the original untrimmed value; update
require_tenant() in the TenantContext class to normalize the id by computing a
stripped version (e.g. tid = str(self.tenant_id).strip()), validate tid, assign
the normalized tid back to self.tenant_id (or otherwise use tid for all
downstream uses) and return the stripped tid so repositories receive a
whitespace-trimmed tenant_id.
---
Outside diff comments:
In `@tests/integration/test_idempotency.py`:
- Around line 165-189: Add a regression test in
tests/integration/test_idempotency.py that simulates a failing execute() call
and asserts the idempotency key is not left as "in_progress" for the full TTL:
create a controlled idempotency execution (using the existing store_and_repo
fixture or store/ repo helpers), call the code path that invokes execute() in a
way that raises an exception, catch/expect that exception in the test, then call
store.reclaim_expired(tenant_id="default") (or advance time / adjust created_at
to simulate TTL expiry) and assert the stale key is removed (e.g., removed == 1
and the repo.list(IDEMPOTENCY_COLLECTION, id_field="key") does not contain the
key). Reference execute(), store.reclaim_expired, request_fingerprint and TTL to
locate the related logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88095bb6-5940-41af-8dd2-af983eb17ca8
📒 Files selected for processing (23)
api/main.pyattestix/audit/__init__.pyattestix/idempotency/__init__.pyattestix/tenancy/__init__.pyaudit/__init__.pyaudit/emitter.pyaudit/events.pyconfig.pydocs/extensibility-v0.4.0.mdidempotency/__init__.pyidempotency/middleware.pyidempotency/store.pypyproject.tomlspecs/001-v0.4.0-extensibility/data-model.mdspecs/001-v0.4.0-extensibility/spec.mdspecs/001-v0.4.0-extensibility/tasks.mdstorage/file_repository.pytenancy/__init__.pytenancy/context.pytests/conftest.pytests/integration/test_idempotency.pytests/integration/test_tenant_isolation.pytests/unit/test_audit_events.py
Comment on lines
+51
to
+56
| # Serialize chain computation within this process so two threads reading | ||
| # the same tenant's last hash cannot both append off the same prev_hash. | ||
| # (The file Repository's own filelock guards cross-process atomicity; this | ||
| # guards in-process emitters sharing one Repository instance.) | ||
| self._lock = threading.Lock() | ||
|
|
There was a problem hiding this comment.
Lock scope does not protect against concurrent emitters in the same process.
Lines 51-56 use an instance lock, so two AuditEventEmitter instances can still compute the same prev_hash concurrently and fork the tenant chain.
Suggested direction
class AuditEventEmitter:
+ _global_lock = threading.Lock()
+ _tenant_locks = {}
@@
- self._lock = threading.Lock()
+ # lock registry is shared across emitter instances
@@
+ `@classmethod`
+ def _lock_for_tenant(cls, tenant_id: str) -> threading.Lock:
+ with cls._global_lock:
+ lock = cls._tenant_locks.get(tenant_id)
+ if lock is None:
+ lock = threading.Lock()
+ cls._tenant_locks[tenant_id] = lock
+ return lock
@@
- with self._lock:
+ with self._lock_for_tenant(tenant_id):
prev_hash = self._last_chain_hash(tenant_id)
event = AuditEvent.create(Also applies to: 89-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@audit/emitter.py` around lines 51 - 56, The instance-level self._lock in
AuditEventEmitter does not prevent two separate emitter instances from racing;
replace it with a shared class-level lock registry keyed by tenant (or
repository) so all emitters for the same tenant serialize chain computation.
Concretely, add a class attribute like AuditEventEmitter._tenant_locks =
defaultdict(threading.Lock) (or similar) and update all places that currently
use self._lock (the initialization and the critical sections around computing
prev_hash and appending the event — the code around lines where self._lock is
set and the block at lines ~89-100) to acquire the tenant-specific lock (e.g.,
with AuditEventEmitter._tenant_locks[tenant_id]) instead of the instance lock.
Ensure locks are released appropriately and avoid leaking locks for one-off
tenants if needed.
Comment on lines
+109
to
+111
| if self._sink is not None: | ||
| self._sink(event) | ||
| return event |
There was a problem hiding this comment.
Guard external sink failures to avoid post-commit error propagation.
If Line 110 raises, the state change and local audit write already committed, but callers still observe failure. Handle sink failures explicitly (log/retry/outbox) instead of surfacing them as hard errors.
Suggested fix
+import logging
@@
+logger = logging.getLogger(__name__)
@@
if self._sink is not None:
- self._sink(event)
+ try:
+ self._sink(event)
+ except Exception:
+ logger.exception("Audit external sink failed for event_id=%s", event.event_id)
return event📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if self._sink is not None: | |
| self._sink(event) | |
| return event | |
| if self._sink is not None: | |
| try: | |
| self._sink(event) | |
| except Exception: | |
| logger.exception("Audit external sink failed for event_id=%s", event.event_id) | |
| return event |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@audit/emitter.py` around lines 109 - 111, Wrap the external sink call
(self._sink(event)) in a try/except so failures from the external sink do not
propagate after the local state and audit write have committed: catch Exception
around self._sink(event), log the error with contextual info (including event
id/payload) using the emitter's logger (e.g., self._logger or the module
logger), and do not re-raise; instead either schedule a retry or enqueue the
event to your outbox/retry mechanism if one exists, but always return event to
callers regardless of sink outcome.
Comment on lines
+194
to
+199
| for ev in events: | ||
| if isinstance(ev, AuditEvent): | ||
| event = ev | ||
| else: | ||
| event = AuditEvent(**{k: ev[k] for k in _AUDIT_FIELDS}) | ||
| if event.prev_hash != expected_prev: |
There was a problem hiding this comment.
Return False on malformed records instead of raising in verify_chain().
Line 198 can raise KeyError/TypeError for partial or corrupted dicts. That violates the boolean verification contract and turns tampering into an exception path.
Suggested fix
def verify_chain(events) -> bool:
@@
expected_prev = GENESIS_HASH
for ev in events:
- if isinstance(ev, AuditEvent):
- event = ev
- else:
- event = AuditEvent(**{k: ev[k] for k in _AUDIT_FIELDS})
+ try:
+ if isinstance(ev, AuditEvent):
+ event = ev
+ else:
+ event = AuditEvent(**{k: ev[k] for k in _AUDIT_FIELDS})
+ except (TypeError, KeyError):
+ return False
if event.prev_hash != expected_prev:
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for ev in events: | |
| if isinstance(ev, AuditEvent): | |
| event = ev | |
| else: | |
| event = AuditEvent(**{k: ev[k] for k in _AUDIT_FIELDS}) | |
| if event.prev_hash != expected_prev: | |
| for ev in events: | |
| try: | |
| if isinstance(ev, AuditEvent): | |
| event = ev | |
| else: | |
| event = AuditEvent(**{k: ev[k] for k in _AUDIT_FIELDS}) | |
| except (TypeError, KeyError): | |
| return False | |
| if event.prev_hash != expected_prev: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@audit/events.py` around lines 194 - 199, In verify_chain(), the loop that
converts each ev into an AuditEvent using AuditEvent(**{k: ev[k] for k in
_AUDIT_FIELDS}) can raise KeyError/TypeError for malformed/corrupted dicts;
change this so any exception during that conversion (or if ev is not the
expected type) causes verify_chain() to return False instead of propagating the
exception—wrap the conversion in a try/except (catching
KeyError/TypeError/ValueError as appropriate) and return False on failure, then
continue the existing prev_hash comparison against expected_prev.
Comment on lines
+82
to
+88
| body = await request.body() | ||
| payload = { | ||
| "method": request.method.upper(), | ||
| "path": request.url.path, | ||
| "body": body.decode("utf-8", "replace"), | ||
| } | ||
| fingerprint = request_fingerprint(payload) |
There was a problem hiding this comment.
Use a lossless, complete fingerprint input.
Line 83–87 omits query parameters and uses decode(..., "replace"), which can map different byte payloads to the same fingerprint input. Include query and preserve body bytes losslessly.
Possible fix
payload = {
"method": request.method.upper(),
"path": request.url.path,
- "body": body.decode("utf-8", "replace"),
+ "query": request.url.query,
+ "body_hex": body.hex(),
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@idempotency/middleware.py` around lines 82 - 88, The current fingerprint
input omits query parameters and decodes the request body lossily; update the
payload passed to request_fingerprint to include the full query (e.g.
request.url.query or request.url.query_string) and preserve body bytes
losslessly (do not decode with "replace"—encode the raw bytes into a
deterministic text form such as hex or base64). Modify the block building
payload in idempotency/middleware.py (where payload is constructed and
request_fingerprint is called) to include a query field and set body to a
lossless encoded representation of the raw body bytes before calling
request_fingerprint.
Comment on lines
+100
to
+105
| if existing.get("status") == "completed": | ||
| return JSONResponse( | ||
| status_code=200, | ||
| content={"idempotent_replay": True, | ||
| "stored_response": existing.get("stored_response")}, | ||
| ) |
There was a problem hiding this comment.
Return the original stored status code on replay.
Line 102 hardcodes 200, so replays don’t preserve the original HTTP outcome. Use the stored status value instead.
Possible fix
if existing.get("status") == "completed":
+ stored = existing.get("stored_response") or {}
return JSONResponse(
- status_code=200,
+ status_code=int(stored.get("status") or 200),
content={"idempotent_replay": True,
- "stored_response": existing.get("stored_response")},
+ "stored_response": stored},
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if existing.get("status") == "completed": | |
| return JSONResponse( | |
| status_code=200, | |
| content={"idempotent_replay": True, | |
| "stored_response": existing.get("stored_response")}, | |
| ) | |
| if existing.get("status") == "completed": | |
| stored = existing.get("stored_response") or {} | |
| return JSONResponse( | |
| status_code=int(stored.get("status") or 200), | |
| content={"idempotent_replay": True, | |
| "stored_response": stored}, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@idempotency/middleware.py` around lines 100 - 105, The replay branch
currently returns a hardcoded status_code=200; update it to use the original
stored status so replays preserve the original HTTP outcome by reading the
status from the stored record (e.g., use existing.get("status_code") or
existing.get("stored_response", {}).get("status_code") with a safe fallback like
200) and pass that as the JSONResponse status_code while still returning
existing.get("stored_response") as the content.
Comment on lines
+131
to
+152
| response = await call_next(request) | ||
|
|
||
| from idempotency.store import minimal_stored_response | ||
|
|
||
| reserved = self._store.get(key, tenant_id=tenant_id) | ||
| created_at = reserved["created_at"] if reserved else _now_iso() | ||
| self._store.update( | ||
| key, | ||
| { | ||
| "key": key, | ||
| "tenant_id": tenant_id, | ||
| "request_fingerprint": fingerprint, | ||
| "stored_response": minimal_stored_response( | ||
| {"status_code": response.status_code}, | ||
| status=response.status_code, | ||
| ), | ||
| "status": "completed", | ||
| "created_at": created_at, | ||
| }, | ||
| tenant_id=tenant_id, | ||
| ) | ||
| return response |
There was a problem hiding this comment.
Protect reservation lifecycle when downstream raises.
If call_next raises, the reserved key never transitions out of in_progress, causing repeated 409s for that key until TTL expiry. Add exception-path cleanup/transition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@idempotency/middleware.py` around lines 131 - 152, call_next can raise and
leave the reservation key stuck in "in_progress"; wrap the call to call_next in
a try/except/finally such that if call_next raises you transition the
reservation out of in_progress (e.g. update self._store.update(key, {...,
"status": "failed", "stored_response": minimal_stored_response(...)} ,
tenant_id=tenant_id) or delete the reservation) and ensure created_at is
preserved from self._store.get(key, tenant_id=tenant_id) (use
reserved["created_at"] if present); reference the existing symbols call_next,
self._store.get, self._store.update, key, tenant_id, and minimal_stored_response
when implementing the cleanup path.
Comment on lines
+233
to
+262
| fingerprint = request_fingerprint(payload) | ||
| existing = store.get(key, tenant_id=tenant_id) | ||
| if existing is not None: | ||
| if existing.get("request_fingerprint") != fingerprint: | ||
| raise IdempotencyConflictError( | ||
| f"Idempotency key {key!r} was already used with a different request " | ||
| f"payload; refusing to overwrite the prior result." | ||
| ) | ||
| if existing.get("status") == STATUS_COMPLETED: | ||
| return existing.get("stored_response"), True | ||
| # in_progress: a concurrent/earlier attempt holds the key. First-writer | ||
| # wins; we do not re-execute. Return the in-progress marker so the caller | ||
| # can surface a 409-style "request in flight" rather than duplicating. | ||
| raise IdempotencyConflictError( | ||
| f"Idempotency key {key!r} is already in progress; not re-executing." | ||
| ) | ||
|
|
||
| # First writer: reserve the key, then execute. | ||
| store.put( | ||
| { | ||
| "key": key, | ||
| "tenant_id": tenant_id, | ||
| "request_fingerprint": fingerprint, | ||
| "stored_response": None, | ||
| "status": STATUS_IN_PROGRESS, | ||
| "created_at": _now().isoformat(), | ||
| }, | ||
| tenant_id=tenant_id, | ||
| ) | ||
| result = execute() |
There was a problem hiding this comment.
Make key reservation atomic to enforce first-writer-wins.
Line 234 and Line 251 perform a non-atomic check-then-insert. Under concurrency, two callers can both see None and both execute. This breaks idempotency guarantees.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@idempotency/store.py` around lines 233 - 262, The check-then-insert around
store.get(...) and store.put(...) is not atomic so concurrent callers can both
reserve the same key; change this to an atomic "put-if-absent" / compare-and-set
operation: implement or call a store method like put_if_absent(key, value,
tenant_id=...) (or use a store transaction/conditional write) that only inserts
the in-progress record when the key does not already exist, returning a
boolean/previous value; if the atomic insert fails because a record already
exists, re-fetch the existing record (using store.get(...)) and apply the same
fingerprint/status checks (using request_fingerprint, STATUS_IN_PROGRESS,
STATUS_COMPLETED) and raise IdempotencyConflictError or return the stored
response accordingly instead of calling execute().
Comment on lines
+262
to
+283
| result = execute() | ||
|
|
||
| status, resource_id = (None, None) | ||
| if summarize is not None: | ||
| status, resource_id = summarize(result) | ||
| stored = minimal_stored_response(result, status=status, resource_id=resource_id) | ||
|
|
||
| store.update( | ||
| key, | ||
| { | ||
| "key": key, | ||
| "tenant_id": tenant_id, | ||
| "request_fingerprint": fingerprint, | ||
| "stored_response": stored, | ||
| "status": STATUS_COMPLETED, | ||
| # Preserve the original creation time so the TTL is measured from the | ||
| # first attempt, not from completion. | ||
| "created_at": existing["created_at"] if existing else _reserved_created_at(store, key, tenant_id), | ||
| }, | ||
| tenant_id=tenant_id, | ||
| ) | ||
| return result, False |
There was a problem hiding this comment.
Handle execute() failures so keys don’t get stuck in_progress.
If execute() raises at Line 262, the record remains in_progress, and future retries with the same key will keep failing until TTL expiry. Add failure cleanup/transition logic before re-raising.
Suggested direction
- result = execute()
+ try:
+ result = execute()
+ except Exception:
+ # release or mark failed so retried requests are not blocked for 24h
+ # (requires explicit store API support for failure transition/removal)
+ raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@idempotency/store.py` around lines 262 - 283, The execute() call in this
block can raise and leave the idempotency record stuck as "in_progress"; wrap
the execute()/summarize/minimal_stored_response flow in a try/except that on
exception updates the same store record (via store.update) for key/tenant_id to
transition from in_progress to a failure state (e.g. STATUS_FAILED or similar),
write a minimal_stored_response containing the error payload/metadata and
preserve the original created_at (using existing["created_at"] or
_reserved_created_at(store, key, tenant_id)), then re-raise the exception;
ensure you reference execute(), summarize, minimal_stored_response,
store.update, existing and _reserved_created_at so the cleanup runs before
propagating the error.
Comment on lines
+53
to
+58
| if not self.tenant_id or not str(self.tenant_id).strip(): | ||
| raise ValueError( | ||
| "TenantContext has no resolvable tenant_id; refusing to default " | ||
| "silently to another tenant's scope." | ||
| ) | ||
| return self.tenant_id |
There was a problem hiding this comment.
Normalize tenant id before returning from require_tenant().
Line 53 validates strip(), but Line 58 returns the untrimmed value. A directly constructed context like " acme " passes validation and then propagates whitespace into repository scoping.
Suggested fix
def require_tenant(self) -> str:
@@
- if not self.tenant_id or not str(self.tenant_id).strip():
+ tenant = str(self.tenant_id).strip() if self.tenant_id is not None else ""
+ if not tenant:
raise ValueError(
"TenantContext has no resolvable tenant_id; refusing to default "
"silently to another tenant's scope."
)
- return self.tenant_id
+ return tenant📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not self.tenant_id or not str(self.tenant_id).strip(): | |
| raise ValueError( | |
| "TenantContext has no resolvable tenant_id; refusing to default " | |
| "silently to another tenant's scope." | |
| ) | |
| return self.tenant_id | |
| tenant = str(self.tenant_id).strip() if self.tenant_id is not None else "" | |
| if not tenant: | |
| raise ValueError( | |
| "TenantContext has no resolvable tenant_id; refusing to default " | |
| "silently to another tenant's scope." | |
| ) | |
| return tenant |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tenancy/context.py` around lines 53 - 58, The require_tenant() method
currently validates self.tenant_id using strip() but returns the original
untrimmed value; update require_tenant() in the TenantContext class to normalize
the id by computing a stripped version (e.g. tid = str(self.tenant_id).strip()),
validate tid, assign the normalized tid back to self.tenant_id (or otherwise use
tid for all downstream uses) and return the stripped tid so repositories receive
a whitespace-trimmed tenant_id.
5 tasks
This was referenced May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Completes the v0.4.0 extensibility layer on top of P1 (#82): US2 (tenant context + structured audit events) and US3 (idempotency).
P2 — tenant context + audit events
tenancy/:TenantContext,DEFAULT_TENANT,resolve_tenant()(optionalX-Attestix-Tenantheader, defaults to"default";require_tenant()rejects blank rather than silently borrowing scope).audit/events.py: frozenAuditEvent+ per-tenant hash chain (reuses the provenanceprev_hash/chain_hash/genesis pattern + JCS canonicalization);verify_chain()detects tampering;change_digestis SHA-256, never the raw body.audit/emitter.py:AuditEventEmitterwith a Repository-backed local sink + injectable external sink (FR-017); FR-028 PII / encryption-at-rest contract documented.tenant_idreads as"default"; single-tenant self-host unchanged. (Storage-side tenant scoping already shipped in P1.)P3 — idempotency
idempotency/store.py:IdempotencyStoreABC +RepositoryIdempotencyStore(24h TTL +reclaim_expired()), surface-agnosticrun_idempotent()helper, and FR-029minimal_stored_response(status + resource_id + response_hash only — never raw key material or signed bodies).idempotency/middleware.py: opt-inIdempotencyMiddleware(not auto-mounted — see Scope).Resolved [NEEDS CLARIFICATION] (T046)
File idempotency concurrency = first-writer-wins via
filelock; scope = surface-agnostic, REST-primary; no-op update emits one event; default REST tenant = header-or-"default". Encoded in spec.md + data-model.md.Test plan
ruff check .clean.Scope (deferred, tree green)
TODOseam inapi/main.py.Summary by CodeRabbit
New Features
Documentation