Add macOS app and Finder badges for workspace status#16
Open
manuelgruber wants to merge 89 commits into
Open
Conversation
Upstream v3.0.0 (2026-04-12) moves the action runtime from Node 20 to Node 24. Inputs, outputs, and env usage are unchanged, so the existing step config keeps working as-is. Matches the repo's pattern of tracking the newest major tag for each action.
…ing pool The TUI previously froze forever at "Discovering repositories..." when any gh CLI subprocess stalled, because blocking Command::output() ran directly on the Tokio runtime with no timeouts. - Add run_gh_with_timeout() in src/auth/gh_cli.rs: 10s hard timeout using Command::spawn() + try_wait() polling + child.kill() on expiry. Refactor is_installed, is_authenticated, get_token, get_username, get_token_for_host to use it. - Wrap get_auth_for_provider() in tokio::task::spawn_blocking at the two async call sites (workflows/sync_workspace.rs and setup/handler.rs) so the async runtime stays responsive while gh is running.
Move scan_all_workspaces, scan_single_repo, and check_important_ignored_files out of src/commands/daemon.rs into a new src/api/service.rs as RepoScanService. The daemon loop, socket handlers, and the CLI `status` command now call the same service, eliminating duplicate scan logic and giving `status` access to the full FinderRepoStatus (branches, remotes, worktrees, badge). This establishes a clean backend/frontend separation: src/ is the API (Rust scanning + git logic), macos/ and future frontends are consumers of the JSON contract. Discovery's scan_local gains ?Sized bounds so it accepts &dyn GitOperations. Tests relocated from commands/daemon_tests.rs to api/service_tests.rs. All 522 tests pass; fmt and clippy clean.
…sting Daemon: --status and --stop now print to stdout directly instead of through Output, which is Quiet by default and was suppressing them. Add a SIGTERM handler alongside SIGINT so `daemon --stop` triggers the socket cleanup branch. macOS: add XcodeGen spec (macos/project.yml), sandbox entitlements for host app and FinderSync extension with a temporary-exception read of ~/.config/git-same/finder/, move StatusReader to Shared so the extension target can reach it, and implement the refreshStatus and openInTerminal @objc actions that ContextMenuBuilder references. Generated .xcodeproj is gitignored.
Switch to absolute-path temp-exception entitlement with real-path coverage for the config dir and the current workspace roots; the prior home-relative-path entitlement resolved against the sandbox container, not real $HOME. Resolve real $HOME in Shared/Constants.swift via getpwuid(getuid()) to bypass the sandbox's homeDirectoryForCurrentUser redirect, so status.json and finder.sock paths point at the daemon's real files. Re-enable App Sandbox on both targets (extensions are required to be sandboxed on macOS 26), revert the FinderSync class rename from an earlier debugging attempt, and rewrite BadgeManager to use NSImage(size:flipped:drawingHandler:) so badge images have valid CGImage-backed pixel data in a sandboxed extension. Note: badges still do not render on macOS 26.4, matching observed behavior for Synology's FinderSync extension. Likely system-level regression; revisit or migrate to NSFileProviderExtension later.
…spec Remove macos/project.yml and commit the generated .xcodeproj directly so the repo no longer depends on XcodeGen. Per-user Xcode state (xcuserdata) stays gitignored.
Drop the per-request os_log and the duplicate NSLog lines added while diagnosing the macOS 26 badge-rendering path. Keep os_log calls on FinderSync init, updateMonitoredDirectories (including the no-status case), and setDirectoryURLs so future issues can still be investigated via `log show --predicate 'subsystem == "com.zaai.git-same.GitSameBadge.FinderSync"'`.
The FinderSync extension previously injected ~10 rows directly into Finder's right-click menu (badge header, branch, commit count, staged line, branches submenu, remotes submenu, worktrees submenu, refresh, terminal). Collapse them under a single "Git-Same" parent that expands on hover. Surface fields the daemon already writes but the extension ignored: workspace name, org name, default branch, top-level ahead/behind, all-branches-synced and all-worktrees-synced indicators, and an "Important ignored files" warning row with the matched patterns as a sub-submenu. Add an analogous Git-Same menu for org folders (org/workspace/path + Refresh) which previously got no menu at all. To support that without iterating the org-folder array on every right-click, replace the orgPaths Set in StatusReader with an orgFoldersByPath map and add an orgFolder(forPath:) accessor mirroring repoStatus(forPath:).
The Git-Same submenu shipped in the previous commit was thin, especially
for org folders (just org/workspace/path/refresh). Surface the rest of
the data the daemon already sends so the right-click menu becomes a
useful at-a-glance dashboard.
Repo menu adds: human-readable status label ("Synced", "Uncommitted
Changes", etc.), full path, last-scan timestamp (relative + absolute),
counts on Branches/Remotes/Worktrees headers, and per-branch detail
sub-submenus showing upstream/ahead/behind/synced.
Org menu adds: repo count + per-badge breakdown, totals (commits,
uncommitted, ahead/behind, stashes), a warning submenu listing repos
with sensitive ignored files, a Repos submenu with one row per repo
(badge + branch) each expanding to a per-repo detail mini-submenu,
workspace root, total orgs in workspace + a sub-submenu listing them
with a checkmark on the current one, and last-scan timestamp.
Pass the extra data into ContextMenuBuilder by extending its API:
build(for:timestamp:socketClient:) for repos, build(for:repos:
workspaceInfo:timestamp:) for orgs. menu(for:) gathers the timestamp
and (for orgs) filters status.repos by org + workspace.
Avoids referencing OwnerType so the change stays decoupled from the
parallel owner-type workstream.
The Git-Same submenu had grown to ~15 inline rows (org info, aggregate stats, repo list, workspace info, last scan, refresh) which made it hard to scan. Group the data into three labeled sub-submenus so users can drill into the level they care about: - Organization: workspace, org/owner, paths, all sibling orgs - Repositories: aggregate stats and per-badge counts (org menu) or this repo's own status (repo menu) - Repository details: branches, remotes, worktrees (repo menu) or the per-repo list (org menu) The Last scan timestamp and the Refresh / Open in Terminal actions stay at the top level of the Git-Same submenu so they remain one hover away. Extends ContextMenuBuilder.build(for repo:) to also accept a FinderWorkspaceInfo so the repo's Organization submenu can show workspace root and sibling orgs the same way the org-folder menu does.
Add an OwnerType enum on OrgFolderInfo populated by a file-backed OwnerTypeCache at ~/.config/git-same/finder/owner_types.json. The daemon spawns a background classifier on startup that calls GET /users/<name> for every configured org and the workspace's own username, caches the result, and the periodic scan picks it up on subsequent iterations. Extend the Provider trait with get_owner_type, implement it on the GitHub client, and stub it in the mock. The daemon and socket REFRESH handler both attach the cache to RepoScanService so the annotation is consistent across scan paths. On the Swift side, decode owner_type in OrgFolderInfo, add a new "user" BadgeID, register a teal "U" badge in BadgeManager, and route FinderSync.requestBadgeIdentifier to the user badge when the folder's ownerType is .user. The workspace's configured username is injected into the scan's owner-folder list so a user account clone (e.g. manuelgruber) gets a "U" even when it isn't in the workspace orgs allowlist. Includes an AmbientUpgradeCache and monitored_roots field added alongside so ambient git repos discovered outside a workspace can be upgraded on demand; covered by colocated tests.
Extend the Swift Badge enum and BadgeID set to handle the new gray ambient case that already exists in the Rust schema, wire the gray branch through FinderSync's request handler, ContextMenuBuilder's aggregate counts and labels, and decode the monitored_roots field so the extension can register ambient scan roots alongside workspace roots. Add three Rust tests for the ambient pipeline: scan_ambient_repo produces a minimal gray entry, scan_all emits gray entries under configured finder.scan_roots, and AmbientUpgradeCache preserves an upgraded semantic color across scans.
Org and user folders render their own letter (O / U) and do not share the "R" family, so flashing a grey R before painting the final badge would be a letter change rather than a loading hint. Restrict the grey→color flash to green/blue/orange/red (the R colors) and let org, user, and daemon-emitted gray badges render directly. prewarmBadges now sets the final O/U badge for org folders without a grey intermediate and tracks them in coloredURLs so subsequent paints stay steady.
…itory view Three changes, all in ContextMenuBuilder.swift: 1. Org-folder menu's Organization submenu splits into Organization (Owner, Type if classified via OwnerType, Path) and Workspace (Name, Root, Orgs in workspace, All orgs with checkmark on the current). Sibling-org listing belongs to the Workspace section because it is workspace-scoped. 2. Repo-folder menu mirrors the split structurally: Organization (just Org name) and Workspace (Name + Root only). The repo menu does NOT include sibling orgs or workspace-org count, since those describe state outside the targeted repo. 3. Repository submenu adopts sentence-style rendering. Bold attributed Status header, then Branch and Upstream (with synced/ahead/behind prose), Default only when it differs, Index (clean or N staged) and Workdir (clean or non-zero parts joined with middle-dot), conditional Stashes and yellow-warning Important ignored files. Identity block (Commits with locale grouping, Path) sits below a separator. The per-repo mini submenu inside the org menu's Repository list reuses the same renderer. Ambient .gray repos collapse the Repository submenu to Status + an italic "Refreshing in background..." hint + Path; the existing on-right-click REFRESH trigger in build(for repo:) is preserved so the daemon upgrades the badge before the user looks again.
The previous design flashed grey for 250ms via DispatchQueue.main.asyncAfter and tracked a coloredURLs set to avoid repeat flashes. In practice this raced Finder's paint tick: sometimes the grey frame was visible, sometimes Finder only painted the color that landed after the timer, and the in-process set did not survive extension suspend/wake. Redefine grey as "we do not yet have a status for this URL" and let Finder's own badge cache do the work. prefillBadges iterates status.repos and status.orgFolders and writes each URL's final badge via setBadgeIdentifier; it runs on init, on every status.json reload, and whenever updateMonitoredDirectories changes directoryURLs. Duplicate writes are no-ops per Apple's docs so repeat calls are free. requestBadgeIdentifier sets the final badge for known URLs and writes grey plus a throttled REFRESH only on a genuine cache miss under a monitored root. Removes coloredURLs, greyHoldDuration, and the applyBadge helper; the timer and its race go with them.
requestBadgeIdentifier painted GitSameBadgeConstants.BadgeID.gray on every cache miss, so every non-repo folder Finder displayed — children inside a repo, unrelated workspace siblings — got a grey "R". The daemon's ambient scan already publishes all real repos to status.repos and prefillBadges paints them, so the optimistic fallback is redundant. Drop the setBadgeIdentifier call on miss and keep only the throttled REFRESH nudge, matching the pre-0399bb9 behavior. Grey is still used for ambient repos via the prefill path.
Align run.sh help output with the current CLI so developers see every subcommand defined in src/cli.rs. daemon, scan, and the no-subcommand TUI entrypoint were missing. Also fill in flags that were previously absent: sync --refresh, sync --no-skip-uncommitted, status --behind, status --org, and workspace default --clear. Install flow and alias-symlink logic unchanged.
…ediately After cloning a repo, sync now sends REFRESH_ALL to the daemon's Unix socket via the existing UnixSocketClient. Without this, status.json stays stale until the daemon's next poll and Finder paints no "R" badge on freshly cloned folders. The call is best-effort: if the daemon is not running, sync still succeeds and the failure is logged at debug only.
…econd daemon write startWatching() opens status.json with O_EVTONLY and, when the file is missing, reschedules itself every 5 s. On the retry that finally succeeds we armed the DispatchSource but never called reload() or onStatusUpdate, so the extension waited for the daemon's next write (up to the polling interval, default 30 s) before any badges could appear. If the daemon had just finished its initial scan and sat idle, badges could stay blank for a full scan cycle. Call reload() plus dispatch onStatusUpdate on the main queue right after source.resume(). Both are idempotent so the common hot path (file present at init) is unaffected.
Reset now sends REFRESH_ALL to the daemon on both success paths (--force and interactive) so Finder badges drop as soon as workspace configs are wiped, matching the post-sync nudge in 6ae60ff. A new top-level `gisa refresh` subcommand (with optional --path) gives users a direct way to force a status.json rewrite when debugging badges or after manual on-disk edits, with a clear error when the daemon is not running. Cheat-sheet and three plan docs under docs/plans/ round out the change.
…ynced Two regressions surfaced by the C/Finder-Badges review: 1. The status command's refactor to RepoScanService dropped per-repo error tracking. scan_repo's unwrap_or_else / unwrap_or_default chain coerced any git failure into all-zero defaults, which compute_badge then resolved to Badge::Green, so a corrupted or partially cloned repo printed as "(clean)" and rolled into "All repositories are clean and up to date". scan_repo now captures the error in a new read_error field and forces Badge::Gray; the CLI counts those repos separately and warns "N repositories could not be checked". 2. for-each-ref reports %(upstream:track) as "[gone]" when a local branch's upstream ref has been deleted. parse_track_info only matched ahead/behind, so [gone] yielded (0, 0) and is_synced evaluated to true (the upstream:short field still returned the dead ref name). That could promote a repo to Badge::Green and tell the user it was safe to delete a branch whose commits no longer existed on any remote. list_branches now clears the upstream when track is [gone] so is_synced and all_branches_synced are correct. Tests cover both paths, including a new MockGit hook for failing status calls per path.
… coupling Both types were defined in tui::app but consumed by non-UI callers (workflows::status_scan, cache::sync_history) - making the engine modules depend on the TUI module. Move them to types::repo_status, drop the now-unnecessary feature gates on workflows::status_scan and cache::sync_history, add SyncHistoryManager to the prelude, and leave a transitional pub use shim in tui::app so existing intra-TUI imports keep compiling until B0.2. First commit of the workspace refactor (Phase B0.1).
…om types:: Removes the transitional pub use shim in tui::app and points each consumer (event, event_tests, screens/dashboard, handler, app itself) at the new canonical home in crate::types. With this commit, no TUI-internal code relies on tui::app re-exporting domain types. Phase B0.2 of the workspace refactor.
The setup module is feature-gated as tui-only at the lib root, but the gating that actually matters is on its internal ratatui usage and on the CLI Setup subcommand wiring. Removing the lib-level cfg is a no-op for behavior (the inner ratatui imports still gate compilation when the tui feature is off, same as before) and prepares for B3, where setup moves into the CLI crate and the cfg lands on its declaration there. Phase B0.3 of the workspace refactor.
cargo fmt rearranges the new use crate::types::* lines to follow rustfmt's external/crate ordering. Pure formatting; no behavior change.
…nary)
Restructures the project from a single git-same crate into a two-member
workspace so the engine library can be reused by future frontends (Phase C
Tauri app, headless daemon split, etc.) without those frontends being able
to accidentally pull in clap/ratatui types.
What moves where (B2/B3):
- crates/git-same-core/ api, auth, cache, checks, config, discovery,
domain, errors, git, infra, ipc, operations,
output, provider, types, workflows
- crates/git-same-cli/ main.rs, lib.rs, cli.rs, banner.rs, commands/,
setup/, tui/, app/, bin/gen_*
Boundary changes:
- Root Cargo.toml is now a pure workspace manifest with hoisted deps and
workspace.package shared metadata.
- git-same-core has no UI deps (no clap, ratatui, crossterm, console).
- git-same-cli depends on git-same-core via a path/version pin.
- gen-completions and gen-manpage move with the CLI crate; their
required-features = ["release-tools"] gating is preserved.
- The binary still publishes as `git-same` ([[bin]].name) so installer
aliases (gisa, gitsa, gitsame) and target/release/git-same path are
unchanged.
Cross-crate test helpers:
- Repo::test() and rand_id() are now gated on `cfg(any(test, feature = "test-utils"))`
so the CLI crate's tests can build them by enabling the test-utils
feature on its dev-dependency on core.
Integration test fix (B4 folded in):
- The test now reads CARGO_BIN_EXE_git-same via env!() (compile-time)
instead of std::env::var_os (runtime). The runtime variant only
worked in the single-crate layout because the broken fallback path
happened to resolve correctly there.
Doc-test paths:
- All `use git_same::*` references inside core's doc comments rewritten
to `use git_same_core::*`.
Phase B1 + B2 + B3 + B4 of the workspace refactor.
The default scan_roots = ["~"] expanded to the user's home folder and was added to the FinderSync extension's directoryURLs. macOS silently refuses to deliver requestBadgeIdentifier calls to extensions that watch the home directory, so badges never rendered on a default install — not even for configured workspaces. The Finder Badges self-check still reported "ready" because the data path was healthy; only OS-level delivery was suppressed. Flip show_ambient to false so a fresh install renders badges out of the box. Users who want the ambient gray badge can opt in after narrowing scan_roots to specific subdirectories.
macOS auto-creates /Volumes/<boot-volume-name> as a symlink to /, and Finder may present home-folder URLs with that alias prefix (e.g. /Volumes/Manuel-SSD-4TB/Users/m/...). Our directoryURLs set only contained the canonical /Users/<user>/... form, so macOS never matched the alias-prefixed URLs against our watched set and Finder never called requestBadgeIdentifier — badges never appeared, even though the extension was loaded and the data path was healthy. updateMonitoredDirectories now enumerates /Volumes/* for symlinks pointing to / and adds the alias-prefixed equivalent of each canonical watched root. The existing path-lookup code in repoLookup already handled both forms, so no other changes are needed.
NSImage.lockFocus() and NSImage(size:flipped:drawingHandler:) both produce blank pixel data inside a sandboxed FinderSync extension on macOS 26.4 — the badge slot reserves space on the folder icon but no glyph appears. This matches the regression observed by other vendors (Synology Drive). SF Symbols are pre-rendered by macOS itself and require no per-process drawing context, so they render reliably under the same sandbox. Replace BadgeManager's labeledBadge (custom rounded rect with text) with symbolBadge using r.circle.fill / o.circle.fill / u.circle.fill in palette colors. Visual identity is preserved: colored R/O/U pucks matching the original five repo states and the org/user folder markers. Also document this and two adjacent footguns (boot-volume alias paths, Google Drive FinderSync interference) in CLAUDE.md so the next maintainer doesn't re-derive them from scratch — they wasted a day in this session.
Iterate the SF-Symbol-based badges: - Switch r/o/u.circle.fill to .square.fill so the badge silhouette matches the original rounded-square aesthetic from the lockFocus-era labeledBadge implementation. - Apply a two-color palette ([.white, color]) so the letter glyph renders white-on-color instead of monochrome-on-color, restoring the visible R/O/U identity. - Bump pointSize from 14 to 256 so badges stay sharp at any Finder icon-size setting. - Use .systemBlue for both Organization and User folder badges (was purple/teal); these share owner-folder semantics so a single hue is easier to read at a glance. - Darken every fill color 20% via shadow(withLevel: 0.2) so the white letter contrast holds at small icon sizes.
Swaps the generic list-view app icon for a twin overlapping-tiles
design that communicates Git-Same's mirror/clone purpose, and aligns
the palette with the TUI banner (blue #3B82F6 -> cyan #06B6D4 ->
green #22C55E).
Adds toolkit/icons/ with:
- generate-icons.swift Renders 5 concept variants as 1024x1024 PNGs
(twin, sync, folder-pair, wordmark, tui-banner) using AppKit /
CoreGraphics. macOS-native, no extra deps.
- promote.sh Regenerates the live asset set (1024 master, all
sizes referenced by tauri.conf.json, icon.icns via iconutil,
icon.ico via sips) from any chosen variant.
- README.md Usage notes.
Gitignores the variants/ preview directory.
Re-renders the twin app-icon variant in Apple's Liquid Glass idiom: translucent panels with a top specular highlight, soft drop shadow, hairline rim stroke, and a subtle bottom tint per tile. The list-row contents stay solid for legibility on top of the glass, and the back tile's green rows are clipped to the area outside the front tile so nothing bleeds through the blue panel. Switches the palette from the TUI hex constants (#3B82F6 / #06B6D4 / #22C55E) to the macOS system colors the Finder Badge extension already uses (.systemBlue + .systemGreen, with the other badge colors also exposed in the Palette struct for future variants). Colors are resolved to sRGB at render time so the PNG output stays deterministic. The diagonal background gradient simplifies to systemBlue (top-left) to systemGreen (bottom-right), dropping the cyan midpoint that has no counterpart in the Finder badge set.
The previous text "Mirror GitHub structure /orgs/repos/ to local file system." was 58 chars and clipped with ellipsis in the narrow sidebar. Replaced with "Mirror GitHub /orgs/repos/ to your file system." which preserves the /orgs/repos/ shape cue and renders without truncation.
The previous subhead "Mirror GitHub structure to local file system." clipped with an ellipsis at the sidebar's banner width. Shortened it to "Mirror GitHub structure to local disk." and added text-align: center so it sits centered under the GIT-SAME ASCII banner.
macOS-only feature that uses NSWorkspace.setIcon to stamp a custom Git-Same blue-folder icon (with the twin-tiles glyph composited onto the front face) onto every workspace root. Finder then shows the icon in sidebar, column, list, icon, and Get Info views — same mechanism Synology Drive uses for its synced folders, which a FinderSync extension can never replicate because it only exposes corner badges. Hooks: - setup/mod.rs::save_workspace paints the icon right after persisting. - monitor/run.rs reapplies on initial scan and every full scan if the Icon\r file is missing (idempotent, one stat per workspace). - reset.rs strips the icon before removing .git-same/, so workspace roots revert to plain blue folders when unregistered. The new core::macos module wraps NSWorkspace.setIcon via objc2 + objc2-app-kit (macOS-only target deps); non-macOS targets get no-op stubs so callers stay platform-agnostic. The ICNS is generated by a new folder-icon variant in toolkit/icons/generate-icons.swift + build-workspace-folder-icns.sh and embedded via include_bytes!. Opt-out via [ui] custom_folder_icon = false in the global config.
Closes the four remaining gaps from the workspace-folder-icon feature shipped in ea20eaa: - crates/git-same-app/src/commands.rs: paint the icon in Tauri save_workspace (gated on config.ui.custom_folder_icon, clearing the previous root if the user moved the workspace), and clear it in delete_workspace. The CLI/TUI path already does this via core::setup::save_workspace; the GUI path bypassed it. - crates/git-same-core/src/config/parser_tests.rs: three new tests for UiConfig - default-true, [ui] custom_folder_icon = false override, missing [ui] section round-trips to the default. - crates/git-same-cli/src/commands/reset_tests.rs: macOS-gated test asserting remove_workspace_dir strips the Icon\r before unwinding the workspace, using a tempdir + missing dot_dir so the global registry is never touched. - .claude/CLAUDE.md: new "Workspace folder branding" section documenting the feature, the [ui] opt-out, and the LSSharedFileList sidebar-snapshot caveat (manual remove + re-add is the only refresh path - no programmatic API exists).
The sign step for Contents/Helpers/git-same passed --sign but no --entitlements, so the cask-installed helper shipped with zero entitlements. macOS then treated every monitor access to ~/Library/Group Containers/group.57KL6Y7V32.com.zaai.git-same/ as cross-app and showed a recurring "Git-Same.app would like to access data from other apps" TCC prompt, attributed to the parent bundle. KeepAlive=true on the LaunchAgent made each respawn a fresh TCC subject so Allow never stuck. Apply crates/git-same-app/entitlements.plist to the helper and add a verify_helper_entitlements gate that runs after signing so the build fails loudly if the application-groups key ever goes missing again.
The sandboxed FinderSync extension triggered macOS's "access data from other apps" (kTCCServiceSystemPolicyAppData) prompt because it made filesystem calls that escaped its app-group container: resolvingSymlinksInPath() on browsed URLs and a /Volumes enumeration in bootVolumeAliasPrefixes(). The captured tccd log attributed the prompt to the extension (GitSameBadges.appex), not the monitor helper fixed in 2795f9a. All badge data already comes from status.json, so the extension needs no out-of-container access. Move boot-volume-alias detection to the non-sandboxed monitor, which now publishes the /Volumes/<name> prefixes in status.json (new boot_volume_aliases field), and replace resolvingSymlinksInPath() with pure-string prefix stripping. The extension's only filesystem handles are now the app-group container (status.json, finder.sock).
build-app-bundle.sh ran sign-app-bundle.sh via `"${SIGN_ARGS[@]}"`. When
notarizing, that array is empty, and macOS's stock bash 3.2 errors with
"unbound variable" on an empty array expansion under `set -u`, aborting
the build before signing/notarization ran and leaving an ad-hoc-signed
bundle. CI never hit it because GitHub runners resolve `bash` to 5.x.
Guard both expansions with ${SIGN_ARGS[@]+"${SIGN_ARGS[@]}"} so an empty
array expands to nothing instead of crashing.
The repositories subtitle was rendered via a bare {sourceLabel()} call,
which Svelte evaluates once at init and never re-runs because the template
expression references no reactive variable. When the GitHub structure loaded
after mount, the repo count updated but the subtitle stayed 'No structure
loaded', producing a contradictory 'N GitHub repos / No structure loaded'.
Bind the subtitle to a reactive $: sourceText = sourceLabel($workspaceStructure)
so it re-renders on load, and reword the source states to live / cached
<age> / cached (offline) / unavailable so a positive count never sits beside
'unavailable' or 'No structure loaded'.
Also tighten the paired table: 40px to 32px rows, slimmer owner-row padding,
muted centered middot placeholders (with title/aria-label) instead of italic
'Not on GitHub' / 'Not cloned locally' text, and an external-link icon that
fades in only on row hover or focus.
The paired GitHub/local table read as one flat list and did not make it obvious that the repo on the left of a line was the same as the one on the right. Restructure it into a comparison/diff view: - A center status gutter (with a hairline seam) carries a per-row glyph: link for matched, right arrow for missing-locally, left arrow for local-only. The absent side shows a muted ghost label. - Add a filter box in the panel head that narrows both sides in sync by repo name, GitHub full_name, or local folder name. - Zebra striping, whole-row hover, an accent owner band with a repo count, and a paired count in the subtitle improve scan-ability.
Bump out-of-range dependencies that needed manifest edits and code migration: - notify 6 -> 8 (no code change; watcher API stable) - objc2 0.5 -> 0.6, objc2-foundation/app-kit 0.2 -> 0.3; migrate folder_icon.rs to AnyThread for NSImage::alloc and drop now-safe unsafe blocks - Replace deprecated lucide-svelte with @lucide/svelte across 12 components - pnpm in-range minors: svelte, vite, svelte-check, @tauri-apps/cli GitHub Actions and rust-toolchain were already current. Transitive crates pinned by other deps' requirements are left as-is. Full workspace tests, clippy, svelte-check, and vite build all pass.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
WalkthroughConverts the repo into a Cargo workspace, adds shared core status/monitor/auth/IPC layers, introduces a Tauri desktop app and macOS FinderSync extension, and rewires the CLI, packaging, and release tooling around the new shared modules. ChangesWorkspace and tooling
Shared core
CLI and desktop app
Finder extension and packaging
Sequence Diagram(s)sequenceDiagram
participant SvelteUI
participant TauriApp
participant Core
participant Monitor
participant Finder
SvelteUI->>TauriApp: startSync / refresh / read_status
TauriApp->>Core: invoke commands
Core->>Monitor: scan_all / rescan_and_merge / write status
Monitor-->>Finder: status.json updates and socket replies
Finder->>Monitor: REFRESH path / REFRESH_ALL
Monitor-->>SvelteUI: status-updated / sync-progress
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
crates/git-same-app/Cargo.toml-16-27 (1)
16-27:⚠️ Potential issue | 🟡 MinorConsolidate Tauri ecosystem dependencies in workspace.
All three Tauri crates (
tauri,tauri-build, andtauri-plugin-dialog) specify only major version"2"but are missing from[workspace.dependencies]. This allows independent version drift (currently: 2.11.3, 2.6.3, 2.7.1) during lockfile refreshes. Either add them toworkspace.dependencieswith pinned versions like other ecosystem libraries (serde,tokio, etc.), or document why they require separate version management.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/Cargo.toml` around lines 16 - 27, The Tauri ecosystem dependencies (tauri-build, tauri, and tauri-plugin-dialog) in the git-same-app Cargo.toml file are not defined in workspace.dependencies, allowing them to drift independently. Add tauri-build, tauri, and tauri-plugin-dialog to the [workspace.dependencies] section with pinned specific versions (e.g., version = "2.x.x" format matching the currently locked versions), then update the git-same-app Cargo.toml to reference these workspace dependencies using the { workspace = true } syntax instead of specifying inline versions. This will ensure consistent version management across the workspace like the other ecosystem libraries (serde, chrono, notify, etc.).crates/git-same-core/src/provider/github/client.rs-271-285 (1)
271-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider URL-encoding the
nameparameter.The
nameparameter is directly interpolated into the URL path without encoding. While GitHub usernames are typically restricted to alphanumeric characters and hyphens, organization names or enterprise installations may have different constraints. Ifnamecontains characters like/,?, or#, the URL path could be malformed.🛡️ Recommended defensive fix
async fn get_owner_type(&self, name: &str) -> Result<OwnerType, ProviderError> { #[derive(serde::Deserialize)] struct UserOrOrg { #[serde(rename = "type")] kind: String, } - let url = self.api_url(&format!("/users/{}", name)); + let encoded_name = urlencoding::encode(name); + let url = self.api_url(&format!("/users/{}", encoded_name)); let payload: UserOrOrg = self.get(&url).await?; Ok(match payload.kind.as_str() { "User" => OwnerType::User, "Organization" => OwnerType::Organization, _ => OwnerType::Unknown, }) }You would need to add the
urlencodingcrate toCargo.tomlif not already present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/provider/github/client.rs` around lines 271 - 285, In the get_owner_type method, the name parameter is directly interpolated into the URL without encoding, which can cause malformed URLs if name contains special characters like forward slashes or query parameters. URL-encode the name parameter before inserting it into the URL path by using the urlencoding crate (add it to Cargo.toml if not present) and applying URL encoding to name in the format string where the URL is constructed with the format macro.crates/git-same-app/ui/src/routes/Requirements.svelte-36-42 (1)
36-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error handling for action promises.
All async actions (
createDefaultConfig,openUrl,installMonitor,restartMonitor) discard their promises withvoid, silently ignoring errors. If these operations fail, the user won't see feedback.🛡️ Recommended: Catch and surface errors
function runAction(action: 'config' | 'extensions' | 'fda' | 'monitor-install' | 'monitor-restart') { - if (action === 'config') void createDefaultConfig().then(loadRequirements); + if (action === 'config') { + createDefaultConfig() + .then(loadRequirements) + .catch((err) => console.error('Failed to create config:', err)); + } - if (action === 'extensions') void openUrl(EXTENSIONS_URL); + if (action === 'extensions') { + openUrl(EXTENSIONS_URL).catch((err) => console.error('Failed to open URL:', err)); + } - if (action === 'fda') void openUrl(FDA_URL); + if (action === 'fda') { + openUrl(FDA_URL).catch((err) => console.error('Failed to open URL:', err)); + } - if (action === 'monitor-install') void installMonitor(); + if (action === 'monitor-install') { + installMonitor().catch((err) => console.error('Failed to install monitor:', err)); + } - if (action === 'monitor-restart') void restartMonitor(); + if (action === 'monitor-restart') { + restartMonitor().catch((err) => console.error('Failed to restart monitor:', err)); + } }Alternatively, surface errors to the user via the
errorMessagestore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/routes/Requirements.svelte` around lines 36 - 42, The runAction function discards promises from async operations (createDefaultConfig, openUrl, installMonitor, restartMonitor) using void, which silently ignores any errors. Add error handling to each async operation by chaining a .catch() handler that surfaces the error to the user. Use the errorMessage store to capture and display error feedback so users are informed when operations fail. Ensure each promise-returning operation has a proper catch block rather than just wrapping it with void.crates/git-same-app/ui/src/routes/Dashboard.svelte-30-37 (1)
30-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winString prefix matching for paths can create false positives.
Line 35 uses
startsWithto check if a repo belongs to a workspace. This will incorrectly match repos when one workspace root is a string prefix of another (e.g.,/Users/m/reposwould match/Users/m/repos-archive).Consider normalizing paths with trailing slashes or using proper path comparison to ensure directory boundaries are respected.
🛡️ Suggested fix using normalized path comparison
function reposFor(workspace: WorkspaceSummary) { return repos.filter( (repo) => repo.workspace === workspace.id || repo.workspace === workspace.name || - repo.path.startsWith(workspace.root), + repo.path.startsWith(workspace.root.endsWith('/') ? workspace.root : workspace.root + '/'), ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/routes/Dashboard.svelte` around lines 30 - 37, The reposFor function's path comparison using startsWith on line 35 creates false positives when one workspace root is a string prefix of another (e.g., /Users/m/repos matches /Users/m/repos-archive). Fix this by normalizing both the workspace.root and repo.path with trailing directory separators before comparison, or alternatively use a proper path comparison utility that respects directory boundaries. Ensure the condition checks that repo.path starts with workspace.root followed by a path separator to prevent substring matches that don't represent actual parent-child directory relationships.crates/git-same-app/ui/src/routes/BadgeBrowser.svelte-30-48 (1)
30-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winString prefix matching for paths can create false positives.
Line 39 uses
startsWithto check if a repo belongs to a workspace. This will incorrectly match repos when one workspace root is a string prefix of another (e.g.,/Users/m/reposwould match/Users/m/repos-archive).Consider normalizing paths with trailing slashes or using proper path comparison to ensure directory boundaries are respected.
🛡️ Suggested fix using normalized path comparison
$: filtered = repos .filter((repo) => { const text = `${repo.path} ${repo.org ?? ''} ${repo.workspace ?? ''} ${repo.current_branch}`.toLowerCase(); const matchesSearch = !search.trim() || text.includes(search.trim().toLowerCase()); const matchesBadge = badgeFilter === 'all' || repo.badge === badgeFilter; const matchesWorkspace = workspaceFilter === 'all' || repo.workspace === workspaceFilter || $workspaces.some( - (workspace) => workspace.id === workspaceFilter && repo.path.startsWith(workspace.root), + (workspace) => workspace.id === workspaceFilter && + repo.path.startsWith(workspace.root.endsWith('/') ? workspace.root : workspace.root + '/'), ); return matchesSearch && matchesBadge && matchesWorkspace; })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/routes/BadgeBrowser.svelte` around lines 30 - 48, The matchesWorkspace filter uses startsWith to check if a repo path belongs to a workspace, which can create false positives when one workspace root is a string prefix of another (e.g., /Users/m/repos matching /Users/m/repos-archive). Fix this by normalizing paths with trailing slashes before the comparison in the startsWith check on workspace.root, or by using a proper path comparison function that respects directory boundaries to ensure prefix matching only occurs at directory boundaries.crates/git-same-app/ui/src/lib/StatStrip.svelte-6-15 (1)
6-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winString prefix matching for paths can create false positives.
Lines 11 and 13 use
startsWithto check if a repo path belongs to a workspace. This will incorrectly match repos when one workspace root is a string prefix of another (e.g.,/Users/m/reposwould match/Users/m/repos-archive).Consider normalizing paths with trailing slashes or using proper path comparison to ensure directory boundaries are respected.
🛡️ Suggested fix using normalized path comparison
$: repos = $snapshot?.status?.repos ?? []; $: workspaceRepos = $selectedWorkspaceId ? repos.filter( (repo) => repo.workspace === $selectedWorkspaceId || ($currentWorkspace?.root - ? repo.path.startsWith($currentWorkspace.root) + ? repo.path.startsWith($currentWorkspace.root.endsWith('/') + ? $currentWorkspace.root + : $currentWorkspace.root + '/') : false) || - repo.path.startsWith($selectedWorkspaceId), + repo.path.startsWith($selectedWorkspaceId.endsWith('/') + ? $selectedWorkspaceId + : $selectedWorkspaceId + '/'), ) : repos;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/lib/StatStrip.svelte` around lines 6 - 15, The startsWith checks on lines 11 and 13 in the workspaceRepos filter logic can create false positives when filesystem paths share prefixes (e.g., /Users/m/repos matching /Users/m/repos-archive). Normalize both the repo.path and the workspace root paths by ensuring they have trailing slashes before performing the startsWith comparison, or implement proper path comparison logic that respects directory boundaries. Apply this normalization to both the $currentWorkspace.root comparison and the $selectedWorkspaceId comparison to ensure only repos that actually belong to the workspace are included in the filtered results.crates/git-same-core/src/ipc/status_file.rs-134-137 (1)
134-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSymlink target comparison may fail for relative vs absolute paths.
Line 135 compares the raw symlink target from
read_link()againsttarget_pathwithout canonicalization. If the existing symlink is relative (e.g.,../../group/status.json) andtarget_pathis absolute (or vice versa), this equality check will fail even when both resolve to the same file, causing the function to unnecessarily remove and recreate the symlink on every invocation.🔧 Proposed fix to canonicalize both paths before comparison
match std::fs::symlink_metadata(legacy_path) { Ok(meta) if meta.file_type().is_symlink() => { - if std::fs::read_link(legacy_path).ok().as_deref() == Some(target_path) { + let current_target = std::fs::read_link(legacy_path).ok(); + let targets_match = current_target + .as_ref() + .and_then(|t| t.canonicalize().ok()) + .zip(target_path.canonicalize().ok()) + .map(|(a, b)| a == b) + .unwrap_or(false); + if targets_match { return Ok(()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/ipc/status_file.rs` around lines 134 - 137, The symlink target comparison at line 135 compares the raw path from std::fs::read_link(legacy_path) against target_path without canonicalizing them first. Since read_link may return a relative path while target_path could be absolute (or vice versa), they may represent the same file but fail the equality check, causing unnecessary symlink recreation. Canonicalize both the result from read_link(legacy_path) and target_path before comparing them to ensure the comparison handles both relative and absolute paths correctly.docs/plans/refresh-subcommand.md-5-49 (1)
5-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
monitoras the canonical verb in the examples.The plan still hard-codes
gisa daemon, but this PR renames that command tomonitorand keepsdaemononly as a compatibility alias. Updating the examples now keeps the docs aligned with the shipped CLI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/refresh-subcommand.md` around lines 5 - 49, The documentation references the old daemon command in multiple places, but the PR renames it to monitor. Update all occurrences of `gisa daemon` to `gisa monitor` throughout the file, including in the error message within the run function's handler that says "Start it with `gisa daemon`" and in the Verification section where the first step mentions running `gisa daemon` in a terminal, to ensure the examples reflect the new canonical command name.crates/git-same-core/src/provider/mock.rs-243-245 (1)
243-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror the mock to the new
get_owner_typecontract.
MockProvidercurrently returnsOrganizationfor every lookup and skipslog_call, so tests can't exercise user-owned paths or assert this method was used. Please branch on the configured username/orgs and record the invocation like the other trait methods.🛠️ Suggested fix
async fn get_owner_type(&self, _name: &str) -> Result<OwnerType, ProviderError> { - Ok(OwnerType::Organization) + self.log_call(&format!("get_owner_type:{}", _name)); + if _name == self.username { + return Ok(OwnerType::User); + } + if self.orgs.iter().any(|org| org.login == _name) { + return Ok(OwnerType::Organization); + } + Ok(OwnerType::Unknown) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/provider/mock.rs` around lines 243 - 245, The get_owner_type method in MockProvider always returns OwnerType::Organization regardless of the provided name parameter and does not call log_call to record the invocation. Update the get_owner_type method to first call log_call with the method name and parameters (similar to how other MockProvider trait methods do), then branch on the configured username and organizations in the mock to return the appropriate OwnerType (User if the name matches a configured user, Organization otherwise) instead of unconditionally returning Organization.crates/git-same-core/src/monitor/run.rs-186-196 (1)
186-196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNon-unix branch silently swallows full-scan errors.
The unix branch logs full-scan failures via
error!, but the non-unix branch usesif let Ok(...)which silently ignores errors. This makes debugging harder on Windows.🐛 Proposed fix to log errors on non-unix
_ = tokio::time::sleep(interval) => { debug!("Safety-net full scan"); - if let Ok(new_status) = service.scan_all(pid) { - reapply_workspace_folder_icons(config, &new_status); - let mut status = shared_status.lock().expect("status mutex poisoned"); - *status = new_status; - let _ = status_writer.write(&status); + match service.scan_all(pid) { + Ok(new_status) => { + reapply_workspace_folder_icons(config, &new_status); + let mut status = shared_status.lock().expect("status mutex poisoned"); + *status = new_status; + if let Err(e) = status_writer.write(&status) { + error!(error = %e, "Failed to write status file after full scan"); + } + } + Err(e) => { + error!(error = %e, "Full scan failed"); + } } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/monitor/run.rs` around lines 186 - 196, In the safety-net full scan block triggered by tokio::time::sleep(interval), the current code uses if let Ok to silently ignore errors from service.scan_all(pid), whereas the unix branch logs these failures. Change the error handling pattern from if let Ok(new_status) to match the error case as well, and add an error! log call when service.scan_all fails to ensure errors are logged for debugging on non-unix platforms.crates/git-same-app/ui/src/main.ts-5-7 (1)
5-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd runtime check for missing mount target.
The non-null assertion
!ongetElementById('app')will cause a runtime crash if the element is missing. Add a defensive check to provide a clearer error message.🛡️ Proposed fix
+const target = document.getElementById('app'); +if (!target) { + throw new Error('Failed to find `#app` element'); +} + const app = mount(App, { - target: document.getElementById('app')!, + target, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/main.ts` around lines 5 - 7, The current code uses a non-null assertion on document.getElementById('app') which will cause an undefined runtime error if the DOM element is missing. Instead, store the result of document.getElementById('app') in a variable without the non-null assertion, add a defensive check to verify the element exists before passing it to the mount function, and throw or log a descriptive error message if the element is not found on the page.crates/git-same-app/ui/src/lib/Topbar.svelte-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse workspace name instead of ID for the title.
The title displays
$currentWorkspace?.idwhich will show a UUID or slug instead of a human-readable workspace name. This is inconsistent withTitleBar.svelteline 38, which correctly uses.name.📝 Proposed fix
- $: title = isSettings ? 'Settings' : $currentWorkspace?.id ?? 'Dashboard'; + $: title = isSettings ? 'Settings' : $currentWorkspace?.name ?? 'Dashboard';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/lib/Topbar.svelte` at line 14, The title assignment in Topbar.svelte is using $currentWorkspace?.id which displays a UUID or slug instead of a human-readable name. Replace the .id property accessor with .name in the reactive assignment for the title variable to display the workspace name instead, maintaining the same fallback to 'Dashboard' when there is no current workspace, which will be consistent with how TitleBar.svelte handles workspace display.
🧹 Nitpick comments (22)
.github/workflows/S5-Build-MacOS-App.yml (1)
23-24: ⚡ Quick winConsider scoping
contents: writepermission to the release job only.The
contents: writepermission is granted at the workflow level (line 24), but only thereleasejob (lines 104-135) needs write access to attach DMG artifacts to the GitHub release. Thebuildjob (lines 27-103) only needs read access.Scoping permissions to the job level reduces the attack surface if the build job is compromised.
🔐 Scope permissions to job level
-permissions: - contents: write - jobs: build: name: Build App (${{ matrix.arch }}) runs-on: macos-14 + permissions: + contents: read strategy: ... release: name: Attach App DMGs to GitHub Release needs: [build] runs-on: ubuntu-latest + permissions: + contents: write steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/S5-Build-MacOS-App.yml around lines 23 - 24, The contents: write permission is currently set at the workflow level (lines 23-24) but only the release job needs write access to attach DMG artifacts to releases, while the build job only requires read access. Remove the permissions block from the workflow level and add a permissions block scoped to only the release job (the one that handles artifact attachment around lines 104-135) with contents: write, leaving the build job with default read-only permissions to reduce the attack surface..github/workflows/S4-Publish-Crates.yml (1)
31-45: ⚡ Quick winConsider more robust version extraction from workspace manifest.
Line 34 uses a fragile awk pattern that assumes
version = "..."appears at the start of a line in the root Cargo.toml. While this works with the current workspace layout (line 10 has no leading whitespace), it could break if:
- The workspace.package section is reformatted with indentation
- Additional version fields are added in other sections
Consider using
cargo metadata --no-deps --format-version 1 | jq -r '.packages[] | select(.name == "git-same-core") | .version'or explicitly parsing the[workspace.package]section.🔧 More robust version extraction
- - name: Wait for crates.io to index git-same-core - run: | - set -euo pipefail - VERSION=$(awk -F'"' '/^version *=/ {print $2; exit}' Cargo.toml) + - name: Wait for crates.io to index git-same-core + run: | + set -euo pipefail + VERSION=$(cargo metadata --no-deps --format-version 1 \ + | jq -r '.packages[] | select(.name == "git-same-core") | .version')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/S4-Publish-Crates.yml around lines 31 - 45, The version extraction logic using awk on line 34 is fragile because it assumes the version field starts at the beginning of a line in Cargo.toml, which could break if the workspace.package section gets indented or reformatted. Replace the awk pattern that extracts VERSION with a more robust approach using cargo metadata command with jq to directly query the package information (selecting the git-same-core package and extracting its version field), or alternatively parse the workspace.package section more explicitly. This ensures the version extraction remains reliable regardless of formatting changes in the Cargo.toml file structure.macos/com.zaai.git-same.monitor.plist (1)
18-21: 🏗️ Heavy liftConsider persistent log locations for production daemon.
The monitor logs are written to
/tmp/, which is cleared on reboot and world-readable. For a production daemon, consider using~/Library/Logs/git-same-monitor.logor/var/log/git-same-monitor.logto ensure logs persist across reboots and aid debugging.This is especially valuable since
KeepAliveis enabled — if the daemon crashes repeatedly, the logs may not survive to help diagnose the issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/com.zaai.git-same.monitor.plist` around lines 18 - 21, The StandardOutPath and StandardErrorPath values in the plist are currently pointing to /tmp/ which gets cleared on reboot and has world-readable permissions. Replace both the StandardOutPath and StandardErrorPath values with a persistent log location that survives reboots and maintains appropriate permissions. Use either ~/Library/Logs/git-same-monitor.log for user-scoped logging or /var/log/git-same-monitor.log for system-scoped logging, depending on your deployment requirements. This is critical since KeepAlive is enabled and repeated daemon crashes will have their logs lost if they remain in /tmp/.crates/git-same-app/src/status_stream.rs (1)
31-38: ⚡ Quick winSilent error handling hides status watcher failures.
The event loop silently ignores:
- All
notifywatcher errors (lines 32-34)read_status_snapshot()failures (line 35)app.emit()failures (line 36)If the watcher stops working (filesystem errors, permission issues, corrupted status file), users won't receive any feedback that live updates have stopped. This could lead to confusion when the dashboard shows stale data.
Consider logging errors to help with debugging and potentially exposing critical failures to the UI.
📝 Proposed improvement to add error logging
for event in rx { - if event.is_err() { + if let Err(error) = event { + eprintln!("status watcher error: {error}"); continue; } if let Ok(snapshot) = read_status_snapshot() { let _ = app.emit("status-updated", snapshot); + } else { + eprintln!("failed to read status snapshot"); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/src/status_stream.rs` around lines 31 - 38, The event loop in the status watcher is silently ignoring three types of errors that should be logged: the error from the notify watcher when event.is_err() is true (lines 32-34), the error when read_status_snapshot() fails (line 35), and the error when app.emit() fails (line 36). Instead of using continue without logging and binding errors with underscore in the let statement, capture and log each error appropriately so that watcher failures, file read issues, and emission failures are visible for debugging. This will help prevent users from being confused by stale dashboard data when the status update mechanism fails silently.crates/git-same-app/src/main.rs (1)
27-32: ⚖️ Poor tradeoffStatus watcher failures are invisible to users.
The setup hook logs watcher startup failures with
eprintln!, which won't be visible to users who launch the app normally (not from a terminal). The app continues running without live status updates, but users won't know why their dashboard isn't updating.Consider surfacing this error in the UI (e.g., a startup banner or warning in the status area) so users can troubleshoot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/src/main.rs` around lines 27 - 32, The status watcher error handling in the setup closure currently uses eprintln! to log failures, which is invisible to users running the app without a terminal. Instead of using eprintln!, modify the error handling in the setup closure to surface the watcher startup failure to the UI, such as by storing the error state in the app or emitting an event that can be displayed to users. This ensures that when status_stream::spawn_watcher fails, users are informed through the application interface rather than through invisible stderr output.crates/git-same-core/src/provider/github/pagination.rs (1)
113-119: 💤 Low valueConsider clarifying the timeout error message.
The error message says "budget for '{initial_url}'" which could be confusing if pagination has already fetched multiple pages before timing out. Consider rephrasing to indicate it's the overall pagination operation that timed out.
💬 Suggested improvement
if Instant::now() >= deadline { return Err(ProviderError::Network(format!( - "Pagination exceeded {}s budget for '{}'", + "Pagination exceeded {}s budget while fetching pages from '{}'", PAGINATION_DEADLINE.as_secs(), initial_url ))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/provider/github/pagination.rs` around lines 113 - 119, In the pagination timeout check where Instant::now() >= deadline evaluates to true, rephrase the error message format string to better clarify that the overall pagination operation has timed out, rather than implying it's specific to the initial_url. Instead of referencing the initial_url in a way that suggests only that specific page failed, use clearer language that indicates the entire pagination process exceeded the deadline budget.crates/git-same-cli/src/commands/monitor_tests.rs (1)
9-13: ⚡ Quick winConsider using a PID that's guaranteed to be invalid.
Testing with PID 99999 is fragile—on some systems this PID could theoretically exist. Consider using
u32::MAXto ensure the PID is out of range.♻️ More reliable test
#[test] fn test_is_process_alive_nonexistent() { - // PID 99999 is very unlikely to exist - assert!(!is_process_alive(99999)); + // Use PID that's guaranteed to be invalid + assert!(!is_process_alive(u32::MAX)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-cli/src/commands/monitor_tests.rs` around lines 9 - 13, The test_is_process_alive_nonexistent function uses a hardcoded PID value of 99999 which could theoretically exist on some systems, making the test unreliable. Replace the PID argument (99999) passed to the is_process_alive function call with u32::MAX to guarantee the PID is out of range on all systems and make the test more robust.crates/git-same-core/src/ipc/unix_socket_tests.rs (1)
9-13: ⚡ Quick winConsider using a PID that's guaranteed to be invalid.
Testing with PID 99999 is fragile—on some systems this PID could theoretically exist. Consider using
u32::MAXor a similar value that's guaranteed to be out of range.♻️ More reliable test
#[test] fn test_is_process_alive_nonexistent() { - // PID 99999 is very unlikely to exist - assert!(!is_process_alive(99999)); + // Use PID that's guaranteed to be invalid + assert!(!is_process_alive(u32::MAX)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/ipc/unix_socket_tests.rs` around lines 9 - 13, In the unix_socket_tests.rs file, locate any test that uses a hardcoded PID value like 99999 (which may not be visible in the provided diff). Replace this hardcoded PID value with u32::MAX or another guaranteed out-of-range value that's portable across all systems and cannot theoretically exist as a valid process ID. This makes the test more robust and reliable.crates/git-same-app/ui/src/routes/Settings.svelte (1)
61-80: 💤 Low valueConsider extracting default values to avoid duplication.
The defaults in
restoreDefaultsare duplicated from the initial state (lines 15-33). If defaults change, both places need updates. Extract them into a constant or factory function.♻️ Optional: Extract defaults
+const DEFAULTS = { + structure: '{org}/{repo}', + concurrency: 4, + syncMode: 'fetch' as const, + // ... etc +}; + let loadedPath = ''; let saving = false; -let structure = '{org}/{repo}'; -let concurrency = 4; +let structure = DEFAULTS.structure; +let concurrency = DEFAULTS.concurrency; // ... function restoreDefaults() { - structure = '{org}/{repo}'; - concurrency = 4; + structure = DEFAULTS.structure; + concurrency = DEFAULTS.concurrency; // ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/routes/Settings.svelte` around lines 61 - 80, The default values in the restoreDefaults function are duplicated from the initial variable declarations elsewhere in the file. Extract these default values (structure, concurrency, syncMode, defaultWorkspace, refreshInterval, cloneDepth, cloneBranch, cloneSubmodules, includeArchived, includeForks, filterOrgsText, filterExcludeText, showAmbient, scanRootsText, finderMaxDepth, finderExcludeText, and monitorFullscanInterval) into a single constant object or factory function defined outside of restoreDefaults. Then update both the initial state assignment and the restoreDefaults function to use this single source of truth, eliminating the duplication.crates/git-same-app/ui/src/routes/Requirements.svelte (1)
26-34: ⚡ Quick winString matching for check names is brittle.
actionForrelies on exact string matches likename === 'Config file'. If check names change in the backend, these actions will break silently. Consider using an enum or constant for check names, or adding akindfield to the check structure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/routes/Requirements.svelte` around lines 26 - 34, The actionFor function uses hardcoded string literals for matching check names, which makes it brittle and prone to silent failures if the backend changes these values. Replace the exact string matching comparisons in actionFor with either constants defined at the module level or an enum that represents the different check types. Alternatively, if possible, modify the check data structure to include a kind or type field and use that for matching instead of relying on the name property. This will make the code more maintainable and catch mismatches at build time rather than runtime.crates/git-same-core/src/auth/process.rs (1)
33-34: 💤 Low valueConsider logging errors from child cleanup.
Both
kill()andwait()errors are silently discarded. While this is usually fine, a failedwait()could leave a zombie process, and logging would help diagnose such cases.🔍 Optional: Log cleanup errors
- let _ = child.kill(); - let _ = child.wait(); + if let Err(e) = child.kill() { + debug!(error = %e, "Failed to kill timed-out child"); + } + if let Err(e) = child.wait() { + warn!(error = %e, "Failed to reap timed-out child; may leave zombie"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/auth/process.rs` around lines 33 - 34, The errors returned from child.kill() and child.wait() calls are being silently discarded with let _ =, which prevents visibility into cleanup failures that could result in zombie processes. Replace the let _ = discards with proper error handling that logs any errors encountered during the kill() and wait() calls. This will provide diagnostic information when child process cleanup fails, particularly important for identifying zombie processes from failed wait() operations.crates/git-same-app/ui/src/routes/Workspace.svelte (1)
74-108: ⚖️ Poor tradeoffDuplicate paths will silently overwrite entries.
Lines 81 and 86-87 assume paths are unique. If multiple remote repos map to the same
local_path, or multiple local repos share a path, only the first entry is retained. While duplicates are unlikely in practice, adding a warning when duplicates are detected would help diagnose configuration issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-app/ui/src/routes/Workspace.svelte` around lines 74 - 108, The buildPairs function silently overwrites entries when multiple remotes or locals map to the same normalized path, which can hide configuration issues. Add warning logs when duplicate paths are detected: when byPath already contains a key before adding a remote entry (near the first for loop processing remotes), and when a local entry with the same path is encountered but a slot already exists with a populated local property (in the else if condition in the second for loop processing locals). These warnings will help users identify configuration problems that cause data loss.crates/git-same-core/src/auth/process_tests.rs (1)
3-31: 💤 Low valueTests rely on Unix-specific commands.
The
trueandsleepcommands are not available on Windows. Since the PR targets macOS, this is acceptable, but consider adding#[cfg(unix)]guards to make the platform requirement explicit and prevent confusing test failures on other platforms.📝 Optional: Add platform guard
+#[cfg(unix)] #[test] fn returns_output_for_fast_command() { +#[cfg(unix)] #[test] fn returns_error_for_missing_binary() { +#[cfg(unix)] #[test] fn times_out_and_kills_slow_command() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/auth/process_tests.rs` around lines 3 - 31, The tests in this file use Unix-specific commands that are not available on Windows, which can cause confusing failures on other platforms. Add the `#[cfg(unix)]` attribute guard to the test functions that rely on Unix commands: specifically `returns_output_for_fast_command` (which uses the "true" command) and `times_out_and_kills_slow_command` (which uses the "sleep" command). This makes the platform requirement explicit in the code and prevents unexpected test failures.crates/git-same-core/src/discovery.rs (1)
328-338: ⚖️ Poor tradeoffRisk of infinite loop if
canonicalizefails within a symlink cycle.The fallback at line 329 allows non-canonical paths into the visited set when
canonicalizefails. If a symlink cycle exists andcanonicalizeconsistently fails for paths within that cycle, the visited-set deduplication will fail, potentially causing an infinite loop.Consider skipping the child entirely when
canonicalizefails, or document this edge case as a known limitation.🛡️ Safer alternative: skip on canonicalization failure
let child = entry.path(); - let canonical_child = std::fs::canonicalize(&child).unwrap_or(child); + let Ok(canonical_child) = std::fs::canonicalize(&child) else { + continue; + }; walk_for_repos(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/discovery.rs` around lines 328 - 338, The code at line 329 uses a fallback to the non-canonical path when canonicalize fails, which can cause the visited set deduplication to miss duplicate entries if a symlink cycle exists, potentially leading to an infinite loop in the walk_for_repos recursive function. Instead of using unwrap_or to fallback to the non-canonical child path, skip the recursive walk_for_repos call entirely when canonicalize fails, ensuring that only successfully canonicalized paths are processed and added to the visited set.crates/git-same-core/src/ipc/mod_tests.rs (1)
71-84: 💤 Low valueConsider making the macOS test conditional instead of using early-return.
The test silently skips all assertions when
HOMEis unset, which could mask test failures in certain CI environments. While most environments setHOME, this pattern makes test results non-deterministic.Consider using
#[cfg_attr(not(target_os = "macos"), ignore)]or failing explicitly when the precondition isn't met, rather than passing silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/ipc/mod_tests.rs` around lines 71 - 84, The test function test_default_path_uses_group_container_on_macos silently returns when the HOME environment variable is not set, causing the test to pass without running any assertions and potentially masking failures in CI environments. Replace the early return pattern with an explicit panic statement that clearly indicates the HOME environment variable is required for this test to run, ensuring test failures are not hidden when preconditions are not met.crates/git-same-core/src/ipc/status_file_tests.rs (1)
191-208: ⚡ Quick winTest does not verify the documented no-op behavior.
The test name
ensure_legacy_symlinks_is_noop_when_legacy_dir_missingsuggests it verifies that the publicensure_legacy_symlinks()helper is a no-op when the legacy directory doesn't exist (as documented in the function's docstring). However, the test:
- Calls the private
ensure_one_symlink()instead of the public helper- Uses an unwritable system path rather than a missing legacy dir
- Discards the result with
let _ = ...and asserts nothingConsider either testing the actual no-op contract by calling
ensure_legacy_symlinks()with a tempdir that has no legacy folder, or renaming the test to reflect that it's a panic-safety check forensure_one_symlink().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/ipc/status_file_tests.rs` around lines 191 - 208, The test `ensure_legacy_symlinks_is_noop_when_legacy_dir_missing` claims to verify the no-op behavior of the public `ensure_legacy_symlinks()` function but instead tests the private `ensure_one_symlink()` with an unwritable system path and makes no assertions. Either refactor the test to call the actual public `ensure_legacy_symlinks()` function with a temporary directory containing no legacy folder and verify that it returns without side effects, or rename the test to accurately reflect that it's testing `ensure_one_symlink()` for panic-safety with unwritable paths and update the comments accordingly to match the actual behavior being tested.crates/git-same-core/src/monitor/run.rs (2)
263-285: 💤 Low valueO(n²) membership check when collecting watched roots.
roots.contains(&canonical)is O(n) inside a loop over workspaces and scan_roots. For typical workspace counts this is negligible, but using aHashSetwould be more idiomatic.♻️ Suggested fix using HashSet
fn collect_watched_roots(config: &Config, status: &FinderStatus) -> Vec<PathBuf> { - let mut roots: Vec<PathBuf> = Vec::new(); + let mut seen: std::collections::HashSet<PathBuf> = std::collections::HashSet::new(); for ws in &status.workspaces { let canonical = std::fs::canonicalize(&ws.root).unwrap_or_else(|_| ws.root.clone()); - if !roots.contains(&canonical) { - roots.push(canonical); - } + seen.insert(canonical); } if config.finder.show_ambient { for raw in &config.finder.scan_roots { let expanded = shellexpand::tilde(raw).to_string(); let path = PathBuf::from(expanded); if !path.exists() { continue; } let canonical = std::fs::canonicalize(&path).unwrap_or(path); - if !roots.contains(&canonical) { - roots.push(canonical); - } + seen.insert(canonical); } } - roots + seen.into_iter().collect() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/monitor/run.rs` around lines 263 - 285, The collect_watched_roots function performs O(n²) membership checks by calling roots.contains() inside loops over workspaces and scan_roots. Replace the Vec<PathBuf> for roots with a HashSet<PathBuf> to enable O(1) membership checks using insert() instead of the contains() and push() pattern. Convert the HashSet back to a Vec<PathBuf> before returning to maintain the function's return type, since the function signature should remain unchanged.
124-145: 💤 Low valueRace between debounce and interval timers may cause redundant full scans.
When
pendingis non-empty and both the debounce timer (750ms) and interval timer expire "simultaneously" (within the same select! wake), tokio::select! picks one branch. If the interval branch wins, the full scan replacesstatusentirely — then the debounce branch fires on the next iteration and rescans the same repos again, wasting CPU.Consider draining
pendinginside the interval branch before replacingstatus, or clearingpendingafter a full scan replaces it.♻️ Suggested fix
_ = tokio::time::sleep(interval) => { debug!("Safety-net full scan"); + pending.clear(); // Full scan supersedes pending incremental rescans match service.scan_all(pid) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/monitor/run.rs` around lines 124 - 145, A race condition exists in the tokio::select! block where both the debounce timer and interval timer may expire simultaneously, causing redundant scans. When the interval branch (which calls service.scan_all) wins the race and replaces the status, the pending variable still contains entries that will be rescanned on the next debounce timer firing. Fix this by clearing the pending collection after the full scan completes in the interval branch, either immediately after calling service.scan_all or after successfully updating the shared_status, so that the subsequent debounce branch iteration has nothing to rescan.crates/git-same-core/src/monitor/socket_handler.rs (1)
44-52: ⚖️ Poor tradeoffNew
ShellGitandRepoScanServicecreated per socket connection.Each connection instantiates a fresh service. This is likely fine for the simple commands, but if
ShellGit::new()or service setup becomes expensive, consider sharing a pre-built service reference passed from the caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/monitor/socket_handler.rs` around lines 44 - 52, The current implementation creates a new ShellGit instance via ShellGit::new() and a new RepoScanService via RepoScanService::new() for every socket connection, which could become expensive if initialization costs grow. Refactor the socket handler function to accept pre-built references to ShellGit and RepoScanService as parameters from the caller instead of instantiating them locally. Keep the builder pattern calls with_owner_types and with_ambient_upgrades if they need to be applied per-connection, but avoid recreating the base service object. This allows expensive initialization to happen once during startup rather than per connection.crates/git-same-core/src/types/finder_status.rs (1)
179-195: 💤 Low valueWell-chosen patterns for common credential files.
The current set covers the most common secret/credential file types. If you want to be more comprehensive, consider adding SSH private keys (
id_rsa,id_dsa,id_ecdsa,id_ed25519),.aws/*,.npmrc,.pypirc,.dockercfg, and.netrc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-same-core/src/types/finder_status.rs` around lines 179 - 195, Add additional credential file patterns to the DEFAULT_IMPORTANT_IGNORED_PATTERNS constant to improve coverage of common secrets and configuration files. Extend the patterns array to include SSH private keys (id_rsa, id_dsa, id_ecdsa, id_ed25519), AWS configuration (.aws/*), npm configuration (.npmrc), Python package index configuration (.pypirc), Docker configuration (.dockercfg), and network authentication files (.netrc) to provide more comprehensive detection of sensitive files that would be lost if the repository were deleted.macos/GitSameBadges/Principal.swift (1)
16-17: 💤 Low valueMinor:
lastRefreshRequestdictionary grows without bound.The throttle dictionary accumulates entries for every unique path that triggers
requestRefresh, but entries are never removed. In a long-running session with many path requests, this could slowly leak memory.Consider adding a periodic cleanup or using a cache with TTL eviction. However, given that FinderSync extensions have a limited lifetime and the paths are bounded by monitored directories, this is unlikely to cause issues in practice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/GitSameBadges/Principal.swift` around lines 16 - 17, The `lastRefreshRequest` dictionary accumulates entries for every unique path without removing them, causing potential memory growth over time. Implement cleanup logic to remove stale entries from this dictionary, either by periodically purging entries older than the `refreshThrottle` interval or by adding logic to remove entries when they're no longer needed. Consider adding a cleanup method that iterates through the dictionary and removes any entries where the time difference between the stored date and the current time exceeds the throttle interval.macos/Shared/SocketProtocol.swift (1)
20-51: 💤 Low valueMinor: Completion handler may not fire if connection enters
.cancelledstate without.failed.The state handler ignores
.cancelledand.waitingstates. If the connection is cancelled externally or enters an unexpected state path, the completion handler won't be called, potentially leaving callers waiting indefinitely. In practice this is unlikely since.failedhandles connection errors, but consider adding a timeout or handling additional states for robustness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Shared/SocketProtocol.swift` around lines 20 - 51, The state update handler in the connection stateUpdateHandler switch statement only handles .ready and .failed cases, leaving .cancelled and .waiting states unhandled, which means the completion callback may never fire if the connection enters these states. Add explicit case handlers for .cancelled and .waiting states in the switch statement that call completion with an appropriate error (such as a connection cancelled or timeout error) to ensure the completion handler always gets invoked regardless of which connection state is reached.
Comment on lines
8
to
10
| fn git_same_binary() -> PathBuf { | ||
| std::env::var_os("CARGO_BIN_EXE_git_same") | ||
| .map(PathBuf::from) | ||
| .unwrap_or_else(|| { | ||
| let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
| path.push("target/debug/git-same"); | ||
| #[cfg(target_os = "windows")] | ||
| path.set_extension("exe"); | ||
| path | ||
| }) | ||
| PathBuf::from(env!("CARGO_BIN_EXE_git-same")) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check which CARGO_BIN_EXE_* variables Cargo sets for the git-same binary.
# Expected: CARGO_BIN_EXE_git_same (with underscore) should be set.
# If env!("CARGO_BIN_EXE_git-same") with hyphen is used but only the underscore version exists, the build will fail.
cd crates/git-same-cli
cargo test --test integration_test --no-run -- --nocapture 2>&1 | head -20 || true
# Also check the binary name in Cargo.toml
echo "=== Binary name in Cargo.toml ==="
rg -A2 '^\[\[bin\]\]' Cargo.toml || rg '^name.*git-same' Cargo.toml
# Check if there are any other usages of CARGO_BIN_EXE in the codebase
echo "=== Other CARGO_BIN_EXE usages ==="
rg 'CARGO_BIN_EXE' --type rustRepository: ZAAI-com/git-same
Length of output: 1078
Change CARGO_BIN_EXE_git-same to CARGO_BIN_EXE_git_same.
Cargo normalizes binary names by replacing hyphens with underscores in CARGO_BIN_EXE_* environment variables. The binary is named git-same, so the variable should be CARGO_BIN_EXE_git_same. Using env!("CARGO_BIN_EXE_git-same") will fail at compile time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/git-same-cli/tests/integration_test.rs` around lines 8 - 10, In the
git_same_binary() function, change the environment variable name from
CARGO_BIN_EXE_git-same to CARGO_BIN_EXE_git_same. Cargo normalizes binary names
by replacing hyphens with underscores in the CARGO_BIN_EXE_* environment
variable names, so the variable must use underscore instead of hyphen to
correctly reference the git-same binary at compile time.
…provider routing Triage all 16 PR #16 review comments and fix the actionable ones: - owner_type_cache: recover from mutex poisoning at every lock site so a panic in one task can't permanently break the cache - socket_handler: terminate STATUS response with a newline to match the line-framed protocol (extracted testable status_response helper) - unix_socket: stop trimming REFRESH path args (preserves valid whitespace) - status.ts: catch listWorkspaces/readStatus rejections in refresh() so callers can't trigger unhandled promise rejections - owner_classifier: group owners by per-workspace provider endpoint so GitHub Enterprise workspaces are no longer queried against github.com - monitor: cfg(unix) guards + non-Unix fallbacks for kill-based helpers - service: move stale ambient-cache eviction out of the and_then closure - pagination: re-check the deadline after the rate-limit sleep - S2 release: pin action-gh-release to v2.6.2 to avoid the v3 checksum upload regression (#790) that affects this repo's .sha256 assets Skipped: status.rs error tracking and the [gone]-upstream badge bug were already fixed; the CARGO_BIN_EXE_git-same env var is correct as-is (Cargo keeps the hyphen). Adds 6 tests; build, clippy, core tests and svelte-check all pass.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/S2-Release-GitHub.yml:
- Line 391: The `softprops/action-gh-release` action reference at line 391 uses
a mutable tag version `@v2.6.2` instead of an immutable commit SHA, which poses
a security risk. Replace the tag reference `@v2.6.2` with the full commit SHA
that corresponds to that release version (find this by checking the
softprops/action-gh-release repository's releases or tags page), and update the
uses statement to pin to that specific commit hash instead of the version tag.
In `@crates/git-same-core/src/monitor/owner_classifier.rs`:
- Around line 72-75: The code silently ignores persistence errors when calling
cache.set to store OwnerType::Unknown in the error handler block (around line
74), which is inconsistent with error reporting elsewhere in the codebase.
Instead of discarding the result with let _ = cache.set(...), capture the result
and log any errors that occur when attempting to persist the Unknown owner type
to cache, ensuring that persistence failures on this fallback path are not
silently ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8fdea75-fffc-483c-9fff-e8e620627efa
📒 Files selected for processing (13)
.github/workflows/S2-Release-GitHub.ymlcrates/git-same-app/ui/src/stores/status.tscrates/git-same-cli/src/commands/monitor.rscrates/git-same-core/src/api/owner_type_cache.rscrates/git-same-core/src/api/owner_type_cache_tests.rscrates/git-same-core/src/api/service.rscrates/git-same-core/src/ipc/unix_socket.rscrates/git-same-core/src/ipc/unix_socket_tests.rscrates/git-same-core/src/monitor/owner_classifier.rscrates/git-same-core/src/monitor/owner_classifier_tests.rscrates/git-same-core/src/monitor/socket_handler.rscrates/git-same-core/src/monitor/socket_handler_tests.rscrates/git-same-core/src/provider/github/pagination.rs
✅ Files skipped from review due to trivial changes (1)
- crates/git-same-core/src/monitor/socket_handler_tests.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/git-same-core/src/ipc/unix_socket_tests.rs
- crates/git-same-core/src/provider/github/pagination.rs
- crates/git-same-cli/src/commands/monitor.rs
- crates/git-same-core/src/api/owner_type_cache.rs
- crates/git-same-core/src/monitor/socket_handler.rs
- crates/git-same-core/src/ipc/unix_socket.rs
- crates/git-same-app/ui/src/stores/status.ts
- crates/git-same-core/src/api/service.rs
|
|
||
| - name: Create/update release | ||
| uses: softprops/action-gh-release@v2 | ||
| uses: softprops/action-gh-release@v2.6.2 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Resolve the immutable commit behind tag v2.6.2
git ls-remote https://github.com/softprops/action-gh-release refs/tags/v2.6.2^{}
# Confirm current workflow reference(s)
rg -n 'softprops/action-gh-release@' .github/workflows/S2-Release-GitHub.ymlRepository: ZAAI-com/git-same
Length of output: 114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Resolve the commit SHA for tag v2.6.2 with explicit error handling
echo "=== Resolving commit SHA for v2.6.2 ==="
COMMIT_SHA=$(git ls-remote --heads --tags https://github.com/softprops/action-gh-release refs/tags/v2.6.2^{} | awk '{print $1}')
if [ -z "$COMMIT_SHA" ]; then
echo "Tag resolution failed, trying alternative method..."
# Try without the ^{} syntax (which dereferences annotated tags)
COMMIT_SHA=$(git ls-remote https://github.com/softprops/action-gh-release refs/tags/v2.6.2 | awk '{print $1}')
fi
echo "Resolved commit SHA: $COMMIT_SHA"
# Show current workflow reference
echo -e "\n=== Current workflow reference ==="
rg -n 'softprops/action-gh-release@' .github/workflows/S2-Release-GitHub.yml
# Check if there are any other unpinned GitHub actions in the workflow
echo -e "\n=== All GitHub actions in this workflow ==="
rg -n 'uses:.*@' .github/workflows/S2-Release-GitHub.yml | head -20Repository: ZAAI-com/git-same
Length of output: 1147
🌐 Web query:
GitHub Actions security best practices pinning commit SHA versus tags mutable references
💡 Result:
Pinning GitHub Actions to a full-length commit SHA is considered the industry-standard security best practice [1][2][3]. Unlike tags or branch references, which are mutable and can be force-updated or redirected by anyone with write access to the action's repository, a full-length commit SHA provides an immutable, cryptographically verifiable reference [1][4][5][6]. Security Risks of Mutable References Using tags (e.g., @v1) or branch names (e.g., @main) introduces significant supply chain risks [7][8][3]. Because these references can be moved, an attacker who compromises an action's repository can point a widely used tag to a malicious commit [7][5][6]. This allows them to execute arbitrary code within the workflows of every repository consuming that action, often with the high privileges typically granted to CI runners [7][8][3]. This vulnerability has been exploited in real-world supply chain attacks, where workflows silently began pulling malicious code without any changes to the user's workflow files [7][8]. Best Practices for Implementation 1. Pin to Full-Length SHA: Always use the 40-character full commit SHA [4][6]. Shortened SHAs are insecure as they are susceptible to collision attacks [9]. 2. Maintain Readability: To keep workflows maintainable, include the original version tag as a comment alongside the pinned SHA (e.g., uses: actions/checkout@b4ffde6 # v4.1.1) [10][5][6]. 3. Automate Updates: Use tools like Dependabot or Renovate to manage updates [10][6]. These tools are capable of monitoring new releases and automatically opening pull requests that bump the pinned SHA, allowing you to review changes before merging [10][5]. 4. Enforce via Policy: If you manage an organization or enterprise, use GitHub's built-in policy features to mandate SHA pinning [1][2][11]. This prevents the accidental use of mutable references by failing any workflow that does not adhere to the requirement [2][7][12]. 5. Trust Considerations: While SHA pinning is the primary defense against malicious updates, you should still only use actions from reputable, verified sources [1][9][13]. Additionally, be aware that even a pinned action may rely on other mutable dependencies at runtime (such as external Docker images or network-fetched scripts), so defense-in-depth remains essential [7].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
- 3: https://spin.atomicobject.com/github-actions-risk/
- 4: https://docs.github.com/en/actions/how-tos/create-and-publish-actions/manage-custom-actions
- 5: https://phpunit.expert/articles/hardening-github-actions-workflows.html
- 6: https://github.com/kubernetes/community/blob/main/github-management/github-actions-policy.md
- 7: https://mikael.barbero.tech/blog/post/2026-03-24-stop-trusting-mutable-references/
- 8: https://sebastion.dev/intelligence/2026-03-12-poc-supply-chain-compromise-via-mutable-git-tag-poisoning
- 9: https://docs.github.com/en/enterprise-server@2.22/actions/security-guides/security-hardening-for-github-actions
- 10: https://safeguard.sh/resources/blog/github-actions-pinning-sha-vs-tag
- 11: https://github.com/github/docs/blob/main/content/actions/reference/security/secure-use.md
- 12: https://github.com/github/docs/blob/main/content/admin/enforcing-policies/enforcing-policies-for-your-enterprise/enforcing-policies-for-github-actions-in-your-enterprise.md
- 13: https://github.com/github/docs/blob/962a1c8dccb8c0f66548b324e5b921b5e4fbc3d6/content/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions.md
Pin softprops/action-gh-release to a commit SHA instead of a tag.
@v2.6.2 is a mutable reference. Tags can be moved or force-updated by repository maintainers (intentionally or through compromise), allowing malicious code to be injected into workflows without changing the workflow file. Pinning to the immutable commit SHA is the industry-standard security practice endorsed by GitHub.
- uses: softprops/action-gh-release@v2.6.2
+ uses: softprops/action-gh-release@3bb12739c298aeb8a4eeaf626c5b8d85266b0e65 # v2.6.2🧰 Tools
🪛 zizmor (1.25.2)
[error] 391-391: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 391-391: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/S2-Release-GitHub.yml at line 391, The
`softprops/action-gh-release` action reference at line 391 uses a mutable tag
version `@v2.6.2` instead of an immutable commit SHA, which poses a security
risk. Replace the tag reference `@v2.6.2` with the full commit SHA that
corresponds to that release version (find this by checking the
softprops/action-gh-release repository's releases or tags page), and update the
uses statement to pin to that specific commit hash instead of the version tag.
Source: Linters/SAST tools
Comment on lines
+72
to
+75
| Err(e) => { | ||
| debug!(name = %name, error = %e, "Owner classification failed, leaving unknown"); | ||
| let _ = cache.set(name, OwnerType::Unknown); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle persistence failures when writing OwnerType::Unknown.
The fallback write ignores cache.set errors, so persistence failures on this path are silent while the success path reports them.
🛠️ Suggested fix
Err(e) => {
debug!(name = %name, error = %e, "Owner classification failed, leaving unknown");
- let _ = cache.set(name, OwnerType::Unknown);
+ if let Err(persist_err) = cache.set(name, OwnerType::Unknown) {
+ warn!(name = %name, error = %persist_err, "Failed to persist owner type");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Err(e) => { | |
| debug!(name = %name, error = %e, "Owner classification failed, leaving unknown"); | |
| let _ = cache.set(name, OwnerType::Unknown); | |
| } | |
| Err(e) => { | |
| debug!(name = %name, error = %e, "Owner classification failed, leaving unknown"); | |
| if let Err(persist_err) = cache.set(name, OwnerType::Unknown) { | |
| warn!(name = %name, error = %persist_err, "Failed to persist owner type"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/git-same-core/src/monitor/owner_classifier.rs` around lines 72 - 75,
The code silently ignores persistence errors when calling cache.set to store
OwnerType::Unknown in the error handler block (around line 74), which is
inconsistent with error reporting elsewhere in the codebase. Instead of
discarding the result with let _ = cache.set(...), capture the result and log
any errors that occur when attempting to persist the Unknown owner type to
cache, ensuring that persistence failures on this fallback path are not silently
ignored.
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
1 participant
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.
This PR splits Git-Same into
git-same-coreandgit-sameworkspace crates while preserving the existing CLI install path.It adds the Tauri macOS app, FinderSync badge extension, monitor IPC/status plumbing, workspace folder icon handling, and app settings/workspace management UI.
It updates packaging, Homebrew templates, release workflows, Conductor scripts, documentation, dependencies, and tests for the new app and workspace layout.
Validation: no local checks were run while creating this PR; the branch includes CI workflow updates for the full validation path.
Summary by CodeRabbit
Release Notes
New Features
monitor(including status/stop) andrefresh(supports--path).Improvements