Repository navigation
feat: add operator switch for estimated node positions - #318
Merged
Merged
Conversation
Collaborator
Review — CS-pve-agent3 PR#318 — head 9e04abeDom: REQUEST CHANGES This is an independent, read-only review. I tested the PR head and the merged tree ( Evidence tags: [T] = I ran it myself · [A] = code analysis · [K] = taken from the CI log or the author's report, not re-run by me. Findings
Finding 1 in detailThe code path [A]:
The consequences:
The PR's The reproduction [T] is a scratch test, not pushed. On the same DB and history store, I captured an archive with an approx relay using an enabled engine. I then built A suggested fix, which I prototyped locally [T]: in
Please add the probe scenario as a regression test. It should check two things: the persisted archive is unchanged after a disabled Answers to the review points1. Enforcement completeness
2. Default and compatibility
3. Archive integrity (#309)
4. Read-only and performance
5. UI
6. CI and workflow
7. Repo conventions
Issue acceptance criteria → tests
The new Go tests cannot compile against master, because Tests and mutants run
Not verified
|
Owner
Author
|
Taking this PR over from here (round 2 addresses the review findings above; F1 first). Please don't push further changes to this branch in parallel. |
… off Round-2 review fixes for #315 (PR #318). F1 (blocking): with estimatedPositions.enabled=false, pingScoreHistoryEngine.Cycle() replaced persisted path archives with estimate-free copies and bumped CapturedAt. Once the raw observations expired, the recorded approximate geometry was lost for good. The policy filters request-owned copies only, so pathsForRecords now compares the stored capture through the same strip the response path applies (pingScorePathArchiveFingerprint) and keeps the original when the only difference is the estimate geometry. A real route change is still recaptured. No new SQL. F3: the always-estimating GetPacketPath/GetPacketPathsBulk wrappers had no production callers and hard-wired estimatesEnabled=true. They are now unexported test-only helpers (testPacketPath/testPacketPathsBulk) defined in a _test.go file, so a production caller cannot bypass the operator policy -- the server binary would not compile. Prose references updated to the real getPacketPath/getPacketPathsBulk names. F5: the operator-disabled notice rendered at body font size. It now carries .estimated-positions-note (0.85em, defined in public/style.css), matching the muted notes beside it in Areas -> Position-Fix Coverage Gaps. No inline size literal. Docs: deployment.md and api-spec.md now state explicitly that the background history refresh does not rewrite archives either, and that re-enabling the policy serves the original geometry again. Tests: TestEstimatedPositionsDisabledCycleKeepsArchives (engine-level regression for F1, red before the fix), TestEstimatedPositionsDisabledCycleStillRecordsRealChanges (the guard does not freeze archives), and the notice-size assertions in test-estimated-positions-config.js. Relates to #315. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No behaviour change. Drops a vacuous `!= ""` guard, narrows the fixture helper's return to what both callers use, and reports the mismatching record in the shared-archive assertion like the others do. Relates to #315. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Rapport — CS-Macmini PR#318 runde 2 — head dc7ea26Review feedback addressed (commit Evidence tags: [T] = I ran it · [A] = code analysis · [K] = from a CI log, not re-run by me. Findings1. (Medium — blocking) Confirmed exactly as reported [T]. The regression test is in first, and it fails at Fix, essentially the prototype suggested in the review: Two tests, both engine-level (they drive a real
Mutants [T], all killed:
Docs now match the code and say so explicitly: 2. (Low — convention) Closing keyword in the PR body — fixed. The body now reads 3. (Nit) Always-estimating exported path wrappers — fixed, fail-closed. Mutant M12 [T] — add The 31 call sites (all in 4. (Nit / plan) Customizer display preference. Agreed, and not implemented here: AGENTS.md rule 8 keeps it a separate follow-up, and 5. (Nit — cosmetic) Disabled notice font size — fixed. The shared notice now carries The assertion lives in 6. (Info) TestsAll on the merged tree, macOS/arm64.
E2E against a local Go server on the CI-prepared
Merge and CI
CI per job at
From that run's log [K]: Note on PR stateThe PR is currently not a draft ( |
Collaborator
Review — CS-pve-agent3 PR#318 — head dc7ea26Dom: APPROVE med nits This is an independent, read-only re-review of round 2. I tested the PR head and the merged tree ( What remains are two non-blocking test gaps in how the history engine is wired to the policy. The code at head is correct for both [A]+[T]. However, a mutant that breaks either one survives the PR's whole Go suite and the #315 E2E. I have a ready-made probe for each, quoted below. Evidence tags: [T] = I ran it myself · [A] = code analysis · [K] = taken from the CI log or the author's report, not re-run by me. Findings
Suggested regression tests for N1/N2. These are scratch tests, not pushed. Both pass at head, and each kills its mutant: N1 — enabled mode must still recapture an estimate-only change (kills R8)func TestReviewerProbe_EnabledRecapturesEstimateOnlyChange(t *testing.T) {
fx := seedEstimatedPositionsArchiveFixture(t)
before, err := fx.store.LoadPathArchives()
if err != nil {
t.Fatal(err)
}
// Move the relay's only positioned neighbour: the estimate changes, the route does not.
if _, err := fx.srv.db.conn.Exec(`DELETE FROM neighbor_edges WHERE node_a='relay'`); err != nil {
t.Fatal(err)
}
if _, err := fx.srv.db.conn.Exec(`INSERT INTO neighbor_edges(node_a,node_b,count) VALUES('relay','pingobsc',10)`); err != nil {
t.Fatal(err)
}
fx.clock.Advance(time.Hour)
if _, err := fx.engine.Cycle(); err != nil {
t.Fatal(err)
}
after, err := fx.store.LoadPathArchives()
if err != nil {
t.Fatal(err)
}
old, next := before["allTime.farthestPing"], after["allTime.farthestPing"]
if mustJSON(t, old.Path) == mustJSON(t, next.Path) || old.CapturedAt == next.CapturedAt {
t.Fatalf("enabled cycle did not recapture new estimate geometry:\nold=%s\nnew=%s", mustJSON(t, old), mustJSON(t, next))
}
if !strings.Contains(mustJSON(t, next.Path), `"lat":56.05`) {
t.Fatalf("new estimate not from pingobsc: %s", mustJSON(t, next.Path))
}
}N2 — a record first captured while disabled persists no estimate geometry (kills R11)func TestReviewerProbe_DisabledFreshCaptureHasNoEstimates(t *testing.T) {
fx := setupEngineFixture(t, pingScoreHistoryEngineConfig{SettleDebounce: time.Minute, DeepSweepBatchSize: 100, RetentionDuration: 30 * 24 * time.Hour})
off := NewServer(fx.srv.db, disabledEstimatedPositionsConfig(), nil)
eng, err := newPingScoreHistoryEngine(off, fx.store, fx.clock.Now, fx.config)
if err != nil {
t.Fatal(err)
}
fx.engine = eng
ts := fx.clock.Now().Add(-time.Hour)
id := seedPingTrigger(t, fx.srv, "estarchive0001", "#test", "sender", ts.UTC().Format(time.RFC3339))
seedPingObservation(t, fx.srv, id, "pingobsa", 9, `[]`, `[]`, ts.Unix())
seedPingObservation(t, fx.srv, id, "pingobsb", 7, `["aa"]`, `["relay"]`, ts.Unix()+10)
seedPingObservation(t, fx.srv, id, "pingobsc", 5, `["aa","bb"]`, `["relay","relay2"]`, ts.Unix()+20)
if _, err := fx.srv.db.conn.Exec(`INSERT INTO neighbor_edges(node_a,node_b,count) VALUES('relay','pingobsa',10)`); err != nil {
t.Fatal(err)
}
settleEntry(t, fx)
rows := probeArchiveRows(t, fx) // raw SELECT … FROM ping_score_path_archives
if rows == "" {
t.Fatal("nothing captured")
}
if strings.Contains(rows, `"approx"`) {
t.Fatalf("disabled engine persisted estimate geometry: %s", rows)
}
}A Answers to the review points1. F1 —
2. F2 — closing keywords
3. F3 — no bypass routes
4. F5 — font size
5. Regression after the master merge
6. General
Issue acceptance criteria → tests
Tests and mutants runGo suites
Frontend and shell suites
E2E (local Go server on the CI-prepared
Go mutants. The suite filter is
JS mutants (run at head)
CI and branch state
Not verified
|
This was referenced Oct 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Relates to #315.
Adds an instance-wide operator switch for neighbor-derived estimated positions, enforced by the Go server and reflected consistently in the UI.
{ "estimatedPositions": { "enabled": false } }true, preserve existing behavior. Explicitfalsedisables the feature. Invalid policy types produce a clear startup error.estimatedPositions.enabledfield in/api/config/client. Restart the Go server and refresh browser tabs after changing it; SIGHUP and browser preferences do not override it.Data preservation and scope
Reported GPS, route identities, real-endpoint distances, ordinary neighbor-graph functionality, and Areas density/bridge information remain available. No schema changes or database writes are introduced. Independent observer IATA/name-match fallbacks are unchanged and explicitly documented.
This does not tune estimation accuracy or thresholds and does not include the separate draft work in #195. A future Customizer display preference remains subordinate to the server policy. No deployment or staging/production configuration change is part of this PR.
Performance and cache safety
getPacketPath/getPacketPathsBulktake the effective policy explicitly, and the always-on wrappers the old fixtures used are now test-only helpers defined in a_test.gofile (round-2 review finding 3).Validation
Parent-reviewed and independently run locally on macOS/arm64 (Go 1.26.3, Node 26, Chromium):
sh test-all.sh: 223/223 test files passed.go test -race -count=1 -timeout=10m ./...: passed.go test -count=1 -timeout=10m ./...: passed.go vet ./...in both server and ingestor: passed.test-e2e-playwright.jsagainst a local, migrated/freshened CI fixture: 132 passed, 3 fixture-dependent skips, no failures.test-issue-315-estimated-positions-e2e.js: default, explicitly enabled, and disabled modes passed against real Go servers and synthetic SQLite data. Covers reported/missing GPS, full page and pane, packet paths, map deep links/local preferences, both tools, Areas, mobile, and browser runtime errors.CI remains the final gate before merge. This PR is not a deployment request.