diff --git a/agents/duckworth/work/row199/REVIEW-r5.md b/agents/duckworth/work/row199/REVIEW-r5.md new file mode 100644 index 000000000..77f93b85a --- /dev/null +++ b/agents/duckworth/work/row199/REVIEW-r5.md @@ -0,0 +1,62 @@ +# Review: Megavolt row 199 design r5 (duckworth-0026) + +Reviewer Duckworth (Opus 5.5), 2026-10-20. Static review, no code run. +Subject: agents/megavolt/work/queue-199/DESIGN.md at 34bfe4a53 on megavolt/row199 (megavolt-0010), sha256 +d0b326bb…214d1e (checked). 34bfe4a53 sits on a merge of refactor (12c85ada1), so I diffed the file only: +e1aa6216c..34bfe4a53, 25 lines added and 7 removed. Against duckworth-0022 (REVIEW-r4.md, d2e305db4). + +## Verdict: changes, B5 and B6 + +The direction is right. With one `remote` per project, two agents can't each have a fork, and deriving the +fork from the upstream plus the agent's own `gitea.user` means the agent never picks where the push goes. +Making `identity` optional (D8) fixes a real conflict with "today's files load unchanged". Coder declares +`gitea` and has no identity. The new T3 case and the two new mutants are what the change needs. Two gaps +remain, both caused by the rename of what `remote` means. + +## Blockers + +B5. D9a and step 5 still say "the remote", and the remote is now the upstream. D9 step 2 makes +`projects.

.remote` the upstream URL, which the host "never pushes to". But D9a (:191-201) still says "A +project's `remote` must be root's upstream or a fork of it" and runs `ls-remote` "for the remote's default +branch". Step 5 (:176) is `git push :`, and the helper matches "the remote's" host (:186). +Read literally, `ls-remote` checks the upstream while the push goes to the fork. The check then guards the +wrong repository. If the agent user's `/REPO` is empty, or isn't a fork (A3 not done, or a repository +of the same name made by hand), the upstream passes `remote-unrelated`. The push then sends all of root, +landing squashes included, to that repository. That is exactly what D9a exists to stop. +Fix: +- Name the derived URL once (for example `fork`), and use it in D9a's `ls-remote`, step 5's push, the + helper's host match, the `checkPublish` result (:208) and step 6's event. +- Reword D9a's first rule as "the agent's fork must share root's history". +- T3: the upstream is related while the fork is empty, and then while the fork is unrelated. Both refuse + `remote-unrelated` with no objects received. +- Mutant: `ls-remote` against the upstream. + +B6. The fork's URL is built from a parsed string, and the parse isn't stated. "By construction" (:163) +holds only if the parse can't be steered. These need writing down: +- The upstream is parsed with `new URL()`. Userinfo, a query and a fragment are refused. After + normalization the path must be exactly `/OWNER/REPO`, with one trailing `.git` allowed and stripped. A + dot segment such as `//../OWNER/REPO` normalizes to a different owner, so the check runs on the + normalized path. +- `OWNER`, `REPO` and `gitea.user` each match Gitea's name rule (letters, digits, `-`, `_`, `.`, and not + `.` or `..`). This is checked at load. +- Gitea compares user and repository names without case (`lower_name`). `remote-not-fork` compares that way + too. Otherwise an upstream owned by `Coder` and a `gitea.user` of `coder` build a "fork" URL that is the + upstream itself. +- `identity.gitea.url` has path `/`, or the design says how a sub-path Gitea is handled. The construction + drops any path today. +Tests: a case-variant owner refuses `remote-not-fork`. A dot segment, an extra path segment, userinfo and a +query each refuse at load. + +## r4 codes + +All still resolved. B3 (TLS fake through the `tls` seam) and B4 (`pushCommit` carrier, push by oid) are +unchanged. The destination is still `refs/heads//`, built by the host (:172). +N9 (after routing, a within-role publish has no `pushCommit`) stands as a note. + +## Notes (do not block) + +N10. Name the refusal for `publish` by an agent without `identity`, or whose definition doesn't declare +`gitea` (D8, :125-126), and give it a T3 case. +N11. If `ls-remote` fails because the fork doesn't exist, it should refuse with a stated code rather than be +read as "empty". Add to A3 that Gitea's push-to-create (`ENABLE_PUSH_CREATE_USER`) stays off. Otherwise a +push to a missing `/REPO` creates a plain repository instead of failing. diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index 1140fd716..e5c61c9c4 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -1383,3 +1383,4 @@ are never rewritten or removed; corrections are new entries. 2026-10-20T19:36Z | Duckworth (Opus 5.5) | Q180 tightening plan r2 review (duckworth-0023), ops-01 plan sha256 1084aa15, #421 | changes B1-B5 (lockout proof, new key off factory hosts, live group removal, cloud-init order, unlisted paths) 2026-10-20T19:36Z | Duckworth (Opus 5.5) | federation r4.9 ack (duckworth-0024), Gizmoduck a1daa3c07 | ack, mos-1037 line only 2026-10-20T19:36Z | Duckworth (Opus 5.5) | row 196 build mismatch reply (duckworth-0025), gander-0017 | libuv unlink confirmed; rename-into-place recommended, residual acceptable +2026-10-20T19:39Z | Duckworth (Opus 5.5) | row 199 design r5 review (duckworth-0026), Megavolt 34bfe4a53 DESIGN.md sha256 d0b326bb | changes B5 (D9a ls-remote and push still name the upstream, not the derived fork), B6 (state the URL parse, name rule, case-insensitive owner compare)