diff --git a/.claude/skills/forge/SKILL.md b/.claude/skills/forge/SKILL.md index f24b4ae37..54618c7f9 100644 --- a/.claude/skills/forge/SKILL.md +++ b/.claude/skills/forge/SKILL.md @@ -47,6 +47,56 @@ 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. +### Gitea / Forgejo specifics + +Forgejo has no GitHub-style PR search and charges its `/pulls` list endpoint per +returned PR object (~0.65s each, measured against Forgejo 15.x), so the gitea +scripts avoid list endpoints wherever a targeted one exists. + +| Concept | How gitea answers it | +|---|---| +| `pr-exists` | `GET pulls/{base}/{head}` — one request. Base is `CODEV_PR_BASE`, else the repo's default branch. | +| `pr-search` | `head:` queries use the same base/head lookup; issue-number queries search `issues?type=pulls&q=` and then resolve each match. | +| `pr-diff` | `pulls/{n}.diff`, or `pulls/{n}/files` for `CODEV_DIFF_NAME_ONLY=1`. | +| `recently-merged` | `issues?type=pulls&state=closed&since=` (a server-side window), then one `pulls/{n}` per match for the head branch. | +| `team-activity`, `on-it-timestamps` | Disabled, permanently. Both are `gh api graphql` pass-throughs and Forgejo has no GraphQL. Callers say so on stderr rather than returning empty. | + +**pr-search query grammar.** The query is parsed, not forwarded. Understood +terms: `head:`, `is:open`, `is:merged`, `is:closed`, `in:body`, and a +bare issue number (`123` or `#123`). With no `is:` qualifier the search spans +every state, so a merged PR is findable after the fact. Anything outside the +grammar returns `[]` and says so on stderr rather than guessing. + +**A merged PR's branch name.** Gitea rewrites `head.ref` to `refs/pull/N/head` +once a merged PR's source branch is deleted, but `head.label` keeps the branch +name. The gitea scripts read `head.label` first, which is what lets `pr-exists` +and `pr-search head:` still find a merged PR. + +**Base branches other than the default.** `pr-exists` and `pr-search head:` need +a base branch. They use the repository's default unless `CODEV_PR_BASE` is set, +so a PR targeting an integration branch needs that variable. + +### Environment overrides + +| Variable | Default | Effect | +|---|---|---| +| `CODEV_REPO` | derived from `origin` | `owner/repo` for the gitea scripts | +| `CODEV_PR_BASE` | the repo's default branch | base branch for `pr-exists` / `pr-search head:` | +| `CODEV_FORGE_TIMEOUT` | 60 | seconds before a single `tea api` call is killed and reported | +| `CODEV_FORGE_PAGED_DEADLINE` | 120 | seconds before a paged walk stops early (exit 3) | +| `CODEV_FORGE_CONCURRENCY` | 8 | parallel PR fetches | +| `CODEV_FORGE_MERGED_DAYS` | 7 | `recently-merged` window when `CODEV_SINCE_DATE` is unset | +| `CODEV_FORGE_MERGED_MAX` | 300 | merged PRs `recently-merged` will resolve before refusing | +| `CODEV_FORGE_SEARCH_MAX` | 10 | PRs `pr-search` resolves for one issue number | + +### Exit statuses + +`0` is an answer and `1` is a failure, as usual. **`3` means the result was +truncated**, and the concept prints nothing on stdout when it returns it: a +partial list is indistinguishable from a complete one once printed, so "nothing +matched" (`[]`, status 0) and "I stopped looking" (status 3) are deliberately +different. `2` is a missing or unusable input. + ### 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 f24b4ae37..54618c7f9 100644 --- a/.codex/skills/forge/SKILL.md +++ b/.codex/skills/forge/SKILL.md @@ -47,6 +47,56 @@ 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. +### Gitea / Forgejo specifics + +Forgejo has no GitHub-style PR search and charges its `/pulls` list endpoint per +returned PR object (~0.65s each, measured against Forgejo 15.x), so the gitea +scripts avoid list endpoints wherever a targeted one exists. + +| Concept | How gitea answers it | +|---|---| +| `pr-exists` | `GET pulls/{base}/{head}` — one request. Base is `CODEV_PR_BASE`, else the repo's default branch. | +| `pr-search` | `head:` queries use the same base/head lookup; issue-number queries search `issues?type=pulls&q=` and then resolve each match. | +| `pr-diff` | `pulls/{n}.diff`, or `pulls/{n}/files` for `CODEV_DIFF_NAME_ONLY=1`. | +| `recently-merged` | `issues?type=pulls&state=closed&since=` (a server-side window), then one `pulls/{n}` per match for the head branch. | +| `team-activity`, `on-it-timestamps` | Disabled, permanently. Both are `gh api graphql` pass-throughs and Forgejo has no GraphQL. Callers say so on stderr rather than returning empty. | + +**pr-search query grammar.** The query is parsed, not forwarded. Understood +terms: `head:`, `is:open`, `is:merged`, `is:closed`, `in:body`, and a +bare issue number (`123` or `#123`). With no `is:` qualifier the search spans +every state, so a merged PR is findable after the fact. Anything outside the +grammar returns `[]` and says so on stderr rather than guessing. + +**A merged PR's branch name.** Gitea rewrites `head.ref` to `refs/pull/N/head` +once a merged PR's source branch is deleted, but `head.label` keeps the branch +name. The gitea scripts read `head.label` first, which is what lets `pr-exists` +and `pr-search head:` still find a merged PR. + +**Base branches other than the default.** `pr-exists` and `pr-search head:` need +a base branch. They use the repository's default unless `CODEV_PR_BASE` is set, +so a PR targeting an integration branch needs that variable. + +### Environment overrides + +| Variable | Default | Effect | +|---|---|---| +| `CODEV_REPO` | derived from `origin` | `owner/repo` for the gitea scripts | +| `CODEV_PR_BASE` | the repo's default branch | base branch for `pr-exists` / `pr-search head:` | +| `CODEV_FORGE_TIMEOUT` | 60 | seconds before a single `tea api` call is killed and reported | +| `CODEV_FORGE_PAGED_DEADLINE` | 120 | seconds before a paged walk stops early (exit 3) | +| `CODEV_FORGE_CONCURRENCY` | 8 | parallel PR fetches | +| `CODEV_FORGE_MERGED_DAYS` | 7 | `recently-merged` window when `CODEV_SINCE_DATE` is unset | +| `CODEV_FORGE_MERGED_MAX` | 300 | merged PRs `recently-merged` will resolve before refusing | +| `CODEV_FORGE_SEARCH_MAX` | 10 | PRs `pr-search` resolves for one issue number | + +### Exit statuses + +`0` is an answer and `1` is a failure, as usual. **`3` means the result was +truncated**, and the concept prints nothing on stdout when it returns it: a +partial list is indistinguishable from a complete one once printed, so "nothing +matched" (`[]`, status 0) and "I stopped looking" (status 3) are deliberately +different. `2` is a missing or unusable input. + ### Disabling concepts Set a concept to `null` to disable it: diff --git a/codev/plans/12-forgejo-gitea-forge-parity.md b/codev/plans/12-forgejo-gitea-forge-parity.md new file mode 100644 index 000000000..7f410e74a --- /dev/null +++ b/codev/plans/12-forgejo-gitea-forge-parity.md @@ -0,0 +1,217 @@ +# PIR Plan: Forgejo/Gitea forge parity — pr-search, pr-diff, and the pr-exists hang + +Issue: #12 · Branch: `builder/pir-12` · Verification target: `~/dev/entriq` (live Forgejo 15.x at `git.pseudoseed.com/pseudoseed/entriq`, tea 0.14.2) + +## Understanding + +Three gaps keep a Forgejo repo from running on the bare `gitea` preset. + +### 1. `pr-exists` does not hang — it takes about seventeen minutes + +I reproduced it live and measured the cost. It is not auth, not connectivity, and not an infinite loop. + +`packages/codev/scripts/forge/gitea/pr-exists.sh:27` calls `tea_api_paged "repos/${REPO}/pulls" "state=all"`, and `_lib.sh:56-75` walks every page at 50 items per page up to `GITEA_MAX_PAGES=100`. The per-request cost on Forgejo's `/pulls` list is **linear in the number of PR objects returned**, not per request: + +| request | time | +|---|---| +| `GET /repos/pseudoseed/entriq/issues/1` | 0.59 s | +| `GET .../pulls?state=all&limit=1` | 0.78 s | +| `GET .../pulls?state=open&limit=1` | 0.94 s | +| `GET .../pulls?state=all&limit=50` | **32.8 s** | + +That is ≈0.65 s per PR object — Forgejo materialises head/base commit info per pull on this endpoint. `x-total-count` on `.../pulls?state=all` reports **1599 PRs** in entriq, so `tea_api_paged` issues 32 requests totalling ≈17 minutes, on top of `jq -s 'add'` re-serialising the whole accumulator once per page (O(n²) on 1599 items). Killing at 25 s and at 120 s both land inside that window with nothing on stdout, which is exactly what was reported. + +Two consequences for the design: + +- **Raising the page size cannot fix this.** The cost is per item, so any approach that enumerates PRs pays ≈17 minutes on this repo. The fix has to stop enumerating. +- **In-process the symptom is different from the symptom that was reported.** `executeForgeCommand`/`executeForgeCommandSync` both pass `timeout: 30_000` (`forge.ts:335`, `forge.ts:378`), and I verified with a minimal repro that Node's `exec` timeout does fire even when a grandchild holds the stdout pipe (`sh -c "sleep 60 | cat"` rejected at 3009 ms under a 3 s timeout, `killed=true`). So under porch the `pr_exists` check does not hang — it fails after 30 s with `output: "null"` (`checks.ts:360-375`), which reads as "no PR exists" rather than "the concept could not answer". The unbounded hang is only visible when the script is run directly from a shell, which is how it was found. Both symptoms need fixing: the script needs its own timeout, and the check needs to distinguish "answered false" from "could not answer". + +### 2 & 3. `pr-search` and `pr-diff` are disabled for gitea + +`forge.ts:129` lists both in `buildPresetFromScripts('gitea', [...])`, which sets them to `null`. No gitea scripts exist. Forgejo has cheap endpoints for both — I verified all of them live (timings below). + +### The fast primitive for branch→PR lookup + +`GET /repos/{owner}/{repo}/pulls/{base}/{head}` answers "is there a PR from this branch" in one request, and it survives the caveat documented at `gitea/pr-exists.sh:16-20`. That caveat says a merged PR whose source branch was deleted reports `head.ref == "refs/pull/N/head"`, so branch-name matching misses it. True — but **`head.label` retains the original branch name**, and the base/head endpoint matches on the stored head branch, not on `head.ref`. Verified against a merged PR whose branch is gone: + +``` +GET /repos/pseudoseed/entriq/pulls/3869 + → state=closed merged=true head.ref="refs/pull/3869/head" head.label="builder/aspir-3860" + +GET /repos/pseudoseed/entriq/pulls/main/builder/aspir-3860 + → 200 in 1.19 s, returns PR 3869 +GET /repos/pseudoseed/entriq/pulls/main/builder/air-364 + → 200 in 0.98 s, returns PR 3855 (open) +GET /repos/pseudoseed/entriq/pulls/main/no-such-branch-xyz + → 404 in 0.28 s +``` + +Slashes in the head branch pass through unencoded (`.../pulls/main/builder/air-364` works), so no URL-escaping is needed. This makes the current scan-and-filter approach obsolete for both `pr-exists` and the `head:` form of `pr-search`: 1 request, ~1 s, and it is *more* correct than the scan because it finds merged PRs with deleted branches. + +Its one limitation: it needs a base branch. Codev PRs target the default branch except for the sequential-PR case (branch from an integration branch). Handled below. + +### Other verified endpoints + +| purpose | endpoint | time | +|---|---|---| +| full diff | `GET /repos/{o}/{r}/pulls/{n}.diff` | 0.30 s | +| changed files | `GET /repos/{o}/{r}/pulls/{n}/files` | 0.51 s | +| search PRs by text | `GET /repos/{o}/{r}/issues?type=pulls&state=all&q=` | 0.62 s | +| PR list metadata only | `GET /repos/{o}/{r}/issues?type=pulls&state=all&limit=50` | 1.75 s | + +Note the last row against the 32.8 s in the first table: the `issues?type=pulls` view of the same 50 PRs is **19× cheaper** because it skips the commit materialisation. It carries `pull_request.merged` and `pull_request.merged_at` but no head/base refs, so it is the right index to search and the wrong shape to return — matches get their refs from a per-number `GET /pulls/{n}` (~0.3 s each). + +### What upstream cluesmith/codev#1331 establishes + +#1331 (open, unmerged; no PR against it in the fork) fixes `pr-search` to search all PR states, because `gh pr list --search` defaults to `--state open` and post-merge `consult --type pr` therefore failed with "No PR found for branch". Our fork still carries the unfixed `github/pr-search.sh` and `gitlab/pr-search.sh`. + +The review on #1331 is as load-bearing as the fix, and I am taking three things from it: + +1. **All-states breaks a caller.** `spawn-worktree.ts:592` queries `in:body #${issueNumber}` and relies on pr-search's open-only default to mean "open PRs". Under all-states it aborts every re-spawn with a factually wrong "Found N open PR(s)". The required fix is to make that call site say what it means: `in:body #${issueNumber} is:open`. +2. **Explicit `is:` qualifiers must override the default state.** `cleanup.ts:340-347` already depends on this with `head:X is:merged` / `head:X is:open`. +3. **`prs[0]` is not necessarily the live PR** once results span states. `PrSearchItem` (`forge-contracts.ts:113`) carries no `state`, so callers cannot defend themselves. Upstream filed this as a follow-up; since I am writing a search implementation from scratch I will order results deterministically (open first, then most recent) and add optional `state` and `baseRefName` to the contract. + +Consequently the gitea `pr-search.sh` must parse a small query grammar rather than passing a string to a search engine that does not exist on Forgejo. The five query forms actually used in this codebase are: + +| call site | query | +|---|---| +| `consult` `findPRForCurrentBranch` (index.ts:1947) | `head:` | +| `consult` `findPRForIssue` (index.ts:1975) | `` | +| `cleanup` merged check (cleanup.ts:341) | `head: is:merged` | +| `cleanup` open check (cleanup.ts:347) | `head: is:open` | +| `spawn-worktree` (spawn-worktree.ts:592) | `in:body #` → becomes `in:body # is:open` | + +### Out of scope, per the issue + +`team-activity` and `on-it-timestamps` stay `null` for gitea. Both are `exec gh api graphql` pass-throughs and Forgejo has no GraphQL. But the issue also asks that callers "degrade loudly rather than silently", and today one of them does not: + +- `fetchOnItTimestamps` (`github.ts:338-345`) reads `forgeConfig?.['on-it-timestamps']` to decide whether a custom command is configured. For a gitea repo the key is absent from user config (it is `null` in the *preset*, not the config), so `customCmd === undefined` and it falls through to the GraphQL path, calls the concept, gets `null` back, and `continue`s — an empty map with nothing on stderr. Silent. +- `fetchTeamGitHubData` (`team-github.ts:337`) does return `error: 'team-activity concept returned no data'`, which surfaces. It just does not say *why*. + +## Proposed Change + +Four commits inside one PR. + +### Commit 1 — `pr-exists`: replace the scan with the base/head lookup, and give every gitea script a timeout + +`gitea/pr-exists.sh` rewritten to: + +1. Resolve `owner/repo` via the existing `gitea_repo` helper. +2. Resolve the base branch: `CODEV_PR_BASE` if set (the name `pr-create.sh` already uses for a base branch), else the repo's `default_branch` from `GET repos/{owner}/{repo}` — the same resolution `pr-create.sh:78-81` already does. +3. `GET repos/{repo}/pulls/{base}/{branch}`; answer `true` when the response is a PR object with `state == "open"` or `merged == true`, `false` on 404 or a non-PR body. +4. Print `true`/`false` on stdout, nothing else. A *failure* to reach the API exits non-zero with a stderr message rather than printing `false` — "I could not tell" must not be spelled the same way as "no". + +New in `_lib.sh`: + +- `gitea_timeout ` — a portable POSIX watchdog (`gtimeout`/`timeout` when present, else a backgrounded child plus a killer subshell whose stdout is redirected to `/dev/null` so it cannot hold the command-substitution pipe open). Bound by `CODEV_FORGE_TIMEOUT`, default 60 s. +- `gitea_api` — `gitea_timeout $CODEV_FORGE_TIMEOUT tea api …`, with the timeout path reporting `gitea forge: timed out after Ns` on stderr and exiting non-zero. All gitea scripts route through this so a future hang surfaces as an error rather than a stuck phase. +- `tea_api_paged` gains a wall-clock deadline in addition to `GITEA_MAX_PAGES`, and warns loudly on stderr when it stops early rather than silently returning a truncated array. + +`tea_api_paged` stays because `pr-list` and `recently-merged` still use it. I am not rewriting those two here — they are outside this issue's scope and are not overridden in entriq — but I will note in the review that on a 1599-PR repo `recently-merged` (`state=closed`, every page) costs the same ≈17 minutes and should get its own issue. The deadline turns that from a stall into a bounded, loud degradation. + +### Commit 2 — `pr-search` for gitea, plus the `is:open` fix at the spawn call site + +New `gitea/pr-search.sh`. Parses `CODEV_SEARCH_QUERY` into: an optional `head:` term, an optional `in:body` marker, zero or more `is:open` / `is:merged` / `is:closed` qualifiers, and any remaining bare term. Default state when no `is:` qualifier is given is **all states** (#1331's fix). Then: + +- **`head:` present** → one `GET repos/{repo}/pulls/{base}/{branch}` (base resolved as in commit 1). Filter by the requested states. +- **bare issue number, with or without `in:body`** → `GET repos/{repo}/issues?type=pulls&state=&q=` for the candidate index, then `GET repos/{repo}/pulls/{n}` for each of the top matches (capped at 10, with a stderr note if the cap truncates) to get `headRefName`/`baseRefName`. A PR belongs to issue N when a word-bounded `#N` appears in title or body, or when N appears word-bounded in the head branch name — the same rule the working entriq shim uses, so `#3386` never matches `#33861`. +- **anything else** → `[]` on stdout, exit 0, and a one-line stderr note naming the unparsed query. No guessing. + +Output: `[{number, title, state, url, headRefName, baseRefName}]`, ordered open-first then most-recently-updated, so `prs[0]` is the live PR. + +`spawn-worktree.ts:592`: query becomes `in:body #${issueNumber} is:open`, with a comment naming #1331's review as the reason. This is inert for the current open-only `github/pr-search.sh` and load-bearing for gitea. + +### Commit 3 — `pr-diff` for gitea, and enable both concepts in the preset + +New `gitea/pr-diff.sh`: +- `CODEV_DIFF_NAME_ONLY=1` → `GET repos/{repo}/pulls/{n}/files` (paged via `tea_api_paged`), emit `.filename` one per line, matching `gh pr diff --name-only`, which is what `consult`'s `fetchPRData` splits on newlines (`index.ts:1483-1486`). +- otherwise → `GET repos/{repo}/pulls/{n}.diff`, raw text on stdout. +- `tea api` exits 0 on HTTP errors (established by `pr-create.sh:39-42`), so both paths assert the response shape and exit non-zero with a stderr message on an error body, rather than emitting an error page as a "diff". + +`forge.ts:129` becomes `buildPresetFromScripts('gitea', ['team-activity', 'on-it-timestamps'])` — identical to the gitlab line. + +### Commit 4 — loud degradation, contract, docs + +- `github.ts` `fetchOnItTimestamps`: decide via `getForgeCommand('on-it-timestamps', forgeConfig)` / `isConceptDisabled` instead of `forgeConfig?.[...]`, so a preset-disabled concept is recognised. When disabled, warn once on stderr naming the provider and what degrades (analytics falls back to PR `createdAt`) and return the empty map. +- `team-github.ts` `fetchTeamGitHubData`: when the concept is disabled, return an error that says so — `team-activity is not available for provider "gitea" (no GraphQL on Forgejo)` — instead of "returned no data". +- `checks.ts` `runPrExistsViaConcept`: when `executeForgeCommand` returns `null`, fail with an explicit error ("the pr-exists concept returned no usable answer — it failed, timed out, or is disabled") rather than `passed:false, output:"null"`. +- `forge-contracts.ts`: add optional `state` and `baseRefName` to `PrSearchItem`, documenting that `consult`'s `findPRForIssue` already reads `baseRefName` and that ordering is open-first. +- `.claude/skills/forge/SKILL.md` and its byte-identical `.codex/skills/forge/SKILL.md` twin: record the gitea query grammar, `CODEV_FORGE_TIMEOUT`, `CODEV_PR_BASE` for `pr-exists`, and the base-branch limitation. + +No `codev-skeleton/` mirror exists for `scripts/forge/` (confirmed by `find codev-skeleton -path '*forge*'` — empty), so these scripts are single-source. The skills directories are the only twin to keep in sync. + +## Files to Change + +- `packages/codev/scripts/forge/gitea/_lib.sh` — add `gitea_timeout`, `gitea_api`; add a wall-clock deadline and a loud truncation warning to `tea_api_paged` +- `packages/codev/scripts/forge/gitea/pr-exists.sh:25-29` — replace the `state=all` scan with the base/head lookup; distinguish "false" from "could not answer" +- `packages/codev/scripts/forge/gitea/pr-search.sh` — new +- `packages/codev/scripts/forge/gitea/pr-diff.sh` — new +- `packages/codev/src/lib/forge.ts:129` — stop disabling `pr-search` and `pr-diff` for gitea +- `packages/codev/src/lib/forge-contracts.ts:113-116` — `PrSearchItem` gains optional `state`, `baseRefName` +- `packages/codev/src/agent-farm/commands/spawn-worktree.ts:592` — query gains `is:open` +- `packages/codev/src/lib/github.ts:338-357` — detect a preset-disabled `on-it-timestamps`; warn once +- `packages/codev/src/lib/team-github.ts:332-342` — name the reason when `team-activity` is disabled +- `packages/codev/src/commands/porch/checks.ts:360-375` — `null` from the concept is a distinct failure, not `false` +- `packages/codev/src/__tests__/pir-12-gitea-pr-concepts.test.ts` — new; fake-`tea` harness in the style of `bugfix-1455-pr-create-concept.test.ts` +- `.claude/skills/forge/SKILL.md`, `.codex/skills/forge/SKILL.md` — document the gitea specifics (kept byte-identical) +- `codev/state/pir-12_thread.md` — builder log, committed with the PR + +Deliberately unchanged: `github/pr-search.sh` and `gitlab/pr-search.sh`. Adding `--state all` there is upstream #1331's diff, and duplicating it in the fork would collide when #1331 lands. Flagged for the architect below. + +## Risks & Alternatives Considered + +- **Risk: the base/head lookup misses a PR whose base is not the default branch.** Sequential-PR work branches from an integration branch. Mitigation: honour `CODEV_PR_BASE`, document it in the skill, and make the miss recoverable — `pr-exists` returning `false` blocks the porch `pr_exists` gate loudly rather than proceeding on a wrong answer. Rejected the obvious hedge (fall back to scanning open PRs when the lookup 404s) because on a repo with many open PRs that reintroduces exactly the 0.65 s-per-item cost this plan exists to remove, on the *failure* path where it would be least expected. +- **Risk: `head.label` is `owner:branch` for a cross-repo fork PR.** Codev builders push branches to the same repo, so the same-repo form is what we see. `pr-search` will pass a head containing `:` through verbatim, matching `pr-create.sh:311`'s established handling. +- **Risk: the query grammar drifts from its callers.** Five call sites, all in this repo, all listed above. Mitigated by testing the exact five strings. +- **Alternative: raise `GITEA_PAGE_LIMIT` / keep scanning.** Rejected — measured cost is per item, not per request; 1599 items cost ≈17 minutes at any page size. +- **Alternative: resolve branch→PR through `git ls-remote origin 'refs/pull/*/head'` and SHA matching.** One cheap network call and base-branch-independent, but it needs the branch's SHA, which is exactly what is gone after a merged branch is deleted. Rejected as fragile. +- **Alternative: raise the Forgejo server's `max_response_items`.** Rejected — it is a per-adopter server setting, and codev cannot require one. +- **Alternative: keep `pr-search` disabled and let entriq keep its shim.** Rejected — it is the issue's first acceptance criterion. + +## Test Plan + +### Unit — `packages/codev/src/__tests__/pir-12-gitea-pr-concepts.test.ts` + +Fake `tea` on `PATH` recording its argv and replaying canned responses, in the style of `bugfix-1455-pr-create-concept.test.ts` (`it.skipIf(!hasJq())` for the jq-dependent cases). + +- `pr-exists` issues exactly **one** `pulls/{base}/{head}` request and **never** a `state=all` list request — the regression pin for the 17-minute scan. +- `pr-exists` → `true` for an open PR; `true` for `merged: true` with `head.ref == refs/pull/N/head`; `false` on 404; **non-zero exit** (not `false`) when `tea` fails. +- `pr-exists` resolves the base from `default_branch` when `CODEV_PR_BASE` is unset, and prefers `CODEV_PR_BASE` when set. +- `gitea_timeout` kills a `tea` that never returns, exits non-zero, and prints the endpoint — asserted against a fake `tea` that sleeps. +- `pr-search` for each of the five real query strings: `head:X`, `head:X is:merged`, `head:X is:open`, ``, `in:body # is:open`. Assert the request shape and that no-`is:` defaults to all states (#1331). +- `pr-search` word-bounded issue matching: query `3386` does not match a PR titled `[Spec 33861]`. +- `pr-search` orders open before merged. +- `pr-search` emits `[]` and exits 0 on an unparseable query. +- `pr-diff` name-only emits bare filenames one per line; full mode emits the raw diff; an error body exits non-zero instead of printing it. +- `forge.ts`: `gitea` preset resolves `pr-search` and `pr-diff` to script paths, and still reports `team-activity` / `on-it-timestamps` as `disabled` (`resolveAllConcepts`). +- `spawn-worktree` passes a query containing `is:open` — anchored to the call, since the existing spawn test mocks the forge layer. +- `.claude/skills/forge/SKILL.md` and `.codex/skills/forge/SKILL.md` are byte-identical. + +Assertions on script *content* are anchored to the command line (`^exec`, argv shape), not `toContain`, so an explanatory comment cannot make a test pass with the code removed — the flaw called out in #1331's review. + +### Live verification against Forgejo — the real gate + +Run from `~/dev/entriq`, against this worktree's scripts, with `~/codev-evidence/entriq-backup-20260821-112045` as the restore point. Timings recorded in the review. + +1. **Before**, with the overrides still in place, capture the baseline. +2. Point entriq at this branch's build and **delete all three overrides** (`issue-view`, `pr-exists`, `pr-search`) from `~/dev/entriq/.codev/config.json`, leaving `"provider": "gitea"` alone. +3. `pr-exists` for `builder/air-364` (open PR 3855) → `true`, **under 5 s**. +4. `pr-exists` for `builder/aspir-3860` (merged PR 3869, branch deleted) → `true` — the case the old scan could not answer at all. +5. `pr-exists` for a branch with no PR → `false`, under 5 s. +6. `pr-search` `head:builder/air-364` → PR 3855 with `headRefName` and `baseRefName`. +7. `pr-search` `3860` → PR 3869, `state` present, open-first ordering. +8. `pr-search` `in:body # is:open` for an issue whose PR is merged → `[]`, i.e. `afx spawn` is not blocked (the #1331 regression). +9. `pr-diff` PR 3855 → real diff (≈30 KB); `CODEV_DIFF_NAME_ONLY=1` → bare filenames. +10. `issue-view` `CODEV_ISSUE_ID=1` → correct JSON on the bare preset, with the override gone. +11. `codev doctor` in entriq reports `pr-search`/`pr-diff` as `preset` and `team-activity`/`on-it-timestamps` as `disabled`. +12. **Restore entriq** to the backup unless every step above passed. Either way, state plainly in the review which of the three overrides are gone and what entriq was left in. + +### Regression + +`npm run build` and `npm test` in this worktree; specifically the existing `forge.test.ts`, `bugfix-1137-gitea-tea-api.test.ts`, `bugfix-1455-pr-create-concept.test.ts`, `doctor.test.ts`, and `spawn-worktree.test.ts`. + +## For the Architect + +1. **`github/pr-search.sh` and `gitlab/pr-search.sh` stay unfixed in this PR.** They carry the exact #759 bug — `gh pr list --search` with no `--state`, so post-merge `consult --type pr` fails on GitHub too. I left them alone because fixing them here duplicates upstream #1331's diff and will conflict when it lands. Say the word if you would rather carry it in the fork. +2. **`recently-merged` and `pr-list` for gitea have the same ≈17-minute cost on a 1599-PR repo.** Neither is overridden in entriq, so neither blocks this issue's acceptance. My plan bounds them with a deadline and a loud warning rather than rewriting them. Tell me if you want the real fix in scope. +3. **Porch's plan artifact path.** The phase prompt named `codev/plans/0012-hide-tmux-status-bar.md` — a pre-existing 2025 plan for an unrelated project. This is the zero-padding collision `artifacts.ts:80-104` already documents for this fork. I wrote `codev/plans/12-forgejo-gitea-forge-parity.md`, which is the canonical exact-match form `findByProjectId` prefers, so `plan_exists` resolves to this file. diff --git a/codev/projects/12-forgejo-gitea-forge-parity-imp/status.yaml b/codev/projects/12-forgejo-gitea-forge-parity-imp/status.yaml new file mode 100644 index 000000000..bfea08c76 --- /dev/null +++ b/codev/projects/12-forgejo-gitea-forge-parity-imp/status.yaml @@ -0,0 +1,30 @@ +id: '12' +title: forgejo-gitea-forge-parity-imp +protocol: pir +phase: verified +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: approved + requested_at: '2026-08-21T17:42:02.874Z' + approved_at: '2026-08-21T17:42:47.030Z' + dev-approval: + status: approved + requested_at: '2026-08-21T18:54:09.129Z' + approved_at: '2026-08-21T19:00:56.448Z' + pr: + status: approved + requested_at: '2026-08-21T19:25:49.669Z' + approved_at: '2026-08-21T19:27:54.025Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-21T17:29:24.650Z' +updated_at: '2026-08-21T19:28:17.095Z' +pr_history: + - phase: review + pr_number: 19 + branch: builder/pir-12 + created_at: '2026-08-21T19:05:35.673Z' +pr_ready_for_human: false diff --git a/codev/resources/arch.md b/codev/resources/arch.md index d59c64541..5a54d570e 100644 --- a/codev/resources/arch.md +++ b/codev/resources/arch.md @@ -2090,6 +2090,17 @@ All interactions with the repository hosting platform (GitHub by default) are ro **Environment variables**: Each concept receives `CODEV_*` env vars (e.g., `CODEV_ISSUE_NUMBER`, `CODEV_PR_NUMBER`) that the command uses to parameterize its output. +**Provider presets** live as on-disk scripts under `packages/codev/scripts/forge//.sh` and are resolved at runtime by `resolveScriptPath`. There is no `codev-skeleton/` mirror — these scripts are single-source. A concept a provider cannot support is set to `null` in its preset (`buildPresetFromScripts`), which is different from "no script": `null` is a deliberate refusal that callers must handle. `gitea` disables only `team-activity` and `on-it-timestamps`, both `gh api graphql` pass-throughs that Forgejo cannot serve at all. + +**A preset-disabled concept is invisible to `forgeConfig` lookups.** `forgeConfig?.['x']` reads *user config*; a concept nulled by a provider preset is absent from it. Code deciding whether a concept is available must ask `getForgeCommand` / `isConceptDisabled`, not index the config — the former is how a gitea repo silently returned an empty "On it" timestamp map for months. + +**Exit-status contract**: `0` an answer, `1` a failure, `2` a missing or unusable input, and **`3` a truncated result — with empty stdout**. A partial list is indistinguishable from a complete one once printed, so a concept that stops early must not print what it got. `null` reaching a caller from `executeForgeCommand` therefore means "could not answer" (failure, 30s timeout, or disabled) and must never be read as an empty answer. + +**Forge behavior verified against Forgejo 15.x / tea 0.14.2** (PIR #12) — two properties that look otherwise: + +- Gitea's `/repos/{o}/{r}/pulls` list is priced **per returned PR object**, not per request: 0.78s at `limit=1`, 32.8s at `limit=50`. Paging it is linear in total PRs regardless of page size, so on a 1599-PR repo a full walk costs ~17 minutes. Anything answerable by a targeted endpoint must not use it. The cheap index for the same rows is `/issues?type=pulls` (~1.8s per 50), which carries `pull_request.merged_at` but no head/base refs. +- `head.ref` is rewritten to `refs/pull/N/head` once a merged PR's source branch is deleted — the normal state of every merged PR — but **`head.label` retains the original branch name**, and `GET /pulls/{base}/{head}` matches on the stored head branch. Branch→PR lookup is therefore possible after branch deletion, which the earlier `head.ref` scan had documented as impossible. + ### Two remote-command paths into an editor surface (Spec 1401) Tower has **two** ways for an external controller to drive an editor, and picking the wrong one diff --git a/codev/resources/lessons-critical.md b/codev/resources/lessons-critical.md index 65db39512..cdad4cac1 100644 --- a/codev/resources/lessons-critical.md +++ b/codev/resources/lessons-critical.md @@ -9,7 +9,7 @@ MAINTAIN polices the cap and keeps the map in sync with lessons-learned.md's sec - Trust the protocol — never skip CMAP/consultation; it catches security, design, and protocol issues solo review misses. - Check for existing work (PRs, git history) before building from scratch. - "It compiled" / "tests pass" is not "it works" — verify the real user path end-to-end before calling it done. -- Model permissions as roles/capabilities, not booleans — booleans don't extend. +- A truncated result is indistinguishable from a complete one once emitted — give "I stopped early" its own signal and emit nothing, never a partial answer that reads as whole. - Single source of truth beats distributed state — consolidate duplicates rather than syncing them. - After any rename or framework change, grep the whole repo across BOTH codev/ and codev-skeleton/ before claiming "all fixed." - When stuck (2 failed hypotheses or ~30 min), get an outside model's perspective instead of guessing. diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index e365f4fbc..263584b1e 100644 --- a/codev/resources/lessons-learned.md +++ b/codev/resources/lessons-learned.md @@ -68,6 +68,8 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated ## Architecture +- **Model permissions as roles/capabilities, not booleans — booleans don't extend.** A boolean answers one question and has to be joined by another the moment a second kind of actor appears; a capability set answers the general question once. (Demoted from the hot tier in PIR #12 to make room for the partial-result rule, which recurs across more subsystems.) + - [From #4] **Do not port a rendering-attribute convention from one TUI to another without measuring the second one.** claude and codex de-emphasize composer placeholder text with SGR-dim, so the render gate skips dim cells as chrome. agy already broke that (its hint is a @@ -635,6 +637,8 @@ so it survives review. Pin the constant to the highest migration block in a test ## Protocol Orchestration +- [From #12, tracked as #20] **A reviewer that never ran must not be spellable as one that approved.** Porch printed "All reviewers approved!" for a project whose codex lane was quota-blocked and had read nothing, and there are *two* independent routes to that: `parseVerdict` maps anything it does not recognise — including the `VERDICT: SKIPPED` the NOT-RUN convention deliberately writes — to `COMMENT`, which `allApprove` counts as approval; and `allApprove` separately returns `true` when `reviews.length === 0`, so *zero* reviewers approves too. The fall-through's own comment states the bug out loud — "no valid VERDICT line found **but the consult ran**" — because the vocabulary was designed for a reviewer that ran and emitted something unparseable, and a reviewer that never ran cannot be expressed in it at all. Two general rules. First: when a convention exists to record an absence, the code that reads it has to know about that convention, or the convention only reassures the person who wrote it. Second: an enum whose values all mean "something happened" cannot represent nothing happening — check whether the missing case is *unhandled* or *inexpressible*, because widening a default fixes the first and not the second. + - [From 0073] Pure YAML state format is simpler than markdown-with-frontmatter for machine-readable state. Standard format, standard libraries, no custom parsing. - [From 0073] Signal-based transitions (`NAME`) are simple and unambiguous for LLM output parsing. "Last signal wins" resolves ambiguity when multiple signals appear in output. - [From 0073] YAML key naming: use underscores (`spec_approval`) not hyphens (`spec-approval`) for compatibility with YAML parsing via regex. @@ -656,6 +660,10 @@ so it survives review. Pin the constant to the highest migration block in a test ## Debugging and Root Cause Analysis +- [From #12] **Killing a process is not the same as unblocking the caller, and a zero exit is not the same as success.** A shell timeout helper wrapped `tea api` in `$(...)`, killed it on schedule, printed its timeout message on time — and the command substitution stayed blocked for minutes, because a grandchild still held the write end of the pipe. Give the wrapped command a temp file instead of the caller's pipe, and redirect the watchdog's own stdout to `/dev/null` for the same reason. The second trap is subtler: classifying "we killed it" from the exit status (143/137) looks equivalent to recording it and is not — POSIX defines operand-less `wait` as *always* returning zero, so a wrapper script killed by SIGTERM reports success with an empty body, and the caller then misdiagnoses the empty response as a different failure entirely. Have the watchdog record that it fired; infer nothing. +- [From #12] **Measure whether a remote endpoint is priced per request or per returned item before optimising around it.** Gitea's `/pulls` list costs ~0.65s *per PR object* (0.78s at `limit=1`, 32.8s at `limit=50`), so paging it costs the same total at any page size and a 1599-PR repo took ~17 minutes to answer one yes/no question. Every instinct — raise the page size, add a page cap, cache it — is useless against a per-item cost; the only fix is a targeted endpoint. One `limit=1` vs `limit=50` timing settles which regime you are in, and it takes a minute. +- [From #12] **A hang reported from a shell is not always the hang your code has in production.** The same `pr-exists` script that never returned from a terminal was, in-process, being killed at 30s by `executeForgeCommand` and returning `null` — which the caller rendered as "no PR exists". Same root cause, two different bugs, and only one of them was the one being reported. Check what the in-process caller does with the failure before assuming the symptom you were handed is the whole symptom. + - [From #4] **Reproduce a reported finding against a pinned commit, never a working tree.** A reviewer reported a false-CLEAN at specific terminal widths; a follow-up sweep found nothing and the finding was retracted — but that sweep had measured the working tree *mid-edit*, with diff --git a/codev/reviews/12-forgejo-gitea-forge-parity.md b/codev/reviews/12-forgejo-gitea-forge-parity.md new file mode 100644 index 000000000..bf44c6d72 --- /dev/null +++ b/codev/reviews/12-forgejo-gitea-forge-parity.md @@ -0,0 +1,200 @@ +# PIR Review: Forgejo/Gitea forge parity — pr-search, pr-diff, and the pr-exists "hang" + +Fixes #12 + +## Summary + +A Forgejo repository can now run on the bare `forge.provider: gitea` preset with no per-concept overrides and no `gh` shim. `pr-search` and `pr-diff` are implemented for gitea and no longer disabled; `pr-exists` stopped enumerating pull requests and answers in one request instead of about seventeen minutes; and `recently-merged`, which had been silently returning nothing on every `afx status` for months, went from about twenty-six minutes to roughly one second. + +The reported bug was a hang. It was not one — it was a ~17-minute loop, and finding that out changed the fix from "add a timeout" to "stop enumerating." + +## Files Changed + +- `packages/codev/scripts/forge/gitea/_lib.sh` (+245 / -0) — timeout, bounded-concurrency fetch, repo/default-branch resolution, paged-walk deadline +- `packages/codev/scripts/forge/gitea/pr-exists.sh` (+104 / -0) — rewritten around the base/head lookup +- `packages/codev/scripts/forge/gitea/pr-search.sh` (+199 / -0) — new +- `packages/codev/scripts/forge/gitea/pr-diff.sh` (+76 / -0) — new +- `packages/codev/scripts/forge/gitea/recently-merged.sh` (+174 / -0) — rewritten around the cheap issues index +- `packages/codev/scripts/forge/gitea/pr-list.sh` (+14 / -0) — truncation is no longer swallowed by a pipe +- `packages/codev/scripts/forge/gitea/issue-view.sh` (+1 / -0) — `# forge-executable: tea` +- `packages/codev/src/lib/forge.ts` (+57 / -0) — gitea preset enables both concepts; shared "concept unavailable" reporting +- `packages/codev/src/lib/forge-contracts.ts` (+17 / -0) — `PrSearchItem` gains `state`, `baseRefName`, `title`, `url` +- `packages/codev/src/lib/github.ts` (+23 / -0) — a preset-disabled `on-it-timestamps` warns instead of returning empty +- `packages/codev/src/lib/team-github.ts` (+19 / -0) — names the provider instead of "returned no data" +- `packages/codev/src/commands/porch/checks.ts` (+17 / -0) — `null` from `pr-exists` is "could not answer", not "no PR" +- `packages/codev/src/agent-farm/commands/spawn-worktree.ts` (+12 / -0) — the collision query says `is:open` +- `packages/codev/src/__tests__/pir-12-gitea-pr-concepts.test.ts` (+~670 / -0) — new +- `packages/codev/src/commands/porch/__tests__/pir-12-pr-exists-null-vs-false.test.ts` (+77 / -0) — new +- `packages/codev/src/__tests__/bugfix-1137-gitea-tea-api.test.ts` (+110 / -0) — fixtures follow the endpoints +- `packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts` (+18 / -0) +- `.claude/skills/forge/SKILL.md`, `.codex/skills/forge/SKILL.md` (+50 each) — kept byte-identical +- `codev/resources/arch.md` (+11 / -0), `codev/resources/lessons-learned.md` (+6 / -0), `codev/resources/lessons-critical.md` (+1 / -1) +- `codev/plans/12-forgejo-gitea-forge-parity.md` (+217 / -0), `codev/state/pir-12_thread.md` (+111 / -0) + +24 files changed, 2108 insertions, 75 deletions. + +## Commits + +- `28e764f8c` [PIR #12] Plan draft: gitea pr-search, pr-diff, and the pr-exists hang +- `63c980789` [PIR #12] fix(gitea): answer pr-exists in one request instead of ~17 minutes +- `d0666871f` [PIR #12] fix: enable the gitea concepts, and stop degrading silently +- `bbc4ebe4a` [PIR #12] test+docs: pin the endpoints, the head.label behaviour, and is:open +- `64cb220d0` [PIR #12] fix(doctor): report tea for every gitea concept, not echo or case +- `e018fee98` [PIR #12] docs: builder thread for the implement phase + +## The measurements that decided the design + +Everything here follows from timings taken against a live Forgejo 15.x (`git.pseudoseed.com/pseudoseed/entriq`, 1599 PRs) with `tea` 0.14.2, not from reasoning about the API. + +| Request | Time | +|---|---| +| `GET /repos/{o}/{r}/issues/1` | 0.59 s | +| `GET .../pulls?state=all&limit=1` | 0.78 s | +| `GET .../pulls?state=all&limit=50` | **32.8 s** | +| `GET .../pulls?state=closed&limit=50` | **48.1 s** | +| `GET .../issues?type=pulls&state=all&limit=50` | 1.75 s | +| `GET .../pulls/{base}/{head}` | 0.17–1.2 s | +| `GET .../pulls/{n}.diff` | 0.30 s | + +The `/pulls` list is priced **per returned PR object**, not per request — roughly 0.65 s each, because Gitea materialises head and base commit info for every row. `pr-exists` walked `state=all` at 50 per page, so it cost 32 pages × ~33 s ≈ 17 minutes; `recently-merged` walked `state=closed` at 48 s a page ≈ 26 minutes. Raising the page size cannot help a per-item cost, and neither can a cache that has to be filled once. The fix had to stop enumerating. + +Two properties of Gitea made that possible, and one of them contradicts a caveat the old code documented as unfixable: + +- **`GET /pulls/{base}/{head}`** answers "is there a PR from this branch" in one request, 404s when there is none, and takes slashes in the head branch unescaped. +- **`head.label` survives branch deletion.** The old scan matched `.head.ref`, and its comment correctly noted that Gitea rewrites that to `refs/pull/N/head` once a merged PR's branch is deleted — concluding that a merged PR could not be found by branch name. But `.head.label` keeps the original name and the base/head endpoint matches on the stored head branch. Verified: PR 3869, merged with its branch gone, reports `head.ref = refs/pull/3869/head` and `head.label = builder/aspir-3860`, and `pulls/main/builder/aspir-3860` returns it. So the new implementation is not only ~900× faster, it answers a question the old one could not. + +`recently-merged` uses a different lever: the `issues?type=pulls` index is nineteen times cheaper than the `/pulls` view of the same rows because it skips that commit materialisation, and it accepts a server-side `since` filter. It carries everything `MergedPrItem` needs except the head branch, which is then fetched per match, eight at a time. + +## The `codev doctor` fix, found during the acceptance run + +Separate from the above and worth its own heading, because it silenced the one diagnostic that should have caught this class of problem. + +`extractExecutable` reads a script's first substantive line to decide what must be on `PATH`. For the gitea preset it was answering **`echo` for `issue-view` and `case` for `pr-list`** — so `codev doctor` on a Forgejo repo told the user to install `echo`, and a genuinely missing `tea` went unreported on exactly the repositories that need it. This is the defect class #1455 exists to close, and the remedy it established is the `# forge-executable:` declaration; two scripts were missing one. The test now asserts it across the entire preset rather than concept by concept, so a new gitea script cannot reintroduce it. + +## What upstream cluesmith/codev#1331 contributed, and what its review contributed + +#1331 (open, unmerged; no PR against it in this fork) fixes `pr-search` to span all PR states, because `gh pr list --search` defaults to open-only and post-merge `consult --type pr` therefore fails with "No PR found for branch". The gitea implementation defaults to all states for that reason. + +Its **review** turned out to matter more than its diff. Making `pr-search` all-states breaks `spawn-worktree.ts`, which queries `in:body #N` and leaned on the old open-only default to mean "open PRs". Under an all-states search that becomes "did this issue *ever* have a PR", and every re-spawn, every follow-up to a partial fix, and every retry after a closed PR aborts with a factually wrong "Found N open PR(s)". The call site now says `is:open` explicitly, and the gitea script honours `is:` qualifiers so that saying it works. The review's second point — that `PrSearchItem` gave callers no `state` with which to defend themselves — is addressed by adding `state` to the contract and ordering results open-first. + +`github/pr-search.sh` and `gitlab/pr-search.sh` are deliberately **not** touched here. They carry the same #759 bug, but fixing them is upstream #1331's diff and would conflict on every future sync. The architect is filing that as an adopt-the-PR job, the way #1146 and #1458 were handled. + +## The truncation contract + +`recently-merged` bounds itself three ways — a default 7-day window when `CODEV_SINCE_DATE` is absent (announced on stderr; never "all time", which is the 26-minute walk), a 300-PR ceiling, and the paged walk's wall-clock deadline. When a bound bites it exits **3 with empty stdout**. + +That emptiness is the point. A short list and a truncated list are indistinguishable once printed, so `[]` with status 0 means "nothing merged" and status 3 means "I stopped looking", and stderr names which bound bit. This is now the documented exit-status contract for forge concepts generally. + +## Test Results + +- `npm run build`: ✓ pass +- `npm test`: 5498 passed, 2 failed — both pre-existing and unrelated (see **Flaky Tests**). 45 tests are new across two files (`pir-12-gitea-pr-concepts.test.ts`, `pir-12-pr-exists-null-vs-false.test.ts`), plus one behavioural spawn assertion and the reworked `bugfix-1137` cases. +- **Live verification** against `git.pseudoseed.com/pseudoseed/entriq` with all three overrides deleted, driven through the real dispatcher (config load → preset → env → script → JSON parse), not by invoking scripts by hand: + + | Call | Time | Result | + |---|---|---| + | `pr-exists` open branch | 986 ms | `true` | + | `pr-exists` merged branch, deleted | 686 ms | `true` | + | `pr-exists` absent branch | 482 ms | `false` | + | `pr-search head:` | 1079 ms | PR 3855 with head + base refs | + | `pr-search ` | 1240 ms | PR 3869, state `merged` | + | `pr-search in:body #3860 is:open` | 620 ms | `[]` — spawn not blocked | + | `pr-diff` name-only / full | 578 / 354 ms | 6 paths / 30 KB diff | + | `issue-view` | 607 ms | correct JSON | + | `pr-list` | 509 ms | 1 open PR | + | `recently-merged` 24 h | 1082 ms | 6 PRs, all head branches resolved | + | `user-identity` | 459 ms | `pseudoseed` | + | `team-activity`, `on-it-timestamps` | — | still resolve as `disabled` | + + entriq's config was then **restored** and diff-verified byte-identical to both the pre-test copy and the architect's 11:20 backup. It runs the globally installed codev 3.3.1, whose preset still disables `pr-search`/`pr-diff`, so its overrides remain load-bearing until this ships. Delete them after this merges and entriq updates. + +## ⚠️ This had TWO of THREE review lanes — Codex never ran + +**Read this before trusting the review depth.** + +| Lane | Verdict | Notes | +|---|---|---| +| **codex** (gpt-5.6-sol) | **NEVER RAN** | Provider usage quota exhausted. Two attempts on 2026-08-21, 19:06:05Z and 19:06:35Z, each refused in ~6 s before any model work began. Quota restores 2026-08-27. **No codex findings exist for this change.** | +| gemini (agy) | APPROVE, HIGH | No issues raised. | +| claude (opus-5) | APPROVE, HIGH | Four non-blocking findings, all real, all fixed — see below. | + +Verbatim provider message, both attempts: + +> You've hit your usage limit. Upgrade to Pro (https://chatgpt.com/explore/pro), visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 27th, 2026 4:01 PM. + +The codex lane was skipped on explicit architect instruction rather than hold a fork-local change for six days. The absence is recorded as a NOT-RUN file at `codev/projects/12-forgejo-gitea-forge-parity-imp/12-review-iter1-codex.txt` carrying `VERDICT: SKIPPED` and `CONFIDENCE: NONE`, so it cannot be misread as a review that happened — but that path is gitignored (`.gitignore:65`), which is why the coverage is stated here too. The same quota blocked codex on #2, #4 and #11 earlier the same day. + +**Porch's own gate summary says otherwise, and it is wrong.** `porch next 12` reports `codex: COMMENT` and prints **"All reviewers approved!"**. `parseVerdict` (`porch/verdict.ts:41-47`) recognises only `APPROVE`, `REQUEST_CHANGES` and `COMMENT`; `VERDICT: SKIPPED` matches none of them, falls through to the "no valid VERDICT line found — treat as COMMENT" default, and `allApproved` counts `COMMENT` as approval. So a lane that never ran is indistinguishable from one that approved, in the exact summary a human reads when deciding to merge. + +There is a **second, independent route** to the same false statement, found by the architect when verifying this in the source: `allApprove` also returns `true` when `reviews.length === 0`, so *zero* reviewers approves as well. And the fall-through's own comment states the bug aloud — "no valid VERDICT line found **but the consult ran**" — because the vocabulary was built for a reviewer that ran and emitted something unparseable. A reviewer that never ran cannot be expressed in it at all, which is why widening a default would not have been enough. + +That is this PR's own lesson pointed back at the tooling — a sixth arrival at it in one day, and the one with the largest blast radius, since it affects every project that records a skip. #2, #4 and #11 all merged with porch reporting unanimous approval while codex had read nothing; those merges were sound because the human approving each gate knew the coverage was two of three and each PR body said so, but that was the human channel working *despite* the tool. It is porch behaviour rather than anything in this diff, so it is **not fixed here** — tracked as **#20**. Read the table above, not porch's summary line. + +**Weigh this specifically.** The absent lane is the one that most often catches shell-quoting and POSIX-portability defects, and this diff is five POSIX `sh` scripts, a hand-rolled process watchdog, and a pile of jq. That is close to the worst pairing of *which* lane is missing against *what* the change is made of. Two of the three bugs found during implementation were exactly that class, and both were caught by tests rather than by reading. + +### What the Claude lane found, and what changed because of it + +All four findings were reproduced before being fixed; none were argued down. + +1. **Two `gitea_api_error` checks were unreachable** (`pr-diff.sh` name-only, `recently-merged.sh`). Gitea answers a 404 with a JSON *object*, which reached `tea_api_paged`'s `jq -s 'add'` first — and an object cannot be added to an array, so the walk died on `jq: error … array ([]) and object ({...}) cannot be added` before the script's own classification ran. Reproduced against live Forgejo exactly as reported. The exit status was non-zero either way, so no wrong answer was ever returned; what was lost was the sentence naming the missing PR. `tea_api_paged` now classifies before accumulating and returns a distinct status 4 with the body on stdout, so the caller's message finally fires. Both paths now tested — the full-diff 404 was covered and the name-only one was not, which is why only one of them was broken. +2. **The plan's Test Plan called for a byte-identity test over the two `SKILL.md` twins and it was never written.** The files were identical; nothing stopped them drifting. Now pinned. +3. **`checks.ts`'s null-vs-false change had no test.** It is a real behaviour change — `null` from the concept means "could not answer", not "no PR exists" — and it mattered concretely, since before this PR the gitea script blew the 30 s ceiling on every run, making `null` the *normal* outcome on that provider. Three tests added in a separate file, because mocking the forge layer is something `checks.test.ts` deliberately avoids. +4. **`gitea_timeout` nits, both fixed.** The marker file's path was derived from the output file's (`"$_tf.fired"`), which `mktemp` does not reserve — a predictable name in a world-writable tmpdir that anyone could pre-create to force every call to report a timeout. Both files now live in a private `mktemp -d`. And the watchdog claimed the timeout unconditionally, so a command finishing in the same instant the deadline passed was reported as timed out, discarding a good answer; it now checks `kill -0` first. That narrows the window rather than closing it — a lock would be needed for that — and a false timeout is a retryable error rather than a wrong answer. + +## Architecture Updates + +**COLD — `codev/resources/arch.md`**, § Integration Points → Forge Concept Commands. Four additions, all current-state reference rather than changelog: + +1. Provider presets are on-disk scripts under `packages/codev/scripts/forge//`, single-source with no skeleton mirror, and `null` in a preset is a deliberate refusal rather than "no script". +2. **A preset-disabled concept is invisible to `forgeConfig` lookups** — `forgeConfig?.['x']` reads user config only. Availability must be asked of `getForgeCommand`/`isConceptDisabled`. This is the system-shape surprise that made `on-it-timestamps` return an empty map silently on every gitea repo. +3. The exit-status contract, including 3-with-empty-stdout, and that `null` from `executeForgeCommand` means "could not answer". +4. The two verified Forgejo properties: per-object pricing on `/pulls`, and `head.label` surviving branch deletion. Both filed here rather than in lessons-learned because they are properties of a system, not general engineering advice — the "looks like X but is actually Y" routing. + +Nothing was promoted to `arch-critical.md`: all of it is reference detail that matters only when writing a forge concept. + +## Lessons Learned Updates + +**HOT — `codev/resources/lessons-critical.md`**, one promotion with displacement, since the file was at its cap of ten: + +> A truncated result is indistinguishable from a complete one once emitted — give "I stopped early" its own signal and emit nothing, never a partial answer that reads as whole. + +It earns the hot slot because the same principle has now been arrived at independently three times in this codebase — `afx send`'s render gate, #13's log extraction, and this PR's exit-3 contract — which is the signal that it should be changing decisions before they are made rather than being rediscovered. + +**Displaced into `lessons-learned.md` § Architecture:** "Model permissions as roles/capabilities, not booleans." Still true and still worth reading; it is the narrowest of the ten, applying only when designing a permission system, where the promoted rule applies to any list, log, render, or fetch that can stop early. Reviewers should push back if they disagree with that trade — it is the judgement call in this diff that is least supported by evidence. + +**COLD — `codev/resources/lessons-learned.md`** § Debugging and Root Cause Analysis, three entries: + +1. **Killing a process is not unblocking the caller, and a zero exit is not success.** Both halves were bugs in this PR's own timeout helper (below). +2. **Measure whether a remote endpoint is priced per request or per returned item before optimising around it.** One `limit=1` vs `limit=50` timing tells you which regime you are in and takes a minute; against a per-item cost, every page-size instinct is useless. +3. **A hang reported from a shell is not always the hang your code has in production.** In-process, `pr-exists` was being killed at 30 s and returning `null`, which the caller rendered as "no PR exists" — same root cause, different bug, and only one of them was the one reported. + +## Things to Look At During PR Review + +- **`gitea_timeout` in `_lib.sh` is the subtlest thing here, and it was wrong twice.** First it killed the command and left the caller blocked anyway: every caller runs it inside `$(...)`, and a grandchild kept the write end of that pipe open, so the timeout message printed at 3 s and the script was still hung two minutes later. The command now writes to a temp file rather than the caller's pipe, and the watchdog's own stdout goes to `/dev/null` for the same reason. Then it classified timeouts by exit status (143/137), which a wrapper using operand-less `wait` defeats — POSIX makes that always return 0, so a process killed by SIGTERM reported success with an empty body and the caller diagnosed an unreadable repository instead. The watchdog now records that it fired and nothing is inferred. Both failure modes have tests. +- **The base-branch limitation in `pr-exists` / `pr-search head:`.** The endpoint needs a base; it uses the repository default unless `CODEV_PR_BASE` is set, so a PR against an integration branch needs that variable. A miss prints `false`, which fails the porch `pr_exists` gate loudly. Falling back to a list scan on the 404 path was rejected deliberately — it would reintroduce the ~17-minute walk on the failure path, where it would be least expected. +- **`pr-search`'s query grammar is a parser, not a pass-through**, covering exactly the five query strings this codebase builds. A sixth would return `[]` with a stderr note. The five and their call sites are tabulated at the top of the script. +- **`bugfix-1137`'s fixtures moved.** Its guarantee is unchanged and still asserted — reads go through `tea api`, never `tea pulls list`, and a paged read is not silently truncated — but `pr-exists` now pages nothing at all, which is the strongest form of that. The fake `tea` no longer serves any `pulls?state=…` fixture, so a reintroduced scan fails there with "no fixture for". +- **Script assertions are anchored to command lines, not `toContain`.** #1331's review caught assertions that passed against the explanatory comment quoting the flag under test, staying green with the flag deleted from the command. The comments here quote the very endpoints being pinned, so the same trap was live. +- **The hot-tier displacement** described above. + +## How to Test Locally + +- **View diff**: VSCode sidebar → right-click builder `pir-12` → **Review Diff** +- **Run dev**: `afx dev pir-12` +- **What to verify**, against any Forgejo repo with `tea` authenticated: + - `CODEV_BRANCH_NAME= sh packages/codev/scripts/forge/gitea/pr-exists.sh` returns `true` in about a second, and issues exactly two requests + - the same for a branch whose PR is **merged and whose branch was deleted** — this is the case the old code could not answer + - a branch with no PR returns `false`; a bad `CODEV_REPO` **errors** rather than returning `false` + - `CODEV_SEARCH_QUERY='in:body # is:open'` returns `[]` for an issue whose PR is merged, i.e. `afx spawn` is not blocked + - `CODEV_SINCE_DATE` unset on `recently-merged.sh` announces a 7-day window rather than walking everything; `CODEV_FORGE_MERGED_MAX=2` over a busy window exits 3 with empty stdout + - `codev doctor` reports `tea` for every enabled gitea concept and `disabled` for `team-activity` / `on-it-timestamps` + +## Flaky Tests + +None skipped or annotated. Two pre-existing failures were left untouched (which of them fires varies per run — they are contention-sensitive, and a third from the same file appeared in an earlier run): + +- `packages/codev/src/__tests__/spec-1280-measurement-instrument.test.ts` — `emits byte-identical output twice at the same commit`, `reports the same total under a C locale as under UTF-8`, and `PHASE_ITERS is a linear comparison constant`. Each invokes `scripts/measure-prompt-surface.sh` twice against a 60 s budget, and that script takes ~31 s per invocation on this machine. Proven pre-existing by running the same test against the **unmodified main checkout**, where it fails identically at 77 s; the architect independently reproduced it there past 300 s. The script costs the same against either root (31.3 s vs 33.0 s), so this is machine speed, not diff content. + +`packages/codev/src/terminal/__tests__/session-manager.test.ts` also drops one timing-sensitive stderr-tail case under full-suite load; the file passes 91/91 in isolation, and a *different* case from it failed on an earlier run, which is the signature of contention rather than a defect. + +This is further evidence for #8 — porch's check timeout is a hardcoded 300 s with no override key, and entriq's config records the same class of problem at 460 s quiet / 859 s contended. diff --git a/codev/state/pir-12_thread.md b/codev/state/pir-12_thread.md new file mode 100644 index 000000000..3ab464125 --- /dev/null +++ b/codev/state/pir-12_thread.md @@ -0,0 +1,148 @@ +# Builder thread — pir-12 (Issue #12, PIR) + +Forgejo/Gitea forge parity: pr-search, pr-diff, and the pr-exists hang. + +## 2026-08-21 — Plan phase + +Investigated live against `~/dev/entriq` (Forgejo 15.x, tea 0.14.2), as the architect +directed. The repo was already paused, so exercising it was safe; I only issued reads. + +**The pr-exists "hang" is a ~17-minute loop, not a deadlock.** Forgejo's `/pulls` list +costs ≈0.65 s *per PR object* (limit=1 → 0.78 s, limit=50 → 32.8 s), entriq has 1599 +PRs (`x-total-count`), and `tea_api_paged` walks all 32 pages. Killing at 25 s and at +120 s both land inside that window with nothing on stdout. Auth and connectivity were +never involved — a single issue fetch is 0.59 s. + +Two things I got wrong on the way and corrected with a repro rather than argument: + +- I assumed Node's `exec` timeout would not fire while a grandchild held the stdout + pipe, which would have meant the in-process path hangs too. It does fire — + `sh -c "sleep 60 | cat"` rejected at 3009 ms under a 3 s timeout. So under porch the + check fails at 30 s with `output: "null"`, which reads as "no PR" instead of "could + not answer". Different bug, still a bug, now in the plan. +- I assumed the documented caveat at `pr-exists.sh:16-20` (merged PRs report + `head.ref == refs/pull/N/head` once the branch is deleted) made merged-PR lookup + impossible. `head.label` keeps the original branch name, and + `GET /pulls/{base}/{head}` matches on it — verified live, PR 3869 found by + `pulls/main/builder/aspir-3860` in 1.19 s. That one endpoint replaces the whole scan + for both `pr-exists` and `pr-search head:`. + +**Upstream cluesmith/codev#1331** is open and unmerged with no PR in this fork. Its +review matters more than its diff: making pr-search all-states breaks +`spawn-worktree.ts:592`, which passes `in:body #N` and leans on the open-only default, +so every re-spawn for an issue with a merged PR would abort on a wrong "Found N open +PR(s)". The gitea script therefore parses `is:` qualifiers, and the spawn call site +gets an explicit `is:open`. + +**Porch named the wrong plan file.** The phase prompt asked for +`codev/plans/0012-hide-tmux-status-bar.md` — an unrelated 2025 plan. That is the +zero-padding collision `artifacts.ts:80-104` already documents for this fork (issue +numbering restarted at 1 against artifacts numbered into the 1400s). I wrote +`codev/plans/12-forgejo-gitea-forge-parity.md`, the exact-match form `findByProjectId` +prefers, and confirmed `plan_exists` resolves to it. Flagged to the architect. + +Two scope questions raised in the plan rather than decided alone: whether to carry +upstream's `github`/`gitlab` pr-search fix in the fork (I said no — it will conflict +when #1331 lands), and whether `recently-merged`/`pr-list`, which pay the same +17-minute cost on a big repo, belong in this issue (I said no — not overridden in +entriq, so not blocking acceptance; bounded with a deadline instead). + +Plan committed, sitting at `plan-approval`. + +## 2026-08-21 — Implement phase + +Plan approved with both scope questions answered, then the architect folded the +`recently-merged` fix in after I reframed it: it is not a slow operation we would +be choosing to break, it is one that has been failing silently on entriq on every +`afx status` and every dashboard poll — 30s node timeout, null, empty panel, +nothing on stderr. + +**What the measurements decided.** Forgejo charges `/pulls` per returned PR +object (0.78s at limit=1, 32.8s at limit=50). entriq has 1599 PRs. So +`pr-exists` cost ~17 minutes and `recently-merged` (state=closed, 48.1s/page × +32) ~26. No page size fixes that; the answer was to stop enumerating. Both now +run in about a second. + +**The discovery that made it clean.** `head.label` keeps the original branch name +after a merged PR's branch is deleted, where `head.ref` becomes +`refs/pull/N/head`. The existing script documented that caveat as unfixable. It +isn't — `GET /pulls/{base}/{head}` matches on the stored head branch, so the new +implementation is not just 900× faster, it answers a question the old one +couldn't. + +**Three bugs my own code had, each caught by a test rather than by reasoning:** + +1. `gitea_timeout` killed the command and left the caller blocked anyway — a + grandchild kept the command-substitution pipe open. The timeout message + printed at 3s and the script was still hung two minutes later. Fixed by + giving the command a temp file instead of the pipe. +2. Then it classified timeouts by exit status (143/137 = "we killed it"), which + a wrapper whose own `wait` takes no operand defeats — POSIX says operand-less + `wait` always returns 0, so a process killed by SIGTERM reported success with + an empty body, and the caller diagnosed an unreadable repository. The + watchdog now records that it fired; nothing is inferred. +3. `--argjson` with 107 PR objects blew ARG_MAX. Only showed up at the 7-day + window, not the 24-hour one. + +**Truncation is exit status 3 with empty stdout**, per the architect's condition. +A short list and a truncated list are indistinguishable once printed, so +"nothing merged" (`[]`, status 0) and "I stopped looking" (status 3, stderr says +which bound bit) are deliberately different things. No `CODEV_SINCE_DATE` means a +7-day window, announced — never "all time", which is the 26-minute walk. + +**Acceptance run.** Deleted all three overrides from `~/dev/entriq/.codev/config.json` +and drove every concept through the real dispatcher against the live Forgejo. +All pass on the bare `provider: gitea` preset, every call under 1.3s. Then +**restored the config**, because entriq runs the globally installed codev 3.3.1, +whose preset still disables pr-search/pr-diff and still carries the old scan — so +the overrides are still load-bearing there until this ships. Verified the file is +byte-identical to both my pre-test copy and the architect's 11:20 backup. The +`git status` dirt in entriq predates me (it is in the architect's backup too). + +The acceptance run also exposed that `codev doctor` reported `echo` as +`issue-view`'s required executable and `case` as `pr-list`'s, so a missing `tea` +went unreported on Forgejo repos. Two `# forge-executable:` declarations, and the +test now asserts it across the whole preset. + +**Flaky, pre-existing, not mine:** three tests in +`spec-1280-measurement-instrument.test.ts` time out at 60s because +`scripts/measure-prompt-surface.sh` takes ~31s per invocation on this machine and +they call it twice. Proven pre-existing by running the same test against the +unmodified main checkout, where it fails identically (77s). Left untouched. + +## 2026-08-21 — Review phase + +PR #19 open, body is the review file, verified byte-identical after creation. + +**Governance routing.** Promoted to `lessons-critical.md`: *a truncated result is +indistinguishable from a complete one once emitted*. The hot file was at its cap of +ten, so this required displacement — "model permissions as roles/capabilities, not +booleans" moved into `lessons-learned.md` § Architecture with an expanded body and a +note saying where it came from and why. + +I argued three independent arrivals at the principle. The architect corrected me +upward: five, all on 2026-08-21, all different subsystems — the render gate returning +CLEAN on a screen it had not proven empty (#4), log extraction refusing to return +arbitrary lines as a diagnosis (#13), this PR's exit-3 contract, a timed-out forge +command becoming `null` and reading as "no results" (#17), and a check that exceeded +its bound reporting as a test failure rather than a timeout (#8). One mistake, five +subsystems, one day. + +`arch.md` gained four lines under Forge Concept Commands. The one the architect +singled out: **a preset-disabled concept is invisible to `forgeConfig` lookups**, +because `forgeConfig?.['x']` reads user config only and a preset `null` is not in it. +That is what silently emptied `on-it-timestamps` on every gitea repo, and it is +invisible until seen. + +**Codex lane never ran.** Provider quota exhausted until 2026-08-27; two attempts +(19:06:05Z and 19:06:35Z) both refused in ~6s before any model work. Recorded as a +NOT-RUN file with `VERDICT: SKIPPED` / `CONFIDENCE: NONE`, the verbatim provider +message, and both attempt times — the same convention #2, #4 and #11 used. That file +is gitignored (`.gitignore:65`), which is exactly why the two-of-three coverage is +stated in the PR body as well. + +Worth naming: the absent lane is the one that most often catches shell-quoting and +POSIX-portability defects, and this diff is five POSIX `sh` scripts, a hand-rolled +process watchdog and a pile of jq. That is the weakest possible pairing of "which lane +is missing" against "what this change is made of", and it is stated in the PR body +rather than buried. diff --git a/packages/codev/scripts/forge/gitea/_lib.sh b/packages/codev/scripts/forge/gitea/_lib.sh index cf289b870..726c23e17 100755 --- a/packages/codev/scripts/forge/gitea/_lib.sh +++ b/packages/codev/scripts/forge/gitea/_lib.sh @@ -53,24 +53,298 @@ GITEA_MAX_PAGES=100 # 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. +# +# EXIT STATUS: 0 = the whole list was fetched, 3 = it STOPPED EARLY and what is +# on stdout is a PREFIX, 4 = the response WAS NOT A LIST (an error body — stdout +# carries it verbatim for the caller to classify with gitea_api_error), 1 = the +# request failed. Callers must distinguish 3 from 0: a short list and a truncated +# list look identical once printed, and reading a truncated one as complete is +# how "recently merged" quietly renders an empty panel. Truncation also always +# says so on stderr. +# +# Status 4 exists because callers cannot classify an error body they never see. +# Gitea answers a 404 with a JSON OBJECT, and this function used to feed that +# straight into `jq -s 'add'`, which cannot add an object to an array — so the +# walk died on a raw `jq: error (at :1): array ([]) and object ({...}) +# cannot be added` and the caller's own gitea_api_error check, sitting after the +# call, was unreachable. The exit status was still non-zero, so no wrong answer +# was ever returned; what was lost was the message saying which PR was missing. +# Found by the claude review lane on PIR #12 and reproduced before fixing. tea_api_paged() { _path="$1" _query="$2" _page=1 _acc='[]' + _items=0 + _deadline=$(( $(date +%s) + GITEA_PAGED_DEADLINE )) while [ "$_page" -le "$GITEA_MAX_PAGES" ]; do + if [ "$(date +%s)" -ge "$_deadline" ]; then + echo "gitea forge: '${_path}' still had pages after ${GITEA_PAGED_DEADLINE}s (${_items} items over $((_page - 1)) pages); stopping. Raise CODEV_FORGE_PAGED_DEADLINE, or narrow the query." >&2 + printf '%s' "$_acc" + return 3 + fi 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="$(gitea_api "$_url")" || return 1 # Blank body or an empty array → no more pages. [ -n "$_resp" ] || break + # Classify BEFORE accumulating. Anything that is not a JSON array cannot be + # a page of results, and must reach the caller intact rather than as a jq + # diagnostic about adding an object to an array. + if ! printf '%s' "$_resp" | jq -e 'type == "array"' >/dev/null 2>&1; then + printf '%s' "$_resp" + return 4 + fi _count="$(printf '%s' "$_resp" | jq 'length')" || return 1 _acc="$(printf '%s\n%s' "$_acc" "$_resp" | jq -s 'add')" || return 1 + _items=$((_items + _count)) [ "$_count" -lt "$GITEA_PAGE_LIMIT" ] && break _page=$((_page + 1)) done + if [ "$_page" -gt "$GITEA_MAX_PAGES" ]; then + echo "gitea forge: '${_path}' hit the ${GITEA_MAX_PAGES}-page ceiling (${_items} items); stopping." >&2 + printf '%s' "$_acc" + return 3 + fi printf '%s' "$_acc" } + +# --------------------------------------------------------------------------- +# Timeouts +# --------------------------------------------------------------------------- + +# Wall-clock ceiling for a single `tea api` call, in seconds. Override with +# CODEV_FORGE_TIMEOUT. 60s is generous for any single-object Gitea endpoint +# (a repo probe is ~0.2s, a PR object ~0.3s, a base/head lookup ~1.2s against a +# real Forgejo) while still being far below the 300s porch check timeout, so a +# stuck call surfaces as a named error inside a phase rather than as a stalled +# phase. +GITEA_TIMEOUT=${CODEV_FORGE_TIMEOUT:-60} +case "$GITEA_TIMEOUT" in + ''|*[!0-9]*) GITEA_TIMEOUT=60 ;; +esac + +# Wall-clock ceiling for a whole paged walk (see tea_api_paged), in seconds. +# Override with CODEV_FORGE_PAGED_DEADLINE. Deliberately larger than +# GITEA_TIMEOUT: a legitimate multi-page walk is several sequential requests. +GITEA_PAGED_DEADLINE=${CODEV_FORGE_PAGED_DEADLINE:-120} +case "$GITEA_PAGED_DEADLINE" in + ''|*[!0-9]*) GITEA_PAGED_DEADLINE=120 ;; +esac + +# Run a command under a wall-clock limit. Returns the command's own exit status, +# or 124 (the exit status `timeout(1)` uses) if the limit was reached. +# +# Deliberately does NOT use timeout(1)/gtimeout even when present. macOS ships +# neither by default, so the fallback would be the path that actually runs for +# most adopters while the tested-in-CI path would be the one that doesn't — +# exactly the arrangement where the untested path rots. One implementation, +# same behaviour everywhere. +# +# TWO THINGS HERE ARE LOAD-BEARING, and both exist because killing the command +# is not the same as unblocking the caller: +# +# 1. The command's stdout goes to a TEMP FILE, not to the caller's pipe. Every +# caller runs this inside `$(...)`. A killed command can leave a grandchild +# holding the write end of that pipe, and the command substitution then +# blocks forever on a process nobody is waiting for — the timeout fires, the +# message prints, and the script still hangs. Measured, not theorised: with +# the command writing straight to the pipe, a 3s timeout against a wrapper +# that spawns `sleep 300` printed its timeout message at 3s and was still +# blocked two minutes later. +# 2. The watchdog subshell's own stdout goes to /dev/null, for the same reason. +# +# Grandchildren are swept with `pkill -P` where it exists. That is best-effort +# and not the guarantee — (1) is the guarantee, and it holds even where `pkill` +# does not exist. +gitea_timeout() { + _limit="$1" + shift + # Both files live in a private mktemp DIRECTORY. The marker's path used to be + # derived from the output file's ("$_tf.fired"), which mktemp does not reserve + # — a predictable name in a world-writable tmpdir that anyone could pre-create + # to make every call report a timeout. + _dir=$(mktemp -d) || return 1 + _tf="${_dir}/out" + _fired="${_dir}/fired" + "$@" >"$_tf" & + _cmd_pid=$! + ( sleep "$_limit" + # Claim the timeout only if there is still something to kill. Writing the + # marker unconditionally misreports a command that finished in the same + # instant the deadline passed — it succeeded, and saying otherwise discards + # a good answer. `kill -0` narrows that window to the gap between this test + # and the signal; it cannot be closed entirely without a lock, and a + # false timeout is a retryable error rather than a wrong answer. + if kill -0 "$_cmd_pid" 2>/dev/null; then + : > "$_fired" + pkill -TERM -P "$_cmd_pid" 2>/dev/null + kill -TERM "$_cmd_pid" 2>/dev/null + sleep 2 + pkill -KILL -P "$_cmd_pid" 2>/dev/null + kill -KILL "$_cmd_pid" 2>/dev/null + fi + ) >/dev/null 2>&1 & + _wd_pid=$! + # `|| _rc=$?` rather than `wait; _rc=$?`: the concept scripts run under + # `set -e`, which would abort here the moment the wrapped command exited + # non-zero — before the timeout could be classified and named. + _rc=0 + wait "$_cmd_pid" || _rc=$? + kill "$_wd_pid" 2>/dev/null + + # Whether the watchdog fired is recorded by the watchdog, not INFERRED from + # the exit status. Inferring it (status 143/137 = "we killed it") looked + # equivalent and is not: a killed process can still exit 0. A wrapper whose + # own `wait` takes no operand does exactly that — POSIX defines operand-less + # `wait` as always returning zero — so its death by SIGTERM was reported as a + # successful call returning an empty body, and the caller then diagnosed an + # unreadable repository instead of a timeout. Found by the test that pins this + # function; the marker file cannot be wrong the same way. + if [ -f "$_fired" ]; then + rm -rf "$_dir" + return 124 + fi + # A half-written response is worse than no response, so output is emitted only + # on the non-timeout path. + cat "$_tf" + rm -rf "$_dir" + return "$_rc" +} + +# `tea api` under GITEA_TIMEOUT, with a named error on the timeout path. +# +# Every gitea concept script goes through this rather than calling `tea api` +# directly, so that a Gitea endpoint which stops returning surfaces as an error +# naming the endpoint instead of a phase that never finishes (issue #12: a +# `state=all` walk of a 1599-PR repo cost ~17 minutes and read as a hang). +# +# NOTE: `tea api` exits 0 on HTTP errors and prints the error body, so a zero +# status from this function means "the request completed", NOT "it succeeded". +# Callers must inspect the response — see gitea_api_error. +gitea_api() { + _rc=0 + gitea_timeout "$GITEA_TIMEOUT" tea api "$@" || _rc=$? + if [ "$_rc" -eq 124 ]; then + echo "gitea forge: 'tea api $*' did not return within ${GITEA_TIMEOUT}s; set CODEV_FORGE_TIMEOUT to raise the limit" >&2 + return 124 + fi + return "$_rc" +} + +# Classify a `tea api` response body. Echoes one of: +# ok — a JSON object or array (the request succeeded) +# notfound — Gitea's 404, in either of the two shapes it uses +# error — anything else (auth failure, 5xx, unparseable body) +# +# Gitea answers 404 two ways: a JSON body `{"message":"The target couldn't be +# found.", ...}` for a known route with an unknown target, and the bare text +# `404 page not found` for an unknown route. Both are verified against Forgejo +# 15.x + tea 0.14.2. +# +# `notfound` is NOT on its own an answer of "no". A mistyped or unreachable +# owner/repo returns a byte-identical 404 to a repo that simply has no such PR, +# so a caller may only read `notfound` as "no" once it has independently +# established that the repo resolves — see gitea_default_branch, which does +# exactly that as a side effect. +gitea_api_error() { + _body="$1" + case "$_body" in + 404*) printf 'notfound'; return 0 ;; + esac + _type=$(printf '%s' "$_body" | jq -r 'type' 2>/dev/null) || _type= + case "$_type" in + object) + _message=$(printf '%s' "$_body" | jq -r '.message // empty' 2>/dev/null) || _message= + case "$_message" in + '') printf 'ok' ;; + *"couldn't be found"*|*"could not be found"*|*'Not found'*|*'not found'*|*'does not exist'*) + printf 'notfound' ;; + *) printf 'error' ;; + esac + ;; + array) printf 'ok' ;; + *) printf 'error' ;; + esac +} + +# Resolve the repository's default branch, and — as the load-bearing side +# effect — prove that the repo resolves at all under the current credentials. +# +# Both matter. Gitea returns the SAME 404 body for "this repo does not exist / +# you cannot see it" as for "this branch has no PR", so a base/head lookup's 404 +# is only readable as "no PR" after this call has succeeded. Callers that skip +# it because they were handed an explicit base would answer `false` for a +# typo'd CODEV_REPO. +gitea_default_branch() { + _repo="$1" + _resp=$(gitea_api "repos/${_repo}") || return 1 + _status=$(gitea_api_error "$_resp") + if [ "$_status" != "ok" ]; then + echo "gitea forge: could not read repository '${_repo}' — Gitea said: ${_resp}" >&2 + echo "gitea forge: set CODEV_REPO=owner/repo, or run from a checkout whose remote is a configured Gitea host" >&2 + return 1 + fi + _branch=$(printf '%s' "$_resp" | jq -r '.default_branch // empty' 2>/dev/null) || _branch= + if [ -z "$_branch" ]; then + echo "gitea forge: repository '${_repo}' reported no default branch; set CODEV_PR_BASE" >&2 + return 1 + fi + printf '%s' "$_branch" +} + +# How many PR objects to fetch at once in gitea_fetch_pulls. Override with +# CODEV_FORGE_CONCURRENCY. +# +# Forgejo answers a single PR object in ~1s, and the alternative — asking the +# list endpoint for the same PRs — costs the same ~1s each AND cannot be bounded +# by which PRs you actually want. Sequential resolution of a week of merges (107 +# PRs on the reference repo) is ~107s, past the 30s ceiling +# `executeForgeCommand` imposes on every concept; at 8 at a time it is ~14s and +# fits. 8 is deliberately polite — this is somebody's self-hosted forge. +GITEA_CONCURRENCY=${CODEV_FORGE_CONCURRENCY:-8} +case "$GITEA_CONCURRENCY" in + ''|*[!0-9]*|0) GITEA_CONCURRENCY=8 ;; +esac + +# Fetch several PR objects by number and emit them as ONE JSON array. +# +# Usage: gitea_fetch_pulls ... +# +# Fetches up to GITEA_CONCURRENCY at a time into a temp dir, then concatenates. +# A number whose fetch fails or returns a non-PR body is DROPPED from the result +# rather than aborting the batch: these are all "resolve some detail about PRs I +# already know exist" calls, and one unreadable PR should not take out the whole +# answer. Ordering of the output array is not meaningful — callers sort. +gitea_fetch_pulls() { + _repo="$1" + shift + [ $# -gt 0 ] || { printf '[]'; return 0; } + + _dir=$(mktemp -d) || return 1 + _n=0 + for _num in "$@"; do + gitea_api "repos/${_repo}/pulls/${_num}" >"${_dir}/${_num}.json" 2>/dev/null & + _n=$((_n + 1)) + if [ $((_n % GITEA_CONCURRENCY)) -eq 0 ]; then + wait + fi + done + wait + + # `jq -s` over the files, keeping only bodies that really are PR objects. + # A plain glob, not `find | xargs`: xargs is free to split a long list across + # SEVERAL jq invocations, which would emit several arrays and silently drop + # all but the first once the caller parsed it. Callers cap their batches (see + # GITEA_MERGED_MAX_PRS), so the glob cannot approach ARG_MAX. + _out=$(jq -s '[ .[] | select(type == "object" and (.number | type) == "number") ]' "$_dir"/*.json 2>/dev/null) || _out= + rm -rf "$_dir" + if [ -z "$_out" ]; then + printf '[]' + return 0 + fi + printf '%s' "$_out" +} diff --git a/packages/codev/scripts/forge/gitea/issue-view.sh b/packages/codev/scripts/forge/gitea/issue-view.sh index b9350d0bf..89174489c 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) # diff --git a/packages/codev/scripts/forge/gitea/pr-diff.sh b/packages/codev/scripts/forge/gitea/pr-diff.sh new file mode 100755 index 000000000..060826be4 --- /dev/null +++ b/packages/codev/scripts/forge/gitea/pr-diff.sh @@ -0,0 +1,80 @@ +#!/bin/sh +# Forge concept: pr-diff (Gitea/Forgejo via tea CLI) +# forge-executable: tea +# Input: CODEV_PR_NUMBER (required) +# CODEV_DIFF_NAME_ONLY (optional, "1" for a bare list of changed paths) +# Output: raw diff text, or one path per line in name-only mode +# +# Two endpoints, because Forgejo splits what `gh pr diff` merges: +# full diff -> `pulls/{n}.diff`, which returns unified diff text (~0.3s) +# name-only -> `pulls/{n}/files`, which returns a JSON array of file objects +# (~0.5s). Parsing filenames back out of the diff text was +# rejected: `.filename` is authoritative for renames and for +# paths containing spaces, where "diff --git a/… b/…" is not. +# +# Name-only output is a bare newline-separated path list, matching +# `gh pr diff --name-only`, because consult's fetchPRData splits it on newlines +# (consult/index.ts). `--name-only` output has no header line to skip. +# +# `tea api` EXITS 0 ON HTTP ERRORS and prints the error body (established by +# pr-create.sh). Emitting that body would hand the caller an error page dressed +# as a diff — a model would then review Gitea's 404 as if it were the change. So +# both paths assert the response really is what they asked for and exit non-zero +# otherwise. +set -e +. "$(dirname "$0")/_lib.sh" + +if [ -z "$CODEV_PR_NUMBER" ]; then + echo "pr-diff: CODEV_PR_NUMBER is required" >&2 + exit 2 +fi + +REPO="$(gitea_repo)" || exit 1 + +fail() { + echo "pr-diff: $1" >&2 + exit 1 +} + +if [ "$CODEV_DIFF_NAME_ONLY" = "1" ]; then + # The changed-file list is paginated like every other Gitea list endpoint. + # `|| rc=$?` rather than a bare assignment: this script runs under `set -e`, + # which would abort on tea_api_paged's status-3 truncation signal before the + # case below ever ran — turning a named diagnostic into a silent exit. + rc=0 + FILES="$(tea_api_paged "repos/${REPO}/pulls/${CODEV_PR_NUMBER}/files" "")" || rc=$? + case $rc in + 0) ;; + 3) fail "the changed-file list for PR #${CODEV_PR_NUMBER} was truncated; a partial file list would understate the review scope" ;; + # 4 = the body was not a list, i.e. an error object. It is on stdout, so it + # can finally be classified into a message that names the PR. + 4) + case "$(gitea_api_error "$FILES")" in + notfound) fail "PR #${CODEV_PR_NUMBER} not found in ${REPO}" ;; + *) fail "Gitea could not list files for PR #${CODEV_PR_NUMBER}: ${FILES}" ;; + esac + ;; + *) exit 1 ;; + esac + printf '%s' "$FILES" | jq -r '.[] | .filename // empty' + exit 0 +fi + +DIFF="$(gitea_api "repos/${REPO}/pulls/${CODEV_PR_NUMBER}.diff")" || exit 1 + +# A unified diff is not JSON, so the JSON-shaped checks in gitea_api_error only +# fire on the error path — which is exactly what makes them a usable test here. +case "$(gitea_api_error "$DIFF")" in + notfound) fail "PR #${CODEV_PR_NUMBER} not found in ${REPO}" ;; +esac +# An empty body is not a diff. A PR with no changes still returns diff text; a +# blank response means the request did not produce one. +if [ -z "$DIFF" ]; then + fail "Gitea returned an empty diff for PR #${CODEV_PR_NUMBER}" +fi +# Guard the remaining error shape: a JSON object where a diff was expected. +if printf '%s' "$DIFF" | jq -e 'type == "object"' >/dev/null 2>&1; then + fail "Gitea returned an error instead of a diff for PR #${CODEV_PR_NUMBER}: ${DIFF}" +fi + +printf '%s\n' "$DIFF" diff --git a/packages/codev/scripts/forge/gitea/pr-exists.sh b/packages/codev/scripts/forge/gitea/pr-exists.sh index 9cc437d3a..b0c8ae6a5 100755 --- a/packages/codev/scripts/forge/gitea/pr-exists.sh +++ b/packages/codev/scripts/forge/gitea/pr-exists.sh @@ -1,29 +1,91 @@ #!/bin/sh -# Forge concept: pr-exists (Gitea via tea CLI) -# Input: CODEV_BRANCH_NAME +# Forge concept: pr-exists (Gitea/Forgejo via tea CLI) +# forge-executable: tea +# Input: CODEV_BRANCH_NAME (required) +# CODEV_PR_BASE (optional — the PR's base branch; defaults to the +# repository's default branch) # 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). +# excluded, matching the github and gitlab scripts (bugfix #568, #653). # -# 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. +# WHY THIS DOES NOT LIST PULLS (issue #12) # -# `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). +# The previous implementation walked `repos/{repo}/pulls?state=all` page by page +# and filtered client-side. That is not merely inefficient — on a real Forgejo it +# does not finish. The cost of that endpoint is per RETURNED PR OBJECT, not per +# request: measured against Forgejo 15.x, `limit=1` answers in 0.78s and +# `limit=50` in 32.8s, i.e. ~0.65s per pull, because Gitea materialises head and +# base commit info for each one. A 1599-PR repository therefore cost ~17 minutes +# for a single yes/no question, which read as a hang and was killed at 25s and at +# 120s with nothing on stdout. Raising the page size cannot help; the fix is to +# stop enumerating. +# +# `GET repos/{repo}/pulls/{base}/{head}` answers the same question in one request +# (~1.2s), and answers it BETTER. The old scan matched on `.head.ref`, and the +# comment it carried noted — correctly — that Gitea rewrites `.head.ref` to +# "refs/pull/N/head" once a merged PR's source branch is deleted, so a merged PR +# could not be found by branch name at all. But `.head.label` retains the +# original branch name, and this endpoint matches on the stored head branch +# rather than on `.head.ref`. Verified against Forgejo 15.x: PR 3869, merged with +# its branch deleted, reports `.head.ref == "refs/pull/3869/head"` and +# `.head.label == "builder/aspir-3860"`, and is returned by +# `pulls/main/builder/aspir-3860`. Slashes in the head branch need no escaping. +# +# LIMITATION: the endpoint requires a base branch, so a PR targeting something +# other than the repository's default branch (sequential-PR work branched off an +# integration branch) needs CODEV_PR_BASE. A miss makes this print "false", which +# fails porch's pr_exists gate loudly rather than proceeding on a wrong answer. +# Falling back to a list scan on the 404 path was rejected deliberately: it would +# reintroduce the ~17-minute walk on the failure path, where it would be least +# expected. +# +# "false" means "no such PR". It never means "I could not tell" — an +# unreachable repo or a failed request exits non-zero with a message on stderr, +# because a gate that reads "could not tell" as "no" is how a silent wrong answer +# gets made. +set -e . "$(dirname "$0")/_lib.sh" + +if [ -z "$CODEV_BRANCH_NAME" ]; then + echo "pr-exists: CODEV_BRANCH_NAME is required" >&2 + exit 2 +fi + 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' + +# Always resolve the repo object, even when CODEV_PR_BASE makes the default +# branch unnecessary. Gitea returns a BYTE-IDENTICAL 404 body for "this repo +# does not exist or you cannot see it" and for "this branch has no PR", so the +# lookup's 404 is only readable as "no PR" once the repo is known to resolve. +# gitea_default_branch does both in one ~0.2s request. +DEFAULT_BASE="$(gitea_default_branch "$REPO")" || exit 1 +BASE=${CODEV_PR_BASE:-$DEFAULT_BASE} + +RESPONSE="$(gitea_api "repos/${REPO}/pulls/${BASE}/${CODEV_BRANCH_NAME}")" || exit 1 + +case "$(gitea_api_error "$RESPONSE")" in + notfound) + # The repo resolved above, so this 404 is the real answer. + echo false + exit 0 + ;; + error) + echo "pr-exists: Gitea could not answer for '${BASE}...${CODEV_BRANCH_NAME}': ${RESPONSE}" >&2 + exit 1 + ;; +esac + +RESULT="$(printf '%s' "$RESPONSE" | jq -r ' + if type == "object" and (.number | type) == "number" + then (.state == "open" or .merged == true) + else empty + end +' 2>/dev/null)" || RESULT= + +if [ -z "$RESULT" ]; then + echo "pr-exists: unexpected response from the Gitea API: ${RESPONSE}" >&2 + exit 1 +fi + +echo "$RESULT" diff --git a/packages/codev/scripts/forge/gitea/pr-list.sh b/packages/codev/scripts/forge/gitea/pr-list.sh index 73c84f4be..daeeac9a4 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) # @@ -24,9 +25,21 @@ # 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). +# +# A truncated list of open PRs is not a shorter list of open PRs — it is a wrong +# one, and it looks identical. tea_api_paged reports that with exit status 3, so +# the walk is run on its own and its status inspected rather than piped straight +# into jq, where it would be discarded. . "$(dirname "$0")/_lib.sh" REPO="$(gitea_repo)" || exit 1 -tea_api_paged "repos/${REPO}/pulls" "state=open" \ +PULLS="$(tea_api_paged "repos/${REPO}/pulls" "state=open")" +case $? in + 0) ;; + 3) echo "pr-list: the open-PR list was truncated; refusing to report a partial list as complete" >&2; exit 3 ;; + 4) echo "pr-list: Gitea did not return a list of pulls for '${REPO}': ${PULLS}" >&2; exit 1 ;; + *) exit 1 ;; +esac +printf '%s' "$PULLS" \ | jq '[.[] | { number, title, diff --git a/packages/codev/scripts/forge/gitea/pr-search.sh b/packages/codev/scripts/forge/gitea/pr-search.sh new file mode 100755 index 000000000..4a0420328 --- /dev/null +++ b/packages/codev/scripts/forge/gitea/pr-search.sh @@ -0,0 +1,199 @@ +#!/bin/sh +# Forge concept: pr-search (Gitea/Forgejo via tea CLI) +# forge-executable: tea +# Input: CODEV_SEARCH_QUERY +# Output: JSON [{number, title, state, url, headRefName, baseRefName}] +# (PrSearchItem in forge-contracts.ts) +# +# Forgejo has no GitHub-style PR search, so the query is PARSED rather than +# forwarded. The grammar covers exactly the five query strings this codebase +# builds, and anything outside it returns [] rather than a guess: +# +# head: consult findPRForCurrentBranch (consult/index.ts) +# head: is:merged afx cleanup, merged check +# head: is:open afx cleanup, open check +# consult findPRForIssue +# in:body # is:open afx spawn, collision check +# +# STATE DEFAULT — upstream cluesmith/codev#1331 (fixes #759). With no `is:` +# qualifier the search spans ALL states, because `consult --type pr` runs after +# the PR is merged and an open-only search fails there with "No PR found for +# branch". An explicit `is:` qualifier overrides that default in both +# directions, which is what makes the two afx cleanup queries and the afx spawn +# query mean what they say. +# +# That default is also why afx spawn's query carries `is:open`. #1331's review +# established that spawn-worktree.ts leaned on the OLD open-only default to mean +# "open PRs", so making the search all-states without touching it aborts every +# re-spawn of an issue that ever had a merged PR, with a factually wrong "Found +# N open PR(s)". The qualifier is passed explicitly there now; this script must +# keep honouring it. +# +# ORDERING — open PRs first, then by descending number. Callers read prs[0] +# (consult/index.ts findPRForCurrentBranch and findPRForIssue both do), and once +# results span states "first" has to mean something. The open PR is the live one; +# among closed ones the highest number is the most recent. +set -e +. "$(dirname "$0")/_lib.sh" + +# How many candidate PRs from an issue-number search get resolved to full PR +# objects. Each costs ~1s against a real Forgejo. An issue with more than this +# many PRs referencing it is a pathological case, and taking the newest ones is +# the right truncation — but say so on stderr rather than quietly shortening. +SEARCH_MAX_CANDIDATES=${CODEV_FORGE_SEARCH_MAX:-10} + +QUERY=${CODEV_SEARCH_QUERY:-} + +# --- parse ----------------------------------------------------------------- +HEAD_BRANCH= +TERM= +WANT_OPEN=0 +WANT_MERGED=0 +WANT_CLOSED=0 +HAS_STATE=0 +UNKNOWN=0 + +for token in $QUERY; do + case "$token" in + head:*) HEAD_BRANCH=${token#head:} ;; + is:open) WANT_OPEN=1; HAS_STATE=1 ;; + is:merged) WANT_MERGED=1; HAS_STATE=1 ;; + is:closed) WANT_CLOSED=1; HAS_STATE=1 ;; + # `in:body` names where GitHub should look. Forgejo's `q=` already searches + # title and body together, so it is accepted and carries no extra meaning. + in:body) ;; + '#'[0-9]*) TERM=${token#\#} ;; + [0-9]*) + case "$token" in + *[!0-9]*) UNKNOWN=1 ;; + *) TERM=$token ;; + esac + ;; + *) UNKNOWN=1 ;; + esac +done + +if [ "$HAS_STATE" -eq 0 ]; then + WANT_OPEN=1; WANT_MERGED=1; WANT_CLOSED=1 +fi + +if [ -z "$HEAD_BRANCH" ] && [ -z "$TERM" ]; then + if [ -n "$QUERY" ]; then + echo "pr-search: gitea does not understand the query '${QUERY}' — returning no matches rather than guessing" >&2 + fi + echo '[]' + exit 0 +fi +if [ "$UNKNOWN" -eq 1 ]; then + echo "pr-search: ignoring unrecognised term(s) in '${QUERY}'" >&2 +fi + +REPO="$(gitea_repo)" || exit 1 +# Resolves the base branch AND proves the repo is reachable — Gitea's 404 for an +# unknown repo is byte-identical to its 404 for a branch with no PR, so without +# this a typo'd CODEV_REPO would return an empty result set that looks like a +# confident "no matches". See gitea_default_branch. +DEFAULT_BASE="$(gitea_default_branch "$REPO")" || exit 1 + +# Shared jq: Gitea's raw state ("open"/"closed" plus a `merged` bool) normalised +# to the three values callers can act on, then filtered and ordered. +SHAPE_AND_FILTER=' + def normstate: if .state == "open" then "open" + elif .merged == true then "merged" + else "closed" end; + [ .[] + | select(type == "object" and (.number | type) == "number") + | { + number, + title: (.title // ""), + state: normstate, + url: (.html_url // .url // ""), + # `.head.ref` becomes "refs/pull/N/head" once a merged PR'"'"'s branch is + # deleted; `.head.label` keeps the branch name. Prefer the label and fall + # back to the ref. (A cross-repo fork PR labels as "owner:branch"; codev + # builders push to the same repo, so the plain form is what we see.) + headRefName: (.head.label // .head.ref // ""), + baseRefName: (.base.ref // "") + } + ] + | map(select( + (.state == "open" and $want_open == 1) or + (.state == "merged" and $want_merged == 1) or + (.state == "closed" and $want_closed == 1) + )) + | sort_by(if .state == "open" then 0 else 1 end, -.number) +' + +emit() { + printf '%s' "$1" | jq \ + --argjson want_open "$WANT_OPEN" \ + --argjson want_merged "$WANT_MERGED" \ + --argjson want_closed "$WANT_CLOSED" \ + "$SHAPE_AND_FILTER" +} + +# --- branch lookup --------------------------------------------------------- +if [ -n "$HEAD_BRANCH" ]; then + BASE=${CODEV_PR_BASE:-$DEFAULT_BASE} + RESPONSE="$(gitea_api "repos/${REPO}/pulls/${BASE}/${HEAD_BRANCH}")" || exit 1 + case "$(gitea_api_error "$RESPONSE")" in + notfound) echo '[]'; exit 0 ;; + error) + echo "pr-search: Gitea could not answer for '${BASE}...${HEAD_BRANCH}': ${RESPONSE}" >&2 + exit 1 + ;; + esac + emit "[${RESPONSE}]" + exit 0 +fi + +# --- issue-number lookup --------------------------------------------------- +# Two steps, because the two things needed live in different places. The cheap +# index (`issues?type=pulls`) can be searched by text and carries title/body, but +# no head or base ref; the PR object carries the refs but its list endpoint costs +# ~1s per PR. So: search the index, decide which PRs belong to the issue, then +# resolve only those. +if [ "$WANT_OPEN" -eq 1 ] && [ "$WANT_MERGED" -eq 0 ] && [ "$WANT_CLOSED" -eq 0 ]; then + INDEX_STATE=open +elif [ "$WANT_OPEN" -eq 0 ]; then + INDEX_STATE=closed +else + INDEX_STATE=all +fi + +INDEX="$(gitea_api "repos/${REPO}/issues?type=pulls&state=${INDEX_STATE}&q=${TERM}&limit=${GITEA_PAGE_LIMIT}")" || exit 1 +case "$(gitea_api_error "$INDEX")" in + notfound) echo '[]'; exit 0 ;; + error) + echo "pr-search: Gitea could not search pulls for '${TERM}': ${INDEX}" >&2 + exit 1 + ;; +esac + +# Gitea's `q=` is a substring match, so it answers "3386" with PRs mentioning +# 33861. Word-bound both forms — "#N" as a cross-reference and a bare N as this +# repo titles them ("[Spec N] ...") — so #3386 never matches #33861. +CANDIDATES="$(printf '%s' "$INDEX" | jq -r --arg n "$TERM" ' + def bounded($s): ($s // "") | test("(?/dev/null)" || CANDIDATES= + +if [ -z "$CANDIDATES" ]; then + echo '[]' + exit 0 +fi + +COUNT=$(printf '%s\n' "$CANDIDATES" | wc -l | tr -d ' ') +if [ "$COUNT" -gt "$SEARCH_MAX_CANDIDATES" ]; then + echo "pr-search: ${COUNT} PRs reference #${TERM}; resolving the ${SEARCH_MAX_CANDIDATES} most recent (raise CODEV_FORGE_SEARCH_MAX to widen)" >&2 + CANDIDATES=$(printf '%s\n' "$CANDIDATES" | head -n "$SEARCH_MAX_CANDIDATES") +fi + +# shellcheck disable=SC2086 # word splitting is the point: one arg per number +PULLS="$(gitea_fetch_pulls "$REPO" $CANDIDATES)" || exit 1 +emit "$PULLS" diff --git a/packages/codev/scripts/forge/gitea/recently-merged.sh b/packages/codev/scripts/forge/gitea/recently-merged.sh index 7f134ce47..9731cfde4 100755 --- a/packages/codev/scripts/forge/gitea/recently-merged.sh +++ b/packages/codev/scripts/forge/gitea/recently-merged.sh @@ -1,30 +1,160 @@ #!/bin/sh -# Forge concept: recently-merged (Gitea via tea CLI) +# Forge concept: recently-merged (Gitea/Forgejo via tea CLI) +# forge-executable: tea +# Input: CODEV_SINCE_DATE (optional — RFC3339, or a bare YYYY-MM-DD) # Output: JSON [{number, title, url, body, createdAt, mergedAt, headRefName}] # (MergedPrItem in forge-contracts.ts) # -# `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). -# -# 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). +# WHY THIS DOES NOT PAGE `pulls?state=closed` (issue #12) +# +# It used to, walking every page and filtering to merged in jq. On the reference +# Forgejo that is 1598 closed pulls at ~1s each — about 26 minutes for a panel +# that wants the last 24 hours. It never surfaced as a hang because +# executeForgeCommand kills a concept at 30s and returns null, so afx status and +# every dashboard poll rendered an EMPTY merged-PR panel with nothing on stderr. +# A silent wrong answer, on every poll, indefinitely. +# +# Two endpoints replace it, because the two things needed live in different +# places: +# +# 1. `issues?type=pulls&state=closed&since=` — the cheap index. ~1.8s +# per 50 against the same repo the pulls list charges ~50s for, because it +# does not materialise head/base commit info. Carries number, title, body, +# created_at, html_url and pull_request.merged_at: everything the contract +# wants except the head branch. `since` is a SERVER-side filter, so the +# window bounds the work rather than the client discarding most of it. +# 2. `pulls/{n}` per surviving match, for the head branch only, fetched +# GITEA_CONCURRENCY at a time (see gitea_fetch_pulls). +# +# `since` filters on updated_at, not merged_at. That is sound here and not an +# approximation: merging a PR updates it, so merged_at <= updated_at always, and +# no PR merged inside the window can have updated_at outside it. The reverse is +# not true — a PR merged long ago and commented on yesterday comes back — so +# merged_at is still checked against the cutoff below. +# +# BOUNDS. Every path through this script is bounded, and it says so when a bound +# bites: +# +# * No CODEV_SINCE_DATE -> a default window of GITEA_MERGED_DEFAULT_DAYS (7), +# announced on stderr. It does NOT mean "all time": +# that is the 26-minute walk, and answering it +# slowly is not better than refusing it. +# * Window too wide -> more than GITEA_MERGED_MAX_PRS (300) merged PRs in +# the window stops with exit status 3. +# * Index still paging -> tea_api_paged's own deadline, also status 3. +# +# EXIT STATUS 3 IS THE TRUNCATION MARKER, and stdout is empty when it is +# returned. "Nothing merged" is `[]` with status 0; "I stopped looking" is status +# 3 with a stderr line saying what bound bit. They must not be spelled the same +# way — a short list and a truncated list are indistinguishable once printed, +# which is the exact shape of the bug this script is replacing. +set -e . "$(dirname "$0")/_lib.sh" + +# Default lookback when the caller names no date. Sized so that the widest +# routine caller still fits inside the 30s ceiling executeForgeCommand puts on +# every concept: a week is ~107 merged PRs on the reference repo, ~14s at +# GITEA_CONCURRENCY=8. A month is ~476, which cannot fit at any concurrency this +# script would be polite enough to use — so it is refused loudly, not attempted. +GITEA_MERGED_DEFAULT_DAYS=${CODEV_FORGE_MERGED_DAYS:-7} +case "$GITEA_MERGED_DEFAULT_DAYS" in + ''|*[!0-9]*|0) GITEA_MERGED_DEFAULT_DAYS=7 ;; +esac + +GITEA_MERGED_MAX_PRS=${CODEV_FORGE_MERGED_MAX:-300} +case "$GITEA_MERGED_MAX_PRS" in + ''|*[!0-9]*|0) GITEA_MERGED_MAX_PRS=300 ;; +esac + REPO="$(gitea_repo)" || exit 1 -tea_api_paged "repos/${REPO}/pulls" "state=closed" \ - | jq '[.[] | select(.merged == true) | { - number, - title, - url: (.html_url // .url), - body: (.body // ""), - createdAt: .created_at, - mergedAt: .merged_at, - headRefName: (.head.ref // "") - }]' + +# --- cutoff ---------------------------------------------------------------- +# Normalise to exactly YYYY-MM-DDTHH:MM:SSZ. Both this and Gitea's timestamps +# are then UTC, second-precision and fixed-width, which is what makes the plain +# string comparison in the jq below a correct chronological one. +# +# `jq -n now` rather than date(1) for the default window: BSD date wants +# `-v-7d` and GNU date wants `-d '7 days ago'`, and jq is already a hard +# dependency of every script here. +if [ -n "$CODEV_SINCE_DATE" ]; then + case "$CODEV_SINCE_DATE" in + ????-??-??) CUTOFF="${CODEV_SINCE_DATE}T00:00:00Z" ;; + ????-??-??T??:??:??Z) CUTOFF="$CODEV_SINCE_DATE" ;; + ????-??-??T??:??:??*) CUTOFF="$(printf '%.19s' "$CODEV_SINCE_DATE")Z" ;; + *) + echo "recently-merged: could not read CODEV_SINCE_DATE='${CODEV_SINCE_DATE}' as a date; expected YYYY-MM-DD or RFC3339" >&2 + exit 2 + ;; + esac +else + CUTOFF="$(jq -rn --argjson s "$((GITEA_MERGED_DEFAULT_DAYS * 86400))" 'now - $s | todate')" + echo "recently-merged: no CODEV_SINCE_DATE given; limiting to the last ${GITEA_MERGED_DEFAULT_DAYS} days (since ${CUTOFF}). Set CODEV_SINCE_DATE, or raise CODEV_FORGE_MERGED_DAYS." >&2 +fi + +# --- cheap index ----------------------------------------------------------- +rc=0 +INDEX="$(tea_api_paged "repos/${REPO}/issues" "type=pulls&state=closed&since=${CUTOFF}")" || rc=$? +case $rc in + 0) ;; + 3) echo "recently-merged: the closed-pull index for '${REPO}' since ${CUTOFF} was truncated; narrow CODEV_SINCE_DATE rather than trusting a partial list" >&2; exit 3 ;; + # 4 = the body was not a list, i.e. an error object, now on stdout to classify. + 4) + case "$(gitea_api_error "$INDEX")" in + notfound) echo "recently-merged: repository '${REPO}' has no readable pull index" >&2 ;; + *) echo "recently-merged: Gitea could not list closed pulls for '${REPO}': ${INDEX}" >&2 ;; + esac + exit 1 + ;; + *) exit 1 ;; +esac + +BASE_RECORDS="$(printf '%s' "$INDEX" | jq --arg cutoff "$CUTOFF" ' + [ .[] + | select(type == "object" and (.number | type) == "number") + | select(.pull_request != null and .pull_request.merged_at != null) + | select(.pull_request.merged_at >= $cutoff) + | { + number, + title: (.title // ""), + url: (.pull_request.html_url // .html_url // ""), + body: (.body // ""), + createdAt: .created_at, + mergedAt: .pull_request.merged_at + } + ] + | sort_by(.mergedAt) | reverse +')" + +COUNT="$(printf '%s' "$BASE_RECORDS" | jq 'length')" +if [ "$COUNT" -eq 0 ]; then + echo '[]' + exit 0 +fi +if [ "$COUNT" -gt "$GITEA_MERGED_MAX_PRS" ]; then + echo "recently-merged: ${COUNT} PRs merged since ${CUTOFF}, over the ${GITEA_MERGED_MAX_PRS} ceiling. Refusing to return a partial list — narrow CODEV_SINCE_DATE, or raise CODEV_FORGE_MERGED_MAX." >&2 + exit 3 +fi + +# --- head branches --------------------------------------------------------- +# The one field the index cannot supply. analytics.ts derives the protocol from +# it (protocolFromBranch), so an empty value is not a harmless omission. +NUMBERS="$(printf '%s' "$BASE_RECORDS" | jq -r '.[].number')" +# shellcheck disable=SC2086 # word splitting is the point: one arg per number +PULLS="$(gitea_fetch_pulls "$REPO" $NUMBERS)" || exit 1 + +# Reduce to a compact {number: branch} map BEFORE it reaches argv. Passing the +# raw PR objects through `--argjson` overflows ARG_MAX: a week of merges on the +# reference repo is 107 full PR objects and `jq: Argument list too long` was the +# result. The map is one short string per PR, so at the GITEA_MERGED_MAX_PRS +# ceiling it is ~13KB — nowhere near the limit. +# +# `.head.ref` reads "refs/pull/N/head" once a merged PR's branch is deleted, +# which is the normal state of every PR this concept returns; `.head.label` +# keeps the branch name. Prefer the label. +HEADS="$(printf '%s' "$PULLS" | jq -c ' + map({ key: (.number | tostring), value: (.head.label // .head.ref // "") }) | from_entries +')" + +printf '%s' "$BASE_RECORDS" | jq --argjson heads "$HEADS" ' + map(. + { headRefName: ($heads[.number | tostring] // "") }) +' 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..c3949e54c 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 @@ -18,6 +18,15 @@ * fast (stderr + non-zero exit) when there's no usable origin remote. * - `issue-view` warns on stderr when the comments fetch degrades to []. * + * Issue #12 note: `pr-exists` and `recently-merged` no longer read the `pulls` + * LIST endpoint, so their fixtures and tests below moved with them. Forgejo + * charges that endpoint per returned PR object (~0.65s each, measured), which + * made a 1599-PR repo cost ~17 minutes for `pr-exists` and ~26 minutes for + * `recently-merged`. #1137's guarantee is unchanged and still asserted: reads go + * through `tea api`, never `tea pulls list`, and a paged read is not silently + * truncated. What changed is which endpoints are paged — `pr-exists` now pages + * nothing at all, which is the strongest form of "does not truncate". + * * `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 @@ -60,13 +69,31 @@ 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}' ;; - # --- 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"}}]' ;; + # --- the repo probe ------------------------------------------------------ + # pr-exists resolves the repo before answering, both to learn the default + # branch and to prove the repo is readable: Gitea's 404 for an unknown repo is + # byte-identical to its 404 for a branch with no PR, so without this probe a + # typo'd repo would answer a confident "false". + repos/acme/widgets) + echo '{"default_branch":"main"}' ;; + + # --- pr-exists: one base/head lookup per branch (#12) -------------------- + # No list call, no paging. "feature/done" is MERGED with its branch deleted, + # so its .head.ref reads refs/pull/40/head and only .head.label still + # carries the branch name — the shape that made a head.ref scan unable to find + # a merged PR at all. + repos/acme/widgets/pulls/main/feature/x) + echo '{"number":42,"state":"open","merged":false,"head":{"ref":"feature/x","label":"feature/x"},"base":{"ref":"main"}}' ;; + repos/acme/widgets/pulls/main/feature/done) + echo '{"number":40,"state":"closed","merged":true,"head":{"ref":"refs/pull/40/head","label":"feature/done"},"base":{"ref":"main"}}' ;; + repos/acme/widgets/pulls/main/feature/abandoned) + echo '{"number":39,"state":"closed","merged":false,"head":{"ref":"feature/abandoned","label":"feature/abandoned"},"base":{"ref":"main"}}' ;; + # Any other branch has no PR. Gitea answers that with a 404 body, which is + # BYTE-IDENTICAL to its 404 for an unreadable repository — the reason + # pr-exists probes the repo first. Served from a file because the real body + # contains an apostrophe. + repos/acme/widgets/pulls/main/*) + cat "$TEA_NOT_FOUND" ;; # --- pr-list: state=open, paginated -------------------------------------- # page 1 = the rich #42 item + 49 pad (50 total). page 2 = 1 item (#900). @@ -75,13 +102,23 @@ case "$2" in "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"}}]' ;; + # --- recently-merged: the cheap ISSUES index, paginated (#12) ------------ + # This index costs ~1.8s per 50 where the pulls list costs ~50s for the same + # rows, because it does not materialise head/base commit info. It carries + # everything the contract needs EXCEPT the head branch, which is then fetched + # per match below. Paging still matters here, so page 1 is a full 50 and #901 + # lives only on page 2. + "repos/acme/widgets/issues?type=pulls&state=closed&since=2026-07-01T00:00:00Z&limit=50&page=1") + jq -cn '[{number:40,title:"Done PR",html_url:"https://git.example.com/acme/widgets/pulls/40",body:"merged body",created_at:"2026-07-02T09:00:00Z",pull_request:{merged_at:"2026-07-05T12:00:00Z"}},{number:39,title:"Abandoned",body:"",created_at:"2026-07-02T09:00:00Z",pull_request:{merged_at:null}}] + [range(48)|{number:(2000+.),title:"pad",body:"",created_at:"d",pull_request:{merged_at:null}}]' ;; + "repos/acme/widgets/issues?type=pulls&state=closed&since=2026-07-01T00:00:00Z&limit=50&page=2") + echo '[{"number":901,"title":"Deep merge","html_url":"https://git.example.com/acme/widgets/pulls/901","body":"deep merged","created_at":"2026-07-03T09:00:00Z","pull_request":{"merged_at":"2026-07-06T12:00:00Z"}}]' ;; + + # Head branches, one request per merged match. Both branches are deleted, so + # .head.label is the only place the name survives. + repos/acme/widgets/pulls/40) + echo '{"number":40,"head":{"ref":"refs/pull/40/head","label":"feature/done"}}' ;; + repos/acme/widgets/pulls/901) + echo '{"number":901,"head":{"ref":"refs/pull/901/head","label":"feature/deep-merge"}}' ;; # --- issue-view ---------------------------------------------------------- repos/acme/widgets/issues/99) @@ -115,6 +152,13 @@ function hasJq(): boolean { const jqAvailable = hasJq(); +/** + * The window `recently-merged` asks for. Since #12 the concept takes a window + * and passes it to the server as a `since` filter rather than walking every + * closed pull and discarding most of it, so the fixtures are keyed by it. + */ +const SINCE = { CODEV_SINCE_DATE: '2026-07-01T00:00:00Z' }; + /** 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)], { @@ -154,7 +198,18 @@ 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 ?? ''}` }; + // Gitea's 404 body, verbatim from Forgejo 15.x. Written to a file so the + // apostrophe in "couldn't" survives the fake tea's shell quoting. + const notFoundPath = join(fixture, 'notfound.json'); + writeFileSync( + notFoundPath, + '{"message":"The target couldn\'t be found.","url":"https://git.example.com/api/swagger","errors":[]}', + ); + runEnv = { + ...process.env, + PATH: `${binDir}:${process.env.PATH ?? ''}`, + TEA_NOT_FOUND: notFoundPath, + }; }); afterAll(() => { @@ -210,7 +265,11 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'feature/x' })).toBe('true'); }); - it('pr-exists is true for a MERGED pull on the branch', () => { + it('pr-exists is true for a MERGED pull whose branch was deleted', () => { + // `.head.ref` on this fixture is "refs/pull/40/head". The old scan matched + // on head.ref and therefore could not find a merged PR by branch name at + // all; the base/head lookup matches on the stored head branch, which Gitea + // also exposes as `.head.label`. expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'feature/done' })).toBe('true'); }); @@ -222,10 +281,17 @@ 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('pr-exists cannot truncate, because it does not list (#12)', () => { + // This replaces the old "a merged PR only on page 2 is found" test. That + // test proved the scan paged correctly; the scan is gone, because paging it + // correctly still cost ~17 minutes on a real 1599-PR Forgejo. The fake tea + // above serves NO `pulls?state=…` fixture any more, so a script that + // reintroduced the scan would fail here with "no fixture for", and the + // endpoint-level assertion lives in pir-12-gitea-pr-concepts.test.ts. + expect(runScript('pr-exists.sh', { CODEV_BRANCH_NAME: 'feature/x' })).toBe('true'); + const { status, stderr } = runScriptFull('pr-exists.sh', { CODEV_BRANCH_NAME: 'feature/x' }); + expect(status).toBe(0); + expect(stderr).not.toContain('no fixture'); }); it('issue-view returns body, browser url, and comments as an ARRAY', () => { @@ -256,7 +322,7 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` }); it('recently-merged keeps merged pulls only and uses merged_at', () => { - const merged = JSON.parse(runScript('recently-merged.sh')); + const merged = JSON.parse(runScript('recently-merged.sh', SINCE)); const done = merged.find((p: { number: number }) => p.number === 40); expect(done).toEqual({ number: 40, @@ -272,7 +338,7 @@ describe.skipIf(!jqAvailable)('bugfix #1137: gitea preset routes reads through ` }); it('recently-merged paginates: a merged PR only on page 2 is included', () => { - const merged = JSON.parse(runScript('recently-merged.sh')); + const merged = JSON.parse(runScript('recently-merged.sh', SINCE)); expect(merged).toHaveLength(2); // #40 (page 1) + #901 (page 2) const deep = merged.find((p: { number: number }) => p.number === 901); expect(deep).toMatchObject({ diff --git a/packages/codev/src/__tests__/pir-12-gitea-pr-concepts.test.ts b/packages/codev/src/__tests__/pir-12-gitea-pr-concepts.test.ts new file mode 100644 index 000000000..deadddc25 --- /dev/null +++ b/packages/codev/src/__tests__/pir-12-gitea-pr-concepts.test.ts @@ -0,0 +1,667 @@ +/** + * Issue #12 — Forgejo/Gitea forge parity: pr-search, pr-diff, and the + * pr-exists "hang". + * + * Three things are pinned here, and the first is the one that matters most. + * + * 1. **pr-exists must never enumerate pulls.** The old implementation walked + * `pulls?state=all` page by page. Forgejo charges that endpoint per RETURNED + * PR OBJECT — measured 0.78s at limit=1 and 32.8s at limit=50 against + * Forgejo 15.x — so on a 1599-PR repository a single yes/no question cost + * ~17 minutes and read as a hang. The regression is easy to reintroduce + * while keeping a behavioural test green, because a scan returns the right + * answer; it just takes a quarter of an hour. So the endpoints are asserted + * directly. + * + * 2. **A merged PR whose branch was deleted is still findable.** Gitea rewrites + * `.head.ref` to "refs/pull/N/head" once the source branch is gone, which is + * the normal state of every merged PR here, but `.head.label` keeps the + * branch name. That is undocumented Gitea behaviour and exactly the kind of + * thing a Forgejo upgrade breaks silently. + * + * 3. **The `is:` qualifier grammar, and afx spawn's explicit `is:open`.** Making + * pr-search all-states (upstream cluesmith/codev#1331, fixing #759) is + * correct and breaks spawn-worktree.ts, which leaned on the old open-only + * default. See the query table in gitea/pr-search.sh. + * + * The script-content assertions are anchored to command lines rather than + * matched with `toContain`, so the explanatory comments in the scripts — which + * quote the very strings under test — cannot make an assertion pass after the + * code it pins has been removed. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync, spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { getForgeCommand, resolveAllConcepts } from '../lib/forge.js'; + +const codevPkgRoot = path.resolve(import.meta.dirname, '..', '..'); +const repoRoot = path.resolve(codevPkgRoot, '..', '..'); +const forgeScripts = path.join(codevPkgRoot, 'scripts', 'forge'); +const gitea = path.join(forgeScripts, 'gitea'); + +function hasJq(): boolean { + try { + execFileSync('sh', ['-c', 'command -v jq'], { stdio: 'ignore' }); + return true; + } catch { + return false; + } +} + +/** An open PR, branch intact. */ +const OPEN_PR = JSON.stringify({ + number: 3855, + title: 'open one', + state: 'open', + merged: false, + html_url: 'https://forge.example.com/o/r/pulls/3855', + url: 'https://forge.example.com/api/v1/repos/o/r/pulls/3855', + head: { ref: 'builder/air-364', label: 'builder/air-364' }, + base: { ref: 'main' }, +}); + +/** + * A MERGED PR whose source branch has been deleted — `.head.ref` has been + * rewritten and only `.head.label` still carries the branch name. Copied from + * the real shape of PR 3869 on the reference Forgejo. + */ +const MERGED_PR_DELETED_BRANCH = JSON.stringify({ + number: 3869, + title: 'merged one', + state: 'closed', + merged: true, + html_url: 'https://forge.example.com/o/r/pulls/3869', + head: { ref: 'refs/pull/3869/head', label: 'builder/aspir-3860' }, + base: { ref: 'main' }, +}); + +/** A PR closed without merging — must never count as "exists". */ +const CLOSED_PR = JSON.stringify({ + number: 3800, + title: 'abandoned', + state: 'closed', + merged: false, + html_url: 'https://forge.example.com/o/r/pulls/3800', + head: { ref: 'builder/abandoned', label: 'builder/abandoned' }, + base: { ref: 'main' }, +}); + +/** Gitea's 404 body. Identical for an unknown repo and an unknown target. */ +const NOT_FOUND = '{"message":"The target couldn\'t be found.","url":"https://forge.example.com/api/swagger","errors":[]}'; + +/** + * A fake `tea` serving the slice of `tea api` these scripts use. + * + * Logs every requested endpoint to `endpoints`, one per line, so a test can + * assert both what was called and — the point of this whole exercise — what + * was not. + * + * TEA_ROUTES is a `\t` table; the first match wins and its + * file is emitted verbatim. The body lives in a FILE rather than in the table + * cell because a raw diff contains newlines, which a line-oriented table + * silently truncates to its first line. + */ +const TEA_STUB = [ + 'if [ "$1" != "api" ]; then echo "unexpected tea invocation: $*" >&2; exit 9; fi', + 'for a in "$@"; do ep=$a; done', + 'printf "%s\\n" "$ep" >> "$TEA_LOG"', + 'while IFS="\t" read -r pattern bodyfile; do', + ' [ -n "$pattern" ] || continue', + ' # shellcheck disable=SC2254 # the pattern is meant to glob', + ' case "$ep" in', + ' $pattern) cat "$bodyfile"; exit 0 ;;', + ' esac', + 'done < "$TEA_ROUTES"', + // Gitea's 404 body carries an apostrophe, so it is read from a file rather + // than quoted into the stub — inlining it broke the stub's own shell syntax. + 'cat "$TEA_NOT_FOUND"', + 'exit 0', +].join('\n'); + +describe('#12 — the gitea preset offers pr-search and pr-diff', () => { + it.each(['pr-search', 'pr-diff', 'pr-exists', 'recently-merged'])( + 'routes %s to the gitea script instead of disabling it', + (concept) => { + const command = getForgeCommand(concept, { provider: 'gitea' }); + expect(command, `gitea has no ${concept} route`).not.toBeNull(); + expect(command).toBe(path.join(gitea, `${concept}.sh`)); + expect(fs.existsSync(command!)).toBe(true); + expect(fs.statSync(command!).mode & 0o111, 'script is not executable').not.toBe(0); + }, + ); + + it.each(['team-activity', 'on-it-timestamps'])( + 'keeps %s disabled — Forgejo has no GraphQL, and this is deliberate', + (concept) => { + expect(getForgeCommand(concept, { provider: 'gitea' })).toBeNull(); + const resolution = resolveAllConcepts({ provider: 'gitea' }).find((r) => r.concept === concept); + expect(resolution?.source).toBe('disabled'); + }, + ); + + it('doctor resolves EVERY enabled gitea concept to tea', () => { + // Not just the new ones. `extractExecutable` reads a script's first + // substantive line, which answered "echo" for issue-view and "case" for + // pr-list — so `codev doctor` on a Forgejo repo told the user to install + // `echo`, and a genuinely missing `tea` went unreported. That is the #1455 + // defect class, and the remedy is the `# forge-executable:` declaration. + // Asserted across the whole preset so a new script cannot reintroduce it. + const wrong = resolveAllConcepts({ provider: 'gitea' }) + .filter((r) => r.source !== 'disabled' && r.executable !== 'tea') + .map((r) => `${r.concept} -> ${r.executable}`); + expect(wrong).toEqual([]); + }); +}); + +describe('#12 — gitea concept scripts against a fake tea', () => { + let tmp: string; + + beforeEach(() => { + tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'pir-12-gitea-')); + }); + + afterEach(() => { + fs.rmSync(tmp, { recursive: true, force: true }); + }); + + /** Install the fake tea with the given endpoint routes. */ + function routes(pairs: Array<[string, string]>): void { + const lines = pairs.map(([pattern, body], i) => { + const bodyFile = path.join(tmp, `body-${i}`); + fs.writeFileSync(bodyFile, body); + return `${pattern}\t${bodyFile}`; + }); + fs.writeFileSync(path.join(tmp, 'routes.tsv'), lines.join('\n') + '\n'); + fs.writeFileSync(path.join(tmp, 'notfound.json'), NOT_FOUND); + const file = path.join(tmp, 'tea'); + fs.writeFileSync(file, `#!/bin/sh\n${TEA_STUB}\n`, { mode: 0o755 }); + fs.chmodSync(file, 0o755); + } + + /** The repo probe every script makes; also what proves the repo resolves. */ + const REPO_ROUTE: [string, string] = ['repos/o/r', '{"default_branch":"main"}']; + + // spawnSync, not execFileSync: several of these assertions are about what the + // script says on stderr while SUCCEEDING (the default-window announcement, + // the truncation notes). execFileSync only surfaces stderr by throwing. + function run(script: string, env: Record = {}): { stdout: string; status: number; stderr: string } { + const r = spawnSync('sh', [path.join(gitea, script)], { + cwd: tmp, + env: { + ...process.env, + PATH: `${tmp}:${process.env.PATH}`, + CODEV_REPO: 'o/r', + TEA_LOG: path.join(tmp, 'endpoints'), + TEA_ROUTES: path.join(tmp, 'routes.tsv'), + TEA_NOT_FOUND: path.join(tmp, 'notfound.json'), + ...env, + }, + encoding: 'utf-8', + }); + return { stdout: r.stdout ?? '', stderr: r.stderr ?? '', status: r.status ?? 1 }; + } + + /** Every endpoint the script asked tea for, in order. */ + function endpoints(): string[] { + const file = path.join(tmp, 'endpoints'); + if (!fs.existsSync(file)) return []; + return fs.readFileSync(file, 'utf-8').split('\n').filter(Boolean); + } + + // ------------------------------------------------------------------------- + // pr-exists + // ------------------------------------------------------------------------- + + describe('pr-exists', () => { + it.skipIf(!hasJq())('answers true for an open PR via the base/head lookup', () => { + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/air-364', OPEN_PR]]); + const { stdout, status } = run('pr-exists.sh', { CODEV_BRANCH_NAME: 'builder/air-364' }); + expect(status).toBe(0); + expect(stdout.trim()).toBe('true'); + }); + + it.skipIf(!hasJq())( + 'finds a MERGED PR whose branch was deleted — head.label, not head.ref', + () => { + // The whole point. `.head.ref` on this PR is "refs/pull/3869/head", so + // any implementation matching on head.ref answers false. The base/head + // endpoint matches on the stored head branch, which Gitea also exposes + // as `.head.label`. If a Forgejo release ever stops honouring that, + // this test is where it shows up rather than in a builder's confused + // "no PR found" three months later. + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/aspir-3860', MERGED_PR_DELETED_BRANCH]]); + const { stdout, status } = run('pr-exists.sh', { CODEV_BRANCH_NAME: 'builder/aspir-3860' }); + expect(status).toBe(0); + expect(stdout.trim()).toBe('true'); + }, + ); + + it.skipIf(!hasJq())('NEVER issues a list call — no state=all, no pulls enumeration', () => { + // The 17-minute regression. A scan would still return the right answer, + // so only the request log can catch its return. + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/air-364', OPEN_PR]]); + run('pr-exists.sh', { CODEV_BRANCH_NAME: 'builder/air-364' }); + + const asked = endpoints(); + expect(asked).toEqual(['repos/o/r', 'repos/o/r/pulls/main/builder/air-364']); + for (const ep of asked) { + expect(ep).not.toMatch(/state=all/); + expect(ep, 'pr-exists must not page a list endpoint').not.toMatch(/[?&]page=/); + expect(ep).not.toMatch(/[?&]limit=/); + } + }); + + it.skipIf(!hasJq())('answers false for a closed-not-merged PR', () => { + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/abandoned', CLOSED_PR]]); + const { stdout } = run('pr-exists.sh', { CODEV_BRANCH_NAME: 'builder/abandoned' }); + expect(stdout.trim()).toBe('false'); + }); + + it.skipIf(!hasJq())('answers false when no PR exists for the branch', () => { + routes([REPO_ROUTE]); + const { stdout, status } = run('pr-exists.sh', { CODEV_BRANCH_NAME: 'never-opened' }); + expect(status).toBe(0); + expect(stdout.trim()).toBe('false'); + }); + + it.skipIf(!hasJq())('ERRORS rather than answering false when the repo cannot be read', () => { + // Gitea returns a byte-identical 404 for "no such repo" and "no such PR". + // Reading the first as "no PR exists" is a silent wrong answer at a gate, + // so the repo probe has to come first and its failure has to be fatal. + routes([]); + const { stdout, status, stderr } = run('pr-exists.sh', { CODEV_BRANCH_NAME: 'anything' }); + expect(status).not.toBe(0); + expect(stdout.trim()).not.toBe('false'); + expect(stderr).toMatch(/could not read repository/); + }); + + it.skipIf(!hasJq())('honours CODEV_PR_BASE for a PR against a non-default base', () => { + routes([REPO_ROUTE, ['repos/o/r/pulls/integration/feature-x', OPEN_PR]]); + const { stdout } = run('pr-exists.sh', { + CODEV_BRANCH_NAME: 'feature-x', + CODEV_PR_BASE: 'integration', + }); + expect(stdout.trim()).toBe('true'); + expect(endpoints()).toContain('repos/o/r/pulls/integration/feature-x'); + }); + + it('requires CODEV_BRANCH_NAME', () => { + routes([REPO_ROUTE]); + const { status, stderr } = run('pr-exists.sh', { CODEV_BRANCH_NAME: '' }); + expect(status).toBe(2); + expect(stderr).toMatch(/CODEV_BRANCH_NAME is required/); + }); + }); + + // ------------------------------------------------------------------------- + // The timeout — a hang must surface as an error + // ------------------------------------------------------------------------- + + describe('gitea_timeout', () => { + it('kills a tea that never returns, and names the endpoint', () => { + // A wrapper that spawns a child and waits: killing the wrapper alone + // leaves the child holding the stdout pipe, and the command substitution + // stays blocked long after the timeout "fired". That failure mode was + // observed before the temp-file decoupling in gitea_timeout, so the stub + // reproduces its shape deliberately. + fs.writeFileSync(path.join(tmp, 'routes.tsv'), '\n'); + fs.writeFileSync(path.join(tmp, 'notfound.json'), NOT_FOUND); + fs.writeFileSync(path.join(tmp, 'tea'), '#!/bin/sh\nsleep 60 &\nwait\n', { mode: 0o755 }); + fs.chmodSync(path.join(tmp, 'tea'), 0o755); + + const started = Date.now(); + const { status, stderr } = run('pr-exists.sh', { + CODEV_BRANCH_NAME: 'whatever', + CODEV_FORGE_TIMEOUT: '2', + }); + const elapsed = Date.now() - started; + + expect(status).not.toBe(0); + expect(stderr).toMatch(/did not return within 2s/); + expect(stderr).toMatch(/repos\/o\/r/); + expect(elapsed, 'the timeout did not actually unblock the caller').toBeLessThan(30_000); + }, 40_000); + }); + + // ------------------------------------------------------------------------- + // pr-search + // ------------------------------------------------------------------------- + + describe('pr-search', () => { + it.skipIf(!hasJq())('head: resolves through the base/head lookup', () => { + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/air-364', OPEN_PR]]); + const { stdout } = run('pr-search.sh', { CODEV_SEARCH_QUERY: 'head:builder/air-364' }); + expect(JSON.parse(stdout)).toEqual([ + { + number: 3855, + title: 'open one', + state: 'open', + url: 'https://forge.example.com/o/r/pulls/3855', + headRefName: 'builder/air-364', + baseRefName: 'main', + }, + ]); + }); + + it.skipIf(!hasJq())('finds a merged PR by branch with no is: qualifier (#1331/#759)', () => { + // The bug #1331 fixes: an open-only default means `consult --type pr` + // cannot find its own PR once it is merged. + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/aspir-3860', MERGED_PR_DELETED_BRANCH]]); + const { stdout } = run('pr-search.sh', { CODEV_SEARCH_QUERY: 'head:builder/aspir-3860' }); + const prs = JSON.parse(stdout); + expect(prs).toHaveLength(1); + expect(prs[0]).toMatchObject({ number: 3869, state: 'merged', headRefName: 'builder/aspir-3860' }); + }); + + it.skipIf(!hasJq())('is:open excludes that same merged PR', () => { + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/aspir-3860', MERGED_PR_DELETED_BRANCH]]); + const { stdout } = run('pr-search.sh', { + CODEV_SEARCH_QUERY: 'head:builder/aspir-3860 is:open', + }); + expect(JSON.parse(stdout)).toEqual([]); + }); + + it.skipIf(!hasJq())('is:merged keeps it (the afx cleanup query)', () => { + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/aspir-3860', MERGED_PR_DELETED_BRANCH]]); + const { stdout } = run('pr-search.sh', { + CODEV_SEARCH_QUERY: 'head:builder/aspir-3860 is:merged', + }); + expect(JSON.parse(stdout)).toHaveLength(1); + }); + + it.skipIf(!hasJq())('is:merged excludes an OPEN PR (the qualifier means what it says)', () => { + routes([REPO_ROUTE, ['repos/o/r/pulls/main/builder/air-364', OPEN_PR]]); + const { stdout } = run('pr-search.sh', { + CODEV_SEARCH_QUERY: 'head:builder/air-364 is:merged', + }); + expect(JSON.parse(stdout)).toEqual([]); + }); + + it.skipIf(!hasJq())('an issue number searches the cheap index, then resolves refs', () => { + const index = JSON.stringify([ + { number: 3869, title: '[Spec 3860] the kit', body: '' }, + { number: 3800, title: 'unrelated', body: 'mentions 33861 only' }, + ]); + routes([ + REPO_ROUTE, + ['repos/o/r/issues\\?type=pulls*', index], + ['repos/o/r/pulls/3869', MERGED_PR_DELETED_BRANCH], + ]); + const { stdout } = run('pr-search.sh', { CODEV_SEARCH_QUERY: '3860' }); + const prs = JSON.parse(stdout); + expect(prs).toHaveLength(1); + expect(prs[0]).toMatchObject({ number: 3869, baseRefName: 'main' }); + + // The index endpoint carries the search term; the expensive pulls list is + // never touched. + const asked = endpoints(); + expect(asked.some((e) => e.includes('type=pulls') && e.includes('q=3860'))).toBe(true); + expect(asked.some((e) => /pulls\?.*state=all/.test(e))).toBe(false); + }); + + it.skipIf(!hasJq())('word-bounds the issue number so #3386 never matches #33861', () => { + const index = JSON.stringify([ + { number: 10, title: '[Spec 33861] a longer number', body: 'also 133860' }, + ]); + routes([REPO_ROUTE, ['repos/o/r/issues\\?type=pulls*', index]]); + const { stdout } = run('pr-search.sh', { CODEV_SEARCH_QUERY: '3386' }); + expect(JSON.parse(stdout)).toEqual([]); + }); + + it.skipIf(!hasJq())('orders open PRs before merged ones, so prs[0] is the live PR', () => { + const index = JSON.stringify([ + { number: 3869, title: 'old merged for #42', body: '' }, + { number: 3855, title: 'live one for #42', body: '' }, + ]); + routes([ + REPO_ROUTE, + ['repos/o/r/issues\\?type=pulls*', index], + ['repos/o/r/pulls/3869', MERGED_PR_DELETED_BRANCH], + ['repos/o/r/pulls/3855', OPEN_PR], + ]); + const { stdout } = run('pr-search.sh', { CODEV_SEARCH_QUERY: '42' }); + const prs = JSON.parse(stdout); + expect(prs.map((p: { number: number }) => p.number)).toEqual([3855, 3869]); + expect(prs[0].state).toBe('open'); + }); + + it.skipIf(!hasJq())('returns [] rather than guessing at a query it does not understand', () => { + routes([REPO_ROUTE]); + const { stdout, status, stderr } = run('pr-search.sh', { + CODEV_SEARCH_QUERY: 'author:someone sort:updated', + }); + expect(status).toBe(0); + expect(JSON.parse(stdout)).toEqual([]); + expect(stderr).toMatch(/does not understand the query/); + }); + }); + + // ------------------------------------------------------------------------- + // pr-diff + // ------------------------------------------------------------------------- + + describe('pr-diff', () => { + const DIFF = 'diff --git a/x.ts b/x.ts\n--- a/x.ts\n+++ b/x.ts\n@@ -1 +1 @@\n-a\n+b'; + + it('returns the raw diff from the .diff endpoint', () => { + routes([REPO_ROUTE, ['repos/o/r/pulls/7.diff', DIFF]]); + const { stdout, status } = run('pr-diff.sh', { CODEV_PR_NUMBER: '7' }); + expect(status).toBe(0); + expect(stdout.trimEnd()).toBe(DIFF); + }); + + it.skipIf(!hasJq())('name-only emits bare paths, one per line, like gh pr diff --name-only', () => { + const files = JSON.stringify([ + { filename: 'apps/web/a.ts' }, + { filename: 'packages/db/b.ts' }, + ]); + routes([REPO_ROUTE, ['repos/o/r/pulls/7/files*', files]]); + const { stdout, status } = run('pr-diff.sh', { + CODEV_PR_NUMBER: '7', + CODEV_DIFF_NAME_ONLY: '1', + }); + expect(status).toBe(0); + expect(stdout.trim().split('\n')).toEqual(['apps/web/a.ts', 'packages/db/b.ts']); + }); + + it.skipIf(!hasJq())('errors instead of emitting Gitea\'s 404 body as if it were a diff', () => { + // A model handed this would review the error page as the change. + routes([REPO_ROUTE]); + const { stdout, status, stderr } = run('pr-diff.sh', { CODEV_PR_NUMBER: '999999' }); + expect(status).not.toBe(0); + expect(stdout).not.toMatch(/couldn't be found/); + expect(stderr).toMatch(/not found/); + }); + + it.skipIf(!hasJq())('name-only names the PR on a 404 instead of leaking a jq error', () => { + // The full-diff 404 path was tested and the name-only one was not, so this + // was broken: the error body reached `tea_api_paged`, whose `jq -s 'add'` + // cannot add an object to an array, and the walk died on + // `jq: error … array ([]) and object ({...}) cannot be added` before the + // script's own classification could run. Non-zero either way, so never a + // wrong answer — but the operator lost the sentence naming the PR. + // Raised by the claude review lane and reproduced against live Forgejo. + routes([REPO_ROUTE]); + const { status, stdout, stderr } = run('pr-diff.sh', { + CODEV_PR_NUMBER: '999999', + CODEV_DIFF_NAME_ONLY: '1', + }); + expect(status).not.toBe(0); + expect(stdout.trim()).toBe(''); + expect(stderr).toMatch(/PR #999999 not found/); + expect(stderr).not.toMatch(/cannot be added/); + }); + + it('requires CODEV_PR_NUMBER', () => { + routes([REPO_ROUTE]); + const { status, stderr } = run('pr-diff.sh', {}); + expect(status).toBe(2); + expect(stderr).toMatch(/CODEV_PR_NUMBER is required/); + }); + }); + + // ------------------------------------------------------------------------- + // recently-merged + // ------------------------------------------------------------------------- + + describe('recently-merged', () => { + function indexOf(entries: Array<{ number: number; mergedAt: string | null }>): string { + return JSON.stringify( + entries.map((e) => ({ + number: e.number, + title: `PR ${e.number}`, + body: 'b', + created_at: '2026-08-01T00:00:00Z', + html_url: `https://forge.example.com/o/r/pulls/${e.number}`, + pull_request: e.mergedAt === null ? { merged_at: null } : { merged_at: e.mergedAt }, + })), + ); + } + + it.skipIf(!hasJq())('reads the cheap index and never pages the pulls list', () => { + routes([ + REPO_ROUTE, + ['repos/o/r/issues\\?type=pulls*', indexOf([{ number: 3869, mergedAt: '2026-08-21T03:12:02Z' }])], + ['repos/o/r/pulls/3869', MERGED_PR_DELETED_BRANCH], + ]); + const { stdout, status } = run('recently-merged.sh', { + CODEV_SINCE_DATE: '2026-08-20T00:00:00Z', + }); + expect(status).toBe(0); + expect(JSON.parse(stdout)).toEqual([ + { + number: 3869, + title: 'PR 3869', + url: 'https://forge.example.com/o/r/pulls/3869', + body: 'b', + createdAt: '2026-08-01T00:00:00Z', + mergedAt: '2026-08-21T03:12:02Z', + headRefName: 'builder/aspir-3860', + }, + ]); + // `state=closed` on the ISSUES index is fine and cheap. What must never + // appear is a walk of the pulls list, which is the ~26-minute path. + expect(endpoints().some((e) => /^repos\/o\/r\/pulls\?/.test(e))).toBe(false); + }); + + it.skipIf(!hasJq())('drops closed-not-merged pulls', () => { + routes([ + REPO_ROUTE, + ['repos/o/r/issues\\?type=pulls*', indexOf([{ number: 3800, mergedAt: null }])], + ]); + const { stdout } = run('recently-merged.sh', { CODEV_SINCE_DATE: '2026-08-20T00:00:00Z' }); + expect(JSON.parse(stdout)).toEqual([]); + }); + + it.skipIf(!hasJq())('drops PRs updated in the window but merged before it', () => { + // `since` filters on updated_at, so an old PR commented on yesterday comes + // back from the index. merged_at is the field that decides. + routes([ + REPO_ROUTE, + ['repos/o/r/issues\\?type=pulls*', indexOf([{ number: 3000, mergedAt: '2026-01-01T00:00:00Z' }])], + ]); + const { stdout } = run('recently-merged.sh', { CODEV_SINCE_DATE: '2026-08-20T00:00:00Z' }); + expect(JSON.parse(stdout)).toEqual([]); + }); + + it.skipIf(!hasJq())('an empty window is [] with status 0 — NOT a truncation', () => { + routes([REPO_ROUTE, ['repos/o/r/issues\\?type=pulls*', '[]']]); + const { stdout, status } = run('recently-merged.sh', { + CODEV_SINCE_DATE: '2026-08-20T00:00:00Z', + }); + expect(status).toBe(0); + expect(JSON.parse(stdout)).toEqual([]); + }); + + it.skipIf(!hasJq())('exits 3 with EMPTY stdout when the window exceeds the ceiling', () => { + // The distinction the whole script exists for: "nothing merged" and "I + // stopped looking" must not be spelled the same way. A partial list is + // indistinguishable from a complete one once printed, so none is printed. + const many = Array.from({ length: 5 }, (_, i) => ({ + number: 100 + i, + mergedAt: '2026-08-21T00:00:00Z', + })); + routes([REPO_ROUTE, ['repos/o/r/issues\\?type=pulls*', indexOf(many)]]); + const { stdout, status, stderr } = run('recently-merged.sh', { + CODEV_SINCE_DATE: '2026-08-20T00:00:00Z', + CODEV_FORGE_MERGED_MAX: '2', + }); + expect(status).toBe(3); + expect(stdout.trim()).toBe(''); + expect(stderr).toMatch(/over the 2 ceiling/); + }); + + it.skipIf(!hasJq())('bounds itself to a default window when given no date, and says so', () => { + routes([REPO_ROUTE, ['repos/o/r/issues\\?type=pulls*', '[]']]); + const { status, stderr } = run('recently-merged.sh', {}); + expect(status).toBe(0); + expect(stderr).toMatch(/limiting to the last 7 days/); + // The window reaches the server as a `since` filter — the bound is not + // client-side discarding, which is what made the old script slow. + expect(endpoints().some((e) => e.includes('since='))).toBe(true); + }); + + it.skipIf(!hasJq())('names the repository on an error body instead of leaking a jq error', () => { + routes([REPO_ROUTE]); // no issues route -> the fake tea answers 404 + const { status, stderr } = run('recently-merged.sh', { + CODEV_SINCE_DATE: '2026-08-20T00:00:00Z', + }); + expect(status).toBe(1); + expect(stderr).toMatch(/no readable pull index/); + expect(stderr).not.toMatch(/cannot be added/); + }); + + it('rejects an unparseable CODEV_SINCE_DATE instead of silently widening', () => { + routes([REPO_ROUTE]); + const { status, stderr } = run('recently-merged.sh', { CODEV_SINCE_DATE: 'last tuesday' }); + expect(status).toBe(2); + expect(stderr).toMatch(/expected YYYY-MM-DD or RFC3339/); + }); + }); +}); + +describe('#12 — the scripts pin their own commands, not their comments', () => { + /** + * These assertions are anchored to code lines. #1331's review caught the + * opposite: assertions written as `content.toContain('--state all')` passed + * against the explanatory comment that quoted the flag, so they stayed green + * with the flag deleted from the command. + */ + it('pr-exists reaches for the base/head endpoint and nothing that lists', () => { + const src = fs.readFileSync(path.join(gitea, 'pr-exists.sh'), 'utf-8'); + const code = src.split('\n').filter((l) => !l.trim().startsWith('#')); + expect(code.some((l) => /gitea_api "repos\/\$\{REPO\}\/pulls\/\$\{BASE\}\//.test(l))).toBe(true); + expect(code.some((l) => l.includes('tea_api_paged'))).toBe(false); + expect(code.some((l) => l.includes('state=all'))).toBe(false); + }); + + it('recently-merged reads the issues index, not the pulls list', () => { + const src = fs.readFileSync(path.join(gitea, 'recently-merged.sh'), 'utf-8'); + const code = src.split('\n').filter((l) => !l.trim().startsWith('#')); + expect(code.some((l) => /tea_api_paged "repos\/\$\{REPO\}\/issues"/.test(l))).toBe(true); + expect(code.some((l) => /tea_api_paged "repos\/\$\{REPO\}\/pulls"/.test(l))).toBe(false); + expect(code.some((l) => l.includes('state=closed') && l.includes('type=pulls'))).toBe(true); + }); + + it('the two forge SKILL.md twins are byte-identical', () => { + // CLAUDE.md's standing rule, and the plan's Test Plan asked for this pin. + // The files WERE identical; nothing stopped them drifting. Raised by the + // claude review lane as a plan item that shipped unimplemented. + const claude = fs.readFileSync(path.join(repoRoot, '.claude/skills/forge/SKILL.md')); + const codex = fs.readFileSync(path.join(repoRoot, '.codex/skills/forge/SKILL.md')); + expect(codex.equals(claude), '.claude and .codex forge skills have drifted').toBe(true); + }); + + it('afx spawn asks pr-search for OPEN PRs explicitly, not by relying on a default', () => { + // #1331's review: without this, every re-spawn of an issue that ever had a + // merged PR aborts with a factually wrong "Found N open PR(s)". + const src = fs.readFileSync( + path.join(codevPkgRoot, 'src', 'agent-farm', 'commands', 'spawn-worktree.ts'), + 'utf-8', + ); + expect(src).toMatch(/CODEV_SEARCH_QUERY:\s*`in:body #\$\{issueNumber\} is:open`/); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts b/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts index e52ce1766..710a1dfdd 100644 --- a/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts @@ -815,6 +815,24 @@ describe('spawn-worktree', () => { ); }); + it('asks pr-search for OPEN PRs explicitly, not by relying on its default', async () => { + // #12 / upstream #1331's review. pr-search now spans every state, so a PR + // that ever referenced this issue and has since merged would otherwise + // come back here and abort the spawn with a factually wrong "Found N open + // PR(s)" — killing every re-spawn, every follow-up to a partial fix and + // every retry after a closed PR. The mock in this file cannot catch a + // wrong query on its own, so the qualifier is asserted directly. + const { existsSync } = await import('node:fs'); + vi.mocked(existsSync).mockReturnValueOnce(false); + executeForgeCommandMock.mockResolvedValueOnce([]); + + await checkBugfixCollisions(42, '/tmp/wt', baseIssue, false); + + const call = executeForgeCommandMock.mock.calls.find((c) => c[0] === 'pr-search'); + expect(call, 'checkBugfixCollisions did not consult pr-search').toBeDefined(); + expect((call![1] as Record).CODEV_SEARCH_QUERY).toBe('in:body #42 is:open'); + }); + it('warns when issue is already closed', async () => { const { existsSync } = await import('node:fs'); vi.mocked(existsSync).mockReturnValueOnce(false); diff --git a/packages/codev/src/agent-farm/commands/spawn-worktree.ts b/packages/codev/src/agent-farm/commands/spawn-worktree.ts index fc525167c..8a9cd25da 100644 --- a/packages/codev/src/agent-farm/commands/spawn-worktree.ts +++ b/packages/codev/src/agent-farm/commands/spawn-worktree.ts @@ -587,10 +587,18 @@ export async function checkBugfixCollisions( } } - // 3. Check for open PRs referencing this issue via pr-search concept + // 3. Check for open PRs referencing this issue via pr-search concept. + // + // `is:open` is explicit and load-bearing. This check used to rely on + // pr-search defaulting to open-only, which was never stated anywhere. Once + // pr-search searches every state — which it must, so that a merged PR is + // findable after the fact (#759/#1331) — an implicit default turns this into + // "did this issue EVER have a PR", and every re-spawn, every follow-up to a + // partial fix and every retry after a closed PR aborts with a factually wrong + // "Found N open PR(s)". Say what we mean instead of leaning on a default. try { const result = await executeForgeCommand('pr-search', { - CODEV_SEARCH_QUERY: `in:body #${issueNumber}`, + CODEV_SEARCH_QUERY: `in:body #${issueNumber} is:open`, }, { forgeConfig }); if (result && Array.isArray(result) && result.length > 0) { const openPRs = result as Array<{ number: number; title?: string; headRefName?: string }>; diff --git a/packages/codev/src/commands/porch/__tests__/pir-12-pr-exists-null-vs-false.test.ts b/packages/codev/src/commands/porch/__tests__/pir-12-pr-exists-null-vs-false.test.ts new file mode 100644 index 000000000..b37facc19 --- /dev/null +++ b/packages/codev/src/commands/porch/__tests__/pir-12-pr-exists-null-vs-false.test.ts @@ -0,0 +1,77 @@ +/** + * PIR #12 — `null` from the pr-exists concept is not `false`. + * + * `executeForgeCommand` returns `null` when the command failed, timed out (it + * imposes a 30s ceiling on every concept), was disabled for the provider, or + * printed something unparseable. None of those mean "there is no PR". The + * check used to report all of them as a plain failed check carrying + * `output: "null"`, which reads as "no PR found" — so a builder at the pr gate + * would be told its PR does not exist and go create a duplicate. + * + * This mattered concretely: before this PR the gitea `pr-exists` script took + * ~17 minutes against a real Forgejo, so the 30s ceiling fired on every run and + * `null` was the *normal* outcome on that provider. + * + * The gitea script is fixed, but the misreading was general — any provider, any + * cause. Raised by the claude review lane as a behaviour change with no test. + * + * These tests mock the forge layer, which is why they live in their own file: + * `checks.test.ts` deliberately avoids that mock so its other cases exercise + * the real dispatcher. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const executeForgeCommandMock = vi.fn(); +vi.mock('../../../lib/forge.js', () => ({ + executeForgeCommand: (...args: unknown[]) => executeForgeCommandMock(...args), + loadForgeConfig: () => ({ provider: 'gitea' }), + getForgeCommand: () => '/scripts/forge/gitea/pr-exists.sh', + isConceptDisabled: () => false, +})); + +const { runPhaseChecks } = await import('../checks.js'); + +describe('#12 — pr_exists distinguishes "no PR" from "could not answer"', () => { + // A real git checkout, not tmpdir: the check reads the current branch with + // `git branch --show-current` before it ever calls the concept, and that + // throws outside a repository — masking every assertion below. + const cwd = process.cwd(); + const env = { PROJECT_ID: '12', PROJECT_TITLE: 'test-project' }; + const checks = { pr_exists: 'unused — the concept is intercepted' }; + + beforeEach(() => { + executeForgeCommandMock.mockReset(); + }); + + it('passes when the concept answers true', async () => { + executeForgeCommandMock.mockResolvedValue('true'); + const [result] = await runPhaseChecks(checks, cwd, env); + expect(result.passed).toBe(true); + }); + + it('fails with output "false" when the concept answers false', async () => { + executeForgeCommandMock.mockResolvedValue('false'); + const [result] = await runPhaseChecks(checks, cwd, env); + expect(result.passed).toBe(false); + expect(result.output).toBe('false'); + // A real answer carries no error — the check failed because there is no PR, + // which is a legitimate, actionable state. + expect(result.error).toBeUndefined(); + }); + + it('fails with a DISTINCT error when the concept returns null', async () => { + executeForgeCommandMock.mockResolvedValue(null); + const [result] = await runPhaseChecks(checks, cwd, env); + + expect(result.passed).toBe(false); + // The load-bearing assertion: it must not present as the string "null", + // which is what made this indistinguishable from an answer of false. + expect(result.output).not.toBe('null'); + expect(result.error).toBeDefined(); + expect(result.error).toMatch(/no usable answer/); + expect(result.error).toMatch(/failed, timed out, or is disabled/); + expect(result.error, 'the error must deny the "no PR exists" reading outright') + .toMatch(/NOT the same as "no PR exists"/); + }); +}); diff --git a/packages/codev/src/commands/porch/checks.ts b/packages/codev/src/commands/porch/checks.ts index bad9fb959..732043898 100644 --- a/packages/codev/src/commands/porch/checks.ts +++ b/packages/codev/src/commands/porch/checks.ts @@ -361,6 +361,23 @@ async function runPrExistsViaConcept( CODEV_BRANCH_NAME: branchName.trim(), }, { cwd, workspaceRoot: cwd }); + // `null` is not `false`. executeForgeCommand returns null when the command + // failed, timed out (it imposes a 30s ceiling), was disabled, or printed + // something unparseable — none of which mean "there is no PR". Reporting it + // as a plain failed check with `output: "null"` reads as "no PR found" and + // sends the builder off to create a duplicate. Say which it was. + if (result === null) { + return { + name, + command: forgeCmd, + passed: false, + error: 'the pr-exists forge concept returned no usable answer — it failed, timed out, ' + + 'or is disabled for this provider. This is NOT the same as "no PR exists"; ' + + 'run the concept command directly to see its stderr.', + duration_ms: Date.now() - startTime, + }; + } + // The concept returns a truthy value (string "true", boolean true, or number > 0) const passed = result === true || result === 'true' || (typeof result === 'number' && result > 0); diff --git a/packages/codev/src/lib/forge-contracts.ts b/packages/codev/src/lib/forge-contracts.ts index ae6b39af6..8a8e3e444 100644 --- a/packages/codev/src/lib/forge-contracts.ts +++ b/packages/codev/src/lib/forge-contracts.ts @@ -113,6 +113,23 @@ export type RecentlyMergedResult = MergedPrItem[]; export interface PrSearchItem { number: number; headRefName: string; + /** + * The PR's base branch. `consult`'s architect path (findPRForIssue) reads it + * to compute a merge-base against the PR's *actual* base rather than the + * repo's default branch, and warns and falls back when it is absent — so a + * concept that can supply it should. + */ + baseRefName?: string; + /** + * `open`, `merged`, or `closed` (closed without merging), when the concept + * normalises it. Callers read `prs[0]`, and a search spanning every state + * (which is the default since #1331/#759 — a merged PR must be findable) can + * otherwise hand them a stale PR with no way to tell. Concepts that emit this + * order their results open-first. + */ + state?: 'open' | 'merged' | 'closed'; + title?: string; + url?: string; } /** Output of the `pr-search` concept command. */ diff --git a/packages/codev/src/lib/forge.ts b/packages/codev/src/lib/forge.ts index 0af6fa153..611df59fa 100644 --- a/packages/codev/src/lib/forge.ts +++ b/packages/codev/src/lib/forge.ts @@ -126,7 +126,12 @@ function getProviderPresets(): Record> { _providerPresets = { github: getDefaultCommands(), gitlab: buildPresetFromScripts('gitlab', ['team-activity', 'on-it-timestamps']), - gitea: buildPresetFromScripts('gitea', ['team-activity', 'on-it-timestamps', 'pr-search', 'pr-diff']), + // pr-search and pr-diff were disabled here until #12 shipped gitea scripts + // for them. team-activity and on-it-timestamps stay disabled and are not + // coming: both are `gh api graphql` pass-throughs and Forgejo has no + // GraphQL. Their callers say so out loud rather than degrading quietly — + // see fetchOnItTimestamps and fetchTeamGitHubData. + gitea: buildPresetFromScripts('gitea', ['team-activity', 'on-it-timestamps']), // pr-create is explicitly disabled (not just "no script") — Linear has no PR // concept of its own, and without this it silently falls through to the // github default (`gh pr create`) instead of failing loudly. That's the @@ -304,6 +309,56 @@ export function isConceptDisabled( return concept in forgeConfig && forgeConfig[concept] === null; } +/** + * Explain, in one sentence, why a concept has no command — for a human reading + * a terminal, not for a log file. + * + * A concept can be unavailable two ways that look identical to a caller and are + * not identical to a user: the project turned it off, or the forge provider + * never had it. Naming the provider is the difference between "why is this + * panel empty" and "right, Forgejo has no GraphQL". + */ +export function describeUnavailableConcept( + concept: string, + forgeConfig?: ForgeConfig | null, +): string { + const provider = forgeConfig?.provider; + if (forgeConfig && concept !== 'provider' && concept in forgeConfig && forgeConfig[concept] === null) { + return `the \`${concept}\` forge concept is disabled in .codev/config.json`; + } + if (provider) { + return `the \`${concept}\` forge concept is not available for provider "${provider}"`; + } + return `the \`${concept}\` forge concept has no command configured`; +} + +/** Concepts already warned about, so a per-poll code path warns once per process. */ +const _warnedConcepts = new Set(); + +/** + * Warn on stderr, once per concept per process, that a concept is unavailable + * and what that costs. + * + * Once per process rather than once per call: these sit on polled paths (the + * overview refreshes every 30s), and a warning printed on every poll is noise + * that trains people to ignore it — which is the same silence it was meant to + * break, arrived at by a different road. + */ +export function warnConceptUnavailable( + concept: string, + forgeConfig: ForgeConfig | null | undefined, + consequence: string, +): void { + if (_warnedConcepts.has(concept)) return; + _warnedConcepts.add(concept); + console.error(`Warning: ${describeUnavailableConcept(concept, forgeConfig)} — ${consequence}.`); +} + +/** Test seam: forget which concepts have been warned about. */ +export function _resetConceptWarnings(): void { + _warnedConcepts.clear(); +} + // ============================================================================= // Execution // ============================================================================= diff --git a/packages/codev/src/lib/github.ts b/packages/codev/src/lib/github.ts index 2797e2ffe..86255e42c 100644 --- a/packages/codev/src/lib/github.ts +++ b/packages/codev/src/lib/github.ts @@ -10,7 +10,13 @@ */ import { UNCATEGORIZED_AREA } from '@cluesmith/codev-sdk/constants'; -import { executeForgeCommand, type ForgeConfig } from './forge.js'; +import { + executeForgeCommand, + getForgeCommand, + isConceptDisabled, + warnConceptUnavailable, + type ForgeConfig, +} from './forge.js'; import { getRepoInfo } from './team-github.js'; import type { IssueViewResult, PrListItem, PrViewResult, IssueListItem } from './forge-contracts.js'; @@ -335,6 +341,21 @@ export async function fetchOnItTimestamps( const unique = [...new Set(issueIds)]; + // A concept disabled by a PROVIDER PRESET rather than by user config used to + // fall through to the GraphQL path below, call the concept, get null back and + // `continue` — an empty map, nothing on stderr, analytics quietly missing its + // wall-clock baseline. `forgeConfig?.[...]` cannot see a preset: for a gitea + // repo the key is absent from user config and null in the preset. Ask the + // resolver, which knows about both. + if (isConceptDisabled('on-it-timestamps', forgeConfig) || getForgeCommand('on-it-timestamps', forgeConfig) === null) { + warnConceptUnavailable( + 'on-it-timestamps', + forgeConfig, + '"On it" timestamps are unavailable, so analytics falls back to PR createdAt for wall-clock time', + ); + return result; + } + // Check if a custom (non-default) on-it-timestamps command is configured. // Custom commands receive CODEV_ISSUE_NUMBERS and return a simple JSON map. const customCmd = forgeConfig?.['on-it-timestamps']; diff --git a/packages/codev/src/lib/team-github.ts b/packages/codev/src/lib/team-github.ts index 027d57da8..b44e82e88 100644 --- a/packages/codev/src/lib/team-github.ts +++ b/packages/codev/src/lib/team-github.ts @@ -11,7 +11,13 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { isValidGitHubHandle } from './team.js'; import type { TeamMember } from './team.js'; -import { executeForgeCommand, type ForgeConfig } from './forge.js'; +import { + executeForgeCommand, + describeUnavailableConcept, + getForgeCommand, + isConceptDisabled, + type ForgeConfig, +} from './forge.js'; const execFileAsync = promisify(execFile); @@ -326,6 +332,17 @@ export async function fetchTeamGitHubData( return { data: new Map(), error: 'Could not determine repository. Configure forge concepts in .codev/config.json.' }; } + // Distinguish "this forge cannot do it" from "the call came back empty". + // Both used to surface as "returned no data", which reads as a transient + // failure and invites a retry that can never succeed: team-activity is a + // batched `gh api graphql` query and Forgejo has no GraphQL at all. + if (isConceptDisabled('team-activity', forgeConfig) || getForgeCommand('team-activity', forgeConfig) === null) { + return { + data: new Map(), + error: `${describeUnavailableConcept('team-activity', forgeConfig)} — team forge activity cannot be reported`, + }; + } + const query = buildTeamGraphQLQuery(validMembers, repo.owner, repo.name); try {