docs(plan): store sources instead of parsing them, and let the agent read them - #24
Merged
Merged
Conversation
…read them The application takes four extensions and refuses the rest, and for PDF and DOCX runs a text extractor whose output is what every downstream reader consumes. That was decided task by task — 3.2, 3.3, 3.4 each named a format — and never as one decision. Looked at whole it contradicts the plan's own out-of-scope clause: "Extraction, summarisation or page writing by the application. That is the agent's." An agent that opens a PDF as a document and an image as an image keeps layout, tables, diagrams and figures — exactly what a text extraction drops. So the application stores the original and records what happened to it. Two findings that shape the work: - Document provenance was never validated. FILE_FRAGMENT is /^p\d+$/ and checks the shape only, so src://report.pdf#p999 resolves today. The gap 3.4 recorded for DOCX was never specific to DOCX. This plan does not weaken document provenance, because there is none to weaken. - retitleSource lives in the desktop main process, not in @open-wiki/access, so the CLI cannot mutate a manifest. A status verb would be a second mutator in a second place, against 9.1 — so the move comes with this work. Reconciles the one persisted field with 6.1's derived-not-persisted rule rather than overriding it: "the agent read this and found nothing worth writing" leaves no trace on the filesystem and is not derivable, while everything else still is. Without it every deliberately discarded source is a permanent 6.6 finding, and a check that cries wolf stops being read. Also notes where the risk moves: the application stops parsing hostile bytes in its privileged main process, and the agent starts opening files nobody looked at — so the skill has to say a source is evidence, not instructions. Scaffolds specs/source-status/, which the plan references. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SiqL2owT9tBkcpVhu423N
📝 WalkthroughWalkthroughThe PR adds an implementation plan for preserving uploaded sources and introduces requirements, design, and task templates for source-status work. ChangesSource storage and status planning
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@plans/sources-stored-not-parsed.md`:
- Line 154: Update the provenance viewer requirement in section 6.4 to restrict
previews and external opening to an explicit allowlist of safe file types; do
not send arbitrary stored files to the system handler. Treat unsupported or
potentially active types as download/reveal-only, requiring explicit user
confirmation before any external opening.
- Around line 76-79: Define explicit upgrade semantics for existing source
directories: specify how manifests missing the persisted processed field are
defaulted or backfilled, and how legacy application-generated text.md files are
retained, ignored, or removed. Align sources/state.ts text-ready derivation and
packages/mcp/src/tools.ts reporting with that policy so equivalent sources
expose consistent state and content regardless of ingestion time.
- Line 120: Update the storage plan around item 2.2 and the related
status-specification sections to explicitly define replacement behavior: either
reject an already-used source name or create a new source and manifest,
consistently. Add acceptance criteria requiring raw files to remain immutable
after writing and requiring atomic writes for both raw-file creation and
manifest mutations.
- Line 122: Define a concrete, testable size policy for checklist item 2.4:
specify the maximum accepted file size, how size is confirmed before dropping,
the refusal behavior when the limit is exceeded, and the information reported
for both drop and inbox outcomes. Ensure the policy constrains the unbounded
file acceptance described by item 2.3 and has observable behavior suitable for
unit tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e9a758b-df6f-4c44-b534-480cef71890b
📒 Files selected for processing (4)
plans/sources-stored-not-parsed.mdspecs/source-status/design.mdspecs/source-status/requirements.mdspecs/source-status/tasks.md
Comment on lines
+76
to
+79
| So: **one declared fact, and everything else stays derived.** `processed` is a | ||
| judgement somebody made, which is exactly the class of thing a filesystem cannot | ||
| observe. It is not a cache of `text.md`, and nothing derivable moves into it. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define upgrade semantics for existing source directories.
The plan adds a persisted processed field and stops application-generated text.md for new PDF/DOCX sources. It does not define how existing manifests without processed or existing extracted text.md files are handled. sources/state.ts:120 still derives text-ready from text.md, and packages/mcp/src/tools.ts:98 still returns it. Equivalent sources could therefore expose different state and content based only on when they were ingested. Define the default or backfill for processed and the policy for legacy text.md files.
Also applies to: 102-109
🧰 Tools
🪛 LanguageTool
[style] ~77-~77: Consider an alternative for the overused word “exactly”.
Context: ... is a judgement somebody made, which is exactly the class of thing a filesystem cannot ...
(EXACTLY_PRECISELY)
🤖 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 `@plans/sources-stored-not-parsed.md` around lines 76 - 79, Define explicit
upgrade semantics for existing source directories: specify how manifests missing
the persisted processed field are defaulted or backfilled, and how legacy
application-generated text.md files are retained, ignored, or removed. Align
sources/state.ts text-ready derivation and packages/mcp/src/tools.ts reporting
with that policy so equivalent sources expose consistent state and content
regardless of ingestion time.
| ## 2 — Accept any file | ||
|
|
||
| - [ ] 2.1 (Unit) Collapse the two adapter tables into one place in `@open-wiki/access`. The desktop's copy is drift waiting to happen, and this work would otherwise edit both | ||
| - [ ] 2.2 (TDD) Store any file: the original is preserved under `raw/<id>/`, the id is still derived and frozen per `adr:0011`, and nothing is refused for its extension. Test-first because it is the write path where a mistake is silent — a file accepted and stored under the wrong name, or outside `raw/`, is not visible until somebody goes looking |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve replacement semantics and preserve atomic storage writes.
docs/adr/0002-workspace-as-a-local-markdown-folder.md defines raw sources as immutable after writing and requires atomic writes. This plan says the original is preserved, but it does not make those guarantees acceptance criteria. It also asks the status specification to define what happens when a source is replaced, while the plan refuses an already-used name. Decide whether replacement is rejected or creates a new source and manifest. Require atomic writes for raw files and manifest mutations.
Also applies to: 123-123, 129-134, 138-139
🤖 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 `@plans/sources-stored-not-parsed.md` at line 120, Update the storage plan
around item 2.2 and the related status-specification sections to explicitly
define replacement behavior: either reject an already-used source name or create
a new source and manifest, consistently. Add acceptance criteria requiring raw
files to remain immutable after writing and requiring atomic writes for both
raw-file creation and manifest mutations.
| - [ ] 2.1 (Unit) Collapse the two adapter tables into one place in `@open-wiki/access`. The desktop's copy is drift waiting to happen, and this work would otherwise edit both | ||
| - [ ] 2.2 (TDD) Store any file: the original is preserved under `raw/<id>/`, the id is still derived and frozen per `adr:0011`, and nothing is refused for its extension. Test-first because it is the write path where a mistake is silent — a file accepted and stored under the wrong name, or outside `raw/`, is not visible until somebody goes looking | ||
| - [ ] 2.3 (Unit) Stop extracting on ingest. `.md` and `.txt` still get a `text.md`, because copying text that is already text is not extraction; PDF and DOCX no longer do | ||
| - [ ] 2.4 (Unit) A size stance, said out loud at the moment of the drop rather than discovered in `git status` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the size policy testable.
Line 122 requires “a size stance” but does not define a limit, confirmation step, refusal behavior, or drop/inbox report. Because Line 120 accepts any file, an implementation could consume unbounded disk space. Specify the policy and its observable failure behavior before implementation.
🤖 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 `@plans/sources-stored-not-parsed.md` at line 122, Define a concrete, testable
size policy for checklist item 2.4: specify the maximum accepted file size, how
size is confirmed before dropping, the refusal behavior when the limit is
exceeded, and the information reported for both drop and inbox outcomes. Ensure
the policy constrains the unbounded file acceptance described by item 2.3 and
has observable behavior suitable for unit tests.
| - [ ] 6.1 (Unit) The sources pane distinguishes stored, unprocessed, processed and cited — and offers the one action that is not the agent's, which is marking something processed by hand | ||
| - [ ] 6.2 (Unit) 6.6's uncited check reports only what is unprocessed *and* uncited, so a source somebody read and discarded stops being a permanent finding | ||
| - [ ] 6.3 (Unit) MCP says what it has: a source with no `text.md` reports its status and its filename rather than returning nothing, so a consulting agent knows the difference between empty and unread. It cannot read the original — that project is not on its disk — and saying so is the honest answer | ||
| - [ ] 6.4 (Unit) The provenance viewer opens what it is given: an image as an image, a PDF at its page, anything else named and offered to the system handler. The renderer's CSP is `default-src 'none'` and `img-src 'self' data:`, so this is a real constraint and not a formality |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not send arbitrary stored files to the system handler.
Line 154 offers “anything else” to the system handler while Line 120 accepts any extension. A malicious .lnk, .url, script, executable, or active document could launch an external application. The renderer CSP does not constrain that handler. Restrict previews to safe types. Make unsupported types download/reveal-only, with explicit confirmation before external opening.
🤖 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 `@plans/sources-stored-not-parsed.md` at line 154, Update the provenance viewer
requirement in section 6.4 to restrict previews and external opening to an
explicit allowlist of safe file types; do not send arbitrary stored files to the
system handler. Treat unsupported or potentially active types as
download/reveal-only, requiring explicit user confirmation before any external
opening.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The correction
The application takes four extensions and refuses the rest, and for two of them runs a
text extractor whose output is what every downstream reader consumes. That was decided
task by task — 3.2, 3.3, 3.4 each named a format and an adapter — and never as one
decision anybody looked at whole.
Looked at whole, it contradicts
plans/open-wiki.md's own out-of-scope clause:So: the application stores the original and records what happened to it. Reading it is
the agent's job. An agent that opens a PDF as a document and an image as an image
keeps layout, tables, diagrams and figures — exactly what a text extraction drops. And
the set of things a source can be stops being a list somebody maintains.
Two findings that shape the work
Document provenance was never validated.
store/provenance.ts:37is/^p\d+$/— itchecks that a fragment looks like
p12, never that page 12 exists.src://report.pdf#p999resolves today. The gap 3.4 recorded for DOCX was never specific to DOCX. Worth stating
plainly, because "this removes document provenance validation" would be a reasonable
objection and it is not true — there is none to remove.
retitleSourceis in the desktop main process (apps/desktop/src/main/edit.ts:408),not in
@open-wiki/access. So the CLI cannot mutate a manifest, and the status verbwould be a second mutator in a second place, against 9.1. The move comes with this work
rather than after it.
Also: the extension table exists twice —
upload.ts:35-40andapps/desktop/src/main/ingest.ts:24-29. Collapsing it is task 2.1, before anything elseedits ingest.
The one persisted field, and why 6.1 survives
6.1 decided source state is derived, never persisted, "because the copy is the one that
goes stale". This does not override it. It observes that one fact is not derivable:
cited.Today that second case is indistinguishable from a source nobody opened, so 6.6 reports
every deliberately-discarded source as a permanent finding. A check that cries wolf is a
check people stop reading.
So: one declared fact, everything else stays derived.
processedis a judgement,which is the class of thing a filesystem cannot observe. Nothing derivable moves into it.
Where risk moves
Narrows for the application.
pdf.tsparses a stranger's bytes in the privilegedmain process today — the exact risk 3.7 refused for the inbox, running on the drop path.
Storing bytes removes a parser from the trusted process.
Widens for the agent. It already reads untrusted source text — 4.13 found a
fabricated
## 3:00heading inside a transcript that survived the provenance check.Handing it PDFs and images extends that to files nobody looked at. Not a reason to
refuse; a reason task 5.2 makes the skill say a source is evidence, not instructions.
What it supersedes in
plans/open-wiki.md3.2/3.3/3.4 (adapters), 3.5 (the drop report), 6.1/6.6 (state and the uncited check),
9.10 (MCP handing back
text.md). Not un-ticked — they shipped and were correct againstwhat was decided then.
Left open, deliberately
Document provenance stays unvalidated, and the application will no longer even hold the
page count. Closing it later means a metadata probe that reads structure without
extracting text, or accepting that a document is cited whole. Recorded now, because "we
removed the parser" is otherwise the story of how it became unfixable.
How it was verified
npx @protonspy/scc validate— no findings.prettier --check— clean. Markdown only.🤖 Generated with Claude Code
https://claude.ai/code/session_018SiqL2owT9tBkcpVhu423N
Summary by CodeRabbit