Reconcile authentication cleanup with current main - #13
Conversation
coenttb
left a comment
There was a problem hiding this comment.
Independent exact-head review of 599df5a66ef6fe82cdd2392f4f1981e0ae6a3101: this must remain draft and open.
Blocking findings:
-
Identity.Consumer.Configuration.Consumer.liveis not source-compatible with currentmain. The existing public function is callable asConsumer.live(...); this head replaces it with a metatype-valued property, and the new test callsConsumer.live.init(...). That test proves the changed spelling rather than preserving the public one. Restore the callable function surface (or an actually source-compatible equivalent) and compile the establishedConsumer.live(...)call in the active test target. -
The cookie tests exercise only the low-level
Server.Response.settinghelper with the access-token name. They do not cover the changed access-plus-refreshwithTokenspath, reauthorization-cookie path, or three-cookie logout expiry path, so the intended error propagation at the changed call sites is still unproved. Add focused active-target success and encoding-failure tests for those paths, or restore any incidental behavior change. -
The new active test imports
Identity_Frontend,Server,Server_Vapor, andURLRouting, but its target does not declare those products as direct dependencies. The test must not rely on transitive imports. -
Exact-head CI is terminal red on both required compiler gates. Swift 6.3 aborts while optimizing the Records dependency on an abstract pack conformance; Swift 6.4 nightly asserts in the structured-queries constraint solver. These failures occur before changed authentication source is diagnosed, so they are external dependency/toolchain blockers rather than evidence of an authentication regression, but both required gates remain unresolved and cannot be bypassed.
Checked positives: the commit is directly based on current main; the replacement is one human-authored commit without prohibited co-author trailers; overlapping main content is preserved; the formatting configuration matches the foundations cohort; EmailAddress trap semantics are preserved; no package dependency, platform, workflow, security, or compiler-gate declaration changed; and the public exposure scan is clean.
|
Provenance: the prior Workspace-mediated IdentityBehavioralTests run executed all 6 tests; 3 passed before the process stopped on: [swift-witnesses] Test default served in LIVE context: key Configuration. Source inspection showed withTokens(for:) resolves Identity.Frontend.Configuration, while the first attempt overrode Identity.Consumer.Configuration in a different test. The pushed correction provides the frontend configuration in that test context and is awaiting CI. The external Swift 6.3 Records optimizer and Swift 6.4 structured-queries solver gates remain required and red. |
coenttb
left a comment
There was a problem hiding this comment.
Independent exact-head review of 9db5bb78e9c73cdc380d46d21f8cdfd939e1f2a0: this head remains blocked.
The added test named High-level token cookie encoding failures propagate still calls the low-level Server.Response.setting(...) helper directly. It therefore does not exercise error propagation through either changed high-level call site (withTokens(for:) or the reauthorization response path), which was the missing behavior from the prior review. The new success tests show that access-plus-refresh and logout cookies render, but they cannot establish that a high-level caller preserves an encoding failure.
Please drive an invalid token through withTokens(for:) under Identity.Frontend.Configuration, or through an actual high-level reauthorization response path, and assert that the typed encoding failure escapes. Keep the required Swift 6.3 Records optimizer and Swift 6.4 structured-queries solver gates binding.
Checked at this head: the callable Consumer.live(...) spelling is restored; the test target declares its direct imports; the new commit is directly atop the previously reviewed head; and the PR remains directly based on current main. The recorded focused six-test run passed the first three tests, then stopped on the wrong dependency override. This head corrects that override in source, but no post-correction local run was performed for this review. Because the source blocker is decisive, exact-head Actions were not rerun, canceled, or enumerated.
|
Addressed the blocker in review 4815681863: the failure case now supplies a valid access token and an invalid refresh token through withTokens(for:) under explicit Identity.Frontend.Configuration, so the changed high-level access-plus-refresh path must propagate the encoding error. No local build, test, or lint was run; CI is authoritative for this head. |
coenttb
left a comment
There was a problem hiding this comment.
Independent exact-head re-review of 1b360be90d7f3a45879f8c56b301587b70e758cf: this head remains blocked.
The prior high-level error-propagation finding is cured: the corrected test supplies an invalid refresh token through withTokens(for:) under explicit Identity.Frontend.Configuration.
The exact-head swift-format lint gate is terminal red. It reports Tests/Authentication Router Parity Tests/Identity Behavioral Tests.swift:2:1 [OrderedImports]: import statements must be lexicographically sorted. Move import Dependencies before import Foundation, then push a new head.
The required Swift 6.3 Records optimizer and Swift 6.4 structured-queries solver gates remain binding. Both were still running when this terminal format failure fixed the verdict, so they were not watched further and are not waived.
Checked at this head: PR #13 is open and draft on current main; Issue #11 is open; the complete three-file surface was reviewed; the callable API and direct test dependencies remain corrected; and the new commit is directly atop the previously reviewed head. No local build, test, or lint was run.
|
Addressed the OrderedImports blocker in review 4815721816: Dependencies now precedes Foundation in the behavioral test. All behavior and test coverage are preserved; no local build, test, lint, or format command was run. |
|
Author-side CI evidence for head 61e0574: the monitored non-compiler checks are terminal with swift-format lint passing, swift-linter passing, and SwiftLint failing on one serious existing function-parameter-count violation in Identity.Consumer.Client.Email.live.swift. Compiler jobs, including the required Swift 6.3 and Swift 6.4 gates, remain owned by their peer lanes and were not assessed here. CI run is authoritative. |
|
Author-side diagnosis for CI head 61e0574: SwiftLint reports one function_parameter_count violation in Identity.Consumer.Client.Email.live.swift. This PR changes only import ordering in that file; the flagged seven-parameter function is unchanged, so fixing it would broaden the preserved three-file patch. No out-of-scope source change or new head was pushed. swift-format and swift-linter pass; compiler jobs remain peer-owned. |
coenttb
left a comment
There was a problem hiding this comment.
Independent exact-head review of 61e05747dba996470705e5247761eeadd868cc57: the reviewed source surface is clean, but landing remains blocked by a required external compiler gate.
Checked at this head:
- The import-order correction is exact, and
swift-format lintis terminal green. - The accepted three-file behavior surface is otherwise unchanged: callable
Consumer.live(...), direct test dependencies, and high-level cookie success/error propagation remain intact. - The head is four commits ahead and zero behind current
main; PR #13 remains open/draft and Issue #11 remains open. - Swift 6.4 nightly is terminal red while compiling
swift-structured-queries-primitives, before changed authentication source is diagnosed.swift-frontendasserts inConstraintSystem::addKeyPathApplicationRootConstraintinCSSimplify.cpp. This is the known external structured-queries constraint-solver failure, not evidence of an authentication regression, but the required gate cannot be bypassed.
Swift 6.3 was still running when the terminal 6.4 failure fixed the verdict. It remains binding and is not waived; prior-head Records optimizer failures are provenance only, not substituted for exact-head evidence. No local build, test, or lint was run, and no CI rerun or cancellation was performed.
Closing — 2026-07-30: superseded on main, with the remainder extracted to #20Reconciled against current
Why this cannot simply be rebased and landed
The one remaining prose difference, two The remainder
Issue #11 stays open: its proposed outcome is that every branch commit is durable on Closing as superseded. The branch |
Recover the six cookie-behavior tests from the closed PR #13 branch (issue11-clean-replacement-20260729) and revalidate them against current main rather than cherry-picking blind, per #20. Two assertions changed shape: setting(cookie:token:configuration:) now converts an encoding failure into a preconditionFailure instead of throwing, so the two encoding-failure tests are re-derived against HTTPCookies.SetCookie.headerValue(), the throwing primitive that function wraps, rather than against the non-throwing wrapper itself. The other four (Consumer.live spelling, valid-token rendering, withTokens(for:) cookie count, expiringIdentityCookies() cookie count) carry over unchanged in substance. Adds Identity_Frontend, Dependencies, Server, Server_Vapor, and HTTP_Cookies as test-target dependencies to support the new coverage.
Summary
Clean replacement for #12, rebuilt directly from current
mainto reconcile Issue #11 without propagating the superseded branch history.Consumer.live(...)construction spelling through the public metatype plus initializerAddresses Issue #11.
Supersedes #12 after preserving its branch, review, and history. Independent review: #12 (review)
Required Swift 6.3 and Swift 6.4 compiler/solver gates remain external blockers; no gates were weakened.