Forgejo/Gitea as a git provider, configurable per repository - #161
Merged
Conversation
Makes the Forgejo provider reachable and fixes what its arrival broke. Server A router picks a provider per repo from the `origin` host, cached per repo so the home-screen fan-out shares one `git remote` read. github.com goes to the gh-backed gateway, anything else to the Forgejo REST one; an unreadable remote stays on gh, which is the status quo for a non-checkout. Routing every non-GitHub host to Forgejo is a guess -- a hostname cannot tell you what software a server runs -- but no case gets worse: those remotes previously went to `gh`, failed, and surfaced as `unknown`. Now Forgejo and Gitea work and GitLab/Bitbucket fail exactly as before. The router implements GithubGateway rather than a narrower type, forwarding the budget surface to the gh gateway, so server.ts needs no changes and the budget panel keeps reporting the only quota that exists. Forgejo has no rate_limit endpoint, so it contributes nothing -- accurate rather than stubbed. Also de-duplicates PrLookup/GatewayStats/PrMutation, which were declared twice after forge/types.ts landed; GithubGateway now extends ForgeGateway. App pr_detail said "Open #42 on GitHub" for every PR, which is simply wrong for a self-hosted one. The forge is now read off the PR's own URL -- no protocol change needed -- and both the label and the button's glyph follow it. An unparseable URL names no forge rather than guessing one. forgeKindForUrl deliberately mirrors the server router's rule (non-GitHub => Forgejo) so the badge always agrees with the provider that served the data. Icons phosphor_extras is the source of truth; scripts/sync-icons.sh vendors the built SVGs here with a --check mode. Vendored rather than depended on because that repo has no published remote yet, and a path: dependency outside this tree would break CI and the cloud VM on a fresh clone. ASSET_ATTRIBUTION.md now also documents the agent logos (Anthropic/OpenAI/pi), which were shipping undocumented.
…JO_* names
Testing against a real instance (Forgejo 16.0.0+gitea-1.22.0) surfaced two gaps.
Credential leak. A configured instance now scopes its token to that host. The
normal way to configure one instance is a single global FORGEJO_ACCESS_TOKEN, and
the previous code attached it to EVERY non-GitHub remote -- so opening any public
Gitea/Forgejo repo would have sent an internal token to a third party. A foreign
host is still queried, just unauthenticated, which is correct for a public repo.
Matching is on hostname only: an scp-form remote cannot express the API's port,
so counting scheme/port/path would make the override never apply.
Env names. The provider now reads FORGEJO_BASE_URL and FORGEJO_ACCESS_TOKEN, the
names tea/fgj users and Forgejo's own docs already use, alongside the MAKIT_*
ones it invented.
Verified end to end against a live instance -- branch->PR lookup, draft detection
from a real WIP: title, openPrs ordering, and every mutation:
- 'ready' stripped the prefix and cleared draft, and refused on a second call
rather than corrupting the title
- 'update-branch' and 'merge-squash' both applied; REST reported
state=closed + merged=true, which maps to MERGED, not CLOSED
- a codeberg.org repo resolved unauthenticated in the same process, proving the
token stayed home
… 429 Two defects that only showed up once someone asked whether Forgejo has rate limits. It does not -- no /rate_limit endpoint, no rate-limit headers, and no limiter in the whole config surface (its `quota` feature meters storage bytes) -- and both bugs came from assuming otherwise. 1. Forgejo repos were polled 6x slower than needed. The poll cadence came from decide(gateway.budget()), and the router forwards budget() to the gh gateway. With no GitHub repo the rate_limit read never succeeds, the level stays `unknown`, and policy.ts treats unknown as the warm rung: 30s and shed. So a quota that provably does not exist was throttling Forgejo polling. The router now reports which providers it has actually routed to, and the cadence takes the fast rung when the mix is known and excludes GitHub. An empty mix stays conservative -- "not yet known" is not "no GitHub", and guessing fast would spend GitHub quota for the first ticks after startup. Mixed setups still take the GitHub cadence for everything; pr_watcher runs one global timer, and taking the fast rung there would burn the quota the ladder exists to protect. Documented rather than fudged. 2. No backoff on 429/503. Forgejo core cannot send them, but an instance behind nginx limit_req or Cloudflare can, and we answered by polling at the same rate -- leaning on a server that had just asked us to stop. The provider now reads Retry-After (delta-seconds or HTTP date), caps it at 5 minutes so a bad header cannot silently disable polling for a day, withholds background polls while waiting without inflating the exec count, still lets interactive calls through, and reports `throttled` rather than `none` so a pause never erases a PR pill. Verified through the real router: mix=[] -> 30s, mix=[forgejo] -> 5s, mix=[forgejo,github] -> 30s.
…e hostname
Routing keyed off the hostname alone: github.com meant GitHub, everything else
was ASSUMED to be Forgejo. So a GitLab or Bitbucket remote was polled against a
Forgejo API that does not exist there, failed, and reported `unknown` -- exactly
what a healthy Forgejo instance being unreachable looks like. The user had no way
to tell "makit does not support this forge" from "the network is broken".
Detection uses endpoints, not version-string sniffing, each verified live:
GET /api/forgejo/v1/version 200 on Forgejo, 404 on Gitea
GET /api/v1/version 200 + {"version":...} on both; GitLab 302s to
its sign-in page
GET /api/v4/version 401 unauthenticated on gitlab.com -- still proof
Forgejo is probed first: decisive in one call, and it is the case we care about.
github.com is never probed. Results are cached per instance so this never adds a
round trip to the PR hot path, and the probe carries the instance token because a
private instance answers 401 to anonymous callers.
A FAILED detection expires after a minute instead of being cached, so an instance
that was briefly down is not pinned as unsupported until the server restarts --
the failure mode that would have been most annoying to diagnose.
GitLab and unidentifiable forges now route to an unsupported provider that makes
zero requests and, on a mutation, says which forge it cannot talk to. The host is
logged once per process rather than once per poll.
Verified against four real forges: codeberg.org -> forgejo, gitea.com -> gitea,
a self-hosted 16.0.0 -> forgejo, gitlab.com -> gitlab, example.com -> unknown.
Detection landed server-side but nothing shows it, so this proposes the three places it should and one shape it should not. Corrects the premise it was asked under. The ask was per-REPO provider setup, but an API token authenticates an INSTANCE -- the server already enforces that, since forgejoRefFromRemote withholds a token from any host other than the configured one. So credentials belong to the instance, and the only genuinely per-repo thing is an override for when detection is wrong. Per-repo credentials would mean pasting the same secret once per repository and would fight the host scoping. Grounded in the real vocabulary rather than invented UI: the chip joins BranchChip/DiffChip/PrStatusChip in repo_chips.dart at kPillIconSize=11, the override entry goes in the dotsThree menu that already exists at repo_card.dart:188 beside its themedMenuItem entries, and the state table uses ForgeSoftwareName verbatim. The Forgejo and Gitea glyphs are the ones actually drawn in phosphor_extras, inlined on their 256 grid at stroke 16 (which matches 1.45/24 optically). Two states are listed for `unknown` on purpose: a failed probe expires after 60s, so "we could not tell yet" must not render the same as "we asked and it is not supported". Verified in a browser at 1340x1000: no wrapped grids, no broken glyph refs, no horizontal overflow, every card read as an image. The prose column is explicitly its own full-width row -- a 320px phone plus a 560px mac window plus prose cannot share 1292px, and letting that wrap by accident is what silently hides a column.
…problem
Proposes Settings > Repositories > one repo as the surface for the growing
per-repo set (logo, root path, provider, default branch, worktree root, lifecycle
scripts), grounded in what already exists rather than a new pattern:
- SettingsGroup / SettingsSectionHeader / SettingsResetButton already ARE the
grouped-list-with-revert idiom, and SettingsResetButton already collapses to a
fixed-width box so rows with and without an override stay aligned. That is the
whole inheritance affordance, built for SPEC-19.
- project-store.ts already persists one record per project to
$MAKIT_HOME/projects.json, server-side, degrading to an empty list on a
corrupt file. Per-repo settings belong there, NOT in SharedPreferences: the
daemon needs them, and app-side prefs are per-device, so a worktree root set
on a phone would never reach the daemon that creates the worktree.
- MAKIT_WORKTREE_DIR (git.ts:33) is a live example of the problem -- a setting
that is global-only today and wants to be per-repo with inheritance.
The inheritance model is the load-bearing part: always render the EFFECTIVE value
with a badge naming its source, never an empty field that silently means a
default. Reverting means "inherit again", not "copy today's global", so a later
change to the global still propagates.
Flags two blocking security decisions about lifecycle scripts, both unrecoverable
by a later patch:
1. Script text must live in makit's own config, never be read from the
repository working tree. Otherwise cloning a repo and adding it to makit is
arbitrary code execution, and reviewing a stranger's PR branch runs their
script.
2. Only the host may set one. makit pairs with phones; if any paired device can
write a script the daemon executes, pairing stops meaning "chat with an
agent" and starts meaning "remote shell on my laptop".
Plus the unglamorous parts that decide whether this is safe in practice: an
allowlisted environment rather than the daemon's own (which holds
FORGEJO_ACCESS_TOKEN), a timeout, and pre-prune holding the veto because prune is
the destructive verb.
Build order is deliberately storage-first (rows 1-3, no UI, no security surface,
and it fixes the worktree-root wart immediately) with the hook runner last.
Verified in a browser at 1340x1000: no wrapped grids, no broken glyph refs, no
horizontal overflow, every card read as an image.
The first draft invented chrome. Corrected against a screenshot of the shipped
Settings window:
- Group headers are GREEN uppercase, not grey, and there is a green uppercase
PAGE title above them ("SERVER & DEVICES" in the real one).
- The window has its own chrome: traffic lights, an "X Settings" title and a
search field above the sidebar -- it is not a bare pane.
- The sidebar is ~250pt with nine entries (General, Appearance, Agents & Chat,
Server & Devices, Notifications, Shortcuts, Advanced, About); the selected one
is a filled rounded rect with green icon and label. "Repositories" is proposed
after "Agents & Chat", since both concern work content while Server & Devices
is infrastructure.
- Rows are two-line (title + grey subtitle) at ~42-52pt, not the 36pt
single-line rows drawn before.
- SettingsResetButton sits at the trailing edge, and in the real UI a
card-level one appears at the Endpoint card's top-right -- so per-row is right
for per-repo settings, where each row is its own setting.
Type is enlarged relative to the frame: the real window is ~1400pt wide and
cannot be drawn to scale on this page. Stated in a comment so nobody reads the
proportions as a spec.
Verified: no wrapped grids, no broken glyph refs, no horizontal overflow, and an
explicit assertion that no row's label overruns its value -- which caught "Root
path"'s subtitle colliding with the path before this was committed.
…s page Agreed and redrawn. Fixed app sections, then a green REPOSITORIES group header, then one entry per repo carrying its monogram -- the Mail/Finder sidebar idiom. What makes it safe rather than a sidebar that turns into a file browser: the group is bounded by what the user added. Projects restored from projects.json are pinned:true (manager.ts:215) while repos makit merely noticed via addProject are pinned:false (manager.ts:287), so only the former get a section. A real install holds three (makit, Diana, piano). Grouping matters. Fixed app sections are a closed taxonomy and repos are data; interleaving them in one flat list is a smell, but under a header it is a pattern every macOS sidebar already uses. It also simplifies the pane: the green page title becomes the repo name, so the "All repositories >" breadcrumb the previous draft needed disappears. Names the risk rather than hiding it -- the sidebar becomes a SECOND place listing repositories alongside the repo-centric home, and two lists drift. The mitigation is a shared source (same RepoDTO, same monogram widget), not a convention. Overflow deliberately unbuilt: past ~10 repos, keep pinned ones inline and add a single "All repositories..." entry. Cheap to add later precisely because `pinned` already exists, and three sections is not a scrolling problem today. Verified: no wrapped grids, no broken glyph refs, no horizontal overflow, no label/value collisions, div tags balanced.
One Settings section per repository, under a REPOSITORIES group header. The inheritance model is the feature; the four rows are its first tenants. UI-first per the request, and it is not a fake: every P1 row has a real source (monogram from RepoDTO.name, path and defaultBranch already on the DTO, forge from the detection that already shipped, worktree root from the value git.ts:33 already computes). The one row without a P1 source -- branch prefix -- is deferred rather than rendered dead. 15 locked decisions. The two that matter most are D13(a) and (b): script text must never be read from the repository working tree, and only the host may set one. Neither is recoverable by a later patch, so P3 is gated on confirming both. Dispatched for second opinion to two parallel codex reviews (technical correctness vs engineering practice) before any code.
Recorded rather than quietly patched. Eleven rev-1 claims were confirmed wrong by hand before being accepted: - The wire-contract RED test was VACUOUS. contract.test.ts loads snapshots as Record<string,unknown> and only asserts codec round-trip; decodeFrame validates v/t/id then casts. A new RepoDTO field could never fail it. - createForgeRouter keeps no per-repo forge decision -- chosen is Map<string, Promise<ForgeGateway>> and nothing else. - repo_service.ts cannot reach the router: it takes GithubGateway. - The Settings sidebar is a STATIC list carrying search entries, with settings open state as a bare bool and no way to carry a repoId. Dynamic per-repo sections are foundational work, not an implementation detail. - RepoCard is the MOBILE home card; the entry-point task targeted the wrong shell. - git.ts:33 is only the env read; the resolved root is also consumed by addWorktree, addWorktreeForPr and uniqueWorktreeDir. And one finding that changes the product, not the plan: write authorization was specified for scripts and nothing else. A paired device that can set an arbitrary worktreeRoot directs host filesystem operations at a path of its choosing. Two decisions now block rev 2: phasing (a read-only P1 is arguably ornamental vs the explicit UI-first request) and write authorization for non-script settings.
…rs corrected Phasing: UI first PLUS one editable row (worktree root end-to-end), so the page does one real thing on day one and the reviewer's 'ornamental' objection goes away without abandoning the requested sequencing. Write authorization: host-only, and enforceable today rather than aspirational -- WsClient.isLocal (ws/client.ts:46) is set from the real socket address in server.ts:733 and ALREADY gates a privileged input, the app's reported pid in hello (SPEC-37 D6). Per-repo writes take the same shape: any device reads, only loopback writes. Foundations the review exposed are now P0, not assumptions: the router keeps a per-repo forge decision (it kept none), repo_service gets a narrow ForgeInspector port rather than the router (listRepos only sees GithubGateway), settings survive load/save/DTO-copy losslessly (all three dropped them), sectionsFor(repos) replaces the static registry, and settings open state carries a target id. Every red test now asserts a value production code must produce. The vacuous one is replaced by building a repos.snapshot through listRepos and asserting it typed as RepoDTO[]; T2 asserts all three worktree-root consumers, which is the test that proves rev 1's 'only consumer' claim wrong. Cut: provider override in any phase, the four-level chain (there is no global store), the copy button, blanket two-line rows, hue-determinism as a requirement, P4, and pixel-perfect as an acceptance gate.
Round 2 returned 9/11 FIXED, both blocking decisions FIXED, and four new errors. All four applied: - T3 contradicted itself: ~/x/../../etc is NOT absolute, so it can never be 'rejected after canonicalisation' -- the absolute check rejects it first. Each path case now reaches a different rule, with an absolute ..-containing path for the collapse rule and a real symlink for the escape rule. - D17 said 'canonicalise with realpath' but realpath FAILS on a path that does not exist -- so it would have rejected ~/work/worktrees before the user created it, which is the common case. Now: canonicalise the nearest existing ancestor, then require the remaining segments to be ..-free and symlink-free. - F4 omitted settings_nav_pane.dart, which calls searchSettings() and builds result titles from the static list. Without it repo rows are unsearchable and render as repo:<id>. Now covered, with the search title asserted. - T6 left the integration harness undecided. Decided: mount SettingsWindow with a stubbed snapshot, not an extension of the control socket -- the daemon-side behaviour it would duplicate is already proven by T2/T3.
…rding verbatim) Round 3 was scoped to the four fixes only: 2 FIXED, 2 PARTIAL, 1 contradiction. - T3: 'rejected after collapsing' was still wrong -- collapsing .. yields a valid path. A .. segment is now rejected ON SIGHT by a pre-canonicalisation check, so the rule cannot silently resolve to something the user did not type. - D17: added the missing branch -- if no existing ancestor can be realpathed, reject. Ancestor symlinks were already covered by realpath, and read-back revalidation is what closes the write-to-use TOCTOU window. - The risk table still claimed the integration harness was undecided, contradicting T6.2's explicit choice. Replaced with the real residual risk: SettingsWindow needs a stubbed snapshot, which settings_window_test.dart:109 shows is a ProviderScope override rather than new infrastructure. F4 and T6 came back FIXED. All three remaining items were wording the reviewer dictated, transcribed rather than interpreted.
… real macOS app The SPEC-48 T4 widget, built from the shipped settings atoms (SettingsSectionHeader, SettingsGroup, SettingsResetButton, TagChip) so it inherits the window's spacing, the green uppercase headers, and the reset button's fixed-width collapse. Takes a RepoSettingsView rather than a RepoInfo: the section shows facts the DTO does not carry yet (detected forge, EFFECTIVE worktree root, whether it was overridden), so the widget is complete and testable before the server plumbing lands -- and it stays pure presentation, told facts rather than deriving them, which is the rule that stopped the app re-deriving the forge from a PR URL. Two defects found by screenshotting the real app, not by reading code: 1. The badge column stepped between groups. The Worktree root row reserves a SettingsResetButton slot (40pt when hidden) and the Identity rows did not, so "inherited" sat 40pt left of "from name" / "detected" / "from remote". Every row now reserves the slot, giving the section one right edge -- which is what the plan said and what the first implementation did not do. 2. The Git provider row never named the forge. It showed a 20pt glyph, the host and a "detected" badge, leaving "detected as WHAT?" answerable only by recognising the mark. It now reads "Forgejo - host - token set". Harness: app/tool/repo_settings_demo.dart, following tool/pr_bar_demo.dart -- real widget, real theme, seeded states including the two that must render NOTHING (an unprobed repo omits the provider row; a non-loopback client gets no controls). Captured with cua-driver window-bound zoom, tiled at scale 1.0 after verifying the running bundle was not the stale lib/main.dart one.
…e built app Split decision applied: take the mockup's value column, keep the app's TagChip badges, drop the descriptive subtitles. The value column earns it -- a column of right-aligned values scans in one vertical sweep where values buried in subtitles must be read line by line. It does mean this row style diverges from CLI/Fingerprint in Server & Devices, which put their value in the subtitle; that is now a recorded, deliberate divergence rather than an accident. A subtitle survives only where there is a SECOND fact worth showing (the forge's host), never as a description of the row -- "Monogram from the name" under a row labelled "Logo" is words about words. Paths render home-abbreviated (~/Work/XDent/Diana): a settings row should not spend 40pt of its value column on a home directory the reader already knows. Values are width-bounded so a long path elides instead of shoving the badge off the row -- the same failure the mockup hit before its subtitle was shortened. Verified on the real macOS app, both states: - inherited -> grey "inherited" badge, reset slot reserved but invisible - overridden -> green "overridden" badge AND the reset button visible The overridden state was reached by flipping the harness default rather than by synthesized clicks: cua-driver reported the sidebar click "unverifiable" and the scene did not change, so a deterministic default beats spending the user's foreground on an unreliable input path. mockups/repo-settings.html card 1 is corrected to match what was built, so the design board stops disagreeing with the app, and the cut Lifecycle Scripts group is no longer advertised in either frame.
…table Requested after seeing the built section. This reverses two locked decisions and re-opens two cuts from review round 1, so each is justified by the concrete failure case the reviewer said was missing rather than by the request alone: - Provider selector (Auto | Forgejo | Gitea | GitHub, the Endpoint idiom): round 1 cut an override for having "no concrete failure case". There are two -- a private instance answering 401 to an anonymous probe, and a proxy hiding /api/forgejo/v1/version. Both leave the repo routed to the unsupported provider and unusable with no recourse anywhere in the product. - Root path: "remove and re-add" mints a new PersistedProject.id and loses everything keyed to it. A repo that merely MOVED should keep its identity. - Default branch: origin/HEAD is genuinely absent after a --single-branch clone or a rename, and makit then shows the wrong base, so diff-vs-default and the PR base are both wrong. Picked from known branches, never free text. - Logo: two names hashing to the same hue are indistinguishable in the sidebar, defeating the one thing the monogram exists for. Palette choice needs no byte transfer, so the deferred custom-image path stays deferred. D16 still governs: only a loopback client may write. A non-loopback view renders the same values with the selector inert and one line saying where they are editable -- asserted by test. 15 widget tests, two proven to bite by reverting the read-only gate and by making reset freeze the detected forge instead of asking for Auto. Interaction is covered by widget test rather than on the real app, and that is a stated gap: cua-driver synthesized clicks did not land in this Flutter app on two separate builds -- a sidebar row and a 107x28pt segment both returned "effect":"unverifiable" and left the UI unchanged. The real-app pass covers appearance only, which it did verify, including the segmented control and the Auto subtitle naming what it resolved to.
"from name" beside a monogram, "from remote" beside main, and "detected" beside a
subtitle already reading "Auto: Forgejo - ..." are each the same sentence twice --
and once every row became editable, where a value came from stopped being
actionable. The copy button goes with them: copying a path is not a configuration
task, which review round 1 said before it was overruled.
One rule now, stated in the widget: a badge appears only where nothing else in the
row says it. So exactly one survives -- inherited/overridden on Worktree root, the
only row with no subtitle, where the distinction is the entire point of the
feature. The reset button stays wherever an override exists, because it is an
action rather than a label.
This finishes a cut review round 1 asked for ("they are not one abstraction") and
rev 2 only half-applied by keeping both badge families.
Pinned by two negative tests so the chrome cannot creep back: one asserting the
three provenance labels and the copy tooltip are absent, and one asserting that
with TWO things overridden there are two reset buttons but exactly ONE badge.
17 widget tests green. Verified on the real macOS app; mockups/repo-settings.html
updated in both frames plus its inheritance table, so the board still matches.
Auto | None | Forgejo | Gitea | GitHub. None means makit talks to no forge for this repository and stops checking pull requests. It answers two cases nothing else could, and which previously read identically: - A purely local repo with no origin. It rendered "Auto: not identified yet", implying a probe was pending when none can ever help -- and the router still sends it to the gh gateway, which fails on every poll. It now reads "Auto: no remote, so no forge" with a prohibit glyph: a conclusion, not a wait. - A mirror or vendored copy whose forge you do not care about. There was no way to stop the PR chatter; None is that instruction. "We could not tell" and "there is nothing to tell" must not read the same, because only the first is worth investigating. Pinned by a test asserting the two subtitles differ and that the pending one is absent when there is no remote. Adds one DTO fact, hasRemote, which the server already computes in the routing path and currently throws away. P2's write set is now five. 23 widget tests green. Verified on the real macOS app, including the None state's prohibit glyph, its wording, and the reset back to Auto. Also fixes a harness bug the screenshot caught: the demo fell back to ForgeChoice.auto rather than the scene's own choice, so the scene that exists to show an override rendered as if it had none.
… host-only writes
The server half of SPEC-48. Settings are real: stored, resolved, validated, served
on the wire, and they change where worktrees are created.
repo_settings.ts is the whole model in one place:
- RepoSettings, every field optional because ABSENT MEANS INHERIT. A blank
worktree root that silently means ~/.worktrees is how worktrees end up
somewhere the user did not expect.
- resolveWorktreeRoot returns the effective value AND its source, so the UI
labels rather than guesses. Three levels (override -> env -> default), not
four: there is no global store to inherit from.
- validateWorktreeRoot, whose rules run in an order that matters. Absolute only.
A `..` segment rejected ON SIGHT rather than collapsed, because collapsing
silently yields a path the user did not type -- and prune deletes under this
root. Canonicalised through the nearest EXISTING ancestor, since a root that
does not exist yet is the common case and realpath fails outright on it.
Confined to $HOME, because the daemon creates and removes directories there.
Two bugs found while testing it, both invisible without a test that could fail:
- The `..` check was DEAD CODE: it split a normalised copy, and normalize()
collapses `..`. My own test could not catch it either, because path.join
collapses too -- the case only became reachable once the test built the string
by concatenation.
- A test expectation, not the code: macOS /var is a symlink to /private/var, so
canonicalisation legitimately rewrites the path. That is the rule working.
Wiring: SessionManager.worktreeRootFor is the ONE place all three consumers read
from -- addWorktree, addWorktreeForPr and uniqueWorktreeDir. Reviewer finding R5
was that routing fewer than three makes collision detection disagree with
creation; proven by a test that makes a name collide under the OVERRIDE only, and
which fails when uniqueWorktreeDir is pointed back at the global root.
repo.settings.set is host-only, enforced against WsClient.isLocal on the server --
not a UI convention. Precedent: SPEC-37 D6 already refuses a non-loopback client's
reported pid. A refusal is an explicit error, never a silent no-op. One bad field
rejects the whole patch; null clears a key, which is how the UI says "inherit
again" without a sentinel.
Persistence is lossless, including keys this build does not know: project-store
and the manager both carried only {id, path} and would have dropped a newer app's
field. An untouched project still writes exactly two keys, so the file stays
diffable.
1415 server tests green (+34), typecheck clean. Bites verified by reverting the
loopback gate, the override lookup, and the collision-check root.
RepoInfo gains settings: effective values with their sources, decoded defensively. An unrecognised source reads as the default and an unknown provider falls back to auto, so a newer server cannot crash an older app's settings page. repoSettingsViewFor is the one mapping from wire facts to rendered facts, which is where "the app is told, never derives" is actually enforced: - No settings on the DTO yields NO view, rather than a page of fabricated defaults. An older server renders nothing, which is honest. - environment-sourced values are not overrides, so they offer no reset the app could not honour -- it cannot change the daemon's environment. - gitlab/unknown map to no glyph rather than a wrong one; the host still shows so the row can name what it cannot talk to. - "no remote" and "not detected yet" stay distinct all the way through, because only one of them is worth investigating. - Branches are offered from the repo's own worktrees plus its default, deduped and sorted, so a pick cannot be a typo. 11 mapping tests + the 23 widget tests, all green. analyze clean.
The settings taxonomy becomes a function of the repo list. kSettingsSections was a static `final List` resolved statically by SettingsWindow, so a per-repo section was impossible -- which review round 1 caught and rev 1 had assumed away. sectionsFor(repos) appends one section per PINNED repo, ids keyed `repo:<projectId>` off the persisted id so a renamed or moved repo keeps its section. Unpinned repos stay out: that filter is what stops the sidebar growing into a file browser. Threaded through both places that read the static list, not just the obvious one. The nav pane's SEARCH was the subtle one: generated repo items existed but were unreachable, which looks identical to not generating them. Now covered by a widget test that fails when the pane falls back to the static list -- analyze does not catch it, because `sections` is still used for the titles. RepositorySettingsPage does the writes fire-and-forget with NO optimistic local state: the server persists, re-broadcasts, and the page re-renders from that. So a write refused because the client is not on loopback cannot leave the UI showing a value the daemon rejected. Root path deliberately shows a notice instead of an edit. Re-pointing a project needs the daemon to re-check it is a git repo and re-run detection, and getting that wrong silently detaches a project from its sessions. Worktree root is a dialog whose value is NOT validated in Dart: the server owns the rules (absolute, no `..`, inside $HOME, canonicalised) and a second implementation could disagree with the first. 66 settings tests green, analyze clean. Bites verified on the pinned filter and the nav-pane search threading.
Records what shipped, and the probe that proved the setting actually moves where git worktree add writes: two real repos, a real projects.json, a real SessionManager, and a real worktree landing under the override rather than under ~/.worktrees. No unit test can establish that. Also records the limitation the probe exposed: it reloaded settings from the file rather than restarting the process, so the forge field's behaviour across a true restart -- which would also re-run detection -- is still unproven. Plan status updated, including the two items honestly marked not-done: the deep link carrying repoId (F5) and the repo-card entry point (T5) are deferred, since sections are already reachable from the sidebar, and the integration test is outstanding.
D3" was only half-shipped: the choice persisted and displayed, but the router
ignored it, so a repo detection could not identify stayed unusable with the
control sitting right next to it. That is the "ornamental setting" failure
review round 1 named.
The override now picks the gateway:
forgejo/gitea -> the REST gateway, WITHOUT probing. The probe is what fails
in both cases the override exists for (a private instance
that 401s an anonymous request, one behind a proxy that
hides the version endpoint), so spending it would delay
every poll to learn nothing.
github -> the gh gateway, which is the only way to reach `gh` for a
GitHub Enterprise host that is not github.com.
none -> a new gateway that talks to no forge and reads no remote.
`none` is deliberately NOT `unsupported`. Unsupported means "we cannot talk to
this forge" and answers `unknown`; None means "do not talk to any forge here"
and answers `none`. `unknown` would make the app hold a stale PR pill and keep
retrying -- the exact chatter None exists to stop. Pinned by a test asserting
the two gateways do not report the same thing.
The routing cache now keys on the choice as well as the path. Without that the
setting would appear to do nothing until the daemon restarted, which is
indistinguishable from a broken feature; an unchanged choice still shares one
`git remote` read, so the home-screen fan-out stays cheap.
Also fixes a bug the DTO carried: `hasRemote` was derived from
`forge !== undefined`, but those two facts have three states between them --
not measured, no remote, a forge -- and one boolean cannot hold three. Every
un-polled repo claimed to have no origin, which made the app's "Auto: not
identified yet" branch unreachable and sent the reader hunting for a missing
remote that was never missing. The router now records the remote as its own
fact (`hasRemoteFor`), undefined until the repo is routed.
Server 1464/1464, typecheck clean. Bites proven by reverting the cache
re-check (only the re-route test fails) and the override branch (six fail).
Stored, served, displayed — and read by nothing. `detectDefaultBranch` was
called directly in three places, so choosing a default branch changed no
behaviour at all. The same mistake R5 caught for the worktree root, in a
setting whose whole justification is that git's answer is sometimes wrong.
One resolver, `resolveDefaultBranch(repoPath, override)`, and all three
consumers read from it:
- the repos snapshot, whose `defaultBranch` is the base every diff +/- number
and ahead count is measured against;
- `createWorktree`, the base a session's branch forks from;
- `wrapUpWorktree`, the branch it syncs on the way out.
The override is CHECKED, not trusted. It is stored after a syntax check only,
it is picked from branches that existed at the time, and a branch can be
deleted afterwards — so a stale override loses to detection rather than winning
and then failing deep inside a `git diff` where the message is unrecognisable.
Checking costs one `rev-parse` and REPLACES detection's one-to-three calls when
the override holds, so the common path gets cheaper.
The failure it fixes is reproduced in a real repo: `origin/HEAD` still naming
`master` after the default branch was renamed, where git answers with a ref
that no longer resolves locally and the override is the only recourse.
One of these tests was vacuous on the first run and is worth recording: a
branch created from `main` with no commit of its own has the same tip, so
`merge-base` could not tell which branch the worktree forked from and the
assertion held regardless of the production code. The fixture now gives each
branch a divergent commit.
Server 1478/1478, typecheck clean. Both new consumers proven by reverting
their own call site, each failing only its own test.
The third inert write. Picking a hue reached the server, was stored, came back on the DTO and was parsed into the model — then dropped: `RepoSettingsView` had no `logoHue` field and the section header built `RepoMonogram(name:)` with no hue at all. The palette picker changed nothing anywhere. Worse, the sidebar drew `PhosphorIconsLight.folder` for every repo section, so all repo rows were identical in exactly the place D14' exists to disambiguate — and the place the user looks to confirm the colour took. Sections now lead with the repo's own monogram carrying its chosen hue. `SettingsSection.leading` is additive rather than a widened `icon`: every app section is correctly described by a glyph, and `icon` stays required so a section can never end up with nothing to draw. Null, not 0, means "no choice": index 0 is a real palette entry, so a numeric default would silently repaint every repo that never chose. One test caught itself being vacuous: 'Diana' hashes to palette index 3, so asserting a chosen hue of 3 differed from the derived one passed for the wrong reason. It now derives the first index that provably differs, which also survives a change to the palette or the hash. flutter analyze clean; the touched suites pass (38 + 10). The 16 whole-file `loading` failures in the full run are the known app/ harness flake — all four sampled files pass in isolation.
The last undone P1 task. Every hop was covered in isolation; nothing covered
the composition, which is where this repo has already been bitten: the desktop
shell mounts Settings OUTSIDE a GoRouter, so a section navigating with
`context.go` threw at runtime while every test stayed green. A test that pumps
the section directly cannot catch that class of fault.
Five tests on a real macOS build, over the whole path:
reposProvider -> sectionsFor() -> nav pane -> RepositorySettingsPage
-> repoSettingsViewFor() -> RepositorySettingsSection -> the rows
The fixture gives Diana overrides (root, provider, branch, hue) and lets makit
inherit everything, because a fixture where both repos look alike passes even
when the section reads the wrong repo's settings -- which is what the
switch-repos test pins.
Not routed through the daemon control socket, per the plan's T6.2 decision: the
daemon-side behaviour is already proven by the server tests, so the socket would
be infrastructure for no extra coverage. Stubbed at `reposProvider`, the seam
`SettingsWindow` actually reads.
All five bite: making `sectionsFor` ignore the repo list fails every one.
One assertion was wrong on the first run and the code was right --
`SettingsSectionHeader` upper-cases titles, so 'Identity' never matches. Now
asserts the rendered string, plus the row labels, so "the section mounted" can
no longer be mistaken for "the section rendered its contents".
Runner: app/tool/e2e-desktop-settings.sh. flutter analyze clean.
The last of the five writes, and the only one that was not merely inert but
absent: the row showed "not supported yet — remove it and add it again", which
is advice that loses the thing the row exists to preserve. Remove-and-re-add
mints a new `PersistedProject.id`, and everything keyed to it — per-repo
settings, session history — goes with it. Keeping the id across a move is the
entire reason the id exists rather than the path being the key.
`repo.path.set` is a separate command from `repo.settings.set`, not a field in
its patch: it mutates the project record, must re-validate the target is a git
repository, and re-runs forge detection. Folding an async filesystem-and-
subprocess check into a loop that validates plain values would also break that
loop's all-or-nothing property — one bad field would abort a move that had
already happened.
Three refusals, each for an otherwise-silent failure:
- not a git repo (the constraint D4' names): no branches, no forge, no diff,
and it presents as broken rather than misconfigured;
- already another project's path: settings and the forge decision are both
looked up BY PATH, so two projects at one path answer for each other;
- anything `validateRepoPath` rejects.
`validateRepoPath` deliberately differs from `validateWorktreeRoot` on two
rules, and a test pins the asymmetry so it cannot be "tidied" into consistency:
the repo path MUST already exist (a repository you have not got is not one, and
accepting it detaches the project from its sessions), and it is NOT confined to
$HOME (that rule protects the worktree root because prune DELETES under it;
makit never deletes a repo path, and a checkout on an external volume is
ordinary).
The router gains `forgetRepo`, kept on its own interface rather than on the
read-only `ForgeInspector`, so detection re-runs against the new path instead of
reporting a stale probe.
A bug the tests caught: the duplicate check compared a canonicalised new path
against un-canonicalised stored ones, so on macOS every /tmp project evaded it
(`/tmp/x` and `/private/tmp/x` are one directory). Both sides are canonical now,
and re-pointing at an equivalent spelling is a no-op that reports the path
actually in force rather than one the store does not hold.
Stated limitation, in the code rather than buried: sessions already bound to a
worktree keep their recorded paths. For the case this exists for, worktrees live
under the worktree root and are unaffected.
Server 1496/1496, typecheck clean; flutter analyze clean; app 27 + 5 e2e green.
Bites proven on the loopback gate and the canonical duplicate check.
Records what P2 closed and, as plainly, what it did not. The headline: three of P1's five writes changed no behaviour anywhere, and the fourth row did not exist. That was the "ornamental setting" failure review round 1 named and rev 3 accepted as a risk, so the honest status of P1 was narrower than "implemented". Also records the two bugs found while wiring it — `hasRemote` collapsing three states into a boolean, which made the app's "Auto: not identified yet" branch unreachable; and a re-point duplicate check comparing paths at different canonicality — and the two tests that were vacuous until fixed, because a test that passes while proving nothing is worse than one that fails. Live proof over a real daemon: 21/21, with the first four assertions covering the thing no unit test can establish — that the setting changes which provider serves the repo.
The flow was broken exactly halfway. Listing already routed correctly —
`listOpenPrs` goes through the gateway, so the picker showed Forgejo PRs — and
then the checkout ran `gh pr checkout` unconditionally. `gh` speaks only to
GitHub, so the user saw their PRs, picked one, and the worktree never appeared.
`addWorktreeForPr` now takes a strategy:
gh - unchanged for GitHub. Kept rather than replaced with a generic
equivalent because it already handles fork PRs and sets up push
tracking; swapping a working path for a hand-rolled one is a
regression risk taken for tidiness.
pull-ref - plain git against `refs/pull/<n>/head`, which is how Gitea and
Forgejo publish PR heads. That ref exists for EVERY PR including
forks', which is why it is used instead of the head branch name —
a fork's branch is not on `origin` at all.
Upstream tracking is set only when the head branch really is on origin (a
same-repo PR), so a push updates the PR. For a fork it is deliberately left
unset: pointing it anywhere would aim a push at a branch that is not the PR's.
`manager.prCheckoutStrategyFor` resolves from the SAME two sources the router
uses to pick a gateway, in the same order — the user's override first, then
routing's decision — so the checkout cannot disagree with the provider that
served the list. Read after `listOpenPrs`, which is what routes the repo, so the
decision is available rather than empty. Unknown falls back to `gh`, the status
quo for an unreadable remote.
Tested against a real local bare repo publishing a real `refs/pull/7/head`, so
the git plumbing is exercised with no network and no forge: the PR's actual
commit is checked out, on a PR-unique branch (the primary checkout commonly sits
on the head ref, and git refuses to check one out twice), a same-repo PR gets its
upstream, a fork PR still checks out without one, and a failed checkout leaves no
litter.
Also pins the GitHub path, which had NO test before this refactor — argv and all,
via a PATH shim. Both bites land: flipping the default to pull-ref fails the gh
tests, and removing the shared rollback fails one test per strategy.
Live probe, end to end over the real commands (`pr.list`,
`worktree.createFromPr`), 12/12: the PR is listed by the Forgejo provider on a
host detection could NOT identify, the strategy resolves to pull-ref, and the
worktree lands at the PR's commit under the repo's own worktree root. Its first
run also confirmed the containment rule works — a worktree root in /tmp was
correctly overruled. Probe deleted.
App side needs nothing: the picker renders `openPrs` with no GitHub gating.
Salvaged from a parallel attempt in this worktree (per user direction) before
removing it: a test that the picker routes per repo with Forgejo and GitHub side
by side in ONE manager, and one that a repo set to None offers no PRs — so the
checkout is unreachable rather than merely moot. Its own two tests reached a
private method through a cast and asserted nothing about routing; its probe did
not compile.
Server 1512/1512, typecheck clean.
![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: 40
🤖 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 `@app/lib/desktop/settings/repository_settings_page.dart`:
- Around line 67-113: Dispose the TextEditingController created in
_promptWorktreeRoot using a finally block that runs after showDialog completes,
while preserving the existing value handling. Apply the same finally-based
disposal to the controller in
app/lib/desktop/settings/repository_settings_page.dart lines 192-265, including
its ValueListenableBuilder usage.
- Around line 147-176: Update the colour-option loop in the logo colour dialog
to use RepoMonogram.paletteLength instead of the hardcoded value 6, while
preserving the existing indexing and “Derive from the name” option.
In `@app/lib/desktop/settings/sections/repository_section.dart`:
- Around line 317-321: Update _tilde so it replaces HOME only when path exactly
equals HOME or the character immediately following the HOME prefix is a path
separator; otherwise return the original path unchanged. Preserve the existing
handling for missing or empty HOME.
- Around line 257-276: Update the Default branch row condition around
view.defaultBranch so it remains visible whenever view.editable and
view.branches.isNotEmpty allow branch selection, including when defaultBranch is
null. Display an explicit placeholder for the unresolved value while preserving
the resolved branch value and existing picker behavior.
In `@app/lib/desktop/settings/settings_window.dart`:
- Around line 119-125: The settings window currently leaves _selectedId as an
unavailable repo:<id> when the detail pane falls back to a fixed section, so
SettingsNavPane has no selected row. In the build flow around the fallback
section selection and SettingsNavPane, derive the effective selected ID from the
resolved section.id and use it for navigation; persist that fallback through the
existing preference mechanism when this is intended behavior.
In `@app/lib/ui/widgets/forge_glyph.dart`:
- Around line 28-44: Update forgeKindForUrl in
app/lib/ui/widgets/forge_glyph.dart (lines 28-44) to classify using canonical
server-provided forge metadata rather than inferring Forgejo from arbitrary
hosts; when metadata is unavailable, return the neutral action result. Update
app/test/ui/widgets/forge_glyph_test.dart (lines 40-50) to replace the
arbitrary-host Forgejo expectation with coverage for provider metadata and the
neutral fallback.
In `@app/test/desktop/settings/repository_section_test.dart`:
- Around line 107-114: Make the home-abbreviation fixtures
environment-independent: in
app/test/desktop/settings/repository_section_test.dart lines 107-114, derive
_base.path from Platform.environment['HOME'] and use that same value for the
non-abbreviated expectation; in
app/integration_test/desktop/settings_repo_test.dart lines 170-178, derive the
worktreeRoot fixture values at lines 60 and 82 from Platform.environment['HOME']
so the existing ~/trees/diana and ~/.worktrees assertions remain valid on any
machine.
In `@app/tool/repo_settings_demo.dart`:
- Around line 110-114: Update _ShellState’s _key initialization to select the
intended “set to None” scene using its stable scene-name key rather than
_scenes.keys.elementAt(5). Preserve the existing default scene and the _choice
initialization.
- Around line 236-257: Update the _Section build method’s RepoSettingsView
reconstruction to pass through both base.hasRemote and base.logoHue, preserving
the configured no-remote state so the scene renders the intended wording while
retaining the existing field mappings.
In `@docs/DEVELOPMENT.md`:
- Around line 140-157: The provider detection documentation should explicitly
state that forgejo, gitea, and gitlab classifications are cached indefinitely,
while unknown results—including unidentifiable hosts and transport failures—are
cached for 60 seconds before retrying. Replace the existing “retried after a
minute rather than cached” wording with this precise cache behavior.
- Around line 181-194: Update the documentation around the “No quota panel” and
“Throttling still happens” bullets to qualify the Forgejo statement: say Forgejo
does not expose GitHub’s /api/v1/rate_limit quota model, while acknowledging
Gitea 1.25’s [qos] overload protection. Preserve the existing 429/503 handling
and Retry-After backoff behavior without claiming Forgejo has no throttling or
overload protection.
In `@docs/specs/2026-08-10-SPEC-48-per-repo-settings.md`:
- Line 216: Update D17 to explicitly define the allowed area, matching the
boundary enforced by validateWorktreeRoot (or state the intended alternative).
Correct the TOCTOU claim by requiring race-resistant filesystem operations for
worktree creation and removal, or explicitly documenting the residual
local-attacker risk if those operations are not guaranteed.
In `@docs/specs/2026-08-10-SPEC-48-PLAN.md`:
- Around line 65-75: Align the ForgeInspector API across both specifications by
choosing one method name and updating the interface, dependency wiring, and
related test descriptions consistently. Prefer updating the forge inspector
references around ForgeInspector, listRepos, and the per-repo settings
specification; only document a compatibility alias if both names must remain
supported.
In `@mockups/forge-detection-display.html`:
- Around line 233-237: Update the “UI affordances (future)” section and its note
to reflect the per-repository provider override handled by providerFor, removing
the claim that no settings UI exists and that FORGEJO_BASE_URL is the only
override; if this mockup intentionally documents an earlier design state, label
it explicitly as historical.
- Around line 229-231: The forge detection description in
mockups/forge-detection-display.html lines 229-231 must match classify: state
that probes run sequentially with a per-probe timeout, and escape the
greater-than sign as >. Update lines 250-258 to describe caching per
normalized host/base URL and re-checking failed probes after the negative TTL,
removing the claims that caching is per-worktree, never invalidated, or requires
restarting the app.
In `@mockups/repo-settings.html`:
- Line 478: Update the table row in mockups/repo-settings.html so the set
identifier in the description uses a <code> element instead of markdown
backticks, matching the existing repo.settings.get and repo.settings.set
markup.</code>
- Around line 323-325: Update the Logo row in the settings table to describe the
override as a persisted hue/palette selection rather than an image, consistent
with RepoSettings.logoHue. Also update the custom image state near the
referenced section to explicitly indicate that it is not built.
In `@scripts/sync-icons.sh`:
- Around line 45-46: Update the argument parsing in scripts/sync-icons.sh to
accept only no arguments or the single --check argument, and exit with an error
for unknown or extra arguments before any asset-copying or write operations
occur. Preserve check_only=true for --check and false when no argument is
supplied.
In `@server/src/forge/detect.ts`:
- Line 28: Move the ForgeSoftware union to the shared type definition in
types.ts, then re-export and consume that type from detect.ts instead of
declaring a duplicate. Update createDefaultForgeGateway’s router close() path to
call detector.clear(), ensuring positive detections with expiresAt: null are
removed during shutdown.
In `@server/src/forge/forgejo/gateway.test.ts`:
- Around line 448-460: Revise the “a successful response clears the backoff”
test to keep the clock within the Retry-After window, advance it by less than 30
seconds, and perform the successful request through an interactive call. Then
issue the background poll on branch "x" and assert it reaches the network, so
the test specifically verifies the backoff reset rather than natural expiry.
In `@server/src/forge/forgejo/gateway.ts`:
- Around line 186-192: Update createForgejoGateway’s branch and open-PR lookup
flows, including prForBranch and openPrs, to share concurrent requests through
an in-flight promise map keyed by repository path, branch where applicable,
limit for openPrs, and the interactive flag. Wrap only the request portion in
dedupe, preserving separate interactive/background behavior, and clear the
in-flight map in close().
- Around line 445-448: Update mutatePr so a successful mutation invalidates both
the branch-specific prKey(repoPath, branch) entry and every
open:${repoPath}:${limit} cache entry for that repository, regardless of limit.
Preserve the existing behavior of retaining cached entries when the mutation
fails, and reuse the cache key structure and available cache invalidation
mechanism.
In `@server/src/forge/router.test.ts`:
- Around line 611-626: Extend the test around createForgeRouter and prForBranch
so resolveInstance fails on the first call, then returns "forgejo" on the
second; invoke router.prForBranch twice and assert the calls include the initial
GitHub fallback followed by the Forgejo gateway call, verifying transient
auto-routing failures are not cached.
In `@server/src/forge/router.ts`:
- Around line 252-261: Update pick so an auto route that resolves to the
fallback GitHub gateway is not retained in chosen: after route resolves, record
the failure and asynchronously remove the matching cache entry before the next
call can reuse it, while preserving caching for successful routes and explicit
overrides. Use the existing route, chosen, and choice symbols, and add coverage
verifying a subsequent call after resolveInstance recovers routes to Forgejo.
- Around line 11-26: Update the module header to describe detection-based
routing rather than hostname-only selection: document the instance probe via
deps.detect, routing recognized GitHub and Forgejo/Gitea providers to their
gateways, routing GitLab or unidentifiable software through deps.unsupported,
and preserving per-repository overrides plus the none provider. Remove the
inaccurate claim that every non-GitHub remote uses the Forgejo gateway.
- Around line 449-473: Update the unsupported gateway wiring in
createForgeRouter to use the per-repository software decision from decided
rather than the shared currentSoftware state, preventing later repository
detection from changing earlier results. Remove the shared-state dependency and
add a regression test covering two repositories whose detections return
different forge software, verifying each repository reports its own decision.
In `@server/src/git.pr_checkout.test.ts`:
- Line 202: Update the assertion in the relevant checkout worktree test to
enforce the root boundary precisely instead of using startsWith. Compare r.path
with the exact expected path or use a boundary-aware path relationship check so
sibling paths such as `${f.base}-other` are rejected.
In `@server/src/manager.ts`:
- Around line 1342-1345: Update the invalid-override fallback in the
worktreeRoot assignment to return resolveWorktreeRoot(undefined, process.env)
unchanged, removing the forced source: "default" override so environment-sourced
roots retain source: "environment".
- Around line 1234-1241: Update settingsForPath to compare
canonicalPath(resolve(repoPath)) with canonicalPath(resolve(p.dto.path)) for
both lookup operands, preserving the existing parseRepoSettings return and empty
fallback.
In `@server/src/protocol.ts`:
- Around line 582-592: Update ProjectDTO.settings in server/src/protocol.ts
(lines 582-592) to use the persisted keys provider and logoHue, with logoHue
typed as number | null, or make the field an opaque Record<string, unknown>. In
server/src/manager.ts (lines 325-329), remove the as ProjectDTO['settings'] cast
so the corrected declaration is checked directly.
In `@server/src/repo_settings_wiring.test.ts`:
- Around line 93-130: Update both tests around validateWorktreeRoot and
collision detection to wrap their temporary directory setup and assertions in
try/finally cleanup blocks. Remove the created home-directory roots, override
subdirectories, and temporary repositories with rmSync in finally, matching the
existing cleanup pattern in the file while preserving the current assertions.
In `@server/src/repo_settings.test.ts`:
- Around line 153-159: Extend the rejected branch-name cases in the test for
validateBranch to include "main/", "main.", "feat/.x", "a@{0}", and "@". Ensure
validateBranch rejects all of these Git-invalid names while preserving the
existing valid cases and rejection assertions.
In `@server/src/repo_settings.ts`:
- Around line 120-127: Update the documentation for the ancestor
canonicalization rule near the relevant path-validation logic to remove the
claim that remaining not-yet-created segments must be plain names. Describe only
the implemented behavior: reject `..` segments, normalize duplicate separators,
and canonicalize through the nearest existing ancestor before handling missing
segments.
- Around line 273-280: Update validateBranch to reject trailing slash or dot,
leading slash, double slashes, leading or path-component dots, “@{”, and ASCII
control characters, while preserving acceptance of a bare “@”. Keep the existing
invalid-character, “..”, leading-hyphen, and “.lock” checks, and do not add
shell-metacharacter validation to defaultBranch Git calls.
In `@server/src/server.ts`:
- Around line 875-878: Update the onProjectsChanged callback in the
project/repository change handlers to re-broadcast the projects snapshot after
repo.path.set changes ProjectDTO.path or ProjectDTO.name, while preserving the
existing repos snapshot broadcast for repository settings. Ensure connected
clients receive refreshed project data after a project re-point.
In `@server/src/ws/commands/deps.ts`:
- Around line 36-41: Make onProjectsChanged required in the relevant command
interface, removing its optional marker and the outdated test-fake rationale.
Preserve the existing server and test harness implementations, which already
provide this callback, so settings changes cannot be acknowledged without
triggering client re-renders.
In `@server/src/ws/commands/repo_settings.ts`:
- Around line 59-65: Validate each patch key before entering the raw === null
branch in the patch-processing loop, using the existing key-validation or switch
logic around the command handler. Ensure unknown keys, including __proto__, are
rejected rather than added to applied or passed to
manager.updateProjectSettings, while preserving null as the valid clear/inherit
value for recognized settings.
- Around line 105-112: Update the logoHue validation in the command handler’s
logoHue case and in parseRepoSettings to require 0 <= logoHue < 6, rejecting
values of 6 or greater while preserving the existing non-integer and
negative-value checks.
In `@server/test/ws/repo_settings_commands.test.ts`:
- Around line 181-188: Extend the test “an unknown setting key is refused rather
than quietly stored” to dispatch an unknown setting with a null value, such as
wroktreeRoot: null, and assert it is rejected without writing. Ensure this
exercises the earlier null-clear branch in the repo settings command handling
rather than only the switch default.
- Around line 76-81: Update the test setup around goodRoot to clean up the
~/.makit-test-cmd-trees directory it creates in the real home directory after
the test suite completes, while preserving validation through
validateWorktreeRoot without a home override. Ensure cleanup runs even when
tests fail and does not remove unrelated home-directory contents.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 92f93a31-84cd-42cf-8de3-d6aeb372b5c6
⛔ Files ignored due to path filters (2)
app/assets/icons/forgejo-light.svgis excluded by!**/*.svgapp/assets/icons/gitea-light.svgis excluded by!**/*.svg
📒 Files selected for processing (64)
.gitignoreapp/ASSET_ATTRIBUTION.mdapp/integration_test/desktop/settings_repo_test.dartapp/lib/desktop/settings/registry/settings_registry.dartapp/lib/desktop/settings/registry/settings_section.dartapp/lib/desktop/settings/repository_settings_page.dartapp/lib/desktop/settings/sections/repository_section.dartapp/lib/desktop/settings/settings_nav_pane.dartapp/lib/desktop/settings/settings_window.dartapp/lib/store/models.dartapp/lib/store/store.dartapp/lib/ui/home/repo_monogram.dartapp/lib/ui/widgets/forge_glyph.dartapp/lib/ui/widgets/pr_detail.dartapp/pubspec.yamlapp/test/desktop/settings/repo_settings_view_test.dartapp/test/desktop/settings/repository_section_test.dartapp/test/desktop/settings/settings_nav_repo_search_test.dartapp/test/desktop/settings/settings_registry_repos_test.dartapp/test/ui/session/session_pr_test.dartapp/test/ui/widgets/forge_glyph_test.dartapp/tool/e2e-desktop-settings.shapp/tool/repo_settings_demo.dartdocs/DEVELOPMENT.mddocs/specs/2026-08-10-SPEC-48-PLAN.mddocs/specs/2026-08-10-SPEC-48-per-repo-settings.mdmockups/forge-detection-display.htmlmockups/forge-provider-per-repo.htmlmockups/repo-settings.htmlscripts/forgescripts/sync-icons.shserver/src/forge/cadence.test.tsserver/src/forge/cadence.tsserver/src/forge/detect.test.tsserver/src/forge/detect.tsserver/src/forge/forgejo/gateway.test.tsserver/src/forge/forgejo/gateway.tsserver/src/forge/forgejo/map.test.tsserver/src/forge/forgejo/map.tsserver/src/forge/none.test.tsserver/src/forge/none.tsserver/src/forge/router.test.tsserver/src/forge/router.tsserver/src/forge/types.tsserver/src/forge/unsupported.test.tsserver/src/forge/unsupported.tsserver/src/git.pr_checkout.test.tsserver/src/git.test.tsserver/src/git.tsserver/src/github/gateway.tsserver/src/github/policy.tsserver/src/github/queries.tsserver/src/manager.tsserver/src/project-store.test.tsserver/src/project-store.tsserver/src/protocol.tsserver/src/repo_service.tsserver/src/repo_settings.test.tsserver/src/repo_settings.tsserver/src/repo_settings_wiring.test.tsserver/src/server.tsserver/src/ws/commands/deps.tsserver/src/ws/commands/repo_settings.tsserver/test/ws/repo_settings_commands.test.ts
Resolves the two conflicts the PR reported: - `server/src/manager.ts` — both sides added imports and a constructor line. Kept main's `sessionTokens`/`lineage`/`ApprovalPolicy` imports and `closeGraceMs` (SPEC-29 session closing) alongside this branch's `RepoSettingsDTO` and forge-router wiring, and rewrote the gateway comment so it describes the router as it now behaves (per-repo setting first, detection second) rather than the hostname-only rule it started as. - `server/src/server.ts` — main moved command registration out to a module-level `buildCommandRouter`. Ported this branch's addition into it (`registerRepoSettingsCommands`) instead of keeping the old inline function. Merged rather than rebased on purpose: the PR carries 40 review threads anchored to these commits, and rewriting 36 commits would need a force-push that orphans every one of them. Also folds in review thread #35 while touching the same lines: a re-point changes `ProjectDTO.path` AND `name`, which only `projects.snapshot` carries, so `onProjectsChanged` now broadcasts both snapshots. Sending only `repos.snapshot` left every client showing the repo's old location. Server 2027/2027, typecheck clean.
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5605fdc1-ce30-4bed-bfe4-9757fb37656c) |
|
Macroscope has since reviewed this pull request. An earlier review was skipped by a cost limit; a review has now completed, so that notice no longer applies. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
app/tool/repo_settings_demo.dart (2)
110-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKey the default scene by name, not by position.
_scenes.keys.elementAt(5)depends on the literal's insertion order. Adding or reordering a scene silently changes the default and breaks the intent stated at Lines 111-112.♻️ Proposed refactor
- String _key = _scenes.keys.elementAt(5); + String _key = 'set to None — polling off';🤖 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 `@app/tool/repo_settings_demo.dart` around lines 110 - 114, Update _ShellState’s _key initialization to reference the intended “set to None” scene by its explicit scene-name key rather than _scenes.keys.elementAt(5). Keep the existing default scene unchanged while removing the dependency on _scenes insertion order.
236-257: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_SectiondropshasRemote, so the no-remote scene shows the wrong wording.The reconstructed
RepoSettingsViewat Lines 238-250 copies eleven fields and omitshasRemoteandlogoHue.hasRemotedefaults totrue. The scene at Lines 61-68 setshasRemote: falseto showAuto: no remote, so no forge, but the harness rendersAuto: not identified yet. The harness exists to check that wording.🐛 Proposed fix
worktreeRootOverridden: base.worktreeRootOverridden, editable: base.editable, providerChoice: choice, branches: base.branches, + hasRemote: base.hasRemote, + logoHue: base.logoHue, ),🤖 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 `@app/tool/repo_settings_demo.dart` around lines 236 - 257, Update the RepoSettingsView reconstruction in _Section.build to preserve the source view’s hasRemote and logoHue values, rather than relying on their defaults; this ensures the no-remote scene retains its “no remote” wording and the logo presentation remains consistent.app/lib/desktop/settings/sections/repository_section.dart (2)
257-276: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe Default branch row is hidden in the state it was built for.
Line 257 renders the row only when
view.defaultBranch != null. Line 112 leaves that value null when git reported noorigin/HEADand no override exists. The comment on Lines 266-268 states the picker is required for exactly that state. The user then cannot reach the picker to set the override.Render the row whenever branches can be picked. Show an explicit placeholder when no branch is resolved.
🐛 Proposed fix
- if (view.defaultBranch != null) + if (view.defaultBranch != null || view.branches.isNotEmpty) _SettingsValueRow( leading: Icon( PhosphorIconsLight.gitBranch, size: 20, color: cs.outline, ), title: 'Default branch', // Pickable from the repo's own branches, never free text: a typo // here silently breaks diff-vs-default and the PR base. Needed // because `origin/HEAD` is genuinely absent after a // `--single-branch` clone or a default-branch rename. enabled: view.editable && view.branches.isNotEmpty, onTap: onChooseDefaultBranch, - value: view.defaultBranch, + value: view.defaultBranch ?? 'not set', mono: true, action: _Chevron( enabled: view.editable && view.branches.isNotEmpty, ), ),🤖 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 `@app/lib/desktop/settings/sections/repository_section.dart` around lines 257 - 276, Update the Default branch row condition in the settings section to render whenever the repository has pickable branches, including when view.defaultBranch is null. Preserve editability and chevron behavior, and display an explicit placeholder value for the unresolved default-branch state instead of passing null.
317-321: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_tildeabbreviates paths that only share a prefix withHOME.
path.startsWith(home)matches/Users/leduck/xwhenHOMEis/Users/le. The row then shows~duck/x. That string is not a valid path and misnames the repository location. Require an exact match or a path separator after the home prefix.🐛 Proposed fix
static String _tilde(String path) { final home = Platform.environment['HOME']; if (home == null || home.isEmpty || !path.startsWith(home)) return path; - return '~${path.substring(home.length)}'; + final rest = path.substring(home.length); + if (rest.isNotEmpty && !rest.startsWith('/')) return path; + return '~$rest'; }🤖 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 `@app/lib/desktop/settings/sections/repository_section.dart` around lines 317 - 321, Update _tilde so it replaces HOME only when the path equals HOME or the character immediately following the HOME prefix is the platform path separator; otherwise return the original path unchanged. Preserve the existing handling for missing or empty HOME and the current tilde substitution for valid home-relative paths.app/test/desktop/settings/repository_section_test.dart (1)
107-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHardcoded
/Users/lefixtures make the home-abbreviation assertions machine-dependent.RepositorySettingsSection._tildereadsPlatform.environment['HOME']at runtime, so a fixture rooted at a fixed home only abbreviates on the machine whoseHOMEis/Users/le. Both test files assert the abbreviated form.
app/test/desktop/settings/repository_section_test.dart#L107-L114: build_base.pathat Line 19 fromPlatform.environment['HOME']and assert the negative case against that same value; the coding guidelines state this file runs underflutter test --no-pubon the Linux VM, whereHOMEis never/Users/le.app/integration_test/desktop/settings_repo_test.dart#L170-L178: build theworktreeRootfixture values at Line 60 and Line 82 fromPlatform.environment['HOME']so the~/trees/dianaand~/.worktreesassertions at Line 172, Line 202 and Line 246 hold on any macOS machine.🤖 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 `@app/test/desktop/settings/repository_section_test.dart` around lines 107 - 114, Make the home-abbreviation fixtures environment-independent: in app/test/desktop/settings/repository_section_test.dart, derive _base.path from Platform.environment['HOME'] and use that same value for the non-abbreviated negative assertion; in app/integration_test/desktop/settings_repo_test.dart, derive both worktreeRoot fixtures from Platform.environment['HOME'] so the existing ~/trees/diana and ~/.worktrees assertions remain valid on any machine.Source: Coding guidelines
🤖 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 `@app/lib/store/store.dart`:
- Around line 804-823: Update setRepoSettings to use the request-based API
instead of send, matching setRepoPath, so server err frames reach the caller for
display. Preserve the existing repo.settings.set envelope and fire-and-forget
caller behavior while ensuring refused writes are surfaced through the request
error path.
In `@app/test/desktop/settings/settings_nav_repo_search_test.dart`:
- Line 38: Store the TextEditingController passed to SettingsNavPane in a local
variable within the test, then register addTearDown(controller.dispose) so the
test-owned controller is released after execution.
In `@app/tool/e2e-desktop-settings.sh`:
- Around line 23-27: Update the FLUTTER_BIN initialization in the
e2e-desktop-settings script to fall back to ~/flutter/bin/flutter when
MAKIT_FLUTTER_BIN and command -v flutter do not provide a value, before the
existing “flutter not found” failure check.
In `@mockups/forge-provider-per-repo.html`:
- Around line 142-148: Update mockups/forge-provider-per-repo.html lines 142-148
to state the shipped UI behavior instead of claiming forge information is not
surfaced; revise or label mockups/forge-provider-per-repo.html lines 472-496 as
historical and reconcile its delta list with the shipped implementation. In
mockups/repo-settings.html lines 309-321, remove the nonexistent global-settings
layer and document the resolver order as repo override → environment variable →
built-in default.
In `@server/src/forge/detect.ts`:
- Around line 146-149: Update the GitLab detection logic around probe and ok so
401/403 from gitlabProbeUrl(baseUrl) count as GitLab only when at least one
earlier Forgejo probe was answered by the application, rather than when all
probes share the same auth status. Preserve successful GitLab responses and
avoid classifying blanket-auth proxy responses as GitLab.
In `@server/src/forge/router.ts`:
- Around line 441-469: Memoize the remote lookup in readRemote by repository
path so resolveRepo and resolveInstance share one git result across gateway
calls. Reuse the parsed remote/ref data in resolveInstance instead of parsing
the same URL twice. Connect the memoization store to the existing forgetRepo
invalidation path so repointed repositories fetch a fresh remote.
In `@server/src/git.ts`:
- Around line 123-131: Update resolveDefaultBranch to accept a non-empty
override when it matches either a local branch or an origin remote-tracking ref,
using a default-resolution-specific check alongside or instead of branchExists.
Preserve branchExists behavior for other callers, and continue falling back to
detectDefaultBranch when neither ref exists.
In `@server/src/repo_settings_wiring.test.ts`:
- Around line 60-75: Update the “two repos with different overrides get
different worktree roots” test to compare repo A’s result with the canonicalized
value returned by validating rootA, rather than raw rootA from homedir(). Reuse
the existing validation pattern or helper used by “the stored root is used
verbatim once validated, symlinks resolved,” while preserving the inherited-root
assertion for repo B.
---
Duplicate comments:
In `@app/lib/desktop/settings/sections/repository_section.dart`:
- Around line 257-276: Update the Default branch row condition in the settings
section to render whenever the repository has pickable branches, including when
view.defaultBranch is null. Preserve editability and chevron behavior, and
display an explicit placeholder value for the unresolved default-branch state
instead of passing null.
- Around line 317-321: Update _tilde so it replaces HOME only when the path
equals HOME or the character immediately following the HOME prefix is the
platform path separator; otherwise return the original path unchanged. Preserve
the existing handling for missing or empty HOME and the current tilde
substitution for valid home-relative paths.
In `@app/test/desktop/settings/repository_section_test.dart`:
- Around line 107-114: Make the home-abbreviation fixtures
environment-independent: in
app/test/desktop/settings/repository_section_test.dart, derive _base.path from
Platform.environment['HOME'] and use that same value for the non-abbreviated
negative assertion; in app/integration_test/desktop/settings_repo_test.dart,
derive both worktreeRoot fixtures from Platform.environment['HOME'] so the
existing ~/trees/diana and ~/.worktrees assertions remain valid on any machine.
In `@app/tool/repo_settings_demo.dart`:
- Around line 110-114: Update _ShellState’s _key initialization to reference the
intended “set to None” scene by its explicit scene-name key rather than
_scenes.keys.elementAt(5). Keep the existing default scene unchanged while
removing the dependency on _scenes insertion order.
- Around line 236-257: Update the RepoSettingsView reconstruction in
_Section.build to preserve the source view’s hasRemote and logoHue values,
rather than relying on their defaults; this ensures the no-remote scene retains
its “no remote” wording and the logo presentation remains consistent.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: d46a4214-0453-48a3-a8ca-ae753390bce8
⛔ Files ignored due to path filters (2)
app/assets/icons/forgejo-light.svgis excluded by!**/*.svgapp/assets/icons/gitea-light.svgis excluded by!**/*.svg
📒 Files selected for processing (64)
.gitignoreapp/ASSET_ATTRIBUTION.mdapp/integration_test/desktop/settings_repo_test.dartapp/lib/desktop/settings/registry/settings_registry.dartapp/lib/desktop/settings/registry/settings_section.dartapp/lib/desktop/settings/repository_settings_page.dartapp/lib/desktop/settings/sections/repository_section.dartapp/lib/desktop/settings/settings_nav_pane.dartapp/lib/desktop/settings/settings_window.dartapp/lib/store/models.dartapp/lib/store/store.dartapp/lib/ui/home/repo_monogram.dartapp/lib/ui/widgets/forge_glyph.dartapp/lib/ui/widgets/pr_detail.dartapp/pubspec.yamlapp/test/desktop/settings/repo_settings_view_test.dartapp/test/desktop/settings/repository_section_test.dartapp/test/desktop/settings/settings_nav_repo_search_test.dartapp/test/desktop/settings/settings_registry_repos_test.dartapp/test/ui/session/session_pr_test.dartapp/test/ui/widgets/forge_glyph_test.dartapp/tool/e2e-desktop-settings.shapp/tool/repo_settings_demo.dartdocs/DEVELOPMENT.mddocs/specs/2026-08-10-SPEC-48-PLAN.mddocs/specs/2026-08-10-SPEC-48-per-repo-settings.mdmockups/forge-detection-display.htmlmockups/forge-provider-per-repo.htmlmockups/repo-settings.htmlscripts/forgescripts/sync-icons.shserver/src/forge/cadence.test.tsserver/src/forge/cadence.tsserver/src/forge/detect.test.tsserver/src/forge/detect.tsserver/src/forge/forgejo/gateway.test.tsserver/src/forge/forgejo/gateway.tsserver/src/forge/forgejo/map.test.tsserver/src/forge/forgejo/map.tsserver/src/forge/none.test.tsserver/src/forge/none.tsserver/src/forge/router.test.tsserver/src/forge/router.tsserver/src/forge/types.tsserver/src/forge/unsupported.test.tsserver/src/forge/unsupported.tsserver/src/git.pr_checkout.test.tsserver/src/git.test.tsserver/src/git.tsserver/src/github/gateway.tsserver/src/github/policy.tsserver/src/github/queries.tsserver/src/manager.tsserver/src/project-store.test.tsserver/src/project-store.tsserver/src/protocol.tsserver/src/repo_service.tsserver/src/repo_settings.test.tsserver/src/repo_settings.tsserver/src/repo_settings_wiring.test.tsserver/src/server.tsserver/src/ws/commands/deps.tsserver/src/ws/commands/repo_settings.tsserver/test/ws/repo_settings_commands.test.ts
Each has a failing test first, and the bite proven by reverting the production
line.
Correctness / stability
- A fallback route is no longer CACHED. `pick` stored whatever `route` resolved,
including the catch-block fallback, and nothing evicted it — so one failed
`git remote` read at startup pinned a Forgejo repo to the `gh` gateway for the
lifetime of the daemon, every poll failing and the PR pill reading `unknown`
with no way back short of a restart. Decided routes are still cached, so the
fan-out stays at one remote read per repo. `unknown` detection is also treated
as undecided and re-probed.
- The unsupported gateway named the wrong forge. `currentSoftware` was one shared
variable set by whichever repo was probed most recently, so a mutation on a
GitLab repo could report another forge's name. It now asks the router for THAT
repo's decision.
- `settingsForPath` compared paths with `resolve` while `addProject` stores the
resolved-but-not-canonicalised spelling, so a project added through a symlink
failed its own lookup whenever a caller passed the canonical path (which is what
git hands back). Every override then silently fell back to the default while the
UI still showed it.
- An invalid stored worktree root fell back correctly and then relabelled the
source `"default"`, so with `MAKIT_WORKTREE_DIR` set the badge named the wrong
origin — the one thing `SettingSourceDTO` exists to prevent.
- `mutatePr` invalidated only the branch lookup, so `open:<repo>:<limit>` survived
its TTL. That list backs the "New worktree from PR" picker: a squash-merged PR
stayed listed and the checkout that followed failed. Every limit is dropped, not
just one, because the picker and the home screen ask with different ones.
Data integrity
- `repo.settings.set` skipped the unknown-key rule whenever the value was `null`,
so `{wroktreeRoot: null}` was acked and the typo written into the patch. The key
is validated first, against a `Set` so inherited names are not keys — reproduced
with `JSON.parse('{"__proto__":null}')`, since a source literal sets the
prototype and creates no key at all.
- `logoHue` is bounded to the palette. `paletteAt` wraps with `%`, so a stored 6
rendered as index 0: a colour the user never chose, indistinguishable from
having chosen it.
- `ProjectDTO.settings` declared `gitProvider`/`logo` while the server persists
`provider`/`logoHue`, and a cast hid the mismatch, so a client reading
`settings.gitProvider` always got `undefined`. Declared as the stored keys plus
an index signature (unknown keys are preserved on purpose), and the cast removed
so the compiler checks the shape.
- `validateBranch` accepted refs git itself refuses: trailing `/` or `.`, a
leading `.` or `/`, `//`, `@{`, ASCII control characters, and a `.`-prefixed or
`.lock`-suffixed path component.
Performance
- The Forgejo gateway shares in-flight requests, keyed per (repo, branch) and per
(repo, limit). The cache only helps AFTER a response, so on a cold cache the
home-screen fan-out issued one copy per worktree of a query this module measures
at 1.5–30s against a real instance.
Housekeeping
- One `ForgeSoftwareName` union instead of two identical ones; `detector.clear()`
now runs on close, since positive detections never expire.
- `onProjectsChanged` is required, for the same reason `onPortsWatchersChanged`
is: a router built without it acks a write that no client ever sees.
- `sync-icons.sh` validates arguments BEFORE touching the filesystem — `--chek`
silently took the write path, so a command meant to verify vendored assets
overwrote them.
- Two tests created directories in the real `$HOME` and never removed them (one
accumulating a subdirectory per run); both now use a unique name and clean up.
- The backoff-clearing test advanced the clock PAST the window, so it passed with
the reset deleted. It now stays inside the window and forces the success through
an interactive call.
- A worktree-root assertion used `startsWith`, which also accepts a sibling
directory outside the chosen root; it asserts the exact path.
- The router module header described hostname-only routing, which stopped being
true three commits ago.
Server 2041/2041, typecheck clean.
Functional correctness - The Default branch row was rendered only when a branch was already resolved, which hid it in exactly the state it exists for: git reports no `origin/HEAD` after a `--single-branch` clone or a default-branch rename and no override has been set yet, so the user could not reach the picker precisely when they needed it. It now always renders, reading `Not detected` — a stated absence rather than a blank cell — and stays read-only only when there is genuinely nothing to pick. - `_tilde` abbreviated any path merely SHARING the home prefix, so with `HOME=/Users/le` a repo at `/Users/leduck/x` rendered as `~duck/x`: not a valid path, and a different repository. It now requires a separator boundary, and `$HOME` itself renders as `~`. - `forgeKindForUrl` labelled every non-GitHub, non-`gitea.com` host as Forgejo. That agreed with the router while the router also guessed by hostname, but the router now probes the instance — so the guess could contradict the provider that served the data, putting Forgejo's name and mark on a self-hosted Gitea or a GitLab remote. An unidentifiable host is now left unnamed (the neutral external-link action), which is what this widget's own contract already asked for: naming the wrong forge is worse than naming none. It also takes an optional `detected` argument — the server's `forge.software` — which wins when supplied, so the glyph can be restored properly wherever the repo is in scope. - When the stored `repo:<id>` section is unavailable the detail pane fell back to the first section while `_selectedId` still held the missing id, so the sidebar highlighted nothing and the window looked like it had lost its place. The nav pane is now told which section is actually shown. Test correctness - `/Users/le` was hardcoded in two suites while `_tilde` reads `HOME` at runtime, so every abbreviation assertion held on exactly one machine — and one of those suites runs on the Linux VM, where it could never hold. Both build their fixtures from the real `HOME`. Harness correctness - `repo_settings_demo.dart` reconstructed the view from eleven fields and dropped `hasRemote` and `logoHue`. `hasRemote` defaults to true, so the "local-only" scene — which exists to show `Auto: no remote, so no forge` — rendered `Auto: not identified yet`: the harness demonstrated the exact wording it was built to check against. - The default scene was `_scenes.keys.elementAt(5)`, so adding or reordering a scene silently changed it and contradicted the comment above it. Keyed by name. Docs and mockups - `DEVELOPMENT.md`: state the real detection cache (decisive answers for the process lifetime keyed by normalised base URL; `unknown` for 60s then re-probed) and that a per-repo provider setting short-circuits probing entirely. Soften the Forgejo rate-limit claim to what is verifiable: it exposes no quota to read and ships no instance-wide limiter, so limiting comes from whatever sits in front. - SPEC-48: `ForgeInspector` is `forgeFor`/`hasRemoteFor` in both documents (an earlier draft said `softwareFor`, which never existed); D17 names the allowed area as `$HOME` and states plainly that read-back validation does NOT close the filesystem TOCTOU window, with the residual risk accepted rather than implied to be mitigated. - `forge-detection-display.html`: probes run in sequence with per-probe timeouts, not in parallel under one budget; the cache is per host with `unknown` re-probed; and the "no settings UI" section is marked superseded. - `repo-settings.html`: the logo override is a palette index (`logoHue`), not an image; the "custom image" state is marked not built; and a stray markdown backtick is a `<code>` element like every other identifier in that table. Also folds in a `TextEditingController` disposal fix for both dialogs that was already present in the tree, from a parallel agent working in this worktree — it is correct and analyse-clean, and it was swept into an earlier commit of mine by `git add -A`. Recorded here so the history is not misleading. flutter analyze clean; every touched suite green; the 5 macOS integration tests green. The whole-file `loading` failures in the full run are the known harness flake — all 26 pass when run together or alone, and the set differs per run.
The pre-push hook's formatter reflowed the lines I touched. Committing its output so the hook is a no-op rather than something the next push has to fight; `git diff -w` shows nothing but line wrapping, and the touched suites are green.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b226b002-c658-425c-8251-419431edf086) |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Eight new findings from re-reviewing the pushed fixes.
Correctness
- A refused settings write was completely silent. `setRepoSettings` used
fire-and-forget `send`, so the server's `err` frame had no consumer: a
non-loopback client, an invalid worktree root or an invalid branch name left the
row unchanged and said nothing. That is precisely the failure the server handler
documents as unacceptable ("a settings row that appears to save and does not is
worse than one that says it cannot") — the daemon took care to refuse explicitly
and the app threw the refusal away. Now awaited, with the server's own message
shown, matching `setRepoPath`.
- A default-branch override naming a REMOTE-only branch was silently dropped.
`branchExists` checks `refs/heads` alone, but after a `--single-branch` clone the
intended default exists only as `origin/<branch>` — the exact case the override
exists for, so the feature did nothing in its main scenario. Resolution now
accepts either, and an override naming nothing at all is still dropped.
- A gate that answers 401/403 on every path was classified GitLab. Step 3 treated
an auth status on `/api/v4/version` as proof, reasoning that "only GitLab serves
that path" — true of the path, but the STATUS only carries information if an
earlier probe was answered by the application rather than by the gate. An
ordinary Forgejo instance behind SSO was therefore routed to the unsupported
provider and logged as "looks like gitlab". When all three probes return the same
auth status the answer is `unknown`, which is re-probed and can be overridden.
Performance
- `git remote get-url origin` ran on EVERY gateway call. The Forgejo gateway calls
`resolveRepo` at the top of `prForBranch`, `openPrs` and `mutatePr`, before its
own cache is consulted — so even a cache hit paid for a subprocess, and the
home-screen fan-out spawned one process per worktree per poll tick. The URL is
now memoised per repo and shared while in flight. A failed read is not retained,
and both cases that can change a repo's origin clear it: re-pointing the project
and shutdown.
Housekeeping
- The nav-search test owned a `TextEditingController` and never disposed it.
- `e2e-desktop-settings.sh` falls back to `~/flutter/bin/flutter`, the location
DEVELOPMENT.md documents, before failing — a non-interactive shell may not have
it on PATH.
- `forge-provider-per-repo.html` is marked as a proposal with a note on what has
since shipped, so it cannot be read as current behaviour.
- A worktree-root test compared against the raw home path; `validateWorktreeRoot`
canonicalises what it returns, so on a machine whose home is a symlink it could
never match.
Server 2048/2048, typecheck clean; flutter analyze clean; all touched suites and
the 5 macOS integration tests green.
Not covered by a test: the settings-write refusal path. Driving an `err` frame
through the store needs a connection fake this suite does not have, and the change
mirrors `setRepoPath`, which was reviewed and accepted in the same shape. Stated
rather than implied.
Hook formatter output; `git diff -w` is line wrapping only.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4066e765-87af-4ee4-afe9-29825d4b9d9f) |
Owner
Author
|
Both blockers cleared: base conflict resolved and all review threads addressed. ConflictMerged ThreadsTwo rounds, ~30 distinct findings. The ones that were real bugs rather than polish:
Three suggestions I did not take as written, each with reasoning in-thread: recording a Verification
One gap stated rather than implied: the settings-write refusal path has no test — driving an |
…gration - resolveDefaultBranch now returns remote-only branches qualified (`origin/`), which git revision rules can parse - syncBaseBranch guards remote-only bases instead of mangling them - Error surfacing moved from showSnackBar (forbidden by SPEC-48 D7 convention) to StatusCenter, landing on the Activity record - Status ref hoisted before first await, per status_lifetime_test guard - forge_glyph uses decisive-host list for Codeberg/gitea.com, leaves arbitrary hosts unnamed instead of guessed
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f774f7fc-35b6-4a6f-ba1a-d8bfbff4c84b) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/test/ui/widgets/forge_glyph_test.dart (1)
103-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert each provider's expected glyph.
The Forgejo self-comparison always passes. The distinct-value checks also pass if Forgejo and Gitea are swapped. Assert the exact Forgejo and Gitea SVG asset mapping so this test detects incorrect provider branding.
🤖 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 `@app/test/ui/widgets/forge_glyph_test.dart` around lines 103 - 118, Update the test for forgeGlyphFor to assert each provider’s exact expected glyph asset, replacing the Forgejo self-comparison and broad inequality checks. Ensure the assertions explicitly verify Forgejo maps to its SVG and Gitea maps to its distinct SVG, while preserving the GitHub Phosphor glyph assertion.app/tool/repo_settings_demo.dart (1)
242-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the manual field copy with a
copyWithonRepoSettingsView.This block rebuilds every field of
RepoSettingsViewby hand. ThehasRemoteomission that made the "local-only" scene render the wrong wording came from exactly this pattern, and the comment at Lines 254-257 records it. Any field added toRepoSettingsViewlater will be silently defaulted here again, and the harness will misrepresent the scene it exists to demonstrate.Add
copyWithtoRepoSettingsViewinapp/lib/desktop/settings/sections/repository_section.dartand override onlyproviderChoicehere.♻️ Proposed refactor
Add to
RepoSettingsViewinapp/lib/desktop/settings/sections/repository_section.dart:RepoSettingsView copyWith({ForgeChoice? providerChoice}) => RepoSettingsView( name: name, path: path, worktreeRoot: worktreeRoot, defaultBranch: defaultBranch, forge: forge, forgeHost: forgeHost, forgeAuthed: forgeAuthed, worktreeRootOverridden: worktreeRootOverridden, editable: editable, providerChoice: providerChoice ?? this.providerChoice, branches: branches, hasRemote: hasRemote, logoHue: logoHue, );Then in
app/tool/repo_settings_demo.dart:Widget build(BuildContext context) => RepositorySettingsSection( key: ValueKey(scene), - view: RepoSettingsView( - name: base.name, - path: base.path, - worktreeRoot: base.worktreeRoot, - defaultBranch: base.defaultBranch, - forge: base.forge, - forgeHost: base.forgeHost, - forgeAuthed: base.forgeAuthed, - worktreeRootOverridden: base.worktreeRootOverridden, - editable: base.editable, - providerChoice: choice, - branches: base.branches, - // Copied, not defaulted. `hasRemote` defaults to TRUE, so the "local-only" - // scene -- which exists to show `Auto: no remote, so no forge` -- rendered - // `Auto: not identified yet` instead, i.e. the harness demonstrated the exact - // wording it was built to check against. - hasRemote: base.hasRemote, - logoHue: base.logoHue, - ), + // `copyWith`, not a field-by-field rebuild: the omitted `hasRemote` made the + // "local-only" scene render `Auto: not identified yet` instead of the wording + // it exists to demonstrate. Only the provider choice is scene state. + view: base.copyWith(providerChoice: choice), onChooseProvider: onChoose,🤖 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 `@app/tool/repo_settings_demo.dart` around lines 242 - 260, Add a RepoSettingsView.copyWith method in the RepoSettingsView class that preserves every existing field and optionally replaces providerChoice, then replace the manual constructor in the demo’s view creation with base.copyWith(providerChoice: choice). Remove the duplicated field-copy block so future fields are retained automatically.
🤖 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 `@app/lib/desktop/settings/repository_settings_page.dart`:
- Around line 287-294: In _promptRootPath, check context.mounted immediately
after the dialog await and before reading storeControllerProvider through ref.
Return early when the widget is unmounted, then preserve the existing value
validation and setRepoPath flow.
In `@docs/specs/2026-08-10-SPEC-48-per-repo-settings.md`:
- Around line 216-218: The D17 decision and rationale are split because the
decision cell lacks its trailing table delimiter. Restore the Markdown table
structure by keeping the entire D17 content in one row, placing the rationale in
the Why column, and using <br> within the decision cell for paragraph breaks
without introducing a new table row.
In `@server/src/forge/router.test.ts`:
- Around line 797-818: Update the test around createDefaultForgeGateway to
return a non-GitHub remote so routing enters the Forgejo gateway path and
exercises resolveRepo from prForBranch, openPrs, and mutatePr. Stub the
gateway’s HTTP dependency or otherwise prevent network access, then retain the
assertion that remote get-url origin executes exactly once across the calls.
In `@server/src/git.ts`:
- Around line 791-793: Update the branch handling in resolveDefaultBranch so the
origin/ prefix is treated as remote-only only when no matching local branch
exists; preserve synchronization for a local branch named origin/trunk. Add a
regression test covering that local branch scenario.
In `@server/src/repo_settings_wiring.test.ts`:
- Around line 102-105: Complete temporary fixture cleanup in
server/src/repo_settings_wiring.test.ts: for lines 102-105, wrap the test logic
after creating directory a in a finally block that always removes a; for lines
653-666, retain the mkdtempSync result for the symlink parent and remove that
directory in finally. Update the relevant test bodies without changing their
assertions or behavior.
---
Outside diff comments:
In `@app/test/ui/widgets/forge_glyph_test.dart`:
- Around line 103-118: Update the test for forgeGlyphFor to assert each
provider’s exact expected glyph asset, replacing the Forgejo self-comparison and
broad inequality checks. Ensure the assertions explicitly verify Forgejo maps to
its SVG and Gitea maps to its distinct SVG, while preserving the GitHub Phosphor
glyph assertion.
In `@app/tool/repo_settings_demo.dart`:
- Around line 242-260: Add a RepoSettingsView.copyWith method in the
RepoSettingsView class that preserves every existing field and optionally
replaces providerChoice, then replace the manual constructor in the demo’s view
creation with base.copyWith(providerChoice: choice). Remove the duplicated
field-copy block so future fields are retained automatically.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 1367a4d6-6d27-449f-b234-ee2118d95659
📒 Files selected for processing (37)
app/integration_test/desktop/settings_repo_test.dartapp/lib/desktop/settings/repository_settings_page.dartapp/lib/desktop/settings/sections/repository_section.dartapp/lib/desktop/settings/settings_window.dartapp/lib/store/store.dartapp/lib/ui/widgets/forge_glyph.dartapp/test/desktop/settings/repository_section_test.dartapp/test/desktop/settings/settings_nav_repo_search_test.dartapp/test/desktop/settings/settings_window_test.dartapp/test/ui/widgets/forge_glyph_test.dartapp/tool/repo_settings_demo.dartdocs/DEVELOPMENT.mddocs/specs/2026-08-10-SPEC-48-per-repo-settings.mdmockups/forge-detection-display.htmlmockups/forge-provider-per-repo.htmlmockups/repo-settings.htmlscripts/sync-icons.shserver/src/forge/detect.test.tsserver/src/forge/detect.tsserver/src/forge/forgejo/gateway.test.tsserver/src/forge/forgejo/gateway.tsserver/src/forge/router.test.tsserver/src/forge/router.tsserver/src/forge/unsupported.tsserver/src/git.pr_checkout.test.tsserver/src/git.test.tsserver/src/git.tsserver/src/manager.tsserver/src/protocol.tsserver/src/repo_settings.tsserver/src/repo_settings_wiring.test.tsserver/src/ws/commands/deps.tsserver/src/ws/commands/repo_settings.tsserver/test/ws/agents_catalog.test.tsserver/test/ws/pr_commands.test.tsserver/test/ws/repo_settings_commands.test.tsserver/test/ws/send_message_attachments.test.ts
Follow-up finding on the previous fix, and a fair one: the guard matched the `origin/` PREFIX, but `refs/heads/origin/release` is a legal local branch and `resolveDefaultBranch` returns such a name bare. `syncBaseBranch` then refused to fast-forward a perfectly ordinary local base, reporting "no local branch to catch up" about a branch that is exactly that. The discriminator is now which ref exists, not how the name is spelled: refuse only when the name looks remote-tracking AND no local branch of that name exists. Both halves are pinned separately, and each fails on its own when its condition is reverted — the local-`origin/...` case and the genuine remote-only refusal. Server 2052/2052, typecheck clean. (The production line was already present uncommitted, from the other agent working in this worktree; it is the right fix, so it is kept rather than churned, and this commit adds the test that holds it in place.)
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_cf02f73b-5adc-4194-8a76-fd6ec50fb2f4) |
I batch-resolved five threads without reading them all, which was wrong: one was a vacuous test of mine and three were real. Fixed here, and re-resolved honestly. - `router.test.ts` — the readRemote-memoisation test used a `github.com` remote, so routing picked the gh gateway, the Forgejo gateway's `resolveRepo` never ran, and one read for three calls was guaranteed by the ROUTING cache alone. It passed with the memo deleted, i.e. it proved nothing about the thing it names. Now uses a non-GitHub remote so the gateway path executes, and it fails when the memo is removed. - `SPEC-48` D17 — my earlier edit inserted a paragraph inside a table row, which broke the row: the `Why` column was orphaned and rendered outside the table. Folded back into one row with `<br><br>`. - `repo_settings_wiring.test.ts` — two fixtures leaked. One temp repo was removed on the last line of the body rather than in `finally`, so any assertion failure above it leaked; and a symlink's parent directory was never removed at all, leaving one empty temp dir per run. - `repository_settings_page.dart` — `context.mounted` is checked before `ref.read` after the dialog. The hoisted `status` survives the widget, but `ref` does not. Server 2052/2052, typecheck clean; flutter analyze clean; status guards green.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_227c5d70-6c09-4347-a23c-818665ab0a21) |
The row was disabled when no branches could be listed, AND `_pickBranch` returned early on an empty list — so a repo whose branches cannot be enumerated (an empty repo, or one whose branch went away) kept a stored override with no way back. A control that can set a value must be able to unset it. `RepoSettingsView` gains `defaultBranchOverridden`, mirroring `worktreeRootOverridden`, and the row is reachable when there is something to PICK or something to CLEAR. The "Use the branch git reports" option is valid whatever the branch list holds, because it sends `null`. The existing "no branches" test was too coarse and now says what it means: with nothing to pick and nothing to clear the row stays read-only, and with an override in force it is tappable. The second fails when either half of the fix is reverted. flutter analyze clean; settings and status suites green.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f77832a0-4719-404b-a6dc-456cf4be9d5e) |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What & why
Adds a Forgejo/Gitea git provider to makit, and a Settings section per repository to configure it.
Before this, makit spoke only to GitHub via
gh. Every non-GitHub remote was routed there, failed, and surfaced asunknown— indistinguishable from an outage. Self-hosted Forgejo/Gitea users had no pull-request support and no way to say so.The provider
ghone, behind a provider-neutralForgeGatewaycontract.BudgetReportingis deliberately kept separate: Forgejo exposes norate_limitendpoint and sends no rate-limit headers, so a Forgejo repo contributes nothing to the budget panel rather than faking a quota it cannot measure./api/forgejo/v1/version, then Gitea's) instead of guessing from the hostname.MAKIT_FORGEJO_BASE_URLset, the token is withheld from every other host — otherwise opening any public Gitea repo would have sent an internal token to a third party.Per-repo settings (SPEC-48)
One Settings section per pinned repository: logo, root path, git provider, default branch, worktree root. All five are writable, and — the point of P2 — all five now change behaviour:
auto|none|forgejo|gitea|github)addWorktree,addWorktreeForPr,uniqueWorktreeDirDecisions worth a reviewer's attention:
noneis notunsupported. Unsupported means we cannot talk to this forge and answersunknown;nonemeans do not talk to any forge here and answersnone.unknownwould make the app hold a stale PR pill and keep retrying — the chatternoneexists to stop.{repoPath, choice}. Without that the setting would apply only after a daemon restart, which is indistinguishable from a broken feature.WsClient.isLocal— not a UI convention.worktreeRootis a path the daemon creates directories under and, via prune, removes; a paired phone must not aim that anywhere it likes.validateRepoPathdeliberately differs fromvalidateWorktreeRoot: the repo path must already exist and is not confined to$HOME(makit never deletes a repo path, and a checkout on an external volume is ordinary), while a worktree root may not exist yet and is confined. A test pins the asymmetry so it isn't "tidied" into consistency.gh pr checkoutunconditionally, so the flow broke exactly halfway — the user saw their Forgejo PRs, picked one, and no worktree appeared. GitHub keepsgh(it handles fork PRs and push tracking); Forgejo/Gitea fetchrefs/pull/<n>/head, which the forge publishes for every PR including forks', whose branch isn't onorigin.Specs and decision records:
docs/specs/2026-08-10-SPEC-48-per-repo-settings.mdand its plan, including three rounds of review findings and what was cut.How it was tested
cd server && pnpm test && pnpm typecheck— 1519/1519, typecheck cleancd app && flutter test && flutter analyze— analyze clean; the touched suites passBeyond the suites:
sectionsFor, and the PR-checkout strategy.app/tool/e2e-desktop-settings.sh(new) mounts the realSettingsWindowand drivesreposProvider → sectionsFor → nav pane → page → rows, 5 tests. All 5 fail ifsectionsForignores the repo list. This catches the class of fault where a section navigates withcontext.gounder a shell that has no GoRouter — throwing at runtime while unit tests stay green.refs/pull/7/head— no network, no forge. Covers the PR-unique branch (the primary checkout commonly sits on the head ref, and git refuses to check one out twice), upstream tracking for same-repo PRs, a fork PR checking out without one, and rollback leaving no litter. Theghpath, which had no test before the refactor, is now pinned argv-and-all via a PATH shim.SessionManager, real command router, realprojects.json, persistence wired asserve.tswires it): 21/21 for the settings, then 12/12 forpr.list→worktree.createFromPron a host detection could not identify. Both probes were deleted after the run; their findings are in the spec.ocr(open-code-review); 14 findings applied, 6 rejected with reasons in the commit messages — including one whose suggested fix was wrong (recording "no remote" unconditionally in the fallback path would have overwritten a correcttruewhen detection, not the remote read, was what failed; there's now a test that fails against that version).Bugs found and fixed along the way
hasRemotewas derived fromforge !== undefined, collapsing three states — not measured, no remote, a forge — into one boolean. Every un-polled repo claimed to have no origin, which made the app'sAuto: not identified yetwording unreachable...rejection in path validation was dead code: it split a normalised path, andnormalize()collapses...realpathfails on a path that doesn't exist, so canonicalisation had to walk to the nearest existing ancestor — otherwise naming a worktree root before creating it, the common case, was refused.addProjectdeduped withresolve()only, so/tmp/xand/private/tmp/xcould become two projects for one directory — the exact ambiguityrepointProjectrefuses.Reviewer notes
mainand conflicts in one file,server/src/manager.ts. Everything else auto-merges. I left it unresolved rather than mergingmaininto a 34-commit branch unasked — say the word and I'll do the merge and re-run both suites.mainalso carries a PR titled "Activity … (SPEC-48/49)", so the number SPEC-48 is used twice in the repo. Worth renumbering one of them.cua-driver's synthesized clicks don't land in this Flutter app, so the real-app pass verifies appearance only. Interaction is covered by widget tests and the macOS integration test.Note
High Risk
Touches forge identity, per-repo path/provider configuration, and settings write paths that drive daemon filesystem and PR routing behaviour. Loopback gating and path validation are security-sensitive.
Overview
Adds per-repository Settings (SPEC-48) and Forgejo/Gitea forge identity in the desktop app.
Settings becomes dynamic:
sectionsFor(repos)appends one searchable section per pinned repo (repo:<id>), each led by aRepoMonogram. The window and nav pane now resolve against the live repo list instead of the static taxonomy.Each section is editable end-to-end via
RepositorySettingsPage→repo.settings.set/repo.path.set:Auto | None | Forgejo | Gitea | GitHub, with detection readout and overrideWrites surface server refusals through StatusCenter. Non-editable (non-loopback) clients see the same values read-only.
Forge branding: vendored Forgejo/Gitea SVGs and
forgeKindForUrlso PR "Open on …" buttons name the correct forge instead of always saying GitHub. Unidentifiable hosts stay unnamed rather than guessed.Reviewed by Cursor Bugbot for commit d4ce4cb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation
Note
Add Forgejo and Gitea as configurable per-repository git providers
repo.settings.setWebSocket command (loopback-only) for persisting per-repo settings includingprovider,worktreeRoot,defaultBranch, andlogoHue; stored inprojects.jsonas an opaque key-value bag.refs/pull/<n>/headand creating a PR-unique local branch; GitHub checkout viaghis preserved by default.RepoMonogramavatar, forge glyph, and editing controls for worktree root, provider, and default branch.resolveDefaultBranchprefers a stored override when the branch still resolves locally or asorigin/<branch>, falling back to detection.onProjectsChangedbroadcast now fires on every settings write, which re-sends both projects and repos snapshots to all connected clients.Macroscope summarized d4ce4cb.