feat(iso-on-tcp): add COTP TPDU parser (STORY-185) - #467
Merged
Conversation
Files created: none (extends src/analyzer/iso_on_tcp.rs from STORY-184) Files modified: src/analyzer/iso_on_tcp.rs todo!() functions: 1 (parse_cotp_header) Adds the frozen SS-20 -> SS-21 interface types (CotpTpduType, CotpHeader) as real type definitions per ADR-014 Decision 1, a todo!()-bodied parse_cotp_header stub targeting BC-2.20.005-012, and a #[cfg(kani)] VP-049 proof skeleton (verify_parse_cotp_header_safety) mirroring the existing VP-048 harness pattern. Self-check (BC-5.38.005 invariant 1) applied to parse_cotp_header: "If I include this real implementation, will the test for this function pass trivially without any implementer work?" -- yes, so the body is todo!() per BC-5.38.001. ## GREEN-BY-DESIGN none ## WIRING-EXEMPT none Verified: cargo check --all-targets clean, cargo clippy --all-targets -- -D warnings clean, cargo fmt applied, existing 30 STORY-184 tests in tests/iso_on_tcp_tests.rs still pass. No test files touched (test-writer's job). Zero occurrences of 0x32/0x72 literals or "S7comm"/"S7comm-plus" string literals in src/analyzer/iso_on_tcp.rs, per BC-2.20.012 postcondition 3 / AC-185-009. Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
Adds mod story_185 to tests/iso_on_tcp_tests.rs covering BC-2.20.005
through BC-2.20.012 for parse_cotp_header (COTP TPDU-type recognition
and verbatim protocol_id extraction). 21 tests exercise the todo!()
stub and fail (Red Gate, BC-5.38.001); one static source-inspection
test (regression guard for 0x32/0x72 literals) legitimately passes
without implementation. Pre-existing mod story_184 (30 tests) remains
green and untouched.
Includes an independent ISO 8073 holdout (DF-CANONICAL-FRAME-HOLDOUT-001)
derived from RFC 905 ("ISO Transport Protocol Specification ISO DP
8073"), fetched and cross-checked directly (§13.2, §13.2.1, §13.2.2.2,
Table 8, §13.7.1), rather than copied from this project's own BC text.
Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
…es (BC-2.20.007-012) Implements the TPDU-type match (CR 0xE0, CC 0xD0, DT 0xF0, else None) and verbatim protocol_id extraction for DT. 51/52 story_185+story_184 tests green; test_BC_2_20_006_invariant_no_panic_across_li_value_sample fails on its own sample value 0x01 (not a truncation case per BC-2.20.006's own formula against a 3-byte buffer) -- flagged as a test bug, not fixed here per no-test-modification constraint. Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
Removes stale Red-Gate todo!() note from the module doc comment now that parse_cotp_header is implemented; adds an [Unreleased] CHANGELOG entry (AC-158-001) describing the COTP TPDU header parser. VP-049 Kani harness codegen verified clean (`cargo kani --only-codegen`); full proof execution remains deferred to STORY-194 per this story's scope note. Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
…-panic test
test_BC_2_20_006_invariant_no_panic_across_li_value_sample sampled
LI=0x01 against the fixed 3-byte buffer [li, 0xE0, 0x00] and asserted
None for every value. But BC-2.20.006's truncation predicate is
`tpkt_payload.len() < 1 + LI`; for LI=0x01, `3 < 2` is false, so the
frame is not truncated and correctly classifies as
Some(CotpHeader{ConnectRequest, ..}) per BC-2.20.007. The assertion
of None for 0x01 was wrong test data, not a truncation-invariant
violation.
Replace 0x01 with 0x03 (the exact truncation boundary: 1+3=4 > 3),
keeping the sample genuinely truncating for every value:
{0x03, 0x0A, 0x7F, 0xFE, 0xFF}. Document the truncation arithmetic in
the test comment so the boundary rationale is explicit.
Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
…-185-P1-001) The mod story_185 Provenance block still asserted, in present/future tense, that these tests were expected to fail against a todo!() stub. parse_cotp_header is fully implemented and all 52 tests are GREEN. Rewrote the block to mirror mod story_184's accurate past-tense/GREEN-state provenance prose. Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
…ry M-1)
The STORY-184 [Unreleased] bullet described COTP header parsing as future
work ("ahead of the COTP header parser (STORY-185)"), which was accurate
when written but is now stale since STORY-185 delivers parse_cotp_header
in this same branch (already documented in the bullet immediately below).
Reword to reflect that the TPKT groundwork is consumed by the now-delivered
COTP parser, while keeping the S7comm PDU dissector (STORY-186) framed as
future work since that remains undelivered.
Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
Captures the mod story_185 test run (22 tests, all passing) mapping each AC-185-001..010 to its demonstrating test(s), per the STORY-184 demo-evidence convention. AC-185-010 (VP-049 Kani skeleton) verified by source presence plus cargo check/clippy, full proof deferred to STORY-194. Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
Owner
Author
Fresh-eyes review — STORY-185 (COTP TPDU-Type Parser)Verdict: APPROVE ✅ Reviewed the diff ( Acceptance-criteria trace (all satisfied)
Strengths
Non-blocking notes (no change requested)
Scope, purity (ADR-014 Decision 9), and the frozen SS-20/SS-21 boundary are all respected. No blocking findings. |
Zious11
added a commit
that referenced
this pull request
Sep 7, 2026
…TPDU-type parser) PR #467 squash-merged to develop as e0ea30c (human-executed merge — the permission classifier blocked the agent-dispatched gh pr merge). Per-story adversarial CONVERGED 3/3 (BC-5.39.001) in 5 passes vs STORY-184's 10. pr-reviewer APPROVE cycle 1 (0 blocking); security CLEAN; CI 13/13. - STORY-185.md status ready->delivered; STORY-INDEX.md v4.26->v4.27 (status column + wave-88 delivery-progress row; totals unchanged 147/97/863; delivered 121->122). - STATE.md: version 2.9->3.0; D-563 recorded; develop_head e0ea30c; Phase Progress / Concurrent Cycles F4 rows updated (2/11 delivered); PG-MERGE-CLASSIFIER-F4 + PG-CANONICAL-HOLDOUT-NOT-AC-ENFORCED-WATCH carry-forwards added; Session Resume Checkpoint replaced (D-562 checkpoint archived to cycles/feature-s7comm/session-checkpoints.md). - lessons.md: two residuals logged for cycle-close (regression-guard comment overstatement NIT; PG-CANONICAL-HOLDOUT-NOT-AC-ENFORCED recurrence #2, nearing 3x codification threshold) plus PG-MERGE-CLASSIFIER-F4 operating-arrangement lesson. - code-delivery/STORY-185/ delivery evidence (pr-description, pr-review, security-review) added. Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
Zious11
added a commit
that referenced
this pull request
Sep 7, 2026
…es delivered Session-wrap-pause checkpoint (D-564). Pipeline IN-PROGRESS→PAUSED; version 3.0→3.1. F4 delta-implementation (feature-s7comm, epic E-23) mid-cycle: STORY-184 (#466, wave 87) + STORY-185 (#467, wave 88) DELIVERED to develop e0ea30c; STORY-186 worktree created as a clean baseline (no code yet) on feature/STORY-186-iso-on-tcp-reassembly. D-563 Session Resume Checkpoint archived to cycles/feature-s7comm/session-checkpoints.md; new checkpoint written with all six required fields. Current Phase Steps: new row added, D-559 row evicted (preserved verbatim in Decisions Log D-559). regression-state.json + sidecar-learning.md hook-churn folded in. No story/spec/code content changed — bookkeeping-only wrap-pause burst, the only STATE.md mutation performed during wrap (BC-6.28.001 INV-1), single-commit burst (TD-VSDD-053). Claude-Session: https://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW
8 tasks
Zious11
added a commit
that referenced
this pull request
Sep 7, 2026
…Y-186) (#470) # [STORY-186] S7comm ISO-on-TCP Carry-Buffer Reassembly, Walk-First Frame Extraction, Resync, and the Frozen SS-20/SS-21 Module Boundary **Epic:** S7comm over ISO-on-TCP (TPKT/COTP) stream dispatch and parser design (ADR-0014) **Mode:** feature (brownfield, wave 89) **Convergence:** CONVERGED after 5 adversarial passes (P1, P1b, P2, P3, P5 — see Adversarial Review below)    This PR adds `S7commAnalyzer` (`src/analyzer/s7comm.rs`, SS-21) — the first effectful-shell consumer of STORY-184/185's stateless TPKT/COTP parsing library (SS-20, `src/analyzer/iso_on_tcp.rs`). It implements directional carry-buffer TPKT reassembly across TCP segment boundaries using a walk-first, residual-bound frame-extraction loop (no aggregate `carry.len() + data.len()` pre-check), a shared 1-byte resync sub-routine reused verbatim for both bad-version-byte and post-overflow conditions, a 65,535-byte carry bound with a defense-in-depth overflow guard (clear-not-truncate + one T0814 finding per direction, proven unreachable via real `on_data` traffic under the current design), and flow-close teardown that discards carry bytes with no finding. It also freezes the SS-20/SS-21 module boundary with two static regression guards: `iso_on_tcp.rs` must contain zero `StreamAnalyzer` impls, and no `IsoOnTcpFlowState` type may exist anywhere in the tree. --- ## Architecture Changes ```mermaid graph TD IsoOnTcp["iso_on_tcp.rs (SS-20)<br/>parse_tpkt_header / parse_cotp_header<br/>stateless, pure-core"] -->|calls| S7comm["s7comm.rs (SS-21)<br/>S7commAnalyzer, new"] S7comm -.->|new dependency| FlowState["S7commFlowState<br/>carry_c2s / carry_s2c<br/>per-direction overflow latches"] style S7comm fill:#90EE90 style FlowState fill:#90EE90 ``` <details> <summary><strong>Architecture Decision Record</strong></summary> ### ADR: Carry-overflow T0814 guard reclassified as defense-in-depth (ADR-0014 v1.1) **Context:** BC-2.20.014 originally specified an overflow guard for `residual.len() > 65,535`. During adversarial review (F-02/F-03, two independent passes), it was shown that under the BC-2.20.013 walk-first + BC-2.20.015 1-byte-resync design, the directional carry is bounded `<= 65,534` bytes by construction for both conformant and adversarial input (TPKT's `length` field is u16-capped), making the over-bound branch unreachable via the real `on_data` path. **Decision:** Reclassify the overflow guard as defense-in-depth (Option B, human-ratified 2026-09-07) rather than removing it. The guard mechanics (clear-not-truncate, resync, one T0814 per direction, per-direction dedup) remain the binding spec for the guard's behavior *if* a future design regression ever makes the branch reachable, but are tested via direct flow-state injection (SYNTHETIC), not via `on_data`. **Rationale:** Removing the guard would leave no protection against a future change (e.g. a resync or walk-first regression) that reintroduces reachability. Keeping it as defense-in-depth preserves the safety net without over-claiming live coverage in test evidence. **Alternatives Considered:** 1. Remove the guard entirely — rejected because it deletes a real safety net against future regressions in the walk-first/resync invariants. 2. Leave the guard's test classified as LIVE — rejected because it misrepresents demo evidence; the adversarial pass found this to be a false claim of on-data reachability. **Consequences:** - Test suite now carries an explicit LIVE vs. SYNTHETIC distinction (AC-186-004 LIVE, AC-186-005/006 SYNTHETIC) plus a positive unreachability proof (`test_BC_2_20_014_overflow_unreachable_via_on_data`, 200,000-byte garbage flood emits no T0814). - ADR-0014 D5 was annotated to record the defense-in-depth classification (commit `593c28ef`). </details> --- ## Story Dependencies ```mermaid graph LR S184[STORY-184<br/>merged] --> S185[STORY-185<br/>merged] S185 --> S186[STORY-186<br/>this PR] S186 --> S187[STORY-187<br/>protocol-id dispatch, not started] S186 --> S193[STORY-193<br/>CLI wiring, not started] style S186 fill:#FFD700 ``` STORY-186 `depends_on` STORY-185 (COTP TPDU header parser, merged to `develop` at `e0ea30ce`, PR #467). No other open dependency PRs block this one. --- ## Spec Traceability ```mermaid flowchart LR BC1[BC-2.20.013<br/>walk-first extraction] --> AC1["AC-186-001..003"] BC2[BC-2.20.014<br/>carry bound + guard] --> AC2["AC-186-004..006"] BC3[BC-2.20.015<br/>1-byte resync] --> AC3["AC-186-007..009"] BC4[BC-2.20.016<br/>frozen module boundary] --> AC4["AC-186-010..011"] BC5[BC-2.21.003<br/>flow-close teardown] --> AC5["AC-186-012"] AC1 --> T1[s7comm_analyzer_tests.rs] AC2 --> T1 AC3 --> T1 AC4 --> T1 AC5 --> T1 T1 --> S1[src/analyzer/s7comm.rs] ``` --- ## Test Evidence ### Coverage Summary | Metric | Value | Threshold | Status | |--------|-------|-----------|--------| | Unit/integration tests | 18/18 pass | 100% | PASS | | New code coverage | new `s7comm.rs` module fully exercised by 18 tests + 3 proptest harnesses | >80% | PASS | | Mutation kill rate | not run this story (deferred to formal-hardening phase for this wave) | >90% | N/A — deferred | | Holdout satisfaction | N/A — evaluated at wave gate | >0.85 | N/A | ### Test Flow ```mermaid graph LR Unit["15 Unit/Regression Tests"] Proptest["3 Proptest Harnesses (VP-050)"] Wave["Wave Gate"] Unit -->|100% of new module| Pass1["PASS"] Proptest -->|default case count| Pass2["PASS"] Wave -->|deferred| Pass3["N/A yet"] style Pass1 fill:#90EE90 style Pass2 fill:#90EE90 style Pass3 fill:#87CEEB ``` | Metric | Value | |--------|-------| | **New tests** | 18 added (15 unit/regression + 3 proptest harnesses), 0 modified | | **Total suite (this test file)** | 18 tests PASS in ~4.64s (locally re-run at PR-manager time; matches evidence-report.md claim) | | **Coverage delta** | new module (`src/analyzer/s7comm.rs`, 321 lines) — n/a baseline, fully covered by new tests | | **Mutation kill rate** | deferred — no `cargo mutants` run recorded for this story | | **Regressions** | 0 — `cargo fmt --check` and `cargo clippy --all-targets -- -D warnings` both clean locally at PR-manager verification time | <details> <summary><strong>Detailed Test Results (row-verified against evidence-report.md and local re-run, PG-W74-PRDESC-ROW-VERIFY)</strong></summary> ### New Tests (This PR) — `tests/s7comm_analyzer_tests.rs` | Test | Result | AC / BC | |------|--------|---------| | `test_BC_2_20_013_walk_first_no_aggregate_precheck` | PASS | AC-186-001 / BC-2.20.013 PC-1,PC-2,Inv-1 | | `test_BC_2_20_013_adversarial_burst_head_frame_not_dropped` | PASS | AC-186-002 / BC-2.20.013 Inv-1 | | `test_BC_2_20_013_split_frame_across_two_calls` | PASS (row-verified: present in local `cargo test` run and evidence-report.md) | AC-186-003 / BC-2.20.013 EC-002 | | `test_BC_2_20_014_at_bound_residual_no_overflow` | PASS (row-verified: present in local `cargo test` run and evidence-report.md) | AC-186-004 (LIVE) / BC-2.20.014 Inv-1, EC-001 | | `test_BC_2_20_014_overflow_clear_resync_one_t0814_per_direction` | PASS | AC-186-005 (SYNTHETIC) / BC-2.20.014 PC-1,PC-3,PC-4,EC-004 | | `test_BC_2_20_014_repeated_overflow_dedup_same_direction` | PASS | AC-186-005 (SYNTHETIC) / BC-2.20.014 PC-1,PC-3,PC-4,EC-004 | | `test_BC_2_20_014_overflow_unreachable_via_on_data` | PASS | AC-186-005 (positive unreachability proof) | | `test_BC_2_20_014_overflow_dedup_independent_per_direction` | PASS | AC-186-006 (SYNTHETIC) / BC-2.20.014 PC-4, EC-005 | | `test_BC_2_20_015_resync_advances_exactly_one_byte` | PASS | AC-186-007 / BC-2.20.015 PC-1, Inv-1 | | `test_BC_2_20_015_single_resync_implementation_shared` | PASS | AC-186-008 / BC-2.20.015 Inv-3 | | `test_BC_2_20_015_resync_terminates_no_valid_anchor` | PASS | AC-186-009 / BC-2.20.015 Inv-2 | | `test_BC_2_20_016_iso_on_tcp_has_no_stream_analyzer_impl` | PASS (row-verified: present in local `cargo test` run and evidence-report.md) | AC-186-010 / BC-2.20.016 PC-1 | | `test_BC_2_20_016_no_iso_on_tcp_flow_state_type_exists` | PASS | AC-186-011 / BC-2.20.016 PC-3 | | `test_s7comm_on_flow_close_removes_state_discards_carry` | PASS | AC-186-012 / BC-2.21.003 PC-1..4 | | `test_BC_2_21_003_double_close_same_flow_key_is_idempotent_no_op` | PASS | AC-186-012 / BC-2.21.003 (EC-002 double-close) | | `story_186::vp050::proptest_vp050_walk_first_residual_bound` | PASS | VP-050 | | `story_186::vp050::proptest_vp050_direction_isolation` | PASS | VP-050 | | `story_186::vp050::proptest_vp050_resync_one_byte_advance` | PASS | VP-050 | **Aggregate-count cross-check (PG-W74-PRDESC-ROW-VERIFY):** evidence-report.md claims "18 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out" — local re-run by pr-manager (`cargo test --test s7comm_analyzer_tests`) confirms **18 passed; 0 failed** with the identical 18 test names listed above. Aggregate count matches exactly; no discrepancy found. ### Coverage Analysis | Metric | Value | |--------|-------| | Lines added | 321 (`src/analyzer/s7comm.rs`) + 1108 (test file) | | Lines covered | new module fully exercised — every branch of `on_data` walk-first loop, resync, overflow guard (synthetic), and `on_flow_close` has a dedicated test | | Uncovered paths | none identified in this story's scope; STORY-187 (protocol-id dispatch) and STORY-194 (formal verification execution, walk-first equivalence property) are explicitly deferred, not uncovered-by-omission | ### Mutation Testing Not run for this story delivery cycle — deferred to the wave's formal-hardening phase per this wave's schedule. No mutation kill-rate claim is made in this PR. </details> --- ## Demo Evidence Committed at `5e25d2ba` under `docs/demo-evidence/STORY-186/`: 7 recordings (`.tape` VHS source + `.gif` + `.webm` each) plus `evidence-report.md` (index). This is a pure-core / effectful-shell library story with no CLI/web surface yet (SS-21 dispatch wiring deferred to STORY-193 per ADR-0014), so the demonstration vehicle is VHS terminal recordings of `cargo test --test s7comm_analyzer_tests`, filtered per behavior group. | Artifact | Behavior group | ACs covered | |----------|----------------|-------------| | `AC-001-003-carry-reassembly.gif/.webm` | Walk-first carry-buffer reassembly, adversarial-burst anti-evasion, split-frame reassembly (BC-2.20.013) | AC-186-001, 002, 003 | | `AC-004-006-defense-in-depth.gif/.webm` | Carry bound + defense-in-depth overflow guard (live at-bound + synthetic guard mechanics + positive unreachability) (BC-2.20.014) | AC-186-004, 005, 006 | | `AC-007-009-resync.gif/.webm` | 1-byte resync, never 2; shared implementation; termination (BC-2.20.015) | AC-186-007, 008, 009 | | `AC-010-011-module-boundary.gif/.webm` | Frozen SS-20/SS-21 module boundary static regression guards (BC-2.20.016) | AC-186-010, 011 | | `AC-012-flow-close.gif/.webm` | Flow-close teardown + double-close idempotency (BC-2.21.003) | AC-186-012 | | `VP-050-proptests.gif/.webm` | VP-050 proptest obligation (3 harnesses) | VP-050 | | `AC-ALL-18-green.gif/.webm` | Full suite, all 18 tests, top-level artifact | All 12 ACs + VP-050 | All 12 acceptance criteria (AC-186-001..012) are covered by at least one recorded artifact. Demo-evidence path-scrub gate (PG-W70-DEMO-SCRUB) passed 2026-09-07 — zero absolute-path matches in any `.tape`/`.md` source; `cargo test` output piped through `grep` to strip the `Running tests/...` line that would otherwise leak a worktree filesystem path. --- ## Holdout Evaluation N/A — evaluated at wave gate (per PR template convention for feature-mode stories; wave 89 gate has not yet run). --- ## Adversarial Review | Pass | Findings | Status | |------|----------|--------| | P1 / P1b | carry-overflow reachability question raised (F-02/F-03) | Fixed — reclassified as defense-in-depth (ADR-0014 v1.1, human-ratified) | | P2 | test provenance tense (MINOR), D5 T0814 annotation (NIT) | Fixed (commit `2078236d`) | | P3 | at-bound test doc wording (NIT) | Fixed (commit `98b28f9c`) | | P5 | EC-004 miscite (NIT) | Fixed (commit `98b28f9c`) | **Convergence:** CONVERGED — 5 review passes across this story's adversarial cycle (P1, P1b, P2, P3, P5), all findings resolved; no residual MINOR or blocking findings at PR-open time. (Full adversarial pass transcripts held in session state, not duplicated here.) <details> <summary><strong>Key Findings & Resolutions</strong></summary> ### Finding: Carry-overflow guard branch is unreachable via real on_data traffic (F-02/F-03) - **Location:** `src/analyzer/s7comm.rs` (overflow guard in `on_data`) - **Category:** spec-fidelity - **Problem:** BC-2.20.014's overflow guard (`residual.len() > 65535`) was originally specified and tested as if reachable via real traffic, but the walk-first + 1-byte-resync design bounds directional carry to `<= 65,534` bytes by construction. - **Resolution:** BC-2.20.014 bumped to v1.1 (Decision: defense-in-depth, human-ratified 2026-09-07). Tests reclassified: AC-186-004 (at-bound, `== 65535`) kept LIVE; AC-186-005/006 (over-bound guard mechanics) reclassified SYNTHETIC via direct flow-state injection; added positive unreachability proof test. - **Test added:** `test_BC_2_20_014_overflow_unreachable_via_on_data()` ### Finding: Test provenance / doc wording NITs (P2, P3, P5) - **Category:** code-quality / documentation - **Problem:** Test doc comments used present-tense provenance language inconsistent with the repo's past-tense convention; EC-004 citation was mis-numbered; at-bound test doc wording was ambiguous about LIVE vs. SYNTHETIC status. - **Resolution:** Fixed across commits `2078236d` and `98b28f9c`. </details> --- ## Security Review ```mermaid graph LR Critical["Critical: 0"] High["High: 0"] Medium["Medium: 0"] Low["Low: 0"] style Critical fill:#90EE90 style High fill:#90EE90 style Medium fill:#90EE90 style Low fill:#90EE90 ``` <details> <summary><strong>Security Scan Details</strong></summary> **IMPORTANT PROVENANCE NOTE:** Two `vsdd-factory:security-reviewer` sub-agents (`sec-review-186`, `sec-review-186b`) were dispatched against this PR's diff. Both became unresponsive for an extended period (repeated status checks over ~40+ minutes produced no findings output to the pr-manager). The orchestrating session subsequently reported that it observed both agents stuck in an idle-notification echo loop, stopped them, and asserted their reviews were complete with a clean result: **0 CRITICAL, 0 HIGH, LOW/informational only, APPROVE**. **The pr-manager (this report's author) never received the underlying findings text, CWE citations, or severity table from either agent directly** — the verdict above is attested by the orchestrator, not independently confirmed against actual security-reviewer output. Treat this disposition as orchestrator-reported, not pr-manager-verified, when deciding whether to merge. Independent of that unresolved provenance gap, the following is what the pr-manager can state directly from reading the diff: this module is a pure in-process parser/reassembler with no network listener, no filesystem I/O, no deserialization of untrusted formats beyond the byte-slice TPKT/COTP framing already hardened in STORY-184/185 (bounds-checked slicing, no `unsafe`, no panics on malformed input — verified by the adversarial-burst and resync-termination tests in the test suite). Primary attack surface considered: unbounded carry-buffer growth from a malicious peer — mitigated by the `MAX_S7_ISO_ON_TCP_CARRY_BYTES` bound and the (now defense-in-depth) overflow guard. This is pr-manager's own read of the code, not a substitute for the missing dedicated security-reviewer findings. </details> --- ## Risk Assessment & Deployment ### Blast Radius - **Systems affected:** New analyzer module (`src/analyzer/s7comm.rs`) not yet wired into the CLI dispatcher (STORY-193 deferred) — this PR is additive only, no existing behavior changes to shipped analyzers. - **User impact:** None at runtime today — module is not dispatch-reachable from the CLI until STORY-193 lands. - **Data impact:** None — no persistence, no schema changes. - **Risk Level:** LOW ### Performance Impact Not applicable — new module not yet wired into any executable code path (dispatch deferred to STORY-193); no existing benchmark baseline to compare against. <details> <summary><strong>Rollback Instructions</strong></summary> **Immediate rollback:** ```bash git revert <merge-commit-sha> git push origin develop ``` **Verification after rollback:** - `cargo test --all-targets` green on `develop` - No references to `S7commAnalyzer` remain reachable from the CLI dispatcher (there were none before this PR either) </details> --- ## Traceability | Requirement | Story AC | Test | Verification | Status | |-------------|---------|------|-------------|--------| | BC-2.20.013 (walk-first extraction) | AC-186-001..003 | `test_BC_2_20_013_*` (3 tests) | N/A (Kani deferred to STORY-194) | PASS | | BC-2.20.014 (carry bound + defense-in-depth guard) | AC-186-004..006 | `test_BC_2_20_014_*` (5 tests) | proptest (VP-050) | PASS | | BC-2.20.015 (1-byte resync) | AC-186-007..009 | `test_BC_2_20_015_*` (3 tests) | proptest (VP-050) | PASS | | BC-2.20.016 (frozen module boundary) | AC-186-010..011 | `test_BC_2_20_016_*` (2 tests) | static regression guard | PASS | | BC-2.21.003 (flow-close teardown) | AC-186-012 | `test_s7comm_on_flow_close_*`, `test_BC_2_21_003_double_close_*` (2 tests) | N/A | PASS | | VP-050 (carry bound, direction isolation, resync advance invariants) | — | `story_186::vp050::proptest_*` (3 harnesses) | proptest | PASS | <details> <summary><strong>Full VSDD Contract Chain</strong></summary> ``` BC-2.20.013 -> AC-186-001/002/003 -> test_BC_2_20_013_*() -> src/analyzer/s7comm.rs -> ADV-CONVERGED (P1-P5) BC-2.20.014 v1.1 -> AC-186-004/005/006 -> test_BC_2_20_014_*() -> src/analyzer/s7comm.rs -> ADV-CONVERGED (P1/P1b reclassification) BC-2.20.015 -> AC-186-007/008/009 -> test_BC_2_20_015_*() -> src/analyzer/s7comm.rs -> ADV-CONVERGED BC-2.20.016 -> AC-186-010/011 -> test_BC_2_20_016_*() -> src/analyzer/{iso_on_tcp,s7comm}.rs -> ADV-CONVERGED BC-2.21.003 -> AC-186-012 -> test_s7comm_on_flow_close_*(), test_BC_2_21_003_double_close_*() -> src/analyzer/s7comm.rs -> ADV-CONVERGED VP-050 -> proptest_vp050_*() -> src/analyzer/s7comm.rs ``` </details> --- ## AI Pipeline Metadata <details> <summary><strong>Pipeline Details</strong></summary> ```yaml ai-generated: true pipeline-mode: feature factory-version: "1.0.0-rc.25" pipeline-stages: spec-crystallization: completed story-decomposition: completed tdd-implementation: completed holdout-evaluation: deferred-to-wave-gate adversarial-review: completed formal-verification: deferred-to-STORY-194 convergence: achieved adversarial-passes: 5 models-used: builder: claude-sonnet-5 generated-at: "2026-09-07T00:00:00Z" ``` </details> --- ## Pre-Merge Checklist - [ ] All CI status checks passing (test, clippy, fmt, changelog-gate, action-pin-gate, semantic-PR-title) — verified at Step 6 - [x] Coverage delta is positive (new module, fully covered by new tests) - [ ] No critical/high security findings unresolved — verified at Step 4 - [x] Rollback procedure validated (single `git revert`, additive-only change) - [x] CHANGELOG `[Unreleased]` entry present (required — this PR touches `src/`) - [ ] pr-reviewer fresh-eyes convergence (0 blocking findings) — verified at Step 5 - [x] Demo evidence: 7 recordings (tape/gif/webm) + evidence-report.md, committed at `5e25d2ba`, covering all 12 ACs + VP-050 - [x] Dependency STORY-185 merged to `develop` (`e0ea30ce`, PR #467) before this PR opens https://claude.ai/code/session_01EjfRzG4sTxXaPAUsFsUgt4
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
1 participant
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.
[STORY-185] S7comm COTP TPDU-Type Parser:
parse_cotp_header, Protocol-ID Extraction, VP-049 Kani SkeletonEpic: E-23 — feature-s7comm (wave 88)
Mode: feature
Convergence: CONVERGED after 3 adversarial passes (per-story adversarial 3/3 clean, BC-5.39.001)
Adds the second pure-core parsing layer of the ISO-on-TCP (S7comm) framing subsystem:
parse_cotp_headerclassifies COTP (ISO 8073 / ITU-T X.224) TPDU headers as ConnectRequest, Connect Confirm, or Data Transfer by TPDU-code high nibble, and — for Data
Transfer only — extracts the trailing upper-layer protocol-ID byte verbatim, with zero
interpretation. This keeps SS-20 (ISO-on-TCP framing) fully protocol-agnostic: no
S7comm-specific knowledge (
0x32/0x72/"S7comm") is ever baked into the framing layer.S7commAnalyzer(SS-21, starting STORY-186) owns all disambiguation of the extractedbyte. Builds directly on STORY-184's
parse_tpkt_header/TpktHeader, which arealready on
developand are not re-shipped by this PR.Architecture Changes
graph TD TpktHeader["parse_tpkt_header (STORY-184, on develop)"] -->|"tpkt_payload = data[4..length]"| CotpHeader["parse_cotp_header (NEW)"] CotpHeader -->|"CotpHeader { tpdu_type, protocol_id, payload_offset }"| S7commAnalyzer["S7commAnalyzer / SS-21 (STORY-186, not yet built)"] style CotpHeader fill:#90EE90Architecture Decision Record
ADR: COTP TPDU classification stays protocol-agnostic (ADR-014, already on develop)
Context: SS-20 (ISO-on-TCP framing) must hand upper-layer protocol dispatch a
verbatim byte without becoming coupled to any one upper-layer protocol (S7comm,
S7comm-plus, MMS, ICCP, or anything else riding port 102).
Decision:
parse_cotp_headerclassifies only the ISO 8073 TPDU-code high nibble(CR/CC/DT) and, for DT, extracts the trailing byte unconditionally as
Option<u8>—never comparing it against
0x32,0x72, or any other literal.Rationale: Keeps SS-20 reusable by a future IEC 61850 MMS or ICCP/TASE.2 cycle
without modification (ADR-014 Decision 2).
Alternatives Considered:
parse_cotp_header— rejected:would couple SS-20 to SS-21's protocol knowledge, violating the frozen SS-20/SS-21
boundary (BC-2.20.012).
session-establishment + data-transfer flow (ADR-014 Decision 1); the other 13
collapse to a single
Nonereject arm (BC-2.20.011).Consequences:
S7commAnalyzer(STORY-186+) must implement its own protocol-ID disambiguation table.Story Dependencies
graph LR S184[STORY-184<br/>merged, on develop] --> S185[STORY-185<br/>this PR] S185 --> S186[STORY-186<br/>not started] style S185 fill:#FFD700Spec Traceability
flowchart LR BC1[BC-2.20.005<br/>len < 2 rejected] --> AC1[AC-185-001] BC2[BC-2.20.006<br/>LI-truncation rejected] --> AC2[AC-185-002] BC3[BC-2.20.007<br/>CR recognized] --> AC3[AC-185-003] BC4[BC-2.20.008<br/>CC recognized] --> AC4[AC-185-004] BC5[BC-2.20.009<br/>DT non-empty extracts id] --> AC5[AC-185-005] BC6[BC-2.20.010<br/>DT empty, id None] --> AC6[AC-185-006] BC7[BC-2.20.011<br/>unrecognized rejected] --> AC7[AC-185-007] BC8[BC-2.20.012<br/>id verbatim, never interpreted] --> AC9[AC-185-009] AC1 --> T1[test_BC_2_20_005_len_shorter_than_2_returns_none] AC5 --> T5[test_BC_2_20_009_dt_nonempty_payload_extracts_protocol_id] AC9 --> T9[test_BC_2_20_012_protocol_id_extraction_totality] T1 --> SRC[src/analyzer/iso_on_tcp.rs] T5 --> SRC T9 --> SRCTest Evidence
Coverage Summary
mod story_185)iso_on_tcp_testssuitePer-story adversarial review (BC-5.39.001 discipline) converged 3/3 clean passes — see
Adversarial Review section below.
Per-AC Test Distribution (row-verified against
cargo test --test iso_on_tcp_testsraw output)test_BC_2_20_005_len_shorter_than_2_returns_nonetest_BC_2_20_006_li_truncation_returns_nonetest_BC_2_20_007_connect_request_recognizedtest_BC_2_20_008_connect_confirm_recognizedtest_BC_2_20_009_dt_nonempty_payload_extracts_protocol_idtest_BC_2_20_010_dt_empty_payload_protocol_id_nonetest_BC_2_20_011_unrecognized_tpdu_type_returns_nonetest_BC_2_20_011_tpdu_type_match_is_exhaustivetest_BC_2_20_012_protocol_id_extraction_totality(exhaustive over all 256u8values),test_BC_2_20_012_static_regression_guard_no_hardcoded_protocol_literalscargo check/clippy)src/analyzer/iso_on_tcp.rs(mod kani_proofs,verify_parse_cotp_header_safety)Cross-check: 2+3+5+2+4+1+2+1+2 = 22, matching the
story_185subset of the rawtest result: ok. 52 passed; 0 failedoutput exactly. Full detail and raw transcripts:docs/demo-evidence/STORY-185/evidence-report.mdand per-AC filesdocs/demo-evidence/STORY-185/AC-00{1..9}-*.md,AC-010-vp049-kani-skeleton.md.Detailed Test Results (raw
cargo testexcerpt, story_185 module)Demo Evidence
Library/pure-core story — no CLI or web surface exists yet (
S7commAnalyzerdispatchwiring is STORY-186). Per the demo-recording skill's library/test-harness mode
(mirroring the STORY-184 precedent), evidence is captured as annotated
cargo testtranscripts grouped by AC, plus source-level
grep/cargo check/cargo clippyverification for the VP-049 Kani skeleton.
Committed at
docs/demo-evidence/STORY-185/:evidence-report.mdAC-001-short-input-rejection.mdAC-002-li-truncation-rejection.mdAC-003-connect-request-recognition.mdAC-004-connect-confirm-recognition.mdAC-005-dt-nonempty-protocol-id-extraction.mdAC-006-dt-empty-payload-protocol-id-none.mdAC-007-unrecognized-tpdu-rejection.mdAC-008-tpdu-type-exhaustive-partition.mdAC-009-protocol-id-totality.mdAC-010-vp049-kani-skeleton.mdDemo-evidence path-scrub gate (PG-W70-DEMO-SCRUB): PASSED, zero absolute-local-path
matches in any evidence file (2026-09-06).
Holdout Evaluation
N/A — evaluated at wave-88 gate (not per-story for this pipeline mode). Note: this
story's own test suite additionally includes 4 unit-level RFC-905 (ISO 8073) holdout
tests (
test_iso8073_rfc905_*) that independently probe the spec against ITU-TX.224/ISO 8073 table 8 and clause 13 — these are a different mechanism from the
Phase-4 pipeline holdout-evaluation gate and are already counted in the 22/22 above.
Adversarial Review
Per-story adversarial review converged 3/3 clean passes (BC-5.39.001 discipline).
Pre-dispositioned finding (non-blocking, flagged for pr-reviewer awareness): a
per-story adversarial NIT observed that the regression-guard test
(
test_BC_2_20_012_static_regression_guard_no_hardcoded_protocol_literals) commentoverstates that "the file never contains
0x32/0x72as contiguous text" — true onlyof the literal tokens the assertion actually checks, not of every possible substring
occurrence. The load-bearing property (no S7comm-specific literal comparison inside
parse_cotp_header's control flow, BC-2.20.012 postcondition 3) holds regardless.Disposition: accepted residual, not a merge blocker.
A COTP-classification-focused review of the implementation found it panic-safe (bounds
checks precede every index into
tpkt_payload), RFC-905/ISO-8073-conformant (TPDU-codehigh-nibble discrimination matches ITU-T X.224 table 8), and protocol-agnostic (zero
0x32/0x72/"S7comm" literals anywhere in the parsing logic — verified by the staticregression-guard test above and by direct inspection).
Security Review
Populated after Step 4 (security-reviewer dispatch) — see PR comment / commit history
for the completed scan. Summary: pure, allocation-free
&[u8]parsing with no I/O, nounsafe, no external input trust boundary beyond standard slice-bounds checks; noinjection/auth/OWASP-Top-10 surface applies to a byte-classification free function.
Risk Assessment & Deployment
Blast Radius
src/analyzer/iso_on_tcp.rsonly (SS-20, ISO-on-TCP framing).No existing analyzer wiring changed —
parse_cotp_headeris not yet called from anyStreamAnalyzerimpl (that wiring is STORY-186).perspective until STORY-186 wires it up.
Feature Flags
None — additive pure-core module, not yet reachable from any runtime path.
Traceability
test_BC_2_20_005_len_shorter_than_2_returns_nonetest_BC_2_20_006_li_truncation_returns_nonetest_BC_2_20_007_connect_request_recognizedtest_BC_2_20_008_connect_confirm_recognizedtest_BC_2_20_009_dt_nonempty_payload_extracts_protocol_idtest_BC_2_20_010_dt_empty_payload_protocol_id_nonetest_BC_2_20_011_unrecognized_tpdu_type_returns_none,test_BC_2_20_011_tpdu_type_match_is_exhaustivetest_BC_2_20_012_protocol_id_extraction_totality(256-value exhaustive loop), static regression guardverify_parse_cotp_header_safety(Kani)VP-049 deferral note: per the story's own scope (and mirroring STORY-184's VP-048
precedent), only the Kani harness skeleton is delivered here — it compiles under
#[cfg(kani)]and targets bounds-safety over symbolic input. The full proof run(TPDU-type-classification exhaustiveness over all 16 nibble values, and protocol-ID
extraction totality over all 256
u8values) is STORY-194's obligation (formalhardening phase), not this PR's.
Full VSDD Contract Chain
AI Pipeline Metadata
Pipeline Details
Pre-Merge Checklist
[Unreleased]entry present (touchessrc/)docs/demo-evidence/STORY-185/, 10 AC files + evidence-report.md)develophttps://claude.ai/code/session_01EQAaPvh9fwaG31jmkPicKW