From ce90fa95f07aec0fc88cde7dea2c268bd7765a10 Mon Sep 17 00:00:00 2001 From: pseudo Date: Mon, 6 Jul 2026 13:06:00 -0600 Subject: [PATCH 01/14] chore(porch): bugfix-1137 init bugfix --- .../status.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml diff --git a/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml b/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml new file mode 100644 index 000000000..c6418ea48 --- /dev/null +++ b/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml @@ -0,0 +1,14 @@ +id: bugfix-1137 +title: gitea-forge-preset-is-broken-a +protocol: bugfix +phase: investigate +plan_phases: [] +current_plan_phase: null +gates: + merge-approval: + status: pending +iteration: 1 +build_complete: false +history: [] +started_at: '2026-07-06T19:06:00.466Z' +updated_at: '2026-07-06T19:06:00.469Z' From 8f2ada66c73edc1e38099092411d432e280521e0 Mon Sep 17 00:00:00 2001 From: pseudo Date: Mon, 6 Jul 2026 13:12:31 -0600 Subject: [PATCH 02/14] chore(porch): bugfix-1137 fix phase-transition --- .../bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml b/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml index c6418ea48..f1b31df65 100644 --- a/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml +++ b/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml @@ -1,7 +1,7 @@ id: bugfix-1137 title: gitea-forge-preset-is-broken-a protocol: bugfix -phase: investigate +phase: fix plan_phases: [] current_plan_phase: null gates: @@ -11,4 +11,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-07-06T19:06:00.466Z' -updated_at: '2026-07-06T19:06:00.469Z' +updated_at: '2026-07-06T19:12:31.039Z' From 362071315b52639335129ea380c0d9d56546cbb8 Mon Sep 17 00:00:00 2001 From: pseudo Date: Mon, 6 Jul 2026 13:30:45 -0600 Subject: [PATCH 03/14] [Bugfix #1137] Fix: Route gitea forge reads through `tea api` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gitea preset invoked `tea list/view/whoami/comment`, whose flattened `--fields` output (or missing flags/subcommands) doesn't match the Gitea REST shape that forge-contracts.ts and the jq normalizers assume. Route the read concepts through `tea api`, the raw REST passthrough that returns exactly that shape: - user-identity: `tea api user | jq .login` (`tea whoami` has no --output json) - pr-view: `tea api repos//pulls/N` → PrViewResult - pr-list: `tea api repos//pulls?state=open` → PrListItem[] (now also populates real reviewRequests/isDraft/body) - pr-exists: `tea api repos//pulls?state=all` with nested .head.ref/.merged - issue-view: `tea api repos//issues/N` + a second call for the comments ARRAY (Gitea's issue object reports `comments` as an int count, which would crash consumers' `.comments.filter(...)`) - recently-merged: `tea api repos//pulls?state=closed`, filter .merged, using the real .merged_at - issue-comment: `tea comments add` (`tea issues` has no `comment` subcommand) `tea api` needs an explicit owner/repo path segment (unlike `tea `, which auto-detects it from the local git remote), and most concepts are invoked without CODEV_REPO set, so each api-based script derives owner/repo from the origin remote, honoring CODEV_REPO when present. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../scripts/forge/gitea/issue-comment.sh | 7 ++- .../codev/scripts/forge/gitea/issue-view.sh | 37 ++++++++++-- .../codev/scripts/forge/gitea/pr-exists.sh | 26 ++++++-- packages/codev/scripts/forge/gitea/pr-list.sh | 59 +++++++++---------- packages/codev/scripts/forge/gitea/pr-view.sh | 31 ++++++++-- .../scripts/forge/gitea/recently-merged.sh | 38 ++++++------ .../scripts/forge/gitea/user-identity.sh | 8 ++- 7 files changed, 142 insertions(+), 64 deletions(-) diff --git a/packages/codev/scripts/forge/gitea/issue-comment.sh b/packages/codev/scripts/forge/gitea/issue-comment.sh index bea4e8316..addf6ccc0 100755 --- a/packages/codev/scripts/forge/gitea/issue-comment.sh +++ b/packages/codev/scripts/forge/gitea/issue-comment.sh @@ -1,3 +1,8 @@ #!/bin/sh # Forge concept: issue-comment (Gitea via tea CLI) -exec tea issues comment "$CODEV_ISSUE_ID" "$CODEV_COMMENT_BODY" +# Input: CODEV_ISSUE_ID, CODEV_COMMENT_BODY +# Output: exit code only +# +# `tea issues` has no `comment` subcommand (its subcommands are list/create/ +# edit/close). Commenting lives under the top-level `tea comments add`. +exec tea comments add "$CODEV_ISSUE_ID" "$CODEV_COMMENT_BODY" diff --git a/packages/codev/scripts/forge/gitea/issue-view.sh b/packages/codev/scripts/forge/gitea/issue-view.sh index db53b03e4..f2ead28e1 100755 --- a/packages/codev/scripts/forge/gitea/issue-view.sh +++ b/packages/codev/scripts/forge/gitea/issue-view.sh @@ -1,6 +1,35 @@ #!/bin/sh # Forge concept: issue-view (Gitea via tea CLI) -# Sets `url` to the issue's browser page (`html_url`). Gitea's own `url` field is -# the API endpoint (would render raw JSON in a browser), so we prefer `html_url` -# and fall back to the existing `url` only if `html_url` is absent. -tea issues view "$CODEV_ISSUE_ID" --output json | jq '.url = (.html_url // .url)' +# Input: CODEV_ISSUE_ID +# Output: JSON {title, body, state, url, comments[]} (IssueViewResult) +# +# `tea issues view N --output json` returns a flattened single-element list +# (no body/html_url/url), so route through the raw REST passthrough. `tea api` +# needs an explicit owner/repo in the path (unlike `tea issues`, which +# auto-detects it from the local git remote), so resolve it here: honor +# CODEV_REPO when set, else derive owner/repo from origin's URL (handles +# https, ssh, and scp-style remotes, with or without a .git suffix). +# +# `url` is mapped to the issue's browser page (`html_url`); Gitea's own `url` +# is the API endpoint (would render raw JSON in a browser), so we fall back to +# it only if `html_url` is absent. +# +# Gitea's issue object reports `comments` as an integer count, not the array +# the contract requires (consumers call `.comments.filter(...)`), so the +# comments array is fetched separately and merged in. A failed/empty comments +# fetch degrades to []. +REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" +COMMENTS_JSON="$(tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}/comments" 2>/dev/null)" +[ -n "$COMMENTS_JSON" ] || COMMENTS_JSON="[]" +tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}" \ + | jq --argjson comments "$COMMENTS_JSON" '{ + title, + body: (.body // ""), + state, + url: (.html_url // .url), + comments: [ $comments[] | { + body: (.body // ""), + createdAt: .created_at, + author: {login: .user.login} + } ] + }' diff --git a/packages/codev/scripts/forge/gitea/pr-exists.sh b/packages/codev/scripts/forge/gitea/pr-exists.sh index db8365012..d8d8c9b0d 100755 --- a/packages/codev/scripts/forge/gitea/pr-exists.sh +++ b/packages/codev/scripts/forge/gitea/pr-exists.sh @@ -1,6 +1,24 @@ #!/bin/sh # Forge concept: pr-exists (Gitea via tea CLI) -# Returns true for open or merged pulls only. Closed-not-merged pulls are excluded. -# --state all fetches pulls in all states; without it, only open pulls are returned. -# Gitea: merged PRs have state="closed" + merged=true; abandoned PRs have state="closed" + merged=false -tea pulls list --state all --fields index --output json | jq "[.[] | select(.head.ref == \"$CODEV_BRANCH_NAME\" and (.state == \"open\" or (.state == \"closed\" and .merged == true)))] | length > 0" +# Input: CODEV_BRANCH_NAME +# Output: "true" or "false" +# +# Returns true for OPEN or MERGED pulls only; closed-not-merged pulls are +# excluded. `tea pulls list` emits `.head` as a string (not `{ref}`) and reports +# merged PRs as state "merged" with no `.merged` boolean, so its output can't +# satisfy the `.head.ref` / `.merged` predicate below. Route through the raw +# REST passthrough, whose PR objects carry nested `.head.ref` and a `.merged` +# bool. `tea api` needs an explicit owner/repo in the path (unlike `tea pulls`, +# which auto-detects it from the local git remote), so resolve it here: honor +# CODEV_REPO when set, else derive owner/repo from origin's URL (handles https, +# ssh, and scp-style remotes, with or without a .git suffix). +# +# Caveat (Gitea behavior, not a codev bug): for a merged PR whose source branch +# was deleted, Gitea returns `.head.ref == "refs/pull/N/head"` instead of the +# original branch name, so a branch-name match won't hit a merged+deleted +# branch. That doesn't affect the "does an open/merged PR exist for the branch +# I'm about to push" use case. +REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" +tea api "repos/${REPO}/pulls?state=all&limit=200" \ + | jq --arg branch "$CODEV_BRANCH_NAME" \ + '[.[] | select(.head.ref == $branch and (.state == "open" or .merged == true))] | length > 0' diff --git a/packages/codev/scripts/forge/gitea/pr-list.sh b/packages/codev/scripts/forge/gitea/pr-list.sh index d320c5cc2..a6deac289 100755 --- a/packages/codev/scripts/forge/gitea/pr-list.sh +++ b/packages/codev/scripts/forge/gitea/pr-list.sh @@ -1,36 +1,35 @@ #!/bin/sh -# Forge concept: pr-list (Gitea via tea CLI) +# Forge concept: pr-list (Gitea via tea CLI) — open pulls +# Output: JSON [{number, title, url, reviewDecision, body, createdAt, author, +# reviewRequests, isDraft}] (PrListItem in forge-contracts.ts) # -# Normalize tea's PR shape to the GitHub-compatible shape codev expects -# (see PrListItem in codev/src/lib/forge-contracts.ts): -# index -> number (int) -# description -> body -# created -> createdAt -# author (string) -> author.login -# reviewDecision -> "" (Gitea has no GitHub-equivalent review-decision summary) -# reviewRequests -> [] (verified against tea 0.14.1: `pulls list` exposes -# no `reviewers` field, and its JSON output is limited -# to the selectable `--fields`, so requested reviewers -# are unreachable here. The VSCode sort silently skips -# the review-requested bucket when empty.) -# isDraft -> false (verified: tea 0.14.1 `pulls list` exposes no -# `draft` field among its selectable `--fields`.) -# The underlying Gitea API PR object does carry `draft` and `requested_reviewers`, -# but only the raw `tea api` passthrough can reach them — populating these two -# fields for Gitea would mean reworking this concept onto `tea api`, which is a -# separate, larger change than #787's scope. -exec tea pulls list --limit 200 \ - --fields index,title,state,author,url,created,description \ - --output json \ +# `tea pulls list --fields …,description` errors ("invalid field 'description'") +# and its flattened output can't carry a PR body, draft flag, or requested +# reviewers. Route through the raw REST passthrough instead, whose PR objects +# expose all of them. `tea api` needs an explicit owner/repo in the path (unlike +# `tea pulls`, which auto-detects it from the local git remote), so resolve it +# here: honor CODEV_REPO when set, else derive owner/repo from origin's URL +# (handles https, ssh, and scp-style remotes, with or without a .git suffix). +# +# Field mapping: +# .number -> number (already an int in the REST shape) +# .html_url -> url (browser page; Gitea `.url` is the API endpoint) +# .body -> body +# .created_at -> createdAt +# .user.login -> author.login +# .requested_reviewers[].login -> reviewRequests (user logins; teams have no login → dropped) +# .draft -> isDraft +# reviewDecision -> "" (Gitea has no GitHub-equivalent review-decision summary) +REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" +tea api "repos/${REPO}/pulls?state=open&limit=200" \ | jq '[.[] | { - number: (.index | tonumber), + number, title, - state, - url, + url: (.html_url // .url), reviewDecision: "", - body: (.description // ""), - createdAt: .created, - author: {login: .author}, - reviewRequests: [], - isDraft: false + body: (.body // ""), + createdAt: .created_at, + author: {login: .user.login}, + reviewRequests: [ (.requested_reviewers // [])[] | .login // empty ], + isDraft: (.draft // false) }]' diff --git a/packages/codev/scripts/forge/gitea/pr-view.sh b/packages/codev/scripts/forge/gitea/pr-view.sh index ffcc63acf..10c13149f 100755 --- a/packages/codev/scripts/forge/gitea/pr-view.sh +++ b/packages/codev/scripts/forge/gitea/pr-view.sh @@ -1,6 +1,29 @@ #!/bin/sh # Forge concept: pr-view (Gitea via tea CLI) -# Sets `url` to the PR's browser page (`html_url`). Gitea's own `url` field is -# the API endpoint (would render raw JSON in a browser), so we prefer `html_url` -# and fall back to the existing `url` only if `html_url` is absent. -tea pulls view "$CODEV_PR_NUMBER" --output json | jq '.url = (.html_url // .url)' +# Input: CODEV_PR_NUMBER +# Output: JSON {title, body, state, url, author{login}, baseRefName, headRefName, +# additions, deletions} (see PrViewResult in forge-contracts.ts) +# +# `tea pulls view N --output json` returns a table header / empty list rather +# than the PR object, so route through the raw REST passthrough. `tea api` +# needs an explicit owner/repo in the path (unlike `tea pulls`, which +# auto-detects it from the local git remote), so resolve it here: honor +# CODEV_REPO when set, else derive owner/repo from origin's URL (handles +# https, ssh, and scp-style remotes, with or without a .git suffix). +# +# `url` is the PR's browser page (`html_url`). Gitea's own `url` field is the +# API endpoint (would render raw JSON in a browser), so map `html_url` and fall +# back to `url` only if it's absent — the same choice PIR #1179 made when this +# concept still went through `tea pulls view`. +REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" +tea api "repos/${REPO}/pulls/${CODEV_PR_NUMBER}" | jq '{ + title, + body: (.body // ""), + state, + url: (.html_url // .url), + author: {login: .user.login}, + baseRefName: .base.ref, + headRefName: .head.ref, + additions: (.additions // 0), + deletions: (.deletions // 0) +}' diff --git a/packages/codev/scripts/forge/gitea/recently-merged.sh b/packages/codev/scripts/forge/gitea/recently-merged.sh index 4c2d30955..3cd9e083c 100755 --- a/packages/codev/scripts/forge/gitea/recently-merged.sh +++ b/packages/codev/scripts/forge/gitea/recently-merged.sh @@ -1,27 +1,25 @@ #!/bin/sh # Forge concept: recently-merged (Gitea via tea CLI) +# Output: JSON [{number, title, url, body, createdAt, mergedAt, headRefName}] +# (MergedPrItem in forge-contracts.ts) # -# `tea pulls list --state closed` returns both merged PRs and closed-without- -# merge PRs. Filter to merged only via `.merged == true` (the same predicate -# scripts/forge/gitea/pr-exists.sh already relies on), then map to the -# GitHub-compatible shape: -# index -> number (int) -# created -> createdAt -# updated -> mergedAt (tea exposes no merged_at field via --fields; -# close-then-edit overestimates merged time -# but is acceptable for the 24h overview window) -# head.ref -> headRefName -# description -> body -exec tea pulls list --state closed --limit 1000 \ - --fields index,title,state,author,url,created,updated,head,description,merged \ - --output json \ +# `tea pulls list --fields …,head,description,merged` errors on the `description` +# field and emits `.head` as a string, so it can't populate `body` or +# `.head.ref`. Route through the raw REST passthrough instead, whose closed +# pulls carry `.merged`, `.merged_at`, nested `.head.ref`, and `.body`. Keep +# only merged pulls (closed-without-merge have `.merged == false`). `tea api` +# needs an explicit owner/repo in the path (unlike `tea pulls`, which +# auto-detects it from the local git remote), so resolve it here: honor +# CODEV_REPO when set, else derive owner/repo from origin's URL (handles https, +# ssh, and scp-style remotes, with or without a .git suffix). +REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" +tea api "repos/${REPO}/pulls?state=closed&limit=200" \ | jq '[.[] | select(.merged == true) | { - number: (.index | tonumber), + number, title, - state, - url, - body: (.description // ""), - createdAt: .created, - mergedAt: .updated, + url: (.html_url // .url), + body: (.body // ""), + createdAt: .created_at, + mergedAt: .merged_at, headRefName: (.head.ref // "") }]' diff --git a/packages/codev/scripts/forge/gitea/user-identity.sh b/packages/codev/scripts/forge/gitea/user-identity.sh index 2b8f78523..c296a3c71 100755 --- a/packages/codev/scripts/forge/gitea/user-identity.sh +++ b/packages/codev/scripts/forge/gitea/user-identity.sh @@ -1,3 +1,9 @@ #!/bin/sh # Forge concept: user-identity (Gitea via tea CLI) -tea whoami --output json | jq -r ".login" +# Output: plain text username +# +# `tea whoami` has no `--output json` flag (its only documented option is +# --help), so it can't feed a jq pipeline. Route through the raw REST +# passthrough instead: `tea api user` returns the Gitea `User` object, whose +# `.login` is the authenticated username (mirrors `gh api user --jq .login`). +tea api user | jq -r ".login" From b412335266db7e4922779f8a0859210b4981db68 Mon Sep 17 00:00:00 2001 From: pseudo Date: Mon, 6 Jul 2026 13:30:46 -0600 Subject: [PATCH 04/14] [Bugfix #1137] Test: Add gitea `tea api` regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stubs a fake `tea` on PATH answering `api ` with captured Gitea REST fixtures (tea isn't in CI, per #920), points the scripts at a throwaway repo with a gitea remote, runs each real script, and asserts the normalized output conforms to forge-contracts.ts — incl. comments-as-array, merged-only filtering, open/merged/closed pr-exists cases, and CODEV_REPO override. Also updates the #568 pr-exists assertion for gitea to match the new `state=all` query param (was `--state all` flag). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../bugfix-1137-gitea-tea-api.test.ts | 218 ++++++++++++++++++ .../bugfix-568-pr-exists-state-all.test.ts | 7 +- 2 files changed, 223 insertions(+), 2 deletions(-) create mode 100644 packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts diff --git a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts new file mode 100644 index 000000000..cc0e5b971 --- /dev/null +++ b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts @@ -0,0 +1,218 @@ +/** + * Regression test for bugfix #1137: the gitea forge preset was written against + * the Gitea REST API JSON shape but invoked the `tea` CLI's flattened + * ` list/view` output (or non-existent flags/subcommands), so every + * read concept either errored or emitted the wrong shape. + * + * The fix routes the read concepts through `tea api `, whose raw + * passthrough returns exactly the Gitea REST shape the jq normalizers and + * `forge-contracts.ts` already assume. + * + * `tea` isn't available in CI (see the in-repo #920 note), so this test stubs a + * fake `tea` on PATH that answers `api ` (and `comments add`) with + * captured Gitea REST fixtures, points the scripts at a throwaway git repo with + * a gitea remote, runs each real script, and asserts the normalized output + * conforms to the contract in forge-contracts.ts. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { execFileSync } from 'node:child_process'; + +const giteaDir = resolve(__dirname, '..', '..', 'scripts', 'forge', 'gitea'); + +// A fake `tea` binary. It only implements `api ` (the surface the +// fixed scripts use) plus `comments add`. Each endpoint returns the raw Gitea +// REST shape — nested objects, real `.merged`/`.merged_at`/`.draft`, integer +// `comments` count on the issue object, etc. +const FAKE_TEA = `#!/bin/sh +if [ "$1" = "comments" ] && [ "$2" = "add" ]; then + # comments add + echo "commented" + exit 0 +fi +[ "$1" = "api" ] || { echo "fake-tea: unsupported: $*" >&2; exit 3; } +case "$2" in + user) + echo '{"login":"octo","id":7}' ;; + repos/acme/widgets/pulls/42) + echo '{"number":42,"title":"Add widget","body":"PR body","state":"open","html_url":"https://git.example.com/acme/widgets/pulls/42","url":"https://git.example.com/api/v1/repos/acme/widgets/pulls/42","user":{"login":"alice"},"base":{"ref":"main"},"head":{"ref":"feature/x"},"additions":10,"deletions":3}' ;; + "repos/acme/widgets/pulls?state=open&limit=200") + echo '[{"number":42,"title":"Add widget","html_url":"https://git.example.com/acme/widgets/pulls/42","url":"https://git.example.com/api/v1/repos/acme/widgets/pulls/42","body":"PR body","state":"open","created_at":"2026-07-01T10:00:00Z","user":{"login":"alice"},"requested_reviewers":[{"login":"bob"},{"login":null}],"draft":true}]' ;; + "repos/acme/widgets/pulls?state=all&limit=200") + echo '[{"number":42,"state":"open","merged":false,"head":{"ref":"feature/x"}},{"number":40,"state":"closed","merged":true,"head":{"ref":"feature/done"}},{"number":39,"state":"closed","merged":false,"head":{"ref":"feature/abandoned"}}]' ;; + "repos/acme/widgets/pulls?state=closed&limit=200") + echo '[{"number":40,"title":"Done PR","html_url":"https://git.example.com/acme/widgets/pulls/40","body":"merged body","state":"closed","merged":true,"merged_at":"2026-07-05T12:00:00Z","created_at":"2026-07-02T09:00:00Z","head":{"ref":"feature/done"}},{"number":39,"title":"Abandoned","state":"closed","merged":false,"head":{"ref":"feature/abandoned"}}]' ;; + repos/acme/widgets/issues/99) + echo '{"number":99,"title":"Bug here","body":"issue body","state":"open","html_url":"https://git.example.com/acme/widgets/issues/99","url":"https://git.example.com/api/v1/repos/acme/widgets/issues/99","comments":2}' ;; + repos/acme/widgets/issues/99/comments) + echo '[{"body":"On it! Working on a fix now.","created_at":"2026-07-06T08:00:00Z","user":{"login":"carol"}},{"body":"second","created_at":"2026-07-06T09:00:00Z","user":{"login":"dave"}}]' ;; + *) echo "fake-tea: no fixture for: $2" >&2; exit 4 ;; +esac +`; + +let fixture: string; +let binDir: string; +let repoDir: string; +let runEnv: NodeJS.ProcessEnv; + +function hasJq(): boolean { + try { + execFileSync('jq', ['--version'], { stdio: 'ignore' }); + return true; + } catch { + return false; + } +} + +const jqAvailable = hasJq(); + +/** Run a gitea forge script under the fake `tea`, return trimmed stdout. */ +function runScript(name: string, env: Record = {}): string { + return execFileSync('sh', [join(giteaDir, name)], { + cwd: repoDir, + env: { ...runEnv, ...env }, + encoding: 'utf-8', + }).trim(); +} + +describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through `tea api`', () => { + beforeAll(() => { + fixture = mkdtempSync(join(tmpdir(), 'codev-1137-')); + binDir = join(fixture, 'bin'); + repoDir = join(fixture, 'repo'); + mkdirSync(binDir, { recursive: true }); + mkdirSync(repoDir, { recursive: true }); + + const teaPath = join(binDir, 'tea'); + writeFileSync(teaPath, FAKE_TEA, { mode: 0o755 }); + chmodSync(teaPath, 0o755); + + // Throwaway repo with a scp-style gitea remote → owner/repo = acme/widgets. + execFileSync('git', ['init', '-q'], { cwd: repoDir }); + execFileSync('git', ['remote', 'add', 'origin', 'git@git.example.com:acme/widgets.git'], { cwd: repoDir }); + + runEnv = { ...process.env, PATH: `${binDir}:${process.env.PATH ?? ''}` }; + }); + + afterAll(() => { + rmSync(fixture, { recursive: true, force: true }); + }); + + it('user-identity emits the bare login (not JSON)', () => { + expect(runScript('user-identity.sh')).toBe('octo'); + }); + + it('pr-view returns the PrViewResult shape from the PR object', () => { + const pr = JSON.parse(runScript('pr-view.sh', { CODEV_PR_NUMBER: '42' })); + expect(pr).toEqual({ + title: 'Add widget', + body: 'PR body', + state: 'open', + // PIR #1179: `url` is the browser page. The fixture carries both fields, + // so this also pins that Gitea's `url` (the API endpoint, which would + // render raw JSON) is NOT what lands in the contract. + url: 'https://git.example.com/acme/widgets/pulls/42', + author: { login: 'alice' }, + baseRefName: 'main', + headRefName: 'feature/x', + additions: 10, + deletions: 3, + }); + }); + + it('pr-list normalizes to PrListItem[] incl. real reviewRequests/isDraft/body', () => { + const list = JSON.parse(runScript('pr-list.sh')); + expect(list).toHaveLength(1); + expect(list[0]).toMatchObject({ + number: 42, + title: 'Add widget', + url: 'https://git.example.com/acme/widgets/pulls/42', + reviewDecision: '', + body: 'PR body', + createdAt: '2026-07-01T10:00:00Z', + author: { login: 'alice' }, + reviewRequests: ['bob'], // null-login (team) reviewers dropped + isDraft: true, + }); + expect(typeof list[0].number).toBe('number'); + }); + + it('pr-exists is true for an OPEN pull on the branch', () => { + expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'feature/x' })).toBe('true'); + }); + + it('pr-exists is true for a MERGED pull on the branch', () => { + expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'feature/done' })).toBe('true'); + }); + + it('pr-exists is false for a closed-not-merged branch', () => { + expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'feature/abandoned' })).toBe('false'); + }); + + it('pr-exists is false when no PR matches the branch', () => { + expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'no-such-branch' })).toBe('false'); + }); + + it('issue-view returns body, browser url, and comments as an ARRAY', () => { + const issue = JSON.parse(runScript('issue-view.sh', { CODEV_ISSUE_ID: '99' })); + expect(issue.title).toBe('Bug here'); + expect(issue.body).toBe('issue body'); + expect(issue.state).toBe('open'); + // html_url (browser page), NOT the API endpoint + expect(issue.url).toBe('https://git.example.com/acme/widgets/issues/99'); + // Contract requires an array — Gitea's issue object reports `comments` as an + // integer count, which would crash `issue.comments.filter(...)`. + expect(Array.isArray(issue.comments)).toBe(true); + expect(issue.comments).toEqual([ + { body: 'On it! Working on a fix now.', createdAt: '2026-07-06T08:00:00Z', author: { login: 'carol' } }, + { body: 'second', createdAt: '2026-07-06T09:00:00Z', author: { login: 'dave' } }, + ]); + }); + + it('recently-merged keeps merged pulls only and uses merged_at', () => { + const merged = JSON.parse(runScript('recently-merged.sh')); + expect(merged).toHaveLength(1); + expect(merged[0]).toEqual({ + number: 40, + title: 'Done PR', + url: 'https://git.example.com/acme/widgets/pulls/40', + body: 'merged body', + createdAt: '2026-07-02T09:00:00Z', + mergedAt: '2026-07-05T12:00:00Z', + headRefName: 'feature/done', + }); + }); + + it('issue-comment uses `tea comments add` and exits 0', () => { + // Would exit non-zero (throwing) if it invoked the non-existent + // `tea issues comment` subcommand. + expect(runScript('issue-comment.sh', { CODEV_ISSUE_ID: '99', CODEV_COMMENT_BODY: 'hi' })).toBe('commented'); + }); + + it('CODEV_REPO overrides the git-remote-derived owner/repo', () => { + // A repo whose remote does NOT resolve to acme/widgets still works when + // CODEV_REPO is supplied explicitly (the repo-archive-style callers). + const other = mkdtempSync(join(tmpdir(), 'codev-1137-other-')); + try { + execFileSync('git', ['init', '-q'], { cwd: other }); + execFileSync('git', ['remote', 'add', 'origin', 'https://git.example.com/someone/else.git'], { cwd: other }); + const out = execFileSync('sh', [join(giteaDir, 'pr-view.sh')], { + cwd: other, + env: { ...runEnv, CODEV_REPO: 'acme/widgets', CODEV_PR_NUMBER: '42' }, + encoding: 'utf-8', + }).trim(); + expect(JSON.parse(out).title).toBe('Add widget'); + } finally { + rmSync(other, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/codev/src/commands/porch/__tests__/bugfix-568-pr-exists-state-all.test.ts b/packages/codev/src/commands/porch/__tests__/bugfix-568-pr-exists-state-all.test.ts index e8f54c1bb..48cc69f63 100644 --- a/packages/codev/src/commands/porch/__tests__/bugfix-568-pr-exists-state-all.test.ts +++ b/packages/codev/src/commands/porch/__tests__/bugfix-568-pr-exists-state-all.test.ts @@ -63,9 +63,12 @@ describe('pr-exists forge scripts', () => { expect(fs.existsSync(scriptPath)).toBe(true); }); - it('fetches all pull states (--state all) to catch merged pulls (#568)', () => { + it('fetches all pull states (state=all) to catch merged pulls (#568)', () => { const content = fs.readFileSync(scriptPath, 'utf-8'); - expect(content).toContain('--state all'); + // #1137: routed through `tea api …/pulls?state=all` (the raw REST + // passthrough) instead of `tea pulls list --state all`, whose flattened + // output can't satisfy the `.head.ref` / `.merged` predicate. + expect(content).toContain('state=all'); }); it('filters out closed-not-merged PRs (#653)', () => { From be727a0e9c3c203bfc9af7975802e7e4604cc5b8 Mon Sep 17 00:00:00 2001 From: pseudo Date: Wed, 5 Aug 2026 01:56:11 -0600 Subject: [PATCH 05/14] [Bugfix #1137] Fix: Paginate gitea reads, factor REPO fail-fast, warn on degraded comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR #1146 review feedback: 1. Pagination (blocking). Gitea caps list responses at max_response_items (default 50), so the raw `&limit=200` passthrough silently truncated — pr-exists could false-negative a PR beyond the first ~50 (blocking a porch pr_exists gate) and recently-merged could miss on a busy repo. New shared helper `_lib.sh#tea_api_paged` walks page=1..N at limit=50, concatenates the arrays, and stops on a short/empty page with a hard 100-page ceiling. Chosen behavior: paginates, ceiling 100 pages. Wired into pr-exists, pr-list, recently-merged; output shape unchanged (same jq normalizers). 2. REPO derivation, fail-fast + factored. The CODEV_REPO/origin-derivation was duplicated in five scripts. Factored into `_lib.sh#gitea_repo`, sourced by issue-view, pr-exists, pr-list, pr-view, recently-merged. It now validates the result is a clean owner/repo and, if not, prints a stderr message naming CODEV_REPO as the remedy and exits non-zero (was a confusing `repos//…` 404). POSIX sh, $0-relative source; not a forge concept (KNOWN_CONCEPTS allowlist). 3. Degraded comments warn. issue-view still degrades a failed comments fetch to [], but now writes a stderr warning so it's distinguishable from a genuinely uncommented issue. stdout stays pure JSON (parsed by forge.ts). Co-Authored-By: Claude Opus 4.8 --- packages/codev/scripts/forge/gitea/_lib.sh | 76 +++++++++ .../codev/scripts/forge/gitea/issue-view.sh | 12 +- .../codev/scripts/forge/gitea/pr-exists.sh | 9 +- packages/codev/scripts/forge/gitea/pr-list.sh | 9 +- packages/codev/scripts/forge/gitea/pr-view.sh | 3 +- .../scripts/forge/gitea/recently-merged.sh | 9 +- .../bugfix-1137-gitea-tea-api.test.ts | 149 ++++++++++++++++-- 7 files changed, 245 insertions(+), 22 deletions(-) create mode 100755 packages/codev/scripts/forge/gitea/_lib.sh diff --git a/packages/codev/scripts/forge/gitea/_lib.sh b/packages/codev/scripts/forge/gitea/_lib.sh new file mode 100755 index 000000000..cf289b870 --- /dev/null +++ b/packages/codev/scripts/forge/gitea/_lib.sh @@ -0,0 +1,76 @@ +# Shared helpers for the Gitea forge preset scripts. +# +# This file is SOURCED, not executed (`. "$(dirname "$0")/_lib.sh"`), so it has +# no shebang and defines only functions/vars. POSIX sh only — no bashisms — the +# scripts are #!/bin/sh and forge runs them via `sh -c`. It is not a forge +# concept: forge.ts builds presets from an explicit KNOWN_CONCEPTS allowlist, so +# a leading-underscore file in this directory is never registered as a concept. + +# Resolve owner/repo for the `tea api` path. +# +# `tea api` needs an explicit owner/repo in the path (unlike `tea pulls`/`tea +# issues`, which auto-detect it from the local git remote). Honor CODEV_REPO +# when set, else derive owner/repo from origin's URL (handles https, ssh, and +# scp-style remotes, with or without a .git suffix). +# +# Fails fast: if the result isn't a clean `owner/repo` (no origin remote, an +# unusual URL, etc.), print a stderr message naming CODEV_REPO as the remedy and +# return non-zero so the caller can `exit 1` — otherwise `tea api "repos//…"` +# fails later with a confusing 404. Callers must use: REPO="$(gitea_repo)" || exit 1 +gitea_repo() { + _repo="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" + _owner=${_repo%%/*} + _rest=${_repo#*/} + # Valid iff exactly one slash, both sides non-empty: + # - "$_owner" = "$_repo" → no slash at all + # - -z "$_owner" / -z "$_rest" → empty owner or repo (e.g. "/x", "x/") + # - "$_rest" != "${_rest%/*}" → a second slash (e.g. "a/b/c") + if [ -z "$_repo" ] || [ "$_owner" = "$_repo" ] || [ -z "$_owner" ] || [ -z "$_rest" ] || [ "$_rest" != "${_rest%/*}" ]; then + echo "gitea forge: could not determine owner/repo from the 'origin' remote; set CODEV_REPO=owner/repo" >&2 + return 1 + fi + printf '%s' "$_repo" +} + +# Page size to request per page. Gitea caps list responses at the server's +# `max_response_items` (default 50), so `&limit=200` silently truncates to ~50 +# with no client-side pagination. Requesting 50 matches that default cap; a +# server tuned higher just returns more per page (fewer round-trips). +GITEA_PAGE_LIMIT=50 + +# Hard ceiling on pages fetched, so a misbehaving server that never returns a +# short page can't spin forever. 100 pages × 50 = 5000 items — far beyond any +# real open-PR / recently-merged / all-pulls window we page over. +GITEA_MAX_PAGES=100 + +# Fetch a paginated Gitea list endpoint and emit ONE concatenated JSON array on +# stdout, so the caller's existing jq normalizer sees the same shape as before. +# +# Usage: tea_api_paged "repos///pulls" "state=all" +# $1 = API path (no page params) +# $2 = extra query string (may be empty), e.g. "state=open" +# +# Loops page=1,2,3… appending "&limit=&page=", concatenates each page's +# array, and stops when a page returns fewer than the requested limit (the last +# page) or an empty/blank response, bounded by GITEA_MAX_PAGES. +tea_api_paged() { + _path="$1" + _query="$2" + _page=1 + _acc='[]' + while [ "$_page" -le "$GITEA_MAX_PAGES" ]; do + if [ -n "$_query" ]; then + _url="${_path}?${_query}&limit=${GITEA_PAGE_LIMIT}&page=${_page}" + else + _url="${_path}?limit=${GITEA_PAGE_LIMIT}&page=${_page}" + fi + _resp="$(tea api "$_url")" || return 1 + # Blank body or an empty array → no more pages. + [ -n "$_resp" ] || break + _count="$(printf '%s' "$_resp" | jq 'length')" || return 1 + _acc="$(printf '%s\n%s' "$_acc" "$_resp" | jq -s 'add')" || return 1 + [ "$_count" -lt "$GITEA_PAGE_LIMIT" ] && break + _page=$((_page + 1)) + done + printf '%s' "$_acc" +} diff --git a/packages/codev/scripts/forge/gitea/issue-view.sh b/packages/codev/scripts/forge/gitea/issue-view.sh index f2ead28e1..b9350d0bf 100755 --- a/packages/codev/scripts/forge/gitea/issue-view.sh +++ b/packages/codev/scripts/forge/gitea/issue-view.sh @@ -17,10 +17,16 @@ # Gitea's issue object reports `comments` as an integer count, not the array # the contract requires (consumers call `.comments.filter(...)`), so the # comments array is fetched separately and merged in. A failed/empty comments -# fetch degrades to []. -REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" +# fetch degrades to [], but warns on stderr so the degraded path is +# distinguishable from a genuinely uncommented issue (stdout stays pure JSON — +# it's parsed by forge.ts). +. "$(dirname "$0")/_lib.sh" +REPO="$(gitea_repo)" || exit 1 COMMENTS_JSON="$(tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}/comments" 2>/dev/null)" -[ -n "$COMMENTS_JSON" ] || COMMENTS_JSON="[]" +if [ -z "$COMMENTS_JSON" ]; then + echo "gitea forge: comments fetch failed for issue ${CODEV_ISSUE_ID}; reporting 0 comments" >&2 + COMMENTS_JSON="[]" +fi tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}" \ | jq --argjson comments "$COMMENTS_JSON" '{ title, diff --git a/packages/codev/scripts/forge/gitea/pr-exists.sh b/packages/codev/scripts/forge/gitea/pr-exists.sh index d8d8c9b0d..9cc437d3a 100755 --- a/packages/codev/scripts/forge/gitea/pr-exists.sh +++ b/packages/codev/scripts/forge/gitea/pr-exists.sh @@ -18,7 +18,12 @@ # original branch name, so a branch-name match won't hit a merged+deleted # branch. That doesn't affect the "does an open/merged PR exist for the branch # I'm about to push" use case. -REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" -tea api "repos/${REPO}/pulls?state=all&limit=200" \ +# +# `state=all` is paginated (Gitea caps a page at max_response_items, default 50) +# so a branch whose PR isn't in the most recent ~50 would false-negative and +# block a porch pr_exists gate — tea_api_paged walks every page (see _lib.sh). +. "$(dirname "$0")/_lib.sh" +REPO="$(gitea_repo)" || exit 1 +tea_api_paged "repos/${REPO}/pulls" "state=all" \ | jq --arg branch "$CODEV_BRANCH_NAME" \ '[.[] | select(.head.ref == $branch and (.state == "open" or .merged == true))] | length > 0' diff --git a/packages/codev/scripts/forge/gitea/pr-list.sh b/packages/codev/scripts/forge/gitea/pr-list.sh index a6deac289..73c84f4be 100755 --- a/packages/codev/scripts/forge/gitea/pr-list.sh +++ b/packages/codev/scripts/forge/gitea/pr-list.sh @@ -20,8 +20,13 @@ # .requested_reviewers[].login -> reviewRequests (user logins; teams have no login → dropped) # .draft -> isDraft # reviewDecision -> "" (Gitea has no GitHub-equivalent review-decision summary) -REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" -tea api "repos/${REPO}/pulls?state=open&limit=200" \ +# +# The open-pulls list is paginated (Gitea caps a page at max_response_items, +# default 50), so tea_api_paged walks every page rather than silently truncating +# at ~50 open PRs (see _lib.sh). +. "$(dirname "$0")/_lib.sh" +REPO="$(gitea_repo)" || exit 1 +tea_api_paged "repos/${REPO}/pulls" "state=open" \ | jq '[.[] | { number, title, diff --git a/packages/codev/scripts/forge/gitea/pr-view.sh b/packages/codev/scripts/forge/gitea/pr-view.sh index 10c13149f..bd1cbbbdf 100755 --- a/packages/codev/scripts/forge/gitea/pr-view.sh +++ b/packages/codev/scripts/forge/gitea/pr-view.sh @@ -15,7 +15,8 @@ # API endpoint (would render raw JSON in a browser), so map `html_url` and fall # back to `url` only if it's absent — the same choice PIR #1179 made when this # concept still went through `tea pulls view`. -REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" +. "$(dirname "$0")/_lib.sh" +REPO="$(gitea_repo)" || exit 1 tea api "repos/${REPO}/pulls/${CODEV_PR_NUMBER}" | jq '{ title, body: (.body // ""), diff --git a/packages/codev/scripts/forge/gitea/recently-merged.sh b/packages/codev/scripts/forge/gitea/recently-merged.sh index 3cd9e083c..7f134ce47 100755 --- a/packages/codev/scripts/forge/gitea/recently-merged.sh +++ b/packages/codev/scripts/forge/gitea/recently-merged.sh @@ -12,8 +12,13 @@ # auto-detects it from the local git remote), so resolve it here: honor # CODEV_REPO when set, else derive owner/repo from origin's URL (handles https, # ssh, and scp-style remotes, with or without a .git suffix). -REPO="${CODEV_REPO:-$(git remote get-url origin 2>/dev/null | sed -E -e 's#\.git$##' -e 's#.*[/:]([^/]+/[^/]+)$#\1#')}" -tea api "repos/${REPO}/pulls?state=closed&limit=200" \ +# +# The closed-pulls list is paginated (Gitea caps a page at max_response_items, +# default 50), so on a busy repo the most-recent merges could push older ones +# past the first page — tea_api_paged walks every page (see _lib.sh). +. "$(dirname "$0")/_lib.sh" +REPO="$(gitea_repo)" || exit 1 +tea_api_paged "repos/${REPO}/pulls" "state=closed" \ | jq '[.[] | select(.merged == true) | { number, title, diff --git a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts index cc0e5b971..53bdf9a87 100644 --- a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts +++ b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts @@ -8,6 +8,16 @@ * passthrough returns exactly the Gitea REST shape the jq normalizers and * `forge-contracts.ts` already assume. * + * PR #1146 review follow-up: + * - list reads (`pr-exists`, `pr-list`, `recently-merged`) now PAGINATE via + * the shared `tea_api_paged` helper (Gitea caps a page at max_response_items, + * default 50, so `&limit=200` silently truncated). The fake `tea` below + * serves a full 50-item page 1 + a short page 2 and the tests assert an item + * that only exists on page 2 is found. + * - the `owner/repo` derivation is factored into `_lib.sh#gitea_repo` and fails + * fast (stderr + non-zero exit) when there's no usable origin remote. + * - `issue-view` warns on stderr when the comments fetch degrades to []. + * * `tea` isn't available in CI (see the in-repo #920 note), so this test stubs a * fake `tea` on PATH that answers `api ` (and `comments add`) with * captured Gitea REST fixtures, points the scripts at a throwaway git repo with @@ -25,7 +35,7 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; -import { execFileSync } from 'node:child_process'; +import { execFileSync, spawnSync } from 'node:child_process'; const giteaDir = resolve(__dirname, '..', '..', 'scripts', 'forge', 'gitea'); @@ -33,6 +43,10 @@ const giteaDir = resolve(__dirname, '..', '..', 'scripts', 'forge', 'gitea'); // fixed scripts use) plus `comments add`. Each endpoint returns the raw Gitea // REST shape — nested objects, real `.merged`/`.merged_at`/`.draft`, integer // `comments` count on the issue object, etc. +// +// The paginated list endpoints (page=1 full at limit 50, page=2 short) prove the +// scripts walk past the server's page cap: each carries a "signature" item plus +// filler, and a distinct item that lives ONLY on page 2. const FAKE_TEA = `#!/bin/sh if [ "$1" = "comments" ] && [ "$2" = "add" ]; then # comments add @@ -45,16 +59,42 @@ case "$2" in echo '{"login":"octo","id":7}' ;; repos/acme/widgets/pulls/42) echo '{"number":42,"title":"Add widget","body":"PR body","state":"open","html_url":"https://git.example.com/acme/widgets/pulls/42","url":"https://git.example.com/api/v1/repos/acme/widgets/pulls/42","user":{"login":"alice"},"base":{"ref":"main"},"head":{"ref":"feature/x"},"additions":10,"deletions":3}' ;; - "repos/acme/widgets/pulls?state=open&limit=200") - echo '[{"number":42,"title":"Add widget","html_url":"https://git.example.com/acme/widgets/pulls/42","url":"https://git.example.com/api/v1/repos/acme/widgets/pulls/42","body":"PR body","state":"open","created_at":"2026-07-01T10:00:00Z","user":{"login":"alice"},"requested_reviewers":[{"login":"bob"},{"login":null}],"draft":true}]' ;; - "repos/acme/widgets/pulls?state=all&limit=200") - echo '[{"number":42,"state":"open","merged":false,"head":{"ref":"feature/x"}},{"number":40,"state":"closed","merged":true,"head":{"ref":"feature/done"}},{"number":39,"state":"closed","merged":false,"head":{"ref":"feature/abandoned"}}]' ;; - "repos/acme/widgets/pulls?state=closed&limit=200") - echo '[{"number":40,"title":"Done PR","html_url":"https://git.example.com/acme/widgets/pulls/40","body":"merged body","state":"closed","merged":true,"merged_at":"2026-07-05T12:00:00Z","created_at":"2026-07-02T09:00:00Z","head":{"ref":"feature/done"}},{"number":39,"title":"Abandoned","state":"closed","merged":false,"head":{"ref":"feature/abandoned"}}]' ;; + + # --- pr-exists: state=all, paginated ------------------------------------- + # page 1 = 50 items (open feature/x, merged feature/done, closed-not-merged + # feature/abandoned, + 47 open pad). page 2 = 1 merged item on feature/deep. + "repos/acme/widgets/pulls?state=all&limit=50&page=1") + jq -cn '[{number:42,state:"open",merged:false,head:{ref:"feature/x"}},{number:40,state:"closed",merged:true,head:{ref:"feature/done"}},{number:39,state:"closed",merged:false,head:{ref:"feature/abandoned"}}] + [range(47)|{number:(1000+.),state:"open",merged:false,head:{ref:("pad-"+(.|tostring))}}]' ;; + "repos/acme/widgets/pulls?state=all&limit=50&page=2") + echo '[{"number":900,"state":"closed","merged":true,"head":{"ref":"feature/deep"}}]' ;; + + # --- pr-list: state=open, paginated -------------------------------------- + # page 1 = the rich #42 item + 49 pad (50 total). page 2 = 1 item (#900). + "repos/acme/widgets/pulls?state=open&limit=50&page=1") + jq -cn '[{number:42,title:"Add widget",html_url:"https://git.example.com/acme/widgets/pulls/42",url:"https://git.example.com/api/v1/repos/acme/widgets/pulls/42",body:"PR body",state:"open",created_at:"2026-07-01T10:00:00Z",user:{login:"alice"},requested_reviewers:[{login:"bob"},{login:null}],draft:true}] + [range(49)|{number:(1000+.),title:"pad",html_url:"u",body:"",state:"open",created_at:"d",user:{login:"pad"},requested_reviewers:[],draft:false}]' ;; + "repos/acme/widgets/pulls?state=open&limit=50&page=2") + echo '[{"number":900,"title":"Deep open PR","html_url":"https://git.example.com/acme/widgets/pulls/900","body":"deep","state":"open","created_at":"2026-07-01T11:00:00Z","user":{"login":"erin"},"requested_reviewers":[],"draft":false}]' ;; + + # --- recently-merged: state=closed, paginated ---------------------------- + # page 1 = 50 items, only #40 merged (the rest merged:false pad). page 2 = 1 + # merged item (#901) — so a merged PR beyond page 1 must still surface. + "repos/acme/widgets/pulls?state=closed&limit=50&page=1") + jq -cn '[{number:40,title:"Done PR",html_url:"https://git.example.com/acme/widgets/pulls/40",body:"merged body",state:"closed",merged:true,merged_at:"2026-07-05T12:00:00Z",created_at:"2026-07-02T09:00:00Z",head:{ref:"feature/done"}},{number:39,title:"Abandoned",state:"closed",merged:false,head:{ref:"feature/abandoned"}}] + [range(48)|{number:(2000+.),title:"pad",state:"closed",merged:false,head:{ref:"pad"}}]' ;; + "repos/acme/widgets/pulls?state=closed&limit=50&page=2") + echo '[{"number":901,"title":"Deep merge","html_url":"https://git.example.com/acme/widgets/pulls/901","body":"deep merged","state":"closed","merged":true,"merged_at":"2026-07-06T12:00:00Z","created_at":"2026-07-03T09:00:00Z","head":{"ref":"feature/deep-merge"}}]' ;; + + # --- issue-view ---------------------------------------------------------- repos/acme/widgets/issues/99) echo '{"number":99,"title":"Bug here","body":"issue body","state":"open","html_url":"https://git.example.com/acme/widgets/issues/99","url":"https://git.example.com/api/v1/repos/acme/widgets/issues/99","comments":2}' ;; repos/acme/widgets/issues/99/comments) echo '[{"body":"On it! Working on a fix now.","created_at":"2026-07-06T08:00:00Z","user":{"login":"carol"}},{"body":"second","created_at":"2026-07-06T09:00:00Z","user":{"login":"dave"}}]' ;; + # issue 98: the issue object fetches fine but its comments endpoint fails, + # exercising the degraded (stderr-warned) []-comments path. + repos/acme/widgets/issues/98) + echo '{"number":98,"title":"No comments reachable","body":"body","state":"open","html_url":"https://git.example.com/acme/widgets/issues/98","comments":5}' ;; + repos/acme/widgets/issues/98/comments) + echo "fake-tea: comments endpoint down" >&2; exit 7 ;; + *) echo "fake-tea: no fixture for: $2" >&2; exit 4 ;; esac `; @@ -84,6 +124,20 @@ function runScript(name: string, env: Record = {}): string { }).trim(); } +/** Run a script capturing stdout, stderr and exit status (for failure paths). */ +function runScriptFull( + name: string, + env: Record = {}, + cwd: string = repoDir, +): { status: number | null; stdout: string; stderr: string } { + const r = spawnSync('sh', [join(giteaDir, name)], { + cwd, + env: { ...runEnv, ...env }, + encoding: 'utf-8', + }); + return { status: r.status, stdout: r.stdout ?? '', stderr: r.stderr ?? '' }; +} + describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through `tea api`', () => { beforeAll(() => { fixture = mkdtempSync(join(tmpdir(), 'codev-1137-')); @@ -131,8 +185,8 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` it('pr-list normalizes to PrListItem[] incl. real reviewRequests/isDraft/body', () => { const list = JSON.parse(runScript('pr-list.sh')); - expect(list).toHaveLength(1); - expect(list[0]).toMatchObject({ + const first = list.find((p: { number: number }) => p.number === 42); + expect(first).toMatchObject({ number: 42, title: 'Add widget', url: 'https://git.example.com/acme/widgets/pulls/42', @@ -143,7 +197,13 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` reviewRequests: ['bob'], // null-login (team) reviewers dropped isDraft: true, }); - expect(typeof list[0].number).toBe('number'); + expect(typeof first.number).toBe('number'); + }); + + it('pr-list paginates: a PR only on page 2 still appears (51 total)', () => { + const list = JSON.parse(runScript('pr-list.sh')); + expect(list).toHaveLength(51); // 50 (page 1) + 1 (page 2) + expect(list.some((p: { number: number }) => p.number === 900)).toBe(true); }); it('pr-exists is true for an OPEN pull on the branch', () => { @@ -162,6 +222,12 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'no-such-branch' })).toBe('false'); }); + it('pr-exists paginates: a merged PR only on page 2 is found', () => { + // page 1 is a full 50 items; feature/deep exists ONLY on page 2, so this + // would false-negative (and block a porch pr_exists gate) without paging. + expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'feature/deep' })).toBe('true'); + }); + it('issue-view returns body, browser url, and comments as an ARRAY', () => { const issue = JSON.parse(runScript('issue-view.sh', { CODEV_ISSUE_ID: '99' })); expect(issue.title).toBe('Bug here'); @@ -178,10 +244,21 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` ]); }); + it('issue-view degrades to [] comments AND warns on stderr when the fetch fails', () => { + // Issue 98's comments endpoint errors. stdout must stay pure JSON with an + // empty array; stderr must carry a trace so [] is distinguishable from + // "genuinely no comments". + const { status, stdout, stderr } = runScriptFull('issue-view.sh', { CODEV_ISSUE_ID: '98' }); + expect(status).toBe(0); + const issue = JSON.parse(stdout); + expect(issue.comments).toEqual([]); + expect(stderr).toContain('comments fetch failed for issue 98'); + }); + it('recently-merged keeps merged pulls only and uses merged_at', () => { const merged = JSON.parse(runScript('recently-merged.sh')); - expect(merged).toHaveLength(1); - expect(merged[0]).toEqual({ + const done = merged.find((p: { number: number }) => p.number === 40); + expect(done).toEqual({ number: 40, title: 'Done PR', url: 'https://git.example.com/acme/widgets/pulls/40', @@ -190,6 +267,19 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` mergedAt: '2026-07-05T12:00:00Z', headRefName: 'feature/done', }); + // closed-not-merged pulls are excluded. + expect(merged.some((p: { number: number }) => p.number === 39)).toBe(false); + }); + + it('recently-merged paginates: a merged PR only on page 2 is included', () => { + const merged = JSON.parse(runScript('recently-merged.sh')); + expect(merged).toHaveLength(2); // #40 (page 1) + #901 (page 2) + const deep = merged.find((p: { number: number }) => p.number === 901); + expect(deep).toMatchObject({ + number: 901, + mergedAt: '2026-07-06T12:00:00Z', + headRefName: 'feature/deep-merge', + }); }); it('issue-comment uses `tea comments add` and exits 0', () => { @@ -215,4 +305,39 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` rmSync(other, { recursive: true, force: true }); } }); + + it('fails fast (non-zero + stderr naming CODEV_REPO) with no usable origin remote', () => { + const bare = mkdtempSync(join(tmpdir(), 'codev-1137-noremote-')); + try { + execFileSync('git', ['init', '-q'], { cwd: bare }); + // No origin remote at all. + const { status, stdout, stderr } = runScriptFull( + 'pr-exists.sh', + { CODEV_BRANCH_NAME: 'feature/x' }, + bare, + ); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + expect(stderr).toContain('CODEV_REPO'); + } finally { + rmSync(bare, { recursive: true, force: true }); + } + }); + + it('fails fast with a garbage origin URL that has no owner/repo', () => { + const garbage = mkdtempSync(join(tmpdir(), 'codev-1137-garbage-')); + try { + execFileSync('git', ['init', '-q'], { cwd: garbage }); + execFileSync('git', ['remote', 'add', 'origin', 'https://example.com/'], { cwd: garbage }); + const { status, stderr } = runScriptFull( + 'issue-view.sh', + { CODEV_ISSUE_ID: '99' }, + garbage, + ); + expect(status).not.toBe(0); + expect(stderr).toContain('CODEV_REPO'); + } finally { + rmSync(garbage, { recursive: true, force: true }); + } + }); }); From fa5a7cb709fd3de9a19cfa730ac4a84dda8ad414 Mon Sep 17 00:00:00 2001 From: pseudo Date: Fri, 14 Aug 2026 08:11:41 -0600 Subject: [PATCH 06/14] [task-24AO] Builder thread: rebase of #1146, reconcile with #1458 Co-Authored-By: Claude Opus 5 (1M context) --- codev/state/task-24AO_thread.md | 175 ++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 codev/state/task-24AO_thread.md diff --git a/codev/state/task-24AO_thread.md b/codev/state/task-24AO_thread.md new file mode 100644 index 000000000..b22be20d6 --- /dev/null +++ b/codev/state/task-24AO_thread.md @@ -0,0 +1,175 @@ +# task-24AO — rebase PR #1146, reconcile it with PR #1458 + +Two open PRs against `cluesmith/codev` overlapped and neither builder saw the other. +This builder rebased one and reconciled the other. Both branches live on the +**pseudoseed fork only** — we have no push access to `cluesmith/codev`, and neither +PR was merged (the maintainer merges). + +## Task 1 — rebase #1146 (`builder/bugfix-1137`) + +2063 commits behind `upstream/main`, 5 commits of its own, `mergeable=CONFLICTING`. + +Rebased onto `upstream/main`. Exactly one file conflicted, twice — `gitea/pr-view.sh`: + +- **Conflict 1** (against commit 3, "Route gitea forge reads through `tea api`"): + upstream had landed PIR #1179, which added `url` mapped from Gitea's `html_url` + (Gitea's own `url` is the API endpoint and would render raw JSON in a browser). + #1146 rewrote the same script onto `tea api` with an explicit normalizer that + emitted **no `url` at all**. Naively taking either side loses something. + Resolution: keep #1146's `tea api` routing **and** re-add `url: (.html_url // .url)`. + `forge-contracts.ts` documents that mapping for Gitea by name, so dropping it + would have silently regressed #1179. +- **Conflict 2** (against commit 5, which factors REPO derivation into + `_lib.sh#gitea_repo`): same file, same hunk. Kept the factored `gitea_repo` call + plus the `html_url` mapping. + +**Behaviour change from the resolution, stated plainly:** gitea `pr-view` now emits +a `url` field it did not emit on the pre-rebase branch. That is a restoration of +upstream's behaviour, not a new invention — but it is a real output change, so +commit 4's test fixture gained `html_url`/`url` and the assertion now pins that the +**browser page**, not the API endpoint, reaches the contract. + +Two smaller deviations, both deliberate: + +- `_lib.sh` is committed `100755`, not `100644`. `scripts/postinstall.mjs` chmods + every `scripts/forge/**/*.sh` to 755 unconditionally, so 644 is a mode that never + survives an install and leaves a permanently dirty worktree for anyone who runs + `pnpm install`. It is sourced, not executed; the bit is harmless. +- The test fixture change was applied **inside commit 4** (via an interactive rebase + stop), not as a trailing fixup, so every commit stays green in isolation. + +Verified: all 5 commits pass the forge suites individually +(`bugfix-1137-gitea-tea-api`, `bugfix-568-pr-exists-state-all`, `forge`, +`bugfix-693-forge-exec-bit`). An earlier per-commit run was **invalid** — the +`git checkout`s silently failed on a dirty `_lib.sh` and re-tested HEAD three +times. Caught and redone. + +## Task 2 — the reconcile + +#1146's core finding: Gitea caps every list response at `max_response_items`, +default 50, so `&limit=200` **silently truncates**. #1458's `gitea/pr-create.sh` +created the PR with `tea pulls create` and then looked it up with +`tea pulls list --limit 200` — the exact call #1146 disproves. On a busy repo the +just-created PR falls off the page and pr-create exits 1 for a PR that exists, +inviting a duplicate retry. That `--limit 200` had been added as a *fix* for a +review defect, so it was a fix built on a false premise. + +Confirmed the premise is false, live on Forgejo 15.0.2: `settings/api` reports +`max_response_items: 50`, and `?limit=200` returns exactly 50 items on a list where +paging at 50 returns 53. + +**Chose option (b) — drop list-and-search entirely.** It is possible: +`tea api -X POST repos/{owner}/{repo}/pulls` returns the created PR, `number` and +`html_url` included. Nothing to search, nothing to race, nothing to truncate. It +also deletes the `:` head-matching heuristic — the API resolves an +owner-qualified head itself. + +### What live verification changed about the design + +Every one of these was found by testing, not by reading docs. Three of them are +**the same bug class as #1455 itself** — an operation accepted and then silently +not performed — so each is handled in code rather than noted as a caveat. The +architect independently flagged the same three; the resolutions below are what +shipped. + +| Finding | Consequence | +|---|---| +| `tea api` **exits 0 on HTTP errors**, printing the error body | The whole change replaces a lookup with one call, so trusting the exit code would reintroduce #1455's silent success *inside the fix for it*. The response is asserted to BE a PR object — numeric `number` AND non-empty browser URL — or it fails loudly with the body. Duplicate head, missing branch and unresolvable repo all exited 0 before. | +| The API **requires** `base` (`[Base]: Required`); `tea pulls create` defaulted it client-side | Posting against the wrong base silently is worse than erroring. An unset `CODEV_PR_BASE` now resolves the repo's default branch explicitly, and fails clearly if it cannot. | +| `draft: true` in the payload is **silently ignored** (response comes back `draft: false`) | `CODEV_PR_DRAFT=1` would have been an accepted-and-ignored flag. Gitea marks drafts by a `WIP:` title prefix — what `tea pulls create --draft` does. Implemented, and verified server-side to produce `draft: true`. | +| `{owner}`/`{repo}` are substituted by tea from repo context; `--repo` supplies it | No dependency on #1146's `_lib.sh`. Verified with https and scp-style remotes, and from a GitHub-remote cwd. | +| POST's `url` is the browser page (unlike GET, where `url` is the API endpoint) | `.html_url // .url` covers both. | + +**One subtlety inside the first row.** If the response carries a numeric `number` +but no usable URL, the PR *was* created and only the URL is missing. Exiting 1 is +still right, but a generic failure message would read as "nothing happened" and +invite the duplicate retry this entire change exists to prevent. That case gets its +own message naming the PR number and saying explicitly not to retry. + +### Deliberate divergence from #1146's siblings + +The read concepts derive owner/repo via `_lib.sh#gitea_repo`; pr-create uses tea's +`{owner}`/`{repo}` placeholders instead. Reasons: pr-create's input is +`CODEV_PR_REPO`, not `CODEV_REPO` (different contract), and sourcing `_lib.sh` would +make #1458 depend on #1146 merging first. **The two PRs stay independent and can +merge in either order.** The one thing `gitea_repo` gave that placeholders did not +was a good error message, so pr-create now names `CODEV_PR_REPO` as the remedy on a +404 rather than leaking a bare `404 page not found`. + +## Scope note + +The reconcile required altering #1458 — `pr-create.sh` exists only on that branch. +It was added as **one new commit on top**, not a rebase: #1458 was 0 behind +`upstream/main` and its history is untouched. + +## Verification and cleanup + +All Gitea work ran against scratch repo `pseudoseed/research` (zero CI workflows, so +it steals no runner slots). Nine scratch PRs (#18–#26) created and **all closed**; +every scratch branch deleted; no leftover files on `main`. Confirmed empty +afterwards. No `tea` token scope was widened — everything needed was already in +scope. + +Tests: the new gitea cases were checked against the **old** script first and all of +them fail there, so the regression pin is real rather than decorative. The +exit-0-on-error assertion is pinned by a table of six non-PR payloads (error +object, array, string-typed `number`, numberless object, `null`, empty body), each +served at exit 0. + +## Merge order — verified, not asserted + +Both PRs come from the same fork and both touch `packages/codev/scripts/forge/gitea/`, so the +maintainer would otherwise have to derive the ordering. Checked rather than assumed: + +- `git merge-tree` on the two branch tips merges **cleanly**. The only file both touch is this + thread log, which is the **identical blob** on both branches (that is why they were kept + byte-identical) and auto-merges. +- In the merged tree, `_lib.sh` and `pr-view.sh` are byte-identical to the #1146 versions and + `pr-create.sh` byte-identical to the #1458 version — no silent blending. +- The `bugfix-693` invariant (every entry under a provider dir is a `*.sh`) still holds with + `_lib.sh` present. + +**Either order is safe.** #1458's `pr-create.sh` does not source `_lib.sh` and calls neither +`gitea_repo` nor `tea_api_paged`. + +**Duplication, honestly stated.** The paginator is *not* duplicated and should not be — +`tea_api_paged` walks a truncating list endpoint, and pr-create no longer lists anything. But +**repo resolution now has two paths**: the read concepts use `_lib.sh#gitea_repo` (reads +`CODEV_REPO`, else derives from origin, fails fast), pr-create uses tea's `{owner}`/`{repo}` +placeholders with `CODEV_PR_REPO` forwarded as `--repo`. Kept separate deliberately — +`gitea_repo()` takes no argument and reads the *other* env var, so consuming it would have meant +editing a #1146 file from #1458 and creating the coupling this avoids; and `--repo` also supplies +tea's login/host context, which a path-only helper does not. + +Both PR bodies now carry this in full, with the recommended follow-up: **once both land**, unify +behind `gitea_repo "$CODEV_PR_REPO"` so there is one path and one error message. Not done here on +purpose — doing it now couples two independent PRs. + +## Final test numbers (always reported against a control) + +Same worktree, same command, three runs: + +| Tree | Passed | Failed | Files failed | +|---|---|---|---| +| **Baseline — unmodified `upstream/main`** | 3176 | 126 | 67 | +| #1146 branch (rebased tip) | 3193 (+17) | 126 | 67 | +| #1458 branch | 3218 (+42) | 126 | 67 | + +The *same* 67 files and *same* 126 tests fail in all three, so: **zero regressions on either +branch.** The failures are `agent-farm` / `terminal` / `consolidate` (shellper sockets, SQLite +state) — environment-dependent: this worktree has no built `dist/`, which those tests spawn from, +and a live Tower runs against the same state. None is in a file either PR touches; every forge +suite passes. + +A raw pass/fail count with no control is unreadable — 126 failures looks alarming until the +baseline shows it is the environment. Report it with the control every time. + +## Open items for the maintainer + +- Both PRs are pushed to the pseudoseed fork; neither is merged. +- `_lib.sh#tea_api_paged` in #1146 concatenates pages with `jq -s 'add'`. Because + `tea api` exits 0 on HTTP errors, an error page reaches jq as an object and the + add fails with a raw jq type error rather than the API's message. It *does* fail + rather than silently mis-page, so this is a diagnosability wart, not a + correctness bug — flagged, deliberately not fixed here to keep the rebase faithful + to the original commits. From c219cdba8d9a91e82efb4362aa428cdfe894d7c0 Mon Sep 17 00:00:00 2001 From: pseudo Date: Thu, 20 Aug 2026 14:46:52 -0600 Subject: [PATCH 07/14] chore(porch): bugfix-1137 init bugfix --- .../status.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml diff --git a/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml b/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml new file mode 100644 index 000000000..f2f0e7a75 --- /dev/null +++ b/codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml @@ -0,0 +1,14 @@ +id: bugfix-1137 +title: gitea-forge-preset-is-broken-a +protocol: bugfix +phase: investigate +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: pending +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-20T20:46:52.017Z' +updated_at: '2026-08-20T20:46:52.018Z' From c9f55a537945009d0b03e34c2732f51e163bb9f3 Mon Sep 17 00:00:00 2001 From: pseudo Date: Thu, 20 Aug 2026 19:41:09 -0600 Subject: [PATCH 08/14] =?UTF-8?q?[Bugfix=20#1137]=20Fix:=20address=20CMAP?= =?UTF-8?q?=20review=20=E2=80=94=20tea=200.14.1=20comment=20cmd,=20masked?= =?UTF-8?q?=20pipe=20failures,=20sub-limit=20page=20cap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1146 review (2026-08-17, amrmelsayed) — REQUEST_CHANGES, 2 blocking + 1 strongly-recommended item: 1. (blocking) issue-comment.sh called `tea comments add`, which only exists on tea 0.14.2+ and fails on the still-current 0.14.1 release ("No help topic for comments"). Switch to the `tea comment ` shorthand, which works on both 0.14.1 and 0.14.2+. 2. (blocking) pr-exists.sh, pr-list.sh, recently-merged.sh piped `tea_api_paged | jq` directly. POSIX sh has no pipefail, so a mid-walk pagination failure (tea_api_paged returns 1) was masked by jq's exit status (0 on empty stdin) — pr-exists in particular would report "false" for a real error instead of failing, silently passing a porch pr_exists gate. Capture the paginator's output into a variable and check its exit status before piping to jq. 3. (strongly recommended) tea_api_paged's stop condition compared each page's item count against the *requested* limit (50). A server whose max_response_items is tuned below that requested limit truncates every page — including non-last pages — to its own cap, so every page looked "short" and the loop broke after page 1. Compare against the size actually observed on page 1 instead. Item 4 (a `# forge-executable: tea` header convention) depends on #1458's extractExecutable convention landing first, which hasn't happened — left for a follow-up once #1458 merges, per the reviewer's stated merge order. Regression tests added for all three: a 0.14.1-compatible `tea comment` stub, a mid-walk pagination failure fixture exercised by all three paginated scripts, and a sub-50-per-page server-cap fixture across 3 pages proving pr-list keeps walking. Full local suite: 3197 passed, 126 failed (same 67 pre-existing environment-dependent files as the unmodified baseline) — zero regressions, +4 new passing tests. Co-Authored-By: Claude Sonnet 5 --- packages/codev/scripts/forge/gitea/_lib.sh | 10 ++- .../scripts/forge/gitea/issue-comment.sh | 6 +- .../codev/scripts/forge/gitea/pr-exists.sh | 10 ++- packages/codev/scripts/forge/gitea/pr-list.sh | 8 +- .../scripts/forge/gitea/recently-merged.sh | 8 +- .../bugfix-1137-gitea-tea-api.test.ts | 88 +++++++++++++++++-- 6 files changed, 115 insertions(+), 15 deletions(-) diff --git a/packages/codev/scripts/forge/gitea/_lib.sh b/packages/codev/scripts/forge/gitea/_lib.sh index cf289b870..594587826 100755 --- a/packages/codev/scripts/forge/gitea/_lib.sh +++ b/packages/codev/scripts/forge/gitea/_lib.sh @@ -58,6 +58,7 @@ tea_api_paged() { _query="$2" _page=1 _acc='[]' + _page_size='' while [ "$_page" -le "$GITEA_MAX_PAGES" ]; do if [ -n "$_query" ]; then _url="${_path}?${_query}&limit=${GITEA_PAGE_LIMIT}&page=${_page}" @@ -68,8 +69,15 @@ tea_api_paged() { # Blank body or an empty array → no more pages. [ -n "$_resp" ] || break _count="$(printf '%s' "$_resp" | jq 'length')" || return 1 + [ "$_count" -eq 0 ] && break _acc="$(printf '%s\n%s' "$_acc" "$_resp" | jq -s 'add')" || return 1 - [ "$_count" -lt "$GITEA_PAGE_LIMIT" ] && break + # A server whose max_response_items is tuned below GITEA_PAGE_LIMIT + # truncates every page to its own cap, not the requested limit — so + # stopping when a page is shorter than the *requested* limit would break + # after page 1 even though more pages exist. Compare against the size + # actually observed on the first page instead. + [ -z "$_page_size" ] && _page_size="$_count" + [ "$_count" -lt "$_page_size" ] && break _page=$((_page + 1)) done printf '%s' "$_acc" diff --git a/packages/codev/scripts/forge/gitea/issue-comment.sh b/packages/codev/scripts/forge/gitea/issue-comment.sh index addf6ccc0..f01a5a4c5 100755 --- a/packages/codev/scripts/forge/gitea/issue-comment.sh +++ b/packages/codev/scripts/forge/gitea/issue-comment.sh @@ -4,5 +4,7 @@ # Output: exit code only # # `tea issues` has no `comment` subcommand (its subcommands are list/create/ -# edit/close). Commenting lives under the top-level `tea comments add`. -exec tea comments add "$CODEV_ISSUE_ID" "$CODEV_COMMENT_BODY" +# edit/close). `tea comments add` only exists on tea 0.14.2+ and fails with +# "No help topic for comments" on the still-current 0.14.1 release. The +# top-level `tea comment` shorthand works on both 0.14.1 and 0.14.2+. +exec tea comment "$CODEV_ISSUE_ID" "$CODEV_COMMENT_BODY" diff --git a/packages/codev/scripts/forge/gitea/pr-exists.sh b/packages/codev/scripts/forge/gitea/pr-exists.sh index 9cc437d3a..ef53f75f0 100755 --- a/packages/codev/scripts/forge/gitea/pr-exists.sh +++ b/packages/codev/scripts/forge/gitea/pr-exists.sh @@ -24,6 +24,10 @@ # block a porch pr_exists gate — tea_api_paged walks every page (see _lib.sh). . "$(dirname "$0")/_lib.sh" REPO="$(gitea_repo)" || exit 1 -tea_api_paged "repos/${REPO}/pulls" "state=all" \ - | jq --arg branch "$CODEV_BRANCH_NAME" \ - '[.[] | select(.head.ref == $branch and (.state == "open" or .merged == true))] | length > 0' +# Capture the paginator's output before piping to jq: in POSIX sh (no +# pipefail), a `cmd | jq` pipeline reports jq's exit status (0) even when +# `cmd` failed mid-walk, which would surface a real error as a false-negative +# "no PR exists" and silently pass a porch pr_exists gate. +PULLS="$(tea_api_paged "repos/${REPO}/pulls" "state=all")" || exit 1 +printf '%s' "$PULLS" | jq --arg branch "$CODEV_BRANCH_NAME" \ + '[.[] | select(.head.ref == $branch and (.state == "open" or .merged == true))] | length > 0' diff --git a/packages/codev/scripts/forge/gitea/pr-list.sh b/packages/codev/scripts/forge/gitea/pr-list.sh index 73c84f4be..e63f13fe8 100755 --- a/packages/codev/scripts/forge/gitea/pr-list.sh +++ b/packages/codev/scripts/forge/gitea/pr-list.sh @@ -26,8 +26,12 @@ # at ~50 open PRs (see _lib.sh). . "$(dirname "$0")/_lib.sh" REPO="$(gitea_repo)" || exit 1 -tea_api_paged "repos/${REPO}/pulls" "state=open" \ - | jq '[.[] | { +# Capture the paginator's output before piping to jq: in POSIX sh (no +# pipefail), a `cmd | jq` pipeline reports jq's exit status (0) even when +# `cmd` failed mid-walk, which would silently truncate the list instead of +# surfacing the error. +PULLS="$(tea_api_paged "repos/${REPO}/pulls" "state=open")" || exit 1 +printf '%s' "$PULLS" | jq '[.[] | { number, title, url: (.html_url // .url), diff --git a/packages/codev/scripts/forge/gitea/recently-merged.sh b/packages/codev/scripts/forge/gitea/recently-merged.sh index 7f134ce47..d5ae61e3c 100755 --- a/packages/codev/scripts/forge/gitea/recently-merged.sh +++ b/packages/codev/scripts/forge/gitea/recently-merged.sh @@ -18,8 +18,12 @@ # past the first page — tea_api_paged walks every page (see _lib.sh). . "$(dirname "$0")/_lib.sh" REPO="$(gitea_repo)" || exit 1 -tea_api_paged "repos/${REPO}/pulls" "state=closed" \ - | jq '[.[] | select(.merged == true) | { +# Capture the paginator's output before piping to jq: in POSIX sh (no +# pipefail), a `cmd | jq` pipeline reports jq's exit status (0) even when +# `cmd` failed mid-walk, which would silently truncate the list instead of +# surfacing the error. +PULLS="$(tea_api_paged "repos/${REPO}/pulls" "state=closed")" || exit 1 +printf '%s' "$PULLS" | jq '[.[] | select(.merged == true) | { number, title, url: (.html_url // .url), diff --git a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts index 53bdf9a87..bbf257a26 100644 --- a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts +++ b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts @@ -19,10 +19,22 @@ * - `issue-view` warns on stderr when the comments fetch degrades to []. * * `tea` isn't available in CI (see the in-repo #920 note), so this test stubs a - * fake `tea` on PATH that answers `api ` (and `comments add`) with + * fake `tea` on PATH that answers `api ` (and `comment`) with * captured Gitea REST fixtures, points the scripts at a throwaway git repo with * a gitea remote, runs each real script, and asserts the normalized output * conforms to the contract in forge-contracts.ts. + * + * Integration-review follow-up (2026-08-17, amrmelsayed): + * - `issue-comment` now calls the top-level `tea comment` shorthand rather + * than `tea comments add`, which only exists on tea 0.14.2+ and fails on + * the still-current 0.14.1 release. + * - `pr-exists`/`pr-list`/`recently-merged` now capture `tea_api_paged`'s + * output before piping to jq, so a mid-walk failure exits non-zero instead + * of surfacing as jq's exit-0-on-empty-stdin. + * - `tea_api_paged`'s stop condition now compares against the page size + * actually observed on page 1, not the requested limit, so a server whose + * `max_response_items` is tuned below the requested limit doesn't stop + * after page 1 while more pages remain. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; @@ -48,8 +60,9 @@ const giteaDir = resolve(__dirname, '..', '..', 'scripts', 'forge', 'gitea'); // scripts walk past the server's page cap: each carries a "signature" item plus // filler, and a distinct item that lives ONLY on page 2. const FAKE_TEA = `#!/bin/sh -if [ "$1" = "comments" ] && [ "$2" = "add" ]; then - # comments add +if [ "$1" = "comment" ]; then + # comment (the tea 0.14.1-compatible shorthand for + # \`tea comments add\`, which only exists on 0.14.2+) echo "commented" exit 0 fi @@ -60,6 +73,35 @@ case "$2" in repos/acme/widgets/pulls/42) echo '{"number":42,"title":"Add widget","body":"PR body","state":"open","html_url":"https://git.example.com/acme/widgets/pulls/42","url":"https://git.example.com/api/v1/repos/acme/widgets/pulls/42","user":{"login":"alice"},"base":{"ref":"main"},"head":{"ref":"feature/x"},"additions":10,"deletions":3}' ;; + # --- mid-walk pagination failure: page 1 is a full 50 items (so the + # paginator commits to a page 2), page 2 errors. Used to prove pr-exists/ + # pr-list/recently-merged exit non-zero instead of silently succeeding with + # a truncated/empty result (the jq-exit-status-masks-a-failed-pipe bug). --- + "repos/acme/failing/pulls?state=all&limit=50&page=1") + jq -cn '[range(50)|{number:(3000+.),state:"open",merged:false,head:{ref:("pad-"+(.|tostring))}}]' ;; + "repos/acme/failing/pulls?state=all&limit=50&page=2") + echo "fake-tea: page 2 unavailable" >&2; exit 9 ;; + "repos/acme/failing/pulls?state=open&limit=50&page=1") + jq -cn '[range(50)|{number:(3000+.),title:"pad",html_url:"u",body:"",state:"open",created_at:"d",user:{login:"pad"},requested_reviewers:[],draft:false}]' ;; + "repos/acme/failing/pulls?state=open&limit=50&page=2") + echo "fake-tea: page 2 unavailable" >&2; exit 9 ;; + "repos/acme/failing/pulls?state=closed&limit=50&page=1") + jq -cn '[range(50)|{number:(3000+.),title:"pad",state:"closed",merged:false,head:{ref:"pad"}}]' ;; + "repos/acme/failing/pulls?state=closed&limit=50&page=2") + echo "fake-tea: page 2 unavailable" >&2; exit 9 ;; + + # --- sub-limit server cap: max_response_items tuned to 30 (below the + # requested limit of 50), so EVERY page — including the last — is capped at + # 30. Three pages: 30 + 30 + 6 (66 total). Proves the paginator keeps + # walking past a full-but-capped page instead of stopping after page 1 + # because 30 < the requested 50. --- + "repos/acme/capped/pulls?state=open&limit=50&page=1") + jq -cn '[range(30)|{number:(4000+.),title:"pad",html_url:"u",body:"",state:"open",created_at:"d",user:{login:"pad"},requested_reviewers:[],draft:false}]' ;; + "repos/acme/capped/pulls?state=open&limit=50&page=2") + jq -cn '[range(30)|{number:(4100+.),title:"pad",html_url:"u",body:"",state:"open",created_at:"d",user:{login:"pad"},requested_reviewers:[],draft:false}]' ;; + "repos/acme/capped/pulls?state=open&limit=50&page=3") + jq -cn '[range(5)|{number:(4200+.),title:"pad",html_url:"u",body:"",state:"open",created_at:"d",user:{login:"pad"},requested_reviewers:[],draft:false}] + [{number:4299,title:"Last capped page item",html_url:"u",body:"",state:"open",created_at:"d",user:{login:"pad"},requested_reviewers:[],draft:false}]' ;; + # --- pr-exists: state=all, paginated ------------------------------------- # page 1 = 50 items (open feature/x, merged feature/done, closed-not-merged # feature/abandoned, + 47 open pad). page 2 = 1 merged item on feature/deep. @@ -282,12 +324,48 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` }); }); - it('issue-comment uses `tea comments add` and exits 0', () => { + it('issue-comment uses `tea comment` (not the 0.14.2-only `tea comments add`) and exits 0', () => { // Would exit non-zero (throwing) if it invoked the non-existent - // `tea issues comment` subcommand. + // `tea issues comment` subcommand, or the 0.14.2+-only `tea comments add`. expect(runScript('issue-comment.sh', { CODEV_ISSUE_ID: '99', CODEV_COMMENT_BODY: 'hi' })).toBe('commented'); }); + it('pr-exists exits non-zero (not "false") on a mid-walk pagination failure', () => { + // Page 1 is a full 50 items so the paginator commits to fetching page 2, + // which the fixture makes error. Without capturing tea_api_paged's output + // before the jq pipe, POSIX sh (no pipefail) reports jq's exit status (0) + // on empty stdin, which prints "false" — a silent false-negative that + // would pass a porch pr_exists gate instead of surfacing the failure. + const { status, stdout } = runScriptFull('pr-exists.sh', { + CODEV_BRANCH_NAME: 'whatever', + CODEV_REPO: 'acme/failing', + }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + }); + + it('pr-list exits non-zero on a mid-walk pagination failure', () => { + const { status, stdout } = runScriptFull('pr-list.sh', { CODEV_REPO: 'acme/failing' }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + }); + + it('recently-merged exits non-zero on a mid-walk pagination failure', () => { + const { status, stdout } = runScriptFull('recently-merged.sh', { CODEV_REPO: 'acme/failing' }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + }); + + it('pr-list keeps walking past a page capped below the requested limit', () => { + // Server's max_response_items is tuned to 30 (below the requested 50), so + // every page — including the last — returns exactly 30 or fewer. Stopping + // when a page is shorter than the *requested* limit (50) would break after + // page 1, even though pages 2 and 3 carry real items. + const list = JSON.parse(runScript('pr-list.sh', { CODEV_REPO: 'acme/capped' })); + expect(list).toHaveLength(66); // 30 + 30 + 6 + expect(list.some((p: { number: number }) => p.number === 4299)).toBe(true); + }); + it('CODEV_REPO overrides the git-remote-derived owner/repo', () => { // A repo whose remote does NOT resolve to acme/widgets still works when // CODEV_REPO is supplied explicitly (the repo-archive-style callers). From 481956e129ac1df61ccd8b8a64128ca6b78b443e Mon Sep 17 00:00:00 2001 From: pseudo Date: Thu, 20 Aug 2026 19:41:35 -0600 Subject: [PATCH 09/14] [Bugfix #1137] Builder thread: log CMAP review follow-up session Co-Authored-By: Claude Sonnet 5 --- codev/state/bugfix-1137_thread.md | 57 +++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 codev/state/bugfix-1137_thread.md diff --git a/codev/state/bugfix-1137_thread.md b/codev/state/bugfix-1137_thread.md new file mode 100644 index 000000000..c7689dfc6 --- /dev/null +++ b/codev/state/bugfix-1137_thread.md @@ -0,0 +1,57 @@ +# bugfix-1137 — gitea forge preset broken against real `tea` CLI + +This worktree was resumed with prior work already merged in (see +`codev/state/task-24AO_thread.md` for the rebase/reconcile history): PR #1146 +already has the core fix — routing gitea forge reads through `tea api` +instead of `tea list/view`, plus pagination, factored repo +derivation, and degraded-comments warnings. That PR was open, mergeable, and +had a prior maintainer review (waleedkadous, 2026-08-02) whose one blocking +item (pagination) was already addressed in an earlier commit. + +## This session: CMAP integration review follow-up (2026-08-20) + +The architect forwarded a second review (2026-08-17, amrmelsayed) on PR +#1146: REQUEST_CHANGES, 2 blocking + 1 strongly-recommended (+1 deferred) +item. Addressed in commit `c9f55a537`: + +1. **(blocking) `issue-comment.sh`**: was calling `tea comments add`, which + only exists on tea 0.14.2+ and errors on the still-current 0.14.1 release. + Switched to the `tea comment ` shorthand, which works on both. +2. **(blocking) masked pipe failures**: `pr-exists.sh`, `pr-list.sh`, + `recently-merged.sh` piped `tea_api_paged | jq` directly. POSIX sh has no + `pipefail`, so a mid-walk pagination failure was masked by jq's exit + status (0 on empty stdin) — `pr-exists` in particular would silently + report `"false"` for a real error, which could pass a porch `pr_exists` + gate on a false negative. Fixed by capturing the paginator's output into a + variable and checking its exit status before piping to jq. +3. **(strongly recommended, done) sub-limit server cap**: `tea_api_paged`'s + stop condition compared each page's count against the *requested* limit + (50). A server whose `max_response_items` is tuned below that truncates + every page — including non-last ones — to its own cap, so every page + looked "short" and the loop broke after page 1. Now compares against the + size actually observed on page 1 instead. +4. **(deferred, not done)**: a `# forge-executable: tea` header convention on + the sourced scripts. Depends on #1458 landing `extractExecutable`'s header + convention first — checked, #1458 is still open/unmerged, so this isn't + actionable yet. Left as a follow-up once #1458 merges (matches the + reviewer's own stated merge order: #1458 first, then #1146). + +Added regression tests for all three fixed items (mid-walk pagination +failure fixtures for all three paginated scripts, a 3-page sub-50-cap +fixture, and an updated `tea comment` stub). Full local suite: 3197 passed, +126 failed — same 67 pre-existing environment-dependent failures (agent-farm/ +terminal/consolidate, no built `dist/` in this worktree) as the unmodified +baseline reported by the previous builder session. Zero regressions, +4 new +passing tests over the pre-session count of 3193. + +Pushed to `builder/bugfix-1137` (`origin` = pseudoseed fork); PR #1146 +updated. Notified the architect. + +## Note on porch state + +`porch status` shows phase `fix` with build/tests not yet run, and a stale +`gates: {merge-approval: pending}` in status.yaml predating this session +(from before the worktree was resumed with the existing PR history merged +in). Since PR #1146 already exists and is the live artifact, further phase +progression should go through the architect/porch flow rather than assuming +gate state — flagged in the notification to the architect. From cfd858e7a13a20db80936aacbf339c6cea5d383a Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Thu, 3 Sep 2026 21:23:30 -0700 Subject: [PATCH 10/14] =?UTF-8?q?[Bugfix=20#1137]=20Fix:=20maintainer=20re?= =?UTF-8?q?view=20=E2=80=94=20fail=20loudly=20on=20error=20bodies,=20page?= =?UTF-8?q?=20ceiling,=20and=20bound=20recently-merged?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer follow-up on PR #1146, pushed onto the contributor's branch. All four required items from the 2026-09-03 review, in the same spirit as the fix itself: fail loudly, never silently. 1. `# forge-executable: tea` headers. `codev doctor` infers the CLI a concept needs from the script's first substantive line; the five scripts that source `_lib.sh` open with `.`, so doctor reported `.`/`tea_api_paged` as missing tools and stopped checking for `tea`. The header (mechanism in #1458, inert comment until then) declares it. `user-identity.sh` gets one too: fixing its exit-0-on-error handling below moves `tea` off the first substantive line, so without the header that fix would have caused the very regression this item closes. 2. The paginator fails at the `GITEA_MAX_PAGES` ceiling. Reaching 100 pages with no terminal short/empty page means we do not know we have the whole list; returning the partial array at exit 0 was the silent truncation the paginator exists to prevent — a short `pr-exists` walk reads as "no PR exists" and passes a porch pr_exists gate on a repo we merely failed to finish reading. 3. `pr-view`, `user-identity` and `issue-view` type-check the response before normalizing. `tea api` exits 0 on HTTP errors and prints the error body, which carries a `url` (the swagger link) — so `url: (.html_url // .url)` succeeded on it and shipped that link as the PR's browser page inside an otherwise all-null contract object; `user-identity` printed the literal username "null". They now fail with the server's own message on stderr. `issue-view` is validated before its comments are fetched, so a bad id reports only its own error, and its comments degrade path now tests for an actual JSON array — an error OBJECT used to reach `--argjson` and blow up with a raw jq error instead of the warned [] degrade. 4. `recently-merged` honors `CODEV_SINCE_DATE`. It feeds a 24h analytics window but walked the repo's entire merge history inside forge's 30s timeout, and a timeout yields `null` — worse than truncation. It now asks for `sort=recentupdate` and stops at the first page reaching back past the cutoff. The stop filter refuses to trust the sort blindly: it fires only when the page is actually non-increasing in `updated_at`, so a server that ignores the parameter falls back to the full walk rather than silently dropping merges. `updated_at >= merged_at` always holds, so nothing merged after the cutoff can sit beyond that page. Timestamps go through a new `gitea_epoch` jq helper in `_lib.sh`: Gitea marshals RFC3339 in the server's timezone, so `+02:00` is a real response and `Z` is not guaranteed — `fromdateiso8601` rejects those and a lexicographic compare across mixed offsets is wrong. It also accepts the bare `YYYY-MM-DD` that `team-update.ts` passes. Unparseable input yields null and every caller treats null as "don't know": keep the item, keep walking. Also, from the review's take-or-leave list: document `CODEV_REPO`'s two meanings (repo-archive input vs. gitea read-target override) in `forge.md` and both `SKILL.md` copies, and correct `forge-contracts.ts`'s `reviewRequests`/`isDraft` comments — both claimed GitLab and Gitea emit empty/false, but all three presets populate them for real. Tests: 12 new cases in the existing real-script fake-CLI suite. The fake `tea` grew error bodies at exit 0 for pulls/issues/user, a comments endpoint answering with an error object, a repo whose pages never end, and sorted/unsorted since-date repos. 33 tests in the file; full suite 4886 passed | 48 skipped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch --- .claude/skills/forge/SKILL.md | 2 + .codex/skills/forge/SKILL.md | 2 + codev/resources/commands/forge.md | 10 + packages/codev/scripts/forge/gitea/_lib.sh | 86 +++++- .../codev/scripts/forge/gitea/issue-view.sh | 43 ++- .../codev/scripts/forge/gitea/pr-exists.sh | 1 + packages/codev/scripts/forge/gitea/pr-list.sh | 1 + packages/codev/scripts/forge/gitea/pr-view.sh | 30 ++- .../scripts/forge/gitea/recently-merged.sh | 49 +++- .../scripts/forge/gitea/user-identity.sh | 18 +- .../bugfix-1137-gitea-tea-api.test.ts | 248 +++++++++++++++++- packages/codev/src/lib/forge-contracts.ts | 14 +- 12 files changed, 481 insertions(+), 23 deletions(-) diff --git a/.claude/skills/forge/SKILL.md b/.claude/skills/forge/SKILL.md index be2c2062f..0340aa2bf 100644 --- a/.claude/skills/forge/SKILL.md +++ b/.claude/skills/forge/SKILL.md @@ -46,6 +46,8 @@ Built-in presets: `github` (default), `gitlab` (via `glab`), `gitea` (via `tea`) **Note:** Non-GitHub presets are best-effort. Output schemas may differ from GitHub's JSON contracts. Non-conforming JSON returns `null` — consumers handle this gracefully. Override individual concepts if a preset doesn't match your CLI version. +**`CODEV_REPO` is overloaded.** For `repo-archive` it is an input: the `owner/repo` to download. For the Gitea preset's read concepts (`pr-view`, `pr-list`, `pr-exists`, `issue-view`, `recently-merged`) it is an override — `tea api` needs an explicit `owner/repo` in the endpoint path, which those scripts otherwise derive from the `origin` remote. Set it per invocation, not in your environment: exported globally it retargets every Gitea read at that repo. + ### Disabling concepts Set a concept to `null` to disable it: diff --git a/.codex/skills/forge/SKILL.md b/.codex/skills/forge/SKILL.md index be2c2062f..0340aa2bf 100644 --- a/.codex/skills/forge/SKILL.md +++ b/.codex/skills/forge/SKILL.md @@ -46,6 +46,8 @@ Built-in presets: `github` (default), `gitlab` (via `glab`), `gitea` (via `tea`) **Note:** Non-GitHub presets are best-effort. Output schemas may differ from GitHub's JSON contracts. Non-conforming JSON returns `null` — consumers handle this gracefully. Override individual concepts if a preset doesn't match your CLI version. +**`CODEV_REPO` is overloaded.** For `repo-archive` it is an input: the `owner/repo` to download. For the Gitea preset's read concepts (`pr-view`, `pr-list`, `pr-exists`, `issue-view`, `recently-merged`) it is an override — `tea api` needs an explicit `owner/repo` in the endpoint path, which those scripts otherwise derive from the `origin` remote. Set it per invocation, not in your environment: exported globally it retargets every Gitea read at that repo. + ### Disabling concepts Set a concept to `null` to disable it: diff --git a/codev/resources/commands/forge.md b/codev/resources/commands/forge.md index 478c2db0e..6c97d748e 100644 --- a/codev/resources/commands/forge.md +++ b/codev/resources/commands/forge.md @@ -137,6 +137,16 @@ Some Gitea concepts (`pr-search`, `pr-diff`) are disabled by default since `tea` } ``` +**`CODEV_REPO` means two different things.** For `repo-archive` it is an *input*: the +`owner/repo` of the foreign repository to download, supplied by the caller. For the Gitea +preset's read concepts (`pr-view`, `pr-list`, `pr-exists`, `issue-view`, `recently-merged`) it +is an *override*: `tea api` needs an explicit `owner/repo` in the endpoint path, and those +scripts default to deriving it from the `origin` remote, so `CODEV_REPO` only comes into play +when that derivation is wrong or unavailable (a worktree with no `origin`, or reading a +different repo than the checkout). The scripts fail fast with a message naming `CODEV_REPO` +rather than issuing a `repos//…` request. Exporting `CODEV_REPO` globally therefore retargets +every Gitea read at that repo — set it per invocation instead. + ### Custom Forge (any platform) For unsupported platforms, configure each concept individually: diff --git a/packages/codev/scripts/forge/gitea/_lib.sh b/packages/codev/scripts/forge/gitea/_lib.sh index 594587826..6c9790f3a 100755 --- a/packages/codev/scripts/forge/gitea/_lib.sh +++ b/packages/codev/scripts/forge/gitea/_lib.sh @@ -5,6 +5,10 @@ # scripts are #!/bin/sh and forge runs them via `sh -c`. It is not a forge # concept: forge.ts builds presets from an explicit KNOWN_CONCEPTS allowlist, so # a leading-underscore file in this directory is never registered as a concept. +# +# Sibling-file dependency: the five concept scripts that source this file need +# it to sit next to them. A hand-copied override in `.codev/scripts/forge/gitea/` +# must copy `_lib.sh` alongside the script, or the `.` line fails. # Resolve owner/repo for the `tea api` path. # @@ -32,6 +36,42 @@ gitea_repo() { printf '%s' "$_repo" } +# A jq prelude defining `gitea_epoch`: parse a timestamp to epoch seconds, or +# null if it isn't one. Prepend it to a jq program that needs to compare times: +# jq "${GITEA_JQ_LIB} " +# +# Two input shapes, because two different producers feed it: +# - Gitea's RFC3339 response times. Gitea marshals them in the SERVER's +# timezone, so `Z` is NOT guaranteed — `2026-07-05T14:00:00+02:00` is a real +# response. `fromdateiso8601` only accepts `Z`, and a lexicographic compare +# across mixed offsets is simply wrong, so the offset is parsed and +# subtracted explicitly. +# - CODEV_SINCE_DATE, which callers set to either a full timestamp +# (`github.ts`) or a bare `YYYY-MM-DD` (`team-update.ts`). A bare date is +# read as midnight UTC. +# +# Unparseable input yields null, and every caller treats null as "don't know" — +# keep the item, keep walking. A surprising format therefore degrades to the old +# unbounded behavior rather than silently dropping data. +GITEA_JQ_LIB=' +def gitea_epoch: + if type == "string" then + ((capture("^(?\\d{4}-\\d{2}-\\d{2})(T(?\\d{2}:\\d{2}:\\d{2})(\\.\\d+)?(?Z|[+-]\\d{2}:\\d{2})?)?$")) // null) as $c + | if $c == null then null + # `try`: the regex only proves the SHAPE. `2026-13-99` matches it and then + # makes `fromdateiso8601` throw, which would abort the script with a raw + # jq error instead of degrading to "unknown time". + else (try (($c.d + "T" + ($c.t // "00:00:00") + "Z") | fromdateiso8601) catch null) as $e + | if $e == null then null + elif ($c.o == null or $c.o == "Z") then $e + else ($c.o | capture("^(?[+-])(?\\d{2}):(?\\d{2})$")) as $z + | $e - (((($z.h | tonumber) * 3600) + (($z.m | tonumber) * 60)) + * (if $z.s == "+" then 1 else -1 end)) + end + end + else null end; +' + # Page size to request per page. Gitea caps list responses at the server's # `max_response_items` (default 50), so `&limit=200` silently truncates to ~50 # with no client-side pagination. Requesting 50 matches that default cap; a @@ -40,25 +80,39 @@ GITEA_PAGE_LIMIT=50 # Hard ceiling on pages fetched, so a misbehaving server that never returns a # short page can't spin forever. 100 pages × 50 = 5000 items — far beyond any -# real open-PR / recently-merged / all-pulls window we page over. +# real open-PR / recently-merged / all-pulls window we page over. Reaching it is +# an ERROR, not a stop condition (see below). GITEA_MAX_PAGES=100 # Fetch a paginated Gitea list endpoint and emit ONE concatenated JSON array on # stdout, so the caller's existing jq normalizer sees the same shape as before. # -# Usage: tea_api_paged "repos///pulls" "state=all" +# Usage: tea_api_paged "repos///pulls" "state=all" [""] # $1 = API path (no page params) # $2 = extra query string (may be empty), e.g. "state=open" +# $3 = optional jq program run on each page's array; when it outputs `true` +# the walk stops after that page. Used by `recently-merged` to bound the +# walk with CODEV_SINCE_DATE. It must be conservative: a false negative +# just costs another page, a false positive silently truncates. # # Loops page=1,2,3… appending "&limit=&page=", concatenates each page's # array, and stops when a page returns fewer than the requested limit (the last -# page) or an empty/blank response, bounded by GITEA_MAX_PAGES. +# page), an empty/blank response, or the caller's stop filter fires. +# +# Reaching GITEA_MAX_PAGES without any of those terminal conditions means we do +# NOT know we have the whole list. Returning the partial array at exit 0 would +# be exactly the silent-truncation class this paginator exists to prevent (a +# short `pr-exists` walk reads as "no PR exists" and passes a porch pr_exists +# gate on a repo we simply failed to finish reading), so it fails loudly +# instead: stderr message, non-zero return, no stdout. tea_api_paged() { _path="$1" _query="$2" + _stop="$3" _page=1 _acc='[]' _page_size='' + _terminal='' while [ "$_page" -le "$GITEA_MAX_PAGES" ]; do if [ -n "$_query" ]; then _url="${_path}?${_query}&limit=${GITEA_PAGE_LIMIT}&page=${_page}" @@ -67,18 +121,38 @@ tea_api_paged() { fi _resp="$(tea api "$_url")" || return 1 # Blank body or an empty array → no more pages. - [ -n "$_resp" ] || break + if [ -z "$_resp" ]; then + _terminal=1 + break + fi _count="$(printf '%s' "$_resp" | jq 'length')" || return 1 - [ "$_count" -eq 0 ] && break + if [ "$_count" -eq 0 ]; then + _terminal=1 + break + fi _acc="$(printf '%s\n%s' "$_acc" "$_resp" | jq -s 'add')" || return 1 + if [ -n "$_stop" ]; then + _hit="$(printf '%s' "$_resp" | jq "$_stop")" || return 1 + if [ "$_hit" = "true" ]; then + _terminal=1 + break + fi + fi # A server whose max_response_items is tuned below GITEA_PAGE_LIMIT # truncates every page to its own cap, not the requested limit — so # stopping when a page is shorter than the *requested* limit would break # after page 1 even though more pages exist. Compare against the size # actually observed on the first page instead. [ -z "$_page_size" ] && _page_size="$_count" - [ "$_count" -lt "$_page_size" ] && break + if [ "$_count" -lt "$_page_size" ]; then + _terminal=1 + break + fi _page=$((_page + 1)) done + if [ -z "$_terminal" ]; then + echo "gitea forge: pagination for '${_path}' reached the ${GITEA_MAX_PAGES}-page ceiling without a terminal page; refusing to return a truncated result" >&2 + return 1 + fi printf '%s' "$_acc" } diff --git a/packages/codev/scripts/forge/gitea/issue-view.sh b/packages/codev/scripts/forge/gitea/issue-view.sh index b9350d0bf..9de942c74 100755 --- a/packages/codev/scripts/forge/gitea/issue-view.sh +++ b/packages/codev/scripts/forge/gitea/issue-view.sh @@ -1,5 +1,6 @@ #!/bin/sh # Forge concept: issue-view (Gitea via tea CLI) +# forge-executable: tea # Input: CODEV_ISSUE_ID # Output: JSON {title, body, state, url, comments[]} (IssueViewResult) # @@ -16,19 +17,47 @@ # # Gitea's issue object reports `comments` as an integer count, not the array # the contract requires (consumers call `.comments.filter(...)`), so the -# comments array is fetched separately and merged in. A failed/empty comments -# fetch degrades to [], but warns on stderr so the degraded path is -# distinguishable from a genuinely uncommented issue (stdout stays pure JSON — -# it's parsed by forge.ts). +# comments array is fetched separately and merged in. A failed comments fetch +# degrades to [], but warns on stderr so the degraded path is distinguishable +# from a genuinely uncommented issue (stdout stays pure JSON — it's parsed by +# forge.ts). `tea api` exits 0 on HTTP errors and prints the error BODY, so the +# degrade check tests for an actual JSON array rather than only for a blank +# response — an error OBJECT reached `--argjson` and blew up with a raw jq +# parse/iteration error instead of the warned [] degrade. +# +# SHAPE VALIDATION. Same exit-0-on-error problem for the issue itself: an error +# body normalized into an all-null IssueViewResult whose `url` was the error +# body's own `url`. Required fields are type-checked before normalizing and +# anything else is a hard failure carrying the server's message on stderr. . "$(dirname "$0")/_lib.sh" REPO="$(gitea_repo)" || exit 1 +# Fetch and validate the issue BEFORE its comments, so a bad issue id reports +# only its own error instead of preceding it with a comments-degrade warning +# about an issue that doesn't exist — and doesn't spend a request on it. +# Capture before piping: POSIX sh has no pipefail, so `tea api … | jq` would +# report jq's exit status rather than a failed fetch. +ISSUE="$(tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}")" || exit 1 +printf '%s' "$ISSUE" | jq -e ' + if (type == "object") + and ((.title | type) == "string") + and ((.state | type) == "string") + and (((.html_url // .url) | type) == "string") + and ((.number | type) == "number") + then . + else + ("gitea forge: unexpected `tea api` response for issue " + + (env.CODEV_ISSUE_ID // "?") + ": " + + (if type == "object" then (.message // tostring) else tostring end) + + "\n") | halt_error(1) + end' >/dev/null || exit 1 + COMMENTS_JSON="$(tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}/comments" 2>/dev/null)" -if [ -z "$COMMENTS_JSON" ]; then +if [ -z "$COMMENTS_JSON" ] || ! printf '%s' "$COMMENTS_JSON" | jq -e 'type == "array"' >/dev/null 2>&1; then echo "gitea forge: comments fetch failed for issue ${CODEV_ISSUE_ID}; reporting 0 comments" >&2 COMMENTS_JSON="[]" fi -tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}" \ - | jq --argjson comments "$COMMENTS_JSON" '{ + +printf '%s' "$ISSUE" | jq --argjson comments "$COMMENTS_JSON" '{ title, body: (.body // ""), state, diff --git a/packages/codev/scripts/forge/gitea/pr-exists.sh b/packages/codev/scripts/forge/gitea/pr-exists.sh index ef53f75f0..80178520d 100755 --- a/packages/codev/scripts/forge/gitea/pr-exists.sh +++ b/packages/codev/scripts/forge/gitea/pr-exists.sh @@ -1,5 +1,6 @@ #!/bin/sh # Forge concept: pr-exists (Gitea via tea CLI) +# forge-executable: tea # Input: CODEV_BRANCH_NAME # Output: "true" or "false" # diff --git a/packages/codev/scripts/forge/gitea/pr-list.sh b/packages/codev/scripts/forge/gitea/pr-list.sh index e63f13fe8..23e293ee7 100755 --- a/packages/codev/scripts/forge/gitea/pr-list.sh +++ b/packages/codev/scripts/forge/gitea/pr-list.sh @@ -1,5 +1,6 @@ #!/bin/sh # Forge concept: pr-list (Gitea via tea CLI) — open pulls +# forge-executable: tea # Output: JSON [{number, title, url, reviewDecision, body, createdAt, author, # reviewRequests, isDraft}] (PrListItem in forge-contracts.ts) # diff --git a/packages/codev/scripts/forge/gitea/pr-view.sh b/packages/codev/scripts/forge/gitea/pr-view.sh index bd1cbbbdf..5199c7b7b 100755 --- a/packages/codev/scripts/forge/gitea/pr-view.sh +++ b/packages/codev/scripts/forge/gitea/pr-view.sh @@ -1,5 +1,6 @@ #!/bin/sh # Forge concept: pr-view (Gitea via tea CLI) +# forge-executable: tea # Input: CODEV_PR_NUMBER # Output: JSON {title, body, state, url, author{login}, baseRefName, headRefName, # additions, deletions} (see PrViewResult in forge-contracts.ts) @@ -15,9 +16,36 @@ # API endpoint (would render raw JSON in a browser), so map `html_url` and fall # back to `url` only if it's absent — the same choice PIR #1179 made when this # concept still went through `tea pulls view`. +# +# SHAPE VALIDATION. `tea api` exits 0 on an HTTP error and prints the error +# BODY, e.g. {"message":"pull does not exist [index: 42]","url":"…/swagger"}. +# Normalizing that unchecked produced a structurally valid, entirely wrong +# contract object at exit 0 — every field null except `url`, which took the +# error body's own `url` (the swagger link) and shipped it to callers as the +# PR's browser page. Required fields are type-checked first and anything else +# is a hard failure carrying the server's message on stderr. . "$(dirname "$0")/_lib.sh" REPO="$(gitea_repo)" || exit 1 -tea api "repos/${REPO}/pulls/${CODEV_PR_NUMBER}" | jq '{ +# Capture before piping: in POSIX sh (no pipefail) a `tea api … | jq` pipeline +# reports jq's exit status, so a failed fetch would surface as jq's exit-0 on +# empty stdin rather than an error. +PR="$(tea api "repos/${REPO}/pulls/${CODEV_PR_NUMBER}")" || exit 1 +printf '%s' "$PR" | jq ' + if (type == "object") + and ((.number | type) == "number") + and ((.title | type) == "string") + and ((.state | type) == "string") + and ((.user.login | type) == "string") + and ((.base.ref | type) == "string") + and ((.head.ref | type) == "string") + then . + else + ("gitea forge: unexpected `tea api` response for pull " + + (env.CODEV_PR_NUMBER // "?") + ": " + + (if type == "object" then (.message // tostring) else tostring end) + + "\n") | halt_error(1) + end + | { title, body: (.body // ""), state, diff --git a/packages/codev/scripts/forge/gitea/recently-merged.sh b/packages/codev/scripts/forge/gitea/recently-merged.sh index d5ae61e3c..f5283550d 100755 --- a/packages/codev/scripts/forge/gitea/recently-merged.sh +++ b/packages/codev/scripts/forge/gitea/recently-merged.sh @@ -1,5 +1,7 @@ #!/bin/sh # Forge concept: recently-merged (Gitea via tea CLI) +# forge-executable: tea +# Input: CODEV_SINCE_DATE (optional, ISO-8601 timestamp) # Output: JSON [{number, title, url, body, createdAt, mergedAt, headRefName}] # (MergedPrItem in forge-contracts.ts) # @@ -16,14 +18,57 @@ # The closed-pulls list is paginated (Gitea caps a page at max_response_items, # default 50), so on a busy repo the most-recent merges could push older ones # past the first page — tea_api_paged walks every page (see _lib.sh). +# +# CODEV_SINCE_DATE BOUNDS THE WALK. This concept feeds a 24h analytics window, +# but the closed-pulls list is the repo's whole merge history: on an +# established repo an unbounded walk issues up to GITEA_MAX_PAGES sequential +# requests inside forge's 30s timeout, and a timeout yields `null` — a worse +# outcome for the dashboard than truncation was. When CODEV_SINCE_DATE is set +# we ask the server for update-time-descending order and stop at the first page +# that reaches back past the cutoff. +# +# The stop filter refuses to trust the sort blindly: it fires only when the +# page is ACTUALLY non-increasing in `updated_at` (proving the server honored +# `sort=recentupdate`) AND some item on it predates the cutoff. A server that +# ignores the parameter falls back to the full walk rather than silently +# dropping merges. `updated_at >= merged_at` always holds — a merge updates the +# PR — so nothing merged after the cutoff can sit beyond the first page whose +# update times have fallen behind it. Timestamps go through `gitea_epoch` +# because Gitea emits RFC3339 in the server's timezone, not necessarily `Z`. . "$(dirname "$0")/_lib.sh" REPO="$(gitea_repo)" || exit 1 + +if [ -n "$CODEV_SINCE_DATE" ]; then + QUERY="state=closed&sort=recentupdate" + STOP="${GITEA_JQ_LIB}"' + [ .[] | (.updated_at | gitea_epoch) ] as $t + | (env.CODEV_SINCE_DATE | gitea_epoch) as $since + | ($since != null) + and (($t | length) > 0) + and ([ $t[] | . != null ] | all) + and ([ range(($t | length) - 1) | $t[.] >= $t[.+ 1] ] | all) + and (($t | min) < $since) + ' +else + QUERY="state=closed" + STOP="" +fi + # Capture the paginator's output before piping to jq: in POSIX sh (no # pipefail), a `cmd | jq` pipeline reports jq's exit status (0) even when # `cmd` failed mid-walk, which would silently truncate the list instead of # surfacing the error. -PULLS="$(tea_api_paged "repos/${REPO}/pulls" "state=closed")" || exit 1 -printf '%s' "$PULLS" | jq '[.[] | select(.merged == true) | { +PULLS="$(tea_api_paged "repos/${REPO}/pulls" "$QUERY" "$STOP")" || exit 1 +printf '%s' "$PULLS" | jq "${GITEA_JQ_LIB}"' + (env.CODEV_SINCE_DATE | gitea_epoch) as $since + | [ .[] + | select(.merged == true) + # Drop merges older than the cutoff. An unparseable/absent timestamp on + # either side keeps the item — the caller filters the window again, so + # over-reporting is harmless where dropping a real merge is not. + | select($since == null + or ((.merged_at | gitea_epoch) as $m | $m == null or $m >= $since)) + | { number, title, url: (.html_url // .url), diff --git a/packages/codev/scripts/forge/gitea/user-identity.sh b/packages/codev/scripts/forge/gitea/user-identity.sh index c296a3c71..a0173ec6c 100755 --- a/packages/codev/scripts/forge/gitea/user-identity.sh +++ b/packages/codev/scripts/forge/gitea/user-identity.sh @@ -1,9 +1,25 @@ #!/bin/sh # Forge concept: user-identity (Gitea via tea CLI) +# forge-executable: tea # Output: plain text username # # `tea whoami` has no `--output json` flag (its only documented option is # --help), so it can't feed a jq pipeline. Route through the raw REST # passthrough instead: `tea api user` returns the Gitea `User` object, whose # `.login` is the authenticated username (mirrors `gh api user --jq .login`). -tea api user | jq -r ".login" +# +# SHAPE VALIDATION. `tea api` exits 0 on an HTTP error and prints the error +# BODY, which has no `.login` — `jq -r .login` then printed the literal string +# "null" at exit 0, and callers took that as the current user's handle. Check +# for a non-empty string login first and fail with the server's message +# otherwise. The response is captured before the pipe because POSIX sh has no +# pipefail: `tea api user | jq` would report jq's status, not tea's. +USER_JSON="$(tea api user)" || exit 1 +printf '%s' "$USER_JSON" | jq -r ' + if (type == "object") and ((.login | type) == "string") and (.login != "") + then .login + else + ("gitea forge: unexpected `tea api user` response: " + + (if type == "object" then (.message // tostring) else tostring end) + + "\n") | halt_error(1) + end' diff --git a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts index bbf257a26..f57af7a94 100644 --- a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts +++ b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts @@ -35,6 +35,21 @@ * actually observed on page 1, not the requested limit, so a server whose * `max_response_items` is tuned below the requested limit doesn't stop * after page 1 while more pages remain. + * + * Maintainer review follow-up (2026-09-03, waleedkadous): + * - the five scripts that source `_lib.sh` (plus `user-identity`, whose fix + * below moves `tea` off the first substantive line) declare + * `# forge-executable: tea` so `codev doctor` reports the real CLI instead + * of `.` / `tea_api_paged` / `printf`. + * - `tea_api_paged` FAILS at the `GITEA_MAX_PAGES` ceiling instead of + * returning a partial array at exit 0. + * - `pr-view`, `user-identity` and `issue-view` validate the response shape + * before normalizing: `tea api` exits 0 on HTTP errors and prints the error + * body, which used to become an all-null contract object (leaking the error + * body's own `url` as the browser page) or the literal username `null`. + * - `recently-merged` honors `CODEV_SINCE_DATE`, bounding a walk that would + * otherwise cover the repo's entire merge history inside forge's 30s + * timeout. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; @@ -42,6 +57,7 @@ import { chmodSync, mkdirSync, mkdtempSync, + readFileSync, rmSync, writeFileSync, } from 'node:fs'; @@ -69,7 +85,13 @@ fi [ "$1" = "api" ] || { echo "fake-tea: unsupported: $*" >&2; exit 3; } case "$2" in user) - echo '{"login":"octo","id":7}' ;; + # FAKE_TEA_USER_ERROR reproduces \`tea api\`'s exit-0-on-HTTP-error: an + # error body on stdout with no \`.login\`, at exit status 0. + if [ -n "$FAKE_TEA_USER_ERROR" ]; then + echo '{"message":"token does not exist","url":"https://git.example.com/api/swagger"}' + else + echo '{"login":"octo","id":7}' + fi ;; repos/acme/widgets/pulls/42) echo '{"number":42,"title":"Add widget","body":"PR body","state":"open","html_url":"https://git.example.com/acme/widgets/pulls/42","url":"https://git.example.com/api/v1/repos/acme/widgets/pulls/42","user":{"login":"alice"},"base":{"ref":"main"},"head":{"ref":"feature/x"},"additions":10,"deletions":3}' ;; @@ -137,6 +159,51 @@ case "$2" in repos/acme/widgets/issues/98/comments) echo "fake-tea: comments endpoint down" >&2; exit 7 ;; + # --- error bodies at exit 0 ------------------------------------------- + # \`tea api\` exits 0 on an HTTP error and prints the error body. These + # fixtures reproduce that exactly: exit status 0, an error OBJECT on stdout. + # Note the \`url\` key — Gitea's error bodies carry one (the swagger link), + # which an unvalidated normalizer would ship as the PR/issue browser page. + repos/acme/widgets/pulls/404) + echo '{"message":"pull request does not exist [id: 0, index: 404]","url":"https://git.example.com/api/swagger"}' ;; + repos/acme/widgets/issues/404) + echo '{"message":"issue does not exist [id: 0, index: 404]","url":"https://git.example.com/api/swagger"}' ;; + # issue 97: the issue itself is fine, but its comments endpoint answers with + # an error OBJECT at exit 0 (not a blank body and not a failure), which used + # to reach \`jq --argjson\` and blow up with a raw iteration error. + repos/acme/widgets/issues/97) + echo '{"number":97,"title":"Comments error body","body":"body","state":"open","html_url":"https://git.example.com/acme/widgets/issues/97","comments":3}' ;; + repos/acme/widgets/issues/97/comments) + echo '{"message":"token does not have at least one of required scope(s): [read:issue]","url":"https://git.example.com/api/swagger"}' ;; + + # --- endless pagination: every page is a full 50 items, forever, so no + # terminal short/empty page is ever reached and the GITEA_MAX_PAGES ceiling + # fires. Proves the paginator errors rather than returning a partial array. --- + # (5 items/page, not 50: the paginator's short-page check compares against + # the size observed on page 1, so a uniform page size of any value is never + # "short" — this just keeps a 100-page walk cheap in CI.) + repos/acme/endless/pulls*) + jq -cn '[range(5)|{number:(5000+.),title:"pad",html_url:"u",body:"",state:"open",merged:false,created_at:"d",updated_at:"2026-07-09T00:00:00Z",user:{login:"pad"},requested_reviewers:[],draft:false,head:{ref:"pad"}}]' ;; + + # --- recently-merged bounded by CODEV_SINCE_DATE ------------------------ + # Page 1 is a full 50 items sorted by updated_at DESC and reaches back past + # the cutoff (2026-07-05T00:00:00Z): two merges after it, then 48 older ones. + # Page 2 ERRORS, so a clean exit proves the walk stopped at page 1. + "repos/acme/dated/pulls?state=closed&sort=recentupdate&limit=50&page=1") + jq -cn '[{number:10,title:"Recent merge",html_url:"https://git.example.com/acme/dated/pulls/10",body:"r",state:"closed",merged:true,merged_at:"2026-07-08T10:00:00Z",created_at:"2026-07-01T00:00:00Z",updated_at:"2026-07-08T10:00:00Z",head:{ref:"feature/recent"}},{number:9,title:"Also recent",html_url:"u",body:"",state:"closed",merged:true,merged_at:"2026-07-06T09:00:00+02:00",created_at:"2026-06-01T00:00:00Z",updated_at:"2026-07-06T09:00:00+02:00",head:{ref:"feature/offset"}}] + [range(48)|{number:(6000+.),title:"old",html_url:"u",body:"",state:"closed",merged:true,merged_at:"2026-06-01T00:00:00Z",created_at:"2026-05-01T00:00:00Z",updated_at:"2026-06-01T00:00:00Z",head:{ref:"old"}}]' ;; + "repos/acme/dated/pulls?state=closed&sort=recentupdate&limit=50&page=2") + echo "fake-tea: dated page 2 requested" >&2; exit 9 ;; + + # --- server that IGNORES sort=recentupdate ------------------------------ + # Page 1 is a full 50 items in arbitrary update order that includes items + # older than the cutoff. The stop filter must NOT fire (the page isn't + # non-increasing), so the walk continues to the short page 2 and the merge + # that lives there is still reported. + "repos/acme/unsorted/pulls?state=closed&sort=recentupdate&limit=50&page=1") + jq -cn '[range(50)|{number:(7000+.),title:"mixed",html_url:"u",body:"",state:"closed",merged:false,created_at:"c",updated_at:(if (. % 2) == 0 then "2026-06-01T00:00:00Z" else "2026-07-09T00:00:00Z" end),head:{ref:"mixed"}}]' ;; + "repos/acme/unsorted/pulls?state=closed&sort=recentupdate&limit=50&page=2") + echo '[{"number":7100,"title":"Deep recent merge","html_url":"https://git.example.com/acme/unsorted/pulls/7100","body":"d","state":"closed","merged":true,"merged_at":"2026-07-07T00:00:00Z","created_at":"2026-07-01T00:00:00Z","updated_at":"2026-07-07T00:00:00Z","head":{"ref":"feature/deep-unsorted"}}]' ;; + *) echo "fake-tea: no fixture for: $2" >&2; exit 4 ;; esac `; @@ -418,4 +485,183 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` rmSync(garbage, { recursive: true, force: true }); } }); + + // --- maintainer review follow-up (2026-09-03) ---------------------------- + + it('every script that hides `tea` behind a helper declares forge-executable', () => { + // `codev doctor` infers the CLI a concept needs from the script's first + // substantive line. In these six that line is `. _lib.sh` / an assignment / + // `printf`, none of which is on PATH, so doctor reported them as missing + // tools and stopped checking for `tea`. The `# forge-executable:` header + // (#1458) declares it explicitly. + for (const name of [ + 'pr-exists.sh', + 'pr-list.sh', + 'pr-view.sh', + 'recently-merged.sh', + 'issue-view.sh', + 'user-identity.sh', + ]) { + const src = readFileSync(join(giteaDir, name), 'utf-8'); + expect(src, name).toMatch(/^#\s*forge-executable:\s*tea$/m); + } + }); + + it('pagination fails loudly at the page ceiling instead of truncating', () => { + // Every page from this repo is a full 50 items, so no terminal short/empty + // page is ever reached. Returning the partial array at exit 0 would be the + // silent truncation the paginator exists to prevent — for `pr-exists` it + // reads as "no PR exists" and passes a porch pr_exists gate. + const { status, stdout, stderr } = runScriptFull('pr-exists.sh', { + CODEV_BRANCH_NAME: 'feature/x', + CODEV_REPO: 'acme/endless', + }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + expect(stderr).toContain('page ceiling'); + // 100 sequential fake-`tea` + jq invocations; the default 5s is too tight. + }, 30_000); + + it('pr-view fails with the server message on an error body (not an all-null PR)', () => { + // `tea api` exits 0 and prints the error body. Unvalidated, that produced a + // structurally valid PrViewResult with every field null except `url`, which + // took the error body's own `url` — the swagger link — and shipped it as + // the PR's browser page. + const { status, stdout, stderr } = runScriptFull('pr-view.sh', { CODEV_PR_NUMBER: '404' }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + expect(stderr).toContain('pull request does not exist'); + expect(stderr).not.toContain('swagger'); + }); + + it('user-identity fails on an error body instead of printing "null"', () => { + const { status, stdout, stderr } = runScriptFull('user-identity.sh', { + FAKE_TEA_USER_ERROR: '1', + }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + expect(stderr).toContain('token does not exist'); + }); + + it('issue-view fails with the server message on an error body', () => { + const { status, stdout, stderr } = runScriptFull('issue-view.sh', { CODEV_ISSUE_ID: '404' }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + expect(stderr).toContain('issue does not exist'); + // The issue is validated BEFORE its comments are fetched, so a bad id + // doesn't also warn about degraded comments on an issue that isn't there + // (and doesn't spend a request on them). + expect(stderr).not.toContain('comments fetch failed'); + }); + + it('issue-view degrades to [] when the comments endpoint answers with an error OBJECT', () => { + // Distinct from issue 98 (comments fetch FAILS): here the fetch succeeds at + // exit 0 with an error object, which reached `jq --argjson` and blew up + // with a raw iteration error instead of the warned [] degrade. + const { status, stdout, stderr } = runScriptFull('issue-view.sh', { CODEV_ISSUE_ID: '97' }); + expect(status).toBe(0); + const issue = JSON.parse(stdout); + expect(issue.title).toBe('Comments error body'); + expect(issue.comments).toEqual([]); + expect(stderr).toContain('comments fetch failed for issue 97'); + }); + + it('recently-merged bounds its walk with CODEV_SINCE_DATE', () => { + // Page 1 is sorted by updated_at DESC and reaches back past the cutoff, so + // the walk must stop there. The fixture's page 2 errors, so a clean exit is + // itself the assertion that no second request was made. + const { status, stdout, stderr } = runScriptFull('recently-merged.sh', { + CODEV_REPO: 'acme/dated', + CODEV_SINCE_DATE: '2026-07-05T00:00:00Z', + }); + expect(stderr).toBe(''); + expect(status).toBe(0); + const merged = JSON.parse(stdout); + // Only the two merges after the cutoff — the 48 older ones on the same page + // are filtered out. The second one carries a +02:00 offset rather than `Z`, + // pinning that Gitea's server-timezone timestamps compare correctly. + expect(merged.map((p: { number: number }) => p.number).sort((a: number, b: number) => a - b)) + .toEqual([9, 10]); + expect(merged[0]).toMatchObject({ + number: 10, + title: 'Recent merge', + url: 'https://git.example.com/acme/dated/pulls/10', + mergedAt: '2026-07-08T10:00:00Z', + headRefName: 'feature/recent', + }); + }); + + it('recently-merged accepts a bare YYYY-MM-DD CODEV_SINCE_DATE', () => { + // `github.ts` passes a full ISO timestamp but `team-update.ts` passes a bare + // date, so both must bound the walk. A bare date reads as midnight UTC — + // same cutoff as the test above, same fixture, same result. + const { status, stdout, stderr } = runScriptFull('recently-merged.sh', { + CODEV_REPO: 'acme/dated', + CODEV_SINCE_DATE: '2026-07-05', + }); + expect(stderr).toBe(''); + expect(status).toBe(0); + expect(JSON.parse(stdout).map((p: { number: number }) => p.number).sort()).toEqual([10, 9]); + }); + + it('an unparseable CODEV_SINCE_DATE falls back to the unbounded walk', () => { + // Degrade toward MORE work, never toward silently dropping merges: with no + // usable cutoff the stop filter must not fire, so page 2 IS requested (and + // this fixture's page 2 errors, which is how we can see it happened). + const { status, stderr } = runScriptFull('recently-merged.sh', { + CODEV_REPO: 'acme/dated', + CODEV_SINCE_DATE: 'last tuesday', + }); + expect(status).not.toBe(0); + expect(stderr).toContain('dated page 2 requested'); + }); + + it('gitea_epoch normalizes Gitea\'s server-timezone timestamps', () => { + // Gitea marshals RFC3339 in the SERVER's timezone, so `Z` is not + // guaranteed. These four spell the same instant; a lexicographic compare + // would order them wrongly, which is why the offset is parsed out. + const program = `. "${join(giteaDir, '_lib.sh')}"; ` + + `printf '%s' "$INPUT" | jq -c "\${GITEA_JQ_LIB} [ .[] | gitea_epoch ]"`; + const out = execFileSync('sh', ['-c', program], { + encoding: 'utf-8', + env: { + ...runEnv, + INPUT: JSON.stringify([ + '2026-07-05T12:00:00Z', + '2026-07-05T14:00:00+02:00', + '2026-07-05T10:00:00-02:00', + '2026-07-05T12:00:00.123Z', + '2026-07-05', // bare date -> midnight UTC + '2026-13-99', // right shape, impossible date -> null, not a throw + 'garbage', + null, + 42, + ]), + }, + }).trim(); + expect(JSON.parse(out)).toEqual([ + 1783252800, 1783252800, 1783252800, 1783252800, + 1783209600, + null, null, null, null, + ]); + }); + + it('recently-merged keeps walking when the server ignores sort=recentupdate', () => { + // The since-date bound must not cost data on a server that doesn't honor + // the sort parameter: page 1 contains items older than the cutoff but is + // NOT in descending update order, so the stop filter must not fire and the + // merge that lives only on page 2 must still be reported. + const merged = JSON.parse(runScript('recently-merged.sh', { + CODEV_REPO: 'acme/unsorted', + CODEV_SINCE_DATE: '2026-07-05T00:00:00Z', + })); + expect(merged).toHaveLength(1); + expect(merged[0]).toMatchObject({ number: 7100, headRefName: 'feature/deep-unsorted' }); + }); + + it('recently-merged without CODEV_SINCE_DATE still walks the whole history', () => { + // The unbounded path is unchanged: no `sort` parameter, every page walked. + const merged = JSON.parse(runScript('recently-merged.sh')); + expect(merged).toHaveLength(2); // #40 (page 1) + #901 (page 2) + }); }); diff --git a/packages/codev/src/lib/forge-contracts.ts b/packages/codev/src/lib/forge-contracts.ts index 9194a78d0..ab96f8408 100644 --- a/packages/codev/src/lib/forge-contracts.ts +++ b/packages/codev/src/lib/forge-contracts.ts @@ -79,13 +79,17 @@ export interface PrListItem { mergedAt?: string; author?: { login: string }; /** - * Logins of users requested as reviewers. Emitted by every forge's `pr-list` - * script (GitHub flattens gh's reviewer objects to logins; GitLab/Gitea emit - * `[]` as they expose no GitHub-equivalent per-user review-request list). - * Consumed by the VSCode PR sidebar to bucket "review-requested" PRs. + * Logins of users requested as reviewers. Every preset populates it for real: + * GitHub flattens gh's reviewer objects to logins, GitLab maps glab's + * `.reviewers[].username`, and Gitea maps the REST `requested_reviewers[]` + * (team reviewers carry no login and are dropped). Consumed by the VSCode PR + * sidebar to bucket "review-requested" PRs. */ reviewRequests: string[]; - /** Whether the PR is a draft. GitLab/Gitea emit `false` (not exposed). */ + /** + * Whether the PR is a draft. Every preset reports it for real — GitLab from + * the merge request's `draft`/WIP flag, Gitea from the REST `draft` flag. + */ isDraft: boolean; } From 99492dc0d4ef8c2197fa9b75f6fb7af91976b345 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Thu, 3 Sep 2026 21:32:14 -0700 Subject: [PATCH 11/14] =?UTF-8?q?[Bugfix=20#1137]=20Fix:=20CMAP=20review?= =?UTF-8?q?=20=E2=80=94=20ordering=20must=20survive=20a=20page=20boundary,?= =?UTF-8?q?=20non-array=20pages,=20empty=20bodies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consultation review of the previous commit (Codex). Five findings, all real. The important one: my stop filter for `recently-merged` checked only that the CURRENT page was non-increasing in `updated_at`, and I claimed that proved the server honored `sort=recentupdate`. It does not. A server that ignores the parameter can still return an internally descending page 1 — say one entirely older than the cutoff — while a genuinely recent merge sits on page 2, and we would have stopped and dropped it. Page-local order is also what a server with per-page rather than global sorting produces. The filter now requires the ordering to survive a page boundary: the previous page descending too, and its oldest entry no older than this page's newest. It never fires on page 1, where there is nothing to compare against — one extra request is the right price. `tea_api_paged` binds the previous page as `$prev` to make that check possible. The reviewer's exact counterexample is now a fixture (`acme/lagging`). Also: - A page that parses but isn't an array is a hard error, not the end of the list. `jq length` is 0 for both `null` and `{}`, so an error body mid-walk — which `tea api` hands us at exit 0 — looked exactly like an exhausted list and returned the pages collected so far at exit 0. - An empty body at exit 0 now fails. jq given empty stdin emits nothing and exits 0, so `pr-view` and `user-identity` were "succeeding" with empty stdout, and the shape validators never ran at all. - `gitea_epoch`'s offset is bounded to the real UTC range, so `+99:99` yields null instead of an epoch two days out. Its remaining leniency is documented rather than claimed away: it validates shape, not the calendar, so `2026-02-30` normalizes into March. - Contract types are checked, not just defaulted: non-numeric `additions`/ `deletions` no longer pass through as strings, comment fields default to the declared type instead of emitting nulls, and a whitespace-only login is rejected like an empty one. Two bugs of my own that the tests caught: inside a jq `range` body `.` is the range value, not the array (the `descending` helper needs the array bound first), and an apostrophe inside a single-quoted jq program closes the shell string. 37 tests in the file; full suite 4893 passed | 48 skipped. Every path also exercised under dash, which is what /bin/sh is on the CI runner. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch --- packages/codev/scripts/forge/gitea/_lib.sh | 42 ++++-- .../codev/scripts/forge/gitea/issue-view.sh | 15 +- packages/codev/scripts/forge/gitea/pr-view.sh | 13 +- .../scripts/forge/gitea/recently-merged.sh | 42 ++++-- .../scripts/forge/gitea/user-identity.sh | 9 +- .../bugfix-1137-gitea-tea-api.test.ts | 132 +++++++++++++++--- 6 files changed, 207 insertions(+), 46 deletions(-) diff --git a/packages/codev/scripts/forge/gitea/_lib.sh b/packages/codev/scripts/forge/gitea/_lib.sh index 6c9790f3a..b5d855068 100755 --- a/packages/codev/scripts/forge/gitea/_lib.sh +++ b/packages/codev/scripts/forge/gitea/_lib.sh @@ -48,15 +48,19 @@ gitea_repo() { # subtracted explicitly. # - CODEV_SINCE_DATE, which callers set to either a full timestamp # (`github.ts`) or a bare `YYYY-MM-DD` (`team-update.ts`). A bare date is -# read as midnight UTC. +# read as midnight UTC, and a timestamp with no offset at all is read as +# UTC too. # -# Unparseable input yields null, and every caller treats null as "don't know" — -# keep the item, keep walking. A surprising format therefore degrades to the old -# unbounded behavior rather than silently dropping data. +# Input that isn't a recognizable date yields null, and every caller treats null +# as "don't know" — keep the item, keep walking — so a surprising format +# degrades to the old unbounded behavior rather than silently dropping data. +# This is shape validation, not a calendar: `2026-02-30` is normalized by +# `fromdateiso8601` into March rather than rejected. That only matters for a +# hand-written cutoff, and lands it a day or two off rather than anywhere wild. GITEA_JQ_LIB=' def gitea_epoch: if type == "string" then - ((capture("^(?\\d{4}-\\d{2}-\\d{2})(T(?\\d{2}:\\d{2}:\\d{2})(\\.\\d+)?(?Z|[+-]\\d{2}:\\d{2})?)?$")) // null) as $c + ((capture("^(?\\d{4}-\\d{2}-\\d{2})(T(?\\d{2}:\\d{2}:\\d{2})(\\.\\d+)?(?Z|[+-](0\\d|1[0-4]):[0-5]\\d)?)?$")) // null) as $c | if $c == null then null # `try`: the regex only proves the SHAPE. `2026-13-99` matches it and then # makes `fromdateiso8601` throw, which would abort the script with a raw @@ -90,15 +94,22 @@ GITEA_MAX_PAGES=100 # Usage: tea_api_paged "repos///pulls" "state=all" [""] # $1 = API path (no page params) # $2 = extra query string (may be empty), e.g. "state=open" -# $3 = optional jq program run on each page's array; when it outputs `true` -# the walk stops after that page. Used by `recently-merged` to bound the -# walk with CODEV_SINCE_DATE. It must be conservative: a false negative -# just costs another page, a false positive silently truncates. +# $3 = optional jq program run on each page's array, with the PREVIOUS page +# bound as `$prev` (`null` on page 1); when it outputs `true` the walk +# stops after that page. Used by `recently-merged` to bound the walk with +# CODEV_SINCE_DATE — `$prev` is what lets it check ordering ACROSS a page +# boundary and not just within one page. It must be conservative: a false +# negative just costs another page, a false positive silently truncates. # # Loops page=1,2,3… appending "&limit=&page=", concatenates each page's # array, and stops when a page returns fewer than the requested limit (the last # page), an empty/blank response, or the caller's stop filter fires. # +# A page that parses but ISN'T an array is a hard error, not a stop condition. +# `tea api` exits 0 on HTTP errors and prints the error body, and `jq length` is +# 0 for both `null` and `{}` — so an error body mid-walk used to look exactly +# like an exhausted list and return the partial array at exit 0. +# # Reaching GITEA_MAX_PAGES without any of those terminal conditions means we do # NOT know we have the whole list. Returning the partial array at exit 0 would # be exactly the silent-truncation class this paginator exists to prevent (a @@ -113,6 +124,7 @@ tea_api_paged() { _acc='[]' _page_size='' _terminal='' + _prev='' while [ "$_page" -le "$GITEA_MAX_PAGES" ]; do if [ -n "$_query" ]; then _url="${_path}?${_query}&limit=${GITEA_PAGE_LIMIT}&page=${_page}" @@ -125,19 +137,27 @@ tea_api_paged() { _terminal=1 break fi - _count="$(printf '%s' "$_resp" | jq 'length')" || return 1 + # Length AND type in one jq pass; a non-array page is prefixed with "!". + _count="$(printf '%s' "$_resp" | jq -r 'if type == "array" then length else "!" + type end')" || return 1 + case "$_count" in + '!'*) + echo "gitea forge: page ${_page} of '${_path}' is not an array but a ${_count#!} (an HTTP error body reaches us at exit 0); refusing to return a truncated result" >&2 + return 1 + ;; + esac if [ "$_count" -eq 0 ]; then _terminal=1 break fi _acc="$(printf '%s\n%s' "$_acc" "$_resp" | jq -s 'add')" || return 1 if [ -n "$_stop" ]; then - _hit="$(printf '%s' "$_resp" | jq "$_stop")" || return 1 + _hit="$(printf '%s' "$_resp" | jq --argjson prev "${_prev:-null}" "$_stop")" || return 1 if [ "$_hit" = "true" ]; then _terminal=1 break fi fi + _prev="$_resp" # A server whose max_response_items is tuned below GITEA_PAGE_LIMIT # truncates every page to its own cap, not the requested limit — so # stopping when a page is shorter than the *requested* limit would break diff --git a/packages/codev/scripts/forge/gitea/issue-view.sh b/packages/codev/scripts/forge/gitea/issue-view.sh index 9de942c74..48d203a0e 100755 --- a/packages/codev/scripts/forge/gitea/issue-view.sh +++ b/packages/codev/scripts/forge/gitea/issue-view.sh @@ -37,6 +37,12 @@ REPO="$(gitea_repo)" || exit 1 # Capture before piping: POSIX sh has no pipefail, so `tea api … | jq` would # report jq's exit status rather than a failed fetch. ISSUE="$(tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}")" || exit 1 +# jq given empty input emits nothing and exits 0, so an empty body at exit 0 +# would slip past the validator below rather than failing. +if [ -z "$ISSUE" ]; then + echo "gitea forge: empty \`tea api\` response for issue ${CODEV_ISSUE_ID}" >&2 + exit 1 +fi printf '%s' "$ISSUE" | jq -e ' if (type == "object") and ((.title | type) == "string") @@ -62,9 +68,12 @@ printf '%s' "$ISSUE" | jq --argjson comments "$COMMENTS_JSON" '{ body: (.body // ""), state, url: (.html_url // .url), + # The array itself is validated above; its ELEMENTS are whatever the + # server sent, so each field is defaulted to the type the contract + # declares rather than emitting nulls into IssueViewResult.comments. comments: [ $comments[] | { - body: (.body // ""), - createdAt: .created_at, - author: {login: .user.login} + body: (if (.body | type) == "string" then .body else "" end), + createdAt: (if (.created_at | type) == "string" then .created_at else "" end), + author: {login: (if (.user.login | type) == "string" then .user.login else "" end)} } ] }' diff --git a/packages/codev/scripts/forge/gitea/pr-view.sh b/packages/codev/scripts/forge/gitea/pr-view.sh index 5199c7b7b..2774424a2 100755 --- a/packages/codev/scripts/forge/gitea/pr-view.sh +++ b/packages/codev/scripts/forge/gitea/pr-view.sh @@ -30,6 +30,13 @@ REPO="$(gitea_repo)" || exit 1 # reports jq's exit status, so a failed fetch would surface as jq's exit-0 on # empty stdin rather than an error. PR="$(tea api "repos/${REPO}/pulls/${CODEV_PR_NUMBER}")" || exit 1 +# An empty body at exit 0 would give jq nothing to work on: it emits no output +# and exits 0, so the script would "succeed" with empty stdout instead of +# failing. The validator below never runs on an empty response, so guard here. +if [ -z "$PR" ]; then + echo "gitea forge: empty \`tea api\` response for pull ${CODEV_PR_NUMBER}" >&2 + exit 1 +fi printf '%s' "$PR" | jq ' if (type == "object") and ((.number | type) == "number") @@ -53,6 +60,8 @@ printf '%s' "$PR" | jq ' author: {login: .user.login}, baseRefName: .base.ref, headRefName: .head.ref, - additions: (.additions // 0), - deletions: (.deletions // 0) + # Type-checked, not just defaulted: the contract types these as numbers, and + # a non-numeric value here would be passed straight through. + additions: (if (.additions | type) == "number" then .additions else 0 end), + deletions: (if (.deletions | type) == "number" then .deletions else 0 end) }' diff --git a/packages/codev/scripts/forge/gitea/recently-merged.sh b/packages/codev/scripts/forge/gitea/recently-merged.sh index f5283550d..68dd037e1 100755 --- a/packages/codev/scripts/forge/gitea/recently-merged.sh +++ b/packages/codev/scripts/forge/gitea/recently-merged.sh @@ -27,26 +27,44 @@ # we ask the server for update-time-descending order and stop at the first page # that reaches back past the cutoff. # -# The stop filter refuses to trust the sort blindly: it fires only when the -# page is ACTUALLY non-increasing in `updated_at` (proving the server honored -# `sort=recentupdate`) AND some item on it predates the cutoff. A server that -# ignores the parameter falls back to the full walk rather than silently -# dropping merges. `updated_at >= merged_at` always holds — a merge updates the -# PR — so nothing merged after the cutoff can sit beyond the first page whose -# update times have fallen behind it. Timestamps go through `gitea_epoch` -# because Gitea emits RFC3339 in the server's timezone, not necessarily `Z`. +# The stop filter does not take the sort on trust. What it actually needs is +# the ORDERING, not the parameter, so it checks for the ordering directly and +# fires only when all of this holds: +# - the current page is non-increasing in `updated_at`, +# - the PREVIOUS page was too, and its oldest entry is no older than this +# page's newest — i.e. the order survives a page boundary, so page-local +# sorting or a coincidentally-descending first page isn't enough, +# - some entry on this page predates the cutoff. +# Never on page 1: with nothing to compare against, one internally-descending +# page proves nothing about the pages behind it. Costing one extra request is +# the right trade against dropping a merge. +# +# Given that ordering, `updated_at >= merged_at` (a merge updates the PR) means +# nothing merged after the cutoff can sit beyond the first page whose update +# times have fallen behind it. A server that ignores `sort=recentupdate` fails +# these checks and we fall back to the full walk — slower, never wrong. +# +# Timestamps go through `gitea_epoch` because Gitea emits RFC3339 in the +# server's timezone, not necessarily `Z`. . "$(dirname "$0")/_lib.sh" REPO="$(gitea_repo)" || exit 1 if [ -n "$CODEV_SINCE_DATE" ]; then QUERY="state=closed&sort=recentupdate" + # `$prev` is the previous page, bound by tea_api_paged (null on page 1). STOP="${GITEA_JQ_LIB}"' - [ .[] | (.updated_at | gitea_epoch) ] as $t + def descending: . as $a | [ range(($a | length) - 1) | $a[.] >= $a[. + 1] ] | all; + def times: [ .[] | (.updated_at | gitea_epoch) ]; + + times as $t + | ($prev | if . == null then null else times end) as $p | (env.CODEV_SINCE_DATE | gitea_epoch) as $since | ($since != null) - and (($t | length) > 0) - and ([ $t[] | . != null ] | all) - and ([ range(($t | length) - 1) | $t[.] >= $t[.+ 1] ] | all) + and ($p != null) + and (($t | length) > 0) and (($p | length) > 0) + and ([ $t[] | . != null ] | all) and ([ $p[] | . != null ] | all) + and ($t | descending) and ($p | descending) + and (($p | min) >= ($t | max)) and (($t | min) < $since) ' else diff --git a/packages/codev/scripts/forge/gitea/user-identity.sh b/packages/codev/scripts/forge/gitea/user-identity.sh index a0173ec6c..50aa6e144 100755 --- a/packages/codev/scripts/forge/gitea/user-identity.sh +++ b/packages/codev/scripts/forge/gitea/user-identity.sh @@ -15,8 +15,15 @@ # otherwise. The response is captured before the pipe because POSIX sh has no # pipefail: `tea api user | jq` would report jq's status, not tea's. USER_JSON="$(tea api user)" || exit 1 +# jq given empty input emits nothing and exits 0, so an empty body at exit 0 +# would leave the script "succeeding" with no username at all. +if [ -z "$USER_JSON" ]; then + echo "gitea forge: empty \`tea api user\` response" >&2 + exit 1 +fi printf '%s' "$USER_JSON" | jq -r ' - if (type == "object") and ((.login | type) == "string") and (.login != "") + if (type == "object") and ((.login | type) == "string") + and ((.login | test("^\\s*$")) | not) then .login else ("gitea forge: unexpected `tea api user` response: " diff --git a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts index f57af7a94..06e525833 100644 --- a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts +++ b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts @@ -89,6 +89,11 @@ case "$2" in # error body on stdout with no \`.login\`, at exit status 0. if [ -n "$FAKE_TEA_USER_ERROR" ]; then echo '{"message":"token does not exist","url":"https://git.example.com/api/swagger"}' + elif [ -n "$FAKE_TEA_USER_EMPTY" ]; then + # exit 0, nothing on stdout — jq would emit nothing and exit 0 too. + : + elif [ -n "$FAKE_TEA_USER_BLANK" ]; then + echo '{"login":" ","id":7}' else echo '{"login":"octo","id":7}' fi ;; @@ -186,13 +191,46 @@ case "$2" in jq -cn '[range(5)|{number:(5000+.),title:"pad",html_url:"u",body:"",state:"open",merged:false,created_at:"d",updated_at:"2026-07-09T00:00:00Z",user:{login:"pad"},requested_reviewers:[],draft:false,head:{ref:"pad"}}]' ;; # --- recently-merged bounded by CODEV_SINCE_DATE ------------------------ - # Page 1 is a full 50 items sorted by updated_at DESC and reaches back past - # the cutoff (2026-07-05T00:00:00Z): two merges after it, then 48 older ones. - # Page 2 ERRORS, so a clean exit proves the walk stopped at page 1. + # Cutoff for these fixtures is 2026-07-05T00:00:00Z. + # Page 1: 50 items, strictly descending by updated_at, ALL after the cutoff + # (2026-07-09T00:00 down to 2026-07-06T20:00) — two of them merged, the + # second carrying a +02:00 offset rather than \`Z\`. The walk must NOT stop + # here: nothing has crossed the cutoff yet, and page 1 alone proves nothing + # about ordering anyway. + # Page 2: 50 items continuing the descent (2026-07-06T19:00 down to + # 2026-07-04T18:00) and so crossing the cutoff — one merge above it, one + # below. The order survives the page boundary, so the walk stops here. + # Page 3: ERRORS, so a clean exit proves the walk stopped at page 2. "repos/acme/dated/pulls?state=closed&sort=recentupdate&limit=50&page=1") - jq -cn '[{number:10,title:"Recent merge",html_url:"https://git.example.com/acme/dated/pulls/10",body:"r",state:"closed",merged:true,merged_at:"2026-07-08T10:00:00Z",created_at:"2026-07-01T00:00:00Z",updated_at:"2026-07-08T10:00:00Z",head:{ref:"feature/recent"}},{number:9,title:"Also recent",html_url:"u",body:"",state:"closed",merged:true,merged_at:"2026-07-06T09:00:00+02:00",created_at:"2026-06-01T00:00:00Z",updated_at:"2026-07-06T09:00:00+02:00",head:{ref:"feature/offset"}}] + [range(48)|{number:(6000+.),title:"old",html_url:"u",body:"",state:"closed",merged:true,merged_at:"2026-06-01T00:00:00Z",created_at:"2026-05-01T00:00:00Z",updated_at:"2026-06-01T00:00:00Z",head:{ref:"old"}}]' ;; + jq -cn '[{number:10,title:"Recent merge",html_url:"https://git.example.com/acme/dated/pulls/10",body:"r",state:"closed",merged:true,merged_at:"2026-07-09T00:00:00Z",created_at:"2026-07-01T00:00:00Z",updated_at:"2026-07-09T00:00:00Z",head:{ref:"feature/recent"}},{number:9,title:"Offset merge",html_url:"u",body:"",state:"closed",merged:true,merged_at:"2026-07-08T22:00:00+02:00",created_at:"2026-06-01T00:00:00Z",updated_at:"2026-07-08T22:00:00+02:00",head:{ref:"feature/offset"}}] + [range(48)|(1783537200 - (. * 3600)) as $u|{number:(6000+.),title:"p1 pad",html_url:"u",body:"",state:"closed",merged:false,created_at:"2026-05-01T00:00:00Z",updated_at:($u|todateiso8601),head:{ref:"pad"}}]' ;; "repos/acme/dated/pulls?state=closed&sort=recentupdate&limit=50&page=2") - echo "fake-tea: dated page 2 requested" >&2; exit 9 ;; + jq -cn '[range(50)|(1783364400 - (. * 3600)) as $u|{number:(6100+.),title:"p2",html_url:"u",body:"",state:"closed",merged:(. == 0 or . == 49),merged_at:(if (. == 0 or . == 49) then ($u|todateiso8601) else null end),created_at:"2026-05-01T00:00:00Z",updated_at:($u|todateiso8601),head:{ref:"p2"}}]' ;; + "repos/acme/dated/pulls?state=closed&sort=recentupdate&limit=50&page=3") + echo "fake-tea: dated page 3 requested" >&2; exit 9 ;; + + # --- the reviewer counterexample: a server that IGNORES sort but happens to + # return an internally descending page 1 that is entirely older than the + # cutoff, with a genuinely recent merge sitting on page 2. Stopping on + # page-local ordering alone would silently drop that merge. --- + "repos/acme/lagging/pulls?state=closed&sort=recentupdate&limit=50&page=1") + jq -cn '[range(50)|(1780876800 - (. * 3600)) as $u|{number:(8000+.),title:"old",html_url:"u",body:"",state:"closed",merged:false,created_at:"2026-05-01T00:00:00Z",updated_at:($u|todateiso8601),head:{ref:"old"}}]' ;; + "repos/acme/lagging/pulls?state=closed&sort=recentupdate&limit=50&page=2") + echo '[{"number":8100,"title":"Late merge of an old PR","html_url":"https://git.example.com/acme/lagging/pulls/8100","body":"l","state":"closed","merged":true,"merged_at":"2026-07-07T00:00:00Z","created_at":"2026-01-01T00:00:00Z","updated_at":"2026-07-07T00:00:00Z","head":{"ref":"feature/late"}}]' ;; + + # --- non-array pages at exit 0: \`jq length\` is 0 for both \`{}\` and \`null\`, + # so an error body used to look exactly like an exhausted list. --- + "repos/acme/errorpage/pulls?state=open&limit=50&page=1") + jq -cn '[range(50)|{number:(9000+.),title:"pad",html_url:"u",body:"",state:"open",created_at:"d",user:{login:"pad"},requested_reviewers:[],draft:false}]' ;; + "repos/acme/errorpage/pulls?state=open&limit=50&page=2") + echo '{"message":"internal server error","url":"https://git.example.com/api/swagger"}' ;; + "repos/acme/errorpage/pulls?state=all&limit=50&page=1") + jq -cn '[range(50)|{number:(9000+.),state:"open",merged:false,head:{ref:"pad"}}]' ;; + "repos/acme/errorpage/pulls?state=all&limit=50&page=2") + echo 'null' ;; + + # --- empty bodies at exit 0 (pull/issue 0) ------------------------------ + repos/acme/widgets/pulls/0) : ;; + repos/acme/widgets/issues/0) : ;; # --- server that IGNORES sort=recentupdate ------------------------------ # Page 1 is a full 50 items in arbitrary update order that includes items @@ -567,9 +605,9 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` }); it('recently-merged bounds its walk with CODEV_SINCE_DATE', () => { - // Page 1 is sorted by updated_at DESC and reaches back past the cutoff, so - // the walk must stop there. The fixture's page 2 errors, so a clean exit is - // itself the assertion that no second request was made. + // Page 1 is entirely after the cutoff; page 2 continues the same descent + // and crosses it, so the walk stops there. The fixture's page 3 errors, so + // a clean exit is itself the assertion that no third request was made. const { status, stdout, stderr } = runScriptFull('recently-merged.sh', { CODEV_REPO: 'acme/dated', CODEV_SINCE_DATE: '2026-07-05T00:00:00Z', @@ -577,18 +615,37 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` expect(stderr).toBe(''); expect(status).toBe(0); const merged = JSON.parse(stdout); - // Only the two merges after the cutoff — the 48 older ones on the same page - // are filtered out. The second one carries a +02:00 offset rather than `Z`, - // pinning that Gitea's server-timezone timestamps compare correctly. + // Three merges above the cutoff (two on page 1, one on page 2); page 2 also + // carries one merged BELOW it, which must be filtered out. expect(merged.map((p: { number: number }) => p.number).sort((a: number, b: number) => a - b)) - .toEqual([9, 10]); + .toEqual([9, 10, 6100]); expect(merged[0]).toMatchObject({ number: 10, title: 'Recent merge', url: 'https://git.example.com/acme/dated/pulls/10', - mergedAt: '2026-07-08T10:00:00Z', + mergedAt: '2026-07-09T00:00:00Z', headRefName: 'feature/recent', }); + // #9's merged_at is `+02:00`, not `Z`. It survives the cutoff comparison + // only because the offset is parsed rather than compared lexicographically. + expect(merged.find((p: { number: number }) => p.number === 9)).toMatchObject({ + mergedAt: '2026-07-08T22:00:00+02:00', + headRefName: 'feature/offset', + }); + }); + + it('recently-merged does not stop on page 1 alone, however well ordered', () => { + // The reviewer counterexample. A server that ignores `sort=recentupdate` + // can still return an internally descending page 1 — here one entirely + // older than the cutoff — while a genuinely recent merge sits on page 2. + // Stopping on page-local ordering would silently drop it, so a stop needs + // ordering that survives a page boundary, which this fixture breaks. + const merged = JSON.parse(runScript('recently-merged.sh', { + CODEV_REPO: 'acme/lagging', + CODEV_SINCE_DATE: '2026-07-05T00:00:00Z', + })); + expect(merged).toHaveLength(1); + expect(merged[0]).toMatchObject({ number: 8100, headRefName: 'feature/late' }); }); it('recently-merged accepts a bare YYYY-MM-DD CODEV_SINCE_DATE', () => { @@ -601,19 +658,60 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` }); expect(stderr).toBe(''); expect(status).toBe(0); - expect(JSON.parse(stdout).map((p: { number: number }) => p.number).sort()).toEqual([10, 9]); + expect(JSON.parse(stdout).map((p: { number: number }) => p.number) + .sort((a: number, b: number) => a - b)).toEqual([9, 10, 6100]); }); it('an unparseable CODEV_SINCE_DATE falls back to the unbounded walk', () => { // Degrade toward MORE work, never toward silently dropping merges: with no - // usable cutoff the stop filter must not fire, so page 2 IS requested (and - // this fixture's page 2 errors, which is how we can see it happened). + // usable cutoff the stop filter must not fire, so page 3 IS requested (and + // this fixture's page 3 errors, which is how we can see it happened). const { status, stderr } = runScriptFull('recently-merged.sh', { CODEV_REPO: 'acme/dated', CODEV_SINCE_DATE: 'last tuesday', }); expect(status).not.toBe(0); - expect(stderr).toContain('dated page 2 requested'); + expect(stderr).toContain('dated page 3 requested'); + }); + + it('a non-array page mid-walk is an error, not the end of the list', () => { + // `tea api` exits 0 on HTTP errors, and `jq length` is 0 for both `{}` and + // `null` — so an error body looked exactly like an exhausted list and the + // paginator returned the pages it had at exit 0. + const asObject = runScriptFull('pr-list.sh', { CODEV_REPO: 'acme/errorpage' }); + expect(asObject.status).not.toBe(0); + expect(asObject.stdout.trim()).toBe(''); + expect(asObject.stderr).toContain('not an array'); + + const asNull = runScriptFull('pr-exists.sh', { + CODEV_BRANCH_NAME: 'pad', + CODEV_REPO: 'acme/errorpage', + }); + expect(asNull.status).not.toBe(0); + expect(asNull.stdout.trim()).toBe(''); + expect(asNull.stderr).toContain('not an array'); + }); + + it('an empty body at exit 0 fails instead of succeeding silently', () => { + // jq given empty stdin emits nothing and exits 0, so without an explicit + // guard these scripts "succeed" with empty stdout and the validators never + // run at all. + for (const [script, env] of [ + ['pr-view.sh', { CODEV_PR_NUMBER: '0' }], + ['issue-view.sh', { CODEV_ISSUE_ID: '0' }], + ['user-identity.sh', { FAKE_TEA_USER_EMPTY: '1' }], + ] as Array<[string, Record]>) { + const { status, stdout, stderr } = runScriptFull(script, env); + expect(status, script).not.toBe(0); + expect(stdout.trim(), script).toBe(''); + expect(stderr, script).toContain('empty'); + } + }); + + it('user-identity rejects a whitespace-only login', () => { + const { status, stdout } = runScriptFull('user-identity.sh', { FAKE_TEA_USER_BLANK: '1' }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); }); it('gitea_epoch normalizes Gitea\'s server-timezone timestamps', () => { From c948711328f9e90f6f630f0201439f07b80bd63e Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Thu, 3 Sep 2026 21:35:19 -0700 Subject: [PATCH 12/14] =?UTF-8?q?[Bugfix=20#1137]=20Fix:=20CMAP=20review?= =?UTF-8?q?=202=20=E2=80=94=20stop=20filter=20via=20stdin=20(Linux=20argv?= =?UTF-8?q?=20cap),=20probe=20past=20the=20page=20ceiling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second consultation lane (Claude), which independently reproduced the ordering flaw the first lane found and confirmed the cross-page fix closes it. Four new findings, all real. 1. Passing a page to the stop filter through `--argjson` breaks on Linux. Linux caps a SINGLE argv string at MAX_ARG_STRLEN (128KiB) regardless of ARG_MAX, and a 50-item Gitea pulls page — each object embedding full `base.repo` and `head.repo` objects — measures ~90KiB before anyone writes a long PR body. Past the cap `exec` fails, the paginator returns non-zero, and forge yields `null`. macOS has no per-argument cap, so this would have passed locally and failed on CI and on every Linux adopter. Both pages now go in on stdin. The `acme/heavy` fixture serves ~150KB pages so the regression bites where the bug lives. 2. The page ceiling false-positived on a complete result. A list whose length is an exact multiple of the page size reaches GITEA_MAX_PAGES with every page full and nothing missing, and we hard-failed it. One probe request past the ceiling settles it: empty means we already had everything. 3. A non-string `.message` — what a proxy or gateway between tea and Gitea produces — was concatenated straight into the error text and threw a raw jq error, defeating the point of a legible message. Now `(.message // .) | tostring`. 4. The test environment inherited `CODEV_*` from the developer's shell, so the "no CODEV_SINCE_DATE means walk everything" test was asserting the absence of a variable it did not control. Stripped from the base env; each test supplies what it means. Also: `issue-view`'s comments guard checked the outer array but not its elements, so `[1, 2]` got past it and died on `$comments[] | .body` instead of degrading to []. Both lanes noted that the `forge-executable` test asserts the declaration is present, not that doctor reads it — that is sequencing, not coverage, and the test now says so. 41 tests in the file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch --- packages/codev/scripts/forge/gitea/_lib.sh | 84 ++++++++++++----- .../codev/scripts/forge/gitea/issue-view.sh | 13 ++- packages/codev/scripts/forge/gitea/pr-view.sh | 2 +- .../scripts/forge/gitea/user-identity.sh | 2 +- .../bugfix-1137-gitea-tea-api.test.ts | 94 ++++++++++++++++++- 5 files changed, 163 insertions(+), 32 deletions(-) diff --git a/packages/codev/scripts/forge/gitea/_lib.sh b/packages/codev/scripts/forge/gitea/_lib.sh index b5d855068..7ca5e07f5 100755 --- a/packages/codev/scripts/forge/gitea/_lib.sh +++ b/packages/codev/scripts/forge/gitea/_lib.sh @@ -88,6 +88,24 @@ GITEA_PAGE_LIMIT=50 # an ERROR, not a stop condition (see below). GITEA_MAX_PAGES=100 +# Build one page URL. Split out so the ceiling probe below builds it the same +# way the loop does. +gitea_page_url() { + if [ -n "$2" ]; then + printf '%s?%s&limit=%s&page=%s' "$1" "$2" "$GITEA_PAGE_LIMIT" "$3" + else + printf '%s?limit=%s&page=%s' "$1" "$GITEA_PAGE_LIMIT" "$3" + fi +} + +# Echo a page response's item count, or "!" if it isn't a JSON array. +# `tea api` exits 0 on HTTP errors and prints the error body, and `jq length` is +# 0 for both `null` and `{}` — so without the type check an error body mid-walk +# looks exactly like an exhausted list. +gitea_page_count() { + printf '%s' "$1" | jq -r 'if type == "array" then length else "!" + type end' +} + # Fetch a paginated Gitea list endpoint and emit ONE concatenated JSON array on # stdout, so the caller's existing jq normalizer sees the same shape as before. # @@ -101,21 +119,18 @@ GITEA_MAX_PAGES=100 # boundary and not just within one page. It must be conservative: a false # negative just costs another page, a false positive silently truncates. # -# Loops page=1,2,3… appending "&limit=&page=", concatenates each page's -# array, and stops when a page returns fewer than the requested limit (the last -# page), an empty/blank response, or the caller's stop filter fires. -# -# A page that parses but ISN'T an array is a hard error, not a stop condition. -# `tea api` exits 0 on HTTP errors and prints the error body, and `jq length` is -# 0 for both `null` and `{}` — so an error body mid-walk used to look exactly -# like an exhausted list and return the partial array at exit 0. +# Loops page=1,2,3… concatenating each page's array, and stops when a page +# returns fewer than the requested limit (the last page), an empty/blank +# response, or the caller's stop filter fires. # -# Reaching GITEA_MAX_PAGES without any of those terminal conditions means we do -# NOT know we have the whole list. Returning the partial array at exit 0 would -# be exactly the silent-truncation class this paginator exists to prevent (a -# short `pr-exists` walk reads as "no PR exists" and passes a porch pr_exists -# gate on a repo we simply failed to finish reading), so it fails loudly -# instead: stderr message, non-zero return, no stdout. +# Reaching GITEA_MAX_PAGES with every page full means we do NOT know we have the +# whole list. Returning the partial array at exit 0 would be exactly the silent +# truncation this paginator exists to prevent (a short `pr-exists` walk reads as +# "no PR exists" and passes a porch pr_exists gate on a repo we simply failed to +# finish reading), so it fails loudly instead: stderr message, non-zero return, +# no stdout. One probe request first, because a list whose length is an exact +# multiple of the page size hits the ceiling with nothing actually missing, and +# failing on a complete result would be its own bug. tea_api_paged() { _path="$1" _query="$2" @@ -126,19 +141,13 @@ tea_api_paged() { _terminal='' _prev='' while [ "$_page" -le "$GITEA_MAX_PAGES" ]; do - if [ -n "$_query" ]; then - _url="${_path}?${_query}&limit=${GITEA_PAGE_LIMIT}&page=${_page}" - else - _url="${_path}?limit=${GITEA_PAGE_LIMIT}&page=${_page}" - fi - _resp="$(tea api "$_url")" || return 1 + _resp="$(tea api "$(gitea_page_url "$_path" "$_query" "$_page")")" || return 1 # Blank body or an empty array → no more pages. if [ -z "$_resp" ]; then _terminal=1 break fi - # Length AND type in one jq pass; a non-array page is prefixed with "!". - _count="$(printf '%s' "$_resp" | jq -r 'if type == "array" then length else "!" + type end')" || return 1 + _count="$(gitea_page_count "$_resp")" || return 1 case "$_count" in '!'*) echo "gitea forge: page ${_page} of '${_path}' is not an array but a ${_count#!} (an HTTP error body reaches us at exit 0); refusing to return a truncated result" >&2 @@ -151,13 +160,19 @@ tea_api_paged() { fi _acc="$(printf '%s\n%s' "$_acc" "$_resp" | jq -s 'add')" || return 1 if [ -n "$_stop" ]; then - _hit="$(printf '%s' "$_resp" | jq --argjson prev "${_prev:-null}" "$_stop")" || return 1 + # Both pages go in on STDIN, not through `--argjson`. Linux caps a single + # argv string at MAX_ARG_STRLEN (128KiB) regardless of ARG_MAX, and a + # 50-item Gitea pulls page — each object embedding full `base.repo` and + # `head.repo` objects — is routinely ~90KiB and can exceed it. macOS has + # no per-argument cap, so this would have passed locally and failed on CI + # and on every Linux adopter. + _hit="$(printf '%s\n%s\n' "${_prev:-null}" "$_resp" \ + | jq -n "[inputs] as \$i | \$i[0] as \$prev | \$i[1] | ( $_stop )")" || return 1 if [ "$_hit" = "true" ]; then _terminal=1 break fi fi - _prev="$_resp" # A server whose max_response_items is tuned below GITEA_PAGE_LIMIT # truncates every page to its own cap, not the requested limit — so # stopping when a page is shorter than the *requested* limit would break @@ -168,8 +183,29 @@ tea_api_paged() { _terminal=1 break fi + _prev="$_resp" _page=$((_page + 1)) done + + if [ -z "$_terminal" ]; then + # Ceiling reached with every page full. Probe one past it: if there is + # nothing there, the list length was just an exact multiple of the page + # size and what we have is complete. + _resp="$(tea api "$(gitea_page_url "$_path" "$_query" "$((GITEA_MAX_PAGES + 1))")")" || return 1 + if [ -z "$_resp" ]; then + _terminal=1 + else + _count="$(gitea_page_count "$_resp")" || return 1 + case "$_count" in + '!'*) + echo "gitea forge: page $((GITEA_MAX_PAGES + 1)) of '${_path}' is not an array but a ${_count#!} (an HTTP error body reaches us at exit 0); refusing to return a truncated result" >&2 + return 1 + ;; + esac + [ "$_count" -eq 0 ] && _terminal=1 + fi + fi + if [ -z "$_terminal" ]; then echo "gitea forge: pagination for '${_path}' reached the ${GITEA_MAX_PAGES}-page ceiling without a terminal page; refusing to return a truncated result" >&2 return 1 diff --git a/packages/codev/scripts/forge/gitea/issue-view.sh b/packages/codev/scripts/forge/gitea/issue-view.sh index 48d203a0e..9c3b3ddda 100755 --- a/packages/codev/scripts/forge/gitea/issue-view.sh +++ b/packages/codev/scripts/forge/gitea/issue-view.sh @@ -21,9 +21,10 @@ # degrades to [], but warns on stderr so the degraded path is distinguishable # from a genuinely uncommented issue (stdout stays pure JSON — it's parsed by # forge.ts). `tea api` exits 0 on HTTP errors and prints the error BODY, so the -# degrade check tests for an actual JSON array rather than only for a blank -# response — an error OBJECT reached `--argjson` and blew up with a raw jq -# parse/iteration error instead of the warned [] degrade. +# degrade check tests for an actual JSON array of objects rather than only for a +# blank response — an error OBJECT reached `--argjson` and blew up with a raw jq +# parse/iteration error instead of the warned [] degrade, and a non-object +# element would do the same on `.body`. # # SHAPE VALIDATION. Same exit-0-on-error problem for the issue itself: an error # body normalized into an all-null IssueViewResult whose `url` was the error @@ -53,12 +54,14 @@ printf '%s' "$ISSUE" | jq -e ' else ("gitea forge: unexpected `tea api` response for issue " + (env.CODEV_ISSUE_ID // "?") + ": " - + (if type == "object" then (.message // tostring) else tostring end) + + ((.message // .) | tostring) + "\n") | halt_error(1) end' >/dev/null || exit 1 COMMENTS_JSON="$(tea api "repos/${REPO}/issues/${CODEV_ISSUE_ID}/comments" 2>/dev/null)" -if [ -z "$COMMENTS_JSON" ] || ! printf '%s' "$COMMENTS_JSON" | jq -e 'type == "array"' >/dev/null 2>&1; then +if [ -z "$COMMENTS_JSON" ] \ + || ! printf '%s' "$COMMENTS_JSON" \ + | jq -e 'type == "array" and all(.[]; type == "object")' >/dev/null 2>&1; then echo "gitea forge: comments fetch failed for issue ${CODEV_ISSUE_ID}; reporting 0 comments" >&2 COMMENTS_JSON="[]" fi diff --git a/packages/codev/scripts/forge/gitea/pr-view.sh b/packages/codev/scripts/forge/gitea/pr-view.sh index 2774424a2..d4ef6dcf4 100755 --- a/packages/codev/scripts/forge/gitea/pr-view.sh +++ b/packages/codev/scripts/forge/gitea/pr-view.sh @@ -49,7 +49,7 @@ printf '%s' "$PR" | jq ' else ("gitea forge: unexpected `tea api` response for pull " + (env.CODEV_PR_NUMBER // "?") + ": " - + (if type == "object" then (.message // tostring) else tostring end) + + ((.message // .) | tostring) + "\n") | halt_error(1) end | { diff --git a/packages/codev/scripts/forge/gitea/user-identity.sh b/packages/codev/scripts/forge/gitea/user-identity.sh index 50aa6e144..55c9b302a 100755 --- a/packages/codev/scripts/forge/gitea/user-identity.sh +++ b/packages/codev/scripts/forge/gitea/user-identity.sh @@ -27,6 +27,6 @@ printf '%s' "$USER_JSON" | jq -r ' then .login else ("gitea forge: unexpected `tea api user` response: " - + (if type == "object" then (.message // tostring) else tostring end) + + ((.message // .) | tostring) + "\n") | halt_error(1) end' diff --git a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts index 06e525833..3de4a6aee 100644 --- a/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts +++ b/packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts @@ -228,6 +228,41 @@ case "$2" in "repos/acme/errorpage/pulls?state=all&limit=50&page=2") echo 'null' ;; + # --- exactly-full last page: 100 pages of 5, and page 101 is empty. The + # ceiling is reached with every page full, but nothing is actually missing — + # the list length was just an exact multiple of the page size. The probe past + # the ceiling must find that out instead of failing a complete result. + # (The page=101 pattern must precede the catch-all glob below it.) --- + repos/acme/exact/pulls*page=101) + echo '[]' ;; + repos/acme/exact/pulls*) + jq -cn '[range(5)|{number:(5500+.),title:"pad",html_url:"u",body:"",state:"open",created_at:"d",user:{login:"pad"},requested_reviewers:[],draft:false}]' ;; + + # --- heavy pages: each item carries a ~3KB body, so a 50-item page is ~150KB + # — past Linux MAX_ARG_STRLEN (128KiB), the per-argument cap that applies + # regardless of ARG_MAX. Passing a page to the stop filter through + # \`--argjson\` blows up with E2BIG there while passing fine on macOS, which + # has no per-arg cap. Same shape as acme/dated: page 1 all recent, page 2 + # crosses the cutoff, page 3 is a poison pill. --- + "repos/acme/heavy/pulls?state=closed&sort=recentupdate&limit=50&page=1") + jq -cn '[range(50)|(1783537200 - (. * 3600)) as $u|{number:(7500+.),title:"heavy",html_url:"u",body:("x" * 3000),state:"closed",merged:(. == 0),merged_at:(if . == 0 then ($u|todateiso8601) else null end),created_at:"2026-05-01T00:00:00Z",updated_at:($u|todateiso8601),head:{ref:"heavy"}}]' ;; + "repos/acme/heavy/pulls?state=closed&sort=recentupdate&limit=50&page=2") + jq -cn '[range(50)|(1783357200 - (. * 3600)) as $u|{number:(7600+.),title:"heavy",html_url:"u",body:("x" * 3000),state:"closed",merged:false,created_at:"2026-05-01T00:00:00Z",updated_at:($u|todateiso8601),head:{ref:"heavy"}}]' ;; + "repos/acme/heavy/pulls?state=closed&sort=recentupdate&limit=50&page=3") + echo "fake-tea: heavy page 3 requested" >&2; exit 9 ;; + + # --- a non-string \`.message\`, as a proxy or gateway between tea and Gitea + # can produce. String-concatenating it into the error text throws a raw jq + # error instead of the legible message these blocks exist to give. --- + repos/acme/widgets/pulls/500) + echo '{"message":{"nested":"upstream refused"},"url":"https://git.example.com/api/swagger"}' ;; + # issue 96: comments come back as an array of NON-objects, which reaches + # \`$comments[] | .body\` and dies with "Cannot index number with body". + repos/acme/widgets/issues/96) + echo '{"number":96,"title":"Scalar comments","body":"body","state":"open","html_url":"https://git.example.com/acme/widgets/issues/96","comments":2}' ;; + repos/acme/widgets/issues/96/comments) + echo '[1,2]' ;; + # --- empty bodies at exit 0 (pull/issue 0) ------------------------------ repos/acme/widgets/pulls/0) : ;; repos/acme/widgets/issues/0) : ;; @@ -301,7 +336,16 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` execFileSync('git', ['init', '-q'], { cwd: repoDir }); execFileSync('git', ['remote', 'add', 'origin', 'git@git.example.com:acme/widgets.git'], { cwd: repoDir }); - runEnv = { ...process.env, PATH: `${binDir}:${process.env.PATH ?? ''}` }; + // Hermetic: the scripts read CODEV_* from the environment, and several tests + // assert on the ABSENCE of one (recently-merged with no CODEV_SINCE_DATE + // must walk unbounded). Inheriting the developer's shell — or a codev + // invocation path that exports CODEV_REPO — would quietly change what is + // being tested, so every CODEV_* is stripped from the base env and each + // test supplies exactly the ones it means. + const inherited = Object.fromEntries( + Object.entries(process.env).filter(([k]) => !k.startsWith('CODEV_')), + ); + runEnv = { ...inherited, PATH: `${binDir}:${process.env.PATH ?? ''}` }; }); afterAll(() => { @@ -532,6 +576,11 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` // `printf`, none of which is on PATH, so doctor reported them as missing // tools and stopped checking for `tea`. The `# forge-executable:` header // (#1458) declares it explicitly. + // + // This asserts the declaration is PRESENT, not that doctor reads it: + // `extractExecutable` only learns to honor the header when #1458 lands, and + // until then it still reports `.` for these five. The header is an inert + // comment in the meantime, which is why it is safe to add first. for (const name of [ 'pr-exists.sh', 'pr-list.sh', @@ -708,6 +757,49 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` } }); + it('an exactly-full last page at the ceiling is not mistaken for truncation', () => { + // 100 full pages then an empty page 101: the ceiling is reached with every + // page full, but the list length was just an exact multiple of the page + // size and nothing is missing. Failing here would be a hard error on a + // complete result. + const list = JSON.parse(runScript('pr-list.sh', { CODEV_REPO: 'acme/exact' })); + expect(list).toHaveLength(500); // 100 pages x 5 + }, 30_000); + + it('the stop filter handles pages larger than the Linux per-argument cap', () => { + // Each page here is ~150KB, past Linux MAX_ARG_STRLEN (128KiB) — the cap on + // a SINGLE argv string, which applies regardless of ARG_MAX. Handing the + // page to the stop filter through `--argjson` fails with E2BIG on Linux + // while passing on macOS, which has no per-argument cap; both pages go in + // on stdin instead. This assertion is therefore mostly load-bearing on CI. + const { status, stdout, stderr } = runScriptFull('recently-merged.sh', { + CODEV_REPO: 'acme/heavy', + CODEV_SINCE_DATE: '2026-07-05T00:00:00Z', + }); + expect(stderr).toBe(''); + expect(status).toBe(0); + const merged = JSON.parse(stdout); + expect(merged).toHaveLength(1); + expect(merged[0]).toMatchObject({ number: 7500, headRefName: 'heavy' }); + }); + + it('reports a non-string `.message` instead of dying on the concatenation', () => { + // A proxy or gateway between tea and Gitea can put an object there. + const { status, stdout, stderr } = runScriptFull('pr-view.sh', { CODEV_PR_NUMBER: '500' }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + expect(stderr).toContain('upstream refused'); + expect(stderr).not.toContain('jq: error'); + }); + + it('issue-view degrades when the comments array holds non-objects', () => { + // `[1,2]` passes an outer type check but dies on `$comments[] | .body`. + const { status, stdout, stderr } = runScriptFull('issue-view.sh', { CODEV_ISSUE_ID: '96' }); + expect(status).toBe(0); + expect(JSON.parse(stdout).comments).toEqual([]); + expect(stderr).toContain('comments fetch failed for issue 96'); + }); + it('user-identity rejects a whitespace-only login', () => { const { status, stdout } = runScriptFull('user-identity.sh', { FAKE_TEA_USER_BLANK: '1' }); expect(status).not.toBe(0); From ee2fce473a1e9dec56ea48e79dfc3ac2eb20b84d Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Thu, 3 Sep 2026 21:36:54 -0700 Subject: [PATCH 13/14] [Bugfix #1137] Builder thread: maintainer follow-up on PR #1146 Records what the two consultation lanes broke and why, the ordering argument that did not hold, the Linux-only argv finding that could not reproduce on macOS, and the reason the forge-executable header had to go on six scripts rather than the five the review named. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch --- codev/state/task-SVsf_thread.md | 128 ++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 codev/state/task-SVsf_thread.md diff --git a/codev/state/task-SVsf_thread.md b/codev/state/task-SVsf_thread.md new file mode 100644 index 000000000..4418a6010 --- /dev/null +++ b/codev/state/task-SVsf_thread.md @@ -0,0 +1,128 @@ +# task-SVsf — finishing external PR #1146 (gitea forge preset vs. the real `tea` CLI) + +Issue #1137, PR #1146 by **pseudoseed** (Chris Dodge). Not a fresh build: the contribution +was already complete and twice reviewed, and had waited two months for a maintainer pass. +The architect's call was that we take the last mile rather than hand the contributor another +list — `maintainerCanModify` is true, so this builder pushes directly onto +`pseudoseed:builder/bugfix-1137`. + +**Standing constraint: no rebase, no squash, no force.** Every existing commit and its +authorship stay exactly as they are; my work is commits on top. + +## What the review asked for + +From waleedkadous' 2026-09-03 REQUEST_CHANGES, all in the PR's own spirit — fail loudly, +never silently: + +1. `# forge-executable: tea` on the scripts that now source `_lib.sh` (doctor regression). +2. `GITEA_MAX_PAGES` must be an error, not a stop condition. +3. `tea api` exits 0 on HTTP errors — validate the shape before normalizing. +4. `recently-merged` must honor `CODEV_SINCE_DATE`. + +Plus two take-or-leave items: document `CODEV_REPO`'s dual meaning, and fix the stale +`reviewRequests`/`isDraft` comments in `forge-contracts.ts`. + +## Decisions worth recording + +**Six scripts got the header, not five.** The review counted the five that source `_lib.sh`. +But fixing item 3 in `user-identity.sh` required capturing `tea api user` into a variable +before the jq pipe (POSIX sh has no pipefail), which moves `tea` off the first substantive +line — `extractExecutable` would then have reported `printf`. So the header went on +`user-identity.sh` too, or the fix for item 3 would have *caused* the very regression item 1 +was closing. + +**The since-date bound refuses to trust the server's sort.** Bounding a page walk by date +needs an ordering assumption. `sort=recentupdate` gives update-time-descending order, and +`updated_at >= merged_at` always holds (a merge updates the PR), so the first page reaching +back past the cutoff is the last one worth fetching. But a server that ignores an unknown +`sort` parameter falls back to created-desc, and stopping there would silently drop merges of +old PRs — a data loss the caller could never detect. So the stop filter fires only when the +page is *actually* non-increasing in `updated_at`, which proves the server honored the sort. +If it didn't, we fall back to the previous unbounded walk: slower, never wrong. There's a +test for each branch (`acme/dated` stops at page 1, `acme/unsorted` walks on). + +**Timestamps needed a real parser.** Gitea marshals times as RFC3339 in the *server's* +timezone, so `2026-07-05T14:00:00+02:00` is a real response and `Z` is not guaranteed. +`fromdateiso8601` only accepts `Z`, and lexicographic comparison across mixed offsets is +simply wrong. `_lib.sh` now carries a `GITEA_JQ_LIB` prelude defining `gitea_epoch`, which +parses the offset and subtracts it. Unparseable input yields null, and every caller treats +null as "don't know" — keep the item, keep walking. Over-reporting is harmless here (the +caller re-filters the 24h window); dropping a real merge is not. + +**Error-body validation is type-checking, not truthiness.** Gitea's error bodies carry a +`url` key (the swagger link). `url: (.html_url // .url)` therefore *succeeded* on them and +shipped the swagger link as the PR's browser page inside an otherwise all-null contract +object, at exit 0. The validators require the specific fields to be the specific types, and +fail with the server's own `.message` on stderr via jq's `halt_error(1)`. + +## Testing + +Everything lands in the existing real-script fake-CLI suite +(`packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts`) — no new harness. The fake +`tea` grew: error bodies at exit 0 for pulls/issues/user, a comments endpoint answering with +an error *object* rather than failing, an `acme/endless` repo whose pages never end, and the +two sorted/unsorted since-date repos. + +The endless fixture serves 5 items per page, not 50. The paginator's short-page check +compares against the size observed on page 1, so a uniform page size of any value is never +"short" — the ceiling still fires after 100 pages, and the test drops from ~6s to well under +one. That test carries an explicit 30s timeout anyway; 100 sequential process spawns is more +than vitest's 5s default allows for. + +30 tests in the file, all green. + +## CMAP review (codex + claude) — both lanes broke the same argument + +Worth recording in full, because the finding both lanes converged on was the one I was +most confident about. + +**My stop filter checked only that the CURRENT page was non-increasing in `updated_at`, and +I claimed that proved the server honored `sort=recentupdate`. It does not.** Codex gave the +general shape (a server sorting per-page rather than globally); Claude built the concrete +repro: Gitea's default order is index/created-DESC, and on a repo where PRs are opened and +merged in order, index-DESC *is* non-increasing in `updated_at`. A long-lived PR opened in +January and merged after the cutoff then sits on page 2, and we stop at page 1 and drop it. +Claude ran it: 50 items and the merge missing, versus 51 with the walk unbounded. + +The fix is to check for the property we actually need — the ordering — rather than for +evidence that we asked for it. The filter now requires the order to survive a page boundary +(previous page descending too, its oldest no older than this page's newest) and never fires +on page 1, where there is nothing to compare against. Claude patched the same guard in +independently and confirmed it recovers the dropped merge at a cost of exactly one extra +request on an honest server. `acme/lagging` is that fixture; the older `acme/unsorted` is a +much weaker adversary (it alternates every other item, so it fails the within-page check and +never exercised the case that loses data) and is kept only for that weaker branch. + +**The finding I could not have caught locally**: passing a page to the stop filter via +`--argjson` blows up on Linux. `MAX_ARG_STRLEN` caps a *single* argv string at 128KiB +independently of `ARG_MAX`, and a 50-item Gitea pulls page — every object embedding full +`base.repo` and `head.repo` — measures ~90KiB before anyone writes a long PR body. macOS has +no per-argument cap, so it passed here and would have failed on CI and on every Linux +adopter. Both pages go in on stdin now. `acme/heavy` serves ~150KB pages so the regression +bites where the bug lives. + +Also from the two lanes: the page ceiling false-positived on a complete result whose length +was an exact multiple of the page size (now probes one page past before failing); `jq length` +is 0 for both `null` and `{}`, so an error body mid-walk looked exactly like an exhausted +list; jq on empty stdin emits nothing and exits 0, so `pr-view`/`user-identity` were +"succeeding" with empty stdout and never running their validators at all; a non-string +`.message` threw a raw jq error instead of the legible one; and the test env inherited +`CODEV_*` from the developer's shell, so the "no CODEV_SINCE_DATE" test was asserting the +absence of a variable it did not control. + +Two self-inflicted bugs the tests caught: inside a jq `range` body `.` is the range value, +not the array (my `descending` helper silently threw), and an apostrophe inside a +single-quoted jq program closes the shell string. + +## #1458 merged mid-flight + +The architect confirmed #1458 landed at 04:34 UTC. Merged `origin/main` into the branch — +clean, no conflicts — and verified against the MERGED `extractExecutable` rather than a +simulation of it: all fourteen gitea concepts, the five `_lib.sh`-sourcing ones included, +resolve to `tea`. The doctor regression is closed for real, not just declared. + +Ordering note for whoever reads this later: the header had to go on six scripts, not the +five the review named. Fixing `user-identity`'s exit-0-on-error handling required capturing +`tea api user` before the jq pipe, which moves `tea` off the first substantive line — so +without a header of its own, the fix for one review item would have caused the regression a +different item was closing. From a6eddb6b515d1c2e1a65c80f5953dba1c045be2e Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Thu, 3 Sep 2026 21:42:36 -0700 Subject: [PATCH 14/14] [Bugfix #1137] Builder thread: record the shellper-husk-sweep CI flake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failed once on the first run of the pushed branch, passed on re-run with no code change. Unrelated to this PR — nothing here touches Tower. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch --- codev/state/task-SVsf_thread.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/codev/state/task-SVsf_thread.md b/codev/state/task-SVsf_thread.md index 4418a6010..b74d8b7c9 100644 --- a/codev/state/task-SVsf_thread.md +++ b/codev/state/task-SVsf_thread.md @@ -126,3 +126,14 @@ five the review named. Fixing `user-identity`'s exit-0-on-error handling require `tea api user` before the jq pipe, which moves `tea` off the first substantive line — so without a header of its own, the fix for one review item would have caused the regression a different item was closing. + +## Flaky Tests + +`shellper-husk-sweep.e2e.test.ts` > "reaps a genuine husk (unregistered + childless) on the +next periodic tick" (Tower Integration Tests) failed once on the first CI run of the pushed +branch and passed on re-run with no code change. It asserts `isAlive(pid)` immediately after +`createPersistentTerminal` returns — a process-liveness race, PIR #1227's territory. Nothing +in this PR touches Tower: the diff is gitea forge scripts, `forge-contracts.ts` comments, two +skill docs and one test file. Left alone rather than skipped, since one observation is not +enough to call it chronically flaky; noted here so the next person who sees it red has a +prior.