Decompose GitHub resources into products - #10
Conversation
coenttb
left a comment
There was a problem hiding this comment.
BLOCKING — exact-head review of 7fb24ee40ce83ed5ed0ec549cf74f08ba0016176 against stacked base 0f49fa7f4a4ca30cde29d81f5c7b7522574cddac.
Checked evidence: PR #10 is public, OPEN, non-draft, and one commit whose parent and merge-base are the exact PR #9 head, so the 64-path diff is Task #7-only. I read the complete diff, all 36 content-identical source moves, the manifest, exports, resource source, and localized tests. The OAuth, Organizations, Repositories, Stargazers, Traffic, and Users seams, their production dependency edges, and the aggregate GitHub re-export are coherent. Task #7 remains OPEN, typed Task, assigned to the source owner, and routed to this exact-owner repository.
Changed-surface blocker: GitHub.OAuth.Client Tests.swift directly imports RFC_3986, but the newly declared GitHub OAuth Core Tests target depends only on GitHub OAuth, and the package manifest does not declare swift-rfc-3986. This relies on transitive visibility through GitHub Standard; every product a target source imports must be a direct target dependency. The smallest fix is for the Task #7 source owner to add the canonical https://github.com/swift-ietf/swift-rfc-3986.git package dependency at the same main branch and .product(name: "RFC 3986", package: "swift-rfc-3986") to GitHub OAuth Core Tests, or remove the direct module import/use through an equally direct lawful surface. Push a new head and return it for exact-head re-review.
Task-attributed evidence at this head remains useful but does not discharge the manifest defect: fresh capped Workspace package tests passed 17 tests across the six resource test targets on Apple Swift 6.4; Swift 6.3.3 strict formatting passed; package lint exited 0; and the manifest evaluation reported seven products.
Next owner: the assigned Task #7 source owner. Landing remains serialized behind PR #9 landing and Task #7 qualifying evidence after this blocker is fixed. I did not edit source, dispatch or wait on CI, consume a Workspace build lane, merge, close or unassign Task #7, or alter PR #9.
coenttb
left a comment
There was a problem hiding this comment.
CLEAN — exact-head re-review of 8976310187809b1b418f98c4fe01847bedb4e04c against stacked base 0f49fa7f4a4ca30cde29d81f5c7b7522574cddac.
Checked evidence: PR #10 is public, OPEN, non-draft, and remains stacked on the exact PR #9 head. The replacement commit is the sole direct child of the previously reviewed head and changes only Package.swift: one canonical https://github.com/swift-ietf/swift-rfc-3986.git dependency on main, plus .product(name: "RFC 3986", package: "swift-rfc-3986") on GitHub OAuth Core Tests. The complete head manifest has four distinct canonical package identities, so the replacement introduces neither a duplicate identity nor an alternate spelling. The upstream swift-rfc-3986 manifest at 690e453 exports that exact product from its current default main branch.
The complete affected-target import context confirms the direct edge is necessary and correctly attached: the client test at this head imports RFC_3986 and uses both RFC_3986.Error and RFC_3986.URI; its two sibling fixture files import only GitHub_OAuth. Task #7 remains OPEN, typed Task, assigned to the source owner, and routed to the exact owning repository.
Prior-reviewed evidence reused with provenance: review #4838469221, bound to parent 7fb24ee40ce83ed5ed0ec549cf74f08ba0016176, read the complete 64-path Task #7 diff and found the resource seams, source moves, target-local tests, production dependency edges, and aggregate re-export coherent; its sole blocker was this missing direct test dependency. Because the replacement is exactly the one-file direct child above, those clean parent findings remain applicable. The source-owner receipt at this replacement head reports manifest evaluation, strict formatting and diff checks, and fresh package evidence; I treat those as task-attributed and did not relabel them as independently originated.
Verdict: the sole direct-dependency blocker is resolved, and no replacement defect exists. Landing remains serialized behind Task #6 / PR #9 landing and Task #7 qualifying evidence. Next owner is the landing coordinator after those prerequisites; this review does not merge, alter PR #9, or change Task #7 state.
|
Temporarily closing this pull request before updating its branch ancestry so the obsolete pull-request-triggered full development matrix is not started. The ancestry-only merge will preserve the exact reviewed Task #7 tree at |
|
Reopened per Task #7 after the swift-institute/.github#186 blocker cleared (merged 2026-08-02T14:31Z). Branch ancestry was already updated onto |
coenttb
left a comment
There was a problem hiding this comment.
Independent review — head cc226a16c0acdd20d213c739287a01f9d2481862, base main @ c27af1c1ab768fbc191f3bb6431c3bc5184aa27d.
Verdict: one blocker, in documentation only. The decomposition itself is correct and needs no rework.
Blocker
README.md lines 86-87 contradict the manifest this PR ships.
No deprecated
GitHub Traffic,GitHub Stargazers, orGitHub Repositoriescompatibility products are provided.
At this head Package.swift declares library products named exactly GitHub Traffic, GitHub Stargazers, and GitHub Repositories. The sentence was true at the base commit and is false at this head, and this PR is what made it false. On a public repository the README is the front door, so this would ship a direct README-versus-manifest self-contradiction in the same commit that creates the products.
Two edits are needed:
- Replace
README.md:86-87. Those names are now first-class per-resource products, not deprecated compatibility shims. README.md:32-39documents only.product(name: "GitHub", package: "swift-github"). Per-resource products as a supported consumer choice are the outcome Task #7 asks for; a consumer reading this head cannot discover that the six exist. Add them to Installation.
The drift is invisible to CI by construction: Tests/GitHub Tests/ReadmeVerificationTests.swift is the file that would assert README fidelity, and it belongs to no declared test target, so it is never compiled or run. See adjacent debt.
Checked and clean
Decomposition. Six targets each own exactly one resource family and nothing else — OAuth, Organizations (/orgs/{org}/repos), Repositories (/repos/{owner}/{repo} and contents), Stargazers, Traffic, Users (/user, /user/emails, /user/repos). Each has its own test target, matching Task #7's stated outcome. Every production blob moved with a rename-only diff, so the split is auditable as pure relocation.
Dependency direction. Downward-only and acyclic: each leaf depends on GitHub Standard and, where used, the primitives; the GitHub aggregate depends on the six leaves; no leaf depends on the aggregate, there is no leaf-to-leaf edge, and no github.com/coenttb/* edge.
Boundaries chosen independently. Target boundaries moved; the namespace did not. All six modules continue to vend types under the single GitHub.* namespace (extension GitHub.Repository.Stargazers living in module GitHub_Stargazers). That is the right call, and it is why the split costs consumers nothing.
Composition — manifest edges match file-level imports exactly. Async Lifecycle Primitives and Either Primitives are declared on exactly GitHub Organizations, GitHub Stargazers, and GitHub Users — precisely the three targets whose sources reference Either / Async.Lifecycle, i.e. the C5 traversal modules. GitHub OAuth, GitHub Repositories, and GitHub Traffic declare GitHub Standard alone and use nothing else. No over-declaration and no under-declaration anywhere in the seven targets.
RFC 3986. Declaring swift-rfc-3986 for GitHub OAuth Core Tests corrects a previously implicit transitive import — that test's import RFC_3986 resolved only through the umbrella re-export chain. It costs no extra resolution, because swift-github-standard already depends on swift-rfc-3986 and it was in the closure already.
API surface — additive; no consumer coordination required. The GitHub product is retained under the same name, and its @_exported public import chain to the six leaves (each re-exporting GitHub Standard, plus the primitives on the traversal targets) reproduces the exact pre-PR transitive surface. I swept the declared consumers rather than inferring: every manifest that depends on this package takes .product(name: "GitHub", package: "swift-github") and nothing else, and every import site spells bare import GitHub. The downstream binding swift-github-http is representative — 15 source files, all import GitHub. No consumer names a resource module, so the new products are purely additive and no consumer-visible rename or move is introduced. That is consistent with the passing API-breakage check rather than merely asserted by it.
Typed throws and leaf errors. The ruled C5 shape is preserved. Splitting the test targets correctly forced resource-local failure leaves — GitHub.Repository.Stargazers.Fixture.Failure, GitHub.Repository.Traffic.Fixture.Failure, and GitHub.User.Fixture.Failure now serve call sites that previously borrowed GitHub.Repository.Fixture.Failure across what has become a target boundary. That is the ruling applied, not worked around.
Hygiene and freshness. Package.resolved is untracked. The base is exactly the current main, which is PR #9's merge commit, so there is no rebase drift.
Adjacent debt — pre-existing, not introduced here, do not fix in this PR
Sources/GitHub Shared/exports.swiftis an undeclared orphan whose@_exported importnames two modules (GitHub_Live_Shared,Dependencies) that this package does not depend on. This PR retired its twin — the formerSources/GitHub Repositories/exports.swift, which named the equally deadGitHub_Repositories_Live/GitHub_Shared— by promoting that directory into a real target. Retiring one of a pair and leaving the other is an incomplete sweep of a single class.Tests/GitHub Tests/,Tests/GitHub Repositories Tests/, andTests/GitHub Traffic Tests/belong to no declared test target. Worth noting for the follow-up:Tests/GitHub Traffic Tests/GitHubTrafficTests.swiftalready opens withimport GitHub_TrafficandTests/GitHub Repositories Tests/GitHubRepositoriesTests.swiftwithimport GitHub_Repositories, so this PR is what first makes those two modules exist — those directories become declarable for the first time here.Tests/GitHub Tests/ReadmeVerificationTests.swiftis the opposite case: it referencesGitHub_Types_Shared,Dependencies, and aGitHub.Clienttype, none of which exist in this package, so it could not compile if declared and needs rewriting rather than wiring.- The PR description states this PR targets
issue-6-c5. The live base ismain, and PR #9 has merged asc27af1c1. Worth refreshing so the description matches the branch it will land from.
Evidence read, and what the full tier must still prove
PR-tier run 30790473082 at this head passed Ubuntu Swift 6.3 (release), Ubuntu Swift 6.4.x nightly (release), SwiftLint, swift-format lint, swift-linter, API breakage (γ-1c), PR title format, Plan, DocC archive, and ci-ok.
It skipped macOS, Windows, every Simulator leg, Ubuntu Swift main nightly (release), and the Broken symlinks / License header (γ-1b) / Test Support dependency integrity / yamllint advisories.
Every green result at this head therefore comes from Linux, in release configuration. Before landing, the full tier has to add:
- macOS and the Simulator legs (iOS/tvOS/watchOS/visionOS), debug. The manifest declares all of these at
.v26and none has been exercised at this head. A six-way target split changes module partitioning and linking, which is exactly the class of change that diverges per platform. - Windows. The highest-risk leg for this change specifically: every new module name contains a space and resolves through an underscored module identifier, and the link graph went from one product to seven.
- Ubuntu Swift main nightly (release) — the only main-nightly leg, and it skipped.
- A per-leg test count at this head. The description's 17-test local run was at
7fb24ee4, an earlier commit, on one platform. This is the single most valuable thing the full tier can produce here: a test target that is mis-declared after a split does not fail, it silently does not run, and a green build will not say so. The run must show all six resource test targets executing, with the total accounting for every test that lived in the formerGitHub Core Tests. - Test Support dependency integrity, which skipped and is directly on-topic for a change that rewrites every target's dependency block.
No landing authority was exercised and no other mutation was made; the PR is left open.
coenttb
left a comment
There was a problem hiding this comment.
Delta re-review — head fdf6637c58b24520afd897898c2145739a24a032, base main @ c27af1c1ab768fbc191f3bb6431c3bc5184aa27d (unmoved).
Verdict: clean. The blocker from my review at cc226a16 is resolved, the secondary finding is resolved, and the fix introduces nothing.
Scope of this re-review
The delta from the previously reviewed head is README.md alone, 26 insertions and 2 deletions, in one commit (fdf6637c, Document per-resource products; correct product availability note). I confirmed the restriction rather than assuming it: no manifest, source, or test blob differs between cc226a16 and this head. Every finding I cleared in the prior review therefore carries forward untouched, and this pass is scoped to the README.
Blocker resolved
The false assertion at the old README.md:86-87 is gone. It now reads that GitHub Traffic, GitHub Stargazers, and GitHub Repositories are first-class per-resource products a consumer can depend on directly, and are not deprecated compatibility shims. That is true at this head and it preserves the original sentence's intent — distinguishing these from the retired shim generation — instead of merely deleting the claim.
Secondary finding resolved
Installation now documents the per-resource products, so the outcome Task #7 asks for is discoverable from the README:
- The Products table gained six rows. I cross-checked it against
Package.swiftrather than reading it for plausibility: the table's seven rows correspond one-for-one to the seven declared.libraryproducts, with none missing and none invented. - The six module identifiers (
GitHub_OAuth,GitHub_Organizations,GitHub_Repositories,GitHub_Stargazers,GitHub_Traffic,GitHub_Users) are the spellings SwiftPM actually derives from these space-containing target names, corroborated by the@testable importlines in the six test targets that compile green at this head. - Each Purpose cell matches the target's actual contents: OAuth ships authorization and token exchange; Repositories ships lookup and content retrieval; Users ships authenticated-user profile, emails, and repository listing; Organizations and Stargazers each ship a listing plus its bounded traversal; and
GitHub.Repository.Traffic.Clientdoes expose exactlyviews,clones,paths, andreferrers. - The added dependency sample spells
.product(name: "GitHub Traffic", package: "swift-github")— a real product name against the package's repo name, so it is copy-pasteable as written.
Nothing introduced
The 26 added lines are a table extension, one fenced sample, and three prose sentences. No claim in them is unsupported by the manifest or the sources at this head, and the umbrella-first Installation guidance is retained ahead of the per-resource alternative, so the default consumer path is unchanged.
Evidence at this head
All checks terminal, with zero failures and zero still running: 11 passed — Ubuntu Swift 6.3 (release), Ubuntu Swift 6.4.x nightly (release), SwiftLint, swift-format lint, swift-linter, API breakage (γ-1c), PR title format, Plan, DocC archive, and both ci-ok gates — and 9 skipped, an identical skip set to the previously reviewed head.
Unchanged from my prior review
The pre-landing gate stands as recorded there, because a README-only commit cannot discharge it: the full tier still has to add the macOS and Simulator legs, Windows, Ubuntu main-nightly, a per-leg test count at this head confirming all six resource test targets actually execute, and Test Support dependency integrity. The adjacent debt likewise stands and remains explicitly out of scope for this PR — the Sources/GitHub Shared/ orphan and the three undeclared test directories.
No landing authority was exercised and no other mutation was made; the PR is left open.
|
Independent verification (separate lane, read-only, posted by coordinator session f2c02446): CLEAN at fdf6637, no update-branch needed (behind 0; prerequisite PR #9 merged and this PR retargeted to main). Matches the ruled per-resource shape: six products/targets, acyclic downward graph, typed leaf errors on moved code. Advisory, adjudicated non-blocking: the aggregate keeps the pre-existing @_exported umbrella convention (extended, not introduced here) — recorded on Task #7 for the family-level decision. Landing via the uniform transaction; #7 stays open pending its own qualifying full-tier evidence. |
Implements the per-resource product/target decomposition from Task #7 as a one-commit stack on PR #9 at its exact independently reviewed head
0f49fa7f4a4ca30cde29d81f5c7b7522574cddac.GitHubas the aggregate;Verification at
7fb24ee40ce83ed5ed0ec549cf74f08ba0016176:workspace package test --package-path . --fresh --jobs 4passed all 17 tests with Apple Swift 6.4;swift-format lint --strict --ignore-unparsable-filespassed across the complete changed Swift surface;workspace package lint --package-path .exited 0 with 37 advisory findings.This PR intentionally targets
issue-6-c5, so its review diff contains only Task #7. Landing remains gated on PR #9 landing first and Task #7 obtaining its own qualifying evidence; this PR does not close Task #7.