feat(dashboard): opt-in canvas graph engine behind ?graph-engine=next#74
feat(dashboard): opt-in canvas graph engine behind ?graph-engine=next#74Coding-Dev-Tools wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6004a432fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR adds a canvas-based knowledge-graph renderer (
Confidence Score: 5/5Safe to merge as an opt-in experimental flag; the classic renderer is the default and the failure latch ensures any engine error degrades cleanly rather than breaking the graph view. All logic paths analyzed — XSS escaping is explicit and mutation-tested, the rAF lifecycle is correctly wired to navigation, GRAPH_ENGINE_PARKED handles the async-creation race, algorithms are iterative and budget-capped, and the CSP gate uses HTMLParser. No new blocking issues found. Files Needing Attention: No files require special attention beyond the existing flagged items (zoomToNode using raw.nodes rather than the live rendered set, and the parser-blocking script load acknowledged in the PR description as a deferred follow-up).
|
| Filename | Overview |
|---|---|
| engraphis/static/engraphis-graph.js | New 870-line canvas engine: XSS-escaped labels, iterative Tarjan bridge detection, pivot-sampled betweenness with budget cap, pause/resume/destroy lifecycle, and _internals seam for offline tests. Well-structured IIFE with no top-level browser or vendor globals. |
| engraphis/static/dashboard.js | Integrates GRAPH_ENGINE with careful PARKED-state intent recording, failure latching, memoized lazy loaders started in parallel, and per-setter engine dispatch (setStyle, setColorBy, setHighlight, freeze, etc.) all guarded before falling through to classic. |
| scripts/externalize_dashboard_assets.py | Extends the CSP asset gate to cover first-party scripts, deferred-script existence checks, and eager-load detection using HTMLParser (case-insensitive) rather than regex. Clean refactor of _inline_assets into _parse_page. |
| tests/test_graph_engine_asset.py | 24 Node-executed tests asserting XSS escaping, iterative bridge detection, betweenness budget, load-order isolation, and style CSP compliance via new Function('window', source)(window) with a bare stub — load-order proof without a browser. |
| tests/test_externalize_dashboard_assets.py | Eight new tests covering deferred-script detection, orphaned-loader rejection, existence checks, and case-insensitive tag parsing; all negative-tested with injected regressions. |
| engraphis/static/index.html | Comment-only change explaining why force-graph.min.js and engraphis-graph.js are absent from eager script tags — no functional change to HTML structure. |
| engraphis/static/dashboard.css | Adds per-style background gradients for galaxy/solar/cyber as CSS attribute selectors on #graph-net and cursor classes for node-hover state — keeps inline styles out of JS as required by the CSP. |
Reviews (4): Last reviewed commit: "Merge branch 'main' into feat/graph-engi..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23b5ca531a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Six findings from PR #74 review. Particles were assigned per relation with no regard for graph size, so the Cyber style with flow enabled created tens of thousands of animated particles on a large workspace. Capped against the existing large-graph signal. Community clustering added every link to the adjacency, so one sparse influences relation merged two unrelated topics into a single component and Community Islands gave them the same colour and force centre. Matched to the classic renderer's semantics. Hover changed hilite/hoverSet without invalidating the canvas, so with reduced motion on, flow off, or the simulation settled, highlight changes were invisible. The redraw is now requested. Show unlinked nodes never reached the engine: graphData() supplied degree-zero nodes while the engine's own filter still dropped them. Leaving the graph view before /graph or a lazy script resolved ran pause against a null renderer, so the pending callback could still create and start one against a hidden pane -- the rAF leak this PR fixed once already, by another route. The CSP gate parsed script tags case-sensitively, so an equivalent HTML spelling browsers load eagerly slipped past both the eager-load rejection and the existence check. Now uses the HTMLParser already in the file, which normalises case. 1441 passed, 0 failed (+12 tests). All asset gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
engraphis/engraphis/static/dashboard.js
Line 919 in 4336566
When the force-graph asset returns HTTP 200 but is truncated or throws during evaluation, this resolves even though ForceGraph was never registered. The callback re-enters graphRender(), sees ForceGraph still undefined, receives the already-resolved memoized promise, and repeats indefinitely through microtasks instead of displaying the unavailable-engine error; validate the global in onload, as loadGraphEngine() already does.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aceb40d054
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…theme Three further findings from PR #74 review. Community IDs were assigned in raw payload order while graphRenderLegend() sorts communities by size and calls the largest "Cluster 1". Node colour indexes the palette by the ID itself, so whenever a smaller component appeared first the legend described one component with another's swatch. Components are now ranked by size before the IDs escape, matching the classic graphComputeCommunities(). Galaxy style held autoPauseRedraw(false) unconditionally, so a settled large graph repainted every node and link every frame forever. The starfield is the only paint force-graph cannot see; the classic path drops it past GPERF.large, so the same 600-node/2400-link signal now skips it and hands the redraw loop back to the vendor. Type colours resolved from hard-coded dark-theme constants, so switching to Light, Midnight, Solarized or Sepia moved the legend and controls while the canvas stayed dark. The engine cannot read CSS custom properties from a canvas, so the dashboard resolves --entity-* and supplies them through a new setThemeColors(), below style palettes and user overrides in priority. 1446 passed, 0 failed (+5 tests). ruff, externalize and manifest gates pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b20742e71a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Six findings from the PR #74 review, all cases where the opt-in renderer diverged from the classic path it is meant to replace. - Relation labels: the dashboard's Labels checkbox drives two label layers on the classic path, but the engine configured no linkCanvasObject, so ticking it painted entity names only and a relation could be read only by hovering it. Paint them, with the classic gates (zoom, dense-graph). - zoomToNode: graphFocus() treats a false return as "run the recovery path". The engine answered from raw.nodes, which keeps the coordinates force-graph left on a node, so an entity hidden by a scope filter or by the auto-collapsed cluster view still reported success and the camera moved to nothing. Expand a collapsed view, then verify against the data force-graph is actually holding. - Reseeding: render() called graphData() unconditionally with newly allocated arrays, so Style, Color by, Labels and Flow each reset the d3 alpha and restarted the layout. Reseed only when the visible set really changed, and invalidate the paint when it did not. - Cooldown: nothing overrode force-graph's 15s default simulation window, so a large store kept simulating (and repainting) far past the 1.1s/80 ticks the classic path allows. Mirror its size-dependent bounds. - Dense graphs: curvature and arrowheads stayed on past the classic GPERF.dense threshold, paying a bezier and a filled triangle per relation across thousands of links. - Community colours: the cluster legend swatches are CSS (.graph-cluster-N) encoding the Cyber palette, but the engine's Cyber and Solar palettes were ordered differently from dashboard.js, so on the default style the legend described each of the first clusters with another cluster's colour. Each is covered by a test that fails without the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 385289fabf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Confirmed and fixed in 038652b.
const LAYOUT_KEYS = ['mode', 'repel', 'link', 'gravity', 'size'];
api.setSettings = patch => {
Object.assign(state.settings, patch);
render(false, LAYOUT_KEYS.some(k => patch && patch[k] !== undefined));
};
Regression test: and post-fix all five are Full offline gate green: 1,453 passed / 0 failed (1,467 collected, 14 skipped), ruff clean, both asset gates pass, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 038652b471
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
🔍 Pre-PR Code Analyzer Review
Verdict: REQUEST_CHANGES (contributor-count gate)
Analysis Summary
Comprehensive opt-in canvas graph engine (?graph-engine=next) with proper fallback to Classic. 2819 additions across 7 files, 48 new tests executing the asset under Node, CI green across Python 3.10/3.11/3.12 + core floor + browser smoke + CodeQL.
✅ What looks correct
- XSS fix (nodeLabel/linkLabel innerHTML sink on untrusted entity labels from ingested memories) — properly escaped, verified against four payloads at the exact accessor force-graph calls. Critical catch.
- Stack overflow — recursive Tarjan rewritten iteratively; 60k-node chain completes correctly.
- rAF leak — pause/resume driven from
selectView, realdestroy(), andGRAPH_ENGINE_PARKEDrecording state for renderers born off-view. Two independent tests pin both paths. - Betweenness — O(V·E) on main thread replaced with lazy pivot-sampled budgeted version; 60k nodes 25s→3s.
- Physics slider regression —
setSettingsnow reheats for exactly the layout-change keys Classic treats that way; appearance-only keys exempted. Regression test countsd3ReheatSimulationinvocations per key. - Lazy loading — both scripts removed from
index.html, memoized loaders, CSP drift gate extended to cover first-party scripts beyond the original three. Negative-tested. - Community colour parity —
COMMUNITY_PALSordering verified byte-identical between engine, dashboard, and CSS legend swatches. - Test harness — 48 tests executing the real asset under Node with only a bare
windowin scope; mutation-tested (removing escaping fails them).
⚠️ Required Action
- Contributor count gate — Single author (
Coding-Dev-Tools) with Claude co-authorship. Swarm policy requires ≥3 distinct agent contributors or Sentinel coordination before approval. Coordinate with Sentinel or add reviewers.
Non-blocking observations
- The
test_create_fails_loudly_when_force_graph_is_unavailableflake noted in the PR body is correctly identified as a Windows subprocess timing issue. Worth watching but not blocking. - No browser QA performed — the PR body states this plainly. Before flipping the flag for users, manual comparison at three viewports + light/dark + reduced-motion is warranted.
Automated review by Pre-PR Code Analyzer (Hermes cron)
A second knowledge-graph renderer wrapping the already-vendored force-graph, opt-in behind ?graph-engine=next. Classic stays the default and the rollback path; graphRender falls through to it on any failure. Four styles are applied CSP-safely through a data-graph-style attribute rather than inline style. Fixed while reviewing, before this shipped: - XSS. setData stamped name on every node, and force-graph's nodeLabel defaults to that accessor and renders through innerHTML. Node names are entity labels extracted from ingested memories. Classic never set name, so the sink did not exist until this feature added it. Both accessors are now explicit and escaped. - Recursive Tarjan overflowed the stack at roughly 29.6k nodes on a chain component; rewritten iteratively. - The rAF loop never stopped: a hidden canvas kept repainting for the whole session. Added pause/resume driven from selectView and a real destroy(). - Unbounded O(V*E) betweenness on the main thread with Array.shift queues, now lazy, pivot-sampled and budgeted: 60k nodes went 25s to 3s, and 0s in the default view. - prefers-reduced-motion was ignored, against an existing dashboard contract. The CSP drift gate never scanned this asset -- it only ever read index.html, dashboard.css and dashboard.js -- so "the gate passes" was vacuous for any new first-party script. Extended to hold them to the same contract and to check that every /static script index.html references actually exists. All four new rules were negative-tested. The original test asserted formatting strings. Replaced with 24 tests that execute the asset under Node with only a bare window in scope, which is itself the load-order proof, and mutation-tested: removing the escaping fails them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adding force-graph.min.js and engraphis-graph.js to index.html loaded them on every dashboard page, not just the graph view. force-graph applies inline styles at runtime, the production CSP is style-src 'self', so the browser blocked them and logged an error per attempt. Eleven pre-existing e2e tests assert a clean console and failed -- a regression this branch introduced. dashboard.js already had loadForceGraph() lazily fetching the vendor bundle for the Classic renderer, so neither script ever needed to be in index.html. Both tags are removed and engraphis-graph.js gets a matching memoized loader. The naive version of that would silently downgrade a ?graph-engine=next deep link to Classic, because graphRenderEngine's guard cannot tell "not fetched yet" from "unavailable". The engine fetch now starts alongside the vendor bundle -- one round trip, not two -- and graphRender waits on it and re-enters, so only a genuine load failure degrades, and that path warns. Proved by executing the real routing source under Node with a stub DOM: deep link renders the engine, a failing asset falls back and warns. Both are tests, and reverting to defer-without-await fails them. The "referenced scripts exist" gate rule became three, so removing the tags strengthens it rather than hollowing it out: lazy script.src references are existence-checked (their only reference is a JS string literal), neither asset may reappear as a parsed script src in index.html, and each must keep a lazy loader so deferring cannot orphan them. All negative-tested. Local gate: 1429 passed, 0 failed. The CSP fix itself cannot be proven locally -- Playwright needs node_modules, which is not installed here -- so CI is the real verification for the 11 e2e assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six findings from PR #74 review. Particles were assigned per relation with no regard for graph size, so the Cyber style with flow enabled created tens of thousands of animated particles on a large workspace. Capped against the existing large-graph signal. Community clustering added every link to the adjacency, so one sparse influences relation merged two unrelated topics into a single component and Community Islands gave them the same colour and force centre. Matched to the classic renderer's semantics. Hover changed hilite/hoverSet without invalidating the canvas, so with reduced motion on, flow off, or the simulation settled, highlight changes were invisible. The redraw is now requested. Show unlinked nodes never reached the engine: graphData() supplied degree-zero nodes while the engine's own filter still dropped them. Leaving the graph view before /graph or a lazy script resolved ran pause against a null renderer, so the pending callback could still create and start one against a hidden pane -- the rAF leak this PR fixed once already, by another route. The CSP gate parsed script tags case-sensitively, so an equivalent HTML spelling browsers load eagerly slipped past both the eager-load rejection and the existence check. Now uses the HTMLParser already in the file, which normalises case. 1441 passed, 0 failed (+12 tests). All asset gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…theme Three further findings from PR #74 review. Community IDs were assigned in raw payload order while graphRenderLegend() sorts communities by size and calls the largest "Cluster 1". Node colour indexes the palette by the ID itself, so whenever a smaller component appeared first the legend described one component with another's swatch. Components are now ranked by size before the IDs escape, matching the classic graphComputeCommunities(). Galaxy style held autoPauseRedraw(false) unconditionally, so a settled large graph repainted every node and link every frame forever. The starfield is the only paint force-graph cannot see; the classic path drops it past GPERF.large, so the same 600-node/2400-link signal now skips it and hands the redraw loop back to the vendor. Type colours resolved from hard-coded dark-theme constants, so switching to Light, Midnight, Solarized or Sepia moved the legend and controls while the canvas stayed dark. The engine cannot read CSS custom properties from a canvas, so the dashboard resolves --entity-* and supplies them through a new setThemeColors(), below style palettes and user overrides in priority. 1446 passed, 0 failed (+5 tests). ruff, externalize and manifest gates pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six findings from the PR #74 review, all cases where the opt-in renderer diverged from the classic path it is meant to replace. - Relation labels: the dashboard's Labels checkbox drives two label layers on the classic path, but the engine configured no linkCanvasObject, so ticking it painted entity names only and a relation could be read only by hovering it. Paint them, with the classic gates (zoom, dense-graph). - zoomToNode: graphFocus() treats a false return as "run the recovery path". The engine answered from raw.nodes, which keeps the coordinates force-graph left on a node, so an entity hidden by a scope filter or by the auto-collapsed cluster view still reported success and the camera moved to nothing. Expand a collapsed view, then verify against the data force-graph is actually holding. - Reseeding: render() called graphData() unconditionally with newly allocated arrays, so Style, Color by, Labels and Flow each reset the d3 alpha and restarted the layout. Reseed only when the visible set really changed, and invalidate the paint when it did not. - Cooldown: nothing overrode force-graph's 15s default simulation window, so a large store kept simulating (and repainting) far past the 1.1s/80 ticks the classic path allows. Mirror its size-dependent bounds. - Dense graphs: curvature and arrowheads stayed on past the classic GPERF.dense threshold, paying a bezier and a filled triangle per relation across thousands of links. - Community colours: the cluster legend swatches are CSS (.graph-cluster-N) encoding the Cyber palette, but the engine's Cyber and Solar palettes were ordered differently from dashboard.js, so on the default style the legend described each of the first clusters with another cluster's colour. Each is covered by a test that fails without the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Under ?graph-engine=next the Repel/Link/Gravity/Size sliders appeared dead.
dashboard.js::graphSet routes them through api.setSettings, which only asked
render() for a reheat when `mode` was present. applyForces() writes the new
charge / link / forceX-forceY / collide values straight into the simulation
force-graph is already running, and a settled graph sits at alpha~0 — so the
new forces were installed and nothing moved until the user found the Reheat
button. `size` is affected too: it feeds d3.forceCollide.
The classic branch of that same function does the opposite — it treats
`repel|link|gravity|size` as a layout change and reheats unless the user asked
for reduced motion. setSettings now matches that set (plus `mode`, which swaps
the whole force arrangement); render() already carries the reduced-motion
exemption, so it is honoured for free. Appearance-only settings (font,
link width, label density, labels, flow) still must not reheat: restarting the
layout because a label got bigger throws away the arrangement being read.
The regression test drives the API against the force-graph stand-in and counts
d3ReheatSimulation() calls per key. Pre-fix it reported
{repel: 0, link: 0, gravity: 0, size: 0, mode: 1}.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pt-in engine The engine already computed `large` (>600 nodes / >2400 links, the classic `GPERF.large` signal) and used it for cooldown, alpha decay and the galaxy starfield, but two per-frame costs the classic path drops at that cutoff were still being paid: - `applyForces()` pinned `forceCollide().iterations(2)`. The second pass is another full quadtree traversal per node per tick, and a large store pays it on the initial layout and on every reheat. Now `iterations(large ? 1 : 2)`, matching `.iterations(GPERF.large?1:2)`. - Every `rich` node still got a glow: the galaxy halo, the solar corona *and* its sphere shading, and the cyber `shadowBlur`. A gradient is a fresh object per node and a blur is a convolution of the drawn shape, both per frame. All three are now gated on `!large`, with the flat `fillStyle = col` fallback the classic path uses for the solar sphere. Both are pinned by tests that drive the asset under Node and read the recorded force/canvas calls back, and both fail against the previous asset (601 collision iterations of 2; 607 gradients painted on a large solar graph). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Node harness in tests/test_graph_engine_asset.py drives the asset against a recording force-graph stand-in, which is right for the logic and proves nothing about a browser: no CSP, no real canvas, no vendor bundle, no <script> loading. The three defects this feature shipped and then fixed were all in that gap. This adds the browser half to the Playwright suite CI already runs (.github/workflows/ci.yml runs `npx playwright test`). Five checks, each pinning a claim this PR makes: - A dashboard page that never opens the graph fetches neither graph script and reports zero CSP violations. This is the actual reason both loads are lazy. - `?graph-engine=next` fetches both assets exactly once, registers EngraphisGraph, assigns GRAPH_ENGINE, hides the degree-zero entity, and puts non-uniform pixels on a sized canvas. - Moving the Repel slider after the layout has settled changes node coordinates. Verified by mutation: narrowing LAYOUT_KEYS back to ['mode'] fails this test, which is the dead-slider regression exactly as a user would have hit it. - The opt-in renderer adds no CSP violation the classic renderer does not already cause. Opening the graph is not CSP-clean under either: force-graph injects <style> elements that `style-src 'self'` blocks (4 of them, identical on both paths, one-time at attach). That is a pre-existing vendor behaviour, and confining it to the graph view is what the lazy loading buys. What this pins is that the new engine contributes nothing on top and touches no inline style attribute. - Without the flag the graph view stays on Classic and never fetches engraphis-graph.js. The force-graph instance is reached by intercepting the vendor global from an init script rather than by adding a test-only accessor to the engine's public API. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
038652b to
1e35156
Compare
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e35156748
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b76fc69f80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6beb34bed2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daa7035f61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a472a88489
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Coding-Dev-Tools has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
* feat(dashboard): opt-in canvas graph engine behind ?graph-engine=next A second knowledge-graph renderer wrapping the already-vendored force-graph, opt-in behind ?graph-engine=next. Classic stays the default and the rollback path; graphRender falls through to it on any failure. Four styles are applied CSP-safely through a data-graph-style attribute rather than inline style. Fixed while reviewing, before this shipped: - XSS. setData stamped name on every node, and force-graph's nodeLabel defaults to that accessor and renders through innerHTML. Node names are entity labels extracted from ingested memories. Classic never set name, so the sink did not exist until this feature added it. Both accessors are now explicit and escaped. - Recursive Tarjan overflowed the stack at roughly 29.6k nodes on a chain component; rewritten iteratively. - The rAF loop never stopped: a hidden canvas kept repainting for the whole session. Added pause/resume driven from selectView and a real destroy(). - Unbounded O(V*E) betweenness on the main thread with Array.shift queues, now lazy, pivot-sampled and budgeted: 60k nodes went 25s to 3s, and 0s in the default view. - prefers-reduced-motion was ignored, against an existing dashboard contract. The CSP drift gate never scanned this asset -- it only ever read index.html, dashboard.css and dashboard.js -- so "the gate passes" was vacuous for any new first-party script. Extended to hold them to the same contract and to check that every /static script index.html references actually exists. All four new rules were negative-tested. The original test asserted formatting strings. Replaced with 24 tests that execute the asset under Node with only a bare window in scope, which is itself the load-order proof, and mutation-tested: removing the escaping fails them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dashboard): lazy-load the graph assets so the CSP stays clean Adding force-graph.min.js and engraphis-graph.js to index.html loaded them on every dashboard page, not just the graph view. force-graph applies inline styles at runtime, the production CSP is style-src 'self', so the browser blocked them and logged an error per attempt. Eleven pre-existing e2e tests assert a clean console and failed -- a regression this branch introduced. dashboard.js already had loadForceGraph() lazily fetching the vendor bundle for the Classic renderer, so neither script ever needed to be in index.html. Both tags are removed and engraphis-graph.js gets a matching memoized loader. The naive version of that would silently downgrade a ?graph-engine=next deep link to Classic, because graphRenderEngine's guard cannot tell "not fetched yet" from "unavailable". The engine fetch now starts alongside the vendor bundle -- one round trip, not two -- and graphRender waits on it and re-enters, so only a genuine load failure degrades, and that path warns. Proved by executing the real routing source under Node with a stub DOM: deep link renders the engine, a failing asset falls back and warns. Both are tests, and reverting to defer-without-await fails them. The "referenced scripts exist" gate rule became three, so removing the tags strengthens it rather than hollowing it out: lazy script.src references are existence-checked (their only reference is a JS string literal), neither asset may reappear as a parsed script src in index.html, and each must keep a lazy loader so deferring cannot orphan them. All negative-tested. Local gate: 1429 passed, 0 failed. The CSP fix itself cannot be proven locally -- Playwright needs node_modules, which is not installed here -- so CI is the real verification for the 11 e2e assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(graph): address review findings on the opt-in engine Six findings from PR #74 review. Particles were assigned per relation with no regard for graph size, so the Cyber style with flow enabled created tens of thousands of animated particles on a large workspace. Capped against the existing large-graph signal. Community clustering added every link to the adjacency, so one sparse influences relation merged two unrelated topics into a single component and Community Islands gave them the same colour and force centre. Matched to the classic renderer's semantics. Hover changed hilite/hoverSet without invalidating the canvas, so with reduced motion on, flow off, or the simulation settled, highlight changes were invisible. The redraw is now requested. Show unlinked nodes never reached the engine: graphData() supplied degree-zero nodes while the engine's own filter still dropped them. Leaving the graph view before /graph or a lazy script resolved ran pause against a null renderer, so the pending callback could still create and start one against a hidden pane -- the rAF leak this PR fixed once already, by another route. The CSP gate parsed script tags case-sensitively, so an equivalent HTML spelling browsers load eagerly slipped past both the eager-load rejection and the existence check. Now uses the HTMLParser already in the file, which normalises case. 1441 passed, 0 failed (+12 tests). All asset gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(graph): rank communities, settle large galaxy graphs, follow the theme Three further findings from PR #74 review. Community IDs were assigned in raw payload order while graphRenderLegend() sorts communities by size and calls the largest "Cluster 1". Node colour indexes the palette by the ID itself, so whenever a smaller component appeared first the legend described one component with another's swatch. Components are now ranked by size before the IDs escape, matching the classic graphComputeCommunities(). Galaxy style held autoPauseRedraw(false) unconditionally, so a settled large graph repainted every node and link every frame forever. The starfield is the only paint force-graph cannot see; the classic path drops it past GPERF.large, so the same 600-node/2400-link signal now skips it and hands the redraw loop back to the vendor. Type colours resolved from hard-coded dark-theme constants, so switching to Light, Midnight, Solarized or Sepia moved the legend and controls while the canvas stayed dark. The engine cannot read CSS custom properties from a canvas, so the dashboard resolves --entity-* and supplies them through a new setThemeColors(), below style palettes and user overrides in priority. 1446 passed, 0 failed (+5 tests). ruff, externalize and manifest gates pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(licensing): make the outage-is-not-a-cancellation test actually test that The test raised CloudSessionError(..., status=503, transient=True), but that class takes only status -- transient belongs to CloudFeatureError. So the construction raised TypeError, the caller's broad except Exception swallowed it, and getattr(exc, "status", None) was None rather than 503. The assertion therefore passed while never exercising a transport failure at all, and could not have caught the regression it exists to prevent: loosening the 402 gate so any error clears a paying customer's access. Verified by mutation -- with the gate changed to any-truthy-status the test now fails with "an outage revoked a paying customer"; before this change it passed even with that mutation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(graph): close the remaining review threads on the opt-in engine Six findings from the PR #74 review, all cases where the opt-in renderer diverged from the classic path it is meant to replace. - Relation labels: the dashboard's Labels checkbox drives two label layers on the classic path, but the engine configured no linkCanvasObject, so ticking it painted entity names only and a relation could be read only by hovering it. Paint them, with the classic gates (zoom, dense-graph). - zoomToNode: graphFocus() treats a false return as "run the recovery path". The engine answered from raw.nodes, which keeps the coordinates force-graph left on a node, so an entity hidden by a scope filter or by the auto-collapsed cluster view still reported success and the camera moved to nothing. Expand a collapsed view, then verify against the data force-graph is actually holding. - Reseeding: render() called graphData() unconditionally with newly allocated arrays, so Style, Color by, Labels and Flow each reset the d3 alpha and restarted the layout. Reseed only when the visible set really changed, and invalidate the paint when it did not. - Cooldown: nothing overrode force-graph's 15s default simulation window, so a large store kept simulating (and repainting) far past the 1.1s/80 ticks the classic path allows. Mirror its size-dependent bounds. - Dense graphs: curvature and arrowheads stayed on past the classic GPERF.dense threshold, paying a bezier and a filled triangle per relation across thousands of links. - Community colours: the cluster legend swatches are CSS (.graph-cluster-N) encoding the Cyber palette, but the engine's Cyber and Solar palettes were ordered differently from dashboard.js, so on the default style the legend described each of the first clusters with another cluster's colour. Each is covered by a test that fails without the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(graph): reheat the simulation when a physics slider moves Under ?graph-engine=next the Repel/Link/Gravity/Size sliders appeared dead. dashboard.js::graphSet routes them through api.setSettings, which only asked render() for a reheat when `mode` was present. applyForces() writes the new charge / link / forceX-forceY / collide values straight into the simulation force-graph is already running, and a settled graph sits at alpha~0 — so the new forces were installed and nothing moved until the user found the Reheat button. `size` is affected too: it feeds d3.forceCollide. The classic branch of that same function does the opposite — it treats `repel|link|gravity|size` as a layout change and reheats unless the user asked for reduced motion. setSettings now matches that set (plus `mode`, which swaps the whole force arrangement); render() already carries the reduced-motion exemption, so it is honoured for free. Appearance-only settings (font, link width, label density, labels, flow) still must not reheat: restarting the layout because a label got bigger throws away the arrangement being read. The regression test drives the API against the force-graph stand-in and counts d3ReheatSimulation() calls per key. Pre-fix it reported {repel: 0, link: 0, gravity: 0, size: 0, mode: 1}. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(licensing): settle cached access on entitlement 402 * Add `engraphis connect --token` so a purchased client can actually connect `cloud_session.save_bootstrap()` is the only function that writes `~/.engraphis/cloud_session.json`, and it had zero production callers — only tests reached it. Meanwhile `docs/AGENT_CONNECT.md` and `.env.example` told customers to prefer that file. Nothing created it, so a paying customer had no supported way to connect a client and every paid feature was undeliverable. This adds the client half of the device-connect flow that the control plane already serves: - `engraphis/device_connect.py` — token validation, stable client identity, the POST to `/v1/devices/connect`, status-keyed error copy, and the call into `save_bootstrap`. - `scripts/connect.py` — the CLI, following `scripts/init.py` conventions (`main(argv=None) -> int`, argparse, no prompts). - `scripts/entry.py` — an `engraphis` front door that dispatches to the existing `engraphis-<verb>` mains, so the portal's `engraphis connect --token ...` is a runnable command. `engraphis-connect` is installed too. Credential handling: the connect token travels in the request body and nowhere else — never printed, logged, or persisted, and never quoted back in an error. Malformed or truncated tokens are rejected before any network call, so a bad paste costs no rate budget and consumes no token. Network access uses the repo's pinned opener with redirects blocked and an explicit timeout; provider bodies are never reflected into messages. A 401 (expired / already used / invalid, which the service deliberately makes indistinguishable) stays distinct from a 402 lapsed subscription, because the fixes differ. Client ids are minted-once random ULIDs in the owner-only state directory rather than a hardware fingerprint: a MAC- or hostname-derived id is a cross-account identifier the customer never agreed to ship, and it changes under exactly the conditions where stability was the point. The command verifies `cloud_session.configured()` after writing, and says so explicitly when only the compute endpoint is missing rather than half-reporting success. Docs updated: `docs/AGENT_CONNECT.md` and `.env.example` now describe `engraphis connect --token` as the real path to the session file. Tests: 43 new, covering the happy path writing a usable session, 401 printing actionable copy and writing nothing, 402 staying distinguishable, the token never appearing in stdout/stderr or any written file, and short/malformed tokens being rejected before the network. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Pre-flight session storage before redeeming the connect token `connect()` spent the single-use connect token on the POST and only then called `save_bootstrap()`. If the state directory had become unwritable, or `cloud_session.json` had been replaced by a symlink, a hard link or a directory, the write failed after the token was already consumed: the customer was left with a spent token and no session, and had to go back to the portal for a new one. Worse, `atomic_private_text` raises `UnsafeStateFile`, which is an `OSError` and not a `CloudSessionError`, so it escaped `connect()`'s handler entirely and surfaced as a raw traceback rather than actionable copy. `cloud_session.preflight_save()` now runs before the exchange. It lives beside `_session_path()`, `_refresh_lock()` and `_save()` because that module owns the paths and the atomic write it predicts; a private copy of those rules in `device_connect` would drift from the save it is meant to model. It reuses the existing primitives -- `private_file_stat` on the session leaf and on its refresh lock, plus the randomized sibling temp file `atomic_private_text` has to create -- and never opens, creates or replaces the session leaf, so an existing session survives a failed pre-flight untouched and a first connect is not left half written. `_refresh_lock()` picks up the extracted `_refresh_lock_path()` so the lock filename is not duplicated. `device_connect._preflight_session_storage()` translates the failure into a `DeviceConnectError` that names the path to fix and states that the token is still redeemable, and `connect()` reuses the returned path for the summary instead of rebuilding it from `_state_dir()`. Tests: 6 new, all asserting `opener.calls == []` -- the POST is never issued. Session leaf and refresh lock replaced by a directory, a genuinely hard-linked session, an uncreatable state directory (`ENGRAPHIS_STATE_DIR` under a regular file), a read-only mount, and a pre-flight that creates nothing and leaves no probe file behind. All six fail against the pre-fix code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address connect review: manifest compute endpoint, timeout and save races Three unresolved review threads on #75, plus one semantic conflict the merge with main could not see. `default_compute_url` now reads the manifest's `compute_plane` and falls back to `DEFAULT_COMPUTE_URL`. The review asked for the constant to be replaced outright, but the shipped `engraphis-commercial/v2` manifest declares only `control_plane`: reading the absent key alone resolves `""`, and `cloud_session.configured()` then rejects the session a production connect just saved, leaving a paying customer "connected" but unusable. Preferring the key removes the drift point the review named -- a manifest-only endpoint change needs no code change -- while the constant stays as the fallback that keeps today's manifest working. `_validated_timeout` refuses a non-finite or non-positive timeout. `argparse`'s `type=float` accepts `nan` and `inf`, which reach the pinned opener's deadline arithmetic and raise `ValueError`/`OverflowError` from inside `urllib`, neither caught by `post_connect`. That broke this module's contract that every failure is a `DeviceConnectError`, so `engraphis connect --timeout nan` printed a traceback instead of the command's error and exit code. Checked at the top of `connect()` so a bad flag is reported as a bad flag rather than masked by whatever the identity or storage pre-flight hits first, and again in `post_connect()` because it is public and owns the socket call. `connect()` now also translates `OSError` from `save_bootstrap`. The pre-flight added in aae7d93 closes the common case, but the state directory can still change between the pre-flight and the write, and `UnsafeStateFile` is an `OSError`, not a `CloudSessionError`, so it escaped as a traceback at the worst moment. The token is spent by then, so the copy says so instead of promising a retry with the same one. The `REGISTRATION` fixture no longer claims the cloud grants `export`. #76 removed that key from the plan->feature tables in both repos because the signed compliance export was never implemented; a fixture asserting the control plane returns it would re-establish exactly the drift that removal cleaned up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Report post-redemption connect failures instead of raising tracebacks Both paths fail *after* the control plane has consumed the single-use connect token, so a raw traceback left the customer unable to tell whether to retry or fetch a new token from the portal. `http.client.IncompleteRead` (a truncated chunked reply) subclasses `HTTPException`/`ValueError` -- neither `OSError` nor `URLError` -- so it escaped both of `post_connect`'s handlers. A third clause is added *after* the transport clause, because `RemoteDisconnected` is both a `ConnectionResetError` and a `BadStatusLine` and means the peer never answered: that one keeps the retryable 503 copy, and a test pins the ordering. `save_bootstrap` re-runs `validate_cloud_base_url` on both endpoints, which resolves the host. A resolver that dies mid-connect raises `CloudUrlUnresolved` and an endpoint that starts resolving to a rejected address raises a bare `ValueError`; both are `ValueError`, so neither the `CloudSessionError` nor the `OSError` handler in `connect()` covered them. Translated in `connect()` rather than by dropping the re-validation, which is still wanted for `save_bootstrap`'s other callers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Read the trial and the access state the cloud sends, and render them `/api/license` hardcoded `"is_trial": False` and `"trial": {"used": False}`. Those were the only assignments in the file, and the client read none of `status`, `trial_ends_at`, or `trial_duration_seconds` from anywhere, so three customer-visible states were wrong or unreachable: * the dashboard's `'TRIAL'` badge branch could never be taken — a trialist saw the same confident PRO/TEAM badge a subscriber sees; * `trial.used` never became true, so "Start hosted Pro/Team trial" was offered to everyone forever, including active subscribers and customers whose trial was already spent. `start_trial` refuses any organization that already holds an entitlement, so for every connected customer that button could only ever return a `TrialAlreadyConsumedError`; and * `cloud_access_active` was computed, returned, and rendered nowhere. After a trial expired or a subscription lapsed the client kept `plan="pro"` and cleared the feature list, so the customer got a confident PRO badge over rows of locked features with no explanation of what had happened. The client now reads what the control plane already sends on the calls it already makes. `cloud_session` persists `status`, `is_trial`, `trial_consumed`, and `trial_ends_at` from the registration/refresh handshake alongside the plan fields, under the same wire names so one parser serves the saved record, the entitlements read, and the compatibility cache. Every field stays individually optional: an older control plane omits them and the honest answer is "not a trial, none consumed", never an invented one. A billing denial keeps the trial facts — a lapse does not un-consume a trial or move its boundary — and drops the status it just contradicted. `/api/license` derives one `access_state` from that: active, trial, trial_expired, lapsed, or inactive. Each is a different thing to tell the customer and a different thing to ask them to do, and the dashboard now says which: * active trial — TRIAL badge, the trial's end date, no trial CTA; * spent trial — TRIAL ENDED, "your free trial has ended, your memories are still local and usable, the trial cannot be started again", subscribe; * paying — PRO/TEAM badge, no trial CTA (a subscriber who never trialled would still be refused one); * lapsed — PRO INACTIVE, why it lapsed when the cloud named a status, and update billing; * never — LOCAL CORE, the local engine is complete on its own, and the trial CTA — the one state it can actually be started in. `plan_source`/`plan_checked_at` were emitted for support and displayed nowhere; the panel now shows which rule produced the answer and when the cloud last confirmed it. No inline styles or handlers were added; both asset gates and `node --check` pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(connect): honor cloud-assigned compute endpoints * fix(graph): trim large graph render costs * fix(review): address open client release findings * fix: close final release hardening gaps * perf(graph): apply the classic renderer's large-graph guards to the opt-in engine The engine already computed `large` (>600 nodes / >2400 links, the classic `GPERF.large` signal) and used it for cooldown, alpha decay and the galaxy starfield, but two per-frame costs the classic path drops at that cutoff were still being paid: - `applyForces()` pinned `forceCollide().iterations(2)`. The second pass is another full quadtree traversal per node per tick, and a large store pays it on the initial layout and on every reheat. Now `iterations(large ? 1 : 2)`, matching `.iterations(GPERF.large?1:2)`. - Every `rich` node still got a glow: the galaxy halo, the solar corona *and* its sphere shading, and the cyber `shadowBlur`. A gradient is a fresh object per node and a blur is a convolution of the drawn shape, both per frame. All three are now gated on `!large`, with the flat `fillStyle = col` fallback the classic path uses for the solar sphere. Both are pinned by tests that drive the asset under Node and read the recorded force/canvas calls back, and both fail against the previous asset (601 collision iterations of 2; 607 gradients painted on a large solar graph). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(graph): exercise the opt-in engine in a real browser The Node harness in tests/test_graph_engine_asset.py drives the asset against a recording force-graph stand-in, which is right for the logic and proves nothing about a browser: no CSP, no real canvas, no vendor bundle, no <script> loading. The three defects this feature shipped and then fixed were all in that gap. This adds the browser half to the Playwright suite CI already runs (.github/workflows/ci.yml runs `npx playwright test`). Five checks, each pinning a claim this PR makes: - A dashboard page that never opens the graph fetches neither graph script and reports zero CSP violations. This is the actual reason both loads are lazy. - `?graph-engine=next` fetches both assets exactly once, registers EngraphisGraph, assigns GRAPH_ENGINE, hides the degree-zero entity, and puts non-uniform pixels on a sized canvas. - Moving the Repel slider after the layout has settled changes node coordinates. Verified by mutation: narrowing LAYOUT_KEYS back to ['mode'] fails this test, which is the dead-slider regression exactly as a user would have hit it. - The opt-in renderer adds no CSP violation the classic renderer does not already cause. Opening the graph is not CSP-clean under either: force-graph injects <style> elements that `style-src 'self'` blocks (4 of them, identical on both paths, one-time at attach). That is a pre-existing vendor behaviour, and confining it to the graph view is what the lazy loading buys. What this pins is that the new engine contributes nothing on top and touches no inline style attribute. - Without the flag the graph view stays on Classic and never fetches engraphis-graph.js. The force-graph instance is reached by intercepting the vendor global from an init script rather than by adding a test-only accessor to the engine's public API. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fix the same IncompleteRead escape in cloud_session._post_refresh Found while fixing the device-connect drain: `_post_refresh` carried the identical `except (OSError, ValueError)` guard around its error-body drain, and had no `http.client.HTTPException` clause at all -- it did not even import `http.client`. Two consequences, both reproduced as failing tests first: - A refresh reply that begins and then stops mid-body makes `HTTPResponse.read` raise `IncompleteRead`, which is not an `OSError`, so it escaped `_post_refresh` as a raw traceback. This is the token-refresh path every paid feature runs through, so the crash surfaced far from its cause. - `IncompleteRead` raised while draining an `HTTPError` body escaped the drain guard and replaced the status-specific copy (401 "connect this installation again", 403, 429) with a traceback. The drain guard is now the shared `_DRAIN_FAILURES` tuple, and a new `HTTPException` clause maps a truncated reply to the existing "temporarily unreachable" copy. That clause sits deliberately *after* the transport clause so `RemoteDisconnected` -- which is both a `ConnectionResetError` and a `BadStatusLine` -- keeps the behaviour it already had. In scope for this PR: it already modifies `cloud_session.py`, and `device_connect`'s drain comment cites this function as the same shape, which was only true of the bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Say the token was spent on every post-2xx failure, and verify the session before printing success Second review round, same family as the first: a failure that can only happen after the control plane answered must never advise retrying a token that answer consumed. - `post_connect`'s oversized / unparseable / non-object body branches said only "invalid connect response". `urllib` raises `HTTPError` for every status >= 400 and `_NoRedirect` turns a 3xx into one, so reaching those branches means a 2xx -- the token is gone. They now carry the same portal guidance as the truncated-reply and missing-credential paths, via a shared `_SPENT_TOKEN_SUFFIX`. - `save_bootstrap` failing with `CloudSessionError` forwarded the bare message. That is how `_refresh_lock` reports a lock that vanished or turned unsafe after the pre-flight, and because it is a `CloudSessionError` rather than an `OSError` it bypassed the clause that explains the token was spent. It now keeps the cause *and* the guidance. - `scripts/connect.py` printed the summary before calling `cloud_session.configured()`. That call reads the session back and can raise, so a complete `--json` success object reached stdout and the command then exited 1 -- a consumer parsing stdout accepted a failed connect. The check now runs first; on failure stdout stays empty. All three reproduced as failing tests first (7 new cases). The oversized case carries an explicit `pytest.param(id=...)` because a 64 KiB parameter overflows the 32767-character limit on `PYTEST_CURRENT_TEST`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Separate connection failures from post-response failures, and fail the preflight when a probe cannot be removed Third review round. `OSError` means opposite things either side of `opener.open()`, and both call sites had them sharing one `try`. - `device_connect.post_connect` now opens and reads in separate `try` blocks. Once `open` returns, urllib has parsed a success status line, so the control plane answered and the single-use token is spent; a `TimeoutError` or `ConnectionResetError` *mid-body* was inheriting the connection-phase copy and telling the customer to retry a token that could not work. Failures before the status line keep the retry copy, which is still right for them. - `cloud_session._post_refresh` gets the same split, with sharper consequences. The rotated credential reaches disk only after the body parses, so a post-response failure leaves the *spent* credential stored. `_public_session_error` maps 503 to `transient=True`, and `CloudFeatureClient.run_job` acts on that by retrying — which resubmits the spent credential, and this module already documents that the control plane answers replay by revoking the whole credential family. Every post-response branch (truncated body, oversized, unparseable, non-object, incomplete credentials) now returns 409, the existing non-transient "connect this installation again" bucket. A test asserts the status still maps to `transient is False`, since that mapping is the thing that actually prevents the retry. This prefers a false "reconnect" over a replay: if the truncation preceded the server's commit the reconnect was unnecessary, but the opposite mistake revokes the whole family and forces the same reconnect from a worse state. - `cloud_session.preflight_save` no longer swallows a failed `os.unlink` of its probe. Creating a file and removing one are separate rights: a directory ACL granting add-file but denying delete let `mkstemp` succeed and `unlink` fail, and `atomic_private_text` finishes with `os.replace`, which needs exactly the right that just failed. The preflight passed on a directory the real save could not use, redeeming the token before failing, and littered a probe on every attempt — the precise drift the preflight exists to prevent. Ten new test cases; every one failed before its fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: close latest release review gaps * Require credential fields to actually be strings `str(response.get(key) or "").strip()` reads like a coercion but is not a validation. JSON arrays and objects arrive as Python `list`/`dict`, and `str()` renders their repr: str(["tok_abc"] or "").strip() -> "['tok_abc']" # truthy, non-empty So a control-plane reply carrying `"refresh_credential": ["tok_abc"]` passed the missing-credential check, `save_bootstrap` persisted the literal text `['tok_abc']`, `configured()` read it back as a usable session, and the CLI reported a successful connection -- while the next refresh submitted that junk and failed. By then the single-use connect token was spent, so the customer could not retry without a new one. Adds `cloud_session.text_field()`, which returns the value only when it is genuinely a string, and uses it at every untrusted-provider boundary: `save_bootstrap`, the rotation handling in `access_for_workspace`, and the pre-save check in `device_connect.connect` -- the last one via the same helper the writer uses, so the two cannot disagree about what counts as present. Identity fields (`installation_id`, `device_id`, `member_id`, `refresh_expires_at`) are dropped rather than stored as a repr, since the dashboard displays them verbatim. Six new cases covering list / dict / int / bool credentials, the writer boundary directly, and the identity fields; all failed before the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: declare version 1.1.0 Bump the five places the client version is written, all of which `tests/test_packaging.py` requires to agree: `pyproject.toml`, the `PackageNotFoundError` fallback in `engraphis/__init__.py`, the commercial manifest that `scripts/check_commercial_manifest.py` cross-checks against pyproject, and the plugin/marketplace pair under `.claude-plugin/`. Editing the two `.claude-plugin/*.json` files invalidates their entries in `.claude-plugin/skill-assets.sha256`, which `test_packaging.py` verifies by hashing the files on disk, so those two digests are regenerated here. All six files are LF, as `.gitattributes` requires. 1.1.0 rather than 1.0.2: the unreleased work adds commands and changes the managed-compute consent default, which is a minor bump. Note that 1.0.1 was declared and dated in the changelog but never tagged or published — PyPI's latest is 1.0.0 — so 1.1.0 is the first release to actually carry it. No tag is pushed by this commit; publishing stays a separate, deliberate step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(graph): honor theme and validate lazy assets * fix(graph): restore readable entity labels * fix: absorb current release review heads * fix(connect): harden ambiguous device activation failures * feat(graph): serve renderer from v2 dashboard * fix(graph): keep v2 assets lazy and themed * fix: close remaining client review gaps * test: pin ambiguous refresh timeout handling * fix(graph): bound label rendering density --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Superseded by #80, which consolidated this change with the complete audited 1.1.0 client release and is now merged into main. |
A second, canvas-based knowledge-graph renderer wrapping the already-vendored
force-graph(no new dependency — it was vendored back in872ee84). Opt-in behind?graph-engine=next; Classic stays the default and the rollback path, andgraphRenderfalls through to it on any failure.Found and fixed during review, before this shipped
XSS — introduced by this feature.
setDatastampednameon every node, and force-graph'snodeLabel/linkLabelaccessors default to"name", rendered through its float tooltip viainnerHTML. Node names are entity labels extracted from ingested memories — untrusted. Classic never setname, so the accessor resolved toundefinedand the sink did not exist until this change created it. Both accessors are now explicit and escaped, verified against four payloads at the exact accessor force-graph calls. The shipped CSP blunts it, butENGRAPHIS_CSPis operator-overridable, so it was not safe to leave.Stack overflow. Recursive Tarjan recursed once per node along a path; a chain component overflowed at ~29.6k frames. Rewritten iteratively — a 60k-node chain now completes with all bridges correct.
Runaway rAF.
autoPauseRedraw(false)plus no teardown left a hidden canvas repainting for the whole session. Addedpause()/resume()driven fromselectView, a realdestroy(), and narrowedautoPauseRedrawto the galaxy starfield only.Unbounded
O(V·E)betweenness on the main thread, withArray.shift()BFS queues. Now lazy, pivot-sampled under a work budget, with head-pointer queues: 60k nodes went 25s → 3s, and 0s in the default view.Dead physics sliders — a regression from the fix directly above it. Narrowing "re-layout on every settings change" went one step too far:
setSettingsended up reheating only whenmodewas present, andgraphSetroutes Repel/Link/Gravity/Size through it.applyForces()writes the new charge / link / forceX-forceY / collide values straight into the simulation force-graph is already running, and a settled graph sits at alpha~0 — so under the flag those four sliders installed a force that moved nothing until the user found the Reheat button.setSettingsnow reheats for exactly the set Classic calls a layout change (repel|link|gravity|size, plusmode), andrender()already carries theprefers-reduced-motionexemption. Appearance-only settings still must not reheat, which the same test pins. Now also pinned in a real browser (see below), where reverting it is what a user would actually have hit.Large-graph work the classic path had already declined to do. The engine computed the same
largesignal Classic callsGPERF.large(>600 nodes / >2400 links) and spent it on cooldown, warmup, alpha/velocity decay and the galaxy starfield — but two per-frame costs were still being paid at any size:applyForces()pinnedforceCollide().iterations(2). The second pass is another full quadtree traversal per node per tick, and a large store pays it on the initial layout and on every reheat. Nowiterations(large ? 1 : 2), matching.iterations(GPERF.large?1:2).richnode still received a glow: the galaxy halo, the solar corona and its sphere shading, and the cybershadowBlur. Classic gates all four on!GPERF.large. A blur is a convolution of the drawn shape and a gradient is a fresh object per node — hundreds rebuilt per tick at the cutoff. All four are now gated, with the flatfillStyle = colfallback Classic uses for the solar sphere.Plus
prefers-reduced-motionbeing ignored against an existing dashboard contract, several silently-dead UI paths (legend, readouts, highlight), and aMath.max(...array)RangeErroron large stores.The graph scripts are no longer loaded on every page view
index.htmlis modified by this PR (+6 lines, all comment). Bothforce-graph.min.js(177 KB) andengraphis-graph.jswere pulled out of the eager<script src>set and are now fetched on demand fromgraphRender()via memoizedloadForceGraph()/loadGraphEngine(). The reason is CSP, not weight: force-graph injects stylesheets at runtime, so under the productionstyle-src 'self'every page load that fetched it reported violations — including the pages that never open the graph.boot()'s parse-time synchronous run is preserved because the loaders are promises resolved insidegraphRender, notdeferon the tag, so a?graph-engine=nextdeep link cannot race into a silent Classic fallback.loadGraphEngine()also rejects on a 200 that never registersEngraphisGraph, since resolving there would handgraphRenderEngine()an undefined global.The CSP gate was vacuous for this file
scripts/externalize_dashboard_assets.pyonly ever readindex.html,dashboard.css, anddashboard.js— it never scanned any other first-party script, so "the drift gate passes" said nothing about a newly added one. Extended to hold first-party scripts to the same no-inline-style/handler contract, plus a check that every/staticscript the page can ask for actually exists on disk — including the lazily loaded ones, whose only reference is a JavaScript string literal that nothing else would catch on a rename. It enforces both halves of the deferral:DEFERRED_SCRIPTSmust stay out ofindex.html, and each must still have a live lazy loader, so deferring a script cannot orphan it. All new rules were negative-tested: each fails on an injected regression and passes when clean.Tests
The original test was near-vacuous (
".style." not in source, plus an exact-formatting string match). Replaced with 52 tests that execute the asset under Node with only a barewindowin scope — which is itself the load-order proof, since any top-level reach forForceGraphordocumentwould throw. Behaviour is driven through a recording force-graph stand-in and read back, rather than pattern-matched. Mutation-tested: removing.nodeLabel(...), weakeningesc(), or restoring either large-graph cost above fails the suite.tests/e2e/graph-engine.spec.js— the browser half, new in this PR. The Node harness has no CSP, no real canvas, no vendor bundle and no<script>loading, and every one of the three defects above lived in exactly that gap. Five Playwright checks, in the suite CI already runs (.github/workflows/ci.yml→npx playwright test):?graph-engine=nextfetches both assets exactly once, registersEngraphisGraph, assignsGRAPH_ENGINE, hides the degree-zero entity, and leaves non-uniform pixels on a sized<canvas>(read back withgetImageData, so a silently-failing paint path cannot pass).LAYOUT_KEYSback to['mode']fails it.style-src-elem, neverstyle-src-attr.engraphis-graph.js.The force-graph instance is reached by intercepting the vendor global from an init script, so the engine's public API gains no test-only accessor.
Offline gate on the rebased branch: 1,457 passed / 0 failed (1,471 collected, 14 skipped), ruff clean, both asset gates pass,
node --checkpasses, evals at recall@k 1.000 on both datasets and the PPR ablation unchanged (vector 0.667 / 1-hop 0.0 / PPR 1.0). Playwright: 16 passed / 0 failed (11 pre-existing + 5 new), run locally against a real Chromium.Known issues, stated plainly
<style>elements when it attaches, whichstyle-src 'self'blocks; the count and the hashes are identical on Classic and on the opt-in engine, and they fire once at attach rather than per frame. This is pre-existing vendor behaviour onmain, not something this PR introduces — what this PR changes is that the cost is now confined to the one view that asked for a graph instead of being paid on every page load. Fixing it properly means patching or replacing the vendor bundle's stylesheet injection, which belongs in its own change; check 4 above is there to make sure the new renderer never adds to the pile in the meantime.test_create_fails_loudly_when_force_graph_is_unavailableflaked once in an earlier full-suite run and has passed in every run since, including the two full runs on this rebase. It shells out tonode, so this is likely subprocess timing on Windows. Worth watching rather than trusting.setAsOf,setGhosts,setSuggestions,setScope,focus,setSizeByare tested but not yet wired to any control — API for a later change, documented as such.docs/webui-ledger-merge.mdwas deliberately not included: it reads as a PR description ("this merge", "the proposal") rather than source-of-truth documentation, and would go stale the moment the flag flips.🤖 Generated with Claude Code