Skip to content

GEN-02: add bounded local artefact text extraction - #1346

Merged
Chris0Jeky merged 20 commits into
mainfrom
issue-1316/artefact-text-extraction
Jul 13, 2026
Merged

GEN-02: add bounded local artefact text extraction#1346
Chris0Jeky merged 20 commits into
mainfrom
issue-1316/artefact-text-extraction

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Summary

  • add the first-match IArtefactTextExtractor contract with bounded plaintext/Markdown and PdfPig PDF text-layer adapters
  • persist append-only, user-scoped extraction history with deterministic latest selection, warning provenance, LF normalization, and UTF-16 span offsets
  • include complete extraction history in bounded and streaming data exports; prove source/account deletion cascades remove extraction records
  • add the artefact extraction seam to autodoc/AGENT_INDEX.md

Implementation notes

  • AC5 decision ? size-bounded synchronous service: GEN-02 exposes an explicit application service but does not wire extraction into upload or an HTTP request. Repository content is copied through an extractor-specific bounded stream (1 MiB plaintext; 10 MiB PDF). A later worker can invoke the same service without changing the persisted contract.
  • PDF extraction is local and deterministic: PdfPig 0.1.15 (Apache-2.0), strict parsing, 100-page cap, 51,200-character cap, 64-level parser stack cap, text layer only. Image-only/scanned PDFs produce no-text-layer; there is no OCR or LLM path.
  • Extractor exceptions persist a content-free extractor-error warning and log only the exception type, not parser detail or user content.
  • Re-extraction appends history. Store-time active-user and source-ownership checks close delete/deactivation races; latest uses CreatedAt then Id ordering.
  • Extracted text is normalized once to LF and measured with .NET UTF-16 indexes. Invalid surrogate contracts are rejected before persistence/export.
  • This PR is stacked on GEN-01: add artefact storage foundation #1341 at corrected base b149bd49; merge the dependency first and do not retarget/delete the stacked branch.

Assumption: the issue's "size-bounded sync" option authorizes an explicit bounded application service without an upload hook. Reason: GEN-02 defines extraction and persistence, while downstream invocation belongs to the worker/review flow. Reversible by invoking IArtefactExtractionService from a worker without a schema or extractor change.

Verification

  • dotnet test backend/Taskdeck.sln -c Release -m:1 --no-restore --filter FullyQualifiedName~ArtefactExtraction ? 31 passed, 0 failed
  • dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --no-restore --filter FullyQualifiedName~MigrationBootstrap ? 7 passed, 0 failed
  • dotnet test backend/Taskdeck.sln -c Release -m:1 --no-restore ? 6,896 passed, 0 failed, 1 pre-existing INV-09 skip
  • node scripts/check-docs-governance.mjs ? passed
  • node scripts/check-golden-principles.mjs ? passed
  • git diff origin/issue-1315/artefact-storage...HEAD --check ? passed
  • dotnet list backend/Taskdeck.sln package --vulnerable --include-transitive ? surfaced the pre-existing unpatched SQLite advisory now tracked in Track SQLite native dependency CVE-2025-6965 until a patched package exists #1345; PdfPig introduced no reported advisory

Docs impact

autodoc/AGENT_INDEX.md now points to the extraction seam and verification command. Canonical direction sections in docs/STATUS.md / the active memory were intentionally not edited here because the operative revival/generalist direction is landing through #1296 and #1328.

Risks / not yet verified

  • Strict PdfPig parsing intentionally converts malformed/unsupported PDFs into reviewable warnings rather than attempting lenient recovery.
  • GitHub CI, fresh independent FULL-tier review, and post-push bot review rounds are pending.
  • The transitive SQLite native package has no patched NuGet release as of this review; remediation is tracked in Track SQLite native dependency CVE-2025-6965 until a patched package exists #1345.

Closes #1316

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

FULL-tier self-review ? exact head c3e3e4c1

I reviewed the complete stacked diff against #1316, the corrected #1341 dependency head, migration/data-retention behavior, cross-user isolation, resource bounds, export/deletion completeness, and all current PR comments/reviews. There were no existing comments or review submissions at review time.

HIGH

  1. Stored content could be materialized before an extractor enforced its input cap. A future higher storage limit could turn extraction into avoidable memory pressure. Fixed in 6d63b5ac: every extractor declares an input-byte limit; orchestration rejects oversized metadata before reading and copies through a limit-enforcing stream. Regression tests cover declared and actual-size overruns.

MEDIUM

  1. Parser exceptions could disclose user-derived details through logs. Fixed in ed79f888: persisted warnings remain content-free and logs now record only exception type plus artefact/extractor identifiers.
  2. A misbehaving future extractor could persist invalid UTF-16 or crowd out required contract warnings. That could break JSON portability or hide truncation/contract failures. Fixed across ed79f888 and 32c7ab82: invalid surrogate sequences are rejected, system warnings are priority-preserved within the cap, and domain persistence enforces the same Unicode boundary.

LOW

  1. A hard-coded PdfPig version could drift after a dependency update. Fixed in c3e3e4c1: extraction provenance derives the loaded assembly's three-part version; the real PDF fixture still verifies 0.1.15 at this head.
  2. The initial empty-PDF fixture did not prove scanned/image-only behavior. Fixed in 6d63b5ac: the test now builds a real image-only PDF and asserts no-text-layer with empty text.

No self-review finding remains unresolved.

Verification

  • focused extraction filter: 31 passed / 0 failed
  • migration bootstrap: 7 passed / 0 failed
  • serialized full backend suite: 6,896 passed / 0 failed / 1 pre-existing INV-09 skip
  • docs governance: passed
  • golden principles: passed
  • stacked diff check: passed
  • dependency audit: PdfPig added no reported advisory; the pre-existing unpatched SQLite native advisory is tracked in Track SQLite native dependency CVE-2025-6965 until a patched package exists #1345

Residual review focus: adversarial PDF resource behavior inside PdfPig, append-history/export race bounds, migration rollback/cascade semantics, and final CI/bot results. This comment does not claim merge eligibility.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

@codex review

Please perform a fresh FULL-tier adversarial review of exact head c3e3e4c. Focus on PDF parser/resource bounds, warning honesty, user isolation and store-time races, migration/cascade deletion, bounded and streaming GDPR export, Unicode/offset discipline, and all existing comments. Report every severity.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a local, size-bounded text extraction feature for plain-text, Markdown, and PDF artefacts, including database persistence for extraction history and integration with the GDPR data export services. The review feedback highlights valuable optimization opportunities, such as caching the deserialized warnings array in the domain entity to reduce CPU overhead, removing redundant raw SQLite queries in the repository in favor of unified LINQ queries, and resetting the position of seekable streams in the PDF extractor to prevent unnecessary buffering.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread backend/src/Taskdeck.Domain/Entities/ArtefactExtraction.cs Outdated
Comment thread backend/src/Taskdeck.Infrastructure/Services/PdfPigArtefactTextExtractor.cs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3e3e4c104

ℹ️ 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".

Comment thread backend/src/Taskdeck.Infrastructure/Services/PdfPigArtefactTextExtractor.cs Outdated
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review remediation — repaired-parent absorb + 6 review threads

Parent absorb / conflict resolution

This PR was stacked on the pre-repair head of #1341 (b149bd49), which left it CONFLICTING/DIRTY. Merged the repaired parent origin/issue-1315/artefact-storage (99cc5f0a) into this branch (merge, not rebase/reset). Sole conflict was in DataExportServiceTests.cs, where two independent tests landed at the same spot — resolved by keeping both:

  • ExportUserDataAsync_ShouldRejectLargeExtractionHistoryBeforeLoadingArtefacts (this PR — extraction-bytes cap)
  • ExportUserDataAsync_ShouldRejectTooManyArtefactRowsBeforeLoadingAnyBlob (parent repair — row-count cap)

Both are valid against the merged service (extraction-bytes check runs before GetBufferedArtefactMetadataAsync, which is the parent's efficient GetByUserAsync(userId, 1, 10_000) probe). Merge commit e07e6723. PR is now MERGEABLE.

Findings → fix → verification

# Finding (file:line) Resolution Commit Verification
1 ArtefactExtraction.cs:29Warnings deserializes on every access (gemini, MEDIUM) Lazy _warnings backing field; constructor seeds it from validated list so neither path re-parses d4addedd ArtefactExtractionTests.Warnings_ShouldCacheParsedResultAcrossAccesses
2 ArtefactExtractionRepository.cs:76 — collapse raw SQL to LINQ in GetLatestForArtefactForUserAsync (gemini, MEDIUM) KEEP raw SQL — genuine SQLite guard: CreatedAt is DateTimeOffset, which the SQLite provider can't deterministically ORDER BY from LINQ; matches convention in ChatSessionRepository/LlmQueueRepository/AuditLogRepository. Added inline rationale c58eca59 ArtefactExtractionPersistenceTests.Queries_ReturnDeterministicHistoryWithinUserBoundary
3 ArtefactExtractionRepository.cs:115 — same in GetByArtefactForUserAsync (gemini, MEDIUM) KEEP raw SQL — same DateTimeOffset ordering rationale, applied consistently; added inline rationale c58eca59 same persistence test (paging/offset)
4 PdfPigArtefactTextExtractor.cs:58 — buffers up to 10 MiB when seekable & Position!=0 (gemini, MEDIUM) Rewind seekable streams (Position=0) and read directly after an up-front Length cap check; buffer only non-seekable 8da0e727 ExtractAsync_ShouldRewindSeekableStreamInsteadOfBuffering
5 PdfPigArtefactTextExtractor.cs:78 — enforce text cap before materializing each page (codex, P2) Bounded word-collection loop over page.GetWords() that stops at the remaining budget and observes cancellation between words; no whole-page string materialization 8da0e727 ExtractAsync_ShouldBoundSingleTextHeavyPage (+ existing ShouldStopAtCharacterCap/ShouldStopAtPageCap)
6 PdfPigArtefactTextExtractor.cs:111 — claims no-text-layer even when pages skipped (codex, P3) Only emit no-text-layer when all pages inspected (NumberOfPages <= MaxPages); when pages skipped, the page-limit warning stands alone 8da0e727 ExtractAsync_ShouldNotClaimNoTextLayerWhenPagesWereSkipped

Test evidence (targeted, SQLite; no Docker)

  • Taskdeck.Domain.Tests filter ~ArtefactExtraction: 6 passed / 0 failed
  • Taskdeck.Application.Tests filter ~ArtefactExtraction|~DataExport|~PlainTextArtefact|~Gdpr: 48 passed / 0 failed
  • Taskdeck.Api.Tests filter ~PdfPig|~ArtefactExtractionPersistence: 13 passed / 0 failed
  • dotnet build Taskdeck.Infrastructure -c Release: 0 warnings / 0 errors

Failing check: "API Integration (windows-latest)"

Investigated — not caused by this PR. The prior run failed a single test, WebhookDeliveryConcurrencyTests.ConcurrentBoardMutations_EachCreatesDeliveryRecord (transient 500 instead of 201 under concurrent board mutations), 1 of 1877, windows-latest only (ubuntu-latest and Backend Unit windows both green). This is unrelated to artefact extraction and matches the known flaky hosted-worker concurrency race family tracked in #1282 (transient 500 during concurrent operations) and #1335 (hosted-worker isolation). Expected to clear on the re-run triggered by this push.

Scope notes

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Follow-up: two LOW/cosmetic nits closed (head 231f2502)

  1. Trailing whitespace at the exact character capPdfPigArtefactTextExtractor.cs. At the boundary a page/word separator could be the final character appended (the following word had zero remaining budget), leaving a stray ' '/'\n'. The truncation path now TrimEnd()s the built string before returning; CharacterLimit semantics are unchanged. Commit 6294a505. Test: ExtractAsync_ShouldNotEndInWhitespaceAtCharacterCap (first page fills to one char short, second page's separator lands on the cap → asserts no trailing whitespace + character-limit).

  2. Repository comment precisionArtefactExtractionRepository.cs. Corrected the earlier wording: the SQLite provider does translate ORDER BY on DateTimeOffset (it orders the stored TEXT, chronologically correct for the all-UTC CreatedAt values here). The real rationale is now stated accurately — an explicit, deterministic in-database ordering that matches the established IsSqlite()/FromSqlInterpolated convention shared by ChatSessionRepository/LlmQueueRepository/AuditLogRepository. Commit 231f2502. This supersedes the imprecise "cannot translate" phrasing in my earlier replies on the two repository threads; the KEEP decision itself stands.

Re-verification (targeted, SQLite)

  • dotnet build Taskdeck.Infrastructure -c Release: 0 warnings / 0 errors
  • Taskdeck.Api.Tests ~PdfPig|~ArtefactExtractionPersistence: 14 passed / 0 failed
  • Taskdeck.Application.Tests ~ArtefactExtraction|~DataExport|~Gdpr: 48 passed / 0 failed

Still a draft; not merged; base unchanged.

@Chris0Jeky
Chris0Jeky changed the base branch from issue-1315/artefact-storage to main July 13, 2026 21:56
…ext-extraction

# Conflicts:
#	autodoc/AGENT_INDEX.md
@Chris0Jeky
Chris0Jeky marked this pull request as ready for review July 13, 2026 21:58
Copilot AI review requested due to automatic review settings July 13, 2026 21:58
@Chris0Jeky
Chris0Jeky merged commit bc9a675 into main Jul 13, 2026
30 of 33 checks passed
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Jul 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

using var document = PdfDocument.Open(pdfStream, parsingOptions);

P2 Badge Add a wall-clock budget around PDF parsing

When extraction receives a hostile PDF under MaxInputBytes, the work inside PdfDocument.Open and the immediate page-tree inspection runs synchronously before any cancellation check and without a timeout. The GEN-02 resource budget requires per-extraction wall-clock timeout/cancellation, so a parser-bomb PDF can still tie up the extraction worker despite the byte/page/character caps; wrap or isolate this parse with a configured timeout and convert timeout into a safe extraction warning.

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

GEN-02: Local text extraction - IArtefactTextExtractor, PDF text layer (PdfPig), extraction records

2 participants