Skip to content

DRU-1130 -- druks: a build's clone fails with 503 when its project has many repos - #1163

Merged
druks-ai[bot] merged 1 commit into
mainfrom
agent/DRU-1130
Nov 10, 2026
Merged

druks-ai[bot] merged 1 commit into
mainfrom
agent/DRU-1130

Conversation

@druks-ai

@druks-ai druks-ai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

What changed

A Software Factory build asks for one GitHub token for all repos of its project. Druks looked up the App installation of each repo in turn. A project with many repos could then pass the drukbox proxy's 5-second wait, and git clone failed with a 503.

Now issue_token looks up the first repo's installation one time. When that installation covers all repositories, the other repos of that owner go into the token with no more lookups. An installation on selected repositories keeps the lookup for each repo. The token still names only the repos that the first repo's installation covers. The client keeps no state.

Trade-off: after an owner resolves to an installation on all its repositories, a deleted or renamed repo of that owner goes directly into the token request. GitHub then refuses the whole token with a 422. The lookup for each repo used to drop that repo from the token silently. Now a stale repo name stops the token with a 422 that names the fix. issue_token raises MissingSecretError, whose message names the repos of the request and says to remove the renamed or deleted repository from the project. The secret route answers the box with that 422, not with a 503 that the exchange retries.

Plan

Implement owner-scoped installation lookup reuse for token issuance, with the decision in Github.Authentication.issue_token, and make the one failure that reuse can cause name its cause.

Binding ticket direction (DRU-1130):
“Look up the installation once for each owner. When that installation covers all repositories of the owner, every repo of that owner goes into the token with no more lookups. An installation with selected repositories keeps the lookup for each repo, as today.”

The ticket’s sole comment adds: “The token must continue excluding repositories outside the first repository’s installation.” The ticket has no assignee, so no GitHub login can be resolved.

Review history on PR #1163 (czpython)

  • Round 1: the lookup reuse stays. A deleted, renamed, or transferred repo of an all-repositories owner now goes straight into the token request. GitHub refuses the whole token with a 422, the route answers 503, the exchange retries, and nothing names the cause. The trade-off stays accepted: no verification lookup comes back and the mint is not retried. The failure must name its cause. This produced the 422 mapping in issue_token.
  • Round 2 (this revision, plan revision 3): the 422 handling is right and stays. Two things change.
    1. The reuse moves out of GitHubAppClient. The hidden state _installations_on_all_repositories makes get_installation_id("acme/gone") return an id for a repo it never checked, and the answer of one call depends on an earlier call. The rule "this installation covers all repositories, so skip the lookups" is a decision about one token and belongs in issue_token.
    2. The 422 message is built from owner and the repo names, not by splitting first again for each name. The message text does not change.

What exists at HEAD (f4cb4b37)

  • backend/druks/core/apis/github.py: GitHubAppClient.__init__ sets self._installations_on_all_repositories under a one-line comment. get_installation_id returns an int, reads the memo first, and writes it after a lookup reports repository_selection == "all". On main (92ebaeb9) the method has no memo and ends each lookup with return (await lookup()).parsed_data.id.
  • backend/druks/core/services.py: issue_token compares each other repo's get_installation_id with the first repo's id, under suppress(GitHubAppNotInstalledError). It wraps VaultSecret.get_or_mint_token in except RequestFailed, re-raises non-422 with a bare raise, and raises MissingSecretError(...) from error for 422. The message rebuilds each name with first.split('/', 1)[0] + '/' + repo_name. get_client (~line 180) and revoke_tokens (~line 194) call get_installation_id.
  • Other callers of get_installation_id are only test doubles and tests (see Tests below). No docs mention it.
  • backend/druks/sandbox/routes.py already answers MissingSecretError with 422 and detail=str(exc); every other exception answers 503. It needs no change.
  • backend/tests/test_managed_github.py: _GitHubKit._mint refuses names in DELETED_REPOSITORIES = ("gone",) with _refused(422) after recording the POST. Three token tests exist: test_an_installation_on_all_of_an_owners_repos_is_looked_up_once_per_token, test_an_installation_on_selected_repos_is_looked_up_for_each_repo, test_a_stale_repo_name_stops_the_token_with_the_fix.
  • The PR body's first paragraph no longer says "services.py does not change."; its trade-off paragraph already names the 422 fix.

Revision 3 changes (this round's delta)

1. backend/druks/core/apis/github.py: the client keeps no state

  • Delete self._installations_on_all_repositories and the comment line above it. __init__ is back to its main shape.
  • Rename get_installation_id to get_installation(self, owner_or_repo: str). The body is the body on main, except that the successful lookup returns (await lookup()).parsed_data. The return type is githubkit's parsed installation model; confirm the import path after uv sync. Delete the memo read and the memo write. The org-to-user fallback, the 404 continue, the non-404 re-raise, and GitHubAppNotInstalledError(owner_or_repo, app=self._slug) stay as on main.
  • Do not keep get_installation_id as a wrapper. One lookup has one name.

2. backend/druks/core/services.py

  • issue_token. Keep the empty-resources guard, the first-lookup except exceptions.GitHubAppNotInstalledError as error: raise MissingSecretError(str(error)) from error, names.sort(), the Audience.resource(...) shape, on_miss=functools.partial(app.create_installation_token, ...), and the RequestFailed import. Replace the id-comparison loop with:
try:
    installation = await app.get_installation(first)
except exceptions.GitHubAppNotInstalledError as error:
    raise MissingSecretError(str(error)) from error
owner, repo_name = first.split("/", 1)
names = [repo_name]
for repo in others:
    repo_owner, repo_name = repo.split("/", 1)
    # An installation is on one account, so a repo of another owner is never in it.
    if repo_owner != owner:
        continue
    if installation.repository_selection == "selected":
        try:
            await app.get_installation(repo)
        except exceptions.GitHubAppNotInstalledError:
            continue
    names.append(repo_name)
names.sort()

Use installation.id where installation_id was used (Audience.resource(...) and the on_miss partial). Do not name a local variable name: issue_token has a name parameter. suppress is still used elsewhere in the module, so its import stays.

  • The 422 handler. Keep except RequestFailed, the status_code != 422 bare raise, and raise ... from error. Build the message from owner and names:
repos = ", ".join(f"{owner}/{repo_name}" for repo_name in names)
raise MissingSecretError(
    f"GitHub refused a token for {repos}. GitHub has no repository under one of "
    "these names. Remove the renamed or deleted repository from the project."
) from error

The message text is unchanged. Keep repo_name as the generator variable, not name. Do not format str(error).

  • get_client and revoke_tokens. Call get_installation(owner_or_repo) and read .id.

3. Tests

  • backend/tests/test_github.py: _TestGitHubAppClient loses self._installations_on_all_repositories = {}. The three calls (~lines 264, 279, 293) use get_installation; the two successful ones compare (await ...).id with 42 and 41. Restore the two fixtures to their main shape (SimpleNamespace(id=42), id=41): the client no longer reads repository_selection.
  • backend/tests/test_github_channel.py:35, backend/tests/test_sandbox_identities.py:447, backend/tests/test_workspaces.py:174 and :196: the fakes replace get_installation (the monkeypatch.setattr(GitHubAppClient, "get_installation", ...) name changes too) and return an object with id, for example SimpleNamespace(id=7). Those tests issue tokens for one repo, so repository_selection is never read; add it only if a test passes several repos of one owner.
  • backend/tests/test_managed_github.py: in test_an_installation_on_selected_repos_is_looked_up_for_each_repo, remove "GET repos/acme/web/installation" from the expected calls. The remaining calls are GET repos/paulo/site/installation, GET repos/paulo/blog/installation, GET repos/paulo/private/installation, then POST app/installations/22/access_tokens ['blog', 'site']. A repo of another owner gets no lookup now; it was never in the first repo's installation, so the token is the same. The all-owner test and the stale-repo test keep their call lists unchanged; check each one under the new logic (one GET, then the mint). DELETED_REPOSITORIES and the _mint refusal stay as they are. Add no other tests.

4. PR #1163 description

Read the body with gh pr view 1163 --json body --jq .body. Replace the second paragraph of "What changed" only (the one that begins "Now GitHubAppClient.get_installation_id…") with: "Now issue_token looks up the first repo's installation one time. When that installation covers all repositories, the other repos of that owner go into the token with no more lookups. An installation on selected repositories keeps the lookup for each repo. The token still names only the repos that the first repo's installation covers. The client keeps no state." Keep the first paragraph, the trade-off paragraph, the Plan section, and the trailing "Plan authored by Druks" comment unchanged. Apply with gh pr edit 1163 --body-file <file>.

Implementation verification (separate from the acceptance criteria)

  • Prepare the documented backend dependencies, proof-app install, Postgres, and Redis. Run uv run pytest backend/tests/test_github.py backend/tests/test_managed_github.py backend/tests/test_github_channel.py backend/tests/test_sandbox_identities.py backend/tests/test_workspaces.py, then uv run ruff check backend druks-calls, uv run ruff format --check backend druks-calls, and uv run pytest backend/.
  • Revert-check the stale-repo test: revert only the 422 handler in services.py, keep the test, confirm it fails, then restore.
  • Revert-check the lookup reuse: restore the old id-comparison loop in issue_token (keep the tests) and confirm the all-owner test fails on the lookup count; then restore.
  • Run druks-house-rules check --base origin/main if installed.
  • Attempt root uv run --locked pytest; report the exact blocker if it cannot run.
  • Run npm --prefix frontend run lint, npm --prefix frontend run build, npm --prefix frontend test, and node --test backend/tests/chat/bridge.test.mjs when executable here. Otherwise report each exact command and blocker as not_run; these are not hard gates for this backend diff.

Acceptance Criteria

  • AC1: A token for several repos of one owner, whose installation covers all its repositories, makes one installation lookup.
    • Verification: Unchanged in substance. test_an_installation_on_all_of_an_owners_repos_is_looked_up_once_per_token calls Github.issue_token for acme/web, acme/api, acme/docs and asserts githubkit.calls is exactly one 'GET repos/acme/web/installation' then one 'POST app/installations/11/access_tokens ['api', 'docs', 'web']'. After acme moves to installation 33, a second issuance makes one fresh GET and mints under 33. The test keeps its call lists and passes under the new issue_token logic. Restoring the old per-repo id-comparison loop in issue_token (keeping the test) makes the lookup-count assertion fail.
  • AC2: A repo of another owner, or a repo outside a selected installation, stays out of the token, as today. A repo of another owner needs no lookup, because an installation is on one account.
    • Verification: test_an_installation_on_selected_repos_is_looked_up_for_each_repo calls issue_token for ('paulo/site', 'acme/web', 'paulo/blog', 'acme/api', 'paulo/private') and asserts githubkit.calls is exactly: 'GET repos/paulo/site/installation', 'GET repos/paulo/blog/installation', 'GET repos/paulo/private/installation', then "POST app/installations/22/access_tokens ['blog', 'site']". The acme/web lookup is gone from the list; the other-owner repos get no lookup and are absent from the token; paulo/private (404) is dropped. test_a_box_token_names_the_repos_its_first_repos_installation_covers stays unmodified and passes: paulo/site and other/gone stay out, and the token is ['api', 'web']. No standalone exclusion tests are added.
  • AC3: Token issuance still resolves the first repository's installation, uses sorted repository names, the same vault audience and on_miss minting, token reuse, and the existing error translations. The decision to skip lookups lives in issue_token.
    • Verification: The diff of backend/druks/core/services.py against 92ebaeb shows changes only in Github.Authentication.issue_token (the get_installation call for the first repo with its GitHubAppNotInstalledError -> MissingSecretError(str(error)) from error kept; the owner/repo_name split; the same-owner loop with the 'An installation is on one account' comment; the per-repo get_installation call only when installation.repository_selection == "selected", with GitHubAppNotInstalledError -> continue; names.sort(); installation.id in Audience.resource and in the on_miss partial; the RequestFailed import and try/except), in get_client, and in revoke_tokens (both call get_installation(...) and read .id). No local variable named name is introduced in issue_token. test_a_box_token_names_the_repos_its_first_repos_installation_covers keeps its token-scope and reordered-resource reuse assertions unmodified. Existing empty-resource, missing-installation, managed-routing, and vault tests pass; no tests are added for these unchanged behaviors.
  • AC4: GitHubAppClient keeps no installation state. get_installation_id is renamed to get_installation and returns the installation GitHub answered (githubkit's parsed data). No get_installation_id wrapper remains, and later token issuances resolve installation information afresh.
    • Verification: The diff of backend/druks/core/apis/github.py against 92ebaeb is only the rename to get_installation, the return of (await lookup()).parsed_data instead of .parsed_data.id, and the matching return annotation. init is as on main, with no _installations_on_all_repositories and no comment about it. grep -rn "get_installation_id\|_installations_on_all_repositories" backend finds nothing. The org-to-user fallback, the 404 continue, the non-404 re-raise, and GitHubAppNotInstalledError remain as on main, shown by test_an_account_installation_falls_back_to_the_user_endpoint (the returned installation's .id is 42), test_an_org_installation_does_not_consult_the_user_endpoint (.id is 41, user endpoint not awaited), and test_installation_lookup_404_names_the_repo. The AC1 scenario's reinstall step (acme moves to 33, one fresh lookup, mint under 33) and test_after_a_reinstall_the_next_client_acts_in_the_new_installation pass.
  • AC5: When GitHub refuses the installation token with a 422, Github.Authentication.issue_token raises MissingSecretError chained from the GitHub error. Its message names the repositories of the request, qualified with the first repo's owner, and the fix (remove the renamed or deleted repository from the project). Every other status propagates unchanged. No installation lookup is added and the mint is not retried. The secret route is unchanged and answers 422 with that message.
    • Verification: test_a_stale_repo_name_stops_the_token_with_the_fix stays and passes with its call list unchanged: Github.issue_token(('acme/web', 'acme/gone')) raises MissingSecretError (pytest.raises) whose message contains 'acme/gone' and 'Remove the renamed or deleted repository from the project.', and githubkit.calls is exactly ['GET repos/acme/web/installation', "POST app/installations/11/access_tokens ['gone', 'web']"]. Inspection of the services.py handler shows: it catches RequestFailed (not RequestError); it checks error.response.status_code != 422 and re-raises with a bare raise; it builds repos = ", ".join(f"{owner}/{repo_name}" for repo_name in names) using the owner bound from first.split("/", 1) in issue_token, with no further split of first inside the join; it raises MissingSecretError(f"GitHub refused a token for {repos}. GitHub has no repository under one of " "these names. Remove the renamed or deleted repository from the project.") from error; and it does not format str(error). Reverting only the 422 handler makes this test fail (RequestFailed escapes). git diff shows backend/druks/sandbox/routes.py unchanged.
  • AC6: The test suite follows the rename and the stateless client, and the PR description matches the new design.
    • Verification: _TestGitHubAppClient in backend/tests/test_github.py has no _installations_on_all_repositories attribute; test_github.py calls get_installation and compares .id; its two round-1 fixtures are back to SimpleNamespace(id=42) and SimpleNamespace(id=41). The fakes in backend/tests/test_github_channel.py, test_sandbox_identities.py, and test_workspaces.py (the two in test_workspaces.py included) monkeypatch GitHubAppClient 'get_installation' and return an object with .id; none refer to get_installation_id. uv run pytest backend/ passes, and uv run ruff check backend druks-calls and uv run ruff format --check backend druks-calls pass. gh pr view 1163 --json body shows the second paragraph of 'What changed' describing issue_token's single first-repo lookup, the skipped lookups for an all-repositories installation, the per-repo lookups for selected repositories, and 'The client keeps no state.' It no longer says the client keeps knowledge for its own life. The trade-off paragraph, the Plan section, and the trailing 'Plan authored by Druks' comment are unchanged.

@druks-ai druks-ai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: comment. Nothing here blocks the merge.

The change does what the description says. get_installation_id reuses an owner's installation only after GitHub reports repository_selection == "all". An installation on selected repositories keeps the lookup for each repo. The memo lives on the client, and services.py builds a new client for each call, so the next token asks GitHub again.

Acceptance criteria, checked against the diff:

  • AC1 is met. test_an_installation_on_all_of_an_owners_repos_is_looked_up_once_per_token makes one lookup for three acme repos and mints for all three names.
  • AC2 is met. test_an_installation_on_selected_repos_is_looked_up_for_each_repo looks up each paulo repo and drops paulo/private. It also drops acme/api through the cached path: the cached id 11 is compared with 22, not added blindly.
  • AC3 is met. services.py is untouched, and test_a_box_token_names_the_repos_its_first_repos_installation_covers is unchanged and passes.
  • AC4 is met. The state is set in __init__, and the method still returns int. The first test re-issues after acme moves to 33 and sees one fresh lookup and a mint under 33.

Verification:

  • I reverted github.py to main and kept the new tests. Both new tests fail, and the other 40 in the two files pass, so checklist item 14 holds.
  • The full backend suite passes on the branch: 2627 passed. ruff check and ruff format --check are clean.
  • repository_selection is a required Literal["all", "selected"] on githubkit's Installation model, so reading it with no fallback is safe.
  • Code search found no use of GitHubAppClient or get_installation_id in the other getdruks repos.

I left two notes inline. The medium one adds evidence about the accepted trade-off that you may want to weigh. The low one is optional.

Comment thread backend/druks/core/apis/github.py Outdated
async def get_installation_id(self, owner_or_repo: str) -> int:
"""The installation that covers a repository ``owner/name``, or an account."""
owner, _, name = owner_or_repo.partition("/")
if installation_id := self._installations_on_all_repositories.get(owner):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium: the trade-off costs more than the description says. After this return, a deleted repo of an all-repositories owner goes straight into create_installation_token, and GitHub refuses the whole token. Two things in this repo make that worse than "the token is refused":

  • Stale names are a known state here. ProjectRepo rows keep the name they were registered under. ProjectRepo.get_for_repo (contrib/software_factory/models.py:191) already warns that a rename or a transfer leaves the old name behind.
  • The build prompt treats a missing related repo as safe. templates/build/_related_repos.md:13 tells the agent: "If a clone fails (repo renamed, deleted, or inaccessible), carry on without it — it's lost context, not a blocker." With this change, one deleted sibling makes the secret route (sandbox/routes.py:48) catch GitHub's 422 as a generic failure. It then answers 503 on every fetch, including the clone of the build's own repo. The box sees the same 503 this PR fixes, and retrying cannot fix it.

The trade-off is your call. If it stands, a follow-up could bring back the old behavior on the failure path only: when the mint returns 422, look up each repo again. That costs extra lookups only when a stale name exists.

Comment thread backend/druks/core/apis/github.py Resolved
@druks-ai
druks-ai Bot marked this pull request as ready for review November 10, 2026 11:45
@druks-ai
druks-ai Bot requested a review from czpython as a code owner November 10, 2026 11:45

@czpython czpython left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lookup reuse is correct and stays. One case needs a change before the merge.

The problem

Druks keeps the project's repo names. A repo can be deleted, renamed, or transferred on GitHub, and Druks keeps the old name.

  • Before this PR, the lookup for that repo failed and issue_token left the repo out of the token. The build worked.
  • With this PR, when the owner's installation covers all its repositories, the old name goes directly into the token request. GitHub refuses the whole token with a 422.
  • That 422 is not a MissingSecretError, so the secret route (backend/druks/sandbox/routes.py) answers 503 and the exchange retries. Every clone of the build fails, also the clone of the build's own repo. The box shows only a 503, the same symptom this ticket fixes, and nothing names the cause.

The trade-off stays: do not add a lookup back, and do not retry the mint with a lookup for each repo. The failure must name its cause.

The change

  1. When GitHub refuses the installation token with a 422, Github.Authentication.issue_token raises MissingSecretError. The message names the repositories of the request and the fix: remove the renamed or deleted repository from the project. Every other status propagates as it does today. This case fits the contract of MissingSecretError: no secret until a person acts, and the message names the fix. The secret route then answers 422 with that reason. Follow the pattern issue_token already uses for GitHubAppNotInstalledError. This changes backend/druks/core/services.py; the plan's "leave services.py untouched" no longer applies.
  2. Add one test: a token request that names a repo GitHub does not have, for an owner whose installation covers all its repositories, raises MissingSecretError. The test must fail without change 1.
  3. Add a one-line comment on _installations_on_all_repositories that states the invariant, and nothing more: the lookups of one token share it, and a new client asks GitHub again, so a reinstall shows on the next token. One line only. Do not explain the design.
  4. Update the trade-off paragraph of the PR description: a stale repo name now stops the token with a 422 that names the fix.

No other changes.

@druks-ai

druks-ai Bot commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the report. The lookup reuse stays. Here is what will change for the stale-name case:

  • Github.Authentication.issue_token in backend/druks/core/services.py will turn GitHub's 422 on the token request into MissingSecretError. The message will name the repositories of the request and ask for the renamed or deleted repository to be removed from the project. The secret route already answers 422 with that message, so the box sees the cause instead of a 503.
  • Every other status propagates as it does today. No lookup is added back, and no per-repository retry is added.
  • A regression test will cover an all-repositories owner with a stale repository name, and it will fail without this change.
  • The installation memo gets a one-line comment on its lifetime, and the PR description's trade-off paragraph will say that a stale name now stops the token with a 422 that names the fix.

This supersedes the earlier plan note that services.py stays untouched.

@druks-ai
druks-ai Bot dismissed czpython’s stale review November 10, 2026 13:14

Addressed in 5bc67ac: issue_token turns GitHub's 422 into a MissingSecretError that names the repos and the fix, one test covers it, the memo has its one-line comment, and the trade-off paragraph is updated.

@druks-ai
druks-ai Bot force-pushed the agent/DRU-1130 branch 2 times, most recently from bb5f993 to f4cb4b3 Compare November 10, 2026 13:37

@druks-ai druks-ai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: approve.

This round does what czpython asked for in round 1, and nothing more. A 422 from the mint now becomes a MissingSecretError. The secret route already answers that error with a 422 and the message. The box no longer gets a 503 that the exchange retries.

Acceptance criteria, checked against the diff:

  • AC1 is met. test_an_installation_on_all_of_an_owners_repos_is_looked_up_once_per_token makes one lookup for three acme repos and mints for all three names.
  • AC2 is met. test_an_installation_on_selected_repos_is_looked_up_for_each_repo looks up each paulo repo and drops paulo/private. It also drops the acme repos, and the memo path compares id 11 with 22 before it adds a name.
  • AC3 is met. Against 92ebaeb9, services.py gains only the RequestFailed import and the try/except around get_or_mint_token. The on_miss partial only wraps to the new indent. test_a_box_token_names_the_repos_its_first_repos_installation_covers is unchanged.
  • AC4 is met. __init__ sets the memo, and get_installation_id still returns int. The AC1 test changes acme to 33, issues again, and sees one fresh lookup and a mint under 33.
  • AC5 is met. The handler catches RequestFailed, the githubkit class for a status failure. It checks for 422, re-raises every other status with a bare raise, chains with from error, and leaves str(error) out. VaultSecret.get_or_mint_token passes an on_miss error up unchanged, and this PR does not change sandbox/routes.py. test_a_stale_repo_name_stops_the_token_with_the_fix pins the message and the exact two calls, so the test proves that no lookup and no retry occur.
  • AC6 is met. Against 27aef075, github.py gains one 99-column comment line above the memo, and nothing else.

Verification:

  • I reverted only services.py and kept the tests. Only the stale-repo test fails. I reverted only github.py. The three new tests fail. Checklist item 14 holds.
  • uv run pytest backend/: 2628 passed. ruff check and ruff format --check are clean. CI is green, including druks-house-rules.
  • In githubkit, Installation.repository_selection is a required Literal["all", "selected"]. The memo condition reads a field that GitHub always sends.

One low, optional remark is inline, about the message expression.

Context I could not check: GitHub answered "not found" when I tried to clone the sibling app repos, so I could not confirm that no app keeps a GitHubAppClient across several get_installation_id calls. In this repo, only core/services.py calls get_installation_id, and each of its callers makes a new client.

Comment thread backend/druks/core/services.py Resolved

@czpython czpython left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 422 handling is right and stays. Two things change: where the lookup reuse lives, and how the error message is built.

1. Move the lookup reuse out of the client

_installations_on_all_repositories is hidden state on GitHubAppClient. With it, get_installation_id("acme/gone") returns an id for a repo it never checked, and the answer of one call depends on an earlier call. It is safe only because Druks builds a new client for each call. The rule "this installation covers all repositories, so skip the lookups" is a decision about one token. It belongs in issue_token, which builds the token.

The client

  • Remove _installations_on_all_repositories and its comment. The client keeps no state.
  • Rename get_installation_id to get_installation. It returns the installation that GitHub answered, which is githubkit's parsed data. The body is the body on main, with return (await lookup()).parsed_data.
  • Do not keep get_installation_id as a wrapper. One lookup has one name.

The callers

  • get_client and revoke_tokens call get_installation and read .id.
  • issue_token decides which repos need a lookup:
installation = await app.get_installation(first)
owner, repo_name = first.split("/", 1)
names = [repo_name]
for repo in others:
    repo_owner, repo_name = repo.split("/", 1)
    # An installation is on one account, so a repo of another owner is never in it.
    if repo_owner != owner:
        continue
    if installation.repository_selection == "selected":
        try:
            await app.get_installation(repo)
        except exceptions.GitHubAppNotInstalledError:
            continue
    names.append(repo_name)
names.sort()

The first lookup keeps its GitHubAppNotInstalledError to MissingSecretError translation. Do not name a local variable name: issue_token has a name parameter.

A repo of another owner now gets no lookup. The token is the same as before, because that repo was never in the first repo's installation.

The tests

  • _TestGitHubAppClient loses the _installations_on_all_repositories attribute.
  • The fakes that replace get_installation_id in test_github_channel.py, test_sandbox_identities.py, and test_workspaces.py replace get_installation and return an object with id.
  • The calls in test_github.py use the new name and compare .id.
  • The call list of test_an_installation_on_selected_repos_is_looked_up_for_each_repo loses the acme/web lookup. The other two new tests keep their call lists.

2. Build the error message from two names

The expression splits first again for each name, and the next string starts with ". ". owner exists after change 1, so:

repos = ", ".join(f"{owner}/{repo_name}" for repo_name in names)
raise MissingSecretError(
    f"GitHub refused a token for {repos}. GitHub has no repository under one of "
    "these names. Remove the renamed or deleted repository from the project."
) from error

The message text does not change.

3. PR description

Update the second paragraph: issue_token looks up the first repo's installation one time. When that installation covers all repositories, the other repos of that owner go into the token with no more lookups. The client keeps no state.

No other changes.

@druks-ai

druks-ai Bot commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, @czpython. Accepted: the lookup reuse moves out of the client and into issue_token, and the error message is built from the owner and the names, as you suggested. Planned changes:

  • GitHubAppClient keeps no state. _installations_on_all_repositories and its comment are removed, and get_installation_id becomes get_installation, returning GitHub's parsed installation. get_client and revoke_tokens read .id.
  • issue_token looks up the first repo once. When that installation covers all repositories, the other repos of the same owner go into the token with no more lookups. For a selected installation, each same-owner repo keeps its own lookup. Repos of other owners get no lookup and stay out of the token.
  • The 422 from the token mint still becomes MissingSecretError, with the message naming the repositories and the fix.
  • Tests and fakes move to get_installation and .id. The one-line memo comment from the ticket goes away with the memo.
  • The PR description's second paragraph is updated, and the trade-off paragraph keeps the 422 wording.

I'll re-request your review once the new commit is pushed.

@druks-ai
druks-ai Bot dismissed czpython’s stale review November 10, 2026 14:18

Addressed in df6f486. GitHubAppClient keeps no state, and get_installation returns the installation. issue_token decides which repos need a lookup. The 422 message is built from owner and the repo names. The PR description says so.

@druks-ai druks-ai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: comment. Two findings to weigh. Neither blocks the merge.

The change does what the description says. I checked the diff against AC1–AC6:

  • issue_token looks up the first repo once. It skips the lookups for an all-repositories installation and keeps them for a selected one.
  • GitHubAppClient keeps no state. No get_installation_id remains in backend/.
  • get_client and revoke_tokens read .id.
  • The 422 handler catches RequestFailed, re-raises every other status, and chains MissingSecretError from the GitHub error.
  • sandbox/routes.py:45 already answers MissingSecretError with a 422 and detail=str(exc), so the box gets the fix and not a 503 that retries.

The old loop compared each repo's installation id with the first repo's id. The new loop drops that compare for a selected installation. That is safe because an App has one installation per account, so a same-owner hit is always the same installation.

Verification on 43a2b7eb: the five touched test files pass (91 tests), and ruff check / ruff format --check on backend druks-calls pass. Each of the three new tests fails if you revert the source change it covers.

Findings (inline):

  1. medium: services.py:106. The owner compare is case-sensitive. On main, a sibling repo whose owner differs from the first repo's owner only in letter case still went into the token. Now it drops out silently. Software Factory's own data makes this mix reachable.
  2. low: apis/github.py:69. get_installation lost its return annotation. AC4 asks for the matching annotation.

A note on scope: druks.core is in the house rules' public list (.druks/house_rules/config.yml, R16), so an app repo could call the old get_installation_id. I could not check that. Cloning the sibling app repos failed (not found) and org code search returned nothing. Before merging, check that no app depends on the old name.

Comment thread backend/druks/core/services.py Outdated
names.append(repo.split("/", 1)[1])
repo_owner, repo_name = repo.split("/", 1)
# An installation is on one account, so a repo of another owner is never in it.
if repo_owner != owner:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium: repo_owner != owner compares owner logins with letter case. GitHub logins ignore case. On main, the per-repo lookup put a same-owner repo into the token whatever case its owner had. Now a sibling whose owner differs only in case drops out of the token, and nothing reports it.

Software Factory's own data reaches this path:

  • get_project_repos (contrib/software_factory/workflows.py:38) builds the resources from subject.repo plus each sibling's ProjectRepo.full_name.
  • add_project_repo (contrib/software_factory/routes.py:178) stores full_name exactly as the user typed it.
  • ProjectRepo.get_for_repo and Project.get_for_repo match with func.lower because the case can differ.
  • In ReviewWorkspace, subject.repo comes from the PR webhook in GitHub's case.

Take a project whose repo is ClawHaven/acme-app and whose sibling was typed as clawhaven/acme-api. The box token leaves out acme-api. The agent's clone of that sibling then fails with an auth error that names no cause. That is the failure this PR set out to fix.

The plan lists this as an accepted risk. The paths above show it is not only theoretical. Casefolding both sides keeps the no-extra-lookup property, and the 422 message still reads correctly because GitHub ignores case:

Suggested change
if repo_owner != owner:
if repo_owner.casefold() != owner.casefold():

)

async def get_installation_id(self, owner_or_repo: str) -> int:
async def get_installation(self, owner_or_repo: str):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low: AC4's verification asks for "the matching return annotation". 43a2b7e removes the annotation instead. get_installation is now the only method of GitHubAppClient without a return type; the other 23 have one.

issue_token now branches on installation.repository_selection. The signature no longer tells a reader that this is githubkit's required Literal["all", "selected"]. from githubkit_schemas.latest.models import Installation imports in this environment. githubkit 0.16 names that path in its deprecation warning for githubkit.versions.latest.

Restore -> Installation, or record in the PR why AC4 changed.

…ts repositories

issue_token looks up the first repo's installation one time. When that
installation covers all repositories, the other repos of that owner go into the
token with no more lookups. An installation on selected repositories keeps the
lookup for each repo. A repo of another owner gets no lookup and stays out of
the token. Owner names match without regard to letter case.

GitHubAppClient.get_installation replaces get_installation_id and returns the
installation that GitHub answers. The client keeps no state.

A renamed or deleted repo of an all-repositories owner goes into the token
request, and GitHub refuses the whole token with a 422. issue_token turns that
422 into a MissingSecretError. Its message names the repos of the request and
the fix: remove the renamed or deleted repository from the project.
@druks-ai
druks-ai Bot merged commit 21510bf into main Nov 10, 2026
6 checks passed
@druks-ai
druks-ai Bot deleted the agent/DRU-1130 branch November 10, 2026 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant