fix(review): 16-agent release-readiness review and hardening - #131
Merged
Conversation
Store/Schema: - Transaction rollback safety on Windows (commits_deferred flag) - Skip redundant index.upsert when index shares store vector table - Embedding-space contract validation and direct SQL matrix path - Restore orphaned Store methods (prompt_eligibility_counts, embedding_space_health, context_savings_grouped, add_sync_bytes, get_sync_stats) and harden _logical_digest for sqlite-vec - Dashboard startup self-check for orphaned Store methods - v11 handoff column migration for sessions table Release readiness: - Strip dead TEAM scope surface (Scope.TEAM, MemoryRecord.team_id, SearchFilter.team_id/caller_id) per AGENTS.md §0 - Remove team_id column from memories table (team_members retained) - Remove conflict_aware profile (zero callers/tests/docs) - Add format/group_by params to MemoryService.context_savings() Other: - watch_repo key name, LLM fallback chain cost tracking - context_savings_grouped SELECT columns - Sync robustness, response budgets, docs sync
- MCP tool engraphis_context_savings: add format and group_by params - v2 API route /context-savings: add format and group_by query params - Read-only API /context-savings: add format and group_by query params - MCP HTTP CLI: minor fix
- eval/extractor_quality.py: offline extractor distillation quality harness - eval/handoff_quality.py: structured session-handoff eval - eval/datasets/handoff_quality.jsonl: handoff scenario fixtures - eval/datasets/sample.jsonl: 3 new entries (link_symbol, hierarchical consolidation, extractor/handoff patterns) - scripts/watch_repo.py: file-watcher for code-graph reindexing - scripts/validate_compose_contract.py: schema contract validator
- dashboard_app.py: pass workspace to stats() when allowed_workspaces is configured (prevents ValidationError on workspace-bound instances) - Sync classic_assets and static bundles with latest dashboard changes
- test_cli_entrypoints: clear ENGRAPHIS_WORKSPACES env for subprocess isolation - test_dashboard_security_headers: monkeypatch.delenv ENGRAPHIS_WORKSPACES - test_savings: adjust for context_savings format/group_by signature - test_sync: align with sync robustness hardening - test_update_check: align with 2-part semver acceptance - test_mcp_server, test_smart_mcp_gateway: align with MCP surface wiring - test_store_class_integrity: new Store method integrity coverage - test_dashboard_v2, test_release_infrastructure, test_secret_hygiene: minor alignment with pro-feature changes
The restored method from b7e80c2 predated the hash-validation contract in _public_receipt_row. Without prev_hash and receipt_hash in the SELECT, every receipt was marked invalid_payload and excluded from aggregation, causing the grouped method to return 0 groups despite valid receipts. Verified: all 4 dimensions (workspace/repo/agent/day) now return correct token aggregation. Savings ratio: 0.6 (180 saved / 300 source). Found by round-3 ContextSavingsGroupedRuntime scout.
Save JSON reports to a directory with timestamped filenames. Cherry-picked from codex/fix-pr-104-codeql-v4 (a4928c5).
… preservation, retention SQL, migration safety, port parsing - llm/client.py: _LLMProviderError accepts *args + message kwarg (fixes TypeError on cost-ceiling path) - llm/client.py: parse_provider_chain detects URL ports via :// prefix check (prevents port-as-ceiling stripping) - core/sync.py: quarantine merge preserves prior reasons alongside inherited_quarantine (audit trail) - stores/vectors.py: upsert_memory preserves existing memory_type when caller passes default semantic - routes/memory.py: AVG(stability) SQL approximation for retention stats (avoids OOM on large workspaces) - routes/vault.py: hashlib suffix on smart-import split doc_ids (collision prevention) - routes/vault.py: health_overview and find_stale bounded to 10k limit - service.py: Windows-safe two-step rename with staging file and rollback for v1->v2 migration - stores/__init__.py: graph backfill uses _replace_support_rows + per-namespace rebuild (O(N) not O(N²)) - config.py: embed_dim downstream consumers use 'is not None' pattern - 6 downstream embed_dim call sites fixed (dashboard_app, mcp_server, read_only_api, inspector, v2_api, embedder) - eval/longmemeval_v2.py: _stored_memory_type_counts scoped to workspace - eval/harness.py: --output-dir writing inside try/except for clean CLI errors - KILO_CODE_INTEGRATION.md: tool count corrected to nine; engraphis_forget row added - .env.example: ENGRAPHIS_SYNC_TOKEN_ORIGIN documented - Test file trailing newlines restored
…verage Closes P1 gap flagged by FinalA_Tests review: - port-stripping fix (localhost:8080 vs ceiling) — 5 cases - multi-provider chain parsing with mixed ceilings - empty/whitespace fallback to default chain - _LLMProviderError positional+kwarg construction contract
… harness improvements - .github/workflows/release.yml: CodeQL config-file reference added - .env.example: ENGRAPHIS_SYNC_TOKEN_ORIGIN documented for standalone sync tokens - docs/KILO_CODE_INTEGRATION.md: Smart tool count corrected to nine; engraphis_forget row added to Classic table - engraphis/dashboard_assets/vendor/d3.min.js: unified across classic/static (no new Function) - eval/longmemeval_v2.py: _stored_memory_type_counts scoped to workspace - eval/harness.py: --output-dir writing inside try/except for clean CLI errors - tests/e2e/demo.spec.js, test_dashboard_vendor_assets.py, test_documentation_contracts.py: new coverage - deploy/force-graph license/yarn lock files added - docs/benchmark-evidence offline fixtures added - Multiple doc updates: AGENTS.md, BENCHMARKS.md, CHANGELOG.md, README.md, SECURITY.md, SYNC.md - Demo infrastructure: screen demo HTML, prepare script, record script - Integration updates: hermes plugin, pi MCP client - Skills: engraphis-memory SKILL.md and references updated
- vault.py: _filename_stem() prevents Path() on untrusted filenames; path containment uses normcase comparison with preserved casing - force-graph.min.js: all 3 vendor copies synced to CSP-modified build (sha256 5b71387a); manifest hash aligned - engraphis-graph.js: finishNodeDrag pins node at drop position in live mode; endManualDrag uses inline pin instead of stale function ref - classic_assets/index.html: added missing graph-show-all button - dashboard.js (both copies): force-graph cache-buster ?v=20260809-csp; graph engine version aligned to physics-guard - ledger.js: graph engine version aligned to physics-guard - dashboard_assets/index.html: ledger.js version aligned; CSS tag preserved at connected-memories - Tests updated: drag test expects pin-preservation, manual drag test assertions aligned with refactored engine, dashboard v2 version assertions corrected, vault security tests verified
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: feae8b0dda
ℹ️ 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".
- Restore finishNodeDrag, reheatLiveLayout, setDragSimulationBudget with proper release-on-drop semantics for live graphs (frozen/static retains pins) - softReheat accepts dragging param; alpha target adapts to drag vs settings context - render() passes dragging flag through to physics path - E2E tests: release assertions (fx/fy undefined for live), bounded movement, API method guards for optional vendor methods - Unit tests: version strings aligned to physics-guard, drag contract assertions match restored functions - 174 tests pass across all affected suites
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd48ced77d
ℹ️ 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".
- vault.py: add post-resolve symlink containment re-verification (defense-in-depth for CodeQL py/path-injection alert #107) - hosted_ledger.py: _load() now passes owner_only=True to read_private_text(), enforcing POSIX owner-only permissions - force-graph license report: bundle.sha256 aligned to CSP-modified vendor hash (5b71387a) from upstream (1008539b) - ledger.js: restore canonical '/ owner / month' and '/ owner / year' billing-unit strings for manifest contract compliance - graph engine: conditional drag handler registration with typeof guards - E2E and unit tests aligned with refactored drag lifecycle
Plan prices must match the commercial manifest billing_unit contract. Test asserts '/ owner / month' and '/ owner / year' substrings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 148b13251f
ℹ️ 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".
…alignment - engraphis-graph.js: restore reheatLiveLayout, setDragSimulationBudget, softReheat(dragging) with DRAG_ALPHA_TARGET/DRAG_SETTLE_DELAY_MS constants; render() passes dragging flag through physics path; finishNodeDrag uses setDragSimulationBudget(false) before reheatLiveLayout(false) - vault.py: reject symlinks explicitly in rglob loop; operate only on lexical descendants of trusted folder (rglob does not recurse through symlink directories) - dashboard.js (classic/static): version tag physics-guard; health view uses /analytics/health endpoint with decay_distribution and conflict_frequency contracts - ledger.js: version tag physics-guard - index.html: version tag physics-guard - Tests: dashboard v2 version assertions aligned; graph engine drag contract tests match restored functions; pro CTA test validates dynamic plan name template
- Add source_vaults, source_imports, source_import_items tables with scope-security triggers (v14/v15 schema migration) - Add _warn_if_db_empty_with_populated_sibling() startup diagnostic - Update documentation version references (schema 13→15) - Add EngraphisGraph proxy in e2e tests for velocity visibility - Restore drag physics constants (DRAG_ALPHA_TARGET, etc.) - Unify version tags to physics-guard across all assets - Update documentation contract tests for schema 15
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77b6b39b02
ℹ️ 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".
…kage Review thread #22: when an unexpected MCP failure includes a credential, private path, or memory content in its exception text, the previous implementation wrote that value and its traceback verbatim to the server log. Log only the exception type; the full traceback remains available via exc_info for operator debugging but the message body is redacted.
Instead of hardcoding 'memory_mentions', use the relation from the code_memory_links row when building bridge edges in _load_bounded_code_graph. Falls back to 'memory_mentions' only when no relation is stored.
- Add reason parameter to link_symbol service method with secret capture rejection - Include reason in audit log detail string - Include reason in receipt metadata and return dict - Add reason parameter to engraphis_link_symbol MCP tool - Mirrors provenance pattern from link() method
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f636b19bf
ℹ️ 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".
Move _recover_stale_graph_jobs() inside the transaction so recovery, active-job check, and job insert are atomic. When caller owns the transaction, recovery correctly reuses it without nested BEGIN/COMMIT.
- Add _PollingWatcher.untrack() so failed reindex files re-appear next cycle - Polling loop calls untrack() on reindex failure instead of dropping changes - Watchdog handler retries failed callbacks up to 3 times with backoff - Tests cover untrack reappearance and unknown-path tolerance
…st (#21) The partial unique index idx_code_mem_live_unique already exists and handles the race condition via INSERT OR IGNORE. This commit: - Adds the unique index to SCHEMA_SQL for explicit documentation - Adds test_link_symbol_concurrent_creation_is_idempotent to verify that concurrent link creation produces exactly one row - All link-related tests pass (5 passed, 1 skipped)
Add test_import_folder_accepts_files_under_any_configured_root to verify that when ENGRAPHIS_IMPORT_ROOTS contains multiple disjoint paths outside home, files under ANY configured root are accepted. The original bug (any(not _path_within_root(...))) required files to be under ALL roots simultaneously, which is impossible for disjoint paths. Fix was applied in 11f9a92; this test prevents regression. Refs: PR #131 review thread #20
…10) When max_response_tokens forces truncation of a grounded answer, fit_text with citation_safe=True selects candidates ending at citation boundaries. However, if the answer fits within budget after shrinking citation bodies but still has trailing text after the last citation marker, it was returned as-is. Add a post-processing step after all budget truncation that ensures grounded answers always end with a [n] citation marker. If no citation marker exists in the answer, the result is converted to an abstention (grounded=False, abstained=True). Also add a regression test verifying that trailing uncited text is stripped from grounded answers under a tight token budget.
Companion to commit 8291e00. Twenty threads race to create the same symbol-memory link; the unique partial index on code_memory_links ensures exactly one row survives. Guards against regressions that weaken the INSERT OR IGNORE contract.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 984f1cbb03
ℹ️ 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".
The polling watcher was hashing every file under node_modules, vendor, build outputs, and other trees that the code indexer prunes via _DEFAULT_EXCLUDE_DIRS and .engraphisignore. Apply the same pruning at scan time so os.walk never descends into excluded trees, eliminating sustained disk/CPU waste on repos with large generated directories. Review thread #25.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78c96c0caa
ℹ️ 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".
The previous commit referenced parse_engraphisignore, which is not exported by engraphis.backends.codegraph. The actual function name is load_ignore_patterns and accepts a str root path. Fixes test collection failure in test_cli_entrypoints.py.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
16 parallel review agents audited all modified files on this branch.
All findings resolved and verified with 174 passing tests.
Changes
Security
path containment uses normcase comparison with preserved casing;
3 new tests covering edge cases and traversal rejection
Asset Consistency
(sha256 5b71387a); manifest hash aligned
index.html, both dashboard.js copies
Graph Engine Fixes
at drop position () instead of clearing ,
matching the documented intent that manual placement persists until
explicit unfreeze
with inline pin logic (function was removed during refactor)
classic and static dashboard.js
(was only in )
Test Alignment
(removed references to deleted , )
Verification
,