feat(client): expose public single-event ingestion API - #124
Diego Colombo (colombod) merged 2 commits into
Conversation
Diego Colombo (colombod)
left a comment
There was a problem hiding this comment.
Deep review with an independent DTU run. Requesting changes on two items; everything else is a follow-up or a note.
Short version: the pattern is right, this is the correct seam for applications composing context-intelligence as a library, and the evidence packet is stronger than most things that merge here. One defect is real, new, and sits on a supported path. One is bookkeeping. One namespace decision calcifies on release.
1. Independent verification
Suites (run against d4a1172)
| Check | Result |
|---|---|
Root uv run --frozen pytest tests -q |
880 passed |
ruff check / ruff format --check |
clean, 178 files |
pyright |
0 errors, 0 warnings |
modules/hook-context-intelligence |
700 passed |
modules/tool-context-intelligence-query (PYTHONPATH=../..) |
195 passed |
modules/tool-context-intelligence-upload |
553 passed, 1 failed |
The single upload-module failure (test_ground_truth_parity.py::test_runtime_sweep_data_parity, a JSONDecodeError) reproduces identically on a clean origin/main worktree at 8789404 — pre-existing, not caused by this PR.
DTU run
Launched this repo's own .amplifier/digital-twin-universe/profiles/context-intelligence-backend.yaml — real Neo4j Community 5.26.22 + standalone context-intelligence-server v6.10.0 (neo4j_connected: true, static-key auth), SERVER_REF=43973967 (server repo HEAD). Destroyed after the run.
Your five checks reproduce independently:
{"passed": true, "checks": {"first_acceptance": true, "duplicate_receipt": true,
"one_graph_event": true, "auth_fails_loudly": true, "network_fails_loudly": true}}
Nine further scenarios this PR did not live-test, run against that backend:
1 working_dir supplied OK queued
2 no data.session_id OK queued, session_id: null
3 no data.timestamp CIERR http_status 400 "data.timestamp is required..."
4 custom envelope+key+extra field OK queued
5a hook-shaped envelope via ingest OK queued
5b new envelope, same v1 key OK duplicate
6 unserializable payload (set) RAW TypeError escapes <-- see blocker B1
7 NaN in payload RAW ValueError escapes <-- see blocker B1
8 timeout=-1 vs real server CIERR timeout, ZERO requests reached the server
9 graph: exactly 1 node per accepted event; the 400'd event never landed; total 4
Scenario 5 is the most useful new evidence. The hook's own envelope (carrying working_dir: "/hookdir") posted first returned queued; the new builder's envelope for the same logical event — a different envelope shape, no working_dir key at all — returned duplicate. The aci-event-v1 key genuinely dedupes across both builders at a live server, and the envelope-shape divergence does not break it. The PR proves this only by comparing hashes in a unit test; it is now proven end-to-end.
Scenario 2 also confirms a real server returns session_id: null, so the receipt validator's allowance for that is required behaviour rather than dead permissiveness.
Note in passing: this PR tested against server HEAD 43973967 (2026-09-16), while our canonical DTU profile still pins 766a9691 (2026-07-14). Your evidence is against a newer server than our own DTU suite defaults to. The stale profile pin is a pre-existing repo issue, not yours.
2. Blockers
B1 — the documented error contract is broken by new code, on a supported path
context_intelligence/client.py:940
body = json.loads(json.dumps(payload, allow_nan=False)) # <- ABOVE the try:The JSON preflight sits outside ingest's try:. A set, a datetime or a NaN in a caller-supplied envelope escapes as a raw TypeError / ValueError — live-proven in probes 6 and 7 above — while README.md:233-237 tells application authors "Failures use CIClientError".
This is not misuse. The API explicitly blesses custom server-compatible envelopes that bypass build_event_payload's own validation, so this is the supported path. A host that writes except CIClientError: around its outbox takes an unhandled crash in production on a payload it constructed itself.
One subtlety on the fix: do not simply move the encode inside the existing try. That block's except ValueError at client.py:975 would relabel a caller's NaN as decode_error / "invalid event receipt" — blaming the server for the caller's payload. It needs its own except and its own error_type.
A verified patch is in §5.
B2 — version not bumped
pyproject.toml:3 still reads version = "0.1.3" while this PR adds a public API and a new [client] extra, so two materially different artifacts answer to the same version. modules/hook-context-intelligence pins the library @main, so consumers resolve by ref today — which is exactly why the bump is cheap now and confusing later. Suggest 0.2.0 in the same commit.
3. Please decide before merge (not asking for a reshape)
S1 — is context_intelligence.upload the right home?
context_intelligence/__init__.py:3-15 defines the package's own three-level taxonomy. build_event_payload is squarely Level 1 — pure transform, no I/O. The upload/ namespace was reserved in writing for the Level 3 session uploader currently in modules/tool-context-intelligence-upload/ (CLI entry point, progress tracking, graph construction). The replacement docstring now has to open by disclaiming what the package does not contain.
The seam also reads as two halves imported at two different depths:
from context_intelligence import AsyncCIClient # top level, in __all__
from context_intelligence.upload import build_event_payload # subpackage, not exportedcontext_intelligence/events.py, re-exported from the top level, would pair with the endpoint (POST /events) and with ingest, and would leave upload/ free for the migration its previous docstring promised.
Cost now: git mv, one __all__ line, and four references (README.md:207, scripts/verify-event-ingestion.py:21,96, one test import) — nothing else in the repo imports it, verified. Cost after release: a permanent alias shim.
Either outcome is fine — accept upload deliberately and say so in the PR, or rename. I'd just rather it be a decision than a default.
4. The pattern itself — endorsed
The mechanism/policy cut is in the right place, and the hook is the evidence. _DestinationDispatcher (logging_handler.py:283) bundles five separable concerns — a capacity-dropping queue (:342, :472-493), backoff with jitter (:39-48), an auth-only circuit breaker (:704-741), a sustained-failure escalation clock (:560-613), and gitignore-style fan-out. Every one is a tuned choice a host application would legitimately make differently. Correctly left out.
Also worth stating, because it pre-empts the obvious objection: the hook's queue is not a durable outbox — it drops on overflow (:493) and skips hard failures to keep the worker moving (:914-917); durability comes from events.jsonl, a separate concern. So there was no reusable outbox to promote. "Too much policy pushed onto callers" does not hold here.
One thing that genuinely is mechanism and is missing (follow-up, not a blocker): _classify_http_outcome (logging_handler.py:160-197). "Is 401 retryable?" is not a host preference — it is a non-obvious fact about this server's protocol: 401 transient (token refresh), 403 permanent, 404/410 meaning the endpoint is undeployed rather than the payload malformed, 408 pulled out of the 4xx catch-all. Every host will re-derive that table and get 401 wrong. Worth filing classify_ingest_failure(err) -> "retry" | "permanent" now while the reasoning is fresh.
5. Verified patch for B1 + B2
Applied to d4a1172 in a scratch worktree, run, then reverted — 885 passed (880 + 5 new), ruff clean, format clean, pyright 0 errors.
# context_intelligence/client.py -- in ingest(), replacing lines 937-941
url = f"{self._server_url}/events"
# JSON encoding is also a strict preflight: no NaN or Infinity on the wire.
# It gets its OWN except block: folding it into the request try below would
# relabel a caller's unserializable envelope as "decode_error", blaming the
# server for the caller's payload.
try:
if not isinstance(payload, dict):
raise TypeError("event payload must be an object")
body = json.loads(json.dumps(payload, allow_nan=False))
except (TypeError, ValueError) as exc:
raise CIClientError(
"event payload is not a JSON-encodable object",
error_type="invalid_payload",
url=url,
) from excplus the widened enum on CIClientError.error_type (client.py:73-74), and:
# tests/test_event_ingestion.py
@pytest.mark.parametrize(
"payload",
[[], "not-an-envelope", {"data": {"unserializable": {1, 2}}},
{"data": {"value": float("nan")}}, {"data": {"value": float("inf")}}],
)
async def test_unusable_caller_envelope_is_a_client_error_not_a_raw_exception(payload):
def handle(request):
pytest.fail("No request is made for an unusable envelope")
with transport(handle), pytest.raises(CIClientError) as caught:
await AsyncCIClient("http://server.invalid", "synthetic-key").ingest(payload)
assert caught.value.error_type == "invalid_payload"Deliberately not pushed to your branch. docs/lanes/public-event-ingestion/evidence/live.json pins client_source_sha256 for context_intelligence/client.py; I confirmed the committed hash matches the file today, and the patch changes it to f3861b02…. A third party editing that file would silently turn a verifiable provenance record into a stale one. Landing this means re-running scripts/verify-event-ingestion.py and re-attesting the evidence, and that should be the author's signature, not a reviewer's. Also, invalid_payload extends a documented public enum — your call on the name.
6. Follow-ups — file, don't block
_auth_headers(client.py:909-914) catches onlyValueError; a real EntraClientAuthenticationErrorescapes unclassified. Pre-existing across all 10 call sites includingcypher:1025— fixing it here would widen the blast radius across the whole client. Butingest's README newly advertises that contract to external authors, so it matters more now than it did.timeout<=0yields zero HTTP attempts (live-proven, probe 8);timeout=Nonewaits unbounded. Pre-existingAsyncCIClient.__init__gap.- The parity test (
tests/test_event_ingestion.py:31-55) asserts key equality for one input, and the two implementations already diverge outside it: the hook coercesworkspace=None -> ""and hashes it (hook/upload.py:15,20) while the helper rejects blank (upload/__init__.py:27-28); the helper passesallow_nan=False(:37), the hook does not (hook/upload.py:12). Neither is a wire break — the helper is the stricter, newer one — but "compatible with hook v1" is a claim about a function currently backed by a single anecdote. Parametrizing over ~5 shapes is three lines in a file this PR already touches. - Single-source the
aci-event-v1scheme. Worth noting the duplication is unforced: the hook already depends on the root library (modules/hook-context-intelligence/pyproject.toml:11, imported at module scope inconfig_resolver.py:11-12). What holds it together today is that.github/workflows/ci.ymlhas no path filters, so a hook-only edit still runs the parity test. classify_ingest_failure(see §4).ingestreturns untypeddict[str, Any]. A 4-lineIngestReceiptTypedDict withLiteral["queued","duplicate"]is a source-compatible widening — free later, and it gives callers exhaustiveness on the one branch that can lose an event.- Per-call
httpx.AsyncClient. Measured on the DTU: 19.42 ms/event vs 13.31 ms pooled — 1.46x, +6.1 ms on plain HTTP over loopback; worse over TLS to a remote host. Consistent withcypher:1026andfetch_blob:1091, so this is a client-wide lifecycle decision, not a reason to special-caseingest. README.md:240-243describesduplicateas "recognition of an existing key" directly beside the retry advice, without repeating that at this server revision aduplicateis not evidence of durable acceptance. The precise warning is indocs/lanes/public-event-ingestion/README.md:68-76— moving one sentence up to where the wrong outbox transition actually gets made would close it.except ValueError(client.py:975) maps both a JSON decode failure and the shape rejection at:958todecode_error. Both are ambiguous-delivery so outboxes behave correctly; the label is just slightly untrue on a public API.canonical.encode()(upload/__init__.py:40) vs the hook's.encode("utf-8")— byte-identical today, but it is a hash input, so worth making explicit.tests/test_package_skeleton.py:126-133still asserts the old placeholder docstring substring. It passes only because the new docstring happens to containtool-context-intelligence-upload.AGENTS.md:96-99reads "ANY ... networking / auth edit -> a real DTU run + the evaluation harness", while:71says "DTU or an equivalent live run". Those two sentences disagree, and this PR sits exactly in the gap. §1 above discharges it either way, but the wording is worth reconciling so the next author is not guessing.modules/tool-context-intelligence-upload::test_runtime_sweep_data_parityfails onmain— unrelated, flagging for visibility.
7. Things I looked at and would explicitly not ask for
- A middle layer (outbox / retry scheduler / spooler). Promote one only after two independent host apps converge on the same need.
- Sync
ingest. It is not a wrapper:_http_post(client.py:133-169) swallows every exception and returnsNoneon all three backends, destroying exactly the "definitely rejected" vs "ambiguous, do not discard" distinction an outbox needs, and there is no_http_post_strict. Separate work. Worth one README line notingasyncio.run(client.ingest(p))so the absence does not read as an oversight. - More live evidence. The existing packet clears the seam bar.
- Anything about the server's check-before-append window. Correctly found, correctly scoped to the server repo, correctly not papered over in the client.
8. Credit
- The idempotency-key compatibility claim is true, and now proven at a live server across two different envelope shapes.
- Finding and documenting the server-side
idempotency_cache.check_and_store-before-durable-append window (main.py:1501-1533) rather than silently working around it — and refusing to promoteduplicateinto a durability claim — is exactly the right call. - Every failure mode I probed against a real server behaved correctly and loudly: 400 with a useful detail, 401, connection refusal, redirect not followed.
Happy to re-review immediately once B1/B2 land and the evidence is regenerated.
|
Diego Colombo (@colombod), this is ready for re-review at
The real server/Neo4j evidence was regenerated after these changes. I rechecked today that both recorded client-source hashes still match this exact head; all five captured seam checks passed. I also reran the focused ingestion suite today: 34 passed. All current-head CI jobs and the CLA check are green. The scope remains the additive event-ingestion library API. This does not change the session logging hook, on-disk session format, session reconstruction/resume, or server implementation. Routing, consent, delivery queues and retry policy remain with callers. The follow-up topics from your review remain outside this PR's scope. Could you please re-review the corrected head? Thank you for the independent verification and specific fixes. |
Diego Colombo (colombod)
left a comment
There was a problem hiding this comment.
Re-reviewed at 79a6162253c7f8c66e5d2af7e9c8ffcdf38865dd. All three items addressed — approving.
Confirmed this head is byte-identical to the one I independently verified, so everything below applies to exactly the commit you asked me to look at.
The three items
B1 — fixed, and the test is better than the patch I offered. The preflight now has its own try and its own error_type, with the reasoning preserved in a comment so the next reader does not re-fold it into the request block. Your test asserts three things where mine asserted one: that the error is classified, that the credential strategy is never consulted, and that the request never reaches the network — plus a datetime case I had missed. Verified live against a real server:
6 unserializable payload (set) CIERR type=invalid_payload (was: raw TypeError)
7 NaN in payload CIERR type=invalid_payload (was: raw ValueError)
B2 — 0.2.0. Build confirms amplifier_bundle_context_intelligence-0.2.0-py3-none-any.whl, shipping context_intelligence/events.py.
S1 — took the rename, and did the better version of it. context_intelligence/events.py at Level 1, re-exported from __all__, and upload/__init__.py restored to its original placeholder rather than left as a stub. That does not just relocate the function, it un-reserves the namespace — and it incidentally closes follow-up #11, since test_package_skeleton.py:126-133 now asserts a docstring that is true again instead of passing on a coincidence.
Also picked up unprompted — appreciated, none of these were asked for as conditions: #3 (parity test parametrized across five JSON shapes, with working_dir="/hookdir" on the hook side to prove the key ignores it), #8 (the README now states a duplicate cannot prove durable acceptance, positioned next to the retry advice where the wrong outbox transition gets made), #10 (.encode("utf-8") made explicit), and the asyncio.run(...) note so the absent sync API reads as deliberate.
Verification at this head
Provenance re-attested — the reason this was yours to land rather than mine:
context_intelligence/client.py committed 62de48c0… actual 62de48c0… MATCH
context_intelligence/events.py committed e513d778… actual e513d778… MATCH
The harness now hashes events.py rather than upload/__init__.py, and executed_at moved to 2026-09-19T03:08:44Z. The evidence packet is internally consistent again.
| Check | Result |
|---|---|
Root pytest tests -q |
891 passed |
tests/test_event_ingestion.py |
34 passed — matches your count |
modules/hook-context-intelligence |
700 passed |
modules/tool-context-intelligence-query |
195 passed |
ruff check / format --check / pyright |
clean · 179 files · 0 errors, 0 warnings |
uv build --no-sources |
sdist + wheel at 0.2.0, wheel contains context_intelligence/events.py |
Clean install, no [client] extra |
build_event_payload imports and works; AsyncCIClient fails loud with the documented ImportError |
| CI at head | all 12 jobs green incl. CLA |
Fresh DTU — since client.py changed, I re-ran the seam rather than trusting the previous run. context-intelligence-backend.yaml, real Neo4j Community 5.26.22 + context-intelligence-server v6.10.0 at server revision 43973967. Your five checks pass, and the nine extra probes from my first review re-run with only the two intended changes:
1 working_dir supplied OK queued
2 no data.session_id OK queued, session_id: null
3 no data.timestamp CIERR http_status 400 "data.timestamp is required..."
4 custom envelope+key+extra field OK queued
5a hook-shaped envelope via ingest OK queued
5b new envelope, same v1 key OK duplicate
8 timeout=-1 vs real server CIERR timeout, zero requests reached the server
9 graph: 1 node per accepted event, total 4; the 400'd event never landed
Cross-compatibility (5a/5b) is intact, graph counts are identical to the pre-fix run, and nothing else moved. No regression.
One housekeeping note for the record: in my first review I flagged modules/tool-context-intelligence-upload::test_runtime_sweep_data_parity failing locally. It also fails on a clean origin/main, and the tool-context-intelligence-upload CI job is green at this head — so it is environment-dependent on my machine and unrelated to this PR in either direction. Nothing for you to do here; noting it so the earlier mention does not linger as an open question.
Scope
Agreed, and no argument from me. The follow-ups from my first review — _auth_headers catching only ValueError (#1), timeout<=0 (#2), single-sourcing the v1 key (#4), classify_ingest_failure (#5), IngestReceipt (#6), per-call AsyncClient (#7), the decode_error label (#9), and the AGENTS.md:71 vs :96-99 contradiction (#12) — are all either pre-existing across the whole client or genuinely separable. Worth issues; not worth widening this PR.
Of those, the one I would file first is #5, classify_ingest_failure. The knowledge in _classify_http_outcome (logging_handler.py:160-197) — 401 transient because a token needs refreshing, 403 permanent, 404/410 meaning the endpoint is undeployed rather than the payload malformed, 408 pulled out of the 4xx catch-all — is protocol fact, not host preference. Every application building on ingest will re-derive that table, and 401 is the one they will get wrong. Worth capturing while the reasoning is still fresh.
Approving
The mechanism/policy split is right, the seam evidence is real and now re-attested against the corrected source, and the namespace is where it belongs. This is a good unlock for applications composing context-intelligence as a library.
Thanks for the thorough turnaround — and for regenerating the evidence rather than letting the hashes drift.
Summary
Applications currently have to mount a telemetry hook or import private uploader code to submit their own events. This adds
build_event_payloadandAsyncCIClient.ingest, reusing existing auth/error contracts while leaving routing, consent, outboxes and retry policy to the caller. The Level 1 builder lives incontext_intelligence.eventsand is re-exported from the package root. Invalid custom payloads raiseCIClientError(error_type="invalid_payload")before auth or HTTP. The package version is 0.2.0. The optional[client]extra supplies HTTP transport. The client preserves both real server receipts (queuedandduplicate) and never retries or follows redirects.Scope / guardrails
The client→server
/eventsseam is exercised through real HTTP, static-key authorization and Neo4j. Existing write-side hook fan-out (fanout.py, dispatchers, logging handlers), query behavior, bundles, agents, skills, modes and server code are unchanged. Blocking credential refresh stays off the event loop, and caller-owned payloads are snapshotted before that refresh.Verification
modules/tool-context-intelligence-query, usingPYTHONPATH=../.. uv run --frozen pytest -qto exercise this checkout. Its existing lock otherwise imports an older client without the existingCIClientErrorclass.uv run --frozen pytest tests -q. Bare root discovery also collects independently packaged module suites and currently fails on missing module dependencies and collidingtests.conftestnames; the intended root suite is selected explicitly.scripts/validate-full.shexited 0 atvalidation_mode: full; effective PASS WITH SUGGESTIONS after adjudicating the known mode-advertising and standalone README false positives. Existing cosmetic agent-description warning is unchanged. The current recipe runner forces the CLI interpreter; a temporary hatchling PYTHONPATH overlay was needed despite the helper’s PATH-based venv. The separate wheel build covers the recipe’s missingpip wheeldry-run.uv build --no-sourcesproduced both sdist and wheel. The wheel's[client]extra installed in a fresh environment and passed isolatedpython -Iimports of both APIs.Real evidence on seams (not mock-only)
43973967ff4bc9a02422814a8c00dfce7624a76dand official Neo4j 5.26.22/APOC: first acceptance (queued), duplicate recognition, exactly one matching graph event, explicit HTTP 401, and connection refusal. The tests' receipt doubles match the observed server responses.Captured synthetic request/response and scores, provenance and limitations, and
scripts/verify-event-ingestion.pymake the run repeatable. Static-key auth was exercised live; Entra refresh uses strategy tests without claiming a live Azure run.Docs & diagrams
Notes / follow-ups
At the tested server revision,
idempotency_cache.check_and_storeprecedes durable append (main.py:1501–1533). If that append fails, a later duplicate receipt alone cannot prove the first event persisted. This client preserves that distinction rather than claiming exactly-once behavior. Correcting that server failure window needs a separate server change and fault-injection test; this run did not simulate a disk failure or server crash.