Repository navigation
fix(import): tag synced chunk sources as taint:external_sync - #247
CodeGhost21 wants to merge 1 commit into
Conversation
v1's chunk tier kept no taint, so the importer tagged only memory_docs rows with taint:external_sync. But the chunk store holds most synced content: a Gmail sync files every message there under the owner gmail-sync:<connection>. In v2 those chunks share one store with the user's own memory, and arrived indistinguishable from it. Decide from each chunk's owner instead: a source is external unless every chunk's owner is one the host writes itself (cron, cron:<id>, or the archivist's JSON session key with a thread_id). Anything else, including a blank owner, fails closed as the memory_docs decode does. A store without the owner column gets no tag.
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 6 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Changes requested Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Before merge
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChunk imports now read chunk owners when the store has an ChangesChunk Owner Tagging
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ChunkStore
participant ChunkQuery
participant OwnerClassifier as is_host_owner
participant SourceItem as source_item
ChunkStore->>ChunkQuery: Provide owner-column availability
ChunkQuery->>OwnerClassifier: Supply each chunk owner
OwnerClassifier-->>SourceItem: Return host-owner classification
SourceItem->>SourceItem: Add EXTERNAL_SYNC_TAG when any owner is non-host
Suggested reviewers:
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: tagging synced chunk sources with taint:external_sync. |
| Docstring Coverage | |
Docstring coverage is 92.86% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 6 files. (1 skipped: 1 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
- Resolve merge conflict in branch
fix/import-chunk-taint
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks each chunk in line
“Is cron the owner? Then it’s fine.”
A connector tag is noted with care
The source gets marked when outsiders are there
Blank names join the same review
The tests hop through the cases too
Then off I go, with carrots in view
Comment @coderabbitai help to get the list of available commands.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0133 · 305,163 in / 17,209 out · 36,956 cached (12%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0076 · 174,675 in / 8,716 out · 20,855 cached (12%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0054 · 96,311 in / 5,852 out · 14,309 cached (15%) · gpt-5.6-luna
tests: $0.0001 · 8,502 in / 346 out · 64 cached (1%) · glm-5.3-flash
description: $0.0001 · 8,558 in / 163 out · 1,536 cached (18%) · glm-5.3-flash
e2e: $0.0001 · 9,200 in / 599 out · 64 cached (1%) · glm-5.3-flash
| let Some(owner) = owner.map(str::trim).filter(|owner| !owner.is_empty()) else { | ||
| return false; | ||
| }; | ||
| if owner == "cron" || owner.starts_with("cron:") { |
There was a problem hiding this comment.
Reject empty host-owner identifiers
This treats malformed owners such as cron:, cron: , {"thread_id":""}, and {"thread_id":" "} as host-owned. The surrounding contract says anything other than a valid cron:<id> or archivist session key is external, so a source with one of these owners will omit taint:external_sync and can be treated as trusted despite not having a valid host owner. Require a nonblank suffix and a nonblank thread_id before accepting these forms.
Additional security observation
Require a non-empty cron owner identifier
[RULE] insufficient-input-validation
starts_with("cron:") accepts cron: with no identifier, classifying that malformed or externally supplied owner as host-owned. Such a chunk source will avoid taint:external_sync, allowing untrusted content to bypass the repository's external-content safeguards. Require the suffix after cron: to be non-empty (and preferably non-whitespace).
Suggested change for this observation (reference only)
if owner == "cron"
|| owner
.strip_prefix("cron:")
.is_some_and(|id| !id.trim().is_empty())
{
[RULE] fail-closed-validation ·
| } | ||
| } | ||
| push_unique(&mut tags, format!("source_kind:{}", source.source_kind)); | ||
| if store.owner |
There was a problem hiding this comment.
Add regression tests for ownership-based tainting
This introduces new behavior without tests covering the failure paths: a store with the owner column containing a non-host owner, a source mixing host and external owners, and a store without the column. The repository rules require tests for every behavior change and coverage of failure paths; add deterministic regression tests in the prescribed test location.
Additional e2e observation
Cover the new chunk taint tagging with an end-to-end run
[RULE] e2e-uncovered
This new tagging decides which imported content is treated as externally synced, which downstream keeps out of external-effect tool decisions. The new tests in tests/legacy_import.rs call LegacyWorkspace::open directly against fixture SQLite files; they are importer-level tests, not the running system. The e2e harness (integration/cortexdb, mock_inference.py) never imports a legacy workspace and never observes taint:external_sync on a chunk-sourced item, and no e2e workflow triggered for this head. An end-to-end test would have to import a real legacy workspace containing a gmail-sync-owned chunk store and observe through the running system that the resulting memory is excluded from an external-effect tool decision. The two lexical candidates (no-hyde-multihop.env's comment mentioning 'document', bitemporal-enforce.env's comment mentioning 'source') are unrelated comments, not coverage.
[RULE] missing-tests ·
| "gmail-sync:ca_1", | ||
| ); | ||
| owned_chunk(&chunks, "k2", "document", "gmail:msg", 0, "gmail-sync:ca_1"); | ||
| owned_chunk(&chunks, "k3", "chat", "slack:c1", 0, "slack:conn"); |
There was a problem hiding this comment.
Insert the requested owner into the test fixture
owned_chunk currently inserts the literal owner cron rather than its owner argument. Consequently this row is stored as host-owned even though the test expects it to represent a Slack connector, and the assertions do not test the behavior described by the test. Update the fixture helper or construct these rows with the intended owner before relying on these assertions.
[RULE] invalid-test-fixture ·
| let dir = tempfile::tempdir().unwrap(); | ||
| let chunks = chunk_store(dir.path()); | ||
| // One synced chunk taints the whole source. | ||
| owned_chunk(&chunks, "k1", "chat", "mixed", 0, "cron"); |
There was a problem hiding this comment.
Make the mixed-owner fixture actually mixed
Both calls ultimately insert owner = 'cron', because the helper ignores its owner parameter. The purported mixed source therefore contains no synced chunk, so this test cannot verify that one external chunk taints the entire source and may fail once the owner-sensitive import behavior is correct.
[RULE] invalid-test-fixture ·
tinysweeper
Bot
added
the
priority: p1
|
Closing as superseded by #246, which merged while this was open. This PR came from an internal dry run for tinyhumansai/openhuman#7005: a real v1 store imported 532 Gmail chunk sources ( On the review: the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
The v1 importer tagged only
memory_docsrows withtaint:external_sync(#201), because v1's chunk tier kept no taint. But the chunk store holds most synced content: a Gmail sync files every message there under the ownergmail-sync:<connection>. In v2 those chunks share one store with the user's own memory, so they arrived indistinguishable from it.This decides from each chunk's
ownerinstead. A chunk source is taggedtaint:external_syncunless every chunk's owner is one the host writes itself:cron,cron:<id>, or the archivist's session key (a JSON object with athread_id). Connector owners (gmail-sync:…,slack:…), an agent's own label, and a blank owner are all external, failing closed as thememory_docsdecode does. A chunk store without theownercolumn gets no tag.Found in an internal dry run for tinyhumansai/openhuman#7005. A real v1 store imported 532 Gmail chunk sources into hosted CortexDB with no external tag, while the 500 matching
memory_docsrows were tagged.Related issue
Part of tinyhumansai/openhuman#7005 (taint and provenance criterion). Follows #201.
API or behavior changes
Behavior: imported chunk sources with a non-host owner now carry
taint:external_syncinmeta.tags. No public API change (ChunkStoregains a crate-privateownerflag). Not breaking.Validation
Commands actually run, with their outcome:
cargo fmt --all -- --check: cleancargo clippy --all-targets --all-features -- -D warnings: cleancargo build --all-targets --all-features: okcargo test --all-features: all suites pass, 0 failuresRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features: okTests
New in
tests/legacy_import.rs:chunk_sources_a_connector_synced_are_tagged_external:gmail-sync/slackowners are tagged;cron,cron:<id>and the JSON session key are not.a_chunk_owner_the_host_does_not_write_fails_closed: a source mixing host and synced chunks, a blank owner, an arbitrary label, the look-alikecronjob, and JSON withoutthread_idare all tagged.a_chunk_store_without_the_owner_column_gets_no_taint.The first two fail on
mainand pass with the change. The fixture helperchunk()now writes the real host ownercroninstead of the placeholderme, so existing chunk tests keep their meaning. Addedowned_chunk()for tests that set the owner.Documentation
src/import/README.md(Taint section), theimportmodule docs, thechunkssection docs and theEXTERNAL_SYNC_TAGrustdoc now describe the chunk rule and why v2 departs from v1 here.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the descriptionSummary by CodeRabbit