Repository navigation
DRU-1130 -- druks: a build's clone fails with 503 when its project has many repos - #1163
Conversation
13c14c4 to
27aef07
Compare
There was a problem hiding this comment.
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_tokenmakes one lookup for threeacmerepos and mints for all three names. - AC2 is met.
test_an_installation_on_selected_repos_is_looked_up_for_each_repolooks up eachpaulorepo and dropspaulo/private. It also dropsacme/apithrough the cached path: the cached id 11 is compared with 22, not added blindly. - AC3 is met.
services.pyis untouched, andtest_a_box_token_names_the_repos_its_first_repos_installation_coversis unchanged and passes. - AC4 is met. The state is set in
__init__, and the method still returnsint. The first test re-issues afteracmemoves to 33 and sees one fresh lookup and a mint under 33.
Verification:
- I reverted
github.pytomainand 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 checkandruff format --checkare clean. repository_selectionis a requiredLiteral["all", "selected"]on githubkit'sInstallationmodel, so reading it with no fallback is safe.- Code search found no use of
GitHubAppClientorget_installation_idin 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.
| 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): |
There was a problem hiding this comment.
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.
ProjectReporows 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:13tells 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.
czpython
left a comment
There was a problem hiding this comment.
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_tokenleft 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
- When GitHub refuses the installation token with a 422,
Github.Authentication.issue_tokenraisesMissingSecretError. 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 ofMissingSecretError: no secret until a person acts, and the message names the fix. The secret route then answers 422 with that reason. Follow the patternissue_tokenalready uses forGitHubAppNotInstalledError. This changesbackend/druks/core/services.py; the plan's "leaveservices.pyuntouched" no longer applies. - 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. - Add a one-line comment on
_installations_on_all_repositoriesthat 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. - 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.
|
Thanks for the report. The lookup reuse stays. Here is what will change for the stale-name case:
This supersedes the earlier plan note that |
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.
bb5f993 to
f4cb4b3
Compare
There was a problem hiding this comment.
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_tokenmakes one lookup for threeacmerepos and mints for all three names. - AC2 is met.
test_an_installation_on_selected_repos_is_looked_up_for_each_repolooks up eachpaulorepo and dropspaulo/private. It also drops theacmerepos, and the memo path compares id 11 with 22 before it adds a name. - AC3 is met. Against
92ebaeb9,services.pygains only theRequestFailedimport and thetry/exceptaroundget_or_mint_token. Theon_misspartial only wraps to the new indent.test_a_box_token_names_the_repos_its_first_repos_installation_coversis unchanged. - AC4 is met.
__init__sets the memo, andget_installation_idstill returnsint. The AC1 test changesacmeto 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 bareraise, chains withfrom error, and leavesstr(error)out.VaultSecret.get_or_mint_tokenpasses anon_misserror up unchanged, and this PR does not changesandbox/routes.py.test_a_stale_repo_name_stops_the_token_with_the_fixpins the message and the exact two calls, so the test proves that no lookup and no retry occur. - AC6 is met. Against
27aef075,github.pygains one 99-column comment line above the memo, and nothing else.
Verification:
- I reverted only
services.pyand kept the tests. Only the stale-repo test fails. I reverted onlygithub.py. The three new tests fail. Checklist item 14 holds. uv run pytest backend/: 2628 passed.ruff checkandruff format --checkare clean. CI is green, includingdruks-house-rules.- In githubkit,
Installation.repository_selectionis a requiredLiteral["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.
czpython
left a comment
There was a problem hiding this comment.
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_repositoriesand its comment. The client keeps no state. - Rename
get_installation_idtoget_installation. It returns the installation that GitHub answered, which is githubkit's parsed data. The body is the body onmain, withreturn (await lookup()).parsed_data. - Do not keep
get_installation_idas a wrapper. One lookup has one name.
The callers
get_clientandrevoke_tokenscallget_installationand read.id.issue_tokendecides 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
_TestGitHubAppClientloses the_installations_on_all_repositoriesattribute.- The fakes that replace
get_installation_idintest_github_channel.py,test_sandbox_identities.py, andtest_workspaces.pyreplaceget_installationand return an object withid. - The calls in
test_github.pyuse the new name and compare.id. - The call list of
test_an_installation_on_selected_repos_is_looked_up_for_each_repoloses theacme/weblookup. 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 errorThe 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.
|
Thanks, @czpython. Accepted: the lookup reuse moves out of the client and into
I'll re-request your review once the new commit is pushed. |
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.
There was a problem hiding this comment.
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_tokenlooks up the first repo once. It skips the lookups for an all-repositories installation and keeps them for a selected one.GitHubAppClientkeeps no state. Noget_installation_idremains inbackend/.get_clientandrevoke_tokensread.id.- The 422 handler catches
RequestFailed, re-raises every other status, and chainsMissingSecretErrorfrom the GitHub error. sandbox/routes.py:45already answersMissingSecretErrorwith a 422 anddetail=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):
- medium:
services.py:106. The owner compare is case-sensitive. Onmain, 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. - low:
apis/github.py:69.get_installationlost 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.
| 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: |
There was a problem hiding this comment.
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 fromsubject.repoplus each sibling'sProjectRepo.full_name.add_project_repo(contrib/software_factory/routes.py:178) storesfull_nameexactly as the user typed it.ProjectRepo.get_for_repoandProject.get_for_repomatch withfunc.lowerbecause the case can differ.- In
ReviewWorkspace,subject.repocomes 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:
| 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): |
There was a problem hiding this comment.
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.
43a2b7e to
ce06cee
Compare
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 clonefailed with a 503.Now
issue_tokenlooks 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_tokenraisesMissingSecretError, 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)
issue_token.GitHubAppClient. The hidden state_installations_on_all_repositoriesmakesget_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 inissue_token.ownerand the repo names, not by splittingfirstagain for each name. The message text does not change.What exists at HEAD (
f4cb4b37)backend/druks/core/apis/github.py:GitHubAppClient.__init__setsself._installations_on_all_repositoriesunder a one-line comment.get_installation_idreturns an int, reads the memo first, and writes it after a lookup reportsrepository_selection == "all". Onmain(92ebaeb9) the method has no memo and ends each lookup withreturn (await lookup()).parsed_data.id.backend/druks/core/services.py:issue_tokencompares each other repo'sget_installation_idwith the first repo's id, undersuppress(GitHubAppNotInstalledError). It wrapsVaultSecret.get_or_mint_tokeninexcept RequestFailed, re-raises non-422 with a bareraise, and raisesMissingSecretError(...) from errorfor 422. The message rebuilds each name withfirst.split('/', 1)[0] + '/' + repo_name.get_client(~line 180) andrevoke_tokens(~line 194) callget_installation_id.get_installation_idare only test doubles and tests (see Tests below). No docs mention it.backend/druks/sandbox/routes.pyalready answersMissingSecretErrorwith 422 anddetail=str(exc); every other exception answers 503. It needs no change.backend/tests/test_managed_github.py:_GitHubKit._mintrefuses names inDELETED_REPOSITORIES = ("gone",)with_refused(422)after recording thePOST. 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.services.pydoes 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 stateself._installations_on_all_repositoriesand the comment line above it.__init__is back to itsmainshape.get_installation_idtoget_installation(self, owner_or_repo: str). The body is the body onmain, except that the successful lookup returns(await lookup()).parsed_data. The return type is githubkit's parsed installation model; confirm the import path afteruv sync. Delete the memo read and the memo write. The org-to-user fallback, the 404 continue, the non-404 re-raise, andGitHubAppNotInstalledError(owner_or_repo, app=self._slug)stay as onmain.get_installation_idas a wrapper. One lookup has one name.2.
backend/druks/core/services.pyissue_token. Keep the empty-resources guard, the first-lookupexcept exceptions.GitHubAppNotInstalledError as error: raise MissingSecretError(str(error)) from error,names.sort(), theAudience.resource(...)shape,on_miss=functools.partial(app.create_installation_token, ...), and theRequestFailedimport. Replace the id-comparison loop with:Use
installation.idwhereinstallation_idwas used (Audience.resource(...)and theon_misspartial). Do not name a local variablename:issue_tokenhas anameparameter.suppressis still used elsewhere in the module, so its import stays.except RequestFailed, thestatus_code != 422bareraise, andraise ... from error. Build the message fromownerandnames:The message text is unchanged. Keep
repo_nameas the generator variable, notname. Do not formatstr(error).get_clientandrevoke_tokens. Callget_installation(owner_or_repo)and read.id.3. Tests
backend/tests/test_github.py:_TestGitHubAppClientlosesself._installations_on_all_repositories = {}. The three calls (~lines 264, 279, 293) useget_installation; the two successful ones compare(await ...).idwith 42 and 41. Restore the two fixtures to theirmainshape (SimpleNamespace(id=42),id=41): the client no longer readsrepository_selection.backend/tests/test_github_channel.py:35,backend/tests/test_sandbox_identities.py:447,backend/tests/test_workspaces.py:174and:196: the fakes replaceget_installation(themonkeypatch.setattr(GitHubAppClient, "get_installation", ...)name changes too) and return an object withid, for exampleSimpleNamespace(id=7). Those tests issue tokens for one repo, sorepository_selectionis never read; add it only if a test passes several repos of one owner.backend/tests/test_managed_github.py: intest_an_installation_on_selected_repos_is_looked_up_for_each_repo, remove"GET repos/acme/web/installation"from the expected calls. The remaining calls areGET repos/paulo/site/installation,GET repos/paulo/blog/installation,GET repos/paulo/private/installation, thenPOST 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 (oneGET, then the mint).DELETED_REPOSITORIESand the_mintrefusal 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 "NowGitHubAppClient.get_installation_id…") with: "Nowissue_tokenlooks 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 withgh pr edit 1163 --body-file <file>.Implementation verification (separate from the acceptance criteria)
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, thenuv run ruff check backend druks-calls,uv run ruff format --check backend druks-calls, anduv run pytest backend/.services.py, keep the test, confirm it fails, then restore.issue_token(keep the tests) and confirm the all-owner test fails on the lookup count; then restore.druks-house-rules check --base origin/mainif installed.uv run --locked pytest; report the exact blocker if it cannot run.npm --prefix frontend run lint,npm --prefix frontend run build,npm --prefix frontend test, andnode --test backend/tests/chat/bridge.test.mjswhen executable here. Otherwise report each exact command and blocker asnot_run; these are not hard gates for this backend diff.Acceptance Criteria
nameis 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.grep -rn "get_installation_id\|_installations_on_all_repositories" backendfinds 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.repos = ", ".join(f"{owner}/{repo_name}" for repo_name in names)using theownerbound 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 diffshows backend/druks/sandbox/routes.py unchanged.uv run pytest backend/passes, anduv run ruff check backend druks-callsanduv run ruff format --check backend druks-callspass.gh pr view 1163 --json bodyshows 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.