Read _isConnected backing storage in deferred Task bodies in GatewayConnectionManager

[ashleeradka]

Why

Follow-up to #29899. Two reads of isConnected inside Task { @MainActor in } bodies still went through the synthesised getter (self.isConnected) rather than the macro-synthesised backing storage (self._isConnected). Switching them makes every read of isConnected outside the public-API path a pure storage compare, which matches the pattern used everywhere else in the file.

Both Task bodies run on a fresh @MainActor turn outside any withObservationTracking context, so _$observationRegistrar.access(_:keyPath:) is already a no-op there. This is purely a visual-consistency change with no behavioral effect.

What changed

• startReconnectionLoop: guard !self.isConnected → guard !self._isConnected.
• scheduleDaemonDidReconnect: guard let self, self.isConnected → guard let self, self._isConnected.

Safety

• Both call sites are inside Task { @MainActor [weak self] in ... } bodies on a fresh actor turn — no active withObservationTracking context — so self.isConnected and self._isConnected return the same value via the same load path at runtime.
• No control-flow change. No public API change. No test changes (existing Defer and coalesce setConnected writes in GatewayConnectionManager #29899 coverage exercises both call sites).

Notes for reviewers

• The AGENTS.md rule about backing-storage reads is scoped to "guarding a self-write." These two sites are current-state checks, not self-write guards, so the rule doesn't strictly apply — the change is justified by file-local consistency, not by the AGENTS.md rule.
• Suggested by a non-blocking nit from Devin Review on Defer and coalesce setConnected writes in GatewayConnectionManager #29899; included here rather than expanded into the AGENTS.md rule because broadening the rule to cover non-self-write reads risks misleading future readers.

Test plan

• Existing unit tests in GatewayConnectionManagerTests cover the connect/disconnect/reconnect-loop paths that exercise both call sites.
• CI on this repo skips macOS checks, so a local Xcode build / swift test on macOS is required to verify the build before merging.

Link to Devin session: https://app.devin.ai/sessions/2da0c092185640b284918ba17b129fa3
Requested by: @ashleeradka

---

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

[vex-assistant-bot]

✅ Approving — pure file-local consistency, runtime no-op

2 lines, 1 file. Both edits are in Task { @MainActor [weak self] in ... } bodies on a fresh actor turn:

1. startReconnectionLoop (L847): guard !self.isConnected → guard !self._isConnected
2. scheduleDaemonDidReconnect (L1041): guard let self, self.isConnected → guard let self, self._isConnected

Why this is safe

The withObservationTracking { ... } onChange: { ... } closure has already returned by the time these Tasks fire on the next @MainActor turn — there's no active tracking context for _$observationRegistrar.access(_:keyPath:) to register a dependency against. The synthesised getter and backing-storage read produce the same value via effectively the same load path at runtime (the registrar access call is a no-op with no observers). Pure visual-consistency cleanup; zero behavioral change.

Why I agree with the AGENTS.md framing

The PR body correctly notes that the AGENTS.md rule is scoped to self-write guards, not current-state checks, and explicitly chooses NOT to broaden the rule. That's the right call — broadening it to "always read backing storage in Task bodies" would mislead future readers about why we use _propertyName (the reason is observation-cycle prevention on writes, not a blanket style preference). Keeping the rule narrow + fixing the inconsistency locally is the cleanest framing.

Verifications

• ✅ weak self + guard let self capture pattern preserved
• ✅ No control-flow change, no public-API change, no test change
• ✅ Sweep against the rest of the file: every other isConnected read I scanned in #29899 and this PR is now either through _isConnected (writes/guards/Task bodies) or via the public API (connect(), disconnect(), external consumers). Internally consistent.
• ✅ Devin: ✅ No Issues Found badge on HEAD d832d75e5c (1 additional finding in Devin UI not exposed via API — same Devin UI gap I flagged on #29899/#29901, non-blocking)
• ✅ Codex: 👍 reaction on PR description
• ✅ CI: Socket Security ×2 + FlexFrame Lint all green; macOS Build/Tests SKIPPED (expected — Boss's local swift test is the gate per PR body)
• ✅ Mergeable: clean (blocked overlay is just the pending combined-status indicator, same as #29899)

Tiny observation (non-blocking)

The synthesised getter still calls _$observationRegistrar.access(_:keyPath:) even when no observers are registered — it's a near-zero-cost lookup, but switching to backing storage elides that path entirely. Vanishingly small perf benefit, definitely not the motivation, just noting for completeness.

Approving on d832d75e5c6dd1118b978d25f70c28414719f795. Merge after local swift test passes.