Conversation
|
Thanks for picking this up — and for the thorough PR description, it made reviewing much easier. The entry point you chose is right: I built your branch and ran it against a I want to start with a scope question, because it affects whether the rest is worth polishing yet. 1. Cross-fork delivery — I don't think the panel switch can land on its ownOnce the panel points at
So on its own, this ships a panel with a button that is broken for its target audience, which reads as misleading rather than incomplete. I'd rather we cover delivery before this goes out. The good news is that most of the plumbing is already there, and your remote picker is what supplies the missing piece: once the folder has a selected remote,
Given (3), one reasonable split is GitHub/Gitea first, with GitLab's cross-project MR either following in the same PR or explicitly gated (refuse the trigger with a clear message rather than failing at push time). Happy to discuss the split — and if you'd rather keep this PR to the panel and do delivery in a second one, that works too, as long as they land together. 2. Blocking — a write carries no repository, so it follows whatever is storedThe same comment request, sent twice, with only the stored setting changed in between:
Nothing in the request names a repository, and there's no broadcast when the selection changes. So any client holding a stale view — a second window, or a browser tab against the same server — posts, closes, files or merges into whatever repository the selection now names while its own header still shows the other one. Affected: The fix already exists in this file: Suggested fix: give the four write commands an 3. Storing the choice in
|
…lone Switching the remote re-points every request at another repository, so everything that was only true of the previous one has to go with it. The page number and the label selection keyed on the folder, which such a switch does not change; the list and count scopes carried no repository at all, so a response already in flight could land under the unchanged scope and paint the rows of the repository the reader had left; and the detail sheet asked for the signed-in account and the permitted merge methods by folder alone. All four now key on the folder AND the remote, derived during render so the reset is committed in the same pass that resolves the new remote — before any effect can fetch against it. A switch also claims a request generation of its own, which is what keeps a stale answer from being believed in the two states where no refetch is fired at all: a remote that resolves to nothing readable, and the frame between the teardown and the resolution. Both sides spell the repository the same way: the page hands the detail sheet the `server_host/owner_repo` string its own scopes are built from.
…not drop The picker's choice lived as a field of the panel-settings blob, and that blob is saved WHOLESALE by the trigger dialog — "use the global defaults" saves by dropping a folder's whole row, so the choice went with it, silently. The picker's own save had the mirror-image problem: it spread the settings in force and wrote them back, which detached the folder from the global row from the first pick on, and made every click a read-modify-write race against the dialog. The selection now lives under its own app_metadata key, one entry per folder and no global row: a remote names a git remote that exists in ONE folder's worktree, so there is nothing for an "all folders" default to mean. A folder with no entry reads the historical `origin`, and a blank or absent name clears the entry — absence IS the default answer. The picker gets its own commands (`forge_remote_get` / `forge_remote_set`, in both modes) and an explicit "Default (origin)" item, which is now the only way back to no choice: the settings dialog does not edit this at all, so without that item a folder that had picked a remote could never go back to the default.
The four write commands carried a folder id and nothing else, so a write followed whatever remote the selection named by the time it arrived: a second window, a browser tab or an old build holding a stale view could post, close, file or merge into a repository other than the one its header showed, and nothing reported it — the request was valid for the folder it named. Every write now carries the coordinates the panel is showing, flattened beside its own fields and optional, so a build that predates the check keeps working. The resolution compares them with what the folder's remote actually resolves to BEFORE spending a credential, and refuses a mismatch with an i18n key the panel recognises. The panel's answer is to re-resolve the repository rather than only report the refusal: the panel is stale, and re-resolving is the one thing that fixes it. Reads deliberately do not carry them: a stale read shows a wrong list rather than writing into the wrong place, and the cost of adding them is every read request struct and every frontend call site.
Triggering a task from an issue on the parent — which is exactly what the picker
is for — pushed the work branch to the parent too, where the contributor who
owns the fork has no write access. The task failed at delivery, after the agent
had done the work; the same happened to a review of someone else's pull request.
The picker is what makes both halves expressible: what the panel reads is the
source, and the folder's own `origin` is the copy codeg can write to.
The fork is recorded at trigger time and every delivery step reads it.
`delivery_push_repo` answers where the work lands for every shape of delivery (a
pull request pushes back to its own head, an issue task to the recorded fork,
the source otherwise), the push uses it, and a new pull request goes out
cross-repository — as `owner:branch` on GitHub and Gitea, and on GitLab as a
request ADDRESSED to the fork with the target named in its body, which is what
that API accepts: GitLab resolves `source_branch` in the project the request is
sent to, so naming the fork while addressing the target is answered
"source_branch does not exist".
The claim path is the half a single delivery cannot see: the search for an
existing pull request, the four-way match and `check_pull_target` all judged the
head by the SOURCE, so a retry opened a duplicate — which the forge answers with
a 422, after the work has been pushed. The list is the source's either way (a
pull request lives where it is merged into); the head's repository is what the
match compares against and what GitHub's `head={owner}:{branch}` pre-filter
names. GitLab's list payload, which names a foreign source project by number
alone (`project-{id}`), is resolved back to the fork's path before anything
compares it — keyed on the id the task recorded, so another fork's merge request
still matches nothing.
GitLab's two project ids — the fork its merge requests are created on, and the
target they are aimed at — are resolved at the trigger, and a task whose fork or
target cannot be read is refused there, where the user can still choose
something else, rather than after the agent's work has nowhere to go.
A review from somebody else's fork is delivered to that fork rather than refused
up front: whether this account may write there is a server-side fact (the
author's "allow edits from maintainers"), so the push is what decides — which is
also the only way a maintainer can push a fix into a contributor's review. A
refusal there names the way out, and the task keeps its commits either way.
|
Thanks for the detailed review. I’ve addressed the four areas you identified, including cross-fork delivery, stale-write protection, repository-scoped frontend state, and moving remote persistence to independent per-folder keys to avoid the shared-blob race. The latest CI matrix is fully green. Re-review requested when you have time. @xintaofei |
3 participants
The repository panel previously always read
origin, so a forked checkout (gh repo forkleavesorigin= your fork andupstream= the parent) could only show the fork's own issues and pull requests.This PR adds a remote picker and completes the fork workflow end to end.
What changed
Remote selection
origin.folder_forge_remote_core, so forge operations follow the repository shown in the panel.ForgeRemote.remote_nameechoes the resolved remote so the UI can show what is in force.ForgePanelSettings.Each folder persists its selection independently:
forge_panel_remote:<folder_id>The frontend still receives an aggregated
ForgeRemoteStore, but that store is assembled from independent rows at read time rather than persisted as one shared JSON blob. This avoids both the settings side effects and cross-folder read-modify-write races noted in the previous review.Safe writes from stale clients
Comment, close/reopen, new-issue and merge writes carry the repository coordinates the client is showing.
The backend compares those coordinates with the folder's currently selected remote before resolving the write. If another window changed the selection, the write is refused rather than silently redirected to another repository, and the UI re-resolves the panel.
Older clients that send no expected coordinates retain the previous behavior.
Cross-fork task delivery
For the fork workflow (
origin= contributor fork, selectedupstream= parent):originis recorded as the writable fork at trigger time;owner:branchhead;Remote-switch state
Changing the selected remote invalidates repository-scoped frontend state:
Closes #743.
Validation
Added regression coverage for the issues identified in the previous review, including:
Latest CI on
98372ce5passes the complete matrix:All checks