mirror of
https://github.com/StikDebug/StikDebug.git
synced 2026-10-16 04:20:30 +00:00
Add fork code-review report
Documents the review of all fork changes since upstream divergence, the two defects fixed (broken swift.yml CI workflow, over-aggressive speed-index segment cap), and the areas verified as correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013DNqCsHShA4yNqgZ6ZZQy3
This commit is contained in:
Claude
parent
e11a6062d1
commit
b394334d8c
1 changed files with 151 additions and 0 deletions
151
CODE_REVIEW_REPORT.md
Normal file
151
CODE_REVIEW_REPORT.md
Normal file
|
|
@ -0,0 +1,151 @@
|
|||
# Fork Code-Review Report — StikDebug-PLUS
|
||||
|
||||
**Date:** 2026-07-14
|
||||
**Branch reviewed:** `claude/fork-code-review-33fkom`
|
||||
**Scope:** All fork-specific changes since divergence from upstream StikDebug (commit `c5b23e2`).
|
||||
|
||||
## Summary
|
||||
|
||||
The fork adds four feature areas on top of upstream StikDebug: a location-simulator
|
||||
overhaul (speed profiles, bus stops, map styles, long-route performance), a
|
||||
cellular/Wi-Fi network path monitor with tunnel auto-reconnect, background
|
||||
keep-alive improvements for debug sessions, and an experimental "hold app alive
|
||||
in background" mode. 15 files changed, ~1,400 lines added.
|
||||
|
||||
I reviewed the full diff against upstream, cross-checked every new FFI call
|
||||
against `idevice.h`, traced the state machines in the changed managers, and
|
||||
confirmed build status via GitHub Actions history. **Two genuine defects were
|
||||
found and fixed; both are pushed to the review branch.** No crashes, compile
|
||||
errors, or logic bugs remain in the reviewed code.
|
||||
|
||||
---
|
||||
|
||||
## Defects fixed
|
||||
|
||||
### 1. Always-failing CI workflow — `.github/workflows/swift.yml` (removed)
|
||||
|
||||
**Severity:** Medium (broken CI signal on every push/PR)
|
||||
|
||||
The workflow ran `swift build` / `swift test`, which require a `Package.swift`.
|
||||
This repository is an Xcode app project with no Swift package, so the job failed
|
||||
with `error: Could not find Package.swift` on **every run since it was added**,
|
||||
marking each push and PR with a red ❌ check.
|
||||
|
||||
The existing **Build Debug IPA** workflow already compiles the project with
|
||||
`xcodebuild` on the same `push`/`pull_request` triggers and passes green, so
|
||||
build validation is fully covered without it.
|
||||
|
||||
- **Fix:** Deleted `swift.yml` (commit `7b1df35`).
|
||||
- **Evidence:** GitHub Actions run `29341384965` (job `87113733956`) —
|
||||
`Could not find Package.swift in this directory or any of its parent directories`.
|
||||
|
||||
### 2. Long rural road segments dropped from the speed index — `MapSelectionView.swift` (fixed)
|
||||
|
||||
**Severity:** Low (accuracy regression on long routes; no crash)
|
||||
|
||||
`RouteSpeedIndex` bucketed each OSM way segment into ~440 m grid cells, but
|
||||
skipped any segment whose bounding box spanned more than **8 cells (~3.5 km)**:
|
||||
|
||||
```swift
|
||||
guard maxX - minX <= 8, maxY - minY <= 8 else { continue }
|
||||
```
|
||||
|
||||
Sparsely-noded rural highways can legitimately run tens of km between OSM
|
||||
geometry nodes. Those segments were silently dropped from the index, so those
|
||||
stretches lost their real speed limit and fell back to average-route pacing —
|
||||
a behavior regression versus upstream, whose (slower) implementation scanned
|
||||
every segment without this cap.
|
||||
|
||||
- **Fix:** Raised the guard to **128 cells (~57 km at the equator)** (commit
|
||||
`e11a606`). This still catches genuinely broken data such as antimeridian
|
||||
jumps (which span ~90,000 cells) while keeping realistic long segments
|
||||
indexed. Per-segment registration stays bounded, and the `nearestWayThreshold`
|
||||
(40 m) distance filter means indexing extra cells never produces false matches.
|
||||
|
||||
---
|
||||
|
||||
## Areas verified as correct (no change needed)
|
||||
|
||||
### Build / compilation
|
||||
- **Build Debug IPA is green** on the fork tip (`346bff6`), confirming the app
|
||||
compiles and that the `xcode-version: 26.6.0` pin resolves on the runner.
|
||||
- `packet.count` is correctly converted to `UInt` for the `uintptr_t`
|
||||
`debug_proxy_send_raw` parameter; `fetchRouteSpeedContext` returns a
|
||||
`RouteSpeedContext` on all paths (both were prior build-fix commits and hold up).
|
||||
|
||||
### Experimental "hold app alive" / JIT keep-alive (`JITEnableContext`, `BackgroundAliveManager`, `DebugKeepAliveLease`)
|
||||
- Every FFI call matches `idevice.h`: `process_control_launch_app` argument
|
||||
order and `bool` flags, `process_control_disable_memory_limit`, and
|
||||
`debug_proxy_send_raw(handle, ptr, uintptr_t)`.
|
||||
- The hand-built RSP continue packet `$c#63` has the correct payload and
|
||||
checksum (`0x63 % 256` → `63`), and is valid in no-ack mode.
|
||||
- The hold sequence mirrors the proven `debugApp` path: drain acks →
|
||||
`QStartNoAckMode` → disable ack mode → `vAttach` → continue → poll for
|
||||
cancellation → interrupt (`0x03`) → detach (`D`).
|
||||
- `HoldToken` is lock-guarded; `BackgroundAliveManager` balances start/stop and
|
||||
its `stillCurrent` check prevents a late-finishing session from clobbering a
|
||||
newer one. `DebugKeepAliveLease()` auto-activates in `init`, so the bare
|
||||
construction in both `BackgroundAliveManager` and `HomeView` is correct.
|
||||
- Background-task renewal re-checks `isActive` before re-arming, so an expiring
|
||||
UIKit assertion no longer tears down long sessions.
|
||||
|
||||
### Background audio / location (`BackgroundAudioManager`, `BackgroundLocationManager`)
|
||||
- The new `force:` (session) vs. unforced (user-toggle) activity counters are
|
||||
balanced on every start/stop path and never go negative (`max(… - 1, 0)`).
|
||||
- `refreshRunningState()` correctly honors forced holds regardless of the user's
|
||||
keep-alive toggles, and `refreshFromSettings()` re-evaluates on toggle change.
|
||||
- The non-zero silent-audio fill is inaudible (~-80 dBFS, DC-free) and guarded
|
||||
by `!format.isInterleaved` with the correct channel/frame iteration; the
|
||||
interleaved branch is an unreachable fallback on the standard mixer format.
|
||||
|
||||
### Network path monitor & tunnel reconnect (`NetworkPathMonitor`, `TunnelManager`)
|
||||
- Path state is lock-guarded; change notifications are deduplicated by signature.
|
||||
- Tunnel retry (3 attempts, 1s→2s backoff) correctly short-circuits on permanent
|
||||
errors (invalid/expired/missing pairing `-9/-17`, bad target IP `-18`, parse
|
||||
failures) instead of pointlessly retrying user-actionable failures.
|
||||
- The reconnect observer is registered on the main queue, matching `start()`'s
|
||||
main-thread expectation; the debounced work item cancels prior pending work.
|
||||
|
||||
### Location simulator — speed profiles, bus stops, map styles (`MapSelectionView`)
|
||||
- The Overpass query moved from a bounding-box GET to a corridor `around` POST;
|
||||
the body is percent-encoded with a form-safe character set and the client
|
||||
timeout (30 s) exceeds the server timeout (25 s).
|
||||
- `OverpassResponse.Element` decodes both way `geometry` and node `lat`/`lon`,
|
||||
which `out tags geom;` emits for nodes — bus-stop parsing is sound.
|
||||
- Bus-stop detection covers both the legacy `highway=bus_stop` and the modern
|
||||
`public_transport=platform/stop_position` + `bus=yes` schemas.
|
||||
- Route state (`routeBusStops`, `isImportedRoute`, `lastFallbackSpeed`,
|
||||
`routeRequestID`) is reset consistently across import / clear / reset / refresh,
|
||||
and every async prefetch guards on `routeRequestID` before applying results.
|
||||
- Profile switching re-plans directions only when walking-vs-driving changes and
|
||||
the route is searched (not imported); otherwise it rebuilds pacing in place.
|
||||
- All SwiftUI/MapKit symbols are valid for the iOS 17.4 deployment target
|
||||
(`MapStyle`, `PointOfInterestCategories`, `Marker`, `.mapStyle`).
|
||||
|
||||
---
|
||||
|
||||
## Minor observations (intentional trade-offs — not changed)
|
||||
|
||||
- **Bus ETA** in the route summary uses MapKit's automobile travel time and does
|
||||
not add the per-stop dwell time, so a bus route's ETA is slightly optimistic.
|
||||
- **Bus stop dwell** (12 s) is applied to the sample nearest the stop (≤25 m),
|
||||
so playback pauses just before arriving rather than exactly at the stop.
|
||||
- **`build_ipa.yml`** has two stray blank lines from an earlier edit; harmless
|
||||
(YAML ignores them).
|
||||
- **Bus-stop cell lookup** assumes non-polar latitudes (documented); above ~80°N
|
||||
a 90 m search radius could exceed a longitude cell. Not reachable in practice.
|
||||
- **`keepAppAlive`** blocks a global-queue thread for the hold duration and opens
|
||||
multiple concurrent debug tunnels; acceptable for an explicitly experimental
|
||||
feature, but worth revisiting if it graduates out of experimental.
|
||||
|
||||
---
|
||||
|
||||
## Commits on the review branch
|
||||
|
||||
| Commit | Change |
|
||||
|--------|--------|
|
||||
| `7b1df35` | Remove Swift Package CI workflow that can never pass |
|
||||
| `e11a606` | Index long rural road segments instead of dropping them |
|
||||
|
||||
Both are pushed to `origin/claude/fork-code-review-33fkom`. The Build Debug IPA
|
||||
workflow will validate compilation on the branch tip.
|
||||
Loading…
Add table
Add a link
Reference in a new issue