Skip to content

fix(ingestion): ingest non-existent absolute-looking strings as text [SDK-234] (gh #3887, follow-up to #3892)#4155

Merged
Vasilije1990 merged 2 commits into
devfrom
feature/sdk-234-windows-posix-path-ingestion
Jul 24, 2026
Merged

fix(ingestion): ingest non-existent absolute-looking strings as text [SDK-234] (gh #3887, follow-up to #3892)#4155
Vasilije1990 merged 2 commits into
devfrom
feature/sdk-234-windows-posix-path-ingestion

Conversation

@rshkarin

@rshkarin rshkarin commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

Follow-up to #3892 (now merged to dev), closing the remaining half of #3887.

#3892 stops the Windows ValueError crash by guarding the absolute-path branch of save_data_item_to_storage with is_absolute(). This PR fixes the other half of the report: a /-prefixed string that is not an actual file (e.g. a text note like /remember to call Bob about the meeting) should be ingested as text, not turned into a file:// URI pointing at a file that does not exist.

The absolute-path branch converted unconditionally, whereas the relative-path branch right below it already required the file to exist (abs_path.is_file()). I made the two consistent by reusing that same, already-computed abs_path: an absolute-looking string is only turned into a file:// URI when it points at an existing file — otherwise it falls through to text ingestion, on every platform. The ACCEPT_LOCAL_FILE_PATH=false rejection of existing local files is unchanged.

While tracing the callers I found the dry-run estimator (cognee/modules/cognify/estimator.py) deliberately mirrors this routing, and it was raising "file does not exist" for missing absolute paths. Since a real run now treats those as text, I updated _path_candidate to match (missing path → raw text; the gate rejects only existing absolute files) and adjusted its two tests.

Behavior note: because the branch now keys on "existing file" (is_file()), a handful of absolute-looking strings that are not regular files also route to text instead of a file:// URI — an existing directory path, a broken symlink, and pathological paths that error during resolve() (embedded null / over-length). This is intended and safe: none of those are ingestible files, and directories are normally expanded upstream by resolve_data_directories before reaching here. It also repairs a latent downstream break — before this change a /-prefixed note produced a broken file:// URI that get_data_file_path turned into a non-existent path and the loader then failed on.

#3892 has merged to dev; this PR is rebased on top of it and now contains only the follow-up commits.

Acceptance Criteria

  • save_data_item_to_storage("/remember to call Bob about the meeting") is ingested as text — no file:// URI, no crash — on Windows, macOS and Linux.
  • An existing absolute file path still converts to a file:// URI; with ACCEPT_LOCAL_FILE_PATH=false it still raises IngestionError.
  • The dry-run estimate prices a missing absolute path as text instead of erroring out.
  • The regression tests run on the regular (Linux) unit-test CI, not only the Windows OS matrix. Verified locally: 38 passed, 1 skipped (the genuine drive-anchored Windows case); ruff format + ruff check clean.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Code refactoring
  • Other (please specify):

Pre-submission Checklist

  • I have tested my changes thoroughly before submitting this PR (See CONTRIBUTING.md)
  • This PR contains minimal changes necessary to address the issue/feature
  • My code follows the project's coding standards and style guidelines
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if applicable)
  • All new and existing tests pass
  • I have searched existing PRs to ensure this change hasn't been submitted already
  • I have linked any relevant issues in the description
  • My commits have clear and descriptive messages

DCO Affirmation

I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.

@github-actions

Copy link
Copy Markdown
Contributor

Hello @rshkarin, thank you for submitting a PR! We will respond as soon as possible.

Comment thread cognee/modules/cognify/estimator.py Dismissed
Comment thread cognee/tasks/ingestion/save_data_item_to_storage.py Dismissed
Comment thread cognee/modules/cognify/estimator.py Fixed
Comment thread cognee/modules/cognify/estimator.py Fixed
Comment thread cognee/tasks/ingestion/save_data_item_to_storage.py Fixed
@rshkarin
rshkarin force-pushed the feature/sdk-234-windows-posix-path-ingestion branch 4 times, most recently from 9303e8e to cc6db4e Compare July 22, 2026 08:58
@rshkarin
rshkarin force-pushed the feature/sdk-234-windows-posix-path-ingestion branch from cc6db4e to 1b4595c Compare July 23, 2026 08:23
dexters1 added a commit that referenced this pull request Jul 23, 2026
…r [SDK-307] (#4193)

## What & why

The **Code Quality** CI job (`uv run ty check .`) is currently **red on
every PR**, including already-merged ones — the check is broken
repo-wide, not by any individual PR.

**Root cause:** `cognee/infrastructure/loaders/core/image_loader.py`
imports Pillow (`PIL`) for its optional EXIF-metadata and
perceptual-hash features. The imports are guarded at runtime with
`try/except ImportError` (graceful degradation), so Pillow is genuinely
optional — but it is **not** installed by the Code Quality job (which
runs only `uv sync --extra dev`; Pillow reaches the env only
transitively via the `unstructured` extra). Because
`[tool.ty.src].include` covers `cognee/infrastructure/loaders`, ty
type-checks that file and emits **5 `unresolved-import` diagnostics**
for `PIL` / `PIL.ExifTags`.

**Evidence it's pre-existing and repo-wide:** Code Quality = FAILURE on
#4155, #4156, #4134 and the already-merged #4153; `image_loader.py` and
the ty config are byte-identical across the last ~34 dev commits.

## Change

Add an inline `# ty: ignore[unresolved-import]` at each of the 5
optional `PIL` import sites — matching the existing convention in the
repo (e.g. `litellm_instructor/.../azure_openai/adapter.py`). These
imports are intentionally optional and already runtime-guarded, so
suppressing the *static* unresolved-import there is the correct
semantics. **No dependency or lockfile change**, and the `try/except`
guards are untouched.

Alternative considered: add Pillow to the `dev` extra so ty can actually
resolve it (real type coverage of the PIL path) — heavier (lockfile
change) and inconsistent with how other optional imports are handled;
left as a possible later improvement.

## Verification (local, ty resolving against a full dev env with Pillow
absent)

- `ty check .`: **5 diagnostics → 0** ("All checks passed!").
- `ruff check` and `ruff format --check`: clean.

Linear: **SDK-307**. Unblocks the Code Quality gate for all open PRs.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@rshkarin
rshkarin force-pushed the feature/sdk-234-windows-posix-path-ingestion branch from 1b4595c to fdc6897 Compare July 23, 2026 09:01
@rshkarin
rshkarin marked this pull request as ready for review July 23, 2026 09:01
@rshkarin
rshkarin force-pushed the feature/sdk-234-windows-posix-path-ingestion branch from fdc6897 to 638aeda Compare July 23, 2026 10:16
rshkarin and others added 2 commits July 23, 2026 15:23
…[SDK-234]

Follow-up to #3892 (gh #3887). save_data_item_to_storage converted any
absolute-looking string to a file:// URI even when no such file existed, so a
"/"-prefixed text note (e.g. "/remember to call Bob") became a broken file://
URI on POSIX — and, before #3892, crashed on Windows. Gate the absolute-path
branch on the file actually existing (reusing the already-sanitized abs_path),
mirroring the relative-path branch: a "/"-prefixed string that is not an
existing file now falls through to text ingestion on every platform. The
ACCEPT_LOCAL_FILE_PATH rejection of existing local files is preserved.

Mirror the same rule in the dry-run estimator (_path_candidate), which predicts
ingestion routing: a non-existent absolute path is raw text, and the gate
rejects only existing absolute files.

The regression tests now run on the Linux/mac CI (not only the Windows matrix):
non-existent absolute path -> text; Windows-style normalization -> text without
raising (the module's os reference is swapped, pathlib left untouched); and the
existing-file gate-off rejection. A genuine drive-anchored Windows path stays a
Windows-only test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up hardening from an adversarial verification pass. Two test-only
additions to lock in behavior the fix changed:

- existing directory (absolute) -> text ingestion (is_file() is False, so it
  no longer becomes a file:// URI). Documents the deliberate divergence from
  the dry-run estimator, which rejects directories with a clearer message.
- non-existent absolute path with ACCEPT_LOCAL_FILE_PATH off -> still text
  (the gate rejects only existing local files), completing mirror-parity with
  the estimator's gate-off assertion.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rshkarin
rshkarin force-pushed the feature/sdk-234-windows-posix-path-ingestion branch from 638aeda to f3b33bb Compare July 23, 2026 13:23
@Vasilije1990
Vasilije1990 merged commit 0b5252d into dev Jul 24, 2026
186 of 192 checks passed
@Vasilije1990
Vasilije1990 deleted the feature/sdk-234-windows-posix-path-ingestion branch July 24, 2026 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants