-
-
Notifications
You must be signed in to change notification settings - Fork 289
Conversation
All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.
There was a problem hiding this 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
i️ 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".
591deae to
eeca7e2
Compare
salmonumbrella
commented
May 29, 2026
I have read the CLA Document and I hereby sign the CLA.
datlechin
commented
May 29, 2026
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
-
Bundled vs registry contradiction.
BeancountPlugin.swift:504andPluginMetadataRegistry+RegistryDefaults.swift:861setisDownloadable: true, but the pbxproj Copy Plug-Ins phase bundles the plugin inside the app. If the plugin instance is ever missing,DatabaseDriver.swift:426throwspluginNotInstalledand the UI offers to download it from the registry, where it must never be published. Pick one model: bundled meansisDownloadable = falseand the metadata snapshot moves intoPluginMetadataRegistry.swift; registry means it leaves the Copy Plug-Ins phase and follows the registry/ABI release flow. -
Missing
DatabaseType.beancountconstant. There is nostatic let beancount, and"Beancount"is not inallKnownTypes(DatabaseType.swift:30).DatabaseTypeTests.swift:54asserts the count equals 17 and only passes because the type was not added. Add the constant, append it toallKnownTypes, add theiconNamecase, and bump the count assertion to 18 in the same commit. -
BQL breaks pagination.
extractBQLQuery(BeancountPluginDriver.swift:1019) only matches abql:/bqlprefix on the raw query.fetchRows(:727) andfetchRowCount(:734) wrap the query asSELECT * FROM (<query>) ..., so a BQL query starts withSELECT, routes to SQLite, and fails. BQL works throughexecutebut not the data-grid browse path. Detect BQL before wrapping, or document BQL as execute-only, and add a paginated-BQL test. -
Missing new-driver checklist items. No
CHANGELOG.md[Unreleased]entry, nodocs/databases/beancount.mdx, no row indocs/index.mdx, and nobeancount-iconasset inAssets.xcassets(referenced byBeancountPlugin.swift:17and the registry snapshot, so the type chooser renders a blank or generic icon).
Runtime bugs (Codex, high confidence)
-
BQL can hang on large output.
executeBQL(BeancountPluginDriver.swift:318-322) callswaitUntilExit()before reading stdout/stderr. Whenrledgerwrites 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. -
Metadata lines misparsed as postings.
BeancountLedgerParser.swift:368-369: a metadata line likereceipt: "abc.pdf"trips the posting guard because the first token contains:, producing a bogus posting with accountreceipt:and shifting the postings table. Restrict posting detection to valid account names, or exclude tokens ending in:.
Confirmed by both reviews
- Glob includes not reparsed when new files appear.
reloadProjectionIfNeeded(BeancountPluginDriver.swift:963, also:402-403) compares signatures of files seen at last parse. Withinclude "imports/*.beancount", a newly added matching file leavesledger.sourceFilesunchanged, 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
-
Drop the local-binary fallback in
download-rustledger.sh:2280. On download failure it copies/opt/homebrew/bin/rledgeror/usr/local/bin/rledgerinto the shipped app, bundling an unverified binary and bypassing the SHA-256 pinning. Fail hard for release builds instead. -
Confirm the
rustledgerlicense permits redistributing the binary inside a signed app before merge.
Minor
SQLITE_TRANSIENT,NSLock.withLock, andArray[safe:]are redefined privately inBeancountPluginDriver.swift:1297-1311. CheckTableProPluginKitfor 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.
@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.
...driver # Conflicts: # .gitignore # CHANGELOG.md # Packages/TableProCore/Sources/TableProCoreTypes/DatabaseType.swift # Packages/TableProCore/Tests/TableProModelsTests/DatabaseTypeTests.swift
There was a problem hiding this 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: bbda21dbba
i️ 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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1 Badge Set Beancount PluginKit version to current ABI
The new Beancount bundle declares PluginKit 16, while the app currently requires PluginKit 18 (PluginManager.currentPluginKitVersion and minimumCompatiblePluginKitVersion are both 18, and validateBundleVersions rejects versions below that). As a result, any installed Beancount plugin built from this target is treated as outdated by the app; the release workflow also defaults releases to the current PluginKit version and will fail its "built PluginKit version matches the release label" check for this plugin.
Useful? React with 👍 / 👎.
...op the hand parser
Summary
.beancountfiles as read-only SQL tables.rledgerhelper for BQL support and ledger validation..beancountas 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="" -skipPackagePluginValidationxcodebuild build -project TablePro.xcodeproj -scheme TablePro -configuration Debug -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO CODE_SIGNING_REQUIRED=NO CODE_SIGN_IDENTITY="" -skipPackagePluginValidationplutil -lint TablePro/Info.plist Plugins/BeancountDriverPlugin/Info.plistswift test --package-path Packages/TableProCore --filter TableProModelsTests.DatabaseTypeTestsscripts/download-rustledger.sh /tmp/tablepro-rledger-pr-check && /tmp/tablepro-rledger-pr-check --version