Skip to content

fix: correlate connection attempts and isolate preferred cancellation - #165

Open
enaboapps wants to merge 1 commit into
mainfrom
codex/connection-diagnostics-164
Open

enaboapps wants to merge 1 commit into
mainfrom
codex/connection-diagnostics-164

Conversation

@enaboapps

Copy link
Copy Markdown
Contributor

Closes #164

Summary

  • Add bounded session-local anonymous PC/attempt labels, request origin, observed peer/reply mode, read-response readiness, and explicit teardown triggers across manager and BLE diagnostics.
  • Export no names, addresses, persistent IDs, credentials, input or raw errors. Clear invalidates correlation contexts; observers cannot disrupt connections. Document capacity/clear semantics and manual interpretation.
  • Reproduced and fixed stale preferred-PC focus cancellation aborting a newer manual connection. Cancellation now checks operation ownership independently of diagnostics.
  • Add manual overlap, preferred/manual, explicit-switch, privacy, observer isolation, alias bounds, and manager/transport correlation regressions.

Validation

  • npm run validate: 594 tests, lint/types/version checks, Expo Doctor 21/21 passed.
  • New preferred/manual focus regression failed before the operation guard (manual state became idle); passes after fix.
  • Automated tests use fake transports only. Physical Linux reconnect cause remains unconfirmed pending correlated phone logs.
  • Independent latest-head review and CI pending. No merge or release included.

@enaboapps

Copy link
Copy Markdown
Contributor Author

Independent latest-head review at c696f5c found no actionable findings. Reviewer checked correlation, privacy bounds/clear semantics, transport operation guards, teardown attribution and preferred-focus cancellation ownership; 106 focused tests passed independently. Full local validation passed 594 tests and Expo Doctor21/21. Native CI remains in progress; no hardware reliability claim.

@enaboapps
enaboapps marked this pull request as ready for review September 12, 2026 10:27
@greptile-apps

greptile-apps Bot commented Sep 12, 2026

Copy link
Copy Markdown

Greptile Summary

Summary

Two diagnostic attribution issues remain:

  • Repeated observations of the same desktop ID longer than 128 characters receive different anonymous PC labels, preventing reliable correlation in exported diagnostics.
  • A delayed saved-PC removal failure can be labeled as belonging to a newer, unrelated connection attempt.

These issues affect diagnostic accuracy only and do not block merging.

Confidence Score: 4/5

Safe to merge: the confirmed issues affect support diagnostics rather than connection behavior, authorization, or stored data.

Focused executable checks reproduced two independent diagnostic attribution failures.

Files Needing Attention: src/diagnostics/DiagnosticLog.ts and src/connection/ConnectionManager.ts

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for the posted P2 finding, and linked it to the corresponding review comment.
  • T-Rex produced a second proof for the posted P2 finding, with its review comment.
  • T-Rex executed a diagnostic long-desktop-id correlation test, saved the source and runtime output, and confirmed the two observations are logged under separate PC labels.
  • T-Rex ran the unpair-attribution Jest test, returned exit code 0, and captured the source and after-log for inspection.
  • T-Rex produced a proof for another posted P2 finding.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (2)

  1. General comment

    P2 Long desktop IDs are not retained for anonymous diagnostic correlation

    • Bug
      • For the same 129-character non-empty desktop ID, two calls to peerObserved during the same diagnostic session logged separate labels (Observed PC-2 and Observed PC-3). This prevents a reader of exported diagnostics from identifying those entries as observations of the same peer.
    • Cause
      • DiagnosticLog.#pc only stores an ID-to-label entry when desktopId.length <= 128. Longer IDs allocate #nextPc++ on every call and are never added to #pcs.
    • Fix
      • Retain a bounded session-local alias for long IDs as well, such as by caching a bounded non-identifying digest/key for every non-empty ID, while preserving the existing 128-entry eviction policy.

    T-Rex Ran code and verified through T-Rex

  2. General comment

    P2 Delayed unpair failures are attributed to a later connection attempt

    • Bug
      • unpair awaits storage.remove(desktopId) and records unpair_failed through #record only after that await. If a connection starts while removal is pending, #startAttempt replaces #attempt, so the removal failure is tagged as belonging to the later connection. The executed test removed first, began connecting saved PC second, then rejected the removal; the diagnostic was logged as [attempt-1 PC-1 saved], where that attempt belongs to second. This misleads support diagnostics and can falsely associate an unpair-storage failure with an unrelated connection attempt.
    • Cause
      • ConnectionManager.#record reads mutable this.#attempt; unpair does not capture the pre-await diagnostic context or deliberately log its storage failure without a connection attempt.
    • Fix
      • Capture the diagnostic attempt before awaiting removal and pass that captured context explicitly, or add an attempt-independent diagnostic logging path for unpair/storage operations. Since unpair is not itself a connection attempt, recording unpair_failed without an attempt is likely the clearest attribution.

    T-Rex Ran code and verified through T-Rex

Prompt To Fix All With AI
### Issue 1
src/diagnostics/DiagnosticLog.ts:74-76
**Keep long IDs correlated**

Desktop IDs longer than 128 characters are never retained in the session alias map. Each later observation of the same peer therefore receives a new anonymous PC label, so exported diagnostics cannot correlate that peer's events. This is a non-blocking diagnostic-quality concern that makes support investigations less reliable.

### Issue 2
src/connection/ConnectionManager.ts:229
**Keep unpair context separate**

A failed saved-PC removal is logged only after the asynchronous removal finishes, using the manager's current connection attempt. If another PC connection begins while removal is pending, the unpair failure is labeled as part of that unrelated connection. This is a non-blocking diagnostic-quality concern that can mislead support investigations.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix: correlate connection attempts and i..." | Re-trigger Greptile

Comment on lines +74 to +76
if (desktopId.length <= 128) {
if (this.#pcs.size >= 128) this.#pcs.delete(this.#pcs.keys().next().value!);
this.#pcs.set(desktopId, pc);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Keep long IDs correlated

Desktop IDs longer than 128 characters are never retained in the session alias map. Each later observation of the same peer therefore receives a new anonymous PC label, so exported diagnostics cannot correlate that peer's events. This is a non-blocking diagnostic-quality concern that makes support investigations less reliable.

Artifacts

Evidence from the check

  • Authored Jest test that invokes the real DiagnosticLog path twice with one 129-character desktop ID and asserts a stable anonymous label; it is the executed reproducer.

Command output from the check

  • Captured Jest output from the focused reproducer shows the two observations received PC-2 and PC-3, proving correlation is broken.

View artifacts

T-Rex Ran code and verified through T-Rex

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/diagnostics/DiagnosticLog.ts
Line: 74-76

Comment:
**Keep long IDs correlated**

Desktop IDs longer than 128 characters are never retained in the session alias map. Each later observation of the same peer therefore receives a new anonymous PC label, so exported diagnostics cannot correlate that peer's events. This is a non-blocking diagnostic-quality concern that makes support investigations less reliable.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

return true;
} catch {
this.diagnostics.add('unpair_failed', 'warning');
this.#record('unpair_failed', 'warning');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Keep unpair context separate

A failed saved-PC removal is logged only after the asynchronous removal finishes, using the manager's current connection attempt. If another PC connection begins while removal is pending, the unpair failure is labeled as part of that unrelated connection. This is a non-blocking diagnostic-quality concern that can mislead support investigations.

Artifacts

Evidence from the check

  • This focused test starts removal for the first saved PC, starts a connection to the second PC while removal is pending, rejects removal, and asserts the diagnostic record; it demonstrates the attribution race.

Command output from the check

  • This captured Jest run logs the observed `unpair_failed` record with the later connection attempt prefix and exits successfully; it confirms the misattribution.

View artifacts

T-Rex Ran code and verified through T-Rex

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/connection/ConnectionManager.ts
Line: 229

Comment:
**Keep unpair context separate**

A failed saved-PC removal is logged only after the asynchronous removal finishes, using the manager's current connection attempt. If another PC connection begins while removal is pending, the unpair failure is labeled as part of that unrelated connection. This is a non-blocking diagnostic-quality concern that can mislead support investigations.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Correlate connection attempts and diagnose automatic switching safely

1 participant