fix(connect): refresh relay credentials before expiry - #9178

💭 I see the previously flagged error in pullRequestDiffHttp.ts has already been fixed by the author, using the proper tagged error class with repository, number, traceId, and cause fields. The environmentHttp.ts change seems to have been reverted, so I'm now focusing on reviewing the current diff for any new violations, starting with the pull request diff controller. Looking at PullRequestDiffCredentialRejectedError, its message hardcodes "(invalid_credential)" as a static string rather than deriving it from stored attributes, but since this mirrors a pattern the author already justified elsewhere in contracts, it's probably not worth re-flagging. I'm also considering whether the mapError call that branches on a _tag check against "EnvironmentAuthInvalidError" should instead use Effect.catchTags for handling statically known tagged failures, since that's the convention for catching rather than mapping. I want to verify this by checking the actual file and the related supervisor.ts changes—specifically how waitForAuthorizationRefresh uses Effect.fnUntraced and Clock, and whether the Clock import follows convention. Let me pull up these files to confirm.

1. browse_code — packages/client-runtime/src/state/pullRequestDiffHttp.ts

2. git_tools — packages/client-runtime/src/connection/supervisor.ts 5392c9b..62480b0

💭 I'm checking whether the supervisor's error convention applies here, and considering whether using a constant message instead of deriving it from structured attributes counts as a violation of the guidance to build error messages from stable structural fields rather than storing an unstructured message alone. Since the message is static prose but the error still carries structured attributes, this doesn't seem like a real violation, and the "(invalid_credential)" literal issue was already addressed elsewhere. I want to look at what actually changed between the previously reviewed commit and the current head, including the supervisor test, the service.ts update adding expiresAtEpochMs, and the constant naming in connection/model.ts, to confirm no new convention issues were introduced.

3. git_tools — 3bb8d45..62480b0

💭 I'm checking whether the inline error mapping follows the boundary construction convention, which it does, and confirming that hardcoding the "(invalid_credential)" reason in the error message is acceptable since that error class exists specifically for this case. The required cause field on the environment auth error looks correct, and the supervisor test addition for the behavior change satisfies the convention. The union type used for the diff load error is fine given RemoteEnvironmentRequestError may already be a type union, so now I want to check whether any consumers of PullRequestDiffLoader.load need updates to handle this new error case, particularly if they use catchTag or catchIf patterns.

4. git_tools — PullRequestDiff(Loader|CredentialRejectedError|LoadError)

💭 I'm checking pullRequests.ts re-exports look fine, and confirming supervisor.ts follows existing conventions like using Effect.fnUntraced for helper functions inside make. I'm also considering whether the mapError ternary versus catchTags convention applies here, since the existing guidance addresses catching errors specifically rather than mapping them. Since this seems like a borderline stylistic preference rather than a real violation, I'm inclined to skip flagging it. I'm also verifying that moving DPOP_ACCESS_TOKEN_REFRESH_SKEW_MS to connection/model.ts makes sense as a shared constant, and that the test file's use of TestClock follows existing patterns. Now I'm looking more closely at the raceAllFirst logic in supervisor.ts to check whether waitForAuthorizationRefresh returning true on success correctly terminates the race as intended. I don't think I need to resolve that earlier comment thread since it's optional and I'd need the node ID anyway—I'll leave it as is. I want to double check that every place constructing the Dpop variant of PreparedHttpAuthorization now supplies the new expiresAtEpochMs field, so I'm searching for those construction sites to make sure nothing breaks type-checking.

5. git_tools — _tag: "Dpop"

💭 All construction sites look updated now, and I don't see any new convention violations. The cached expiration path in authorization/service.ts checks out, and the tests here actually validate behavior rather than just proving a mechanical refactor, so that's fine too. Since there are no findings, this should be a success state — the check defaults to failure only when findings exist, so with everything clear I'll report success with "All clear" as the summary.