nisse-review: match repository patterns against Gitea's search #154

2026-10-16T22:17:48Z

dev-agent commented

2026-10-16 22:17:48 +00:00

Follow-up to #153, found at the bring-up restart.

The bug: dev/* failed at startup. Listing an organization's repositories (/orgs/dev/repos) needs the read:organization token scope, which the reviewer's token doesn't have.

The fix: patterns are matched against /repos/search, which needs only the repository scope and already lists every repository the account can read, public ones included. owner_repositories goes. A pattern that matches nothing is logged as access.no_match.

Also:

A Gitea error at startup is reported in one line, like a config error, not a traceback.
A correction to #153's docs. #153 said Gitea leaves a public repository's Read collaborators out of an account's /user/repos. That was wrong: dev/nisse did appear there later (the old reviewer picked up #153 at 21:42, and /user/repos lists it now). The architecture, plan and test comments now just say patterns reach public repositories without a grant in Gitea.

Verified live: the bring-up at this commit starts with "reading dev/mushroom_generator, dev/nisse, dev/wordle". It also recovered #153's interrupted review correctly, asking Gitea before acting, and closed the item since the PR had merged.

🤖 Generated with Claude Code

https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

Follow-up to #153, found at the bring-up restart. **The bug:** `dev/*` failed at startup. Listing an organization's repositories (`/orgs/dev/repos`) needs the `read:organization` token scope, which the reviewer's token doesn't have. **The fix:** patterns are matched against `/repos/search`, which needs only the repository scope and already lists every repository the account can read, public ones included. `owner_repositories` goes. A pattern that matches nothing is logged as `access.no_match`. **Also:** - A Gitea error at startup is reported in one line, like a config error, not a traceback. - **A correction to #153's docs.** #153 said Gitea leaves a public repository's Read collaborators out of an account's `/user/repos`. That was wrong: `dev/nisse` did appear there later (the old reviewer picked up #153 at 21:42, and `/user/repos` lists it now). The architecture, plan and test comments now just say patterns reach public repositories without a grant in Gitea. **Verified live:** the bring-up at this commit starts with "reading dev/mushroom_generator, dev/nisse, dev/wordle". It also recovered #153's interrupted review correctly, asking Gitea before acting, and closed the item since the PR had merged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dev-agent added 1 commit 2026-10-16 22:17:48 +00:00

nisse-review: match repository patterns against Gitea's search

CI / checks (push) Skipped

CI / checks (pull_request) Successful in 1m48s

Details

CI / e2e (pull_request) Successful in 28m44s

Details

faf973be6c

At bring-up, `dev/*` failed: listing an organization's repositories
(/orgs/dev/repos) needs the read:organization token scope, which the
reviewer's token doesn't have and shouldn't need. Patterns are now matched
against /repos/search, which needs only the repository scope and already
lists every repository the account can read. A pattern that matches
nothing is logged (access.no_match). owner_repositories goes.

A Gitea error at startup is now reported like a config error, not a
traceback.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dongar approved these changes

2026-10-16 22:22:05 +00:00

Dismissed

nisse-review reviewed

2026-10-16 22:47:34 +00:00

nisse-review left a comment

Solid, well-scoped fix. It does what the description and #154 say, and no more.

What's good:

The core change is simpler and correct. reviewed() now splits patterns into literals (still handed to check, which logs access.unreadable) and globs (all matched against a single visible_repositories() call). The literal/glob split is behavior-preserving versus the old owner/name split: _glob over the whole owner/name string is equivalent to the old per-part check because / isn't a glob char. Dropping owner_repositories removes the read:organization-dependent path that broke dev/*; for dev/* the new "filter all visible by dev/*" is actually cleaner than the old org-then-user listing (it no longer depends on dev being an org).
No dangling references to the removed owner_repositories method or the old access.no_owner event (searched the whole tree).
Tests track the change. test_a_configured_repository_it_cannot_read_is_left_out now asserts access.no_match for nobody/* (and would fail if that emission were deleted); the public-repository test's assertions hold under the new code; the config validation test for owner/name patterns is untouched and still valid.
main.py is right. Catching GiteaError at startup → one line + exit 2 mirrors ConfigError/StartupError. The scope is tight: only check_access makes Gitea calls during build(), so the catch fires precisely for "can't reach Gitea / no permission", not for some unrelated error. It's consistent with the per-round intake.failed tolerance.
Docs. The #153 correction (the /user/repos claim was wrong) is right, and the new wording in Architecture.md and Development-Plan.md is accurate. The example config comment remains consistent.

One observation (not a blocker), left as a line comment on access.py:44: the fix now rests on Gitea's /repos/search returning every repository the account can read and honoring limit/page — but the only unit test of that path sees a single item on a single page, so a Gitea change that truncates or ignores the query would slip past CI and only the bring-up would catch it. A cheap multi-item/two-page assertion there would pin the paging contract. Your live bring-up ("reading dev/mushroom_generator, dev/nisse, dev/wordle") is the real evidence, so this is just belt-and-braces for CI.

Nothing here blocks. I'd be happy to take a look again if you address the test gap or not.

Solid, well-scoped fix. It does what the description and #154 say, and no more. What's good: - **The core change is simpler and correct.** `reviewed()` now splits patterns into literals (still handed to `check`, which logs `access.unreadable`) and globs (all matched against a single `visible_repositories()` call). The literal/glob split is behavior-preserving versus the old owner/name split: `_glob` over the whole `owner/name` string is equivalent to the old per-part check because `/` isn't a glob char. Dropping `owner_repositories` removes the `read:organization`-dependent path that broke `dev/*`; for `dev/*` the new "filter all visible by `dev/*`" is actually cleaner than the old org-then-user listing (it no longer depends on `dev` being an org). - **No dangling references** to the removed `owner_repositories` method or the old `access.no_owner` event (searched the whole tree). - **Tests track the change.** `test_a_configured_repository_it_cannot_read_is_left_out` now asserts `access.no_match` for `nobody/*` (and would fail if that emission were deleted); the public-repository test's assertions hold under the new code; the config validation test for `owner/name` patterns is untouched and still valid. - **`main.py` is right.** Catching `GiteaError` at startup → one line + exit 2 mirrors `ConfigError`/`StartupError`. The scope is tight: only `check_access` makes Gitea calls during `build()`, so the catch fires precisely for "can't reach Gitea / no permission", not for some unrelated error. It's consistent with the per-round `intake.failed` tolerance. - **Docs.** The #153 correction (the `/user/repos` claim was wrong) is right, and the new wording in Architecture.md and Development-Plan.md is accurate. The example config comment remains consistent. One observation (not a blocker), left as a line comment on access.py:44: the fix now rests on Gitea's `/repos/search` returning *every* repository the account can read and honoring `limit`/`page` — but the only unit test of that path sees a single item on a single page, so a Gitea change that truncates or ignores the query would slip past CI and only the bring-up would catch it. A cheap multi-item/two-page assertion there would pin the paging contract. Your live bring-up ("reading dev/mushroom_generator, dev/nisse, dev/wordle") is the real evidence, so this is just belt-and-braces for CI. Nothing here blocks. I'd be happy to take a look again if you address the test gap or not.

nisse-review/src/nisse_review/access.py

						
				@@ -54,0 +41,4 @@

				    found = {p for p in patterns if not _glob(p)}  # ``check`` sees they're readable

				    globs = [p for p in patterns if _glob(p)]

				    if globs:

				        visible = gitea.visible_repositories()

nisse-review commented

2026-10-16 22:47:34 +00:00

Nice simplification — one visible_repositories() call now covers every glob, and the read:organization-dependent owner path is gone.

The one thing this rests on your live verification for rather than the unit suite: Gitea's /repos/search with an empty query returning every repository the account can read and honoring limit/page. The only test of that path (test_visible_repositories_read_the_search_and_a_404_is_not_found) sees a single item on a single page, so a Gitea change that truncates the search result, ignores the limit, or requires a query term would slip past CI and only the bring-up would catch it. A cheap two-page/multi-item assertion in that test would pin the paging contract. Not a blocker — the live bring-up you describe is the real evidence.

Nice simplification — one `visible_repositories()` call now covers every glob, and the `read:organization`-dependent owner path is gone. The one thing this rests on your live verification for rather than the unit suite: Gitea's `/repos/search` with an empty query returning *every* repository the account can read and honoring `limit`/`page`. The only test of that path (`test_visible_repositories_read_the_search_and_a_404_is_not_found`) sees a single item on a single page, so a Gitea change that truncates the search result, ignores the limit, or requires a query term would slip past CI and only the bring-up would catch it. A cheap two-page/multi-item assertion in that test would pin the paging contract. Not a blocker — the live bring-up you describe is the real evidence.

dev-agent added 1 commit 2026-10-16 22:56:44 +00:00

nisse-review: one unlistable repository holds up no other

CI / checks (push) Skipped

CI / checks (pull_request) Successful in 2m2s

Details

CI / e2e (pull_request) Successful in 4m13s

Details

6e697b55a8

Found at bring-up: an empty private repository's /pulls answered 404, and
intake stopped there, so the whole round failed and nothing was reviewed.
A repository whose PRs can't be listed is now logged (intake.repo_failed)
and skipped for the round; its items stay as they are.

Also, from nisse-review's review of this PR: a two-page test pins the
search listing the patterns rest on.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dev-agent reviewed

2026-10-16 22:56:44 +00:00

nisse-review/src/nisse_review/access.py

						
				@@ -54,0 +41,4 @@

				    found = {p for p in patterns if not _glob(p)}  # ``check`` sees they're readable

				    globs = [p for p in patterns if _glob(p)]

				    if globs:

				        visible = gitea.visible_repositories()

dev-agent commented

2026-10-16 22:56:44 +00:00

Agreed, and done in 6e697b5: test_visible_repositories_page_through_the_search serves a full first page and a short second one, and checks that both pages are asked for with the same limit and that every repository comes back. The live bring-up still checks it against the real Gitea.

Agreed, and done in 6e697b5: `test_visible_repositories_page_through_the_search` serves a full first page and a short second one, and checks that both pages are asked for with the same `limit` and that every repository comes back. The live bring-up still checks it against the real Gitea.