context-graph-enrich: two-regime rework (full-session T1, curate clustering, Batch API) - #124
Merged
Merged
Conversation
…tering, Batch API) Implements the LLP 0028 redesign; the spec edit lands with the code. - Per-session high-water mark replaces the global (timestamp, tiebreak) cursor (state.js schema v4). T1 propose now extracts a whole DAG-ordered session in a single call — closing the 12k-char truncation defect (silent loss on 47% of sessions). Two regime selectors: ongoing (settled + past-watermark, capped) and backfill (all sessions). - T2 curate clusters by recall-region + embedding-cosine the no-recall remainder (hypaware.embedder, best-effort), with content-based shared context replacing the structural one-hop neighborhood. merge now writes a committed row under the canonical key with the merging session's anchor → a `produced` edge per contributing session; the node dedups by content-addressed id. - @hypaware/completion-anthropic gains an optional `batch` surface (Anthropic Message Batches: submit/poll/results/cancel; refusal = success; provider error messages never surfaced). New `hyp enrich backfill` command, and the ongoing curate daemon source is now submit-and-collect (job state in the sidecar) so frontier work never blocks a tick. npm test 1251 pass / 0 fail; tsc 0 errors; lint clean; all @refs resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Dual-agent review —
|
| Source | Finding (severity, evidence) | Intersects |
|---|---|---|
| Codex #4 / Claude | propose/curate sidecar lost-update race (major; propose.js:48,106 / batch.js:139,168) | Concurrency surface; Risk #1, #2 |
| Codex #1 | same-timestamp watermark drops parts (major; propose.js:153-260) | Risk #3 (data loss) |
| Codex #2 | merge without item_type mis-routes produced edge (major; contract.js:32 / curate.js:557) |
Risk #4 |
| Claude | parseBackfillArgv untested (minor; commands.js) |
Risk #5 (coverage) |
| Claude | LLP 0028 "DAG order" ref dishonest (minor; llp/0028:70-71) | Targets (LLP), not a runtime surface |
| Claude | inline import('...') types (minor; batch.js:219-220, curate.js:130) |
Targets (style), not a runtime surface |
Codex review
Fix Validations
Enrichment truncation defect
- Status: correct
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:65, hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:78, hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:197, hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:238
- Assessment: The old slice model is gone: a selected session reads all filtered parts, builds one transcript, and makes one T1 call. I did find separate ordering/watermark issues below, but the 12k input truncation path itself is removed.
Findings
1) Behavioral Correctness
- Severity: major
- Confidence: high
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:153, hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:155, hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:178, hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:182, hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:260
- Why it matters: The persisted mark is
(ts,id), but the ongoing selector only aggregatesMAX(ts)and excludeslastTs <= prev.ts, so a newly arrived same-timestamp part with a higher tiebreak id is never selected and its text is lost. - Suggested fix: Make the session selector compare the full latest tuple, not timestamp alone. For example, return the latest row id at the max timestamp per session, compare
(last_ts,last_id)to the stored mark, and add a regression where mark{ts:1000,id:'p1'}must reselect a session whose latest part is{ts:1000,id:'p2'}.
4) Concurrency, Ordering & State Safety
- Severity: major
- Confidence: high
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:48, hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:106, hypaware-core/plugins-workspace/context-graph-enrich/src/batch.js:136, hypaware-core/plugins-workspace/context-graph-enrich/src/batch.js:139, hypaware-core/plugins-workspace/context-graph-enrich/src/index.js:73, hypaware-core/plugins-workspace/context-graph-enrich/src/index.js:80
- Why it matters:
enrich-proposeandenrich-curateare independent sources sharing one sidecar; a long propose tick can readcurate_job: null, the curate tick can submit and persist a batch job, and then propose can write its stalestate.curate_jobback to null, orphaning the provider batch and allowing duplicate submits. - Suggested fix: Re-read state immediately before the propose write and merge into the latest state: preserve
latest.curate_joband mergelatest.session_markswithnewMarks. Add an interleaving test that writes acurate_jobbetween propose’s initial read and final write.
2) Contract & Interface Fidelity
- Severity: major
- Confidence: medium
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/contract.js:32, hypaware-core/plugins-workspace/context-graph-enrich/src/prompts.js:63, hypaware-core/plugins-workspace/context-graph-enrich/src/prompts.js:71, hypaware-core/plugins-workspace/context-graph-enrich/src/curate.js:557, hypaware-core/plugins-workspace/context-graph-enrich/src/curate.js:562
- Why it matters: Merge provenance only converges under the same canonical
(item_type,item_id), but the tool schema does not requireitem_type; if the model returnsmerge_intowithoutitem_type, routing falls back to the prospect type and can attach the produced edge to the wrong content-addressed node. - Suggested fix: Treat merge without
item_typeas invalid/pending, or make the schema enforce conditional merge fields and add a routeDecision test for omitteditem_type.
No Finding
- Change Impact / Blast Radius
- Error Handling & Resilience
- Security Surface
- Resource Lifecycle & Cleanup
- Release Safety
- Test Evidence Quality
- Architectural Consistency
- Debuggability & Operability
Evidence Bundle
- Changed hot paths: per-session propose selector/watermark, shared state sidecar writes, ongoing curate submit/collect, merge-to-committed routing, Anthropic batch client.
- Impacted callers: hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:392, hypaware-core/plugins-workspace/context-graph-enrich/src/batch.js:291, hypaware-core/plugins-workspace/context-graph-enrich/src/index.js:73, hypaware-core/plugins-workspace/context-graph-enrich/src/index.js:80.
- Impacted tests: test/plugins/context-graph-enrich-propose.test.js:138, test/plugins/context-graph-enrich-propose.test.js:312, test/plugins/context-graph-enrich-batch.test.js:192, test/plugins/context-graph-enrich-state.test.js:33, test/plugins/context-graph-enrich-curate.test.js:89.
- Unresolved uncertainty: I did not run tests; review is based on the provided diff plus targeted trace reads. I did not verify current Anthropic API behavior externally.
Claude review
Claude review
Propose tick clobbers a concurrently-submitted curate batch job (lost update → orphaned batch + double spend)
- Severity: major
- Confidence: 85
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:48,106
- Why it matters:
runProposeTickreadsstateonce at tick start (:48), then does a long per-sessioncomplete()await loop, then writeswriteState(..., { schema_version: 4, session_marks: {...}, curate_job: state.curate_job })(:106) using the START-OF-TICK snapshot ofcurate_job. The propose and curate daemon sources run on independent timers with no shared lock. If a curate tick'ssubmitCurateJobpersists a newcurate_jobduring propose's await window, propose's write resetscurate_jobback to its stale value (typicallynull): the submitted batch is never collected (results lost) and the next curate tick submits a duplicate batch over the same pool (frontier-model double spend). This is the mirror of a race the authors explicitly guarded against —submitCurateJob(batch.js:139-142) andcollectCurateJob(batch.js:203) both re-read state right before writing precisely to avoid clobbering concurrent mark advances; the propose side was left unguarded. Found independently by two reviewers (shallow-bug and historical-context). - Suggested fix: Make propose's final write a read-modify-write that preserves the current
curate_job:const cur = readState(runtime.stateDir); writeState(runtime.stateDir, { schema_version: 4, session_marks: { ...cur.session_marks, ...newMarks }, curate_job: cur.curate_job }), or factor a sharedupdateState(stateDir, fn)helper both paths use.
LLP 0028 claims a "DAG order" the code deliberately does not implement
- Severity: minor
- Confidence: 88
- Evidence: llp/0028-context-graph-enrichment.decision.md:70-71 vs hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js (
orderSessionParts) - Why it matters: LLP 0028 §two-tiers-one-pipeline says a session's parts "are stitched in DAG order (
message_index/previous_message_id/agent_id)". The implementation sorts purely by(timestamp, tiebreak)and its own JSDoc explicitly rejects those columns ("without coupling toai_gateway_messages-specific columns ... which a custom source may lack").orderSessionPartscarries@ref LLP 0028#two-tiers-one-pipeline [implements], so an[implements]ref points at a design the code intentionally does not realize — violating CLAUDE.md's "Keep refs honest" / "land the doc edit in the same commit as the code". - Suggested fix: Edit LLP 0028:70-71 to describe the actual ordering — a deterministic
(timestamp, tiebreak)sort chosen over message-graph columns for source-portability.
New backfill argv parser (parseBackfillArgv) ships with no test
- Severity: minor
- Confidence: 90
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/commands.js (
parseBackfillArgv); no references toparseBackfillArgv/runEnrichBackfill/--propose-only/--curate-onlyanywhere undertest/ - Why it matters: CLAUDE.md mandates traditional tests for deterministic argv/config parsing-and-validation.
parseBackfillArgvis a pure function with three distinct validation rules (reject unknown--flags, reject unexpected positionals, reject mutually-exclusive--propose-only+--curate-only), none exercised. A regression (dropping the mutual-exclusion check, or astartsWith('--')ordering bug) would ship silently. Every other new pure helper in this PR (cosine,greedyCosineClusters,chunkBySize,clusterByRecallRegion,sessionMark) got a focused test; this is the gap. - Suggested fix: Add a test for
parseBackfillArgvcovering bare argv, each flag alone, both flags (error), an unknown--flag(error), and a stray positional (error).
Inline import('...') type expressions in JSDoc
- Severity: minor
- Confidence: 90
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/batch.js:219-220; hypaware-core/plugins-workspace/context-graph-enrich/src/curate.js:130
- Why it matters: CLAUDE.md Code Style: "Never use inline
import('...')types. Declare type imports at the top of the file with@importJSDoc comments, then reference the bare names." These newly-added annotations (import('.../collectivus-plugin-kernel-types.d.ts').VectorSearchHit[],.CompletionRequest) are exactly the forbidden pattern, and both files already have a top-of-file@importblock from that same.d.ts, so the names should just be added there. - Suggested fix: Add
VectorSearchHit/CompletionRequestto the existing top-of-file@importblocks inbatch.jsandcurate.jsand reference them bare.
Reports: .git/dual-review/pr-124
…outing) Fixes the dual-review findings on the two-regime enrichment rework: - propose: select sessions on the EXACT (ts, tiebreak) tuple, not MAX(ts) alone. buildSessionAggregateQuery now ranks parts with ROW_NUMBER() OVER (PARTITION BY anchor ORDER BY ts DESC, tiebreak DESC) and keeps rn=1, so a same-millisecond part that advanced a settled session past its mark is reselected instead of silently dropped (Codex #1). selectSessions compares the full tuple via cmpMark; the exact match also avoids re-selecting already-enriched sessions every tick. - state: add updateState() read-modify-write helper (atomic w.r.t. the event loop). propose's final write and batch submit/collect now merge into the latest on-disk state, so a curate_job submitted during propose's await window is preserved rather than clobbered to null — no orphaned batch / double spend (Codex #4 / Claude). - curate: an under-specified merge (missing merge_into or item_type) can't be routed to the right content-addressed node, so routeDecision returns it pending (no commit, no resolution) and routeClusterDecisions leaves it in the queue, rather than attaching the produced edge to the wrong node (Codex #2). - commands: export + test parseBackfillArgv (bare argv, each flag, mutual exclusion, unknown flag, stray positional, flag ordering). - llp 0028: describe the actual deterministic (timestamp, tiebreak) ordering chosen over message-graph columns for source portability, so the orderSessionParts [implements] ref is honest. - batch/curate: move VectorSearchHit/CompletionRequest to top-of-file @import blocks (no inline import('...') types). npm test 1264 pass / 1 skipped; tsc --noEmit clean; lint clean (379 files). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… flake) The pre-existing batch tests failed in CI (ubuntu-latest's Node 20) while passing on local Node 24: `pollUntilEnded` awaits `delay()`, whose setTimeout was `unref()`'d, so the test-runner event loop could drain before the timer fired — reported as "Promise resolution is still pending but the event loop has already resolved", poisoning the whole batch test file. Removing the unref also fixes a latent bug: this delay is awaited inside the backfill command's run-to-completion poll loop, so it must keep the loop alive — an unref'd timer could let `hyp enrich backfill` exit mid-poll. The daemon source intervals keep their unref (correct: never block shutdown); the abort signal still clears this timer for prompt cancellation. npm test 1264 pass / 1 skipped; tsc clean; lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the LLP 0028 redesign (the spec edit lands with the code, one PR). Closes the enrichment truncation defect and adds the two-regime (backfill + ongoing) pipeline through the Anthropic Batch API.
Phase A — per-session watermark + full-session T1
state.jsschema v4 sidecar holds a per-session high-water mark, replacing the global(timestamp, tiebreak)keyset cursor.propose.jsselects a whole session, stitches its filtered parts in DAG order, and makes one T1 call per session — no more 12k-char cap (which silently lost the tail of ~47% of sessions / ~70% of NL content).ongoing(settled = latest part older thansettle_cutoff_minutesAND past mark, capped bymax_sessions_per_tick) vsbackfill(all sessions), via a cheapGROUP BY sessionaggregate.Phase B — curate clustering + produced-edge-per-session
curate.jsclusters by recall-region (bucket warm prospects by top recalled node id) + embedding-cosine the no-recall remainder (useshypaware.embedder, best-effort; falls back to per-session grouping). Shared context is now content-based (union of recalled committed nodes), not the structural one-hop neighborhood.mergenow writes a committed row under the canonical key with the merging session's anchor → aproducededge per contributing session; the node collapses by content-addressed id.recall_cluster_floor,cluster_similarity,max_cluster_size(droppedexpand_depth).Phase C — Batch API + backfill + ongoing batch regime
@hypaware/completion-anthropicgains an optionalbatchsurface (Anthropic Message Batches:submit/poll/results/cancel, raw HTTP + injected fetch; a refusal is a successful result; provider error messages are never surfaced).hyp enrich backfillcommand (out of daemon: propose every session → curate the whole pool via the Batch API, polling to completion).Verification
npm test→ 1251 pass / 0 fail / 1 skippedtsc -p tsconfig.json --noEmit→ 0 errorsnpm run lint→ clean (378 files)@refannotations resolve to real LLP 0028 headings.Notes / follow-ups
ANTHROPICAPI key. Coverage here is unit tests (incl. fake-fetch/fake-batch) + typecheck.🤖 Generated with Claude Code