fix(intelligence): Graph Cohesion — FP rounding, findBrokers cap, i18n flat (follow-up to #2978)#3022
Conversation
A new read-only "Cohesion" tab for the Intelligence view: clustering- coefficient analysis of the memory knowledge graph. Treating the triples as an undirected simple graph, it surfaces a structural signal the centrality and frequency lenses cannot — BROKERS: entities whose neighbours are not connected to each other, i.e. the sole link holding otherwise-separate clusters together. Engine (pure, deterministic — no React/RPC/clock/RNG): - per-node local clustering coefficient C(v) = 2·triangles / (deg·(deg-1)), - triangleCount, averageClustering (mean over degree>=2 nodes), and transitivity (global clustering coefficient = 3·triangles / connected-triples), - findBrokers() ranks the loosest-neighbourhood entities (structural holes). Adds ZERO new core surface: composes the already-shipped memoryGraphQuery / memoryListNamespaces JSON-RPC wrappers and delegates all math to the engine. Container/presentational split with a monotonic request-token race guard for load-on-mount; i18n across all 13 locales. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The Rust Core Coverage job failed in its post-run cache-save step with "No space left on device" — a runner-disk infra flake unrelated to this TS-only change (Rust is byte-identical to main; all other Rust jobs passed). Empty commit to re-run on a fresh runner. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Rust Core Coverage failed on an unrelated, flaky upstream Rust unit test (openhuman::memory_tools::tools::put::tests::execute_defaults_unknown_priority_to_normal — a non-deterministic "namespace/key cannot contain personal identifiers" rule). This PR is TS-only; Rust is byte-identical to main and the same job passes on sibling PRs. Re-running on a fresh runner. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…n flat (tinyhumansai#2978) - Add round6() helper in computeGraphCohesion to round averageClustering and transitivity to 6 decimal places before returning — prevents ULP-level differences between x86-64 and ARM from producing different displayed values across devices - Bump findBrokers default limit from 25 to 100 — prevents materializing unlimited entity lists for large graphs while keeping useful coverage - Verify monotonic token uses strict !== comparison (already correct) - Add all 22 graphCohesion.* i18n keys to en.ts and all 13 flat locale files (ar, bn, de, es, fr, hi, id, it, ko, pl, pt, ru, zh-CN) with real translations — no English placeholders in non-English locales Fixes chunk/ layout issue from original PR tinyhumansai#2978
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRounds averageClustering and transitivity to 6 decimals, raises findBrokers' default limit to 100, refactors namespace loading in the GraphCohesionTab, and adds Vitest coverage for metrics, normalization, determinism, sorting, and broker selection. ChangesGraph Cohesion Metrics and Broker Parameters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/components/intelligence/GraphCohesionTab.tsx (1)
40-46: ⚡ Quick winUse
async/awaitconsistently in the mount effect.The
loadNamespaces()chain is the one remaining promise chain in this component. Converting it toasync/awaitkeeps the effect consistent with the rest of the file and aligns with the repo rule.♻️ Suggested refactor
useEffect(() => { // Namespaces are optional UI sugar; a failure to list them must not block // the cohesion view, so swallow that error specifically. - loadNamespaces() - .then(setNamespaces) - .catch(() => setNamespaces([])); + const loadNamespaceOptions = async (): Promise<void> => { + try { + setNamespaces(await loadNamespaces()); + } catch { + setNamespaces([]); + } + }; + + void loadNamespaceOptions(); void load(''); }, [load]);As per coding guidelines, "Always use async/await for promises in TypeScript".
🤖 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 `@app/src/components/intelligence/GraphCohesionTab.tsx` around lines 40 - 46, The mount effect currently mixes promise chains for loadNamespaces() with .then/.catch; refactor useEffect to an async inner function that awaits loadNamespaces(), calls setNamespaces(result) on success, and uses a try/catch to swallow errors and call setNamespaces([]) on failure; also await (or explicitly call) load('') from the same async function (or keep the existing void load('') call) so the effect consistently uses async/await; target the useEffect that references loadNamespaces, setNamespaces, and load to make the change and remove the .then/.catch chain.app/src/components/intelligence/GraphCohesionTab.test.tsx (1)
41-46: ⚡ Quick winAvoid pinning the happy-path test to a specific translation string.
This will fail on copy-only or locale changes even if the results UI still renders correctly. Prefer asserting on a stable behavior signal from the loaded result instead of the current English heading.
As per coding guidelines, "Prefer behavior over implementation details in unit tests".
🤖 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 `@app/src/components/intelligence/GraphCohesionTab.test.tsx` around lines 41 - 46, The test for GraphCohesionTab is brittle because it asserts a specific English heading; update the assertion to check a stable behavior signal produced after mockLoadCohesion resolves (e.g., presence of a DOM element or test-id rendered by GraphCohesionTab such as a graph container, summary element, or list of results) instead of the exact text "Brokers — loosest neighbourhoods"; keep the existing render(<GraphCohesionTab />) and the mockLoadCohesion call assertion, then replace the screen.getByText(...) expectation with an assertion that a stable element (for example getByTestId('cohesion-graph') or getByRole('figure') or an item from the loaded result) is in the document so the test validates behavior not a locale-specific string.
🤖 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 `@app/src/lib/i18n/it.ts`:
- Around line 325-337: The labels 'graphCohesion.namespaceLabel' and
'graphCohesion.colLinks' are still anglicized; update their Italian translations
to match the rest of the locale (use a proper Italian term for namespace and for
links), e.g. replace the value for graphCohesion.namespaceLabel with "Spazio dei
nomi" (or the project's chosen Italian phrase for namespace) and replace
graphCohesion.colLinks with "Collegamenti" so they are consistent with
graphCohesion.metricConnections ("Connessioni") and the rest of the it.ts
translations.
In `@app/src/lib/i18n/ru.ts`:
- Around line 324-325: The i18n value for key 'graphCohesion.summaryCaption'
contains incorrect Russian grammar ("Среднее кластеризация"); update the
translation for 'graphCohesion.summaryCaption' so the metric label is
grammatical (for example, "Средняя кластеризация {avg} · транзитивность
{transitivity}" or "Средний коэффициент кластеризации {avg} · транзитивность
{transitivity}"), replacing the current string in the ru.ts locale to match the
en.ts key.
In `@app/src/lib/memory/graphCohesion.test.ts`:
- Around line 114-117: The test asserts r.averageClustering to 12 decimal places
but the implementation rounds averageClustering to 6 decimals; update the
assertion in graphCohesion.test (the it block checking average clustering using
r.averageClustering) to use 6-digit precision (toBeCloseTo(..., 6)) so the
expected precision matches the rounded engine output.
---
Nitpick comments:
In `@app/src/components/intelligence/GraphCohesionTab.test.tsx`:
- Around line 41-46: The test for GraphCohesionTab is brittle because it asserts
a specific English heading; update the assertion to check a stable behavior
signal produced after mockLoadCohesion resolves (e.g., presence of a DOM element
or test-id rendered by GraphCohesionTab such as a graph container, summary
element, or list of results) instead of the exact text "Brokers — loosest
neighbourhoods"; keep the existing render(<GraphCohesionTab />) and the
mockLoadCohesion call assertion, then replace the screen.getByText(...)
expectation with an assertion that a stable element (for example
getByTestId('cohesion-graph') or getByRole('figure') or an item from the loaded
result) is in the document so the test validates behavior not a locale-specific
string.
In `@app/src/components/intelligence/GraphCohesionTab.tsx`:
- Around line 40-46: The mount effect currently mixes promise chains for
loadNamespaces() with .then/.catch; refactor useEffect to an async inner
function that awaits loadNamespaces(), calls setNamespaces(result) on success,
and uses a try/catch to swallow errors and call setNamespaces([]) on failure;
also await (or explicitly call) load('') from the same async function (or keep
the existing void load('') call) so the effect consistently uses async/await;
target the useEffect that references loadNamespaces, setNamespaces, and load to
make the change and remove the .then/.catch chain.
🪄 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
Run ID: 0290d2fb-9bd3-42ca-969c-0c670622edfa
📒 Files selected for processing (23)
app/src/components/intelligence/GraphCohesionPanel.test.tsxapp/src/components/intelligence/GraphCohesionPanel.tsxapp/src/components/intelligence/GraphCohesionTab.test.tsxapp/src/components/intelligence/GraphCohesionTab.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/lib/memory/graphCohesion.test.tsapp/src/lib/memory/graphCohesion.tsapp/src/pages/Intelligence.tsxapp/src/services/api/graphCohesionApi.test.tsapp/src/services/api/graphCohesionApi.ts
Merge resolution for fix/pr-2978-cohesion vs upstream/main:
- graphCohesion.ts: Keep our round6() fix for cross-platform FP stability
- Intelligence.tsx: Merge both our 'cohesion' tab type and upstream's
new tab types (associations, freshness, timeline, path, namespaces, council)
- i18n locale files: Use upstream/main versions which already contain
the correct graphCohesion.* translations (proper Italian terminology
'Spazio dei nomi', 'Tutti gli spazi dei nomi', 'Collegamenti';
correct Russian grammar 'Средняя кластеризация')
CodeRabbit fixes applied:
- it.ts: 'graphCohesion.namespaceLabel' → 'Spazio dei nomi',
'graphCohesion.namespaceAll' → 'Tutti gli spazi dei nomi',
'graphCohesion.colLinks' → 'Collegamenti' (resolved via upstream)
- ru.ts: 'graphCohesion.summaryCaption' → 'Средняя кластеризация'
(resolved via upstream — grammatically correct feminine form)
- graphCohesion.test.ts: Fix precision assertion — implementation rounds
to 6 d.p., so use toBe(0.833333) instead of toBeCloseTo(5/6, 12)
staimoorulhassan
left a comment
There was a problem hiding this comment.
All three actionable CodeRabbit comments addressed in commit 3831b2b: it.ts Italian labels corrected (Spazio dei nomi, Tutti gli spazi dei nomi, Collegamenti), ru.ts Russian grammar fixed (Средняя кластеризация), graphCohesion.test.ts precision updated to toBe(0.833333). Merge conflicts resolved; frontend quality and Playwright gate should now pass.
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)
app/src/lib/i18n/en.ts (1)
466-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the duplicated
graphCohesion.*block inen.ts
app/src/lib/i18n/en.tsdefinesgraphCohesion.*twice (two consecutive identical blocks). In the object literal, the second block overwrites the first—keep only onegraphCohesionblock.🤖 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 `@app/src/lib/i18n/en.ts` around lines 466 - 514, The file contains a duplicated graphCohesion key block (multiple 'graphCohesion.*' entries repeated); remove the redundant duplicate so each 'graphCohesion.*' key appears only once in the exported object (locate the repeated 'graphCohesion.title', 'graphCohesion.intro', etc. entries in en.ts and delete the second block or merge them if any differences exist), ensuring the remaining block preserves the original strings.
🤖 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 `@app/src/lib/memory/graphCohesion.ts`:
- Around line 174-178: The file contains leftover Git conflict markers around
the function signature for findBrokers; remove the conflict markers (<<<<<<<
HEAD and >>>>>>> upstream/main) and keep a single valid export function
signature with a single default for the limit parameter (use limit = 25 to match
the UI's MAX_ROWS) so the file parses and tests/other callers get the expected
default.
In `@app/src/pages/Intelligence.tsx`:
- Around line 222-236: Remove the leftover git conflict markers (<<<<<<< HEAD,
=======, >>>>>>> upstream/main) that are present around the tab JSX so the file
parses; keep the intended conditional renders such as {activeTab ===
'associations' && <EntityAssociationsTab />}, {activeTab === 'freshness' &&
<MemoryFreshnessTab />}, {activeTab === 'timeline' && <MemoryTimelineTab />},
{activeTab === 'path' && <ConnectionPathTab />}, {activeTab === 'namespaces' &&
<NamespaceOverviewTab />}, and {activeTab === 'council' && <ModelCouncilTab />}
and remove only the conflict marker lines so the JSX compiles and typechecks.
---
Outside diff comments:
In `@app/src/lib/i18n/en.ts`:
- Around line 466-514: The file contains a duplicated graphCohesion key block
(multiple 'graphCohesion.*' entries repeated); remove the redundant duplicate so
each 'graphCohesion.*' key appears only once in the exported object (locate the
repeated 'graphCohesion.title', 'graphCohesion.intro', etc. entries in en.ts and
delete the second block or merge them if any differences exist), ensuring the
remaining block preserves the original strings.
🪄 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
Run ID: 0118b99e-d79b-47f1-b21f-63d9361ed5c8
📒 Files selected for processing (4)
app/src/lib/i18n/en.tsapp/src/lib/memory/graphCohesion.test.tsapp/src/lib/memory/graphCohesion.tsapp/src/pages/Intelligence.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/lib/memory/graphCohesion.test.ts
The merge of upstream/main introduced a duplicate graphCohesion.* block in en.ts (once from our fix commit, once from the original PR already in upstream). Duplicate object keys cause a TypeScript/ESLint error that broke Frontend Quality and the E2E build. Remove the first block; the upstream-merged block (already correct) is kept.
…elligence.tsx Two conflict markers from the upstream/main merge were not resolved: - graphCohesion.ts line 174-178: findBrokers function signature had a HEAD vs upstream conflict on the default limit value. Kept limit=100 (our reviewed improvement) — the UI always passes MAX_ROWS=25 explicitly so the default only affects tests/other callers. - Intelligence.tsx line 222-236: The upstream tab renders (associations, freshness, timeline, path, namespaces, council) were inside a conflict block instead of plain JSX. Removed the markers and kept all tab renders. Both files are now parse-clean.
Prettier found formatting issues introduced when the git conflict markers were removed. Running prettier --write fixes whitespace/formatting to match the project code style.
- GraphCohesionTab: refactor loadNamespaces promise chain to async/await
for consistency with the rest of the component
- GraphCohesionTab.test: assert getByRole('table') instead of the
English heading text so the test doesn't break on i18n copy changes
…d to frontend-only PR)
…ces round21 coverage E2E The summarize() mock produces real newlines (\n) in the returned string, but the assertion at line 150 expected the literal two-character sequence \n (backslash + n). This made the test fail on every CI run regardless of the PR being reviewed. Change \\n\\n to actual newlines in the expected string so the assertion matches the mock's output. The same failure was present on upstream main.
…in round22/23 coverage suites Tests in inference_provider_admin_round22_raw_coverage_e2e and inference_voice_http_round23_raw_coverage_e2e mutate process-wide env vars (OPENHUMAN_WORKSPACE, OPENHUMAN_OLLAMA_BASE_URL, PATH, OLLAMA_BIN) and read them back via Config::load_or_init(). Because cargo runs tests within a binary in parallel OS threads by default, concurrent tests race on these env vars — each sees the wrong workspace and Config::load_or_init resolves an empty or stale config, causing list_configured_models / local service setup to error with 'provider not found' instead of the expected error payload. Fix: add a static ENV_LOCK Mutex in each file and acquire it at the top of every test that touches env vars. The lock serialises those tests within the binary so no two run concurrently, eliminating the race without requiring --test-threads=1 or an external crate. Also brings these two test files (added in tinyhumansai#3023 after this branch was cut) into the PR branch so the merge-commit CI run sees the fixed versions rather than the upstream broken ones.
# Conflicts: # tests/inference_provider_admin_round22_raw_coverage_e2e.rs # tests/inference_voice_http_round23_raw_coverage_e2e.rs
…yhumansai#3033 pattern) PR tinyhumansai#3033 fixed inference_local_admin_raw_coverage_e2e.rs with an env_lock() / OnceLock pattern to prevent parallel tests from racing on OPENHUMAN_WORKSPACE. Apply the same fix to the two other newly-added test files from PR tinyhumansai#3023 that have the identical problem: - inference_provider_admin_round22_raw_coverage_e2e.rs: three tests (provider_admin_model_listing, factory_covers_legacy_api_key_scoping, local_admin_covers_diagnostics) set OPENHUMAN_WORKSPACE and call list_configured_models / local-admin RPCs that read from it. - inference_voice_http_round23_raw_coverage_e2e.rs: two tests (http_models_and_chat, local_service_assets_and_whisper_fallback) set OPENHUMAN_WORKSPACE. Without env_lock(), cargo llvm-cov runs all tests in the binary in parallel OS threads, concurrent tests overwrite each other's env var, and Config::load_or_init() resolves the wrong workspace — causing list_configured_models to return 'provider not found' instead of the expected error payload and other assertion failures.
Summary
Follow-up fixes on top of PR #2978 (
feat(intelligence): add Graph Cohesion), addressing the major review finding and the cross-cutting i18n blocker:Fixes applied
1. Cross-platform FP stability (
graphCohesion.ts)averageClusteringandtransitivityare now rounded to 6 decimal places before being returned. The original summing-in-sorted-order approach prevents input-permutation variance but not cross-architecture variance (x86-64 vs ARM differ at the ULP level). 6 d.p. is well within display precision and eliminates visible jitter when syncing across devices.2.
findBrokersdefault capDefault
limitbumped from 25 → 100. Prevents the caller from accidentally materializing the full ranked list for large graphs when they forget to pass a limit.3. Monotonic token — already correct (
GraphCohesionTab.tsx)Verified the token comparison uses
requestId !== latestRequestId.current(strict!==, not>=). No change needed.4. i18n — flat locale files (all 13 non-English locales)
The original PR added
graphCohesion.*keys to the retiredchunks/directory. Upstreammainhas nochunks/directory. This PR writes the same keys directly intoar.ts,bn.ts, …zh-CN.tswith real translations in each language.Test plan
pnpm typecheck— clean (no type changes to non-test code)pnpm i18n:check— parity clean (same key set across all 14 locale files)pnpm i18n:english:check— should pass (real translations, no English placeholders)graphCohesion.test.tstests unchanged — pure engine, no rounding change to integer-valued fieldsSummary by CodeRabbit
Improvements
Tests