Uh oh!
There was an error while loading. Please reload this page.
[Bugfix #1137] Fix gitea forge preset against the real tea CLI - #1146
Conversation
waleedkadous
left a comment
There was a problem hiding this comment.
Excellent work — thank you for the disciplined re-do, and apologies for the review latency. This is what a model bugfix PR looks like: every tea-CLI deficiency documented in-script with reasoning, the REST passthrough returning exactly the shape forge-contracts.ts expects, and a genuinely well-built regression suite (fake tea on PATH serving captured REST fixtures, the real scripts executed, contract-shape assertions, the comments-as-int crash and null-login team reviewers both covered). We verified every output mapping field-by-field against the contracts — all conform — and the switch from string-interpolated jq to --arg in pr-exists is a quiet security improvement worth crediting.
One substantive question before merge, and two optional polish items:
1. Pagination cap (the one we'd like addressed or answered). Gitea servers cap page size at max_response_items (default 50), so ?limit=200 likely returns 50 items with no client-side pagination in the raw passthrough. That means pr-exists?state=all can false-negative for a branch whose PR isn't in the most recent ~50 (which would block a porch pr_exists gate), and recently-merged (previously --limit 1000) can miss on a busy repo. A pagination loop (page=1..N until a short page) would settle it — or at minimum a comment documenting the server-side cap and the false-negative window, so the next debugger isn't blind. Happy with either; we'd just like the behavior to be chosen rather than inherited.
2. (Polish, optional) With no origin remote or an unusual URL, REPO silently becomes empty/garbage and tea api "repos//…" fails with a confusing 404. An explicit [ -n "$REPO" ] || { echo "…set CODEV_REPO" >&2; exit 1; } naming the remedy would fit this repo's fail-fast convention — ideally factored once since the derivation appears in five scripts.
3. (Polish, optional) A failed comments fetch silently yields comments: [] — indistinguishable from "no comments" for consumers reading issue discussion. A stderr warning on the degraded path would keep the graceful behavior while leaving a trace.
Verdict: approve once item 1 is addressed (fix or documented caveat — your choice). Items 2–3 are welcome in this PR or a follow-up, contributor's choice.
…ast, warn on degraded comments Addresses PR cluesmith#1146 review feedback: 1. Pagination (blocking). Gitea caps list responses at max_response_items (default 50), so the raw `&limit=200` passthrough silently truncated — pr-exists could false-negative a PR beyond the first ~50 (blocking a porch pr_exists gate) and recently-merged could miss on a busy repo. New shared helper `_lib.sh#tea_api_paged` walks page=1..N at limit=50, concatenates the arrays, and stops on a short/empty page with a hard 100-page ceiling. Chosen behavior: paginates, ceiling 100 pages. Wired into pr-exists, pr-list, recently-merged; output shape unchanged (same jq normalizers). 2. REPO derivation, fail-fast + factored. The CODEV_REPO/origin-derivation was duplicated in five scripts. Factored into `_lib.sh#gitea_repo`, sourced by issue-view, pr-exists, pr-list, pr-view, recently-merged. It now validates the result is a clean owner/repo and, if not, prints a stderr message naming CODEV_REPO as the remedy and exits non-zero (was a confusing `repos//…` 404). POSIX sh, $0-relative source; not a forge concept (KNOWN_CONCEPTS allowlist). 3. Degraded comments warn. issue-view still degrades a failed comments fetch to [], but now writes a stderr warning so it's distinguishable from a genuinely uncommented issue. stdout stays pure JSON (parsed by forge.ts). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pseudoseed
commented
Aug 5, 2026
Thanks for the thorough review — all three items addressed in 1. Pagination — fixed (real page loop, not a comment). You're right that 2. REPO fail-fast, factored once — done. The 3. Degraded comments warn — done. Full suite green in the worktree: 3449 passed | 48 skipped, 0 failures ( |
pseudoseed
commented
Aug 14, 2026
@waleedkadous let me know if there's anything else that needs to be addressed with this one :) |
Verified against a real Forgejo + |
| concept | result |
|---|---|
user-identity | FAILIncorrect Usage: flag provided but not defined: -output |
issue-view | FAILjq: Cannot index array with string "html_url" |
pr-list | FAIL, exit 0 — prints Error: invalid field 'description' and still returns success |
recently-merged | FAIL, exit 0 — same |
pr-view | returns a list, not the requested PR |
issue-list, issue-search, pr-exists, recently-closed, auth-status | OK |
The two exit-0 cases are the nastiest: a caller checking the exit status sees success and gets an error string where JSON should be.
After — this branch, same environment
| concept | result |
|---|---|
user-identity | OK — user |
issue-view | OK — object with title, body, state, url, comments[] |
pr-list | OK — normalized PrListItem[] |
pr-view | OK — single PR object, correct one |
pr-exists | OK — true |
recently-merged | OK — merged-only, correct merged_at ordering |
All six previously-broken concepts now work. No regressions in the five that already worked.
Two notes
The comments-as-int catch is real and would have bitten immediately. Gitea returns comments as an integer count on the issue object; our issue-view on the released version failed exactly there. The second call for the comments array is necessary, not defensive.
One nearly-false report from me, worth stating so nobody repeats it. My first run of this branch's issue-view failed with Cannot index array with string "title". That was my harness, not your code — I had exported CODEV_ISSUE_NUMBER where the contract is CODEV_ISSUE_ID, so the path resolved to the issue list endpoint. With the correct variable it works. Flagging it because the failure mode is plausible-looking and someone else testing this could draw the wrong conclusion.
Unrelated gap this surfaced
pr-create is not a forge concept at all, so gh pr create stays hardcoded in the skeleton prompts (porch/prompts/pr.md, protocols/{air,spir,pir,bugfix,maintain}/…). That means a Gitea/Forgejo user still needs a gh shim on PATH no matter how complete this preset becomes. Not this PR's problem — filing separately — but relevant if anyone assumes a working gitea preset makes gh unnecessary.
Happy to re-run against any further revisions.
The gitea preset invoked `tea <entity> list/view/whoami/comment`, whose flattened `--fields` output (or missing flags/subcommands) doesn't match the Gitea REST shape that forge-contracts.ts and the jq normalizers assume. Route the read concepts through `tea api`, the raw REST passthrough that returns exactly that shape: - user-identity: `tea api user | jq .login` (`tea whoami` has no --output json) - pr-view: `tea api repos/<repo>/pulls/N` → PrViewResult - pr-list: `tea api repos/<repo>/pulls?state=open` → PrListItem[] (now also populates real reviewRequests/isDraft/body) - pr-exists: `tea api repos/<repo>/pulls?state=all` with nested .head.ref/.merged - issue-view: `tea api repos/<repo>/issues/N` + a second call for the comments ARRAY (Gitea's issue object reports `comments` as an int count, which would crash consumers' `.comments.filter(...)`) - recently-merged: `tea api repos/<repo>/pulls?state=closed`, filter .merged, using the real .merged_at - issue-comment: `tea comments add` (`tea issues` has no `comment` subcommand) `tea api` needs an explicit owner/repo path segment (unlike `tea <entity>`, which auto-detects it from the local git remote), and most concepts are invoked without CODEV_REPO set, so each api-based script derives owner/repo from the origin remote, honoring CODEV_REPO when present. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stubs a fake `tea` on PATH answering `api <endpoint>` with captured Gitea REST fixtures (tea isn't in CI, per cluesmith#920), points the scripts at a throwaway repo with a gitea remote, runs each real script, and asserts the normalized output conforms to forge-contracts.ts — incl. comments-as-array, merged-only filtering, open/merged/closed pr-exists cases, and CODEV_REPO override. Also updates the cluesmith#568 pr-exists assertion for gitea to match the new `state=all` query param (was `--state all` flag). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ast, warn on degraded comments Addresses PR cluesmith#1146 review feedback: 1. Pagination (blocking). Gitea caps list responses at max_response_items (default 50), so the raw `&limit=200` passthrough silently truncated — pr-exists could false-negative a PR beyond the first ~50 (blocking a porch pr_exists gate) and recently-merged could miss on a busy repo. New shared helper `_lib.sh#tea_api_paged` walks page=1..N at limit=50, concatenates the arrays, and stops on a short/empty page with a hard 100-page ceiling. Chosen behavior: paginates, ceiling 100 pages. Wired into pr-exists, pr-list, recently-merged; output shape unchanged (same jq normalizers). 2. REPO derivation, fail-fast + factored. The CODEV_REPO/origin-derivation was duplicated in five scripts. Factored into `_lib.sh#gitea_repo`, sourced by issue-view, pr-exists, pr-list, pr-view, recently-merged. It now validates the result is a clean owner/repo and, if not, prints a stderr message naming CODEV_REPO as the remedy and exits non-zero (was a confusing `repos//…` 404). POSIX sh, $0-relative source; not a forge concept (KNOWN_CONCEPTS allowlist). 3. Degraded comments warn. issue-view still degrades a failed comments fetch to [], but now writes a stderr warning so it's distinguishable from a genuinely uncommented issue. stdout stays pure JSON (parsed by forge.ts). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3c2e3c2 to
86b82acCompare…luesmith#1458 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rop the lookup The gitea `pr-create` ran `tea pulls create` (whose output is a rendered, ANSI-decorated view, not parseable) and then searched for the PR it had just made with `tea pulls list --limit 200`. That search was built on a disproven assumption. cluesmith#1146 established, and this change re-confirmed against live Forgejo 15.0.2, that Gitea caps every list response at the server's `max_response_items` — default 50. `settings/api` reports 50, and a `?limit=200` request returns exactly 50 items where paging at 50 returns 53. So `--limit 200` silently truncates: on a busy repo the just-created PR falls off the first page, and pr-create reported created the PR but could not find an open pull for head '<branch>' and exited 1 for a PR that exists — inviting a duplicate retry at the single most important write in the protocol. Rather than paginate the lookup, remove it. `tea api -X POST repos/{owner}/{repo}/pulls` RETURNS the created PR — `number` and `html_url` — in its response body, so there is nothing to search, nothing to race, and nothing to truncate. It also drops the `<user>:<branch>` head-matching heuristic: the API resolves an owner-qualified head itself. Live verification against tea 0.14.2 + Forgejo 15.0.2 turned up three defects in the obvious version of that change. Each is the same bug class as cluesmith#1455 itself — an operation accepted and then silently not performed — so each is handled in code, not left as a caveat. 1. `tea api` EXITS 0 on HTTP errors, printing the error body. Since the whole change replaces a lookup with a single call, trusting that exit code would reintroduce cluesmith#1455's silent success inside the fix for it: a 404 or 422 would be reported as a created PR. The response is therefore asserted to BE a PR object — an object carrying a numeric `number` AND a non-empty browser URL — and anything else fails loudly with the response body. Pinned by tests that feed an error object, an array, a string-typed `number`, a numberless object, `null` and an empty body, all at exit 0. The one case where `number` is present but the URL is not gets its own message: the PR WAS created, so it names the number and says not to retry. Reading that as "nothing happened" is how duplicates get opened. 2. `base` is REQUIRED by the API — it answers `[Base]: Required` — where `tea pulls create` defaulted it client-side. Silently posting against the wrong base would be worse than erroring, so an unset CODEV_PR_BASE now resolves the repo's default branch explicitly, and fails with a clear message if that cannot be resolved. 3. `draft: true` in the payload is SILENTLY IGNORED (the response comes back `draft: false`), so CODEV_PR_DRAFT=1 would have been an accepted-and-ignored flag. Gitea marks a draft by a `WIP:` title prefix — exactly what `tea pulls create --draft` does — so that is now implemented, and verified server-side to produce `draft: true`. Also verified live: `{owner}`/`{repo}` are substituted by tea from the repo context, with `--repo owner/name` supplying it when the cwd has no Gitea remote (checked with https and scp-style remotes, and from a GitHub-remote cwd); `url` on the create response is the browser page, so `.html_url // .url` lands the right one in the contract; and the body round-trips byte-identically, being built with `jq --arg` and fed on stdin (`-d @-`) rather than surviving an argv round-trip. The unresolvable-repo case used to surface as a bare `404 page not found`; it now names CODEV_PR_REPO as the remedy, matching the fail-fast ergonomics of `_lib.sh#gitea_repo` in cluesmith#1146 without taking a dependency on that PR — this change stands alone and the two can merge in either order. Tests: the gitea half of the concept suite is rewritten against a `tea api` stub. Every new case fails against the previous script and passes against this one, including an explicit assertion that no `pulls`/`list`/`--limit` call is made at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…luesmith#1458 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…luesmith#1458 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
86b82ac to
fa5a7cbCompare…luesmith#1458 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
amrmelsayed
commented
Aug 17, 2026
Reviewer Integration Review (CMAP-3)Three-lane consultation (Gemini, GPT-5.6 Sol via Codex, Claude Opus) plus an independent architect pass. Every finding below was re-verified by the reviewing architect against the actual source, and the tea-CLI claims against the installed release binary, before being credited. Verdict: REQUEST CHANGES (light). The core fix is correct, honestly documented, and the best-evidenced community PR this repo has received: the Verified solid (no action)
Blocking1. 2. Paginator failure exits 0 with empty stdout, which reads as a false-negative PULLS="$(tea_api_paged "repos/${REPO}/pulls""state=all")"||exit 1
printf'%s'"$PULLS"| jq …This also converts the Strongly recommended in this PR (both small, both introduced here)3. 4. The stop condition trusts that the server honored Maintainer's call: in-PR or filed as follow-ups
Follow-up issues to file after merge (none blocking)
Process notes
Merge authority rests with the maintainer; this review is the reviewing architect's recommendation, not a gate approval. |
amrmelsayed
commented
Aug 17, 2026
Follow-up from the reviewing architect: finding 3 just got smallerMerge order has been ruled by the maintainer: #1458 first, then this PR. That changes the shape of one item in the review above. #1458 ships a #!/bin/sh# Forge concept: pr-exists (Gitea via tea CLI)# forge-executable: teaThe deeper extraction work the review asked for (resolving through sourced-helper calls, plus a test) is not required of you — that burden moved to #1458's mechanism, and a hardening addition to its builtin skip list is being pressed on that PR separately. Everything else stands as written, and none of it waits on #1458: the two blockers ( |
…ktree # Conflicts: # codev/projects/bugfix-1137-gitea-forge-preset-is-broken-a/status.yaml
…n PR cluesmith#1458 1. linear preset now explicitly disables pr-create instead of silently falling through to the github default (`gh pr create`) — the same silent-fallthrough bug class cluesmith#1455 closes, just found by the integration reviewer in a different preset. 2. Add `.` and `source` to extractExecutable's SHELL_BUILTINS, so a script that opens with `. "$(dirname "$0")/_lib.sh"` (the shape sibling PR cluesmith#1146's read scripts use) isn't misreported by `codev doctor` as needing an executable literally named `.`. 3. gitea/pr-create.sh's default-branch resolution named CODEV_PR_BASE as the remedy even when the real failure was an unresolvable repo (GET 404) — the POST path already named CODEV_PR_REPO correctly for the same root cause; the GET path now matches it. Reviewer: amrmelsayed (CMAP integration review, 2026-08-17). Verdict: APPROVE with these three pre-merge recommendations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… cmd, masked pipe failures, sub-limit page cap PR cluesmith#1146 review (2026-08-17, amrmelsayed) — REQUEST_CHANGES, 2 blocking + 1 strongly-recommended item: 1. (blocking) issue-comment.sh called `tea comments add`, which only exists on tea 0.14.2+ and fails on the still-current 0.14.1 release ("No help topic for comments"). Switch to the `tea comment <id> <body>` shorthand, which works on both 0.14.1 and 0.14.2+. 2. (blocking) pr-exists.sh, pr-list.sh, recently-merged.sh piped `tea_api_paged | jq` directly. POSIX sh has no pipefail, so a mid-walk pagination failure (tea_api_paged returns 1) was masked by jq's exit status (0 on empty stdin) — pr-exists in particular would report "false" for a real error instead of failing, silently passing a porch pr_exists gate. Capture the paginator's output into a variable and check its exit status before piping to jq. 3. (strongly recommended) tea_api_paged's stop condition compared each page's item count against the *requested* limit (50). A server whose max_response_items is tuned below that requested limit truncates every page — including non-last pages — to its own cap, so every page looked "short" and the loop broke after page 1. Compare against the size actually observed on page 1 instead. Item 4 (a `# forge-executable: tea` header convention) depends on cluesmith#1458's extractExecutable convention landing first, which hasn't happened — left for a follow-up once cluesmith#1458 merges, per the reviewer's stated merge order. Regression tests added for all three: a 0.14.1-compatible `tea comment` stub, a mid-walk pagination failure fixture exercised by all three paginated scripts, and a sub-50-per-page server-cap fixture across 3 pages proving pr-list keeps walking. Full local suite: 3197 passed, 126 failed (same 67 pre-existing environment-dependent files as the unmodified baseline) — zero regressions, +4 new passing tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sion Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pseudoseed
commented
Aug 21, 2026
Review follow-up (2026-08-20)Addressed both blocking items and the strongly-recommended item from the 2026-08-17 review, pushed as 1. (blocking) 2. (blocking) masked pipe failures in the paginated scripts Fixed by capturing the paginator's output into a variable and checking its exit status before piping to jq: PULLS="$(tea_api_paged "repos/${REPO}/pulls""state=all")"||exit 1
printf'%s'"$PULLS"| jq …3. (strongly recommended, done) sub-limit server cap 4. (deferred, not done in this PR) TestingAdded regression coverage for all three fixes in
Full local suite: 3197 passed, 126 failed (67 files) — the same pre-existing environment-dependent failures (agent-farm/terminal/consolidate; no built |
Adopt our two stranded upstream fixes: gitea tea-api reads (cluesmith#1146) + pr-create forge concept (cluesmith#1458)
waleedkadous
left a comment
There was a problem hiding this comment.
First — apologies for how long this sat; two months without a review is not how we want to treat a contribution, and this one is good: the real-script fake-CLI suite (contract normalization, pagination past page one, sub-50 server caps, mid-walk failures, tea 0.14.1 comment compatibility) is exactly the evidence this kind of fix needs, and moving branch input through jq --arg is a security improvement. Both integration reviews (codex + claude) agree the core mapping is right. Four things I'd like fixed before merge, all in the same spirit as the fix itself — fail loudly, never silently:
codev doctorregression. The five scripts that nowsource _lib.shmake doctor's executable extractor report.(and latertea_api_paged) instead oftea— verified,which .fails under/bin/sh. #1458 introduces a# forge-executable: teaheader declaration for exactly this; please add those five lines here regardless of merge order so the regression can't ship.- Silent truncation at
GITEA_MAX_PAGES. Reaching 100 pages without seeing a terminal short/empty page returns a partial array at exit 0 — the same false-negativepr-existsclass the paginator exists to prevent. Fail explicitly instead. - Error bodies normalized into valid-looking output.
tea apiexits 0 on HTTP errors, andpr-view/user-identity/issue-viewnormalize without validating the response shape — an error object becomes an all-null PR (pr-vieweven leaks the error body'surlinto the contract) or the usernamenull, at exit 0. Validate required fields/types before normalizing; the paginated scripts already fail loudly, so this brings the rest in line. recently-merged.shignoresCODEV_SINCE_DATEand can issue up to 100 sequential requests inside forge's 30-second timeout — on an established repo that turns the analytics path intonull. Honoring the since-date bounds it.
Smaller, take-or-leave: CODEV_REPO is now overloaded (repo-archive's foreign-repo input vs. gitea read targeting) — a line in forge.md and both SKILL.md copies would help; and forge-contracts.ts's reviewRequests/isDraft comments go stale with this PR.
Merge order: #1458 first (it brings the # forge-executable mechanism), then this. Happy to turn the two around quickly once these land — and thank you for sticking with it.
waleedkadous
commented
Sep 4, 2026
Given how long this waited on us, we'll take the last mile ourselves rather than hand you a list: a builder will push the review items directly onto this branch (you enabled maintainer edits — thank you), your commits and authorship stay as they are, and I'll re-review and merge once green. If you'd rather make the changes yourself, just say so and we'll hold off. |
… bodies, page ceiling, and bound recently-merged Maintainer follow-up on PR cluesmith#1146, pushed onto the contributor's branch. All four required items from the 2026-09-03 review, in the same spirit as the fix itself: fail loudly, never silently. 1. `# forge-executable: tea` headers. `codev doctor` infers the CLI a concept needs from the script's first substantive line; the five scripts that source `_lib.sh` open with `.`, so doctor reported `.`/`tea_api_paged` as missing tools and stopped checking for `tea`. The header (mechanism in cluesmith#1458, inert comment until then) declares it. `user-identity.sh` gets one too: fixing its exit-0-on-error handling below moves `tea` off the first substantive line, so without the header that fix would have caused the very regression this item closes. 2. The paginator fails at the `GITEA_MAX_PAGES` ceiling. Reaching 100 pages with no terminal short/empty page means we do not know we have the whole list; returning the partial array at exit 0 was the silent truncation the paginator exists to prevent — a short `pr-exists` walk reads as "no PR exists" and passes a porch pr_exists gate on a repo we merely failed to finish reading. 3. `pr-view`, `user-identity` and `issue-view` type-check the response before normalizing. `tea api` exits 0 on HTTP errors and prints the error body, which carries a `url` (the swagger link) — so `url: (.html_url // .url)` succeeded on it and shipped that link as the PR's browser page inside an otherwise all-null contract object; `user-identity` printed the literal username "null". They now fail with the server's own message on stderr. `issue-view` is validated before its comments are fetched, so a bad id reports only its own error, and its comments degrade path now tests for an actual JSON array — an error OBJECT used to reach `--argjson` and blow up with a raw jq error instead of the warned [] degrade. 4. `recently-merged` honors `CODEV_SINCE_DATE`. It feeds a 24h analytics window but walked the repo's entire merge history inside forge's 30s timeout, and a timeout yields `null` — worse than truncation. It now asks for `sort=recentupdate` and stops at the first page reaching back past the cutoff. The stop filter refuses to trust the sort blindly: it fires only when the page is actually non-increasing in `updated_at`, so a server that ignores the parameter falls back to the full walk rather than silently dropping merges. `updated_at >= merged_at` always holds, so nothing merged after the cutoff can sit beyond that page. Timestamps go through a new `gitea_epoch` jq helper in `_lib.sh`: Gitea marshals RFC3339 in the server's timezone, so `+02:00` is a real response and `Z` is not guaranteed — `fromdateiso8601` rejects those and a lexicographic compare across mixed offsets is wrong. It also accepts the bare `YYYY-MM-DD` that `team-update.ts` passes. Unparseable input yields null and every caller treats null as "don't know": keep the item, keep walking. Also, from the review's take-or-leave list: document `CODEV_REPO`'s two meanings (repo-archive input vs. gitea read-target override) in `forge.md` and both `SKILL.md` copies, and correct `forge-contracts.ts`'s `reviewRequests`/`isDraft` comments — both claimed GitLab and Gitea emit empty/false, but all three presets populate them for real. Tests: 12 new cases in the existing real-script fake-CLI suite. The fake `tea` grew error bodies at exit 0 for pulls/issues/user, a comments endpoint answering with an error object, a repo whose pages never end, and sorted/unsorted since-date repos. 33 tests in the file; full suite 4886 passed | 48 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch
…ge boundary, non-array pages, empty bodies
Consultation review of the previous commit (Codex). Five findings, all real.
The important one: my stop filter for `recently-merged` checked only that the
CURRENT page was non-increasing in `updated_at`, and I claimed that proved the
server honored `sort=recentupdate`. It does not. A server that ignores the
parameter can still return an internally descending page 1 — say one entirely
older than the cutoff — while a genuinely recent merge sits on page 2, and we
would have stopped and dropped it. Page-local order is also what a server with
per-page rather than global sorting produces.
The filter now requires the ordering to survive a page boundary: the previous
page descending too, and its oldest entry no older than this page's newest. It
never fires on page 1, where there is nothing to compare against — one extra
request is the right price. `tea_api_paged` binds the previous page as `$prev`
to make that check possible. The reviewer's exact counterexample is now a
fixture (`acme/lagging`).
Also:
- A page that parses but isn't an array is a hard error, not the end of the
list. `jq length` is 0 for both `null` and `{}`, so an error body mid-walk —
which `tea api` hands us at exit 0 — looked exactly like an exhausted list and
returned the pages collected so far at exit 0.
- An empty body at exit 0 now fails. jq given empty stdin emits nothing and
exits 0, so `pr-view` and `user-identity` were "succeeding" with empty stdout,
and the shape validators never ran at all.
- `gitea_epoch`'s offset is bounded to the real UTC range, so `+99:99` yields
null instead of an epoch two days out. Its remaining leniency is documented
rather than claimed away: it validates shape, not the calendar, so
`2026-02-30` normalizes into March.
- Contract types are checked, not just defaulted: non-numeric `additions`/
`deletions` no longer pass through as strings, comment fields default to the
declared type instead of emitting nulls, and a whitespace-only login is
rejected like an empty one.
Two bugs of my own that the tests caught: inside a jq `range` body `.` is the
range value, not the array (the `descending` helper needs the array bound
first), and an apostrophe inside a single-quoted jq program closes the shell
string.
37 tests in the file; full suite 4893 passed | 48 skipped. Every path also
exercised under dash, which is what /bin/sh is on the CI runner.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch…inux argv cap), probe past the page ceiling Second consultation lane (Claude), which independently reproduced the ordering flaw the first lane found and confirmed the cross-page fix closes it. Four new findings, all real. 1. Passing a page to the stop filter through `--argjson` breaks on Linux. Linux caps a SINGLE argv string at MAX_ARG_STRLEN (128KiB) regardless of ARG_MAX, and a 50-item Gitea pulls page — each object embedding full `base.repo` and `head.repo` objects — measures ~90KiB before anyone writes a long PR body. Past the cap `exec` fails, the paginator returns non-zero, and forge yields `null`. macOS has no per-argument cap, so this would have passed locally and failed on CI and on every Linux adopter. Both pages now go in on stdin. The `acme/heavy` fixture serves ~150KB pages so the regression bites where the bug lives. 2. The page ceiling false-positived on a complete result. A list whose length is an exact multiple of the page size reaches GITEA_MAX_PAGES with every page full and nothing missing, and we hard-failed it. One probe request past the ceiling settles it: empty means we already had everything. 3. A non-string `.message` — what a proxy or gateway between tea and Gitea produces — was concatenated straight into the error text and threw a raw jq error, defeating the point of a legible message. Now `(.message // .) | tostring`. 4. The test environment inherited `CODEV_*` from the developer's shell, so the "no CODEV_SINCE_DATE means walk everything" test was asserting the absence of a variable it did not control. Stripped from the base env; each test supplies what it means. Also: `issue-view`'s comments guard checked the outer array but not its elements, so `[1, 2]` got past it and died on `$comments[] | .body` instead of degrading to []. Both lanes noted that the `forge-executable` test asserts the declaration is present, not that doctor reads it — that is sequencing, not coverage, and the test now says so. 41 tests in the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch
…uesmith#1146 Records what the two consultation lanes broke and why, the ordering argument that did not hold, the Linux-only argv finding that could not reproduce on macOS, and the reason the forge-executable header had to go on six scripts rather than the five the review named. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch
waleedkadous
commented
Sep 4, 2026
Pushed the review items onto this branch as we offered — four commits on top of yours, nothing rebased or squashed, all ten of your commits and their authorship untouched. Thank you again for the contribution and for the patience; the real-script fake-CLI suite you built is what made all of this cheap to do, and every fix below is a test in your harness. Also merged The four required items1. 2. The page ceiling fails loudly. Reaching 3. Shape validation before normalizing. The nastiest part of this one is that Gitea's error bodies carry a 4. Plus the take-or-leave items: What the consultation caught, because it's the interesting partI first wrote the since-date bound to fire when the current page was non-increasing in The fix is to check for the property we actually need — the ordering — rather than for evidence that we asked for it. A stop now requires the order to survive a page boundary (previous page descending too, its oldest no older than this page's newest) and never fires on page 1, where there's nothing to compare against. A server that ignores the parameter falls back to your full walk: slower, never wrong. Costs exactly one extra request on an honest server. The other finding I'm glad we ran: passing a page to the stop filter through Smaller ones from the same pass: Testing41 tests in Full suite after merging main: 5549 passed | 48 skipped, 0 failures. Every script also exercised under Two follow-ups from the earlier reviews are still unfiled and still worth doing, neither in scope here: migrating Over to @waleedkadous for the re-review. |
…p CI flake Failed once on the first run of the pushed branch, passed on re-run with no code change. Unrelated to this PR — nothing here touches Tower. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012HvYzPVkD7jYoE7j9q61Ch
waleedkadous
left a comment
There was a problem hiding this comment.
Re-reviewed after the maintainer-side commits: six # forge-executable: tea headers (verified against the merged extractExecutable — all fourteen gitea concepts resolve to tea), the paginator now fails loudly at the page ceiling with a one-page probe so exact-multiple lists don't false-alarm, non-array error bodies are refused instead of normalized to null, recently-merged honors CODEV_SINCE_DATE with the stop filter fed via stdin (Linux argv cap), plus the docs and stale comments. 41 tests in the contributor's own fake-CLI harness; CI 7/7 green on a6eddb6. Approving and merging — thank you, @pseudoseed, for the contribution and the patience.
Uh oh!
There was an error while loading. Please reload this page.
Fixes#1137.
Bugfix-protocol re-do of the earlier SPIR-style PR #1138 (now closed), per maintainer request. Same root cause, plus a real regression test.
Problem
The
giteaforge preset was authored against the Gitea REST API JSON shape, but the scripts invoke theteaCLI, whose output shape differs — and several concepts referenced flags/fields/subcommandsteadoesn't have. Per the in-repo#920note,teawasn't available in the authoring environment, so the preset was never run end-to-end.Fix
Route the read concepts through
tea api(raw REST passthrough returning the shapeforge-contracts.ts+ the jq normalizers expect):tea api user | jq .login(tea whoamihas no--output json)tea api repos/<repo>/pulls/N→PrViewResult(incl. additions/deletions)tea api repos/<repo>/pulls?state=open→PrListItem[]tea api repos/<repo>/pulls?state=allwith nested.head.ref/.mergedtea api repos/<repo>/issues/N+ a second call for the comments array (Gitea reportscommentsas an int count, which would crash.comments.filter(...))tea api repos/<repo>/pulls?state=closed, filter.merged, using real.merged_attea comments add(tea issueshas nocommentsubcommand)tea apineeds an explicit owner/repo path segment, so each api-based script derives owner/repo from the origin remote (honoringCODEV_REPOwhen set).Testing
teaon PATH answeringapi <endpoint>with captured Gitea REST fixtures (tea isn't in CI, per vscode: editor-tab webview for rich backlog search #920), runs each real script, and asserts the normalized output conforms toforge-contracts.ts— incl. comments-as-array, merged-only filtering, open/merged/closedpr-exists, andCODEV_REPOoverride.pr-existsassertion for the newstate=allquery param.🤖 Generated with Claude Code
Rebased onto
main(2026-08-14)This branch was 2063 commits behind and
mergeable=CONFLICTING. Rebased ontoupstream/main; force-pushed to the fork. All 5 original commits preserved.Exactly one file conflicted, twice —
scripts/forge/gitea/pr-view.sh.pr-viewnow emitsurlWhile this branch sat, PIR #1179 landed on
mainand gave giteapr-viewaurlfield mapped from Gitea'shtml_url:This PR rewrites that same script onto
tea apiwith an explicit normalizer — which emitted nourlat all. Taking either side of the conflict wholesale loses something: take ours and #1179 is silently reverted, take theirs and thetea apifix is lost.Resolution: both. The script keeps this PR's
tea apirouting and re-addsurl: (.html_url // .url).So, stated plainly rather than left in the diff: gitea
pr-viewnow returns aurlfield that the pre-rebase branch did not return. That is a restoration ofmain's behaviour, not a new invention —forge-contracts.tsdocuments the Gitea mapping by name ("Giteahtml_url— Gitea'surlis the API endpoint, do not use it") — but it is a real change to this concept's output versus what this PR previously proposed, so it should not be discovered from the diff.bugfix-1137-gitea-tea-api.test.tswas updated accordingly: thepulls/42fixture now carries bothhtml_urlandurl, and the assertion pins that the browser page, not the API endpoint, is what reaches the contract.Two smaller deliberate deviations
_lib.shis committed100755, not100644.scripts/postinstall.mjschmods everyscripts/forge/**/*.shto 755 unconditionally, so 644 is a mode that never survives an install and leaves a permanently dirty worktree for anyone who runspnpm install. The file is sourced, not executed; the bit is inert.Relationship to #1458
#1458 (
pr-createas a forge concept) landed while this PR was open, and its giteapr-create.shlooked the new PR up withtea pulls list --limit 200— the exact call this PR proves silently truncates. That has been fixed on #1458's branch, not here: it now creates viatea api -X POST …/pulls, which returns the created PR directly, so the lookup is gone rather than paginated.Re-confirmed live against Forgejo 15.0.2 while doing so:
settings/apireportsmax_response_items: 50, and a?limit=200request returns exactly 50 items on a list where paging at 50 returns 53. The premise behind this PR's pagination work holds.Merge-order implications are spelled out in full at the end of this description.
Verification
bugfix-1137-gitea-tea-api,bugfix-568-pr-exists-state-all,forge,bugfix-693-forge-exec-bit).@cluesmith/codevunit suite, rebased tip: 3193 passed, 126 failed (67 files).upstream/mainin the same worktree: 3176 passed, 126 failed (67 files) — the same 67 files and the same 126 tests.agent-farm/terminal/consolidate(attach, session-manager, shellper sockets, SQLite state), environment-dependent — this worktree has no builtdist/, which those tests spawn from, and a live Tower is running against the same state. None is in a file this PR touches, and every forge suite passes.Merge order with the sibling PR — verified, not assumed
#1146 and #1458 come from the same fork and both touch
packages/codev/scripts/forge/gitea/, so the ordering question is fair. The answer:Either order is safe. There is no dependency and no conflict.
git merge-treeon the two branch tips merges cleanly. The only file both touch is the builder thread log, which is the identical blob on both branches and auto-merges.pr-create.shdoes notsource _lib.shand does not callgitea_repoortea_api_paged. Nothing in it resolves against #1146._lib.shandpr-view.share byte-identical to #1146's versions andpr-create.shbyte-identical to #1458's — no silent blending. Thebugfix-693invariant (every entry under each provider dir is a*.sh) still holds with_lib.shpresent.Does #1458 duplicate something #1146 makes shared?
The paginator: no, and it shouldn't.
_lib.sh#tea_api_pagedexists to walk a truncating list endpoint.pr-createno longer lists anything — it reads the new PR out of the create response — so there is no pagination for it to share. That is the point of the reconcile rather than an oversight.Repo resolution: yes, there are two paths, and this is worth a follow-up.
. _lib.sh→gitea_repo(), which honoursCODEV_REPO, else derivesowner/repofrom the origin remote, and fails fast namingCODEV_REPOas the remedy.pr-create: tea's own{owner}/{repo}placeholders, withCODEV_PR_REPOforwarded astea --repo.They were kept separate deliberately, for two reasons rather than by omission:
pr-createtakesCODEV_PR_REPO; the read concepts takeCODEV_REPO.gitea_repo()reads the latter and takes no argument, sopr-createcould not call it without changing its signature — which would mean editing a [Bugfix #1137] Fix gitea forge preset against the real tea CLI #1146 file from [Bugfix #1455] Add a pr-create forge concept so non-GitHub forges can open PRs #1458 and creating exactly the merge-order coupling this avoids.--repodoes more than fill a path. It also supplies tea's repo/login context, verified working from a cwd whose remote is not a Gitea host. A path-only helper does not do that.Recommended follow-up (not done here, deliberately): once both PRs have landed, unify the two behind one helper that takes the override variable as a parameter — e.g.
gitea_repo "$CODEV_PR_REPO"— so there is one repo-resolution path with one error message. Doing it now would couple two independent PRs; doing it never leaves two paths that will drift. It is a small, mechanical change against a tree where both are already present.The one ergonomic gap that split created has been closed in the meantime: an unresolvable repo used to surface from
pr-createas a bare404 page not found, and now namesCODEV_PR_REPOas the remedy, matchinggitea_repo()'s fail-fast message.