Skip to content

feat (plugin-beancount): Add Beancount driver#1476

Open
salmonumbrella wants to merge 4 commits into
TableProApp:mainfrom
salmonumbrella:issue-1474-beancount-driver
Open

feat (plugin-beancount): Add Beancount driver#1476
salmonumbrella wants to merge 4 commits into
TableProApp:mainfrom
salmonumbrella:issue-1474-beancount-driver

Conversation

@salmonumbrella
Copy link
Copy Markdown

Summary

  • Add a Beancount database-driver plugin that opens .beancount files as read-only SQL tables.
  • Bundle and discover the RustLedger rledger helper for BQL support and ledger validation.
  • Register .beancount as a TablePro document type so installed builds can open ledgers from Finder.

Closes #1474

Test Plan

  • xcodebuild test -project TablePro.xcodeproj -scheme TablePro -destination 'platform=macOS' -only-testing:TableProTests/BeancountLedgerParserTests -only-testing:TableProTests/BeancountPluginDriverTests -only-testing:TableProTests/BeancountDriverMetadataTests CODE_SIGNING_ALLOWED=NO CODE_SIGNING_REQUIRED=NO CODE_SIGN_IDENTITY="" -skipPackagePluginValidation
  • xcodebuild build -project TablePro.xcodeproj -scheme TablePro -configuration Debug -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO CODE_SIGNING_REQUIRED=NO CODE_SIGN_IDENTITY="" -skipPackagePluginValidation
  • plutil -lint TablePro/Info.plist Plugins/BeancountDriverPlugin/Info.plist
  • swift test --package-path Packages/TableProCore --filter TableProModelsTests.DatabaseTypeTests
  • scripts/download-rustledger.sh /tmp/tablepro-rledger-pr-check && /tmp/tablepro-rledger-pr-check --version

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

Copy link
Copy Markdown

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

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: 591deaed67

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread Plugins/BeancountDriverPlugin/BeancountLedgerParser.swift Outdated
@salmonumbrella salmonumbrella changed the title Add Beancount driver plugin feat (plugin): Add Beancount driver May 29, 2026
@salmonumbrella salmonumbrella force-pushed the issue-1474-beancount-driver branch from 591deae to eeca7e2 Compare May 29, 2026 03:55
@salmonumbrella salmonumbrella changed the title feat (plugin): Add Beancount driver feat (plugin-beancount): Add Beancount driver May 29, 2026
@salmonumbrella
Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA.

github-actions Bot added a commit that referenced this pull request May 29, 2026
@datlechin
Copy link
Copy Markdown
Member

Code review: Beancount driver

Two automated reviews ran against this branch: a repo-wide review and a Codex diff-scoped review. They are complementary. Findings below are merged and deduplicated. Parser core, read-only enforcement, and the array-argv Process invocation are solid; the issues are integration, two runtime bugs, and the new-driver checklist.

Blocking

  1. Bundled vs registry contradiction. BeancountPlugin.swift:504 and PluginMetadataRegistry+RegistryDefaults.swift:861 set isDownloadable: true, but the pbxproj Copy Plug-Ins phase bundles the plugin inside the app. If the plugin instance is ever missing, DatabaseDriver.swift:426 throws pluginNotInstalled and the UI offers to download it from the registry, where it must never be published. Pick one model: bundled means isDownloadable = false and the metadata snapshot moves into PluginMetadataRegistry.swift; registry means it leaves the Copy Plug-Ins phase and follows the registry/ABI release flow.

  2. Missing DatabaseType.beancount constant. There is no static let beancount, and "Beancount" is not in allKnownTypes (DatabaseType.swift:30). DatabaseTypeTests.swift:54 asserts the count equals 17 and only passes because the type was not added. Add the constant, append it to allKnownTypes, add the iconName case, and bump the count assertion to 18 in the same commit.

  3. BQL breaks pagination. extractBQLQuery (BeancountPluginDriver.swift:1019) only matches a bql:/bql prefix on the raw query. fetchRows (:727) and fetchRowCount (:734) wrap the query as SELECT * FROM (<query>) ..., so a BQL query starts with SELECT, routes to SQLite, and fails. BQL works through execute but not the data-grid browse path. Detect BQL before wrapping, or document BQL as execute-only, and add a paginated-BQL test.

  4. Missing new-driver checklist items. No CHANGELOG.md [Unreleased] entry, no docs/databases/beancount.mdx, no row in docs/index.mdx, and no beancount-icon asset in Assets.xcassets (referenced by BeancountPlugin.swift:17 and the registry snapshot, so the type chooser renders a blank or generic icon).

Runtime bugs (Codex, high confidence)

  1. BQL can hang on large output. executeBQL (BeancountPluginDriver.swift:318-322) calls waitUntilExit() before reading stdout/stderr. When rledger writes enough JSON (or stderr on failure) to fill the pipe buffer, the child blocks on write and the call hangs indefinitely. Drain both pipes asynchronously, or read them before waiting.

  2. Metadata lines misparsed as postings. BeancountLedgerParser.swift:368-369: a metadata line like receipt: "abc.pdf" trips the posting guard because the first token contains :, producing a bogus posting with account receipt: and shifting the postings table. Restrict posting detection to valid account names, or exclude tokens ending in :.

Confirmed by both reviews

  1. Glob includes not reparsed when new files appear. reloadProjectionIfNeeded (BeancountPluginDriver.swift:963, also :402-403) compares signatures of files seen at last parse. With include "imports/*.beancount", a newly added matching file leaves ledger.sourceFiles unchanged, so no reload fires and new entries stay invisible until reconnect. Track the glob include-root directory mtime, or re-resolve includes on each reload.

Security

  1. Drop the local-binary fallback in download-rustledger.sh:2280. On download failure it copies /opt/homebrew/bin/rledger or /usr/local/bin/rledger into the shipped app, bundling an unverified binary and bypassing the SHA-256 pinning. Fail hard for release builds instead.

  2. Confirm the rustledger license permits redistributing the binary inside a signed app before merge.

Minor

  • SQLITE_TRANSIENT, NSLock.withLock, and Array[safe:] are redefined privately in BeancountPluginDriver.swift:1297-1311. Check TableProPluginKit for a shared helper before duplicating.

What is good

Parser with includes/cycle guard, in-memory SQLite projection, layered read-only enforcement (isReadOnlyQuery allowlist + PRAGMA query_only = ON + parameterized inserts), file-change reload, and BQL invoked via array argv (no shell injection). Checksum pinning in the download script is correct.

@salmonumbrella
Copy link
Copy Markdown
Author

salmonumbrella commented May 30, 2026

@datlechin I reworked Beancount to follow the DuckDB-style registry model: it is now isDownloadable, its metadata lives in registry defaults, and BeancountDriver.tableplugin is no longer copied into or built as a dependency of the main app. The plugin release workflow now builds Beancount, includes it in release-all-plugins.sh, packages the pinned rledger helper inside Contents/Resources, and signs executable helper resources before signing the plugin bundle.

I also tightened download-rustledger.sh so release CI cannot use TABLEPRO_RUSTLEDGER_BINARY; release builds must use the pinned SHA-256-verified download.

License check: rustledger is GPL-3.0, and TablePro is AGPLv3, so redistribution is compatible subject to preserving notices/source availability.

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.

feat: add a Beancount driver plugin to open .beancount ledgers

2 participants