mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
7b25993f22fe1754de7826f117c0cbe633949961
2984 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Nicolò Boschi
|
7b25993f22 |
docs: changelog and blog post for v0.10.0 (#4245)
* docs: changelog and blog post for v0.10.0 * fix(changelog): batch a release's commits and summarize them in parallel The 0.10.0 range summarized to 37 entries, against 65 for the 145-commit release before it: whole user-facing fixes were unlisted. Measured on that range, holding model and prompt fixed, one call over the whole release produces 120 entries against 152 for five batches — 40 commits get an entry only when batched, against 8 only when not. The input was never the constraint: a 245-commit release is ~8500 tokens of commit subjects in total, so every arrangement fits the context window many times over. What batching bounds is how many commits the model weighs at once before it starts summarizing them away. * Commits are batched by token count and the batches summarized concurrently, followed by a dedup pass that merges the same change described twice across a batch boundary. The dedup result is re-ordered by the release's own commit order and filtered to commit_ids that were in the input, so a dropped or invented id cannot reorder the changelog or point a reader at a commit that isn't there. * max_completion_tokens=16000 was a *shared* budget on a reasoning model — reasoning bills against it, so a long commit list spends most of it before the first entry is emitted. Worth 8 entries on its own (37 -> 45). Raised to 100000. * The grouping rule read as licence to merge unrelated fixes that touch the same area, and "focus on user-facing changes" as licence to drop anything whose commit title sounds internal. Both are now scoped; this was the largest single effect (45 -> 126). Default model is now gpt-5.6-terra. |
||
|
Nicolò Boschi
|
1a0a177771 |
chore(db): drop the unused memory_units_bm25 materialized view (#4357)
Nothing ever read it — keyword retrieval queries memory_units.search_vector, which retain and consolidation keep current inline. Its only consumer was the REFRESH in hindsight-admin restore, which left a stale snapshot that looked like a recall bug (#4338). |
||
|
Nicolò Boschi
|
cdac00842b |
fix(profiling): arm the profiler in the workers, not only the supervisor (#4254)
`main()` installs the profiler before uvicorn starts, but `--workers N` makes uvicorn spawn children that re-import `hindsight_api.server:app` and never run `main()`. So on any multi-worker deployment the only armed profiler lived in the supervisor, and its report was `keep_subprocess_alive` and `ping` at 0.01 cores while the workers served every request. That is worse than having no profile, because it looks like an answer — it faithfully names the busiest function in a process that serves nothing. Arming it where the app is imported covers the worker processes, and `install()` already guards on a module-level flag and catches the ValueError cProfile raises when the process-global tool is held, so arming in both places is safe. |
||
|
Nicolò Boschi
|
5a60831751 |
fix(migrations): don't maintain memory_units or mental_models indexes under a custom memories store (#4326)
* fix(migrations): leave memory_units alone when a custom memories store owns the rows With a store-owned MemoriesExtension the memory rows never land in Postgres, yet the post-migration reconcile still resized memory_units.embedding and rebuilt its vector/text indexes. That made pgvector's 2000-dim HNSW limit fail startup for embedding models the custom store handles fine. Thread skip_memory_units (from get_memories().store_owned) through run_migrations_for_schemas into ensure_embedding_dimension, ensure_vector_extension and ensure_text_search_extension, from all three callers (engine startup, tenant provisioning, admin CLI). mental_models and the other Postgres-resident tables are still reconciled. * fix(migrations): keep mental_models.embedding unindexed under a custom memories store A store-owned MemoriesExtension answers every mental-model vector query (search_knowledge_pages / _semantic); Postgres only hydrates rows by id. The vector index on mental_models.embedding is therefore dead weight, and building it still tripped pgvector's 2000-dim HNSW limit at startup. Under a store-owned store the column still follows the model dimension (it is written) but any vector index is dropped and none is created. Renames the flag to store_owned_memories since it now covers more than memory_units. * fix(migrations): drop the mental_models BM25 index too under a custom memories store A store-owned store answers knowledge-page search, so idx_mental_models_text_search has no reader either; on native Postgres still maintains it on every page write because search_vector is a generated column. Under store_owned_memories the text-search reconcile now just drops it. Moving back to Postgres must not wedge boot: a populated table whose column is in shape but whose index is missing used to read as a backend switch and raise. Both reconciles now rebuild a missing index in place (text search: any table; vector: mental_models, whose resize path was the only thing that built it). * test(admin): let the migration fakes accept store_owned_memories |
||
|
Nicolò Boschi
|
bd6dd0d1ea |
chore(embed): re-sync the bundled env.example with the repo-root template (#4355)
#4319 added HINDSIGHT_API_LOOP_LAG_METRIC and HINDSIGHT_API_METRICS_WORKER_LABEL to .env.example without re-copying it into hindsight-embed, so test_bundled_template_matches_repo_root fails on every PR that triggers the embed jobs. |
||
|
Chris Bartholomew
|
54fc68e705 |
feat(llm): per-member timeout and retry budget in a multi-LLM chain (#4336)
* feat(llm): per-member timeout and retry budget in a multi-LLM chain
A failover chain is itself a retry: when a member fails, the next one is tried.
Retrying a non-terminal member first only delays that handoff, and when the
member is failing *because* it is saturated -- rejecting with 503, or timing
out under its own queue -- the immediate retry is near-certain to fail the same
way. Every one of those attempts holds the caller's slot.
The terminal member is the opposite case. It has nowhere to fail over to, so
its retry budget is the only thing standing between a transient error and a
failed request, and it is the member where honouring a Retry-After actually
pays.
A single operation-wide MAX_RETRIES cannot express both, and until now that was
the only knob: _member_to_llm applied the operation's resolved request defaults
to every member. Lowering it to fail over promptly also stripped the last
member's ability to ride out a rate limit.
Adds HINDSIGHT_API_<OP>LLM_<n>_TIMEOUT and _MAX_RETRIES, so a chain can be
configured to fail fast on the way down and retry only at the bottom:
HINDSIGHT_API_LLM_MAX_RETRIES=0 # primary: hand off immediately
HINDSIGHT_API_LLM_1_MAX_RETRIES=2 # last member: absorb transients
Unset means inherit, so an existing chain is unchanged -- _member_call_defaults
returns the operation's kwargs untouched when a member overrides neither field.
0 is deliberately distinct from unset: it is a meaningful setting (fail over
immediately) and is parsed as 0 rather than collapsing back to the default.
Per-member timeout matters for the same reason and is arguably the sharper
tool: a stalled member holds a slot for the full operation timeout before the
chain can move on, but lowering that timeout globally also cuts off members
whose responses are legitimately slow.
Tests cover inherit-when-unset (asserting the kwargs are identical to the
operation's, so the untouched path cannot drift), override, the two fields
being independent, explicit 0 surviving, per-op prefixes, invalid input, and
the end-to-end shape this exists for. Each override test was confirmed to fail
with the override logic removed.
* fix(retain): let each chain member own its transport retry budget
Fact extraction forwarded its operation retry budget on every call, and the
chain hands per-call kwargs to every member unchanged, so a per-call value
beat each member's HINDSIGHT_API_LLM_<n>_MAX_RETRIES. With the primary at 0
and the terminal member at 2, retain gave the terminal member 0 retries.
The retain LLM is already built with that budget as its default, so dropping
the per-call value keeps single-LLM behaviour and lets member overrides apply.
The malformed-JSON re-prompt loop still counts from the operation budget.
---------
Co-authored-by: Nicolò Boschi <boschi1997@gmail.com>
|
||
|
Nicolò Boschi
|
6ac46e2307 |
feat(api,clients): admission control with a bounded wait, and client retry (#4253)
* feat(api,clients): admission control with a bounded wait, and client retry The engine already caps concurrent work (`recall_max_concurrent` and friends), but `async with semaphore` is backpressure, not admission control: it bounds how much runs and lets an unbounded queue form behind it. Measured on a 2-vCPU container, 1024 concurrent recalls against a 32-permit semaphore produced a 12.8s p50 -- the latency did not go away, it moved into the semaphore queue, and the server spent CPU on responses whose callers had long since gone. Server: per-operation lanes with a deadline ------------------------------------------- `api/admission.py` adds lanes with two numbers: how much runs concurrently, and how long a request may queue before it is refused with 503 + `Retry-After`. Enforced by an `admit_for` dependency on the same routes that carry `precheck_for`, so a refusal happens before the body is deserialised -- the cheapest point to say no, and where an extension already rejects on quota. Lanes are per operation because per-request cost spans three orders of magnitude (measured: ~0.1ms /health/live, ~0.5ms bank stats, ~23ms recall); one global cap calibrated for recall would throttle health checks 100x too hard. Only recall, reflect and retain get lanes -- the other PrecheckOperations are low-volume administrative routes, and gating them would add knobs nobody tunes. Limits are PER WORKER and derived from the CPU budget this process actually has, via the existing cgroup-aware detector (`os.cpu_count()` reports the host's cores under `--cpus`, which would size limits for a machine the process cannot use). `in_flight` is a latency target, not a capacity limit: a c=1024 sweep measured throughput flat at 40-45 rps whether the limit was 8, 16 or 24 per worker, while p50 moved 1.4s -> 2.3s. It is bounded on both sides -- too low throttles I/O-bound work (8 permits against a 500ms provider caps a worker at 16 rps), too high rebuilds the queue this exists to prevent. A queued request whose client disconnects releases its place immediately, using the token `ClientDisconnectCancellationMiddleware` already puts on the scope. That is what makes a patient 30s deadline affordable: the queue self-cleans, so waiting costs nothing when nobody is listening. Verified end to end -- a client that gave up at 1s freed its slot at 1.003s, not at the deadline. Clients: retry the idempotent calls ----------------------------------- Both maintained wrappers retry recall and reflect on 429/503. Writes are not retried: the Python wrapper documents that `operation_id` is ignored for synchronous retain, so a retry there could duplicate. Two properties matter more than the retry. `Retry-After` is honoured, because the server sends it knowing its own queue depth. And the wait is jittered -- a burst that all receive `Retry-After: 1` and obey it exactly returns in lockstep and rebuilds the spike. The generated Python client ships `ExponentialRetry`, which has neither, and was off by default; this is why it stays off. * fix(admission): decrement queued once on abandon, drop no-op lanes, regen docs skill - An abandoned waiter decremented stats.queued in its except branch and again in finally, driving the gauge negative. - admit_for on dry-run/mental-model/files routes was a no-op (no lane exists). - Stale config comments on the kill switch and reflect sizing. - HTTP-level test for 503 + Retry-After; regenerated docs skill. * chore(embed): re-sync bundled env.example with repo root |
||
|
Nicolò Boschi
|
6a37c052a1 | release(coding-agents): v0.6.1 integrations/coding-agents/v0.6.1 | ||
|
Nicolò Boschi
|
daaba65424 | fix(coding-agents): keep the reflect fallback silent, leave its details to the diag log (#4354) | ||
|
Nicolò Boschi
|
2cd0561f14 |
feat(metrics): /metrics covers every worker (labelled api_worker), and event-loop lag as a histogram (#4319)
* feat(metrics): a metrics port per worker, and event-loop lag as a histogram With --workers N every worker is its own process with its own metrics, but they share one port, so a scrape of /metrics reaches one worker at random. Counters jump between processes from one scrape to the next (a rate over them reads each switch as a reset), and process_cpu_seconds_total describes a random worker, so a worker whose event loop is saturated is invisible while the pod total still looks like headroom. - HINDSIGHT_API_METRICS_WORKER_BASE_PORT (default 0, off): each worker also serves its own registry on BASE + slot. The slot (0..N-1) is claimed with an exclusive flock on a per-slot lock file; the kernel drops it when the process exits, so a respawned worker takes over its predecessor's port. /metrics on the API port is unchanged. - HINDSIGHT_API_LOOP_LAG_METRIC (default false): the existing loop-lag probe records every sample in a hindsight.event_loop.lag histogram (seconds, with sub-second buckets), independent of its log reports. * feat(metrics): one /metrics covering every worker, labelled api_worker=<slot> Replaces the per-worker ports from the previous commit. Labelling alone would not fix the scrape: each scrape still reaches one worker, the others' series go missing from about half the scrapes, and Prometheus marks them stale. So with HINDSIGHT_API_METRICS_WORKER_LABEL on, each worker claims a slot (flock on a per-slot lock file; the kernel frees it when the process exits) and publishes a snapshot of its registry every 5 s to a directory the server's workers share. /metrics, whichever worker answers, returns every live worker's series with api_worker="<slot>": its own read live, the others from their latest snapshot, never summed. A snapshot older than 15 s is a gone worker and is skipped; a corrupt one is skipped without breaking the scrape. One port and one scrape target, so no change to charts or scrape configs. The event-loop lag histogram from the previous commit is unchanged. |
||
|
Chris Bartholomew
|
e3efe5dd8b |
chore(deps): update js-yaml and vitest in the nemoclaw integration (#4265)
Bump the js-yaml dependency from ^4.3.1 to ^4.3.2 and the vitest devDependency from ^4.0.18 to ^4.1.11, which also moves the transitive @vitest/mocker to 4.1.11. Both stay inside the existing major line. |
||
|
Chris Bartholomew
|
bde55237f5 |
chore(deps): update adm-zip in the zapier integration (#4322)
Raise the adm-zip override floor to >=0.6.1 and update the lockfile (0.6.0 -> 0.6.1). Closes Dependabot alert #1500. |
||
|
Chris Bartholomew
|
9faa236469 |
chore(deps): update vitest in the openclaw integration (#4301)
Routine dependency maintenance for the openclaw integration. - vitest: 4.1.5 -> 4.1.11 (devDependency, in-major) - @vitest/mocker: 4.1.5 -> 4.1.11 (transitive, moves with vitest) - @vitest/ui: 4.1.5 -> 4.1.11 (moves with vitest) Manifest floors raised to ^4.1.11 so the resolution is reproducible rather than dependent on registry state at install time. Tests unchanged at 343 across 13 files; tsc and the build are clean. |
||
|
Chris Bartholomew
|
c2561a9278 |
chore(deps): update vitest and esbuild in the paperclip integration (#4300)
Routine dependency maintenance for the paperclip integration. - vitest: 4.1.3 -> 4.1.11 (devDependency, in-major) - @vitest/mocker: 4.1.3 -> 4.1.11 (transitive, moves with vitest) - esbuild: 0.25.12 -> 0.28.2 (devDependency) The esbuild bump is required to keep the lockfile installable. esbuild is an optional peerDependency of vite (^0.27.0 || ^0.28.0), which vitest pulls in. With the manifest pinned at ^0.25.0 the two ranges cannot be satisfied by one copy, so the tree carried two full platform-binary sets. Aligning the manifest lets a single hoisted copy satisfy both, which is where most of the lockfile line reduction comes from. Build output is byte-identical before and after; tests unchanged at 57. |
||
|
Nicolò Boschi
|
4cc131c0b2 | release(coding-agents): v0.6.0 integrations/coding-agents/v0.6.0 | ||
|
Nicolò Boschi
|
220dcbdd92 |
feat(coding-agents): record Hindsight tool usage and move logs to ~/.hindsight/coding-agents-logs (#4325)
Adds usage.jsonl: one line per finished user turn with the hindsight_* tools called and whether the reply credited Hindsight memory, plus a `stats` command that reports call ratio and credit-after-retrieval rate per agent. Moves plugin.log and the diag trail out of /tmp (shared, reboot-wiped, unbounded) into an owner-only dir with size-based rotation. |
||
|
Nicolò Boschi
|
71e74444dd | release(coding-agents): v0.5.5 integrations/coding-agents/v0.5.5 | ||
|
Nicolò Boschi
|
a1ecadcd9d |
feat(coding-agents): fall back to knowledge pages, then observations, when the session reflect fails (#4324)
A reflect that times out or 5xxs used to leave the session with no memory. Now the hook searches knowledge pages for the goal and injects the matches; if none match, it injects a raw recall of the bank's observations. 4xx and transport errors still skip memory, since every endpoint would fail the same way. The reflect cap drops from 25s to 20s so the 7s fallback budget still fits the host's 30s hook window. |
||
|
Nicolò Boschi
|
48b62ee081 |
feat(api): attachments on recall and read surfaces for store-owned banks (#4321)
* feat(attachments): per-fact attachments for store-owned banks, from the rows the store returns
Recall, GET /memories/list and GET /memories/{id} now report per-fact
attachments for banks whose memories store owns its rows. The ids travel
on what the store already returns, so resolving them adds no store
round-trip and never reads memory_units for such a bank (that read plans
across every per-bank vector index).
- memories/base.py: META_ATTACHMENT_IDS; FactRecord.attachment_ids,
written to metadata_bag as a deduplicated JSON list (omitted when
empty); build_fact_records fills it from the processed fact;
StoredMemory.attachment_ids; list/get items carry "attachment_ids".
- search/types.py: RetrievalResult.attachment_ids (None = not carried),
passed through ScoredResult.to_dict.
- response_models.py: MemoryFact.attachment_ids, internal (excluded
from serialisation), set from fused results or by a store-answered
recall.
- memory_engine.py: attachments_for_memories(..., carried=) resolves
the carried (document_id, ids) per unit for a store-owned bank and
still returns before any Postgres access when nothing is carried; the
per-document resolution is shared with the unchanged SQL path.
- api/http.py: recall passes the carried ids; list/get take
"attachment_ids" off store-rendered items so the payload has the same
shape on either backend.
Filenames stay null for store-owned banks: document_attachments has an FK
to documents, and the store-owned retain writes no SQL documents row.
* fix(attachments): document and chunk attachments for store-owned banks
GET /documents/{id}, GET /documents/{id}/chunks and GET /chunks/{id}
returned no attachments for a bank whose memories store owns its
documents: attachments_for_chunks read the SQL chunks table and
attachments_for_documents read document_attachments, and neither holds
anything for such a bank (it keeps chunks in the store, and has no SQL
documents row for the edge FK to reference).
Same principle as the per-fact read: derive the ids from the text the
route already read from the store, and resolve them against the
attachments table.
- attachments_for_chunks(..., carried_texts=): chunk_id ->
(document_id, chunk_text). For a store-owned bank the ids come from
the carried text only; the chunks table is never read, and a page with
no placeholder returns before any Postgres access.
- attachments_for_documents(..., carried_texts=): document_id -> text.
For a store-owned bank the ids come from that text; a document with no
carried text (the retain-ingress revisit) is read from the store, and
its chunk texts stand in when the full text is not kept.
- http: list-chunks, get-chunk and get-document pass the texts they hold.
Filenames stay null for these banks: they live on the document edge.
The Postgres path is unchanged.
|
||
|
Nicolò Boschi
|
09abae19f1 | release(coding-agents): v0.5.4 integrations/coding-agents/v0.5.4 | ||
|
Nicolò Boschi
|
f392155926 | feat(coding-agents): warn at session start when the old hindsight-memory Claude plugin is still active (#4320) | ||
|
Nicolò Boschi
|
630c3a63e7 |
fix(reflect): grounding defects in reflect and knowledge-page delta refresh, plus system-evals (#4304)
* test(dev): add a deterministic retrieval eval for reflect's forced prelude
We had no baseline to judge a retrieval change against, so a change to how
reflect's opening hierarchy (mental models -> observations -> recall) picks
its queries could not be told apart from a regression.
The corpus is authored, never extracted: every stored row's text is
byte-identical to the YAML, which is what makes gold labelling possible at
all. Facts and observations are retained with the mock provider scripted
through set_response_callback; mental models are created with explicit
content; staleness is produced by ORDERING (stale models created before the
facts, fresh ones after) because it is derived, not stored.
Questions are grouped by which layer should answer them -- mm_only,
mm_stale, observations_only, raw_facts_only, multi_layer, near_miss, absent
-- so the eval exercises the descent decision and not just one search. The
facts carry deliberate near misses; a corpus of unrelated facts scores 1.0
for any query and measures nothing.
Scored by rank WITHIN each layer. Set membership saturates once the corpus
is smaller than one recall page, and a flat ranking scores every raw fact
behind every mental model however good the query was. So recall@3 and MRR
per layer, plus the short-circuit fire rate reported separately, since the
mental-model query is what decides it.
First deliverable is the floor/ceiling experiment, which needs no LLM in
either arm: the question verbatim (the real fallback when planning fails)
against a hand-written ideal query. First run:
recall@3 floor=0.929 ceiling=1.000 gap=+0.071
MRR floor=0.762 ceiling=1.000 gap=+0.238
Wording moves retrieval, mostly through rank rather than presence, with the
mm_stale question the largest mover (0.50 -> 1.00) -- the case where a wrong
mental model has to be superseded by raw facts.
Two behaviours it already surfaced, neither introduced here:
- A created-but-never-refreshed mental model is always stale: staleness
resolves from the refresh stamps and an unstamped model is reported stale
unconditionally, so it can never short-circuit the descent until something
refreshes it once.
- One stale model suppresses the short-circuit for all of them, since the
rule requires every returned model to be fresh. An unrelated stale model
in the top-5 keeps the descent going even when a fresh model answers the
question outright.
* test(dev): grade reflect's actual answer, not just what it retrieved
The retrieval eval scores the evidence set, never the prose reflect returns.
That is a necessary condition -- reflect is grounded, so an unretrieved fact
cannot be answered -- and nowhere near a sufficient one.
The blind spot is the category the whole exercise is about. For the mm_stale
question, retrieval can return the stale "Stripe" mental model AND the Adyen
facts, score recall@k = 1.0, and the answer can still say Stripe because the
model trusted the summary over the raw facts. The retrieval tier calls that a
pass.
So answer_eval.py runs reflect_async end to end on a real model and grades
the text with an independent judge, scoring two things separately because
they fail differently:
- correct: meets answer_criteria. Missing it can just mean incomplete.
- trap: asserts must_not_claim, the specific wrong answer the question baits.
That is a grounding failure, and it is the number that matters -- a
confidently wrong answer is worse than a hedged one.
Every question runs N times and the output is a rate, not a verdict. Not for
CI. The judge mirrors tests/llm_judge.py (independent model, majority
confirmation on a "not met") but is reimplemented here because that module
lives under tests/ and is not importable from this package; the eval warns
when the judge and reflect resolve to the same model, since the local .env
makes that the default and a model grading its own output agrees with itself.
First run -- gemini-2.5-flash-lite reflecting, gemini-2.5-flash judging, 2
runs, budget=low, on this branch:
overall correct 93.8%, trap rate 0% on every baited question
multi_layer 50%, everything else 100%
The multi_layer miss is real run-to-run variance rather than a judge
artifact: one run named the platform-team handover, the other dropped it.
This is one arm only. Comparing main against the branch on the same corpus,
model and N is what actually settles the cold-query question, and is not done
here.
* test(dev): hard corpus, failure attribution, and runner env-precedence fixes
WIP checkpoint before A/B testing the thought_signature failure.
* test(dev): stop the outage corpus generating two "April 2026 outage" rows
The numeric_precision cluster cycled months with `i % 12` and years with
`i // 12`, which produced a second row claiming to be THE April 2026 outage
with different values (850 connections / 87 minutes vs the gold's 200 / 47).
The question then had two contradictory answers, and reflect reporting
"conflicting information" -- exactly what its Conflicts and Ambiguity rules
prescribe -- was scored as a failure. The corpus was wrong, not the answer.
I reported it as a reflect defect before checking; it was mine.
Near-misses must differ in what they ASSERT, never in what they claim to BE.
Each outage now owns a distinct (month, year) slot with April 2026 reserved
for the gold row, and `_assert_subjects_are_unique` fails the build when two
rows in a cluster name the same subject -- verified by re-introducing the
collision, which the guard catches.
* fix(reflect): don't manufacture a value for a period the memories don't cover
Asked for an engineering headcount in a year the bank held no data for,
reflect extrapolated backwards from the following year's monthly figures and
answered with a specific number -- calling it "reliably inferred" and
"reliably deduced". Three runs out of three, on gemini-3.7-flash. That is not
a hedge: it is a fabricated data point wearing the language of certainty, and
it is worse than "not recorded" because a reader cannot tell the difference.
The prompts asked for it. Every path that writes an answer said some version
of "if the exact answer isn't stated, use what IS stated to give the best
possible answer", with "only say you don't have information if the retrieved
data is truly unrelated" closing the escape hatch. A neighbouring year IS
related, so declining was effectively disallowed.
The missing distinction: inference may CHARACTERISE what the data covers; it
may not MANUFACTURE a value for something the data does not cover.
_GROUNDING_BOUNDARY states that, and is shared by all three answer paths (the
tool-loop system prompt, the forced-synthesis system prompt, and the
final-synthesis instructions) so they cannot drift apart. It explicitly
preserves qualitative inference, because a rule read as "never infer" would
break the synthesis that makes reflect worth having.
Tests split per the convention: the wiring is deterministic and asserted
directly, including that the rule keeps its teeth and its carve-out. The
behavioural pair is marked hs_llm_core and its docstring says plainly what it
does NOT do -- it does not reproduce the incident (verified: it passes against
the pre-fix prompt on two models), it guards the contract, and its more
valuable half is the check that the rule has not become a refusal reflex.
Reproduction lives in hindsight-dev/benchmarks/prelude (hq-absent-2024).
488 reflect/prompt tests pass; the golden prompt fixture is updated.
* test(dev): accept reflect's accurate UK qualification on the scoped_truth question
The criteria said 2FA is "mandatory for EU (and UK) accounts", which reads as
unconditional. The memory actually says UK accounts are mandatory FROM 2026,
and reflect answered "UK Accounts: Mandatory starting in 2026" -- more precise
than the criteria, and marked wrong for it (1 run in 6).
Scoring a correct answer as a failure is the worse error for a benchmark: it
manufactures a defect to chase. The criteria now accepts any accurate
treatment of the UK and keeps the real assertion, which is that the answer
must be regionally qualified rather than a flat yes or no.
* fix(mental-models): stop delta ops treating the batch-only synthesis as authoritative
The delta refresh writes a synthesis from the new batch alone, then asks a
second call to merge it into the stored page. That call read the synthesis as
evidence: its "a total of 4" counted only the batch and replaced a page's 3
customers with 4; its "no release was deployed" described only the batch and
overwrote a production release recorded one wave earlier.
Label the synthesis UNTRUSTED (only the supporting facts justify an operation)
and add the combine-not-swap, absence-is-not-contradiction and refutation
threshold rules. Replayed against both captured failures: 0/5 -> 5/5.
* test: add hindsight-system-evals, published as a quality metric by the perf monitor
Blackbox quality evals over a real hindsight-api and a real model, through the
published Python client only — the system-tests shape without the stub, since
stubbing the model would score the stub. First suite: knowledge-page
convergence, the eval that found the delta-ops regressions. Each page is
graded twice: correct, and whether it stores the specific baited falsehood.
Runs in perf-test.yml (daily, not on PRs — needs secrets, and one red run is as
likely noise as regression) and publishes correct rate and trap count to the
continuous performance monitor. Seeding uses chunks retain with consolidation
off, so the only model calls are the ones under test: minimum acceptance ~90s,
full ~5 min.
Also: the hindsight-dev knowledge-page tools used to find and replay those
failures (kp_eval, kp_diagnose, replay_gemini, iterate_delta_prompt).
* refactor: move the reflect evals into hindsight-system-evals and trim the new prompt text
Everything from hindsight-dev/benchmarks/prelude now lives in the blackbox
package, driven only through the public client:
- test_02_reflect_answers: the one-shot reflect eval (minimum acceptance is the
2024-headcount incident), with retrieval-vs-reasoning blame from the tool trace;
- debug/diagnose_page: dry-runs a page's second refresh and dumps every traced
prompt; debug/replay_delta_ops: replays one captured delta-ops request per
prompt variant and interrogates the model.
The retrieval floor/ceiling tier is dropped: it measured the planned-prelude
change (#4066), which was closed, and needed engine internals to author layers.
Prompt size, measured against main:
- grounding boundary rewritten compactly and no longer repeated in the final
instructions (the final and reduce calls already carry it in the system
prompt, so it was sent twice): tool loop +131 tokens, final +112;
- delta ops: the combine-not-swap rule removed. Ablation by replaying the two
captured failures: every other piece is load-bearing (dropping the absence or
refutation rules, or the long synthesis paragraph, falls to 2-3/5), this one
is not (5/5 on both without it). +491 tokens.
Full run on the result: 14/14 correct, 0 traps (7 pages, 7 reflect answers).
* test(system-evals): drop an unused property and type the page eval tests
|
||
|
Nicolò Boschi
|
cabfbb9fe0 |
fix(coding-agents): admit one codebase survey per bank, held by a heartbeat lease (#4303)
* fix(coding-agents): admit one codebase survey per bank * fix(coding-agents): hold the survey lease with a heartbeat supervisor The PID-owned survey lock could wedge a bank forever: a recycled PID answers kill(pid, 0) as alive, and the lock had no expiry. Replace it with a lease the holder refreshes every 5s, reclaimable after 30s of silence. The survey agent cannot heartbeat, so it now runs under a tiny detached Node supervisor (dist/survey-supervisor.js) that holds the lease for exactly the agent's lifetime, releases on exit, and kills the agent if the lease was taken over. Only rename/utimes/stat are used, so it behaves the same on macOS, Linux and Windows; no flock, O_EXCL or PID probes. * fix(coding-agents): hand the survey spec to its supervisor via env, not argv Reproducing #4255 end to end (6 concurrent SessionStart hooks on a cold bank) showed the supervisor SIGKILLed within milliseconds of starting, so no survey ran at all. A harmless stand-in launched from the same point with the spec in argv was killed 5/5; the same launch with the spec in the environment survived 5/5. The spec carries the whole agent command line (prompt, inline MCP config, tool deny-list), which endpoint security (SentinelOne here) kills on sight when a hook-launched node process carries it. The supervisor now reads HINDSIGHT_SURVEY_SPEC and deletes it before starting the agent, so the agent and its MCP server never inherit it. --------- Co-authored-by: r266-tech <r266-tech@users.noreply.github.com> |
||
|
Nicolò Boschi
|
59e25d74f8 |
test(system): the remaining epic stories, and two bugs they found (#4293)
* test(system): stories 62, 63, 72, 74 — operations, entity resolution and links - 62/63 a failed operation carries its error and can be retried in place (retry re-runs the stored work; re-submitting is what creates duplicates), and a batch retain reports a parent over per-item children so one bad item in fifty is diagnosable without re-running the other forty-nine. - 72 resolve_entities gates *fuzzy* resolution, not normalisation. A case difference collapses whichever way the flag is set, so a test using only a case variant sees no difference and proves nothing — the story uses a genuine variant and pins the normalisation case separately. - 74 entity links follow a document replacement: the old entity goes, the new one arrives, the links move with them and graph traversal follows. The last test in 74 FAILS, on #4291: entities.mention_count is only ever incremented — there is no decrement anywhere in the codebase — so replacing or deleting a document leaves it inflated. The link table is correct and only the denormalised counter drifts, which makes it invisible until you compare them. It is user-visible on the entity list, sizes graph nodes, and orders the curation listing; the drift is proportional to how often documents are updated, and the coding-agents integration re-retains under one document_id every turn. Asserted against the link count rather than a fixed number, so it keeps holding whatever the fixture grows into. * test(system): stories 28, 33, 36, 37, 41, 93 — the rest of the epic - 28 one bank consolidates one run at a time. A burst of five triggers produces one pass, because concurrent runs race on the layer they are both editing: each reads the observations, each decides independently what to retire, and the second to write undoes half the first's conclusions. - 33 refresh dedupe (#3487): five requests, one operation, and the model still ends up refreshed — collapsing to *zero* looks identical from the queue. - 36/37 a page rename reaches its backing model, and retracting the evidence schedules the page to be rewritten. What the rewrite *says* is deliberately not asserted: pages refresh in delta mode, so driving the wording from a stub would mean writing the delta myself and then asserting I had written it. - 41 reflect raises rather than degrading. A model that never calls a tool, or answers empty, must not yield "" — an agent cannot tell that from "your bank holds nothing about this", and the two demand opposite responses. - 93 an inline image reaches the extraction model as an image part, in order after its caption, and is stored against both document and chunk. Two tests FAIL, on two bugs found writing them: - #4291 entities.mention_count is only ever incremented — no decrement exists anywhere — so replacing or deleting a document leaves it inflated. It is user-visible, sizes graph nodes, and is the primary sort key of the curation listing. - #4292 both binary download endpoints declare application/json alongside their real media type, so every generated SDK decodes the bytes as text and corrupts them. Fetching an attachment — the documented audit path behind an image-derived fact — is impossible through any client. Both are left going through the published client rather than reaching around it with raw HTTP: an SDK-inaccessible endpoint is the defect, not an obstacle. * test(system): story 45, the reflect answer-length budget `max_tokens` on reflect targets the *visible answer*, not the provider — the distinction that matters because an agent embedding the answer in its own context has a budget, and an overrun does not fail loudly, it silently pushes something else out of the window. An over-long answer triggers a second call that rewrites it to fit. Pinned: the rewrite is told both the target and the whole text (a truncated copy would make it trim the wrong end), and it does not happen at all when the answer already fits or no budget was given — asserted by declaring no rule for the trim step, since an unscripted call fails the test. Took three wrong turns to find: `max_tokens` does not trim the evidence envelope, and neither does `reflect_source_facts_max_tokens`, which governs the source facts behind observations. The budget is on the answer. * test(system): mark #4291 and #4292 as fixed in the stories that found them Both fixes are on main (#4296, #4315), so the two tests that failed on them now pass. Their docstrings said 'fails today'; they now say what the bug was and which fix closed it, so the history stays readable without the issue open. |
||
|
Nicolò Boschi
|
31a8c52db5 |
refactor: replace httpx and sync HTTP with aiohttp across production code (#4318)
Production code now talks HTTP through aiohttp only, and only asynchronously. httpx's async client is 5-12x slower per request than aiohttp at the concurrency we run embeddings/rerank/LLM calls with (local stub: 1.3k vs 6.8k req/s at c=8, 527 vs 6.3k at c=64), and the sync paths needed a thread per in-flight request. - engine/aiohttp_session.py: LoopLocal / LoopLocalSession (one session per event loop, created lazily, closed-loop entries released), per_phase_timeout (httpx-style per-phase timeouts), raise_for_status -> UpstreamHTTPError (keeps body + status_code for remote_retry), close_loop_sessions() called from MemoryEngine.close(). - Embeddings interface is async (encode/encode_query/encode_documents); remote providers use aiohttp or the SDK's async client, bounded fan-out via a per-loop semaphore instead of a thread pool. Local models stay in a worker thread. - Rerankers, Codex/Nous/xAI OAuth providers (async token refresh with a per-loop asyncio.Lock + non-blocking flock), Fireworks, Ollama-native, llama.cpp, LlamaParse and Iris parsers moved to aiohttp. - Webhook SSRF guard re-implemented as an aiohttp resolver that only returns validated addresses (no DNS cache, no redirects, no env proxies), with IP literals checked before sending. - hindsight-embed probes, supabase-tenant extension and the litellm integration's async hooks moved to aiohttp. - ruff TID251 bans httpx/requests/urllib.request/urllib3/http.client in hindsight-api-slim and hindsight-embed production code; code-review skill documents the rule (tests exempt). |
||
|
Nicolò Boschi
|
88456981f6 |
fix(api): declare binary download bodies so generated clients return bytes (#4315)
* fix(api): declare binary download bodies so generated clients return bytes (#4292) GET attachments/{id} and files/download/{key} returned a raw Response with no response_class, so FastAPI added application/json next to the declared binary media type. The generated clients picked JSON and decoded the body as text, corrupting the bytes (UnicodeDecodeError on a PNG's 0x89 magic byte). Set response_class=Response and a string/binary schema on both routes, and regenerate the spec and clients (Python now returns bytearray, TS Blob). Tests: an api-slim spec guard that no 200 response mixes application/json with a binary media type, and a live-server Python client test that retains an inline PNG and fetches it back byte-identical. * chore(docs-skill): regenerate the skill's OpenAPI copy for the binary download schema |
||
|
Nicolò Boschi
|
e33debb958 |
fix(coding-agents): record why an automatic reflect failed (#4316)
A reflect_failed diag line read either "reflect 500" or "This operation was aborted (20)", with no bank, so neither could be traced server-side. - reflect() keeps the server's response body (first 1000 chars) on a non-2xx reply, like req() already does. - Our own deadline now reads "reflect timed out after Nms" (the abort stays on the cause chain). - reflect_failed records the bank and the timeout, and no longer cuts the error at describeError's 200-char default. |
||
|
Nicolò Boschi
|
d9df6d2a4d |
perf(recall): three CPU cuts on the recall path (audit serialization, phase sampling, on-loop query embedding) (#4314)
* perf(api): serialize a recall's audit row once, and decode embedding batches with orjson Two costs on every recall that a CPU profile of a recall-heavy API put at ~5% of its busy CPU (450 recalls/s, 2 vCPU): - The HTTP audit wrapper built a Python dict of the whole response with model_dump(mode="json") on the request path, and the writer then re-encoded that dict with json.dumps (3.5% alone). A pydantic response now goes straight to JSON with model_dump_json(), one pass in Rust, carried on AuditEntry.response_json. Anything else still goes through _safe_json, which uses orjson when it is installed. The stored document is the same; the column is JSON, so key order and escaping are not observable. - A TEI embedding batch is a large JSON array of floats, parsed by the stdlib decoder behind response.json() (1.4%). orjson.loads(response.content) when available. orjson is optional in both places: without it the previous code path runs unchanged. * perf(metrics): opt-in sampling for recall-phase observations Every recall records ~10 phase histograms, and OTel's aggregation behind them was ~4.6% of a recall-heavy API's busy CPU. HINDSIGHT_API_RECALL_PHASE_SAMPLE_EVERY=N records 1 in N calls, sampled independently per call, so each phase's distribution -- and its percentiles -- stay unbiased; only absolute counts scale by 1/N. The default of 1 records everything, as before. * perf(embeddings): embed a recall query on the event loop instead of a worker thread A recall embeds one short string. On the thread path that costs an executor hop plus httpx's pure-Python sync stack: 1.02 ms of CPU per query, against 0.39 ms for an aiohttp request made on the loop (measured in-process against the same TEI server; identical vectors). RemoteTEIEmbeddings.aencode_query makes one attempt and returns None on any failure, so the existing thread path and its retry policy still handle every error. It also declines when uninitialized or when a test injected a client. generate_embeddings_batch sends both paths through the same alignment and vector validation. * fix(recall-perf): declare orjson, move phase sampling into config orjson was imported behind an ImportError guard but never declared, so it was not installed and both speedups (audit rows, TEI decode) were dead code; ty also failed on the unresolved import. Declare it and drop the fallbacks. HINDSIGHT_API_RECALL_PHASE_SAMPLE_EVERY was read straight from the environment in metrics.py; it is now a HindsightConfig field with docs and env-template entries, plus a sampling unit test. The TEI embedder's aiohttp session attributes are initialised in __init__ instead of via getattr. * docs: regenerate docs skill for the recall-phase sampling flag * test(tei): hold per-thread clients so a freed client's reused id can't read as shared |
||
|
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. |