Stop destroying merged entities, close two API holes, and fix two wrong queries - #25
Merged
Merged
Conversation
…ng queries Five defects from the audit, each found by asking what a feature actually does rather than what it claims. Merging a duplicate entity could destroy it. The three edge-transfer steps were each wrapped in a bare catch, and the DETACH DELETE that followed sat outside every try, so a failed transfer deleted the duplicate with its relationships never copied, while callers counted a success. The delete now runs only when every transfer succeeded; a failure is logged, returned to the caller, and leaves the duplicate intact. That state is recoverable rather than corrupt, because each transfer query only matches edges still attached to the duplicate, so a retry converges. The four steps still are not one transaction, which is noted in the code: this handles exceptions, not process death mid-merge. The search endpoint let a caller write Cypher. Its label filter was built by string concatenation, so `types=Function) OR (true` escaped the predicate and returned every node type. Labels cannot be parameterized, so the fix is an allowlist checked before any query runs. Every mutating request was cross-site forgeable. CORS decides whether a browser may read a response, not whether the request runs, and the body parser accepted text/plain, which a simple cross-origin POST can send without a preflight. Now any request that is not GET, HEAD or OPTIONS must carry an application/json content type, and an Origin header, when present, must pass the allowlist CORS already uses. Guarding by method rather than by a list of known routes means a route added later is covered by default instead of shipping unguarded. Where-is-this-used answered "nowhere" for most types. The query pinned the declaration with an unordered LIMIT 1, but a type name exists both as a declaration node and as a separate TypeRef proxy with no file path, and USES_TYPE, HAS_PARAM and RETURNS all land on the proxy. It now matches every node carrying the name and deduplicates, and works out the declaring file once per query so same-file usages are still classified correctly rather than being reported as coming from somewhere else. Half of all symbols reported no callees. In the search enrichment, n was bound inside the first OPTIONAL MATCH alongside the caller edge, so a symbol nobody calls left n null and every later clause computed against null. On the live graph that cost 20 of the 41 symbols that have callees. Binding n first fixes it. The enrichment map was also keyed on name alone, so one declaration's numbers could overwrite another's; it now keys on file, name and line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Two rounds of adversarial review found four defects in the first pass, each of the same shape: correct for the case the tests constructed, wrong for one they did not, behind a comment that sounded rigorous. Merging still deleted a duplicate when the canonical entity was already gone. The guard was "no transfer threw", but a Cypher MATCH on a missing node returns zero rows rather than raising, so all three transfers quietly moved nothing, the error list stayed empty, and the delete ran. Both entities are now checked for existence before anything is deleted, and the two absences mean different things: a missing duplicate is a no-op, since something already consumed it, while a missing canonical is a failure, because the edges have nowhere to go. A no-op merge then counted as a merge. Beyond inflating the number, it could double-book one physical deletion, because the same entity can appear in two candidate pairs and the first pair's real merge already counted it. Callers now count on whether a delete happened, not on whether the call avoided throwing. Reference classification guessed a file when it could not know one. Rows whose edge lands on a real declaration use that declaration's file, which was the earlier fix. Rows landing on a shared type proxy fell back to one file picked arbitrarily from everything matching the name, so with two declarations sharing a name across files, one was right by coincidence. The fallback now applies only when the matched set implies a single unambiguous file, and otherwise reports that it cannot tell, which is the honest answer. The label allowlist rejected labels that were never a threat. It had been copied from the vector-search list, which covers the labels vector search can return, while this parameter filters a Cypher path where Commit and TypeRef are perfectly valid. Labels are now discovered from the graph itself, so there is no second list to drift. An empty graph reports that nothing is indexed rather than declaring every label invalid, and an empty discovery result is not cached, so a first index becomes visible immediately. Filtering by type also hid its own shortfalls. The filter runs after ranking, so a narrow type could empty a truncated page, and the notice explaining that was computed and then dropped on the path where it mattered most. Both outcomes now say what happened, and say plainly when the results came from the weaker substring search instead of the ranked one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects from the third review round, both reproduced against a real graph before anything was changed. Merging an entity with itself destroyed it. The existence check added last round asked whether each key matched a node, never whether the two keys were the same key. When they were, both sides bound to the same node, the transfers cross-joined, and the delete removed every node carrying that key including the one meant to survive. Calling it on "Sarah" left no Sarah and no edges, and reported a clean success. The fix is not a guard on the symptom. Entities are identified everywhere by text and type, roughly fifteen methods do it, and the entity-creation code already assumed that made writes race-free, which is only true with a constraint behind it. Nothing enforced one. So the schema now does, and mergeEntities refuses two inputs it cannot serve honestly: the same key on both sides, and a key matching more than one node. The constraint is best-effort by nature, since it lands in a failed state without raising when a graph already holds violations, and the comment says so rather than implying more. The refusals are the part that always applies. Consolidating entities that already share a key needs a separate tool that works on node identity, because text and type genuinely cannot tell them apart. That is not in here. Separately, a search filter naming no real label returned a 500 carrying the engine's own error text. An empty label list was treated as a filter rather than as nothing, so it discarded every hit and then built a WHERE clause with an empty predicate. It is now rejected where the filter is resolved, so neither search path can meet that state. Auditing for the same shape elsewhere turned up a second live instance, a non-numeric limit on the graph endpoint, so every route now returns a fixed message and logs the real error server side. The raw Cypher endpoint is deliberately exempt: there the engine's error is feedback about a query the caller wrote themselves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ot fire Two small corrections from the third review. The guard that refuses an ambiguous merge blamed the wrong side. Its two OPTIONAL MATCH clauses share no variable, so they cross-join, and each count reported the product rather than its own side's cardinality. Because the duplicate side is checked first, a merge whose canonical key was the ambiguous one was refused with a message naming the duplicate, which is fine. The refusal was always correct; only the explanation was wrong, and that explanation reaches the logs of the tooling meant to make this cleanup safe, so it would have sent someone to the wrong entity. Counting each side distinctly fixes it. Only one of the two directions could ever have shown the fault, because when the duplicate side is the ambiguous one the corrupted number happens to equal the true one, which is why the existing symmetric test could not have caught it. The embeddings route kept a branch that echoed an error message verbatim when it mentioned embeddings not being configured, on the grounds that such a message is our own text rather than engine output. That is no longer true: the function it names returns a benign empty result instead of throwing, so nothing it produces reaches that branch. What can reach it is an unrelated failure whose text happens to contain the same words, which is exactly the raw output the branch was supposed to exclude. Since it can no longer do the job it was written for, it is gone rather than merely re-explained. Also corrected the schema comment, which described the constraint's failed state as more permanent than it is. Once the conflicting data is removed, the next connect retries and the constraint becomes active on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch was previously deployed
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.
The first batch from the audit. Five defects, each of the same character: fully wired, no errors, tests passing, and quietly doing the wrong thing.
Merging a duplicate entity could destroy it
The three edge-transfer steps in
mergeEntitieswere each wrapped in a barecatch {}, and theDETACH DELETEthat followed sat outside every try. A failed transfer still deleted the duplicate, with the edges that never got copied going with it, while callers inentity-resolution.tsdiscarded the return value and counted a success.The delete now runs only when every transfer succeeded. A failure is logged, returned to the caller, and leaves the duplicate in place. That state is recoverable rather than corrupt: each transfer query only matches edges still attached to the duplicate, so a retry converges to a full merge. The four steps are still not one transaction, and the code says so: this handles exceptions, not process death mid-merge.
This was the only finding in the audit that destroyed data rather than hiding it.
The search endpoint let a caller write Cypher
/api/search?types=built its label filter by string concatenation.Cypher cannot parameterize a label, so the fix is an allowlist checked before any query runs. Now returns 400 naming the offending label;
types=Function,Classstill works.Every mutating request was cross-site forgeable
CORS decides whether a browser may read a response, not whether the request runs. The body parser accepted
text/plain, which a simple cross-origin POST can send without triggering a preflight, so the side effect landed even though the attacker could not see the reply.Any request that is not GET, HEAD or OPTIONS must now carry
Content-Type: application/json, and anOriginheader, when present, must pass the same allowlist CORS already uses.The first version of this guarded a hardcoded list of four route paths. That was changed in review: guarding by method means a route added later is covered by default rather than shipping unguarded because someone did not know this file existed. Demonstrated side by side:
Verified no GET route mutates anything, and that all four dashboard fetch calls already send a JSON content type, so nothing legitimate is refused.
"Where is this used" answered nowhere for most types
The references query pinned its target with an unordered
WITH target LIMIT 1. A type name exists in this graph twice: as a declaration node with a realfilePath, and as a separateTypeRefproxy withfilePath: null.USES_TYPE,HAS_PARAMandRETURNSall terminate on the proxy, so pinning the declaration dropped every one of them.It now matches every node carrying the name and deduplicates. Location filters still disambiguate two genuinely separate declarations, but never exclude a node that has no location to disambiguate against, so this stays correct once
TypeRefnodes gain real file paths.The first version of this fix introduced a second bug that its own tests passed over:
sameFilecompared against whichever target the edge happened to land on, so a reference arriving through the proxy was always classified as coming from another file, and the declaring file appeared inreferencingFiles. Caught in review against the live graph. The declaring file is now worked out once per query. Verified in both directions, since the easy overcorrection is to mark everything same-file:Half of all symbols reported no callees
In
enrichFromGraph,nwas bound inside the firstOPTIONAL MATCHalongside the caller edge. A symbol nobody calls leftnnull, and every clause after it computed against null, so a function that calls other functions but is called by nothing reported zero callees.On the live graph, 20 of the 41 symbols that have callees lost them. It also degraded ranking, since the reranker builds its graph signals from the same map.
The map was also keyed on
namealone, so with collisions the last row won and a hit could be decorated with another symbol's numbers. It now keys on file, name and line, and correctly reconciles thatVariablenodes store their line aslinerather thanstartLine.Verification
pnpm turbo run typecheck20/20 andpnpm turbo run test32/32.pnpm release:checkpasses end to end with all four MCP tools and the dashboard smoke-verified. Embedded driver integration 49/49.Test counts: graph 50 to 57, core 159 to 163, api 59 to 85. Every fix has a test that was demonstrated failing against the old code first. The
ERRORlines now visible in the graph suite output are the merge tests deliberately exercising transfer failures, which is the logging that did not exist before.Deliberately not in this change
The seven-label allowlist now exists in three packages with no shared source of truth. Real problem, crosses package boundaries, and it gets its own change.
🤖 Generated with Claude Code