fix(graph): cloud pull sha over stable fields + code-graph PRD#228
Open
efenocchi wants to merge 7 commits into
Open
fix(graph): cloud pull sha over stable fields + code-graph PRD#228efenocchi wants to merge 7 commits into
efenocchi wants to merge 7 commits into
Conversation
Push stores snapshot_sha256 computed over the stable fields only (excludes observation), but pull was hashing the entire payload bytes (observation included) and comparing the two. Every real cloud row was therefore rejected with 'snapshot_sha256 mismatch' — team pull silently failed and only legacy rows without a populated sha column slipped through. Pull now parses the payload, validates nodes/links shape, and recomputes the hash via computeSnapshotSha256 (the same function push uses), comparing the stable-only hash against the column. Adds a regression suite covering: a row whose observation differs accepts, a tampered payload rejects, and non-JSON / shape-invalid payloads reject.
Turns the planning notes into a PRD: problem, goals/non-goals, project invariant, success metrics, and dispatch instructions (one block = one unit of work an agent/team can pick up). Keeps the detailed A-H backlog with per-task acceptance/effort/files. LLM enrichment (block E) and embeddings (block F) are strictly opt-in, gated on OPENAI_API_KEY / OPENROUTER_API_KEY (or the local embeddings daemon for F). Default stays 100% AST, zero network, zero cost. A5 (cloud pull sha bug) is marked done — fixed in the preceding commit.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements deterministic snapshot hash verification for cloud-based graph snapshots: ChangesCode Graph Snapshot Verification and Roadmap
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 7 files changed
Generated for commit 99741e5. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/graph/deeplake-pull.ts (1)
168-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify the payload's
graphidentity matches the selected row.The SHA check only proves the payload is self-consistent; it does not prove it belongs to this
head/repoKey. A mis-keyed row whosegraph.commit_shaorgraph.repo_keydisagrees with the SELECT key will still be written to${head}.jsonand recorded in.last-build.json, which poisons the local cache with a different snapshot. Add explicit equality checks againstheadandrepoKeybefore the write path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graph/deeplake-pull.ts` around lines 168 - 177, The SHA check only ensures payload integrity but not that it belongs to the requested head/repoKey; add explicit identity checks before any write path: after computing and validating computedSha (using computeSnapshotSha256 and parsedSnapshot) verify parsedSnapshot.graph.commit_sha === head and parsedSnapshot.graph.repo_key === repoKey (or their canonical forms) and if either mismatches return errorOutcome("SELECT cloud row", new Error(...)) with a clear message indicating which field mismatched (commit_sha or repo_key) and the expected vs actual values so the mis-keyed row is rejected rather than written to `${head}.json` / `.last-build.json`.
🧹 Nitpick comments (1)
docs/GRAPH-ROADMAP.md (1)
9-9: ⚡ Quick winReplace machine-local
/tmpreference with a durable repo/PR artifact link.Line 9 points to
/tmp/codex-graph-output.md, which is not shareable/reproducible for reviewers. Prefer a committed file path or a PR comment/link.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/GRAPH-ROADMAP.md` at line 9, In docs/GRAPH-ROADMAP.md update the table entry that references `/tmp/codex-graph-output.md` (the line containing "graphify e Understand-Anything in `_graph-comparison/`; review Codex in `/tmp/codex-graph-output.md`") so it points to a durable, shareable artifact — either commit and reference a file in the repo (e.g., `_graph-comparison/codex-graph-output.md`) or replace the `/tmp/...` path with a PR comment/link or CI/artifact URL; ensure the new reference is accessible to reviewers and update the table cell text accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/GRAPH-ROADMAP.md`:
- Around line 159-173: Add language identifiers to the two fenced code blocks
that currently open without a language (the block containing the roadmap diagram
"Blocco A ... Blocco G" and the block starting "Query: 'pushSnapshot' — 2
matches...") so markdownlint rule MD040 is satisfied; replace the opening triple
backticks with triple backticks plus a language (use "text") for both fenced
blocks around the roadmap diagram and the query/example block.
In `@src/graph/deeplake-pull.ts`:
- Around line 158-166: After JSON.parse you must guard against non-object
results (e.g., JSON null) before accessing properties; update the code around
parsedSnapshot (the variable produced in the try/catch) to first check that
typeof parsedSnapshot === "object" && parsedSnapshot !== null and return
errorOutcome("parse cloud snapshot", new Error("snapshot not an object")) if
that fails, then perform the existing Array.isArray checks on (parsedSnapshot as
{ nodes?: unknown }).nodes and .links; ensure no property access occurs until
the non-null object guard passes so pullSnapshot returns the documented error
outcome for invalid payloads.
---
Outside diff comments:
In `@src/graph/deeplake-pull.ts`:
- Around line 168-177: The SHA check only ensures payload integrity but not that
it belongs to the requested head/repoKey; add explicit identity checks before
any write path: after computing and validating computedSha (using
computeSnapshotSha256 and parsedSnapshot) verify parsedSnapshot.graph.commit_sha
=== head and parsedSnapshot.graph.repo_key === repoKey (or their canonical
forms) and if either mismatches return errorOutcome("SELECT cloud row", new
Error(...)) with a clear message indicating which field mismatched (commit_sha
or repo_key) and the expected vs actual values so the mis-keyed row is rejected
rather than written to `${head}.json` / `.last-build.json`.
---
Nitpick comments:
In `@docs/GRAPH-ROADMAP.md`:
- Line 9: In docs/GRAPH-ROADMAP.md update the table entry that references
`/tmp/codex-graph-output.md` (the line containing "graphify e
Understand-Anything in `_graph-comparison/`; review Codex in
`/tmp/codex-graph-output.md`") so it points to a durable, shareable artifact —
either commit and reference a file in the repo (e.g.,
`_graph-comparison/codex-graph-output.md`) or replace the `/tmp/...` path with a
PR comment/link or CI/artifact URL; ensure the new reference is accessible to
reviewers and update the table cell text accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b2e3eb7-e710-4ef5-a809-74edffa3eec2
📒 Files selected for processing (3)
docs/GRAPH-ROADMAP.mdsrc/graph/deeplake-pull.tstests/shared/graph/deeplake-pull.test.ts
Comment on lines
+159
to
+173
| ``` | ||
| Blocco A Infra (Cursor parity, skill, pull hash) ← blocker operativo | ||
| ↓ | ||
| Blocco B Cross-file calls TS ← valore core del grafo | ||
| ↓ | ||
| Blocco C query/ one-shot (2-in-1) + VFS arricchito ← UX agente | ||
| ↓ | ||
| Blocco D Ricerca fuzzy su find/query ← search migliore | ||
| ↓ | ||
| Blocco E Enrichment LLM (async, opt-in) ← summary, tags, layers | ||
| ↓ | ||
| Blocco F Embeddings + hybrid search ← NL query, ranking vector | ||
| ↓ | ||
| Blocco G Team enriched sync + altre lingue ← dopo core solido | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint.
Line 159 and Line 504 open fenced blocks without a language, which triggers MD040 and can fail docs lint pipelines.
Proposed patch
-```
+```text
Blocco A Infra (Cursor parity, skill, pull hash) ← blocker operativo
↓
Blocco B Cross-file calls TS ← valore core del grafo
@@
Blocco G Team enriched sync + altre lingue ← dopo core solido
-```
+```
-```
+```text
Query: "pushSnapshot" — 2 matches, expanded 1 hop
MATCH [1] src/graph/deeplake-push.ts:pushSnapshot:function (exported, fan_out=3)
@@
MATCH [2] ...
-```
+```Also applies to: 504-512
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 159-159: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/GRAPH-ROADMAP.md` around lines 159 - 173, Add language identifiers to
the two fenced code blocks that currently open without a language (the block
containing the roadmap diagram "Blocco A ... Blocco G" and the block starting
"Query: 'pushSnapshot' — 2 matches...") so markdownlint rule MD040 is satisfied;
replace the opening triple backticks with triple backticks plus a language (use
"text") for both fenced blocks around the roadmap diagram and the query/example
block.
Comment on lines
+158
to
+166
| let parsedSnapshot: GraphSnapshot; | ||
| try { | ||
| parsedSnapshot = JSON.parse(cloudPayload) as GraphSnapshot; | ||
| } catch (err) { | ||
| return errorOutcome("parse cloud snapshot", err); | ||
| } | ||
| if (!Array.isArray((parsedSnapshot as { nodes?: unknown }).nodes) || | ||
| !Array.isArray((parsedSnapshot as { links?: unknown }).links)) { | ||
| return errorOutcome("parse cloud snapshot", new Error("snapshot missing nodes/links arrays")); |
There was a problem hiding this comment.
Reject JSON null and other non-object payloads before touching nodes/links.
JSON.parse("null") succeeds here, and the subsequent property access throws before pullSnapshot can return the documented error outcome. Please first guard that the parsed value is a non-null object, then validate the snapshot shape before hashing or writing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/graph/deeplake-pull.ts` around lines 158 - 166, After JSON.parse you must
guard against non-object results (e.g., JSON null) before accessing properties;
update the code around parsedSnapshot (the variable produced in the try/catch)
to first check that typeof parsedSnapshot === "object" && parsedSnapshot !==
null and return errorOutcome("parse cloud snapshot", new Error("snapshot not an
object")) if that fails, then perform the existing Array.isArray checks on
(parsedSnapshot as { nodes?: unknown }).nodes and .links; ensure no property
access occurs until the non-null object guard passes so pullSnapshot returns the
documented error outcome for invalid payloads.
…t + codex) - F0: embeddings are explicitly opt-in and persisted (settings.json env toggle via update-config, or graph_embeddings flag in ~/.deeplake/config.json). Default OFF; missing backend falls back to lexical with no error. - Section 6 Definition of Done, applies to every block: tests, MANDATORY cross-agent verification (Cursor was silently broken and we didn't know), and Codex review via the gstack codex skill, capped at 2 invocations/block.
- Section 7 Progress: GitHub-renderable checklist of every block grouped by milestone; A5 ticked (done), rest open. - F1: note that the embeddings daemon already exists and today only embeds memory (summary/message), not the graph; this block reuses it.
…d wiring
Closes the silent gap where the code graph only worked on Claude Code.
- New shared parser src/graph/graph-command.ts (tryGraphRead +
parseReadTargetPath): given a rewritten shell command, detects a
cat/head/tail/ls on the /graph/* virtual subtree and synthesizes the
body via handleGraphVfs. One source of truth so per-agent hooks stop
drifting.
- Wire it into Cursor, Codex, and Hermes pre-tool-use hooks (Claude Code
already had inline handling). Each returns its agent-correct decision
shape (cursor updated_input echo; codex/hermes {action:block}).
- Cursor SessionStart now emits graphContextLine (was missing).
- Cursor auto-build: add graph-on-stop to the cursor esbuild bundle (with
tree-sitter externalized) and register it on stop + sessionEnd in
install-cursor.ts buildHookConfig().
Hardened per two rounds of Codex review: only cat<path> or a single
trailing |head/|tail is a graph read (a |grep falls through); reject ..
path traversal; strip quotes and skip leading flags; require exactly one
file operand (multi-file reads fall through). Codex hook uses input.cwd.
Tests: tests/shared/graph/graph-command.test.ts (16) cover the parser
edge cases and routing; full suite 3751 green.
Read-only skill teaching the model when/how to query the code graph via the memory mount (index.md / find/ / show/), with anti-patterns: Incoming (0) != dead code (intra-file calls only), the graph can be stale, find/ is lexical not semantic, and never run a build (hooks handle it). Cursor and pi have no skill loader and rely on the SessionStart inject instead.
A1-A4 ticked in the progress checklist; baseline table and a DoD 6.2 cross-agent status block record what works where (read intercept on CC/Cursor/Codex/Hermes; OpenClaw/pi gaps and Codex/Hermes auto-build deferred to G3) so the next session doesn't rediscover them by surprise.
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.
Summary
Two graph-scoped changes that belong together:
fix(graph): cloud pull sha verification — P1 bug. Push stored
snapshot_sha256computed over the stable fields only (excludesobservation), but pull was hashing the entire payload (observation included) and comparing the two. Every real cloud row was rejected withsnapshot_sha256 mismatch— team pull silently failed; only legacy rows without a populated sha column slipped through. Pull now parses the payload, validatesnodes/linksshape, and recomputes the hash viacomputeSnapshotSha256(the same function push uses), comparing the stable-only hash against the column.docs(graph): code-graph PRD — turns the planning notes into a PRD with problem / goals / non-goals / invariant / success metrics / dispatch instructions, plus the detailed A–H backlog (per-task acceptance, effort, files). LLM enrichment (block E) and embeddings (block F) are strictly opt-in, gated on
OPENAI_API_KEY/OPENROUTER_API_KEY(or the local embeddings daemon for F). Default stays 100% AST, zero network, zero cost.How the bug was found
Cross-read of our
src/graph/against graphify and Understand-Anything (cloned into a sibling dir), then a Codex review pass flagged the push/pull hash divergence. Confirmed by quotingdeeplake-push.ts:96(stable hash) vsdeeplake-pull.ts:155(full-payload hash, now fixed).Tests
tests/shared/graph/deeplake-pull.test.ts: 23/23 green (+5 new regression tests).observationdiffers from build time → accepts (was the production failure).npm run buildgreen (tsc + esbuild).tests/cli/install-consent.test.ts/cli-index.test.ts(untouched by this diff).Notes
Session Context
Session transcript
Summary by CodeRabbit
Documentation
Bug Fixes
Tests