test: prove transport isolation and device-code cancellation contracts - #64
Conversation
Adds scripts/check-transport-isolation.sh to make verify so PutioSDK.request stays caller-isolated and no @Concurrent body reads self members or bare config/delegate. Delegate-callback tests assert delivery off the caller's actor. Device-code cancellation tests now pin the sleep and post-poll cases deterministically by observing URLSession task completion and holding the owning actor, replacing the in-flight cancel that only exercised the URLError.cancelled path.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The new ActorHold synchronization helper can silently time out and proceed, which can make the added cancellation proof flaky or false-positive under slow/failed coordination.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds executable guards and stronger concurrency/cancellation proofs around the transport isolation contract (#52) and device-code polling cancellation semantics, ensuring regressions can’t slip through Swift 5 language mode compilation.
Changes:
- Introduces a
make verifystructural audit that enforces transport isolation rules inPutioSDK.swift(no@concurrentrequest, no off-actor reads ofself./config/delegateinside@concurrentbodies). - Strengthens tests to assert delegate callbacks occur off the caller actor (using
@MainActor+ “not main thread” proxy). - Reworks device-code cancellation tests to ensure cancellation is checked post-poll (not just normalized
URLError.cancelled), using a session task completion observer plus actor-based coordination.
File summaries
| File | Description |
|---|---|
| Tests/PutioSDKTests/TransportSupport.swift | Adds URLSession task-completion observer support for more precise cancellation test timing. |
| Tests/PutioSDKTests/PutioSDKTransportTests.swift | Adds a test that asserts delegate callbacks arrive off the caller actor/thread. |
| Tests/PutioSDKTests/PutioSDKAuthTests.swift | Reworks device-code cancellation tests to prove post-poll cancellation behavior and adds off-actor delegate callback assertion. |
| scripts/lib/check-transport-isolation.py | Implements the structural scan enforcing forbidden reads inside @concurrent bodies. |
| scripts/check-transport-isolation.sh | Adds a make/CI-invoked wrapper to run the transport isolation audit. |
| PutioSDK/Classes/PutioSDK.swift | Renames the cross-hop delegate parameter to prevent bare delegate usage inside @concurrent helpers. |
| Makefile | Wires the new transport isolation audit into make verify (and adds a dedicated target). |
| docs/TESTING.md | Documents the new verify step and what it enforces. |
| docs/ARCHITECTURE.md | Documents the structural enforcement of the transport isolation contract. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a9c07988a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… seam The audit now blanks Swift comments and string literals before locating @Concurrent bodies, recognises the attribute regardless of line layout, checks the opening-brace line, requires both perform and execute to stay @Concurrent, and is exercised by pass/fail fixtures. Device-code cancellation tests park the SDK at its pollCompleted/willSleep boundaries via an internal observer and suspend instead of blocking cooperative threads.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbb8e05bd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…poll sleep The audit lexes interpolation expressions with the same comment and string rules as top-level code and exempts only true argument-label positions, so ternary operands are audited. The interval sleep goes through an internal sleeper seam; the sleep test cancels once the SDK is suspended inside it and a pre-checked uncancellable delay now fails the elapsed bound.
…oll sleep The isolation audit recognises self?. and self!. member access and blanks extended regex literals before brace tracking. A new test releases the SDK into the real Task.sleep and cancels it, so an uncancellable default sleeper trips the elapsed bound.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 478bc08bd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Every perform/execute declaration must be @Concurrent so an unannotated overload cannot slip past, and member access through a dot with whitespace or a comment is no longer misreported. Test rendezvous waits now expire after five seconds inside the owning actor, so a silent observer fails loudly instead of hanging the suite.
… sleep entry The isolation audit drops backtick identifier escapes so an escaped self or config is audited, and honours backslash escapes inside extended regex literals. The production-sleep test wraps the named sleep primitive with a synchronous entry signal and cancels only once entry is observed.
The audit flags every bare self token in @Concurrent bodies so a parenthesised receiver cannot read state. Sleep tests now bound the response to cancellation rather than total elapsed time, and two new tests exercise the shipped default sleeper directly and through the polling loop.
…ezvous failure The shipped sleep primitive takes a synchronous entry hook, so both the direct test and the default-loop test cancel only after the SDK is at the real Task.sleep. Rendezvous cleanups cancel the polling task as well as releasing the barrier. The audit's metatype exemption is whitespace-aware.
The loop sleeps only via deviceCodePollClock, so a test clock can observe entry from inside the suspension and prove cancellation lands during an active sleep, while a spy clock forwards to ContinuousClock so the real primitive is cancelled after observed entry. Replaces the sleeper and entry-observer seams.
…se coverage A member-access exemption no longer matches the trailing dot of ... or ..<, so a config read after a range operator is rejected. The spy-clock test's comments and docs now say it bounds cancellation response only; the observable-clock test carries the deterministic mid-sleep proof.
…bels in the audit
…pressions in the audit
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7deb9cf091
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if source[j] == "\\" and j + 1 < n: | ||
| # An escaped character (including `\/`) never terminates the literal. | ||
| out.append(blank(source[j])) | ||
| out.append(blank(source[j + 1])) | ||
| j += 2 |
There was a problem hiding this comment.
When an extended regex contains interpolation, such as #/token: \#(config.token)/#, this branch treats the interpolation introducer as an escaped character and then blanks the entire expression as regex text. Extended-regex interpolation is valid in Swift 5 mode, so the audit reports success even though the @concurrent body reads mutable SDK state off-actor. Parse \ plus the matching hash count and ( as interpolation, then lex its contents as executable Swift just as scan_string does.
Useful? React with 👍 / 👎.
| body = text[k:end] | ||
| for offset, what in sorted(forbidden_reads(body)): |
There was a problem hiding this comment.
Because only the lexical bodies carrying @concurrent are inspected, the existing execute body can call an instance helper such as buildURLRequest, and that helper can read config or delegate without the audit noticing. I verified that adding _ = config.token to the current buildURLRequest makes this script report success, even though the helper executes synchronously on the global executor and reintroduces the mutable-state race this gate is meant to prevent. Include reachable instance helpers in the audit or make the transport helpers static and pass every dependency explicitly.
Useful? React with 👍 / 👎.
| for attr in re.finditer(r"@concurrent\b", text): | ||
| decl = FUNC_DECL.search(text, attr.end()) | ||
| between = text[attr.end() : decl.start()] if decl else "" | ||
| if not decl or re.search(r"[{};]", between): | ||
| failures.append(f"line {line_of(text, attr.start())}: @concurrent is not attached to a func") |
There was a problem hiding this comment.
A valid preceding declaration such as var callback: (@concurrent @Sendable () async -> Void)? causes this search to associate that type attribute with the next func perform, because the intervening property declaration contains no brace or semicolon. The audit then accepts an unannotated perform, allowing its decoding and delegate callback work to inherit the caller actor. This is fresh evidence beyond the earlier declaration-count fix: every helper declaration is examined, but an unrelated attribute is still recorded at the helper's offset. Parse the containing declaration or otherwise require the attribute to belong to the function declaration itself.
Useful? React with 👍 / 👎.
Problem
Independent review (slopguard, Codex) of the v3.6.0 range raised two test gaps:
@concurrentbody would compile and silently reintroduce the race. The delegate-off-actor contract in ARCHITECTURE.md was also unasserted.URLError.cancellednormalization and never reached the post-poll cancellation check with a result in hand. Removing that check left them green.Solution
scripts/check-transport-isolation.shruns inmake verify:requestmust not be@concurrent, and no@concurrentbody inPutioSDK.swiftmay useself.members or bareconfig/delegate. The concurrent helpers now takedelegateReferenceso the bare identifier is forbidden. Verified the audit fails when a baredelegateread is reintroduced.@MainActorand assert the callback is not on the main thread, both for the shared transport and for the device-code failure path.TaskCompletionObserveron the session (the asyncdata(for:)API only forwardsdidFinishCollecting metricsto the session delegate) plus an actor host. The sleep case cancels after the poll has completed and bounds elapsed time. The post-poll case holds the owning actor so the.authorizedresumption stays queued, cancels, then releases. Droppingtry Task.checkCancellation()after the poll makes that test fail withauthorized(token: <redacted>).Proof
make verifygreen locally, coverage 91.62%.delegateread; post-poll test catches the removed cancellation check.