Skip to content

Commit 5df3642

Browse files
bedaHovorkaclaude
andcommitted
fix(#541): address code-review findings on perception ports
Tighten the perception-port slice (PR #722) per code review: - Replace Train.getTimetable() with four narrow read-only val properties (timetableOriginName, timetableDestinationName, scheduledDepartureTime, scheduledArrivalTime). The accessor handed out the live mutable Timetable (its in/out/length are var; DynamicInOut endpoints expose mutable semaphore state). The port only needs four scalar snapshot values, so the leak was an unnecessary encapsulation regression on Train's public API. Update the sole caller (DefaultNetworkPerceptionPort.toTimetableReading) and the test mock wiring. - Cache block lookups in DefaultNetworkPerceptionPort, mirroring the existing semaphoreCache/semaphoreByName pattern. blockCache + blockById are built once at construction (graph edges are stable for the lifetime of a context); blockOccupancy(id) becomes an O(1) map lookup instead of an O(n) graph re-scan recomputing blockId per block. - Add the missing perception-port design ADR (docs/superpowers/specs/2026-07-07-541-perception-port-design.md) following the specs/ convention, recording the four key decisions: SimulationEnvironment backing, eager lifetime-of-port caches, direct-construction (non-Koin) wiring, and the narrow Train property surface. No behavior change; DefaultNetworkPerceptionPortTest (20 tests) and RuleBasedDispatcherDeterminismTest (10 real vyhybna.xml runs) stay green. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent de482ea commit 5df3642

4 files changed

Lines changed: 218 additions & 35 deletions

File tree

core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/ports/DefaultNetworkPerceptionPort.kt

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -82,14 +82,44 @@ class DefaultNetworkPerceptionPort(
8282
private val semaphoreByName: Map<String, DynamicRailSemaphore> =
8383
semaphoreCache.filter { it.name.isNotBlank() }.associateBy { it.name }
8484

85+
// ── Block cache built once at construction ─────────────────────────────
86+
87+
/**
88+
* All [DynamicTrackBlock] edges in the network, read once from the simulation
89+
* graph during construction. The graph edges are stable for the lifetime of a
90+
* simulation context — blocks are not added or removed at runtime — so, like
91+
* [semaphoreCache], this list is built once and never invalidated. Live state
92+
* (occupancy, train id) is read on cached references at query time in
93+
* [DynamicTrackBlock.toReading].
94+
*
95+
* Note: [blockById] is keyed by [blockId], which falls back to `"unknown"` for
96+
* blocks that have neither an explicit name nor named endpoint separators.
97+
* Two such blocks would collide on the same id and one would shadow the other
98+
* in [blockById]. This is an edge case that the bundled `vyhybna.xml` fixture
99+
* does not exercise (all blocks there are named); callers needing unambiguous
100+
* lookup should rely on explicitly named blocks.
101+
*/
102+
private val blockCache: List<DynamicTrackBlock> =
103+
env
104+
.getGraph()
105+
.values()
106+
.filterIsInstance<DynamicTrackBlock>()
107+
.toList()
108+
109+
/**
110+
* Index from [blockId] → [DynamicTrackBlock] for O(1) name-based lookup, built
111+
* from [blockCache]. Mirrors [semaphoreByName].
112+
*/
113+
private val blockById: Map<String, DynamicTrackBlock> =
114+
blockCache.associateBy { blockId(it) }
115+
85116
// ── Block helper ──────────────────────────────────────────────────────
86117

87118
/**
88-
* All dynamic track blocks in the network, read directly from the simulation
89-
* graph on each call. The graph edges are stable; blocks are not added or
90-
* removed at runtime.
119+
* All dynamic track blocks in the network, served from the construction-time
120+
* [blockCache] (see its KDoc for the stability rationale).
91121
*/
92-
private fun allBlocks(): Collection<DynamicTrackBlock> = env.getGraph().values().filterIsInstance<DynamicTrackBlock>()
122+
private fun allBlocks(): Collection<DynamicTrackBlock> = blockCache
93123

94124
/**
95125
* Derives a stable, human-readable identifier for a [DynamicTrackBlock].
@@ -142,7 +172,7 @@ class DefaultNetworkPerceptionPort(
142172
// ── NetworkPerceptionPort — block occupancies ─────────────────────────
143173

144174
override fun blockOccupancy(blockId: String): BlockOccupancyReading? {
145-
val block = allBlocks().firstOrNull { blockId(it) == blockId } ?: return null
175+
val block = blockById[blockId] ?: return null
146176
return block.toReading()
147177
}
148178

@@ -187,16 +217,14 @@ class DefaultNetworkPerceptionPort(
187217

188218
override fun allTrainTimetables(): List<TimetableReading> = activeTrains().map { it.toTimetableReading() }
189219

190-
private fun Train.toTimetableReading(): TimetableReading {
191-
val timetable = getTimetable()
192-
return TimetableReading(
220+
private fun Train.toTimetableReading(): TimetableReading =
221+
TimetableReading(
193222
trainId = name,
194-
originInOutName = timetable.getIn().name,
195-
destinationInOutName = timetable.getOut().name,
196-
scheduledDepartureTime = timetable.getInTime().value,
197-
scheduledArrivalTime = timetable.getOutTime().value
223+
originInOutName = timetableOriginName,
224+
destinationInOutName = timetableDestinationName,
225+
scheduledDepartureTime = scheduledDepartureTime,
226+
scheduledArrivalTime = scheduledArrivalTime
198227
)
199-
}
200228

201229
// ── Grid scan ─────────────────────────────────────────────────────────
202230

core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/sim/Train.kt

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,16 +1216,33 @@ class Train :
12161216
get() = timetable.getIn()
12171217

12181218
/**
1219-
* Returns the train's timetable for agent perception.
1219+
* Read-only snapshot of the origin/destination InOut names and scheduled times
1220+
* derived from this train's [Timetable], for agent perception.
12201221
*
1221-
* Exposes origin/destination InOuts and scheduled times to the
1222-
* [cz.vutbr.fit.interlockSim.ports.NetworkPerceptionPort] implementation without
1223-
* requiring the perception layer to hold a direct reference to the private field.
1222+
* These four properties expose exactly what the
1223+
* [cz.vutbr.fit.interlockSim.ports.NetworkPerceptionPort] implementation needs to
1224+
* build a [cz.vutbr.fit.interlockSim.ports.TimetableReading] — scalar values only,
1225+
* no live references to the [Timetable] or its mutable [DynamicInOut] endpoints.
1226+
* Handing out the [Timetable] itself would let callers mutate reachable sim state
1227+
* (e.g. via [Timetable.reverseDirection]); these narrow properties keep the
1228+
* perception data flow strictly one-directional (snapshot out, nothing back in).
12241229
*
1225-
* @return The [Timetable] governing this train's current journey.
12261230
* @since Issue #541 (SP0.2 — Goal 10 sensor ports)
12271231
*/
1228-
fun getTimetable(): Timetable = timetable
1232+
val timetableOriginName: String
1233+
get() = timetable.getIn().name
1234+
1235+
/** @see timetableOriginName */
1236+
val timetableDestinationName: String
1237+
get() = timetable.getOut().name
1238+
1239+
/** @see timetableOriginName */
1240+
val scheduledDepartureTime: Double
1241+
get() = timetable.getInTime().value
1242+
1243+
/** @see timetableOriginName */
1244+
val scheduledArrivalTime: Double
1245+
get() = timetable.getOutTime().value
12291246

12301247
/**
12311248
* Track section where the train's front is currently located.

core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/ports/DefaultNetworkPerceptionPortTest.kt

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import assertk.assertions.isEqualTo
1616
import assertk.assertions.isNull
1717
import cz.vutbr.fit.interlockSim.context.RailwayNetGrid
1818
import cz.vutbr.fit.interlockSim.context.SimulationEnvironment
19-
import cz.vutbr.fit.interlockSim.objects.cells.DynamicInOut
2019
import cz.vutbr.fit.interlockSim.objects.cells.DynamicRailSemaphore
2120
import cz.vutbr.fit.interlockSim.objects.cells.Signal
2221
import cz.vutbr.fit.interlockSim.objects.core.Cell
@@ -25,8 +24,6 @@ import cz.vutbr.fit.interlockSim.objects.core.TrackFacility
2524
import cz.vutbr.fit.interlockSim.objects.core.TrackOccupant
2625
import cz.vutbr.fit.interlockSim.objects.tracks.DynamicTrackBlock
2726
import cz.vutbr.fit.interlockSim.objects.tracks.TrackSection
28-
import cz.vutbr.fit.interlockSim.sim.Time
29-
import cz.vutbr.fit.interlockSim.sim.Timetable
3027
import cz.vutbr.fit.interlockSim.sim.Train
3128
import cz.vutbr.fit.interlockSim.util.ExtendedUnorientedGraph
3229
import cz.vutbr.fit.interlockSim.util.Point
@@ -88,25 +85,18 @@ class DefaultNetworkPerceptionPortTest {
8885
destName: String = "B",
8986
departureTime: Double = 0.0,
9087
arrivalTime: Double = 60.0
91-
): Train {
92-
val inOut = mockk<DynamicInOut>(relaxed = true)
93-
every { inOut.name } returns originName
94-
val outOut = mockk<DynamicInOut>(relaxed = true)
95-
every { outOut.name } returns destName
96-
val timetable = mockk<Timetable>(relaxed = true)
97-
every { timetable.getIn() } returns inOut
98-
every { timetable.getOut() } returns outOut
99-
every { timetable.getInTime() } returns Time(departureTime)
100-
every { timetable.getOutTime() } returns Time(arrivalTime)
101-
return mockk<Train>(relaxed = true).also {
88+
): Train =
89+
mockk<Train>(relaxed = true).also {
10290
every { it.name } returns name
10391
every { it.getVelocity() } returns velocity
10492
every { it.getAcceleration() } returns acceleration
10593
every { it.totalDistance } returns totalDistance
10694
every { it.frontSection } returns frontSection
107-
every { it.getTimetable() } returns timetable
95+
every { it.timetableOriginName } returns originName
96+
every { it.timetableDestinationName } returns destName
97+
every { it.scheduledDepartureTime } returns departureTime
98+
every { it.scheduledArrivalTime } returns arrivalTime
10899
}
109-
}
110100

111101
/**
112102
* Builds a [SimulationEnvironment] stub backed by a 3×1 grid.
Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
# Design: Network Perception Port (#541)
2+
3+
**Issue:** [bedaHovorka/interlockSim#541](https://github.com/bedaHovorka/interlockSim/issues/541)
4+
**Parent:** [bedaHovorka/interlockSim#532](https://github.com/bedaHovorka/interlockSim/issues/532) — Goal 10 DISPATCHER agent
5+
**Canonical authority:** [#532](https://github.com/bedaHovorka/interlockSim/issues/532) issue body. Where this
6+
document conflicts with it, the #532 body wins.
7+
**Status:** Final design record (2026-07-07), retro-documenting the perception port shipped in PR #722.
8+
9+
---
10+
11+
## 1. Summary
12+
13+
This slice introduces a **read-only sensor layer** between the simulation and future
14+
perception-driven dispatchers. `cz.vutbr.fit.interlockSim.ports.NetworkPerceptionPort` is a
15+
KMP-clean interface in `:core` `commonMain` whose methods each return an immutable snapshot
16+
(`*Reading` value type) of one facet of the network: signal aspects, block occupancies, train
17+
positions, and train timetables. The default implementation,
18+
`DefaultNetworkPerceptionPort`, is backed by the live `SimulationEnvironment` plus a supplier of
19+
currently active trains.
20+
21+
The port is **wired into `DispatcherTickContext`** but **not yet consumed** by
22+
`RuleBasedDispatcher` — the current dispatcher is a plain FIFO/topology dispatcher that needs no
23+
perception. The port exists so that the Goal 10 LLM dispatcher track (SP1.6, Koog tools) can observe
24+
the network through a stable, decoupled, value-typed surface instead of reaching into `sim/`
25+
internals. Each interface method maps one-to-one to a future Koog tool.
26+
27+
No actuator behavior, dispatch logic, or simulation state changes belong in this slice. The port
28+
is purely observational.
29+
30+
---
31+
32+
## 2. Goals and Non-Goals
33+
34+
### Goals
35+
36+
1. Give future dispatchers a read-only, string-keyed view of the network.
37+
2. Return **immutable snapshots only** — no live mutable references out to callers.
38+
3. Stay KMP-clean: no `java.*`/`javax.*` in `commonMain` (enforced by
39+
`:core:checkCoreCommonMainPurity`).
40+
4. Introduce the abstraction **without changing dispatch behavior**`RuleBasedDispatcher` keeps
41+
its current semantics.
42+
5. Keep the wiring conservative for `sim/`: no new Koin bindings, no refactoring of working sim
43+
logic.
44+
45+
### Non-Goals
46+
47+
1. Not a dispatch API — the port observes; it does not decide or act.
48+
2. Not consumed by `RuleBasedDispatcher` in this slice.
49+
3. No thread-safety contract beyond single-threaded kDisco access (documented on the class).
50+
4. No block-occupancy or signal change events/streaming — synchronous pull queries only.
51+
52+
---
53+
54+
## 3. Package and Source Location
55+
56+
All types live in `:core` `commonMain`, package `cz.vutbr.fit.interlockSim.ports`:
57+
58+
- `NetworkPerceptionPort.kt` — the interface.
59+
- `DefaultNetworkPerceptionPort.kt` — the `SimulationEnvironment`-backed implementation.
60+
- `SemaphoreReading.kt`, `BlockOccupancyReading.kt`, `TrainPositionReading.kt`,
61+
`TimetableReading.kt` — immutable `data class` value types (all `val`, primitives/`String`/
62+
`enum`/`Signal` only).
63+
64+
`commonMain` placement keeps the port available to both the JVM dispatcher and any future native
65+
target. The purity gate guarantees no JVM-only API leaks in.
66+
67+
---
68+
69+
## 4. Public API Surface
70+
71+
`NetworkPerceptionPort` exposes, per facet:
72+
73+
- **Signal aspects:** `signalAspect(name): SemaphoreReading?`, `allSignalAspects(): List<SemaphoreReading>`.
74+
- **Block occupancies:** `blockOccupancy(blockId): BlockOccupancyReading?`,
75+
`allBlockOccupancies(): List<BlockOccupancyReading>`.
76+
- **Train positions:** `trainPosition(trainId): TrainPositionReading?`,
77+
`allTrainPositions(): List<TrainPositionReading>`.
78+
- **Train timetables:** `trainTimetable(trainId): TimetableReading?`,
79+
`allTrainTimetables(): List<TimetableReading>`.
80+
81+
Each `*Reading` is a `data class` of immutable fields; `trainId`/`blockId` are `String?` or `String`.
82+
The interface KDoc notes the intended one-method-per-Koog-tool mapping for SP1.6.
83+
84+
---
85+
86+
## 5. Key Decisions
87+
88+
**D1 — Backed by `SimulationEnvironment`, not a parallel state copy.**
89+
The implementation reads the grid (`getRailWayNetGrid()`) and the graph (`getGraph()`) directly
90+
from the simulation environment. There is no duplicate shadow state to keep in sync; the
91+
environment is the single source of truth and the port is a read lens over it.
92+
93+
**D2 — Eager, lifetime-of-port caches; live state read at query time.**
94+
Both semaphores (`semaphoreCache` / `semaphoreByName`) and blocks (`blockCache` / `blockById`)
95+
are scanned once at construction and indexed by name/id. Cells and graph edges are stable for the
96+
lifetime of a simulation context (no cells/edges are added or removed at runtime), so the caches
97+
store **references** to the dynamic objects and are never invalidated. Live state
98+
(`sem.signal`, `block.getState()` / `trainName` / `occupant`) is read on those cached references at
99+
each query. This makes `signalAspect` / `blockOccupancy` O(1) lookups and `allSignalAspects` /
100+
`allBlockOccupancies` O(n) with no per-call graph re-scan. (The block cache was added after the
101+
initial PR in response to code review — see §7.)
102+
103+
**D3 — Direct construction in `ShuntingLoop.createTickContext()`, not Koin.**
104+
`perceptionPort` is built by direct construction inside `ShuntingLoop.createTickContext()`, with
105+
`activeTrains = { approvedTrains.toList() }` supplied as a lambda so it always reflects the
106+
currently approved trains at query time. This deliberately avoids a Koin binding, consistent with
107+
the conservative, minimal-change policy for the `sim/` package: the port is rebuilt per tick
108+
context, owns no global state, and introduces no DI lifecycle complexity. The Koin restriction for
109+
`sim/` was lifted 2026-03-20, but the conservative choice to *not* use it here is intentional and
110+
recorded.
111+
112+
**D4 — `Train` exposes narrow read-only scalar properties, not the live `Timetable`.**
113+
The port needs four timetable-derived values (origin name, destination name, scheduled departure
114+
time, scheduled arrival time). Rather than expose `fun getTimetable(): Timetable` — which would
115+
hand callers a live reference to the mutable `Timetable` (its `in`/`out`/`length` are `var`, and its
116+
`DynamicInOut` endpoints expose mutable semaphore state) — `Train` exposes four `val` properties
117+
(`timetableOriginName`, `timetableDestinationName`, `scheduledDepartureTime`,
118+
`scheduledArrivalTime`) delegating to the private `timetable` field. This keeps the perception
119+
data flow strictly one-directional: snapshots out, nothing mutable back in. (This was tightened
120+
after the initial PR in response to code review — see §7.)
121+
122+
---
123+
124+
## 6. Traceability to Goal 10
125+
126+
- **Parent:** #532 (Goal 10 DISPATCHER agent).
127+
- **Downstream consumers:** SP1.6 (Koog tool wiring — one port method → one tool, per the
128+
`NetworkPerceptionPort` KDoc), and the SP3.1 LLM dispatcher track documented in
129+
`docs/GOAL_10_SP3_1_LLM_MODEL_EVALUATION.md`.
130+
- **Sibling design:** `docs/superpowers/specs/2026-07-04-570-operating-vocabulary-dsl-design.md`
131+
(the typed vocabulary the dispatcher will emit/consume; the perception port is the *input* side).
132+
- **Paramount example:** `vyhybna.xml` — the port must fully describe this network's signals,
133+
blocks, and the single train's timetable.
134+
135+
---
136+
137+
## 7. Implementation Checklist
138+
139+
This slice shipped in PR #722 ("Define sensor ports for perception interface", closes #541). This
140+
document is the retro-record. The two post-review tightenings are:
141+
142+
- [x] D4: replaced `Train.getTimetable()` with four narrow `val` properties (encapsulation fix).
143+
- [x] D2: added the `blockCache` / `blockById` index so `blockOccupancy(id)` is an O(1) map lookup
144+
instead of an O(n) graph re-scan with recomputed `blockId` per block.
145+
146+
Tests: `DefaultNetworkPerceptionPortTest` (20 unit tests, MockK) covers the port contract;
147+
`RuleBasedDispatcherDeterminismTest` (10 real `vyhybna.xml` runs through kDisco) is the
148+
end-to-end guard that the `ShuntingLoop` rewrite + port wiring did not alter simulation behavior.

0 commit comments

Comments
 (0)