Skip to content

feat: add commit_files for multi-path commits and fix repo-relative path resolution for nested config dirs - #679

Open
Scott McMaster (scottmcmaster) wants to merge 1 commit into
mainfrom
09-04-scott-git-repo-rel-path
Open

feat: add commit_files for multi-path commits and fix repo-relative path resolution for nested config dirs#679
Scott McMaster (scottmcmaster) wants to merge 1 commit into
mainfrom
09-04-scott-git-repo-rel-path

Conversation

@scottmcmaster

@scottmcmaster Scott McMaster (scottmcmaster) commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

This is to improve/harden the git layer to make it easier and less bug-prone supporting nested config dirs, based on this comment from Alex Shabalin (@alex-sparus) here: #655 (comment)

It also proactively adds the commit_files function which was in the PR that inspired said comment.

The main thing to look at is the repo_relative_path method -- the correctness of its logic as well as its call sites.

Test Plan

New unit tests.

Docs

  • Docs updated (companion PR in darkmatter/nixmac-web: #___)
  • No docs update needed

Copy link
Copy Markdown
Collaborator Author

@scottmcmaster Scott McMaster (scottmcmaster) changed the title scott-git-repo-rel-path feat: add commit_files for multi-path commits and fix repo-relative path resolution for nested config dirs Sep 4, 2026
@darkmatter

darkmatter Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🎨 Storybook preview

Open Storybook preview

Updated for 69e2e8d


⚠️ Detected UI changes (5)

These stories' HTML snapshots changed. I've added screenshots + links to the changed stories below. Review them carefully then accept the changes to regenerate baselines and include them in this PR:

Flows/Evolve › Playground

Flows/Evolve › Playground

Flows/Evolve › 1. Begin (idle)

Flows/Evolve › 1. Begin (idle)

Flows/Evolve › 2. Evolving (progress)

Flows/Evolve › 2. Evolving (progress)

Flows/Evolve › Evolving With Error Event

Flows/Evolve › Evolving With Error Event

Flows/Evolve › 3. Review (changes generated)

Flows/Evolve › 3. Review (changes generated)


Accept UI changes

  • Click here to accept these changes

Alternatively, you can run bun run test:update-snapshots locally to re-generate the baselines and then push the changes to this PR.

What does this do?

The screenshots above show UI changes detected by the Storybook
snapshot tests run on this PR. Each image is the rendered output of
a Storybook story from the code in this PR branch; the snapshot
test compared it against the committed baseline in
__snapshots__/ and flagged the difference.

Checking the box tells the darkmatter[bot] to regenerate the
baselines from this PR's current code and commit them directly to
this branch. The new baselines become the source of truth for
future runs — only accept after confirming the visual changes are
intentional.

Comparison baseline: the committed __snapshots__/ files on this
PR branch (carried forward from develop). Accept updates them in
place on this branch.

@darkmatteragent darkmatteragent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — APPROVE

69e2e8d2c82f · 0 findings

Approve: solid hardening — every producer/consumer boundary checks out. - All app callers pass the git toplevel plus repo-relative filenames, which repo_relative_path reproduces unchanged; the nested-config-dir case is fixed as intended. - Per-hunk commit/discard keeps hunk granularity through find_hunk_by_hash; no sibling-row mutation exists. - Escape/symlink paths fail closed before mutation, and the index.write() reorder preserves intent-to-add entries, with new tests locking both in.

No findings.

@darkmatteragent

Copy link
Copy Markdown
Reviewer · darkmatter agent VERDICT
APPROVE
Approve: solid hardening — every producer/consumer boundary checks out. - All app callers pass the git toplevel plus repo-relative filenames, which `repo_relative_path` reproduces unchanged; the nested-config-dir case is fixed as intended. - Per-hunk commit/discard keeps hunk granularity through `find_hunk_by_hash`; no sibling-row mutation exists. - Escape/symlink paths fail closed before mutation, and the `index.write()` reorder preserves intent-to-add entries, with new tests locking both in.

No findings.

reviewed: 69e2e8d — 2026-09-04T07:53Z verdict: approve findings: 0

Reviewer avatar Reviewer
darkmatter agent
Run details
Metadata Value
Agent darkmatter/agent/reviewer
Session pr:darkmatter/nixmac#679
Kind pr
Runtime platform-runtime
Configured model litellm/glm-5.2-fp8
Target darkmatter/nixmac#679
Revision 69e2e8d2c82fe2af0ff501d8f13b678545dc336c
Verdict approve
Reviewed at 2026-09-04T07:53Z
Logs Open Braintrust session

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor
Warnings
⚠️

No Linear issue ID found in this PR's title, description, or branch name (expected something like ENG-123). Add one so this work is traceable in Linear, or add #no-linear to the PR description to acknowledge it's intentionally untracked.

📋 PR Overview

Lines changed 324 (+299 / -25)
Files 0 added, 2 modified, 0 deleted
Draft / WIP no
Has Test Plan yes
Linear issue no
No Test Plan Needed no
New UI components no
New Storybook stories no
New Rust modules no
New TS source files no
New tests no
package.json touched no
Cargo.toml touched no
Infra / CI touched no

🔬 Coverage

Report Lines Statements Functions Branches
apps/native/coverage/coverage-summary.json 34.3% 33.8% 29.0% 29.2%

Generated by 🚫 dangerJS against 69e2e8d

@@ -425,8 +482,7 @@ pub fn restore_hunk(dir: &str, path: &str, hunk: &str) -> Result<()> {
/// as an empty blob instead.
pub fn commit_hunk(dir: &str, path: &str, hunk: &str, message: &str) -> Result<CommitInfo> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity 2 — not blocking

commit_hunk still has the write-before-bail shape this PR fixes in commit_files: further down (line 550) the index is written before the nothing to commit check. It's practically unreachable today (find_hunk_by_hash just found the hunk in the live diff), but the lookup→commit window is a check-then-act race, and when the bail does fire it clobbers intent-to-add entries the same way. Same two-line move as in commit_files?

@@ -318,8 +376,7 @@ fn index_entry_differs_from_head(
/// drift row alive. Purely unstaged drift leaves the index untouched.
pub fn restore_hunk(dir: &str, path: &str, hunk: &str) -> Result<()> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity 2 — not blocking

Now that paths here are dir-relative, restore_hunk/commit_hunk are only correct with dir = repo root: find_hunk_by_hash and file_diff_contents still match workdir-relative names, so a future caller passing a nested config dir would look a hunk up under one convention and resolve the file under the other. Worth a one-line comment on these two functions ("callers must pass the repo root until the query side is dir-relative too") so the first nested-dir caller finds out at review time instead of at runtime?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh damn! I guess I missed #655 (comment) before writing my review comment in #655 ... I think this probably solves my issues! Looking forward to re-try once this is in :)

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.

4 participants