mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
b05546b57dd255f86da3f9940955b0ec130bbeca
2956 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Nicolò Boschi
|
b05546b57d |
fix(recall): a store-owned bank's attachment lookup does not read memory_units (#4306)
Recall resolves the attachments behind each fact by reading memory_units.attachment_ids, on
every recall. For a bank whose memories store owns its rows that table holds none of them, so
the read can only return nothing -- and it is not the cheap empty read it looks like.
memory_units carries partial vector indexes per bank, and the planner opens and locks every
index on a table to plan any statement against it. In a tenant with 5,286 banks that is 15,877
indexes: planning took 434-484 ms and 15,880 locks (15,863 on the slow path) for a statement
that executed in 0.04 ms, against ~2 ms and 22 locks in a tenant with a few banks. Twenty
concurrent recalls saturated LWLock:LockManager for the whole database: a 2 vCPU API pod fell
from 180 to ~13 recalls/s, and unrelated pods' readiness checks slowed from 6 ms to ~870 ms.
The guard uses the same store_owned_for check as the other store-owned paths. Store-owned banks
report no attachments on recall until the store can carry the ids itself.
The test asserts the lookup returns before it reads the bank profile or takes a connection,
not merely that it returns {}: an empty result is what the expensive read produced as well.
|
||
|
Nicolò Boschi
|
5be9ad9156 |
perf(recall): account for a recall's time per phase, and fix the three things that showed up (#4299)
* diag: measure event-loop lag, to tell a slow await from a busy loop
Every per-phase timer in recall measures its own await, so a request that is runnable but not
running is invisible to all of them: the phases stay fast while the total inflates. Measured on
this API under load, the instrumented phases covered 10% of a recall's wall time (store hop 32ms,
embedding 5ms of a 356ms mean) and no candidate I/O accounted for the other 90% — CPU, Postgres,
the embedding service, the store, auth and the recall semaphore were each measured and excluded.
Those two explanations need different fixes and look identical from the phase timings, so this
measures the difference directly. The probe sleeps a known interval and reports the overshoot,
which is time the loop spent elsewhere while the probe was ready. Lag near zero means a real await
is missing a timer; lag tracking request latency means the loop is oversubscribed and tuning I/O
will not help.
Off unless HINDSIGHT_API_LOOP_LAG is set, and started from the lifespan hook so it runs on the
loop that serves requests.
* diag(recall): time the tenant auth await, and stop post_recall being zero by construction
Two blind spots on the recall path, found while chasing 90% of a recall's wall time that no phase
timer accounted for (store hop 32ms, embedding 5ms of a 356ms mean, event-loop lag 0.2ms — so a
real await, not a busy loop).
`recall_duration` was measured at the END of the handler, so it already contained response
building, and `post_recall = handler_duration - pre_recall - recall_duration` was ~0 by
construction. The line reported pre=0 post=0 for every request and charged everything to
`recall=`, whichever layer actually spent it. It now ends where the engine call returns.
`recall_async` awaits `_authenticate_tenant` before any phase timer starts, so that call was
invisible to both the phase metrics and the handler's split. It is timed and logged when it
exceeds 25ms.
Diagnostics only; no behaviour change.
* diag(recall): time the four untimed awaits in recall_async
`recall_async` awaits four things that no phase timer covers: the operation-validator pre-hook
(`validate_recall`), fuzzy tag-group resolution, the bank-config read, and the validator post-hook
(`on_recall_complete`). Together with the already-timed embedding and store hop they are the whole
request, so with them dark the phase metrics accounted for only ~10% of a recall's wall time and
the remaining 90% looked like it belonged to some I/O nobody had instrumented.
Each is timed and logged over 25ms. Diagnostics only; no behaviour change.
Note for whoever reads the numbers: measured through a port-forward these are dominated by round
trip latency, so the useful output is WHICH await dominates in-cluster, not the millisecond values
from a laptop.
* diag(recall): trace the whole request, not the parts that already had timers
The recall phase metrics accounted for ~10% of a request's wall time on a loaded fleet — store hop
32ms and embedding 5ms of a 356ms mean — and the missing 90% survived every candidate: CPU at 4%,
both databases idle with sub-millisecond queries, TEI at 9%, the store at 0.6%, the recall
semaphore at ~0, connection pinning ruled out by a keep-alive A/B, and event-loop lag flat at
0.2ms (so a real await, not a busy loop). Each candidate was excluded one build at a time, which
is slow and only ever rules things out.
This times the whole path instead:
* `http_to_handler` — ASGI entry to the endpoint body. `handler_start` is set INSIDE the endpoint,
so routing, body parsing and dependency resolution (auth among them) sat outside every existing
timer and read as unattributed time.
* `engine_call` — the whole engine call, marked diagnostic because it contains the store and
embedding phases and would otherwise double-count.
* `post_engine` — from the engine returning to the response being built.
* `validate_pre`, `bank_config`, `fuzzy_tags`, `validate_post` — the four awaits in `recall_async`
that no phase covered.
They are metrics, not logs over a threshold: a `>25ms` line shows only the tail and cannot
distinguish "small and constant" from "small and rare", which is how a 30ms phase on 2% of
requests briefly looked like an explanation for 319ms on all of them.
`recall_duration` also now ends where the engine call returns rather than at the end of the
handler, so `post_recall` — computed as the remainder — stops being ~0 by construction.
Diagnostics only; no behaviour change.
* diag(recall): time the two awaits in the precheck dependency
`http_to_handler` — ASGI entry to the endpoint body — turned out to be a third of a recall, and
nothing inside it was timed. It contains the `precheck_for` dependency, which the docstring says
"authenticates the tenant" before the body is read, so the FIRST and uncached `_authenticate_tenant`
call happens here. The one inside `recall_async` is a cached re-resolution, which is why auth kept
measuring cheap and kept being cleared as a suspect.
Adds `dep_auth` and `dep_precheck`. Measured locally through a port-forward, `dep_auth` is 76% of
the pre-handler time and the largest single phase of the request — larger than the whole engine
call. Round-trip latency inflates the absolute value; the ranking is the finding.
Diagnostics only; no behaviour change.
* diag(recall): close the accounting — the missing time is the metering post-hook
The recall waterfall had ~950ms (45% of the engine call) that no phase covered, with the API at
1% of one core and every await, async-with and async-for in `recall_async` already timed. The
cause was an instrumentation bug of my own: `on_recall_complete` has THREE call sites and only the
first was wrapped, so `validate_post` recorded 0.0ms while the path actually taken went through
one of the other two.
With all three timed the residual is 1ms, and the breakdown is:
validate_post 968 ms 45% metering post-hook
search_with_retries 529 ms 24% the store + embedding
validate_pre 404 ms 19% metering pre-hook
bank_config 265 ms 12%
engine_auth, fuzzy_tags, semaphore_acquire, validate_post ~ 0
Measured through a port-forward, so the absolute numbers are round-trip dominated; the shares are
the finding. Metering is 63% of the engine call and is two synchronous control-plane hops per
recall — a credits read before the work and a credits UPDATE plus `usage_records` and
`crm_milestones` inserts after it.
Also splits `http_to_handler` into `mw_and_routing` / `deps_total` / `body_parse`, which closes
that region to a 0ms residual, and adds `recall_async_body` to separate "inside the coroutine"
from "between the handler's timer and the body running" (the latter is 0).
Diagnostics only; no behaviour change.
* diag(recall): time the hop minus the store's own reported stages
A store-answered recall reports the store's per-stage timings and `full_recall`, the whole
hop. The gap between them is ours -- the client, the serialization either side, and any
time the request sat in the channel -- and there was no way to read it.
It cannot be derived after the fact: percentiles of separate phases are not additive, so
subtracting one phase's p99 from another's says nothing. Recorded per request instead.
On a measured window it is 4.8 ms mean and 10 ms at p99, which is what rules the client
out as the source of a 1.9 s request tail.
* perf(metrics): record a phase once, not into a histogram and a counter
`record_recall_phase` and `record_validator_phase` each wrote the same measurement twice:
into a duration histogram, and into a parallel counter of invocations. The histogram
already carries `_count` for the identical attribute set, so the counter was a second copy
of a number that was never missing.
It is not free. A py-spy profile of the API under recall load put OpenTelemetry's
`consume_measurement` at 8.9% of the process's busy CPU, and `record_recall_phase` alone
at 5.35% -- the aggregation path, not the record call, is the cost, and it ran twice.
`hindsight.recall.phase.calls` and `hindsight.validator.phase.calls` are removed rather
than left emitting: a metric that exists and is never written is worse than one that is
gone. Use `hindsight_recall_phase_duration_seconds_count`, which has the same value.
Also adds `HINDSIGHT_API_RECALL_DIAGNOSTIC_PHASES=false` to drop the subset phases, which
are the bulk of the instruments on a busy path and are only wanted while diagnosing.
test_profiling sliced the histogram's argument block to the counter that follows it; the
delimiter moves to the next instrument.
* perf(api): a tunable gzip floor, and no TLS setup for a plaintext embedder
Two things a CPU profile of a recall-heavy API found, neither of which buys anything on a
deployment that is CPU-bound rather than bandwidth-bound:
GZipMiddleware compressed every response over 1 KB, which a recall always is. That was
~5% of the request's CPU. `HINDSIGHT_API_GZIP_MIN_SIZE` raises the floor, and a negative
value drops the middleware. Default is unchanged at 1024.
`httpx.Client()` builds an SSLContext and loads the system CA bundle whatever the scheme,
and an in-cluster TEI is plain http://. `ssl.load_default_certs` showed up in the profile
for exactly that, paid again for each thread the pool retires and recreates. Skipped when
the base URL is http://; an https:// TEI verifies exactly as before.
* style: ruff format the recall phase timers
verify-generated-files runs the lint hook and then fails on any resulting diff; these four
files were committed unformatted. Formatting only, no behaviour change.
* review(recall-perf): route the new knobs through config, tidy the timers, add tests
- HINDSIGHT_API_GZIP_MIN_SIZE / RECALL_DIAGNOSTIC_PHASES / LOOP_LAG_REPORT_SECONDS
(renamed from LOOP_LAG) are HindsightConfig fields now, documented and in .env.example,
instead of ad-hoc os.environ reads.
- get_request_context: the timestamp line sat above the docstring, which demoted it
to a no-op string.
- _bind_bank_id decides once per function whether to time the recall body, and no
longer swallows exceptions from the metrics call.
- Reuse semaphore_wait_start / backend_acquire_start instead of parallel timers.
- loop_lag: keep a strong reference to the probe task, drop the noqa lambda.
- Tests: diagnostic-phase flag, TEI verify for http vs https, loop-lag probe.
- ruff format (the verify-generated-files failure).
|
||
|
Nicolò Boschi
|
d11371c2ba |
perf(db): skip asyncpg's release-time reset (#4259)
The pool releases a connection without asyncpg's reset query. That query runs on every release and cost ~1.9% of a recall-heavy API's busy CPU in a py-spy profile (the reset and release frames). Measured on a CPU-bound 2 vCPU API pod: 9.8 ms of CPU per request instead of 10.5, and a ceiling of 182 rps instead of 175. Safety depends on what the reset was cleaning up. Its first statement is `SELECT pg_advisory_unlock_all()`, so a session-scoped advisory lock taken on a pooled connection would now outlive its holder. Core has one session-scoped lock: the migration runner's `pg_try_advisory_lock`. It runs on SQLAlchemy's own connection, not this pool, and releases explicitly. Extensions that take session-scoped locks on the pool must release them explicitly or use transaction-scoped locks. `db_session_setup_on_acquire` stays on by default, because a transaction-mode pooler still needs session settings re-applied on every acquire. |
||
|
Nicolò Boschi
|
4a01f97ed7 |
test(retain): make the post-commit convergence test causal, not a wall-clock race (#4294)
test_staggered_partial_overlap_retains_avoid_redundant_extraction fired ten same-document retains 0.25s apart and asserted that fewer than ten extracted. That assumes a retain commits inside the 0.25s gap. On a slow runner none had committed before the last request started, every request extracted, and the test failed with `got 10/10` — twice in a row on the #4278 CI run while green locally and on other PRs the same day. Await request 0 to completion instead, so the remaining nine provably start against the committed state, and assert the exact count (1) rather than `< n`: with the ordering causal, nothing here is timing-dependent, and a range would only hide a regression that re-extracts for some of the later writers. Mutation-checked: disabling the pre-extraction freshness recheck in orchestrator.py leaves BOTH the old and the new test green, so the recheck was never covered by this test and the rewrite gives up no coverage. What it does guard — later writers taking the delta no-change path instead of re-running the LLM per request — is now asserted exactly. |
||
|
Nicolò Boschi
|
729b1120c4 |
fix(coding-agents): repair Grok config.toml left with an unmarked Hindsight block (#4295) (#4302)
grok-build install only stripped the marker-delimited block, so an unmarked [mcp_servers.hindsight] + hook tables from an earlier config survived and the fresh block redefined them. TOML rejects duplicate tables, Grok could not parse the file, and every MCP server was silently disabled. - detect leftover Hindsight entries from the parsed document (smol-toml), with a textual fallback when the file is already broken - strip unmarked [mcp_servers.hindsight] and our [[hooks.*]] entries textually, keeping the user's comments and formatting - validate the composed config parses before writing; refuse otherwise - strip every marked block, not just the first - smol-toml is inlined into the bundle like jsonc-parser |
||
|
Nicolò Boschi
|
64a98aa56c |
fix(entities): give mention_count back when the mentions go (#4291) (#4296)
* fix(entities): give mention_count back when the mentions go (#4291) `entities.mention_count` was incremented once per mention at retain time and never decremented. Replacing a document, deleting one, or invalidating a memory left the counter where it was, so an entity's count tracked how often its documents had been rewritten rather than how many facts mention it. The coding-agents integration re-retains a conversation under the same document_id every turn, so a long session inflated every entity in it by roughly the number of turns. That counter is not internal bookkeeping: it is returned by list_entities, orders that listing, and sizes and colours the graph nodes, so an inflated entity outranks genuinely well-attested ones. Every unlink path — document replace/delete, memory delete, invalidation, an edit that rewrites the entity set — already funnels through enqueue_entity_prune_candidates, called inside the triggering transaction and before the delete fires, which is exactly where the postings are still countable. The release goes there rather than at the six call sites: it subtracts, per entity, the unit_entities rows about to go, flooring at zero. Deriving the count at read time instead would make list_entities aggregate over the bank's whole unit_entities to order a single page — the O(bank) query shape that broke the entity prune in #3222. Reverting an invalidation re-posts the entities that still exist, so it adds the mention back for exactly those; the two errors used to cancel, and fixing only the delete side would have turned a stale-high counter into a stale-low one. Both dialects implement the two new ops, taking the `entities` row locks in ascending id order — the order retain's upsert and the orphan prune take them — so a delete cannot cycle against a concurrent retain. * refactor(entities): make the posting retire and restore one statement each The release ran as a second statement next to the enqueue, reading the same unit_entities rows twice, and the revert took three statements to decide, write and credit its postings. Both are one operation on one read now. `release_entity_postings` replaces `enqueue_entity_maintenance` plus the separate release: on PG a single statement whose CTEs queue the candidates and debit the counts from one aggregate scan; on Oracle one GROUP BY read feeding both executemany calls, since MERGE has no RETURNING to hang the second write off. The lock protocol is what makes the merge safe, and it is now stated as an invariant rather than a statement order: an entity's queue row is locked before its own `entities` row, and queue rows are locked ascending. The `victims` CTE referencing `enqueued` enforces the first half, the inner ORDER BY the second, and both hold under every plan shape — EXPLAIN picks a lazily pulled semi join here, where the two lock sets interleave per entity rather than completing one before the other. `restore_entity_postings` likewise folds the survivor lookup, the posting insert and the credit into one statement, so a mention can only be credited for a posting that was actually written. * docs(entities): correct a stale op reference and state the Oracle credit assumption |
||
|
Nicolò Boschi
|
4af316e749 |
feat(control-plane): pin the current bank to the top of the bank selector (#4298)
The selector rendered banks in server order (last write descending), so the bank you were already in could sit anywhere in a long list — or on a page that had not been fetched yet. Hoist it to the first row and mark it visually (accent background, inset primary ring, primary check, bold name) so the dropdown opens on where you are. The rest keep their relative server order, and a bank that has not been paged in cannot be hoisted, so the list is returned unchanged in that case. |
||
|
Nicolò Boschi
|
203d95c7c6 |
fix(curation): clear the legacy event_date when occurred_start is cleared (#4278)
`update_memory_unit` derived the legacy `event_date` with `new_occ_start or live.event_date`, which cannot tell "cleared" from "omitted": clearing occurred_start parses to None and falls through to the old value, so the memory keeps the occurrence the user just removed. Repeating the PATCH is idempotently wrong for the same reason. That column still drives temporal-link regeneration (memories/pg/graph.py) and the `date` the curation API and UI show, so the stale value is visible and keeps temporal adjacency to the old period. Key off the raw `occurred_start` parameter instead, and on an explicit edit re-derive it with retain's own rule — `occurred_start or mentioned_at` (memories/pg/writes.py) — so curation and retain agree. An omitted occurred_start is not an occurrence edit and leaves event_date untouched. Fixes #4237 |
||
|
Gowtham Sai
|
5df6398fa1 |
fix(reflect): uniquify tool_call ids so strict APIs accept the turn (#4250)
The reflect agent built its assistant message from the raw ids in result.tool_calls and emitted one tool_result per call keyed by the same raw id. When a provider or an OpenAI-compatible gateway returns two calls sharing an id -- or blanks one out -- the serialized turn carries two tool_use blocks with the same id and two tool_result blocks pointing at it, and a strict Anthropic API rejects the whole request with 'each tool_use must have a single result'. Pick the wire ids once, positionally, before serialising, and reuse that list for both the assistant tool_calls and the tool_result messages so the two stay one-to-one. Only colliding or empty ids are rewritten, so a conforming provider keeps the ids it minted. Uniqueness spans the whole reflect loop rather than a single batch: the loop serialises into one request, and a gateway that blanks an id blanks it on every turn, so per-batch deduping would mint the same replacement twice and reintroduce the collision. The premature-done guardrail, which loops by construction, routes through the same helper instead of writing the raw (possibly empty) id. The result slots are still indexed by position, never by tool_call_id -- the wire ids are a parallel positional list rather than a map. |
||
|
Nicolò Boschi
|
b5034cb690 |
test(system): blackbox system-test coverage across the epic (#4216)
* test(system): stories 02-09 for the recall surface Eight stories over the retrieval pipeline, all driven through the published client against a real server. Part of #4214. - 02 every retrieval arm runs, and fusion records what each one found. Asserted through `trace=True`: a blended final score cannot distinguish "both arms agreed" from "one arm did all the work". Also pins that `graph` returns nothing for a single-document bank, and that temporal is a scoring component rather than a fourth arm despite the docs' "four strategies". - 03 `max_tokens` spends the budget in rank order. The middle case looks wrong and is the point: at a tight budget the *second*-ranked fact comes back, because the top one does not fit and must skip only itself (#3688). - 04 `budget` and `max_tokens` are independent dials — depth asserted via the numeric budget in the trace, since any corpus small enough to read finds everything at every level. - 05 the `assistant` -> `experience` rename between the extraction schema and the read model, which nothing in either schema hints at. - 06 tag scoping, and that fuzzy matching is opt-in via a `tag_groups` leaf rather than the plain `tags` parameter. Includes the short-word cliff: the same single transposition clears the 0.45 trigram floor in `typescript` (0.467) and misses it in `music` (0.333). - 07 the two clocks. `fact_kind="event"` is load-bearing and silent — a date on a `conversation` fact is discarded with no error — and recency decays from the event date, which `query_timestamp` re-anchors. - 08 an absurd date must not deny the whole bank: a future-dated fact makes `days_ago` negative, and the unclamped exponential fails every recall rather than mis-ranking one row. - 09 `min_scores` floors two different things. `reranker`/`final` filter results; `semantic`/`keyword` gate their own retrieval arm, so flooring one alone removes nothing because the other arm supplies the fact back. The no-op is pinned deliberately — the parameter reads like a quality filter and is not. `payloads.Fact` gains `fact_kind` for story 07. Each story declares the consolidate step explicitly rather than sharing a helper: the suite's rule is that no LLM call is answered by default, and hiding one behind a fixture would be the first exception. * test(system): stories 10-15, documents and lifecycle - 10 a retain is a document: id, verbatim text, chunk composite id, provenance - 11 replace, including the pure-deletion case where the revision only *stops* saying something and nothing new arrives to overwrite it - 12 append accumulates instead of re-running the removal diff; re-sending a turn does not duplicate its fact - 13 delete takes only its own facts, and — the #3429 shape — two banks sharing a document_id do not take each other down - 14 reprocess is not a silent no-op: the extraction answer changes between the retain and the reprocess, so the new facts can only appear if the stored text was genuinely re-read - 15 curation: edit reaches the read path and is stamped edited_at; invalidation stops answering recalls while keeping reason and timestamp for an audit Found while writing these, filed rather than worked around: #4218 (list rows are untyped dicts while single-fetch siblings return models) — the reason this file mixes item["id"] with attribute access. * test(system): stories 20-27, observations and consolidation - 20 an observation is written *over* the facts, not instead of them, and its cited evidence resolves to facts that are still present - 22 new evidence for an existing claim merges into it (proof_count grows, one observation) rather than growing a near-duplicate sibling - 23 contradictory evidence supersedes: create + delete, so a recall never hands an agent both sides with equal confidence — while the historical *fact* survives the synthesis that summarised it - 25 clear_observations empties the derived layer and leaves every source fact, and deleting the last document behind an observation retires it too - 26 trigger_consolidation reports its operation and whether it deduplicated, so a double-fired trigger cannot quietly run twice - 27 prefer_observations drops the facts an observation superseded, and include_source_facts returns them as keyed provenance instead Restores `answers_with` to the rulebook, trimmed in #4212 for having no consumer: consolidation replies must cite source_fact_ids the server minted during the retain that triggered them, which a literal payload cannot know. * test(system): story 30, mental models, plus the machinery to drive reflect A mental-model refresh runs the full reflect loop in the worker, so the suite had to learn to drive an agentic conversation: - `tool=` matcher on rules. The reflect loop sends the same system prompt every turn and varies only the tools it offers, so the tool list is the one thing that separates one rung from the next; prompt substrings cannot. - `returns_tool_call` restored (it has consumers now), plus `calls_the_offered_tool`, which climbs whichever rung it is on. A story about what reflect concludes should not have to enumerate the ladder, or break when a rung is added. - `reflect.reflect_loop` wraps both into one call. - Two anchors, not one: the search turns and the answering turn use different system prompts, and anchoring only the first leaves the final turn unmatched. Also fixed two things in the harness that this exposed: - The miss report showed only the user message. A refresh sends the bare source query there, so the report named no anchor and showed nothing useful; it now carries the whole conversation and the tools offered. - `wait_until_settled` waited out the worker's retry backoff on an operation that had already recorded an error, turning a fast loud-miss into a 90s timeout. It now reports the error immediately, and the test server runs with worker retries off — against a deterministic stub a retry cannot change the answer. * test(system): stories 35 and 40, knowledge pages and reflect - 35 a page is a mental model with a place in a tree: both ids resolve, the tree carries the name someone browses by, search finds it, and deleting it leaves the facts. Also pins the defaults that make a page different from a bare model — delta refresh after consolidation, over observations, excluding models — the kind of thing a refactor flattens silently. - 40 reflect answers from what it searched. The second test is the load-bearing one and is written inside out: only the answering turn is scripted, so if the server could ever reach an answer without searching first, no search turn would arrive and the test would pass. It asserts the unscripted search turn did arrive, then clears it — the one place in the suite where an unmatched call is the subject rather than a gap. Quality is deliberately not asserted here: the stub supplies the answer text, so these cover the mechanism. Judging what the model actually says needs a real model and stays with the hs_llm_core judge tests. * test(system): stories 43, 50 and 80 — directives, bank config, isolation - 43 a directive is only real if its text reaches the model. Storing, listing and returning it prove nothing; these assert it arrives in the reflect prompt, under the MANDATORY heading (delivered as a rule, not as context), that a bank without directives ships no such section, and that deleting one stops it being sent. Whether the model obeys stays with the judge tests. - 50 config updates are additive. The clobber failure is invisible — no error, the bank just reverts every other field to default — so the assertions are about the neighbours, not the field that changed. - 80 bank isolation across all four verbs, on two banks that deliberately share a document_id (#3429). Read, count, update and delete are separate statements with separate predicates, so getting three right proves nothing about the fourth; each is checked on its own. Restores LLMStub.calls plus prompts_for(step): prompt assembly is deterministic even where the model's reading of it is not, so 'was the directive sent' can be asserted directly instead of judged. * test(system): stories 70 and 90 — entities/graph and multilingual - 70 entities are what stitch documents together. Two documents sharing only 'Alice' merge into one entity, and a query for a word appearing in just one of them reaches the other purely through that link. This is the traversal story 02 could not exercise: remove it and the fact vanishes while everything else still passes. - 90 non-Latin content round-trips byte-for-byte, entities keep their own names, mixed-script text keeps its embedded Latin names, and a Latin query reaches the Chinese sentence containing one. Emoji cover the astral-plane case. Story 90 first failed on the *stub*, not the product: the lexical embedder tokenised `[a-z0-9]+` only, so Chinese text produced no tokens at all and both query and memory collapsed onto the same fallback vector. A test double that cannot represent CJK cannot test CJK, so the tokeniser now emits one token per CJK codepoint — roughly what a real analyser does at the unigram level. ASCII tokenisation is unchanged, so the pinned scores in stories 01 and 09 still hold. * test(system): story 60, async retain and idempotent operations An async retain returns a receipt and moves the whole 'did it land?' question onto the operation record, so the record has to answer it alone: a terminal status, and result metadata saying what was stored — 'completed' on its own cannot distinguish work done from work skipped. The idempotency tests are the load-bearing ones. A caller retries when a request times out and cannot tell a lost request from a slow one; without a caller-supplied operation_id the safe retry does not exist. Both directions are pinned: the same id replayed after completion stores one memory, not two, and two different ids over identical content stay two operations — deduplicating on content instead of the caller's id would be its own silent data loss. * test(system): code-review fixes in the harness - _slots returned a two-item tuple, which the project bans outright; it is a _Slots dataclass now. Introduced in #4212 and missed by that review. - returns_tool_call has no consumers again — calls_the_offered_tool covers every reflect turn — so it comes back out. Second time this method has been added and removed; the rule holding is that surface ships when something uses it. * test(system): story 21, consolidation failure and recovery Consolidation is the one background job that both reads and deletes, so a failure partway through is the most dangerous moment in the system. The correct behaviour turns out to be boring, which is the point: a model returning nonsense loses the synthesis and not one fact, the round completes rather than wedging the bank, and failed_consolidation surfaces the backlog. Recovery is explicit and worth writing down: trigger_consolidation does NOT pick failed facts back up — they stay claimed, so a scheduled round will not re-feed a poison input forever — and recover_consolidation is the deliberate door. Test-env changes this needed, each because a production default is noise for a single deterministic server: bank stats cached 60s (a test asserting on a counter reads a value from before its own action), and the maintenance start jitter that spreads sweeps over a minute to stop a fleet stampeding one database. Two things checked and NOT filed as bugs, both of which looked like one: failed_consolidation appearing stuck was the 60s stats cache, and a failed refresh sitting at status=pending was the worker's retry backoff. * test(system): story 52, whole-bank export and import Everything in a bank points at everything else by id, and every one of those references is minted by the source instance — so an import has to rewrite them all, in one pass, without missing a layer. Miss one and the import still succeeds: the counts are right and only the provenance is broken. That is the bug that has been fixed twice here, so the assertions chase references rather than counts, and check that an imported observation cites facts that exist in the destination. Also pinned: the derived layers are opt-in both ways, the page's backing model exists in the destination rather than naming one left behind, and the imported bank actually answers recalls (rows arriving is not the same as rebuilt indexes). The first draft failed on a test bug worth recording: the destination's own auto-consolidation runs over the imported facts like any other write, so an observation appearing there proved nothing — it might have been carried or invented locally moments later. Every import now silences the destination's consolidation first, which makes each observation necessarily an imported one. * test(system): stories 31, 32 and 64 — staleness, refresh safety, webhooks - 31 the watermark that makes 'always current' work. Both failure directions matter: a watermark that never advances refreshes forever at full cost, and a staleness check that never fires leaves a model quietly frozen while still answering confidently. Also pins that going stale does not blank the answer. - 32 the two wipe guards. A refresh whose scope matches nothing must keep the answer it has — writing 'nothing retrieved' through as 'no content' destroys work the bank cannot re-derive. And a dry run touches neither content, watermarks, nor history, or it is not a dry run. - 64 webhooks, asserted against a real receiver rather than the server's own delivery log, which only proves it tried. The signature is recomputed the way a receiver would — a signature over the wrong bytes is a header that looks right and verifies nowhere. Both SSRF cases from GHSA-ggrr-69wp-fj54 are pinned as security properties, refused at registration. The stub gains a webhook receiver: it is the only endpoint a hermetic test can offer. The test server allowlists exactly that host, so a webhook aimed at any other private address still fails and the guard stays genuinely under test. * test(system): stories 42, 44, 45 — disposition, structured output, tag groups - 42 disposition and mission reach the reasoning prompt. Whether a trait changes the *answer* is a question for the judge tests; that the knob is connected at all is deterministic, and its failure is silent — the API accepts the setting, returns it on read, and the agent behaves identically. Opposite dispositions are asserted to produce different prompts, which a constant string would fail. - 44 structured output is a second extraction call over the prose answer, not a constraint on it. Pins that the prose is unchanged, that nothing pays for the extra call without a schema, and — per #4230 — that a failed extraction is currently indistinguishable from an empty one. - 45 tag_groups and/or/not and nesting, on a corpus where each wrong operator returns a different non-empty set rather than nothing. Three harness bugs surfaced by giving a bank a reflect mission, all of which would have broken every reflect story the first time anyone set one: - The "reflect" anchor was the default role line, which a mission *replaces*. Re-anchored on the CRITICAL preamble, present on every reflect turn. - The final turn is a free choice among every search tool plus the finish tool, and calls_the_offered_tool always took the first — so it searched, was offered the same choice again, and never terminated. Finishing is now claimed by its own rule registered first, and carries the answer in its argument (calling it bare ends the loop with "the done tool returned no answer"). - The search rules and the prose turn share a system prompt; only the search turns carry tools. Rules can now require tools, so the ladder's rules stop swallowing the turn meant to write the answer. * test(system): stories 24, 54, 92 — observation scopes, templates/preview, chunking - 24 observation_scopes decides which observations a multi-tag memory feeds. The isolation is the point: a lesson tagged for a student and a teacher must not produce one observation belonging to neither, and consolidation's all_strict matching is what keeps one party's observations out of another's. Both documented spellings are pinned — [[]] is one global scope, [] is *zero* and falls back to combined, one character apart with no error either way. - 54 templates carry configuration and not memories, leave untouched fields unset rather than freezing today's defaults, and the prompt preview matches what a real retain actually sends. Also pins that retain_custom_instructions is only consulted in `custom` extraction mode — stored, returned on read, and silently unused otherwise. - 92 chunking: contiguous indexes, reassembly loses nothing at the seams, one extraction call per chunk (fewer means a piece was never read, more means paying twice), and the stored document stays byte-identical because it is what a reprocess re-reads. Fixed a real hole in the harness: the client was a path dependency installed non-editable, so the venv held a *copy* taken whenever it was last built. The suite had been testing a stale snapshot — preview_prompt exists in the repo's client and was simply absent from the installed one. Now editable. Two template tests are skipped rather than rewritten against raw HTTP: the import endpoint cannot be called from any SDK (#4232). Reaching around the client to make them pass would hide exactly the defect a client-driven suite exists to surface. * test(system): make the open defects fail instead of documenting them The suite was green while five filed issues sat unfixed, because it accommodated every one of them: comments saying "this returns a dict, see #4218" and then dict access; a skip on the import round trip; and — worst — a test asserting the *current* wrong answer for #4230, which would have failed the day someone fixed it and taught the next reader to delete it rather than read it. A comment is a code-review note, not a gate. These now assert the contract we want and fail until the product honours it: - #4217 the recall trace must report the caller's query_timestamp, not the moment the trace was built - #4218 list rows must be typed like their single-fetch siblings - #4221 every wrapper convenience method must have an async twin — written over the whole family, so the next one added without a twin also fails - #4230 a failed structured-output extraction must be distinguishable from an empty one - #4232 a bank template must round-trip through the SDK (the two skips removed) Deliberately not xfail: an expected-failure marker keeps the run green, so nothing forces the question, and it outlives the bug by months. They sit in one file because they are temporary — when an issue lands, its test moves into the story it belongs to and the file shrinks. When it is empty, delete it. * test(system): fold the open-defect tests back into their stories All five issues are fixed on main, so the temporary defect file has done its job and is deleted. Each contract moves to where a reader would look for it: - #4217 trace anchor -> story 07 (temporal) - #4218 typed list rows -> story 10, plus ~70 call sites across the suite that had been reading rows as dicts and now use attribute access - #4221 async wrapper parity -> a new story 51, since it is a contract about the published client rather than about the server - #4230 structured-output failure is reported -> story 44 - #4232 template round trip -> story 54 (the two skips were already removed) Each keeps a line naming the issue it came from, so the history stays readable without the file that tracked it. Note for anyone converting dict access after a typing change: only *list rows* became models. Trace payloads, score components and mental-model history entries are still plain dicts, and a blanket regex over `x["field"]` rewrites those too — it did here, and turned ten passing tests red before being walked back. * docs(review): require a system story for new user-facing capabilities The system suite only stays useful if it grows with the product, and nothing was asking for that. Adds step 6b to the code-review skill and a pointer in CLAUDE.md's Testing section so the rule is visible before code is written, not only at review. The trigger is deliberately about *composition*, not size: a change needs a story when it adds a capability someone can name, or when it makes two existing capabilities meet for the first time. That second case is the one the ~500-file api-slim suite structurally cannot cover, and where every bug this suite was built for actually lived. Step 6b also carries the review checks the suite's own conventions depend on, each learned by getting it wrong here: go through the published client (reaching around it hid that import_bank_template was uncallable from every SDK, #4232), declare every LLM call, await background work rather than disabling it, assert the whole deterministic payload, and make an unmet contract *fail* rather than be documented — neither xfail nor a test pinning today's wrong answer. Also drops `extra_env` from start_hindsight_server: a parameter no caller ever passed, found by this review. |
||
|
Nicolò Boschi
|
558017a84a |
feat(control-plane): add a pause button for constellation ambient motion (#4289)
The constellation's stars drift, pulse and twinkle continuously, which some users find distracting while reading the graph. Add a Pause/Play toggle to the view's toolbar. Motion now runs off an accumulated clock that only advances while enabled, so pausing freezes every drift/pulse/shimmer exactly where it is and resuming continues without a jump. Pan, zoom, hover and click keep working while paused. The preference is stored globally in localStorage (not per bank), so it follows the user across every constellation. |
||
|
Nicolò Boschi
|
e551cc260d | release(coding-agents): v0.5.3 integrations/coding-agents/v0.5.3 | ||
|
Nicolò Boschi
|
11e624325b |
refactor(config): make HindsightConfig the only parser of HINDSIGHT_API_* env vars (#4260)
* refactor(config): make HindsightConfig the only parser of HINDSIGHT_API_* env vars Thirty-odd call sites across the engine read HINDSIGHT_API_* out of os.environ themselves rather than off the resolved config. Two parsers for one variable is how the engine and the config drift apart: LLMProvider.from_env() had grown its own copies of the provider defaulting, the Gemini tier gating and the cache-affinity default, each carrying a comment asking the next reader not to let them disagree. Those comments are now unnecessary. Every fixed, server-level HINDSIGHT_API_* value is parsed in config.py and read as a field. Seventeen variables that worked but had no field got one, including the seven xai-oauth knobs; the five that carry secrets are registered in _CREDENTIAL_FIELDS so they stay off the API surface. Three fields become `str | None` — host, otel_service_name, xai_oauth_base_url. Each had a caller that needed to tell "the operator set this" from "this is the default" and was reading the environment a second time to find out. The default is now applied at the single point of use. requires_api_key moves to a new leaf module, engine/provider_auth.py. config.py needs it while building HindsightConfig and llm_wrapper needs the built config at import time to size its semaphores; that cycle is the reason the LLM factory had its own env parser to begin with. Both existing import paths still work. Two consequences worth knowing: * LLMProvider.from_env() now builds the full config, so an unrelated invalid setting surfaces there instead of being bypassed. The test that asserted the opposite asserts the new contract instead. * resolve_daemon_host_port() takes configured_host from its caller rather than reading HINDSIGHT_API_HOST itself. Value vocabularies are preserved exactly where they differed from _parse_boolean_env — ACCESS_LOG still accepts yes/on, XAI_OAUTH_DEBUG_HEADERS still never raises — so no working deployment turns into a start-up error. A new test walks the package AST and fails on any HINDSIGHT_API_* read outside config.py, with a short exemption list (standalone Alembic, pre-config bootstraps, the open-ended per-extension config namespaces) and a second test that fails when an exemption goes stale. tests/conftest.py resets the config cache per test: now that values are read off a cached config, a test's monkeypatch.setenv would otherwise land against whichever config the first test in that xdist worker happened to build. * fix(config): restore the DEFAULT_HOST import and keep .env authoritative Two defects from the previous commit, both caught by CI's server start rather than the suite. DEFAULT_HOST was dropped from main.py's imports during a rebase while `config.host or DEFAULT_HOST` stayed, so every entry point died with a NameError. No test caught it: each one hands _parse_cli_args a config whose host is already a string, so the fallback branch never evaluated. The new TestParseCliArgsHostDefault covers the unset-host path, and --help no longer advertises "default: None". The second is worse. HindsightConfig is cached process-wide on first build, and an entry point imports its whole module graph before main() reaches load_dotenv_for_entrypoint(). Modules reading the config at import scope (llm_wrapper sizes its semaphores there) therefore froze a config built before the .env was applied, and it stayed frozen — a discovered .env silently ignored, surfacing as "LLM API key is required" on a server that had always started. load_dotenv_for_entrypoint() now clears the cache after loading, and daemon.py's log path and poller.py's backpressure value resolve per call instead of at import. |
||
|
Nicolò Boschi
|
e1310d34f9 |
feat(api,control-plane): recall results carry the attachments behind each fact (#4277)
* feat(api,control-plane): recall results carry the attachments behind each fact A recall result reported no attachments, so an agent that recalled a fact derived from a screenshot had no way to show it. The only handle available was `include.chunks`, and going through the chunk is wrong: a chunk lists every attachment its text references, so a fact drawn from the prose beside a screenshot would be shown that screenshot as its evidence. Recall now returns `attachments[]` on each result, resolved from the per-fact edge the extractor recorded at retain time — the same edge the memory read endpoints already return. It is unconditional rather than another `include` flag: the ids live on `memory_units.attachment_ids`, so a bank that has retained no attachments pays one indexed read that resolves nothing. The Recall Analyzer renders them beneath each result, and drops the score breakdown row in favour of entity and tag chips (the shared facet chips) plus the occurred/mentioned timestamps — the per-signal scores are a retrieval debugging concern and are already in the Trace tab. Scores render at four significant digits with the exact value on hover; fixed decimals would collapse 0.001125 and 0.001004 to the same "0.001", which is why they were unrounded before. Entities are included by default because that flag gates the entity names on each result, not just the observations block. * chore: regenerate the docs-skill OpenAPI reference |
||
|
Nicolò Boschi
|
565303d913 |
docs(documents): say the tags PATCH replaces the array, and test clearing it (#4272)
* test(documents): cover clearing a document's tags with an empty array
The tags PATCH replaces the array rather than merging it, so `tags: []` is how
a caller drops every tag. Every guard on that path is written `is not None`
rather than a truthiness check so the empty list survives it, but nothing
exercised it: a regression to `if tags:` would have turned a clear into a
silent no-op and a 200.
Adds an engine test (clears the document's tags and its units', runs the same
observation-invalidation cascade, and is a no-op when repeated) and an HTTP test
(PATCH `{"tags": []}` is 200, an omitted `tags` is still 422).
* docs(documents): say that the tags PATCH replaces the array
The endpoint description and the docs page both said only that tags are
"propagated to all associated memory units", which leaves the question a caller
actually has — does sending a tag ADD it, and how do I drop one — unanswered.
The replace semantics were documented in exactly one place: two comments in the
CLI tab of the docs page, which an API or SDK user never reads.
States it where they will see it: the array replaces rather than merges, an
omitted tag is dropped, `[]` clears them all, and only an omitted FIELD is the
422. Regenerates the spec, the clients and the docs skill.
|
||
|
Chris Bartholomew
|
5c8e644f1e |
chore(deps): update vitest in the n8n integration (#4274)
Bump the vitest devDependency to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
2591eaaaf4 |
chore(deps): update vitest in the obsidian integration (#4275)
Bump the vitest devDependency to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
827073de85 |
chore(deps): update vitest in the flowise integration (#4273)
Bump the vitest devDependency to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
aaf673edc8 |
chore(deps): update vitest in the eve integration (#4270)
Bump the vitest devDependency to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
b768e03a7c |
chore(deps): update vitest in the opencode integration (#4271)
Bump the vitest devDependency to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
94d4b17bd0 |
chore(deps): update vitest in the eliza integration (#4269)
Bump the vitest devDependency to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
0388e64fc1 |
chore(deps): update vitest in the chat integration (#4268)
Bump the vitest devDependency to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
72dd8d6504 |
chore(deps): update vitest in the ai-sdk integration (#4267)
Bump the vitest devDependency to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
ce3a04f0ce |
chore(deps): update hono and vitest in the coding-agents integration (#4266)
Raise the hono override floor from >=4.12.34 to >=4.13.5, resolving to 4.13.7, and move the vitest devDependency from ^3.2.6 to ^4.1.11. The vitest change crosses a major boundary because the whole 2.1.0-4.1.10 range is affected, so 4.1.11 is the lowest available fix. |
||
|
Chris Bartholomew
|
5f3c4970da |
chore(deps): update npm dependencies in the root lockfile (#4261)
* chore(deps): update npm dependencies in the root lockfile Raise the override floors for next, svgo, sharp, js-yaml, colord and joi, and bump the vitest devDependency across the three workspaces that declare it, so the root workspace tree resolves to current versions. Resolved versions: next 16.3.4, svgo 4.1.0, sharp 0.35.4, js-yaml 4.3.2 (3.15.2 for the two v3-scoped consumers), colord 2.10.0, joi 17.13.7, vitest and @vitest/mocker 4.1.11. Every bump stays inside the existing major line. The lockfile was regenerated with npm 11. npm 10.9.2 has an overrides-plus-workspaces bug that applies an override at the hoisted root while de-hoisting fresh copies of the old version into hindsight-all-npm and hindsight-clients/typescript, which leaves the tree worse than before. npm 11 resolves each package to a single hoisted copy. It also prunes a handful of optional peerDependency entries that npm 10 records, which accounts for most of the deletions in the lockfile diff; npm ci under npm 10.9.2 still installs from the result cleanly. * chore(deps): regenerate the agent-sdk standalone lockfile to match its manifest hindsight-tools/hindsight-agent-sdk is both a root workspace and carries its own standalone package-lock.json. The previous commit raised its vitest devDependency to ^4.1.11 but regenerated only the root lockfile, which left the standalone one recording ^4.1.2 / 4.1.5 and out of sync with its own manifest. test-hindsight-agent-sdk installs from the root lockfile, so CI would not have caught the drift, but a standalone npm ci in that directory would have failed. Regenerated with npm 11; vitest and @vitest/mocker now resolve to 4.1.11 there too. |
||
|
Chris Bartholomew
|
b4443eedd9 |
chore(deps): update js-yaml in the zapier integration (#4264)
Raise the js-yaml override floor from >=4.3.1 to >=4.3.2, resolving to 4.3.2. Stays inside the existing major line. |
||
|
Chris Bartholomew
|
d175efbcfb |
chore(deps): update sharp and vitest in the cloudflare-oauth-proxy integration (#4263)
Raise the sharp override floor from >=0.35.0 to >=0.35.4 and bump the vitest devDependency from ^4.1.6 to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Both stay inside the existing major line. |
||
|
Chris Bartholomew
|
93a5a5fa38 |
chore(deps): update httpx2 and httpcore2 in the pydantic-ai integration (#4262)
Upgrade both from 2.7.0 to 2.12.0 in the pydantic-ai integration lockfile. Both arrive transitively through genai-prices, which pydantic-ai-slim depends on; neither is declared directly, and no manifest change is needed because the existing constraints already admit 2.12.0. httpx2 2.12.0 declares a new dependency on httpx2-jsfetch, which is recorded in the lockfile but gated behind `python_full_version >= '3.12' and sys_platform == 'emscripten'`. It is a Pyodide/Emscripten fetch transport and never installs on a normal platform; `uv sync --frozen` does not pull it in. |
||
|
Nicolò Boschi
|
179938a655 |
fix(retain): stop chunk ids colliding across banks (#4257)
* fix(retain): stop chunk ids colliding across banks
Chunk ids flattened (bank_id, document_id, chunk_index) with a plain
underscore join, so ('a', 'b_c') and ('a_b', 'c') both produced 'a_b_c_0'.
Both are arbitrary caller-supplied strings, and 'chunks' is keyed on the id
alone, so the second bank's retain overwrote the first bank's chunk row.
Build the id through engine/chunk_ids.py, which escapes the separator inside
each component and so is injective. Ids are unchanged where neither component
contains a separator; parsing still reads the ambiguous ids written before
this. Ids that already collide in a deployed database stay possible, so the
upsert now refuses a conflicting row owned by another bank instead of
overwriting it, and the delta delete is scoped to the bank when one is given.
Fixes #4244
* test(retain): cover legacy chunk ids on read and on update
A document stored before the id fix keeps its unescaped chunk ids — there is
no migration — so both reading it and re-retaining over it have to stay
correct. Ages a freshly retained document's rows back to the legacy shape and
asserts the addressed chunk route still resolves them, and that editing one
section replaces exactly the chunks covering it (escaped id) while every other
chunk keeps the legacy id it was stored under, one row per chunk_index.
* refactor(retain): require bank_id on the chunk delete, drop the duplicate id helper
Review follow-ups on the #4244 fix. `delete_chunks_by_ids` took bank_id as an
optional argument with a None default, so its bank predicate was applied under a
conditional — the latent shape the isolation rules warn about, even though every
caller passes one. Make it required and let both statements carry it
unconditionally.
`chunk_index_in` and `resolve_chunk_id_in` resolved the same id the same way for
every input; the caller that had the document id can compare it against the
resolved one instead, so only the latter remains.
|
||
|
Nicolò Boschi
|
1f513a6393 |
feat(coding-agents): add ZCode as a supported harness (#4240) (#4258)
feat(coding-agents): add ZCode as a supported harness (#4240) Adds ZCode (Z.ai's GLM coding agent) to the shared coding-agents package, so its users get the same knowledge-page, reflect and configuration experience as every other supported agent instead of the standalone hindsight-zcode integration. Three hook registrations, a stdio MCP server and the companion skill, all in ZCode's own CLI config and home (~/.zcode) — never the user's real Claude Code settings, even though ZCode embeds the Claude Code agent runtime and speaks its hook protocol. Config hooks ship disabled, so the installer flips hooks.enabled; uninstall removes the block again when nothing else is registered there. The one genuinely new mechanism is a per-session TURN JOURNAL (core/turn-journal.ts). Every other hook harness hands its Stop hook a file holding the whole conversation, which is what the incremental write-back needs: retainLiveSession re-reads the full transcript and the retain cursor sends only the turns added since the last write. ZCode has no such file — Stop carries the reply plus a temp, assistant-only transcript it deletes as the hook returns, and no user prompt at all. So the plugin keeps the conversation itself: the prompt hook appends the user turn, the Stop hook appends the reply, and retain then reads it exactly as any host transcript. Nothing downstream changes. Two host quirks worth knowing, both verified against the runtime rather than assumed: - hook budgets are `timeoutMs` MILLISECONDS (30000/30000/60000), like qwen-code and unlike everything else; the declared timeoutUnit makes that checkable. - registrations use ZCode's "process" argv shape, not a command string — it spawns hooks without a shell, so `node "…/zcode-hook.js"` would be looked up verbatim as one executable name and never run. `--import-conversations` is deliberately unsupported and says why: ZCode persists no session transcripts, so there is no history on disk to backfill from. Verified against real ZCode 0.16.5: all three hooks fire with correct bank derivation, the journal captures the user/assistant pair, `zcode skills list` finds the companion skill, and all 8 hindsight MCP tools reach the model. A Docker E2E (e2e/Dockerfile.zcode) asserts injection AND retention end to end against the published tarball — fuller than grok-build, factory-droid and qwen-code, which are retention-only. Its entry script merges the stub provider into the config rather than rendering it, because that file is the same one the installer owns. Guard tests, since the sibling that forgets is the one nobody tests: a family-wide check that journalPrompt and retain.journal are declared together, and that a journal harness parses neither a host transcript path nor a Stop-event reply — both would be applied on top of the journal, and ZCode's own payload carries last_assistant_message. |
||
|
Nicolò Boschi
|
8bcb4bc524 |
feat(coding-agents): refresh knowledge pages hourly and staggered by default (#4241)
* feat(coding-agents): refresh knowledge pages hourly and staggered by default Pages shipped with `refresh_after_consolidation`, so every consolidation on an actively worked repo paid one LLM synthesis per page (#3506). The default is now the hourly hashed schedule `H * * * *`: each page refreshes on its own minute of the hour, and the server skips a tick with nothing new to fold in, so an idle repo pays nothing. `pageTriggerType` defaults to "cron" and `pageTriggerCron` defaults to DEFAULT_PAGE_TRIGGER_CRON, so "cron" without an expression is no longer a broken config that falls back to auto-refresh — it is the default. `auto-refresh` stays available for repos that want pages current within the consolidation. Existing pages keep the trigger they were created with; this changes what NEW pages get. * feat(coding-agents): re-sync an existing page's refresh policy to the config The trigger drift check only compared `tags_match`, so a bank seeded before a policy change kept the trigger its pages were created with — the hourly schedule would have reached new repos only, and every already-installed plugin would have gone on paying one LLM synthesis per page per consolidation. seedPages() now compares the whole policy this plugin states (schedule and auto-refresh, not just tags_match) against the page's OWN resolved trigger — a hashed cron differs per page, so comparing the shared `H * * * *` would report drift on every page every session — and PATCHes the resolved expression rather than the literal `H`, which no server can parse. Manual needs an explicit `refresh_cron: null`: the server drops the unstated counterpart of a TRUTHY refresh field, so `refresh_after_consolidation: false` alone would leave a page firing on the cron it already had. * feat(coding-agents): re-sync the captured initiative pages too seedPages walked the five-page taxonomy only, so on a bank that had been worked for a while the migration missed most of what it was meant to move: one page per captured initiative, each stamped by captureInitiative with the same trigger. On a real bank that was 18 pages against 5 — every one still refreshing on every consolidation. The Initiatives folder is walked from the tree read seedPages already does, and only the trigger is patched: an initiative's name and source_query are written once from its title, and re-stating either would rebuild a page whose question never changed. * fix(coding-agents): scope a captured initiative page to its project, and state its budget A seeded page's query names the subject and tells the synthesizer to leave out facts about the dependencies the repo merely uses (#3476). An initiative page said only "drawn from the project's memory" — no subject at all, so on a bank several repos share it could not say which project it meant, and on any bank it had nothing to weigh a dependency's facts against. It synthesizes from the same memories as the pages that do carry the clause. max_tokens is stated for the same reason seedPages states it: left implicit the page takes whatever the server's default happens to be, which is only coincidentally PAGE_MAX_TOKENS. New pages only. The query is not re-synced onto existing initiative pages — changing source_query schedules a refresh, and rebuilding every initiative page in a bank is not worth a clause that matters far less to a page a title already scopes. |
||
|
Nicolò Boschi
|
f5b3f76a8d |
fix(reflect): say when the structured-output extraction failed (#4230) (#4248)
Reflect with a `response_schema` runs a second LLM call that reshapes the prose answer into the caller's schema. When that call errored or returned something unparseable, the bare `except` swallowed it and the caller got 200 with `structured_output: null` — indistinguishable from an answer that genuinely held nothing matching the schema. The machine-readable half, which is the reason a caller supplied a schema at all, failed invisibly: no retry signal, no alert. Returning 200 with the text answer is still right; the missing piece was saying the structured half did not happen. `StructuredOutputResult` now carries an `error`, and reflect surfaces it as a nullable `structured_output_error` on the response. Present => the extraction broke (retryable); absent with a null `structured_output` => nothing to extract. The mental-model refresh path uses the same helper and now records the reason in its `structured_output_failed` failure detail instead of only "extraction failed". |
||
|
Nicolò Boschi
|
1081a2ea4f |
fix(api): expose the bank template import request body (#4247)
`POST /v1/default/banks/{bank_id}/import` read its manifest off the raw `Request`, so
FastAPI emitted no `requestBody` for the operation and every generated SDK's
`import_bank_template` had no parameter to send the manifest in — export returned a
typed `BankTemplateManifest` with nowhere to send it, and import was reachable only by
dropping to raw HTTP.
The schema is published via `openapi_extra` rather than by declaring
`manifest: BankTemplateManifest` as a parameter, which would hand validation to FastAPI
and turn the endpoint's established 400 responses into 422s. The handler is unchanged.
All three generated clients now express the body: Python gains a required
`bank_template_manifest` argument and the `Content-Type` header, Go gains
`BankTemplateManifest()` with a nil guard, and TypeScript's `ImportBankTemplateData.body`
goes from `never` to `BankTemplateManifest`.
Declaring a body also puts the four manifest fields in scope for `cli-coverage-check`;
they are recorded as CLI-skipped because `bank import-template` takes the whole manifest
as a JSON file, so flattening it into flags would defeat the export/import round trip.
Supersedes #4238.
Fixes #4232
|
||
|
Nicolò Boschi
|
6f441b0aeb |
revert: drop free-threaded CPython 3.14 support (#4037, #4067) (#4234)
Load testing did not justify the maintenance cost of the -py3.14t target, so this removes it and the multi-loop server built on top of it. Removed outright: - `hindsight_api/_free_threading.py` and `HINDSIGHT_API_FREE_THREADING` — the guard that turned CPython's GIL-re-enable RuntimeWarning into an error. - `docker/standalone/Dockerfile.freethreaded`, `docker/freethreaded-smoke.sh`, the `test-api (free-threaded 3.14)` CI job, and the `-py3.14t` release image (including its `latest=false` carve-out in the image metadata step). - `multi_loop.py`, `HINDSIGHT_API_EVENT_LOOPS` / `--event-loops`, and `_serve_multi_loop`. Several event loops in one process is only a throughput win without the GIL; on a stock build the loops take turns, which `main.py` already warned about. Multi-loop hooks reverted with it: `run_background_tasks` on MemoryEngine and both `create_app`s, `LLMTraceRecorder.bind_loop` and its per-loop filter, and — from the #4123 follow-up — `ExtensionContext.is_primary` plus the thread-local context in `Extension`. With one loop per process the flag is permanently True and only one context is ever set, so both were dead weight on a public extension interface. `HINDSIGHT_API_MIGRATION_ISOLATION` loses its `auto` mode and now defaults to `false`. `auto` isolated only on a free-threaded interpreter, so this changes nothing for any existing deployment; `true`/`false` still force it either way. Kept, because they are real races that threads hit under the GIL too and only their rationale was free-threading-specific: the dateparser lock and the `regex>=2026.9.3` floor, one TEI HTTP client per thread, the shared bounded embeddings request pool, `bank_stats_cache`'s per-loop coalescing, and `_cross_loop.py` (still used by llm_wrapper, cross_encoder and llamacpp_llm). Their comments now stand on plain thread/loop-safety grounds. Ordinary Python 3.14 is untouched: the `build-api-python-versions` matrix still covers 3.11-3.14 and the litellm >=1.93.0 cp314 floor stays. Verified: lint.sh, ty, and the deterministic suite (8645 passed). OpenAPI and the generated clients show no drift, and the two .env.example copies stay byte-identical. |
||
|
Nicolò Boschi
|
e31a07855b |
chore: drop the self-hosted gh-stars chart and stray root screenshots
The README now embeds the official star-history.com chart, so the nicoloboschi/gh-stars workflow and the committed chart it generated are no longer needed. Also removes 10 screenshots accidentally committed to the repository root; none of them were referenced anywhere. |
||
|
Rafael Kallis
|
4bf49c5ba0 |
feat(extensions): add StaticKeysTenantExtension — env-configured per-user API keys with per-schema isolation (#3675)
* feat(extensions): add StaticKeysTenantExtension to the extensions registry Env-configured static API keys with per-user schema isolation, shipped as a standalone extension package (hindsight_ext_static_keys_tenant) following the supabase-tenant pattern: pyproject for tests, Dockerfile for image packaging, registry README entry, and developer docs pointer. Carries over the reviewed implementation: no third-party deps beyond the server, constant-time byte key comparison, fail-fast init validation (schema collisions, >63-char schema names, duplicate keys), and lowercase user-id normalization matching Postgres identifier folding. * fix(extensions): never echo API keys in config errors; derive per-key metering ids Review round 2 (nicoloboschi), must-fix #2 + inline comments: - The ValueError messages for a malformed HINDSIGHT_API_TENANT_USERS entry quoted the raw entry (user_id:api_key pair), so a misconfiguration like 'rafael:' would print the key of a nearby entry into startup logs — one paste into an issue and the key is disclosed. Errors now report the entry's index (and the user id once validated), never the key. - The duplicate-key error named the key itself; it now names the two conflicting user ids and the key's sha256-derived key_id. - _KeyEntry gains a stable, non-secret key_id (sha256 truncated to 16 hex chars), and RequestContext.api_key_id now carries it instead of a duplicate of tenant_id — metering can finally tell which of a user's keys authenticated, and errors can name a key without disclosing it. - Reworded the constant-time comment: the loop stops at the first match, so comparisons still depend on the matching key's position; harmless (invalid keys traverse the whole list) but the old text overpromised. * fix(extensions): refuse HINDSIGHT_API_TENANT_MCP_AUTH_DISABLED at startup Review round 2 (nicoloboschi), should-fix #4. On ApiKeyTenantExtension the flag downgrades one shared key to none; here it would hand unauthenticated MCP clients the base schema in a deployment built for per-user isolation. The extension now raises ValueError at init when the variable is set, and authenticate_mcp always delegates to authenticate() (no bypass). Documented in the package README's variable table. * docs(extensions): document key constraints and pre-provisioning Review round 2 (nicoloboschi), should-fix #3 + poller nit: - README states the two key-format constraints (ASCII, no comma) and why: the comma is the pair separator, and a non-ASCII key can never authenticate because header values arrive latin-1-decoded while env values are utf-8-decoded — the bytes never match, so the key would fail closed with a permanent silent 401. - Documents hindsight-admin run-db-migration as the way to pre-provision all configured tenant schemas, so the worker's idle-cycle fallback probes hit real schemas instead of raising swallowed EXISTS errors. * fix(extensions): serialize concurrent first-provision per schema Review round 2 (nicoloboschi), nit. Two concurrent first requests for the same user both saw the schema missing and both called run_migration (race inherited from supabase-tenant). A per-schema asyncio.Lock now serializes first initialization, with a re-check inside the lock so the loser of the race skips the redundant migration. Concurrent requests use distinct locks, so unrelated users never wait on each other. * ci(extensions): run static-keys-tenant tests and build its image Review round 2 (nicoloboschi), must-fix #1. The registry package had no CI coverage: its 40+ tests and its Dockerfile were never exercised on any change. Mirrors the supabase-tenant wiring exactly — a detect-changes filter and output mapping for hindsight-extensions/static-keys-tenant/**, a test-extension-static-keys-tenant job (uv sync, pytest, docker build on the latest-slim base), and the job in the report-pr-status gate. * fix(extensions): pre-encode configured keys once at init Follow-up to the constant-time comment (review round 2, inline nit): _KeyEntry now stores the compare_digest-ready bytes (utf-8/surrogateescape, the same codec bearer-token bytes are recovered with), so authenticate() encodes only the incoming key per request instead of re-encoding every configured key. Loop behavior is unchanged — bytes vs bytes, no fast path. |
||
|
Nicolò Boschi
|
7871d9bd1f | update star history | ||
|
Nicolò Boschi
|
2544a73c4c |
perf(api): replace both BaseHTTPMiddleware with pure ASGI (3.2x on cheap routes) (#4235)
* perf(api): replace both BaseHTTPMiddleware with pure ASGI
`@app.middleware("http")` installs a Starlette BaseHTTPMiddleware, which per
request spawns a child task and pipes the response through a pair of anyio
memory-object streams. The API had two of them, and on cheap routes that
machinery cost more than the endpoint.
Measured on the real API in a 2-CPU container, 32 concurrent clients driven
from 4 processes inside Docker (a single-process client and the host port
proxy both bottleneck before the server does, so neither is used):
/health/live baseline 2476 / 2393 rps p99 108 / 106 ms CPU ~98%
this PR 7917 / 7545 rps p99 17 / 20 ms CPU ~87%
3.2x throughput and an 82% lower p99, at lower CPU. Recall is unchanged
(39/27 rps before, 35/40 after -- fully overlapping): it is CPU-bound at
~23ms of Python per request and nowhere near the middleware's ceiling.
Nothing is dropped. Both jobs move rather than go away:
* HTTP metrics -> `HttpObservabilityMiddleware`, a pure-ASGI middleware that
wraps `send` to read the status. Same metrics, no task hop.
* Unknown-param reporting -> `UnknownParamsRoute`, an APIRoute subclass. It
runs after routing, which removes everything the old version did per
request to re-derive what the router already knew: two walks of
`app.routes` calling `route.matches()`, an uncached `inspect.signature`,
and a second `json.loads` of the whole body. Known names now come from
FastAPI's own `dependant` at startup, and the body is read via the same
`Request` FastAPI uses, whose `json()` caches -- so it is parsed once.
The route hands the names to the middleware through the ASGI scope rather
than setting the header itself, so `X-Ignored-Params` still appears on error
responses; a route handler cannot add a header to a response built by an
exception handler above it.
tests/test_unknown_params.py previously defined its own inline copy of the
old middleware and so passed regardless of what shipped. It now builds the
app the way create_app does and drives the real classes; the cases are
unchanged, and they caught a real bug during this work (FastAPI's pydantic-v2
ModelField exposes the annotation as `field_info.annotation`, not `type_`).
* fix(api): keep unknown-param reporting on extension routes, sanitise the header
Two follow-ups on the pure-ASGI rewrite:
- Routes contributed via include_router keep their source class, so the
extension router's routes arrived as plain APIRoute and silently lost the
reporting the old middleware gave them. adopt_included_routes() re-classes
them after the include.
- The names in X-Ignored-Params are percent-decoded client input: a non-latin-1
one raised UnicodeEncodeError inside send (a 500 from a typo'd query param)
and one carrying CR/LF would have split the response. Sanitise to printable
ASCII.
Also bans BaseHTTPMiddleware in the code-review skill, with the pure-ASGI /
APIRoute alternatives.
* test(api): drop the always-true scope-key assertion
* fix(api): adopt the route class on the source router, not after the include
FastAPI 0.141 (what the free-threaded job resolves; the pinned env is 0.136)
rewrote include_router to keep the included router lazily and materialise its
routes later from the source router's route_class, so re-classing app.routes
after the include found nothing and the extension routes lost the header again.
Do it on the source router instead, covering both resolutions: set
route_class (>= 0.141) and re-class the already-built route objects
(<= 0.140). Verified against both versions.
|
||
|
github-actions[bot]
|
d2120b88b8 | chore: update star history | ||
|
Nicolò Boschi
|
134207d3c3 |
fix(retain): a store-owned retain must not fail on its own log line (#4236)
`retain` on the memories seam returns a mapping, but both store-owned write paths formatted their log line with `resp.seq` / `resp.new_entities`. Against a real store-owned backend that raises `AttributeError: 'dict' object has no attribute 'seq'` from inside the log call, and because the write has already been committed at that point, the caller gets a 500 for a retain that actually succeeded. Observed as a steady stream of failed retains under load while the data was being written correctly. The doubles are why this passed CI. `tests/test_retain_store_owned_no_connection.py` returned `SimpleNamespace(seq=3, new_entities=1)` from its fake `retain` — an attribute object, the one shape no real implementation produces. A double that is easier to satisfy than the contract tests the double rather than the code, so both call sites were exercised on every run and neither could fail. - both log lines read the mapping (`resp.get(...)`), so a missing key cannot fail a write either - the doubles return the documented mapping, which makes these tests catch the bug: reverting either call site now fails at that exact line - the interface pins the return value, which was previously undeclared — that is what let the two sides disagree without either looking wrong |
||
|
Nicolò Boschi
|
ef3ccdba3a |
fix(api): type the list and graph rows instead of returning bare dicts (#4218) (#4233)
* fix(api): type the list and graph rows instead of returning bare dicts (#4218) `list_memories`, `list_documents`, `get_graph` and `get_entity_graph` declared their rows as `dict[str, Any]`, so every generated SDK handed callers untyped dicts while the single-fetch siblings returned real models — `listing.items[0].id` failed with an `AttributeError` and a server-side rename became a runtime `KeyError` rather than a build error. Each row now has a model (`DocumentListItem`, `MemoryUnitListItem`, the Cytoscape node/edge envelopes and `MemoryGraphTableRow`), sharing an `OpenRowModel` base that keeps the wire byte-identical: - `extra="allow"`, so a key the server emits and the model does not declare still reaches the client — a memories store that owns its own document or entity registry builds these rows itself. - the routes keep emitting nulls. `ExcludeNoneRoute` was already enabling `response_model_exclude_none` for them, but `exclude_none` never reached inside a `dict` value, so the rows' nulls were always on the wire; typing them would have started dropping those keys. `additionalProperties` is stripped from the published schema: openapi-generator 7.10.0's Python generator crashes on a schema pairing it with a nullable `anyOf` property, which every row here has. The CLI moves to attribute access, which exposes a latent bug in `bank graph`: its node lookups read `node["type"]`/`node["id"]` through the Cytoscape `data` envelope, so the sample always printed "unknown [unknown]" with no text. * docs(examples): read list rows by attribute now that they are typed |
||
|
Chris Bartholomew
|
fb94ce0341 |
feat(mental-models): default list to metadata; MCP list returns metadata only (#4225)
* feat(mental-models): default list to metadata; MCP list returns metadata only Listing mental models defaulted to returning every model's full synthesized content (and reflect_response). That bloats a caller's context and lets a single list call pull an entire bank's synthesized knowledge in bulk, when the intended way to read a model's content is the single-model read. - MCP list_mental_models tool: returns metadata only (id, name, tags, staleness); the `detail` parameter is removed. An agent discovers models here and reads a specific model's content with get_mental_model. - HTTP GET .../mental-models: `detail` now defaults to `metadata` instead of `full`. Content stays available opt-in via `detail=content`/`full`, and when requested it is delivered and metered the same as a single-model read. - Engine list_mental_models is unchanged and still honors `detail` for internal callers (bank-template export/import need full content). - Regenerated OpenAPI + clients (Python/TypeScript/Go). Tests: the MCP tool is metadata-only with no `detail` param; the HTTP list defaults to metadata and returns content only when detail=content is passed; is_stale is still reported per model on the list. * fix(mental-models): follow through on the list default flip in every caller Flipping the list endpoint's `detail` default from `full` to `metadata` left the callers that were relying on the old default reading nulls. - Control plane: `MentalModelsView` now asks for `detail=content` — it renders the content preview, source query and trigger chips, and seeds the update dialog from the listed row, so metadata alone crashed the search filter (`m.source_query.toLowerCase()` on null) and would have clobbered every trigger setting on save. The search filter is null-guarded too. - CLI: `hindsight mental-model list` asks for `content` (`--verbose` → `full`), restoring the per-row preview and keeping `--output json` useful to scripts. - Docs: the detail-levels table said `full (default)` for both endpoints and showed a `detail` argument on the `list_mental_models` MCP tool that no longer exists; the three SDK list examples printed `source_query` off a default list. Added an upgrade note. - Wrapper clients: the Python docstring still promised a server-side `full` default; the TS one said nothing. - Dropped the "metered the same as a single-model read" claim from the endpoint docstring — a `detail=content` list still validates as one `LIST_MENTAL_MODELS` bank read, not one read per model. * fix(hindsight-all): let the facade ask for mental-model content `mental_models.list()` in both facade paths (the client wrapper and the embedded namespaces) forwarded no `detail`, so after the list default flipped to metadata a hindsight-all caller got content-free rows with no way to ask for more — the one wrapper where the capability was not just defaulted away but unreachable. Forwards `detail` like the TypeScript and Python wrappers do. --------- Co-authored-by: Nicolò Boschi <boschi1997@gmail.com> |
||
|
Nicolò Boschi
|
f16e515f5e |
perf(recall): stop building trace payloads when no trace was asked for (#4231)
* perf(recall): stop building trace payloads when no trace was asked for
`SearchTracer` is constructed for every recall so the `[phases]` accounting
always has somewhere to write, and `phases_only` suppresses everything else
*inside* the tracer. Three call sites still guarded on `if tracer:` and so
built their payloads eagerly in the CALLER before handing them over, where
they were immediately dropped:
- `[(r.id, r.__dict__) for r in results]` per retrieval arm, per fact type
- `[(mc.id, mc.retrieval.__dict__, {...}) for mc in merged_candidates]`,
twice (RRF merge and again after reranking)
- `[sr.to_dict() for sr in scored_results]`
A py-spy profile of a recall under concurrent load put those list
comprehensions at 11% of all non-idle samples -- the single largest item,
and entirely wasted work on the normal `trace=false` path.
Gate the payload construction on `enable_trace`, which is the same condition
`phases_only` encodes. The `add_phase_metric` calls stay unconditional, so
phase timings are unaffected; they move out of the guarded blocks rather than
inside them. The entry-point hydration fetch keeps its behaviour too -- its
`phases_only` check is now implied by the enclosing guard.
This is the same fix already applied at the `finalize()` call site and to the
entry-point fetch, both of which carry the note "`enable_trace`, NOT
`if tracer`: the tracer now always exists".
* perf(recall): drop the last eager trace payload and make the guard a rule
The visit_node loop still ran on every recall: it walks every scored
result, builds the kwargs, and visit_node throws them away under
phases_only. Same shape as the three payload builds already fixed here.
Guard it on enable_trace, and remove the trap that keeps producing this
bug: the tracer is always constructed, so 'if tracer:' is always true.
No call site guards on it any more -- phase metrics run unguarded (that
is why the tracer is unconditional), payload builds sit behind
enable_trace -- and an AST check in
tests/test_recall_tracer_payload_gating.py fails on a new one.
|
||
|
Nicolò Boschi
|
2bf10435d5 |
fix(python-client): add the missing async twins to the convenience wrapper (#4221) (#4228)
* fix(python-client): add the missing async twins to the convenience wrapper (#4221) The class docstring promises an `a`-prefixed variant for every convenience method, but 27 of them had none. That is not just inconvenient: the sync methods go through `_run_async` -> `loop.run_until_complete`, which raises `RuntimeError: This event loop is already running` inside a live loop. So mental models, knowledge pages, directives and bank config were unreachable through the wrapper from exactly the contexts the docstring points at (FastAPI, LangGraph, CrewAI), and callers had to drop to the generated SDK for a whole feature area. Each of the 27 now has its implementation on `a<name>` with the sync method forwarding to it via `_run_async`, so there is one body per operation rather than two that can drift. `tests/test_async_sync_parity.py` guards the family: every public convenience method must have an async twin, the twins must actually be sync/async, and their signatures must match argument for argument (a twin that quietly drops a parameter is the #2975/#3042 failure mode). Plus the issue's repro as a regression test — the async twin called from inside a running event loop. * fix(dev): read the bank-config updates dict from aupdate_bank_config The client-coverage check anchored on the sync update_bank_config to find the enumerated updates dict. That body now lives on the async twin, with the sync method forwarding to it, so the check saw a forwarder and reported all 48 fields as accepted-but-never-forwarded. |
||
|
Nicolò Boschi
|
1dd7cf2a93 | release(coding-agents): v0.5.2 integrations/coding-agents/v0.5.2 | ||
|
Xiaoping Liao
|
511c86e10c |
fix: defer retain completion outbox until store commit (#4203)
* fix: defer retain completion outbox until store commit Queue reached callbacks while a retain session buffers memories, then publish document counts after its successful commit. Cover document factories, unchanged and zero-fact retains, and commit failures. * fix(retain): do not fail a committed retain when the deferred outbox write fails The deferred completion outbox now runs after the store session has committed, so a failure there cannot be undone by failing the retain — it would only report a stored document as lost and invite a duplicate re-submit. Log the dropped retain.completed event and let the retain report the truth. --------- Co-authored-by: Nicolò Boschi <boschi1997@gmail.com> |
||
|
Chris Bartholomew
|
33e986cf96 |
docs: point the Slack community links at a redirect instead of a raw invite token (#4223)
The Slack invite token was pasted verbatim into twelve places across ten files here. Slack shared-invite links expire — on the plan this workspace uses they are capped at 30 days, with no "never expires" option — so every one of those twelve copies had gone stale. An expired Slack invite gives no useful signal. It quietly redirects to the workspace's generic signup page, which is restricted to a company email domain and tells the visitor to contact an administrator for an invitation. It reads like the community is closed, or like a permissions bug. Two people reported it that way this week before anyone realised the link was simply dead. Rotating a token across twelve hardcoded copies every 30 days was never going to happen, which is how it got this stale. Four of the copies are frozen versioned sidebars (0.6 through 0.9) that nobody thinks to grep. They are updated here too: a reader on old docs deserves a working link as much as anyone. Everything now points at https://vectorize.io/slack, which redirects to the current invite. Rotating becomes a one-line change in one repository, and the links in this one stop going stale. Claude-Session: https://claude.ai/code/session_011mnzArjiCdBxa8dh1H47Fa |
||
|
Nicolò Boschi
|
fda9697773 |
fix(retain): give a split append the whole document as its body, not just the tail (#3989) (#4229)
An oversized item is sliced into sub-batches, and each slice reports the document it belongs to so `documents.original_text` stores the complete payload rather than one slice (#1838). For a replace the item IS the document, so the slice reported itself and that was right. For an APPEND the item is only the new tail — `retain_batch` prepends the stored body afterwards — so every slice reported the tail as the whole document and the stored body was truncated to it. The facts survive that append; the earlier chunks are still committed. They do not survive the NEXT one, which prepends the truncated body, diffs it against the stored chunks, finds the ones whose text is no longer in the body, and tombstones their facts. So a document's fact count goes UP and then DOWN while its content only ever grew — the signature reported in #3989, where a session transcript grew 212,335 -> 267,311 characters as its facts fell 197 -> 131. It was attributed to the coding-agent's client-side replace fallback; the loss is here, on the ordinary append path, silent because the chunks come from the real content so extraction still looks correct. The splitter now reports `append_document_body(base, tail)` for an append. The base is read once, hoisted above the splitter from the block that already read it for `append_prepend_chunks`. Three changes so the class is harder to reintroduce: - ONE definition of how a document's parts become its body. The JSON-array merge (#2409) had a second copy inside the prepend; both now call `merge_json_array_parts`, so the body the splitter PREDICTS and the body `retain_batch` BUILDS cannot disagree. - An append is monotonic, and `assert_append_extends_stored_body` now enforces it where the prediction and the stored base are both already in hand. `AppendWouldTruncateDocument` is raised, not logged: a failed append is recoverable (the caller resubmits, retain is idempotent by operation_id), a truncating one is not. - `document_body_override` -> `full_document_body`. The old name read as "override the body with this item's text", which is exactly the wrong model for an append and exactly the mistake that was made. Delta's oversized-replacement safety valve now takes `delta_full_body` (None for an append) rather than the body being written: an append satisfies "strictly appends the stored source" by construction now, and that branch preserves historical chunks without extracting the tail. No test could reach it either way; binding it to the narrower value keeps its reachability unchanged rather than resting on that. Fixes #3989. |
||
|
Nicolò Boschi
|
e366ff407e |
fix(recall): report the caller's query_timestamp in the search trace (#4227)
`trace.query.timestamp` was `datetime.now(UTC)` at finalize time, so a recall anchored with `query_timestamp` reported today's date even though the anchor had been applied to recency scoring. Anyone debugging a ranking read the field and concluded their anchor was ignored. The tracer now takes the resolved anchor (`_recall_scoring_now(question_date)`, the same value the scoring uses) and records it, falling back to now when the caller supplied none. Fixes #4217 |
||
|
Nicolò Boschi
|
da0444a72a |
feat(profiling): env-configured CPU profile, reported to the logs (#4215)
* feat(profiling): env-configured CPU profile, reported to the logs
Answers "what is burning the CPU?" for a process you cannot attach a debugger to.
HINDSIGHT_API_PROFILE holds JSON -- the shape config.py already uses for structured env
config -- and unset, nothing starts:
HINDSIGHT_API_PROFILE='{"every": 60, "top": 20}'
It reports to the log stream rather than a file or an endpoint, because the case it
exists for is a process dying without explanation: a file inside the container dies with
the container unless a volume was mounted in advance, and an endpoint needs a live
process and a route to it. Container runtimes keep the previous container's stdout, so
the last report before a crash is still readable afterwards. Each report is flushed as it
is written, since a fatal signal takes buffered output with it.
Three things the implementation had to work around, all verified on a running API:
* The profiler is process-wide and single-instance. Since 3.12 it is a global
monitoring tool, so enable() covers every thread whatever thread calls it, and a
second concurrent profiler raises `tool 2 is already in use`. Per-thread profilers
are not possible; this arms one for the process.
* Snapshots must use getstats(), which reads the accumulated entries without stopping
the profiler. Snapshot-and-clear through pstats disables the global tool, and every
report after the first then silently contains nothing. Reports are deltas between
snapshots, and a test asserts a second window still has data.
* Sampling via sys._current_frames() is not an alternative on a free-threaded build: it
stops the world, so it catches threads parked at safe points, which are I/O waits. It
reported event-loop threads idle in selectors.select while /proc showed those same
threads at 50-65% of a core. Every report therefore carries per-thread CPU from
/proc, which profiler overhead cannot distort, as the arbiter.
py-spy remains the better tool on a GIL build. It cannot read a Py_GIL_DISABLED process
at all -- it locates threads through the GIL -- which is what left free-threaded
deployments with nothing, and is why this exists.
* fix(profiling): key window baselines by label, not id(code)
CPython reuses object ids once an object is freed, and code objects are not all
long-lived -- a process compiling code at runtime frees them constantly (the profiler's
own output showed 8,684 compile() calls in one 30s window). Keyed by id(), a reused
address would subtract another function's baseline and report a nonsense delta, silently,
because the number still looks like a number.
Found reviewing the diff, not by a failure.
* chore(docs): regenerate the docs skill mirror
skills/hindsight-docs/references/ is generated from hindsight-docs/, and
verify-generated-files fails when the two diverge. Produced by
./scripts/generate-docs-skill.sh, not edited by hand.
* fix(profiling): arm in create_app too, or --workers deployments profile the supervisor
uvicorn with `--workers N` spawns worker processes that import the app and never run
main(). Arming only in main() therefore profiles the supervisor -- which does nothing but
waitpid() and ping its children -- while every request is served in a worker it cannot
see.
Found by running it against a real 2-worker deployment: 63 report lines, every one of
them supervisor bookkeeping (waitpid, is_alive, multiprocess.ping, pickling), total
tottime 0.005s, with the process serving 162 requests/s the whole time.
create_app() is what a worker does import, so arming there covers them. install() is
idempotent, so a single-process deployment that arms in both places gets one profiler.
* fix(metrics): give the recall phase histogram millisecond-scale buckets
Its unit is seconds and recall phases take milliseconds, but it was created without
explicit boundaries, so the SDK default applied: 0, 5, 10, 25, ... In seconds, that makes
the first bucket everything under five seconds, and every recall phase landed in it.
The histogram could therefore report a mean but no usable percentile. Asked for per-phase
p50/p90/p99 on a live pod it answered 2500/4500/4950 ms for all fifteen phases at once --
the interpolated midpoints of that first bucket, not measurements. A mean cannot explain a
tail, and explaining the tail is what a phase breakdown is for: a phase averaging 49 ms is
perfectly consistent with a 460 ms p99, and the histogram is what should tell them apart.
Boundaries now span 1 ms to 10 s, which covers both a sub-millisecond fuse and a pathological
store call.
|