Repository navigation
Conversation
The prepared GitHub owner read and parsed repository.preparedGitHub.privateKeyPath only at its first token preparation, so a missing, public, encrypted or non-RSA key let the worker report ready while every maintenance pass logged PREPARED_GITHUB_MAINTENANCE_FAILED without a reason (finding 2217). The owner now reads and parses the key the way issuance does before it starts, zeroes the file bytes, and refuses with PreparedGitHubAppKeyError. The worker logs PREPARED_GITHUB_APP_KEY_INVALID with a fixed reason class (app_key_unreadable, app_key_unparsable, app_key_unsupported), never key bytes or the path. Maintenance still re-reads the key, so a rotated Secret applies. The chart's upgrade check mounts no private key and is unchanged.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Add a breaking-changes entry: a bad App key now stops the worker, which the upgrade check cannot catch. Give the runbook its own section for the refusal and note that a key that goes bad at runtime is still read only at the next preparation. Mention the code in the controller reference. Close the signing key if the key-version export throws, note why the signer's refusal can only be the key type, reserve socket room for the longest case's directory, and check stderr for any PEM header.
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs changes before merge. What this changesThis PR validates the GitHub App key during worker startup and reports a fixed diagnostic before readiness when validation fails. Example: The prepared-Git worker mounts an EC P-256 private key instead of an RSA App key.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness⛔ Needs changes before merge - 1 item remains Keep this PR open: it addresses a current startup failure, and its maintainer author explicitly accepts stopping the worker when its App key is unusable. Priority: P2 Before merge
Findings
Tests
Agent review detailsHow this fits togetherThe controller worker owns prepared GitHub credentials, reads its private App-key mount, and uses the existing signing owner to prepare repository material for authorized Agent revisions. flowchart TD
A[Worker startup] --> B[Load repository configuration]
B --> C[Validate mounted App key]
C --> D[Fixed startup refusal]
C --> E[Start prepared credential owner]
E --> F[Reload key during preparation]
Technical reviewBest possible solution: Retain validation in the existing prepared credential owner, document its startup ordering, and establish that an existing Installation can upgrade and recover with its settings intact. Do we have a high-confidence way to reproduce the issue? Current-main source establishes that startup can finish without reading the App key and that preparation later encounters the invalid file; the author reports a base-failing regression test, which this read-only review did not execute. Is this the best way to solve the issue? Reusing the issuer's existing key restrictions at startup is a focused repair that preserves worker-only custody and rotation behavior; runtime maintenance diagnostics remain an explicitly separate follow-up. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against a651091ac5c4. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceAcceptance criteria:
What I checked:
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 22, 2026, 10:35 PM ET / October 23, 2026, 02:35 UTC. |
Problem
The worker read and parsed
repository.preparedGitHub.privateKeyPathonly when it first prepared a token. A missing file, a public key, a passphrase-protected key or a non-RSA key in the App-key Secret let the worker report ready. Every maintenance pass then loggedPREPARED_GITHUB_MAINTENANCE_FAILEDwith no reason, and revisions waited for Git material that never arrived (finding 2217).Change
startPreparedGitHubGitOwnernow reads the App key before it starts. It parses the key the way issuance does: a private regular file, an unencrypted private key, RSA of 2048 to 8192 bits. It zeroes the file bytes afterwards. A key that cannot sign throwsPreparedGitHubAppKeyError. Maintenance still reads the key again on each pass, so a rotated Secret still applies.worker.startup-errorwith codePREPARED_GITHUB_APP_KEY_INVALIDand one of three fixed reasons:app_key_unreadable,app_key_unparsableorapp_key_unsupported. The log never contains key bytes or the path.breaking-changes.mdhas a 2026-10-23 entry: a bad key now stops the worker, and the upgrade check can't catch it.Verification
New test in
configuration-startup: it boots the real worker against an unreachable database. It covers a valid key, which gets past the owner toPERSISTENCE_UNAVAILABLE, and five bad keys:Each bad key asserts the code and reason, and that stderr contains neither PEM text nor the key directory. On main the test fails: the bad keys reach
PERSISTENCE_UNAVAILABLE.qa-repository-installationno longer checks the mounted key by hand. Starting the owner now does that check.Locally (Node 24): the whole
configuration-startupandqa-repository-installationfiles pass (40/40). The two touched tests passed 20/20 at 2 CPUs on the final head.tsc --build, eslint, Prettier anddocs:check(word limit, links, specs) are clean.Internal review: approve with nits. All applied:
Follow-up, not done here: give
PREPARED_GITHUB_MAINTENANCE_FAILEDa reason when a key goes bad at runtime. That needsTokenServiceto pass the error through.Risks