Conversation
Acts on the review of #277 and #278. The DEC-A2 rule indexed EVERY way as a possible corroborator, so a building outline, a `landuse` edge, a waterway or another wall could vouch for a gate. That is the likeliest false-positive shape in real data, because `entrance=*` nodes are overwhelmingly building-outline vertices -- "building entrance node + outline + a fence within a metre" would have opened the fence. It was not hypothetical: the one corpus opening the rule had added at Sylt came from `barrier=wall` 739515786 corroborating `barrier=wall` 740958910. The below-surface veto was also asymmetric -- applied to the gate node but not to the corroborating way -- so a `tunnel=yes` road crossing in plan would have opened the wall above it, DEC-R12-1's rejected failure mode arriving from the other side. Deliberately NOT "same layer as the barrier": the Tower's own corroborating way is `bridge=yes layer=1`. So DEC-A2 now changes NOTHING at any of the eight corpus sites. Both openings it originally produced were false positives, and a rule that is a no-op everywhere looks exactly like a rule that works -- which is why the demonstration is `agent-route.tower-gate.test.ts` on real Tower geometry, and why both the test and the sidecar say so. Also from the review: the "NOT a vertex of any barrier" docstrings were false (nothing tested membership, and could not); `Enu` duplicated `EnuPoint`; touching-counts-as-crossing was undocumented and is now a stated decision with a T-junction test; and the tolerance boundary and non-perpendicular crossings were untested. And two JSDoc defects in the r488 recorder: the header prescribed observing `documentElement`, which is the exact bug the implementation was fixed to avoid, and the new function had been inserted between `recordStatus`'s doc block and `recordStatus` itself -- so the block documented the wrong function, on the one pair where confusing them costs the most.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @cs-util's task in 4m 9s —— View job ReviewTwo inline comments, both on the new restriction and its test. Nothing blocking.
Inline findings
Checked and consistent
One note on the prose. Several of the new docstrings narrate their own edit history rather than the rule — "An earlier version of this comment claimed … which was simply false", "that restriction was a review finding rather than the original design", "(raised in review on #277)" in four test docstrings. Git and this PR hold that; a reader in six months wants the invariant. The DEC-A2 measurement history is different and worth keeping, because it is the evidence for the rule's narrowness. Not verified: I could not run the suite — |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gps-plus-slam | 3c19511 | Commit Preview URL Branch Preview URL |
Aug 08 2026, 11:05 PM |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * `bridge=yes layer=1`, so above-surface ways have to stay acceptable. | ||
| */ | ||
| function canCorroborate(feature: OsmFeature & { type: "way" }): boolean { | ||
| return feature.tags["highway"] !== undefined && !isBelowSurface(feature); |
There was a problem hiding this comment.
A presence test on highway, in the one file that already documents why a presence test is the wrong shape. DENIED_ENTRANCES exists precisely because entrance had "degraded to a presence test" and entrance=no opened the one node OSM took the trouble to deny. highway has the same denial-shaped values, and one structural one:
highway=construction/highway=proposed— a route that does not exist yet vouching for a gate.highway=pedestrian+area=yes— this is the building-outline argument arriving from the other side. A plaza polygon is an outline, not a line through, and plazas abut walls constantly;roads.ts:139-147already refuses exactly this shape ("A highway AREA is a surface, not a ribbon"). The docstring above says "highwayis the routable set" — witharea=yesin it, it is not.
Moderate confidence on how often this bites real data; measured corpus impact is zero today, so this is about the rule's argument holding rather than a live wrong opening. But isRoad is right next door and encodes "a highway that is genuinely a linear route", including the tunnel/covered veto that canCorroborate reimplements one half of via isBelowSurface. Either reuse it or add the two exclusions:
const highway = feature.tags["highway"];
if (highway === undefined || NON_ROUTE_HIGHWAYS.has(highway)) return false;
if (feature.tags["area"] === "yes") return false; // an outline, not a way through
return !isBelowSurface(feature);
| expect(opens(GATE_ON_BARRIER_M)).toBe(true); | ||
| expect(opens(GATE_ON_BARRIER_M * 3)).toBe(false); |
There was a problem hiding this comment.
This test does not pin the boundary its own docstring says it pins. gateNear offsets in longitude by M, a latitude metre, so at 51.5° N the real offsets are 1 m → 0.62 m and 3 m → 1.87 m. The docstring acknowledges the scaling but then claims "an off-by-a-comparison in nearestOnLine would be invisible without this" — it still would be: nothing here lands within 0.38 m of GATE_ON_BARRIER_M, and > vs >= at the boundary is untestable at 0.62 m. The bracket only narrows from (0.12 m, 2.49 m) — the existing 0.2 and *4 cases — to (0.62 m, 1.87 m).
Convert the offset properly and the test becomes what it claims to be:
/** A metre EAST in degrees of longitude at ORIGIN's latitude. */
const LNG_M = M / Math.cos((ORIGIN.lat * Math.PI) / 180);then have gateNear use LNG_M for the offset and assert opens(GATE_ON_BARRIER_M * 0.95) === true / opens(GATE_ON_BARRIER_M * 1.05) === false. The other cases in this block (0.2, *3, *4) stay correct under the change — they only move further from the boundary, and crossingAt's ±5 m arms are unaffected by a 1.6× east scaling.
1 participant
Acts on the review of #277 and #278. Restricts which ways may corroborate a gate, which removed the last corpus opening -- both the rule originally produced were false positives. Plus the T-junction decision, the stale docstrings, and two JSDoc defects in the r488 recorder.