Pr 24 fix#29
Conversation
Add update_check module with tests, email_outbox retention tests, and LLM config tests. Refine billing, config, service, app, dashboard_app, mcp_server, v2_api, license_cloud, llm/client, email_outbox, and inspector. Update dashboard UI (css/js/index.html), conftest, and .env.example.
![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: d0cd06cac1
ℹ️ 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".
| c.execute( | ||
| "UPDATE OR IGNORE edge_supports SET edge_id=? WHERE edge_id=?", | ||
| (collision["id"], ed["id"]), | ||
| ) | ||
| c.execute("DELETE FROM edge_supports WHERE edge_id=?", (ed["id"],)) | ||
| c.execute("DELETE FROM edges WHERE id=?", (ed["id"],)) |
There was a problem hiding this comment.
When a source edge collides with a target edge during a normal workspace merge, this moves the normalized edge_supports rows but then deletes the source edge without merging its edges.provenance. The surviving edge consequently still reports only the destination edge's memory_ids/source metadata, even though it now has evidence from both workspaces. This leaves the stored provenance inconsistent for legacy consumers and exports; merge the edge-level provenance as Store._deduplicate_live_edges() does before deleting the duplicate.
Useful? React with 👍 / 👎.
| db_dir = os.path.dirname(os.path.abspath(settings.db_path)) | ||
| if db_dir: | ||
| candidates.append(os.path.join(db_dir, ".engraphis_update_check.json")) | ||
| except Exception: # noqa: BLE001 - config unavailable/misconfigured → temp dir | ||
| pass | ||
| candidates.append(os.path.join(tempfile.gettempdir(), "engraphis_update_check.json")) | ||
| return candidates[0] if candidates else None |
There was a problem hiding this comment.
For deployments whose configured DB directory exists but is read-only (for example, an immutable application directory), this always returns that first candidate rather than trying the temp candidate. _read_cache then stays empty and _write_cache silently fails, so every later dashboard bootstrap schedules another update request and the reminder never gets a usable cache. Probe/write the DB-adjacent location before selecting it, or retry the temp path after cache I/O fails.
Useful? React with 👍 / 👎.
…p fastapi in core-floor The team-invite endpoint now requires invite_url (PR #29 change), but ~12 tests still omitted it, causing 400 responses where other status codes were expected. Added invite_url to all affected test payloads. Also added pytest.importorskip('fastapi') to the two test_update_check API tests that import v2_api, fixing the core-floor (numpy-only, py3.9) job which has no fastapi installed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13ed1f5d9e
ℹ️ 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".
| new_src = entity_remap.get(ed["src"], ed["src"]) | ||
| new_dst = entity_remap.get(ed["dst"], ed["dst"]) | ||
| new_repo = _new_repo(ed["repo_id"]) |
There was a problem hiding this comment.
For symmetric relations such as related, source and destination edges are canonicalized by their original entity IDs in Store.upsert_edge(). If the source IDs sort in the opposite order from the target IDs, remapping produces the reverse (src, dst) pair here, so the collision query misses the target edge and the merge leaves two live copies of the same undirected relation. This occurs for overlapping entities with independently generated IDs and persists until a later process startup happens to run _deduplicate_live_edges().
Useful? React with 👍 / 👎.
| # Merge evidence: re-point supports, skip duplicates. | ||
| c.execute( | ||
| "UPDATE OR IGNORE edge_supports SET edge_id=? WHERE edge_id=?", | ||
| (collision["id"], ed["id"]), | ||
| ) |
There was a problem hiding this comment.
When both colliding edges have a live support with the same (memory_id, source_kind), this UPDATE OR IGNORE leaves the source support in place because of the live-support unique index, and the following delete removes it. Consequently, a higher source confidence (and its support provenance) is discarded in favor of the target value; graph-scene confidence filtering and evidence ordering can then understate or hide the merged relation. Merge duplicate support rows as the store's deduplication routine does instead of ignoring them.
Useful? React with 👍 / 👎.
| c.execute("DELETE FROM edge_supports WHERE edge_id=?", (ed["id"],)) | ||
| c.execute("DELETE FROM edges WHERE id=?", (ed["id"],)) |
There was a problem hiding this comment.
For a collision between otherwise identical live edges, the source row is deleted without incorporating its weight, so a source relation with a higher weight is silently downgraded to the destination edge's lower weight. Weighted graph recall consumes Edge.weight, and the existing edge deduplication path explicitly retains the maximum weight; apply the same merge rule here before deleting the source edge.
Useful? React with 👍 / 👎.
The send_team_invite client function defaults invite_url='' which now fails validation. Pass an explicit invite_url in the roundtrip and 402 tests. Also add invite_url to the two security-hardening rate-limit tests that were getting 400 (missing field) instead of 429 (rate limit).
…w-required field The team_invite endpoint now requires invite_url (returns 400 without it). Four tests still omitted it, causing CI failures on PR #29: - test_send_team_invite_client_roundtrip - test_send_team_invite_client_reports_reason_on_402 - test_m2_team_invite_rate_limits_invalid_key_flood - test_m2_invite_and_register_share_one_burst_budget All tests pass locally after adding the missing field.
…ixes from origin/pr-24-fix)
|
Closing: this PR is based on a pre-#24 state and its diff against current main would REVERT already-merged work (removes CI workflows, update_check, email_outbox retention, service.py merge-dedup). The only net-new content is 2 additional invite_url test additions, which are trivial and can be landed separately. All CI passed but the merge would be destructive. |
1 participant
Description
Type
Verification
python -m pytest tests/ -qpassesruff check .passespython -m eval.harness --dataset eval/datasets/sample.jsonl --k 5passes