feat(org): Implement capture functionality#171
Open
szaffarano wants to merge 18 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #171 +/- ##
==========================================
+ Coverage 93.07% 93.45% +0.38%
==========================================
Files 32 37 +5
Lines 2846 3730 +884
==========================================
+ Hits 2649 3486 +837
- Misses 197 244 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds “capture” support end-to-end (core library + MCP tool + CLI), allowing users/clients to append a fully-formed org heading (with metadata like TODO/priority/tags, planning timestamps, properties, and optional datetree expansion) into an org file.
Changes:
- Implement
OrgMode::capture_appendinorg-corewith validation, heading-path targeting, planning timestamps, properties, and datetree expansion. - Expose capture via a new MCP tool (
org-capture) and a new CLI subcommand (org capture). - Add extensive unit + integration tests and update documentation/config to describe the new capability.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the new org-capture tool, org capture CLI usage, timestamp grammar, and new config/env var. |
| org-mcp-server/tests/integration_tests/tools_tests.rs | Adds MCP integration tests for org-capture behavior and validation errors. |
| org-mcp-server/src/tools/org_capture.rs | Implements the MCP tool wrapper that maps request params into org_core::CaptureEntry. |
| org-mcp-server/src/tools/mod.rs | Registers the new org_capture tool module. |
| org-mcp-server/src/resources/mod.rs | Adds org-capture to the advertised tools list. |
| org-mcp-server/src/core.rs | Adds tool_router_capture() to the server tool router composition. |
| org-mcp-server/src/config.rs | Updates test config initialization to use OrgConfig::default() for new fields. |
| org-core/src/org_mode.rs | Adds the core capture implementation, helpers (timestamp parsing/formatting, datetree, heading-path resolution), and many unit tests. |
| org-core/src/lib.rs | Re-exports capture-related public types (CaptureEntry, CaptureResult, PropertyPair). |
| org-core/src/error.rs | Adds capture-specific error variants and Display formatting. |
| org-core/src/config.rs | Adds org_auto_created_property config with defaults + loader wiring + tests. |
| org-cli/src/main.rs | Adds a new Capture subcommand to the CLI entrypoint. |
| org-cli/src/commands/mod.rs | Exposes the new capture command module and re-exports CaptureCommand. |
| org-cli/src/commands/capture.rs | Implements org capture CLI command that constructs CaptureEntry and prints results. |
| CLAUDE.md | Updates repo guidance docs to reflect capture functionality and architecture details. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
szaffarano
added a commit
that referenced
this pull request
May 11, 2026
Four real bugs flagged by Cursor Bugbot and Copilot: - Path traversal: validate file_rel lexically before any filesystem call. Reject absolute paths, root/drive prefixes, and `..` components. Closes a hole where a non-existent parent directory was created via create_dir_all without containment check. - Level bump on fully matched target_heading: an explicit entry.level smaller than parent_level + 1 is now silently bumped (clamped to MAX_HEADING_LEVEL). Previously only the "create missing levels" path applied the bump; the "fully matched" path used the user level as-is, producing structurally broken org files. - target_heading segments: reject empty/whitespace segments (e.g., "A//B", "/A", "A/") which would otherwise create headings with empty titles. - README: clarify the auto-CREATED format shows an inactive timestamp like `[YYYY-MM-DD Day HH:MM]`, not the literal `<now>` placeholder. Adds four regression tests covering the path-traversal, absolute-path, level-bump, and empty-segment cases.
Four real bugs flagged by Cursor Bugbot and Copilot: - Path traversal: validate file_rel lexically before any filesystem call. Reject absolute paths, root/drive prefixes, and `..` components. Closes a hole where a non-existent parent directory was created via create_dir_all without containment check. - Level bump on fully matched target_heading: an explicit entry.level smaller than parent_level + 1 is now silently bumped (clamped to MAX_HEADING_LEVEL). Previously only the "create missing levels" path applied the bump; the "fully matched" path used the user level as-is, producing structurally broken org files. - target_heading segments: reject empty/whitespace segments (e.g., "A//B", "/A", "A/") which would otherwise create headings with empty titles. - README: clarify the auto-CREATED format shows an inactive timestamp like `[YYYY-MM-DD Day HH:MM]`, not the literal `<now>` placeholder. Adds four regression tests covering the path-traversal, absolute-path, level-bump, and empty-segment cases.
Break the 3614-line god file into focused submodules under org-core/src/org_mode/: types.rs — data types (OrgMode, TreeNode, SearchResult, etc.) core.rs — file listing, search, outline, tasks, agenda query capture.rs — capture_append + validation + private helpers + tests agenda.rs — date utilities + AgendaViewType + tests tests.rs — integration-style capture/datetree tests mod.rs — module wiring and re-exports Also moved private-function unit tests into the submodule where the tested code lives (format_heading tests to capture.rs, add_repeater_duration tests to agenda.rs) to avoid visibility bumps. Update CLAUDE.md file path references.
szaffarano
force-pushed
the
features/capture
branch
from
May 14, 2026 12:54
73f7781 to
d8be00b
Compare
- fix(capture): prevent UTF-8 panic in - fix(capture): reject leading zero counts (e.g. +00d) by parsing numeric value - fix(capture): normalize property keys to uppercase for case-insensitive duplicate detection - fix(capture): canonicalize parent directory after create_dir_all to verify path stays within org directory
Owner
Author
PR #171 Review Response SummaryAll 13 review comments have been evaluated. Here's the status of each: Already Addressed (no changes needed)
Fixed in commit
|
| # | Comment | Fix |
|---|---|---|
| 10 | Security check when parent missing | After create_dir_all, canonicalize parent + starts_with check |
| 11 | UTF-8 panic in is_count_unit |
split_at(s.len() - 1) → chars().next_back() |
| 12 | Zero-count bypass with leading zeros | num != "0" → num.parse::<u32>() > 0 |
| 13 | Case-sensitive duplicate property keys | seen.insert(p.key.clone()) → seen.insert(p.key.to_uppercase()) |
Not Fixed (deferred)
| # | Comment | Reason |
|---|---|---|
| 2 | fs::rename fails on Windows |
Requires cross-platform atomic-replace crate; lower priority |
| 3 | Lockfile cleanup on Windows | Requires Windows-specific lockfile strategy; lower priority |
Tests Added
test_parse_iso_timestamp_rejects_multibyte_utf8test_parse_iso_timestamp_rejects_leading_zero_counttest_capture_rejects_case_insensitive_duplicate_property_keys
Verification
cargo test --workspace→ 466 passed, 0 failedcargo clippy --all-targets --all-features -- -D warnings→ clean
- Fix find_heading_path sibling-matching bug by tracking matched depth on the open_stack instead of a boolean, ensuring siblings are not incorrectly accepted as children. - Fix lockfile removal ordering: drop handle before remove_file for Windows compatibility. - Clamp intermediate heading levels to MAX_HEADING_LEVEL during prefix creation to preserve hierarchy. - Fix MCP datetree implication: datetree_date now implies datetree=true to match CLI behavior. - Clean up temp file on any error path in atomic_write to prevent stale .tmp.* file accumulation. - Replied to non-issue PR comments (fd.lock() compilation and missing parent dir fsync).
szaffarano
force-pushed
the
features/capture
branch
from
May 14, 2026 18:40
c2bb847 to
54a1caa
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5eb743e. Configure here.
- Trim target_heading segments before heading path matching so that paths like "Projects / Work" correctly match existing headings instead of creating spurious new headings with embedded whitespace. - Replace manual temp-file creation in atomic_write with tempfile::Builder, which uses O_EXCL and a random name to prevent symlink-redirect attacks. Promotes tempfile from dev-dependency to regular dependency. - Copy target file permissions onto the temp file before rename on Unix, preventing atomic writes from silently widening access (e.g. 0600 → 0644).
- Reorder lockfile cleanup to be platform-conditional: on Unix, unlink while holding the lock (preserving stat-after-lock race invariant); on non-Unix, drop the file handle first since Windows cannot unlink an open file. - Update InvalidTimestamp display message to describe the full accepted grammar including optional repeater (+N|++N|.+N) and warning (-N) suffixes, replacing the incomplete YYYY-MM-DD / HH:MM-only description. - Validate the nearest existing ancestor of the target parent directory before calling create_dir_all, preventing a symlink inside org_directory from escaping the root before the post-creation canonicalization check.
- validate_relative_file_path now requires at least one Normal component, rejecting "" and "." which both resolved full_path to org_dir and caused the lockfile to be created in an unexpected location. - Replaced silent MAX_HEADING_LEVEL clamping with InvalidLevel errors in the prefix-creation loop and the final level computation so callers get a clear error instead of a silently misplaced same-level heading.
Two-layer defence for dot-trailing paths like "subdir/." or "a/b/./": - validate_relative_file_path: rejects any file_rel whose string (after stripping trailing slashes) ends with "/." — catches the non-existent case before any filesystem access. - capture_append: adds an is_dir() check on full_path immediately after the exists() test — catches the existing-directory case with a clear InvalidDirectory error instead of a confusing IoError deep in atomic_write.
Comment on lines
+119
to
+123
| let format = self.format.as_ref().unwrap_or({ | ||
| match cli.default_format.as_str() { | ||
| "json" => &OutputFormat::Json, | ||
| _ => &OutputFormat::Plain, | ||
| } |
Comment on lines
+105
to
+106
| let datetree = datetree || datetree_date.is_some(); | ||
|
|
Comment on lines
+440
to
+450
| let p = Path::new(file_rel); | ||
| if p.is_absolute() { | ||
| return Err(OrgModeError::InvalidDirectory(format!( | ||
| "absolute path not allowed: {file_rel}" | ||
| ))); | ||
| } | ||
| if file_rel.trim_end_matches('/').ends_with("/.") { | ||
| return Err(OrgModeError::InvalidDirectory(format!( | ||
| "file path must refer to a file, not a directory: {file_rel}" | ||
| ))); | ||
| } |
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
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.
Summary
Adds
org-capture— a write tool for the MCP server (and matchingorg-cli capturesubcommand) that appends fully-formed entries to org files. Built in three layered
phases on this branch:
slash-separated
target_headingpath with missing-level creation.into sibling subtrees), exclusive lockfile with stat-after-lock retry, atomic
write via temp-file + fsync + rename, validation of title/level/tags, fail-closed
path canonicalization, full MCP error-code mapping.
repeater (
+N{u}/++N{u}/.+N{u}) and warning (-N{u}) timestampsuffixes, property drawer with key/value/uniqueness validation, configurable
auto-
:CREATED:prepend (org_auto_created_property, default on), andYear/Month/Day datetree expansion (
--datetree [--datetree-date YYYY-MM-DD]).Surfaces
OrgMode::capture_append,PropertyPair,ParsedTimestamp,parse_iso_timestamp/format_org_timestamp/datetree_segments, 11 newOrgModeErrorvariants.org-capturetool withCaptureRequestJSON schema;validation errors map to
INVALID_PARAMS.capturesubcommand with--scheduled / --deadline / --closed / --property KEY=VALUE / --datetree / --datetree-dateflags.Docs
README adds the tool/command listings, configuration entry, env-var, and a full
capture-usage section with the timestamp grammar. CLAUDE.md adds the
capture-path internals (locking, atomic write, validation order, helpers).
Test plan
cargo test --workspace— 402 tests, 0 failurescargo clippy --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— clean--datetree --datetree-date 2026-05-10creates* 2026 / ** 2026-05 May / *** 2026-05-10 Sunday / **** entryconcurrent writes, malformed timestamps, malformed property keys, repeater
round-trip, auto-CREATED, datetree
Note
Medium Risk
Adds a new write path that creates/updates org files with locking, atomic writes, and extensive validation; bugs here could corrupt user data or reject valid inputs. Changes are well-covered by new unit/integration tests but touch core file I/O and config behavior.
Overview
Adds end-to-end capture (write) support:
org-coreimplementsOrgMode::capture_appendto append a new heading (optionally under a hierarchicaltarget_heading) with TODO/priority/tags/body, planning fields (SCHEDULED/DEADLINE/CLOSED) including repeater/warning suffix parsing, optional property drawer entries, and Year/Month/Day datetree expansion.Hardens the write path with exclusive lockfiles, atomic temp-file + fsync + rename writes, strict path validation/canonicalization to prevent directory escape, and introduces new
OrgModeErrorvariants for validation failures. Adds configorg_auto_created_property(defaulttrue) to auto-prepend:CREATED:unless user-supplied.Exposes capture via a new
org-cli capturesubcommand and an MCPorg-capturetool (with INVALID_PARAMS mapping for validation errors), and updates docs (README.md,CLAUDE.md) plus extensive new unit/integration tests for core/CLI/MCP flows.Reviewed by Cursor Bugbot for commit 48e6e14. Bugbot is set up for automated code reviews on this repo. Configure here.