Repository navigation
feat(sql-files): review Git changes and commit selected scripts - #1454
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge (after prerequisite #1423 lands; this draft bundles it) Notes
Files Reviewed (40 files)
Reviewed by claude-sonnet-5.5 · Input: 254 · Output: 5.3K · Cached: 404.4K Review guidance: REVIEW.md from base branch |
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for keeping commits scoped to approved saved scripts, using a separate index and preserving unrelated staging. I reviewed 4a2fb03c2bbe7030278a033207131f17040b115f, including the bundled linked-folder prerequisite and desktop/web capability boundaries.
The 41 targeted frontend tests, typecheck, production frontend build, targeted ESLint and desktop cargo check passed. The targeted Rust run had 42 passes and one failure: signing_failure_does_not_move_head_or_modify_the_index unexpectedly creates a commit when the inherited Git signing format is SSH.
I reproduced this independently with a generated throwaway Ed25519 key and temporary GIT_CONFIG_GLOBAL: gpg.format=ssh fails the new test, while changing only that format to openpgp makes it pass. With global Git configuration disabled, all 43 targeted Rust tests pass. The test overrides gpg.program, which applies to OpenPGP, but leaves the signing format inherited, so it can use a developer's real SSH signer instead of exercising failure.
Please make the signing-failure fixture independent of user/system Git configuration. Pin its format and failed signer explicitly, and cover SSH failure separately if required. This is a new test portability/isolation issue, not a failure attributed to existing main code.
I have not run real-app E2E or the full merge-gate chain for this revision. After the test fix and prerequisite #1423, verify the native dialog with a disposable nested scripts folder: selected-script-only commit, unrelated staging retained, stale-file/index/HEAD rejection, failing hook/signing behavior and an unsaved editor draft retained throughout.
| git(temp.path(), &["config", "commit.gpgsign", "true"]); | ||
| git( | ||
| temp.path(), | ||
| &["config", "gpg.program", "tabularis-missing-test-signer"], |
There was a problem hiding this comment.
[P2] Isolate this failure test from the inherited signing format. With a valid SSH signing key and gpg.format=ssh, setting gpg.program to a missing executable has no effect; the production commit correctly uses the SSH signer and this test fails at line 276. I reproduced it with a disposable key and temporary global config, then got a pass by changing only the format to OpenPGP. Pin the fixture's format/failed signer and prevent tests from invoking a developer's configured signer.
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for isolating both signing formats and using a fixture signing key. All 43 linked-folder/Git Rust tests now pass with my normal Git configuration, including the SSH and OpenPGP failure cases. The 18 frontend tests and typecheck also pass.
I exercised the Git modal in the real Linux Tauri app: opened a linked SQL file, edited and saved it, reviewed its diff, selected it and committed it. The resulting commit contains only that SQL script; an unrelated file staged externally before the test remains staged and unchanged. This resolves my signing-test blocker. Approved.
NewtTheWolf
left a comment
There was a problem hiding this comment.
I resolved the conflicts after #1423 landed and rechecked c07bc98. The 43 Rust linked-folder/Git tests, 18 frontend tests, typecheck and production build pass on the integrated tree.
The Git implementation, linked-file writing code and Git modal are unchanged from cf5018c, where I verified the native diff/selected-file commit flow. The external staged file was preserved in that app test. Approved on the updated head.
Closes #1418. Depends on #1423 (#1409), which is still open. This draft includes that prerequisite branch merged onto current
main; review the final feature commit separately until #1423 lands.Linked SQL folders now offer a desktop dialog to inspect saved-file Git diffs and explicitly commit selected, reviewed scripts. Unsaved editor drafts are left alone. The containing repository may be larger than the approved folder or a Git worktree. Renames appear as an explicit deletion/addition pair.
The backend uses literal argument arrays and an alternate index starting from HEAD. Review tokens bind disk content, staged blobs, HEAD/ref and the real index. Commits acquire the real index lock, preserve unrelated staged entries, refuse stale reviews/conflicts/in-progress merges, run commit hooks, honor signing, and update the branch with an expected-old-HEAD check. Hook/signing errors abort before the ref update; failures after creation return the commit id with a warning. No reset, clean or remote operations are used.
Only persisted approved folder ids reach the desktop wrappers; paths, symlinks, file types and sizes are checked in the shared Git module. Browser RPC has no routes for the three new commands. The feature is independent of built-in or external database drivers. UI strings are included in all 13 locales.
Validation:
git diff --checkpassed.detect_changeswas run on the staged feature. It reported CRITICAL/all 300 flows and attributed unrelated grid flows to an isolated new Rust test. Direct reference inspection instead confirms three production callers of the approved-root resolver, all new desktop wrappers. The 26-file feature diff and additive command contract were checked directly; graph risk classification is not reliable for this run.cargo check --no-default-featuresis locally blocked before compiling the app: vendored OpenSSL requires Perl, which is absent from PATH. Full app integration, root-authorization/RPC tests and the Unix-only symlink/literal-special-path cases require CI. No interactive desktop smoke test was performed.Manual check: in a disposable repository with an initial commit, link a nested
scriptsfolder containing a modifiedquery.sqland stage an unrelated file outside it. Open Review Git changes, inspectquery.sql, check it, enter a message and commit. Confirm the new commit contains only the script and the unrelated file remains staged. Review again, edit the file externally and confirm the commit is refused. Repeat with a failing pre-commit hook and verify HEAD stays unchanged. Keep an unsaved editor draft open throughout and confirm its content is retained.Limits: Git 2.36+ and an existing HEAD on a checked-out branch are required. Binary/non-UTF-8 scripts and files/blobs/diffs above 2 MiB are refused; status is capped at 1000 scripts. Repository hooks/signers execute according to the user's Git configuration and can wait or modify working files; the app never discards those changes.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.