fix: batch dedupe must ignore TTL expiry (zero-loss); scope depends_on edges on task create (security) - #190
Conversation
…n edges on task create (security) Two follow-ups from a cross-cutting audit. Zero-loss: existing_source_ids() backed batch idempotency on _load_active_rows, which hides TTL-expired rows. Once an item's forget_after passed, its source_id left the dedup set and a re-POST re-ingested it, writing a second archive/vector row every time. Dedup now reads all physically-present rows (new include_expired path); superseded rows stay excluded so cleared content still re-adds, and recall-time TTL behavior is unchanged. Security: POST /tasks with depends_on skipped _enforce_edge_project_scope, letting a project-scoped token create cross-project blocks edges and enumerate foreign task existence. Each depends_on id is now scope-checked against the bound project with the same non-enumerating 403 as the edge endpoints. Tokenless/standalone unchanged.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change preserves expired physical rows for batch deduplication while keeping them hidden from recall, and adds project-scope validation for ChangesTTL deduplication
Task dependency project scope
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_ttl_filter.py (2)
313-315: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert archive deduplication too.
This test only verifies the vector table. The PR contract also requires no duplicate archive row; assert the agent’s conversation archive still contains exactly one record after the second POST.
🤖 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 `@tests/test_ttl_filter.py` around lines 313 - 315, Extend the test after the vector_memory count assertion to query the conversation archive table and assert it contains exactly one record for the same id after the second POST, verifying archive deduplication alongside physical-row deduplication.
318-339: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover superseded rows explicitly.
Add a source-ID row, supersede it, and assert its ID is absent. This ensures
include_expired=Truedoes not accidentally make intentionally cleared rows block future re-imports.🤖 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 `@tests/test_ttl_filter.py` around lines 318 - 339, Extend test_existing_source_ids_includes_expired_rows to add a source-ID row, supersede or clear it using the store’s existing supersession API, then assert its ID is absent from existing_source_ids(agent="a"). Preserve the existing assertions for expired and active rows so intentionally superseded rows are excluded while expired rows still dedupe.
🤖 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.
Nitpick comments:
In `@tests/test_ttl_filter.py`:
- Around line 313-315: Extend the test after the vector_memory count assertion
to query the conversation archive table and assert it contains exactly one
record for the same id after the second POST, verifying archive deduplication
alongside physical-row deduplication.
- Around line 318-339: Extend test_existing_source_ids_includes_expired_rows to
add a source-ID row, supersede or clear it using the store’s existing
supersession API, then assert its ID is absent from
existing_source_ids(agent="a"). Preserve the existing assertions for expired and
active rows so intentionally superseded rows are excluded while expired rows
still dedupe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a96ca555-cf8b-47ad-b9b9-778f134f8b52
📒 Files selected for processing (5)
CHANGELOG.mdtaosmd/http_server.pytaosmd/vector_memory.pytests/test_http_server_registry_auth.pytests/test_ttl_filter.py
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # project's tasks. Foreign and nonexistent ids yield the identical | ||
| # 403 so task existence cannot be probed. Unbound (tokenless / | ||
| # standalone) requests pass through untouched. | ||
| if project is not None and depends_on: |
There was a problem hiding this comment.
WARNING: Scope guard also fires for tokenless requests that include a project in the body, contradicting the PR's "tokenless/standalone unchanged" claim.
project here is the value returned by _apply_token_binding, which for a tokenless request is just the caller-supplied body project (not a token-bound value). So a standalone client that names project in the body and supplies a cross-project depends_on will now be rejected with 403, whereas before this PR (and unlike _handle_task_add_edge, which forces project=None for tokenless so it never enforces) such requests succeeded. This is inconsistent with the sibling edge endpoints and the stated guarantee. Consider gating the guard on actual token binding rather than merely project is not None.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # standalone) requests pass through untouched. | ||
| if project is not None and depends_on: | ||
| for dep_id in depends_on: | ||
| if not self._enforce_edge_project_scope(project, dep_id, dep_id): |
There was a problem hiding this comment.
SUGGESTION: Two minor issues in this loop.
- N+1 query: each
dep_idtriggers a separatetask_projects(...)lookup. The edge endpoints batch both ids into a singletask_projects([from_id, to_id])call; collapsing the wholedepends_onlist into one lookup (then checking each result) avoids O(N) DB round-trips. - Input validation:
depends_onis only checked to be alist, not that its elements are strings. A non-string element is passed straight intotask_projectsand yields a misleading 403 ("task not available in the token's project scope") instead of a 400. Consider validatingisinstance(dep_id, str)and raising_BadRequest.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| out: set[str] = set() | ||
| for row in self._load_active_rows(search_agents=[agent] if agent else None): | ||
| for row in self._load_active_rows( | ||
| search_agents=[agent] if agent else None, include_expired=True |
There was a problem hiding this comment.
SUGGESTION: Re-POST with a refreshed (future) forget_after is now skipped, so TTL cannot be extended via idempotent re-ingest.
Because existing_source_ids now reports expired-but-present rows, a caller who re-POSTs the same source_id with a new future forget_after gets skipped (idempotency) and the old row stays expired/hidden from recall. This is the intended zero-loss behavior, but it means re-ingest can't revive or extend TTL on an existing id — callers needing to refresh expiry must use a new id or an explicit update path. Worth calling out in docs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 8a84b8c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8a84b8c)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Reviewed by hy3-20260706:free · Input: 59.3K · Output: 9.5K · Cached: 295.9K |
Review FIX-FIRST on #190: the depends_on guard keyed on the project returned by _apply_token_binding, which for a tokenless caller is the body-supplied tag, not a token binding. A tokenless caller tagging its task one project while depending on a task in another got a 403 where master returned 200 -- a real standalone regression, contradicting the "tokenless unchanged" claim and diverging from the /edges handlers, which derive scope purely from the token. _apply_token_binding now surfaces the verified project_id claim on self._token_project (reset per call; None for tokenless/global). The guard enforces only when token_project is not None, mirroring /edges. No second authorize()/grants pass. The tokenless test now uses a genuinely cross-project dep and asserts 200, proving standalone is truly unrestricted.
| # (``token_project is None``) pass through untouched. | ||
| if token_project is not None and depends_on: | ||
| for dep_id in depends_on: | ||
| if not self._enforce_edge_project_scope(token_project, dep_id, dep_id): |
There was a problem hiding this comment.
SUGGESTION: Two issues remain in this per-dependency loop (carried from the prior review; the tokenless scope bug it raised was fixed by keying on token_project).
- N+1 lookup: each
dep_idtriggers its owntask_projects([dep_id, dep_id])round-trip (see_enforce_edge_project_scopeat line 1494). The sibling edge endpoints pass both ids to a singletask_projects(...)call; collapsing the wholedepends_onlist into one lookup (then checking each result) avoids O(N) DB round-trips. - Input validation:
depends_onis only checked to be alist(line 1354), not that its elements are strings. A non-string element is passed straight into_enforce_edge_project_scope(which expectsstr) and, sincetask_projectswon't match a non-string key, yields a misleading 403 ("task not available in the token's project scope") instead of a 400. Validateisinstance(dep_id, str)and raise_BadRequest.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Two follow-ups from a full cross-cutting audit of the tree (both confirmed by @taOS-dev on the bus, with fix directions). TDD: failing test first, watched it fail for the right reason, then fixed.
Bug 1 (major, zero-loss + broken batch idempotency) —
taosmd/vector_memory.pyThe earlier
forget_afterwork taught_load_active_rowsto hide TTL-expired rows from recall. Butexisting_source_ids()(the set that makesPOST /ingest/batchidempotent) is built on_load_active_rows, so once a batch item'sforget_afterpassed, itssource_iddropped out of the dedup set. Re-POSTing the same id then re-ingested it, writing a second archive and vector row on every re-POST — unbounded duplication of hidden-but-still-present content, a zero-loss violation.Fix: dedup now reads all physically-present rows via a new
include_expiredpath on_load_active_rows. A TTL-expired row is only hidden from recall; it still exists on disk, so its id still dedupes. Superseded rows (valid_toset) stay excluded from the set — intentionally-cleared content should re-add on re-import — and recall-time TTL behavior is untouched (search still hides expired rows).Bug 2 (major, security scoping gap) —
taosmd/http_server.py_handle_task_createCreate was the one graph-mutating path that skipped
_enforce_edge_project_scope. It applied token binding, then passeddepends_onstraight intocreate_task, which makes ablocksedge with no scope check. A project-scoped registry token could therefore create cross-project edges and enumerate foreign task existence (real id → 200, bogus id → 400 naming the missing task).Fix: when a token binds a project, each
depends_onid is checked to belong to that project beforecreate_task, returning the same non-enumerating 403 the edge endpoints use (foreign and nonexistent ids indistinguishable). Tokenless/standalone unchanged.Tests
tests/test_ttl_filter.py:test_ingest_batch_expired_item_still_dedupes(re-POST an expired id → ingested=0/skipped=1, one physical row),test_existing_source_ids_includes_expired_rows.tests/test_http_server_registry_auth.py: cross-projectdepends_on→ 403; foreign vs bogus → identical 403 (non-enumerating); same-project → success; tokenless → unchanged.Full suite: 1086 passed.
Summary by CodeRabbit