From 4d51c1304d9316841c30e31c32a2da258eaedf83 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 4 Oct 2026 22:09:21 -0500 Subject: [PATCH] docs(slice1): row 35 SR round 1 review record (darkwing) Verdict changes, #1517 comment 26709. Gitea 1.27.1 and Vikunja 2.7.0 source checks for Sage's three questions; two mint defects. Co-Authored-By: Claude Opus 5.5 --- .../work/slice1-sr-review-r1-2026-10-04.md | 181 ++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 agents/darkwing/work/slice1-sr-review-r1-2026-10-04.md diff --git a/agents/darkwing/work/slice1-sr-review-r1-2026-10-04.md b/agents/darkwing/work/slice1-sr-review-r1-2026-10-04.md new file mode 100644 index 00000000..021d802d --- /dev/null +++ b/agents/darkwing/work/slice1-sr-review-r1-2026-10-04.md @@ -0,0 +1,181 @@ +# Queue row 35 (SR), round 1 review (#1517) + +Darkwing, 2026-10-04. Request: #1517 comment 26707. Candidate: commit +c9c1699a, `docs/guides/slice-1-identities.md`. Brief: +`docs/plans/2026-10-04_slice-1.md`, row SR. + +Verdict: changes. The scope tables are right, and so are Sage's three +assumptions, with one correction to the push claim. Two command defects +need fixing before Jason runs the guide: the `mint` error guidance can't +be seen, and a missing token field writes the word `null` into a token +file that then passes the section 4 check. The rest are small text +changes. + +I read the source and didn't run anything live. Gitea's version endpoint +on our instance reports 1.27.1, so I read tag v1.27.1. Vikunja is tag +v2.7.0, the version the guide pins. Line numbers below are from those +tags. + +## Sage's three questions + +### 1. Reviewer-bot: `write:repository` plus Read access + +The scope half is right. In `routers/api/v1/api.go` the `/repos` group +that holds `POST /pulls`, `POST /pulls/{index}/reviews` and +`POST .../reviews/{id}` closes at line 1548 with +`tokenRequiresScopes(AccessTokenScopeCategoryRepository)`. Issue +comments are in the group closing at line 1684 with the issue category. +`tokenRequiresScopes` (line 323) asks for the write level on POST, PUT, +PATCH and DELETE. So a review needs `write:repository`, and a comment +needs `write:issue`. + +A Read collaborator can create and submit all three review types. The +`/pulls` group applies `mustAllowPulls`, `reqRepoReader(unit.TypeCode)` +and `reqToken()`, and no writer check. The handler refuses only an +approval or rejection of your own PR (`pull_review.go:688-699`). + +The push half needs a correction. A branch or tag push is refused, but +not by the HTTP permission check. For receive-pack, +`routers/web/repo/githttp.go` drops the required access to Read when git +supports proc-receive (lines 184-187). The refusal comes later, in the +pre-receive hook: `assertCanWriteRef` in +`routers/private/hook_pre_receive.go` needs code write and answers 403. +The exception is `refs/for/`, the AGit flow. It needs only read +access to pull requests (`CanCreatePullRequest`, line 88). With this +token, reviewer-bot can push to `refs/for/next` and open a pull request +with any content it likes. + +That doesn't break slice 1, because the broker holds the token and has +no action that runs a git push for the reviewer role. But "Read access +is what stops it pushing" overstates it. Suggested text for lines 69-72: + +> Reviewer-bot's `write:repository` is there because Gitea checks pull +> request reviews against the repository scope. Its Read collaborator +> access stops pushes to branches and tags. It does not stop an AGit +> push to `refs/for/`, which opens a pull request. That's +> acceptable only because the broker holds the token and offers the +> reviewer no push action. + +One more thing worth a line. Gitea counts a review toward required +approvals only when it's official, and a Read user's review isn't +official by default (`IsOfficialReviewer`, `models/issues/review.go:276`; +`IsUserOfficialReviewer`, `models/git/protected_branch.go:234`). The +exception is a protected branch with the approvals whitelist turned on +and reviewer-bot on it. If a protection rule on `next` requires +approvals and reviewer-bot's verdict should count, the guide has to say +to whitelist it. If merges stay Jason's and approvals are advisory, say +that instead. + +### 2. Vikunja `/api/v2/login` returns `token` + +Confirmed. `pkg/routes/api/v2/auth_login.go`, `authLogin` (lines +87-106), returns the body type `authTokenBody` (line 43), whose JWT +field is `token`. The body also carries a `$schema` link, because +`huma.go:74` uses `huma.DefaultConfig`. The script ignores extra keys, +so that's harmless. The route exists only when local or LDAP login is +enabled (line 63), which section 2 already requires. Keep the stop for a +missing field, but replace "assumed from v1" at line 138 with "confirmed +in the v2.7.0 source (`auth_login.go`)". + +### 3. Bot names `bot--` + +Keep them. Vikunja usernames are global to the instance, and the guide's +reason (two businesses on one instance) holds. Amend the PRD's REQ-CRED-1 +wording from `bot-` to match. The `bot-` prefix itself is +Vikunja's rule for bot accounts. + +The Gitea bot names have the same problem and the guide doesn't address +it. `pm-bot`, `cto-bot`, `coder-bot` and `reviewer-bot` are global +Gitea users, so a second business on the same Gitea can't reuse them. +Either name them `-pm-bot` and so on, or state that one set of +Gitea bots serves every business on the instance. I'd take the first; +it keeps a business's revocation from touching another business. + +## Defects in the commands + +### D1. `mint` hides the error it tells you to read (lines 164, 216, 228-230) + +`api` uses `curl -sf`. With `-f`, an HTTP error status makes curl exit +22 and write nothing, so the 400 body with code 14002 never appears. +And `rm -f "$resp"` runs after the failed chain, so even +`--fail-with-body` would lose it. Suggested `mint` body: + +```sh +mint() { # mint ROLE BOT_ID SCOPES_FILE + local r=$1 id=$2 sc=$3 resp="$S/.mint-$1.json" + jq -n --arg t "$BIZ-$r-$(date +%F)" --argjson o "$id" --arg e "$EXP" --slurpfile p "$sc" \ + '{title:$t, owner_id:$o, expires_at:$e, permissions:$p[0]}' | + curl -s --fail-with-body -H @"$S/vikunja-owner.hdr" -H 'Content-Type: application/json' \ + -X POST "$VK/api/v2/tokens" --data-binary @- -o "$resp" || + { jq -c '{code, message}' "$resp"; rm -f "$resp"; return 1; } + jq -je '.token | strings' "$resp" > "$S/$r-vikunja.token" && + jq -c '{id, owner_id, expires_at, starts_tk: (.token | startswith("tk_"))}' "$resp" + rm -f "$resp" +} +``` + +An error body carries no token, so printing `code` and `message` is +safe. `--fail-with-body` needs curl 7.76 or later. + +### D2. A missing `token` field writes `null` (line 217) + +`jq -j .token` on a body without `token` prints `null` and exits 0. I +checked: `echo '{}' | jq -j .token` gives `null`, exit 0. The file is +then 0600 with size 4, and section 4's "600 and a nonzero size" passes. +`jq -je '.token | strings'` prints nothing and exits 4, so the chain +stops. That's the change in D1. + +## Smaller changes + +1. Gitea tokens, step 4 (line 73). "Copy with an editor" can leave a + swap or backup file next to the token, in `$S` or wherever the editor + keeps them. Suggest instead, in the same `umask 077` shell: + `read -rs t && printf %s "$t" > "$S/pm-gitea.token"; unset t`. + `read` and `printf` are builtins, so the value doesn't reach `ps` or + the history. Do the same for the Gitea rotation in section 5. +2. Trailing newlines. That step also says "one line". An editor adds a + newline, `printf %s` doesn't. Row S3's broker will trim one trailing + newline either way; I'll put that in S3. +3. `EXP` is a timestamp (line 32), and the business file's `expires` is + `YYYY-MM-DD` (row S1 validates that). Say to write the date part of + `EXP`. The broker treats 00:00Z on that date as the expiry, which is + the same instant `EXP` names. +4. The labels template. Section 3 step 4 refers to "the labels the + business file template lists". Row SR's brief owns + `templates/business/mosaic-stack.example.json`, and it isn't in the + candidate. Either add it or list the labels in the guide. S1 will + send the business file example the template should follow. +5. Path B (lines 113-115). The binary is `/app/vikunja/vikunja`, as the + guide says (Dockerfile lines 49-50). `user create` without `-p` + prompts through `term.ReadPassword` (`pkg/cmd/user.go:151`). Without + a TTY it exits through `log.Fatalf`, so the guide's `-it` matters. + Replace "If your build doesn't prompt, stop" with "Keep `-it`; without + a terminal the command exits instead of prompting." +6. Path B, `--user "$(id -u):$(id -g)"`. Right. The image runs as uid + 1000, but nothing at `/db` or `/app/vikunja/files` has to belong to + 1000. Both mounts are required, because `/app/vikunja` itself isn't + writable to another uid and Vikunja writes a test file into the files + directory at startup. Worth one sentence so nobody drops a mount. +7. Rotation, Vikunja. The owner can delete a bot's token: the v2.7.0 + route's description says so, and `APIToken.CanDelete` + (`pkg/models/api_tokens_permissions.go:25`) checks for it. The + rotation step logs in again, which recreates + `$S/vikunja-owner.hdr`, and `api` and `mint` were defined in another + shell. Say to rerun section 0, the owner login and the two function + definitions, and to finish with section 4's `rm -f`. + +## What I checked and found right + +- The scope JSON in section 3 matches addendum B section 2 byte for + byte: sync, pm and the shared worker file. +- Shares: role bots write (1), sync read (0). +- The secret handling: `umask 077` up front, the service secret through + `printf` and command substitution, the owner password through + `getpass`, the header passed as `-H @file`, the header file deleted in + section 4, and `stat` rather than `cat`. Nine token files is right: + four Gitea, five Vikunja. +- The Gitea scope table. pm-bot needs only `read:repository`, since its + labels, assignees and closes go through issue routes. The three + workers need `write:repository` for reviews and pull requests. +- The revocation paths in section 5 end in a 401 or 403, which the + broker refuses rather than retries.