50 個聚焦面向的多代理審查；每輪含錯誤/改善與驗證方式。錯誤項經統合對抗式判定後列入 backlog。
| 輪數 | findings 總數 | error 標記數 |
|---|---|---|
| 50 | 186 | 95 |
Audited all findings against the live codebase (full read of HsmsConnectionStateMachine, HsmsConnection, SecsSession, Secs2Codec, SecsItem, GemEquipmentHandler, parser, tests) and ran the suite: build green, 23/23 tests pass (9 codec + 14 integration). Adversarial verdict: the 50-round "error" set is dominated by false positives and low-impact polish. The single most defensible defect is the missing Reject.req (SType=7) handler, which creates a Reject↔Reject loop with a non-compliant peer. The codec length-overflow family (>0xFFFFFF silently truncates in WriteHeader) is real but only reachable by local app code building a >16MB in-memory item — the 1MB receive cap (HsmsMessageParser.MaxMessageBytes) means no peer can ever round-trip such a frame, so it is defense-in-depth, not an over-the-wire bug. Pending-transaction cleanup on disconnect is a genuine robustness gap but bounded by T3 (callers fail within T3Seconds, not "indefinitely" as claimed). Many high-severity HSMS findings are simply wrong about SEMI E37/E5: Separate.req has no response in HSMS (rounds 5/6 "missing Separate.rsp SType=10" — there is no such message); S9 messages are legitimately odd-function unacknowledged primaries carrying a 10-byte MHEAD as bare B[10] (rounds 19/20/21 "S9 must be even / must be L[B] / must reuse SystemBytes" all contradict the spec and the passing tests); the round-4 "Linktest not bidirectional / shared timer" claim misreads the architecture (Active and Passive are separate HsmsConnection instances, each with its own LinktestTimer, both arming Linktest on entering SELECTED). The I1 "unchecked truncation" (round 13) is impossible because the parameter is already sbyte[]. SystemBytes overflow (rounds 16/22) is a real-but-negligible 2^32-message wraparound. Recommendation: fix the Reject.req handler; optionally add an encode-side length guard and a disconnect-time pending-abort for hardening; ignore the rest.
| # | 項目 | 嚴重度 | 判定為真 | 理由 | 相關輪次 |
|---|---|---|---|---|---|
| 1 | Missing Reject.req (SType=7) handler causes Reject↔Reject loop with non-compliant peer | medium | ✓ | VERIFIED in HsmsConnectionStateMachine.cs:81-95: the SType switch has cases 0-6 and 9 but no case 7; a received Reject.req falls to default which sends BuildRejectReq back, so two peers that both reject can ping-pong. GetSTypeName does map 7→Reject.req (HsmsMessage.cs:51) so the name exists but no behavior. Lowered from the reviewers' 'high' to medium: only triggers against a misbehaving/garbled peer, single connection impact, no data corruption, and the loop is naturally throttled by network round-trips. Cheap, correct fix: add `case 7:` that logs reason (HeaderByte3) and does nothing (or tears down) without replying. SType=8 Reject.rsp (round 6b) is the same family and even lower priority (rarely used in HSMS-SS). | 6 |
| 2 | Pending SECS transactions not aborted on TCP disconnect (callers wait up to T3 instead of failing fast) | low | ✓ | VERIFIED in SecsSession.cs: StateChanged is forwarded (line 41) but no internal handler aborts _pending TCS on transition to NotConnected; on a mid-transaction drop, SendAndWaitAsync blocks until the Task.Delay(T3) branch wins (lines 71-73). The reviewers' 'wait indefinitely' is FALSE — T3Seconds caps it. Real improvement (fail within ~100ms vs T3), but bounded, no corruption, and replies for a dropped connection can't arrive anyway. Fix: subscribe to StateChanged, on NotConnected TrySetException on all _pending and clear. Round 8's secondary claim (T3 should emit an HSMS Reject.req reason=3 to the remote) is optional and arguably out of scope for a simulator. | 8 |
| 3 | Encode-side length not validated; values >0xFFFFFF silently truncate in WriteHeader | low | ✓ | VERIFIED in Secs2Codec.cs:36-44: numLenBytes maxes at 3 and only the low 24 bits are written, so a length >0xFFFFFF truncates with no guard; decode is protected (parser MaxMessageBytes=1MB, HsmsMessageParser.cs:30-31) but encode is not — a real asymmetry. Practical severity is LOW: to trigger it, local app code must build ONE SecsItem holding >16,777,215 bytes/elements in memory; and because every peer rejects frames >1MB on receive, such a frame can never round-trip over the wire. Rounds 9/10/11/14 are the SAME issue (dedup to one). Cheap defensive fix: `if (length<0||length>0xFFFFFF) throw` at top of WriteHeader (or cap at 1MB to match the parser). Not exploitable remotely. | 9,10,11,14 |
| 4 | List decode pre-allocates new List(length) before per-child bounds checks (minor allocation amplification) | low | ✓ | PARTIALLY real, mostly neutralized. Secs2Codec.cs:82 does `new List<SecsItem>(length)` using an attacker-influenced count. Round 15's OOM-via-0xFFFFFF scenario is largely defused by the upstream 1MB frame cap: each child consumes ≥1 body byte, so a real frame bounds child count to ~1M, and the loop's per-child bounds check (lines 87-88, 61-62) stops over-reads. Worst case is a transient ~16M-capacity list allocation if length=0xFFFFFF is crafted, then immediate InvalidDataException — a small, recoverable allocation spike, not a crash. Round 15's companion claim 'validate numLenBytes>3' is FALSE: numLenBytes = formatByte & 0x03 is mathematically ≤3, and 3 is a legal SEMI E5 value. Optional hardening: cap list count or clamp initial capacity. | 15 |
| 5 | SystemBytes counter wraps after 2^32 messages (theoretical request/reply collision) | low | ✓ | Real but negligible. SecsSession._sysBytes is int incremented via Interlocked and cast to uint (lines 21,50) — this yields a correct monotonic uint sequence that wraps at 2^32; HsmsConnectionStateMachine._nextSystemBytes (uint, line 14) wraps identically. A collision needs ~4.3 billion in-flight-overlapping transactions on one long-lived session AND an old pending entry still unmatched at wrap — astronomically unlikely for this simulator. Rounds 16/16b/22/22b are the SAME issue (dedup). Round 22's int→uint change is cosmetic (collision behavior is identical). The control-plane 'no _pending tracking for Linktest/Deselect' sub-claim (round 16b) is minor: responses only reset/cancel a timer, and stray control responses are filtered by current state. | 16,22 |
| 6 | JIS-8 (SecsFormat.J) is an incomplete/unused feature — no factory, ScalarText decodes as ASCII | low | ✓ | VERIFIED: SecsFormat.J=0x11 is defined (SecsFormat.cs:10), ElementSize treats it as 1 byte, ScalarText uses Encoding.ASCII (SecsItem.cs:167), there is no SecsItem.J() factory and SmlJson has no 'J' key (SmlJson.cs). So J can be decoded off the wire but not constructed/round-tripped, and bytes 0x80-0xFF would garble in SML display. This is dead/partial functionality, not a bug in any exercised path — the simulator never advertises JIS-8 and no test uses it. Rounds 9b/12a/12b/12c are the SAME feature gap (dedup). Resolve by either removing J from the enum or adding a proper factory+accessor; low priority either way. | 9,12 |
| 7 | GEM state-machine polish: S1F15 doesn't clear CommunicationState; S1F13 has no ControlState precondition | low | ✓ | Debatable, not a clear defect. GemEquipmentHandler S1F15 (lines 97-101) sets ControlState=Offline without touching CommunicationState, and S1F13 (73-79) accepts Establish Communications unconditionally. In SEMI E30 the control-state and communication-state dimensions are largely orthogonal; many compliant implementations do NOT force NotCommunicating on going offline, and gating S1F13 on online is implementation-specific. The integration test Full_S1_flow asserts the CURRENT behavior and passes. At most a strictness/conformance-profile choice; no functional break. | 23 |
| 8 | SupportedStreams lists 9 but no inbound S9 dispatch (cosmetic logic inconsistency) | low | ✓ | Real-but-inert. GemEquipmentHandler.cs:17 includes 9 in SupportedStreams, but HandleAsync has no S9 case, so an inbound S9 to the equipment yields S9F5 (Unrecognized Function) rather than S9F3. Equipment legitimately never needs to process inbound S9, so this is a self-consistency wart with no functional consequence. Trivial cleanup: drop 9 from SupportedStreams so unexpected S9 maps to S9F3. Not worth a dedicated change unless touching that file anyway. | 17 |
| 9 | FALSE POSITIVE: 'Separate.req must be answered with Separate.rsp (SType=10)' | low | — | INCORRECT per SEMI E37. The Separate procedure is a one-way connection-abort: Separate.req has NO response, and there is no SType=10 'Separate.rsp' in HSMS (SType 7=Reject, 9=Separate; 8 is Reject.rsp, 10 is undefined). Current HandleSeparateReq (HsmsConnectionStateMachine.cs:180-184) correctly cancels the Linktest timer and disconnects. Round 5's companion 'thundering-herd on Passive re-listen' is also weak: the loopback tests reconnect reliably and PassiveConnectionStrategy Stop/Start within one event-loop turn; no EADDRINUSE observed in 14 passing integration tests. Implementing the suggested Separate.rsp would itself violate the spec. | 5 |
| 10 | FALSE POSITIVE cluster: S9 must be even-function / body must be L[B] / must reuse request SystemBytes | low | — | All three contradict SEMI E5/E30 and the passing tests. (a) S9 stream messages are defined as ODD-function unacknowledged PRIMARIES (S9F1/F3/F5/F7…), so 'odd violates E5' is wrong (round 19). (b) S9 body is a bare 10-byte MHEAD = B[10], NOT L[B[10]]; GemEquipmentHandler sends SecsItem.B(m.Header) and the tests assert Stream=9/Function=3 or 5 and pass (round 20). (c) As a primary, S9 carries its OWN SystemBytes and conveys the offending header in the body, not via reply-pairing, so 'must reuse SystemBytes' is wrong (round 19b). (d) Round 21's claim that S9F3/F5 are 'sent without body' is factually wrong — UnknownAsync builds shead=B(m.Header) and passes it for both (lines 225-229). Header is verified 10 bytes (HeaderSize=10; SecsSession extracts RawFrame[4..14]). | 19,20,21 |
| 11 | FALSE POSITIVE: 'Linktest not bidirectional / Active and Passive share one timer' | low | — | Misreads the architecture. Active and Passive are SEPARATE HsmsConnection instances (typically separate processes), each owning its OWN LinktestTimer (HsmsConnection.cs:22,58). Both sides call _startTimer(Linktest) on entering SELECTED (HsmsConnectionStateMachine.cs:117 passive, :131 active), and each independently fires HandleTimeoutAsync→sends its own Linktest.req (line 199-201). So Linktest IS bidirectional; there is no shared instance. The round-4b 'T6 log says Linktest when it was a Deselect' is a cosmetic log-string imprecision at worst (HandleTimeoutAsync logs a generic reason in SELECTED), not a state-corruption bug. | 4 |
| 12 | FALSE POSITIVE: I1/U1 factories silently truncate out-of-range values | low | — | Impossible as described. SecsItem.I1(params sbyte[] values) (SecsItem.cs:58-63) already constrains inputs to -128..127 via the sbyte parameter type; the (byte)sbyte cast is total and lossless. The reviewer's own repro `SecsItem.I1(unchecked((sbyte)256))` passes a VALID sbyte (256 unchecked → 0) — there is no 256 reaching the method. U1 takes byte[] (0..255) similarly. The round-trip test RoundTrip_all_scalar_formats exercises I1(-128,127) and passes. A checked cast would never throw. No defect. | 13 |
| 13 | FALSE/SPECULATIVE cluster: Select.rsp non-Connected handling, busy-retry, status 2-4, T7 race, T6 disambiguation, Passive single-session, secondary W-bit, device-ID validation, timer disposal races | low | — | Grab-bag of low-confidence hardening/conformance suggestions, none of which manifest as a failure in the green suite. Notable rebuttals: (rounds 3b/7b) Select.rsp(1) Busy DOES retry — HandleSelectRsp case 1 increments _selectBusyRetryCount and only force-disconnects at SelectBusyMaxRetry=3 (lines 133-139), contradicting 'disconnects prematurely'. (round 7) LinktestTimer.Stop double-dispose is safe — CTS.Dispose is idempotent and all calls are serialized through the single-reader event loop, so the concurrent-Stop race can't occur. (round 2b) Passive auto-relisten after drop is an intentional simulator convenience (PostEvent Connect on NotConnected, HsmsConnection.cs:222), documented and test-covered; calling it a 'single-session violation' is a conformance-profile opinion. (rounds 1/3) ignoring unexpected Select.rsp/Deselect.rsp in wrong states is benign defensive behavior. (round 18/21b) secondary-with-W-bit and device-ID mismatch validation are nice-to-haves with no current failure. All are speculative robustness polish, not defects. | 1,2,3,7,18,21 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Incomplete Select.rsp handling on non-Connected states | The method only processes Select.rsp when state is Connected (line 122: `if (_state != HsmsState.Connected) return;`). However, if an Active side accidentally receives a Select.rsp while already in Selected state (e.g., due to timing race or protocol violation), the response is silently ignored instead of rejecting or logging it. Per SEMI E37, unexpected Select.rsp messages in non-Connected states should trigger protocol error handling. Similarly for Deselect.rsp (line 161). HsmsConnectionStateMachine.cs:120-147 (HandleSelectRsp method) | Add explicit handling for Select.rsp/Deselect.rsp received in unexpected states: either log a warning and send Reject.req, or transition to NotConnected with error description. This hardens against malformed peer behavior. | Test: Send Select.rsp manually while in Selected state; verify state machine logs error and does not corrupt internal state. Can verify by intercepting network traffic with tcpdump or by adding unit test with mock message injection. |
| 錯誤 | medium | T7 cancellation missing in Connected→Selected path | When Passive side receives Select.req and transitions from Connected to Selected (line 116), T7 timer is cancelled (line 113). However, when Active side receives Select.rsp(0) and transitions to Selected (line 130), T7 is cancelled (line 128) but there's a moment where the state is still Connected—any T7 timeout that fires between receiving Select.rsp and transitioning to Selected will incorrectly force disconnect. Per SEMI E37, T7 should be cancelled immediately upon receiving Select.rsp(0), before state transition. HsmsConnectionStateMachine.cs:113-117 (HandleSelectReqAsync method, Passive path) | Restructure HandleSelectRsp to cancel T7 (and T6) at the start of the success path (case 0) before any other operations, ensuring no window where T7 can fire after Select.rsp(0) is received. | Add integration test: Active side receives Select.rsp(0); inject T7 timeout shortly after (by reducing T7 value to ~1ms in test scenario) and verify state machine transitions to Selected and does NOT disconnect. Monitor state transition log. |
| 改善 | low | Silent ignore of Reject.req not explicitly addressed in spec compliance | The code builds a switch on SType and logs unknown SType values (line 92), but there is no explicit case handler for SType=7 (Reject.req per SEMI E37). While ignoring Reject.req may be acceptable application behavior (the spec allows either side to reject), there's no comment explaining why it's silently dropped rather than logged with a warning. HsmsConnectionStateMachine.cs:79-95 (HandleMessageAsync method) | Add an explicit case for SType=7 Reject.req (or a comment block explaining that Reject.req is intentionally not handled at state machine level). If handling is desired, log it and evaluate whether to transition to NotConnected based on the reason code (reason=4 = Entity not selected, etc.). | Grep for any handling of Reject responses elsewhere in the codebase (Session layer, etc.). If none exists, add a unit test verifying that Reject.req does not crash the state machine and document the design decision. |
| 改善 | low | Linktest timer not explicitly restarted after Linktest.req sent via user command | When user calls SendLinktest, a Linktest.req is sent and T6 timer is started (line 218). However, the Linktest interval timer is not affected. Per SEMI E37, the periodic Linktest interval should typically be reset when a Linktest.req is sent manually. Currently, if user sends a manual Linktest.req shortly before the periodic Linktest timer would have fired, the periodic one will fire immediately after, creating a burst of Linktest traffic. HsmsConnectionStateMachine.cs:216-218 (SendLinktest user command) | Consider whether periodic Linktest timer should be cancelled and restarted when user sends a manual Linktest.req. If so, add `_cancelTimer(TimeoutKind.Linktest); _startTimer(TimeoutKind.Linktest);` to the SendLinktest handler. | Add integration test: manual SendLinktest called at time T; measure time to next automatic Linktest message. Verify behavior matches intended design (either resets timer or accumulates). Document the choice in comments. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Passive role can initiate Deselect, violating SEMI E37 protocol | HsmsConnectionStateMachine.cs lines 220-223 allows any role to call SendDeselect(), which generates a Deselect.req. Per SEMI E37, only the Active entity (Host) may initiate Deselect. The Passive entity (Equipment) must wait for the Active entity's Deselect.req and respond with Deselect.rsp. A Passive/Equipment simulator calling SendDeselect() violates the protocol state machine. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:220-223 and HsmsConnection.cs:95 | Add a role check in HandleUserCommandAsync() before allowing SendDeselect: only permit UserCommandKind.SendDeselect when _options.Mode == ConnectionMode.Active. Alternatively, move SendDeselect() to a separate ActiveConnectionSession class. | Write a unit test: create Passive HsmsConnection, call SendDeselect(), verify that it either throws InvalidOperationException or logs a warning that the command was rejected. Ensure this test fails on current code and passes after fix. |
| 錯誤 | high | Passive listener is recreated after TCP drop, violating HSMS-SS single-session model | PassiveConnectionStrategy.cs explicitly documents 'HSMS-SS 單 Session 模型' (single session model), but HsmsConnection.cs line 222 invokes PostEvent(UserCommandKind.Connect) after Passive state exits NotConnected. This triggers EstablishConnectionAsync() → _strategy.EstablishAsync(), which calls PassiveConnectionStrategy.EstablishAsync() again, creating a new TcpListener. Per SEMI E37 HSMS-SS, once a Passive entity accepts one TCP connection and that connection closes, the listener must terminate and NOT accept new connections (one connection per passive session lifecycle). Current code violates this by recreating the listener. src/SecsGem.Core/Hsms/HsmsConnection.cs:219-222 and PassiveConnectionStrategy.cs:7 | Modify HsmsConnection.OnStateMachineStateChanged(): for Passive mode, when state transitions to NotConnected, do NOT automatically restart with Connect. Instead, only Passive applications should call Disconnect() explicitly; after that, Passive remains offline. Alternatively, track a flag 'hasAcceptedOnce' and prevent EstablishConnectionAsync() from retrying if Passive has already accepted one connection. | Loopback integration test: (1) Passive connects and accepts, (2) Host connects, both reach Selected, (3) Host sends Disconnect(), (4) after TCP closes, verify Passive.State is NotConnected and does NOT automatically restart listening. Use a timeout (e.g., 3 seconds) to confirm no listener is re-established. This test should fail on current code. |
| 錯誤 | medium | T6 timer cancel logic does not distinguish between message types (Deselect vs Linktest) | HsmsConnectionStateMachine.cs line 162 (HandleDeselectRsp) unconditionally cancels T6 regardless of whether T6 was started for this Deselect.req or for an unrelated Linktest.req. In rare sequences like: SendDeselect() [starts T6], then Linktest.req arrives before Deselect.rsp, the state machine will start another transaction. When Deselect.rsp finally arrives, it cancels T6 without knowing which pending request T6 actually guards. This can leave a Linktest.req hanging indefinitely. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:159-166 | Introduce a flag (e.g., '_pendingDeselectSystemBytes') to track which request T6 guards. Only cancel T6 in HandleDeselectRsp() if the systemBytes match. Alternatively, use a state machine refinement: add an intermediate state like 'DeselectPending' so the state machine rejects outbound control messages (Linktest.req) until Deselect completes. | Unit test the state machine directly: ProcessAsync(UserCommandEvent(SendDeselect)), then ProcessAsync(TimeoutFiredEvent(Linktest)) which posts Linktest.req, then ProcessAsync(MessageReceivedEvent(DeselectRsp)). Verify that after DeselectRsp, if a T6 timeout fires for the Linktest.req, it is still handled correctly (not already cancelled). This test should expose the bug on current code. |
| 改善 | medium | SendDeselect() public API lacks role enforcement documentation and validation | HsmsConnection.SendDeselect() (line 95) is public and has no inline documentation stating it is Active-only. A developer writing Equipment (Passive) code could accidentally call this method, violating the protocol. While the state machine may reject it in practice (lines 220-223 require _state == HsmsState.Selected), there is no explicit error or warning. src/SecsGem.Core/Hsms/HsmsConnection.cs:95 | Add XML documentation: '/// <remarks>Active (Host) role only. Passive (Equipment) role must not call this.</remarks>'. Alternatively, add an explicit check: 'if (_options.Mode != ConnectionMode.Active) throw new InvalidOperationException("SendDeselect is Active-only");' to fail fast with a clear error message. | Code review: check that SendDeselect() now has a clear role restriction comment or throws an exception if called from Passive mode. Write a unit test that verifies Passive cannot call SendDeselect (either exception or silent rejection with a log warning is acceptable, but must be explicit). |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Missing Select.rsp(2) for Already Selected state | HandleSelectReqAsync (HsmsConnectionStateMachine.cs:110-118) accepts Select.req only when state==Connected, but never validates if we're already Selected. Per SEMI E37, receiving Select.req while Selected should reply with Select.rsp status=2 (Already in use), not ignore it. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:110-118 | Add a check: if _state==Selected, send Select.rsp with status=2 and remain in Selected state. This prevents duplicate select attempts and clarifies device state. | Unit test: Send Select.req twice in succession from passive side. Verify second Select.req receives status=2 response, not ignored. Also verify state remains Selected. |
| 錯誤 | high | Select.rsp(1) Busy handling disconnects prematurely | HandleSelectRsp (lines 133-139) calls ForceDisconnect() when status=1 (Busy), but SEMI E37 specifies Busy should only cause disconnect after SelectBusyMaxRetry retries. Currently, ANY Busy response forces disconnect then waits T5. This violates the spec's intent that Busy is transient and recoverable. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:133-139 | Change logic: on status=1, do NOT ForceDisconnect immediately. Instead, cancel T6, increment retry count, and if <limit, start T5 timer and re-send Select.req. Only ForceDisconnect after max retries exceeded. | Integration test: Mock passive side to return Select.rsp(1) twice, then Select.rsp(0). Verify active side retries T5 twice, then reaches Selected. Verify retry counter is properly reset. |
| 錯誤 | medium | Select.rsp status 2-4 not distinguished per SEMI E37 | Lines 141-146 handle Select.rsp with status 2/3/4 identically (all fall to default case, log error, disconnect). However SEMI E37 defines distinct meanings: status=2 (Already in use - remote already selected elsewhere), status=3 (Not ready), status=4 (Connect exhausted). Each may warrant different handling or logging. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:141-146 | Add explicit case for status=2,3,4 with specific log messages identifying the rejection reason. Consider whether any should retry (per SEMI E37) versus immediate disconnect. | Unit tests: Send Select.rsp with status=2,3,4 separately. Verify each produces correct log message identifying the rejection type, and state transitions to NotConnected. |
| 改善 | medium | Deselect.rsp handler missing state validation | HandleDeselectRsp (lines 159-166) assumes state==Selected but does not guard against it. If Deselect.rsp arrives while in Connected state (e.g., duplicate or out-of-order), it silently returns without logging. This can mask protocol violations or race conditions. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:159-166 | Add early return with log like HandleSelectReqAsync does (line 112). Example: if (_state != HsmsState.Selected) { _log("[WARN] Deselect.rsp received in non-Selected state"); return; }. Prevents silent failures and aids debugging. | Test scenario: Inject Deselect.rsp while in Connected state. Verify warning is logged and state does not change. Confirm T7 timer is not incorrectly started. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Linktest not bidirectional: only initiator sends periodically, responder waits passively | Current implementation: Linktest timer fires only when a single party (typically Active mode) has started it. The Linktest timer is created per HsmsConnection instance (HsmsConnection.cs:58), and is started when EITHER party enters SELECTED (HsmsConnectionStateMachine.cs:117, 131). However, the timer is the SAME instance for both sides — so both sides start the SAME timer at the same moment. But logically, per SEMI E37, both sides should INDEPENDENTLY maintain Linktest heartbeats so that either side can detect the other's failure. Currently, if the Active side initiates and the Passive side ONLY responds, then if the Passive side wants to detect Active's failure, it gets no opportunity because it never sends Linktest.req. The specification (SEMI E37 Section 10.2) states Linktest is a bidirectional heartbeat — 'either entity may initiate'. The code only starts Linktest upon entering SELECTED, not continuously from both sides. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:199-201 (HandleTimeoutAsync for Linktest), HsmsConnection.cs:58 (LinktestTimer shared per instance) | Implement independent Linktest initiation for BOTH sides after entering SELECTED. Options: (1) Introduce a separate 'LinkTestInitiator' role per connection (e.g., both Active and Passive always initiate independently on their own timer), OR (2) Alternate initiation (ping-pong style where after responding, the responder resets its own Linktest timer to initiate next). SEMI E37 Annex A illustrates both entities sending Linktest.req within their own Linktest intervals. Currently only one side reliably sends (the one that started first or had the timer enabled). Test both directions: (a) Active initiates, Passive must also initiate independent Linktests; (b) Passive must detect Active's failure if Active stops responding, even if Passive initiated last Linktest. | (1) Create a test where both Active and Passive have LinktestIntervalSeconds=1s; measure that BOTH sides send Linktest.req frames independently within 1s of entering SELECTED. (2) Inject a failure where one side stops responding to Linktest.req; verify the OTHER side detects it via T6 timeout within ~5s (or T6 value). Currently likely to fail because responder-only doesn't send probes. (3) Verify both sides independently reset Linktest timer on receiving/sending Linktest frames (not just on response). |
| 錯誤 | high | T6 timeout on Linktest response applies to both Select and Linktest contexts — ambiguous in code comment | In HsmsConnectionStateMachine.cs lines 196-197, when T6 fires in SELECTED state, it logs 'T6 逾時（Linktest 無回應）'. But T6 is defined in SEMI E37 as 'Control transaction timeout' which applies to SELECT, DESELECT, AND LINKTEST. The code conflates T6's use. Line 194-195 handles T6 timeout during CONNECTED state (waiting for Select.rsp), but lines 196-197 assume if we're SELECTED and T6 fires, it MUST be from a Linktest.req we sent. This is correct behavior, but the state machine doesn't distinguish between: (a) T6 started from Select.req (state=CONNECTED), (b) T6 started from Linktest.req (state=SELECTED), (c) T6 started from Deselect.req (state=SELECTED). Lines 196-197 only check state==SELECTED, which includes both (b) and (c). If Deselect.req was sent and T6 times out, it will incorrectly log 'Linktest 無回應' instead of 'Deselect 無回應'. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:186-204 (HandleTimeoutAsync), lines 196-198 specifically | Track which control message triggered T6 in current flight (add a field like '_pendingControlMessageType' set when starting T6, cleared on response or timeout). When T6 fires in SELECTED state, check this field to log accurate reason: 'T6 逾時（Linktest 無回應）' vs 'T6 逾時（Deselect 無回應）'. Alternatively, use a discriminated union or enum for T6 context. This clarifies logs and makes future debugging easier. | Send a Deselect.req while in SELECTED (line 220-222), let T6 timeout, and check the log message. Currently it will say 'Linktest 無回應' (wrong). After fix, verify it says 'Deselect 無回應'. |
| 改善 | medium | Linktest interval and T6 relationship not explicitly enforced: should validate T6 >= Linktest interval | SEMI E37 Annex A states that Linktest is sent at regular intervals, and T6 is the timeout waiting for response. Best practice: T6 should be ≥ (Linktest interval - some margin) to avoid false timeouts. Currently, HsmsOptions.cs allows arbitrary values: LinktestIntervalSeconds=5 (default), T6Seconds=5 (default). If misconfigured (e.g., Linktest=10s, T6=2s), a responder that takes >2s to reply will trigger false T6 timeout and drop the connection. The code has no validation in HsmsOptions constructor or setter. src/SecsGem.Core/Hsms/HsmsOptions.cs:1-27 (class definition with no validation) | Add a validation method (e.g., 'Validate()' called in constructor) that warns or errors if T6Seconds is significantly less than LinktestIntervalSeconds (e.g., require T6 >= Linktest * 1.5). Document the constraint in comments. Example: T6=5s should allow Linktest to fire at ~3-4s interval with 1s network latency margin. | Create an HsmsOptions with LinktestIntervalSeconds=10, T6Seconds=2, and call a new Validate() method. Verify it raises a warning or error. Then set T6Seconds=15 and verify validation passes. Run integration test with these mis-tuned values and observe T6 timeouts (before fix) or successful Linktest exchange (after fix with corrected tuning). |
| 改善 | medium | LinktestTimer.RunAsync does not propagate exceptions from _onFire callback — swallows errors silently | In LinktestTimer.cs lines 32-40, the RunAsync method catches OperationCanceledException but allows any other exception thrown by _onFire() to crash the async task without logging. If _onFire (the lambda at HsmsConnection.cs:58 that posts TimeoutFiredEvent) throws, the Linktest timer will silently stop firing and no further Linktests will be sent. The connection will appear stuck or will eventually T6 timeout on the next Linktest initiator's request. There is no try-catch or logging to signal this failure. src/SecsGem.Core/Hsms/LinktestTimer.cs:32-40 (RunAsync method), line 37 specifically (_onFire() call) | Wrap _onFire() in a try-catch in RunAsync: catch all exceptions, log them, and continue the loop. Example: try { _onFire(); } catch (Exception ex) { /* log warning, but don't stop timer */ }. Alternatively, require the caller to provide error handling or expose a separate error event. This ensures Linktest keeps firing even if event posting has transient issues. | Unit test: Create a LinktestTimer with an _onFire that throws an exception on the 2nd call. Verify (a) before fix: no error logged, timer stops silently; (b) after fix: error is logged, timer continues to fire (3rd call succeeds). Measure via a counter or spy on _onFire invocations. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Separate.req 收訊未回應，違反 SEMI E37 | HsmsConnectionStateMachine.HandleSeparateReq()（行 180-184）收到對方 Separate.req 時只取消 Linktest timer 並強制斷線，但未發送 Separate.rsp。SEMI E37 控制平面協定規定 Separate.req 應收到 Separate.rsp（SType=10）回應，或至少應以其他控制訊息確認。當前實作使連線無法正常關閉握手，可能導致網路緩衝滯留、遠端超時等。 src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:180-184 | 添加 Separate.rsp（SType=10）至 HsmsMessageBuilder，並在 HandleSeparateReq() 發送回應後再斷線。建議：(1) 新增 BuildSeparateRsp() 方法；(2) 在 HandleSeparateReq() 內 await _sendBytes(HsmsMessageBuilder.BuildSeparateRsp(message.SystemBytes)) 再調用 ForceDisconnect。 | 撰寫整合測試：建立 Host(Active) 與 Equipment(Passive) 真實 TCP 連線至 SELECTED，Host 呼叫 SendSeparate() 後檢查雙邊接收到正確的 Separate.req/rsp 序列，確認訊息日誌記錄中 Separate.rsp 出現。 |
| 錯誤 | high | Passive 斷線後未等待新連線，Active 無法感知 Passive 已重新監聽 | HsmsConnection.OnStateMachineStateChanged()（行 210-223）中，Passive 模式（ConnectionMode.Passive）轉入 NotConnected 後立即 PostEvent(UserCommandKind.Connect) 重新監聽。但 PassiveConnectionStrategy.EstablishAsync() 每次呼叫都會 Stop() 舊 listener 再 Start() 新 listener，中間沒有延遲。若 Active 端試圖在監聽啟動前重連，會連線失敗；若 Passive 被動斷線後，Active 側收到 TcpDisconnected 並經 T5 timeout 才重連，兩邊時序難同步，導致頻繁「連線失敗→T5 等待→重試」的 thundering herd 問題。 src/SecsGem.Core/Hsms/HsmsConnection.cs:214-222; PassiveConnectionStrategy.cs:16-36 | Passive 策略在重新監聽前加入短暫延遲（如 50-100ms），確保 listener socket 完全釋放；或在 PassiveConnectionStrategy 中捕捉 Address already in use 異常並重試。同時在 HsmsConnection 層針對 Passive 模式提供可設定的「reconnect grace delay」，讓雙方斷線後有足夠時間同步再連線。建議值：T5 的 10-20%（例如 T5=10s 時延遲 1-2s）。 | 撰寫壓力測試：Host(Active) 與 Equipment(Passive) 反覆執行「進入 SELECTED → Passive 呼叫 SendSeparate() 主動斷線 → 1s 內立即重連」100 次，統計成功連線次數與失敗日誌中 EADDRINUSE 或連線拒絕的頻率；期望 100% 成功率。 |
| 改善 | medium | 缺少 Separate.rsp（SType=10）訊息型別支援 | HsmsMessage.GetSTypeName()（行 42-54）與 HsmsMessageBuilder 只定義了 SType 0-7 及 9，但 SEMI E37 規定 SType=10 為 Separate.rsp。若未來收到 Separate.rsp，會被視為不支援型別（落入 default 分支），記錄為 'SType=10'，且狀態機無專用處理路徑，可能導致訊息被誤判或丟棄。 src/SecsGem.Core/Hsms/HsmsMessage.cs:42-54; HsmsMessageBuilder.cs（無 BuildSeparateRsp）; HsmsConnectionStateMachine.cs:81-95 | 新增 SType=10 的訊息定義與處理：(1) HsmsMessage.GetSTypeName() 加入 `10 => "Separate.rsp"`；(2) HsmsMessageBuilder 新增 `BuildSeparateRsp(uint systemBytes)` 方法；(3) HsmsConnectionStateMachine.HandleMessageAsync() 新增 case 10 處理邏輯（當收到 Separate.rsp 時，若已在進行斷線，則確認斷線完成；否則視為協定錯誤）。 | 單元測試：(1) 測試 GetSTypeName(10) 回傳 'Separate.rsp'；(2) 測試 BuildSeparateRsp(123) 生成正確 frame（SType=10、SystemBytes=123）；(3) 整合測試模擬對方送 Separate.rsp，確認狀態機正確識別而非拒絕。 |
| 改善 | medium | Active 端斷線發送 Separate.req 未確保送出（best effort 忽略例外） | HsmsConnectionStateMachine.HandleUserCommandAsync() 中 UserCommandKind.Disconnect 分支（行 210-215）執行 Separate.req 發送時使用 try-catch 吞掉所有例外（best effort）。若此時 TCP 已斷開或 _networkStream 為 null，訊息會無聲失敗。對端無法收到明確的 Separate.req，只能等 T6 timeout（5s）才能偵測到超時，導致斷線延遲。 src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:210-215 | 區分斷線原因：(1) 若是主動 Disconnect()（_isRunning=false），應嘗試優雅發送 Separate.req，若失敗應記錄警告日誌；(2) 若是被動收到對端斷線，直接跳過 Separate.req。在 Disconnect() 路徑中傳入額外參數（如 isGraceful=true）讓狀態機區別對待。同時在 SendBytesAsync 中若 _networkStream==null 時顯示警告而非無聲失敗。 | 整合測試：Host(Active) 進入 SELECTED 後立即 Disconnect()，監控日誌確認 Separate.req 被記錄為「已發送」（不論對端是否收到）；若 TCP 已關閉，日誌應顯示警告而非被 try-catch 吞掉。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Missing Reject.req (SType=7) handler in state machine | The HandleMessageAsync switch statement (HsmsConnectionStateMachine.cs:79-96) does not have a case for SType=7 (Reject.req). When a remote peer sends Reject.req, it falls through to the default case, which responds with another Reject.req (reason 1), creating a potential rejection loop. According to SEMI E37, received Reject.req should be logged and handled gracefully, possibly triggering connection teardown or state cleanup. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:79-96 | Add explicit case 7 handler: `case 7: HandleRejectReqAsync(message); break;` and implement HandleRejectReqAsync to log the rejection reason (from HeaderByte3), inspect the rejected SType (from HeaderByte2), and decide whether to retry, deselect, or disconnect based on the reason code. | Write integration test: Active peer sends invalid SType, Passive responds with Reject.req(reason=1). Verify Active peer receives Reject.req(7) without responding with another Reject. Check logs for proper reason code parsing. |
| 錯誤 | medium | Missing SType=8 (Reject.rsp) mapping and handler | HsmsMessage.GetSTypeName (HsmsMessage.cs:42-54) skips SType=8, and HandleMessageAsync (HsmsConnectionStateMachine.cs:79-96) has no case for it. According to SEMI E37, SType=8 is Reject.rsp. This control message is currently treated as unknown (falls to default case, responds with Reject.req reason 1), which violates the protocol—only Reject.req should trigger Reject responses. src/SecsGem.Core/Hsms/HsmsMessage.cs:51-52 and HsmsConnectionStateMachine.cs:81-95 | Add to GetSTypeName: `8 => "Reject.rsp",` and add handler case to match: `case 8: HandleRejectRspAsync(message); break;` Implement HandleRejectRspAsync to acknowledge receipt, log the reason, and update internal transaction tracking (if any pending Reject.req awaits response). | Add unit test to HsmsMessage: assert GetSTypeName(8) == "Reject.rsp". Write integration test: send Reject.rsp from passive side, verify active side logs it correctly without sending Reject.req in response. |
| 錯誤 | medium | PType validation missing—no rejection of non-SECS-II protocol types | HsmsMessageParser (HsmsMessageParser.cs:41-61) reads PType byte but does not validate it. HsmsConnectionStateMachine does not check message.PType. SEMI E37 defines reason code 2 ("PType not supported"), but the code never sends it. If a non-SECS-II PType arrives, it should be rejected with reason 2, not silently processed. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:79-96 | In HandleMessageAsync, add early validation: if (message.PType != 0) { log and send BuildRejectReq(message.SystemBytes, 0, reason: 2); return; } This ensures compliance with SEMI E37 and provides diagnostic clarity for protocol violations. | Write unit test: manually construct HsmsMessage with PType=1 (non-SECS-II), inject into state machine. Verify outbound Reject.req has reason=2 and HeaderByte2=0 (indicating PType rejection). Verify log mentions PType mismatch. |
| 改善 | low | Reason code 3 ('Transaction not open') never sent | HsmsMessageBuilder.cs:49 documents reason code 3: 'Transaction not open', but the code only sends reason codes 1 (unknown SType) and 4 (entity not selected). Reason 3 is applicable when a response message (e.g., Select.rsp, Deselect.rsp) arrives without a matching pending request. This scenario is not currently detectable because there is no pending-request tracking in the state machine. src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs | Add optional future enhancement: track pending control requests (Select, Deselect, Linktest) by SystemBytes, and if a response arrives with unmatched SystemBytes, send Reject.req with reason 3. This improves robustness against malformed or out-of-order peers. | Document that reason 3 is reserved for future transaction tracking. Add comment in HsmsConnectionStateMachine explaining when reason 3 would apply (e.g., orphaned response). Consider adding a TODO if pending-request correlation is planned. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | LinktestTimer CancellationTokenSource Double-Dispose Risk | LinktestTimer.Stop() (line 25-26) calls both _cts?.Cancel() and _cts?.Dispose() without guarding against concurrent Stop() calls or calls after Dispose(). If Stop() is invoked twice from different threads (e.g., CloseConnection + DisposeAsync race), the second Dispose() on already-disposed CTS will not throw but masks the actual issue. The CTS is also not null-checked after Cancel(). src/SecsGem.Core/Hsms/LinktestTimer.cs:23-30 | Guard CTS disposal: extract the cts once (var cts = _cts), set _cts = null immediately, then dispose outside the null-check. This prevents double-dispose in concurrent scenarios and matches CancellationTokenSource disposal best practices. | Write a concurrent test: call Start() -> Stop() from thread A while Dispose() runs on thread B. Should not throw ObjectDisposedException. Use dotMemory or GC.GetTotalMemory() before/after 100 iterations to detect leaks. |
| 錯誤 | high | HsmsConnection.StartTimer() Task Continuation Leak in Fire-and-Forget Pattern | StartTimer() (lines 284-291) uses Task.Delay(delay, token).ContinueWith(..., TaskScheduler.Default) as fire-and-forget (_ = ...). The ContinueWith task is never awaited or tracked, so if the continuation itself throws or if the event loop is disposed mid-continuation, the exception is silently swallowed. Additionally, the cts reference is captured in closure and never explicitly freed after the task completes. src/SecsGem.Core/Hsms/HsmsConnection.cs:281-291 | Store the ContinueWith task in _timers[kind] or a separate collection so it can be awaited on cancellation. Alternatively, replace ContinueWith with a more explicit async pattern: 'while (await delay with token)' in a dedicated background task. This ensures the delay task is properly tracked and disposed. | Run HsmsLoopbackTests with many rapid Connect/Disconnect cycles (e.g., 100 iterations). Monitor handle count and Task count via Process.GetCurrentProcess().Handles and TaskScheduler.Current stats. Verify no unobserved exceptions in event viewer. |
| 錯誤 | high | T5 Timer Not Cancelled in StartTimer() Idempotency Check | StartTimer() calls CancelTimer(kind) at line 272 to cancel the old timer before starting a new one. However, if StartTimer(T5) is called while a T5 timeout callback is in-flight (between Task.Delay completion and PostEvent execution), the old cts is disposed while the continuation is still running. This violates SEMI E37 which requires T5 to be a single, deterministic delay: multiple overlapping T5 intervals can occur if Disconnect -> T5Start -> Timeout -> T5Start races collide. src/SecsGem.Core/Hsms/HsmsConnection.cs:264-291 | For T5 specifically, guard against re-entrance by checking if a T5 is already pending before calling StartTimer(). In CloseConnection() (line 314), document WHY T5 is not cancelled (to allow re-connect retry), and in Disconnect flow, ensure T5 is explicitly cancelled. Alternatively, use a Semaphore to serialize T5 callback execution with new T5 starts. | Write a test: OnStateMachineStateChanged -> NotConnected (starts T5), immediately call StartTimer(T5) again before T5 fires. Capture all T5 fires via a counter. Expect exactly one Connect event, not two. Run 1000 times with varying T5 delays. |
| 改善 | medium | Missing Defensive Checks on Timer Operations After Dispose | HsmsConnectionStateMachine and state handlers call _startTimer() and _cancelTimer() delegates without knowing if HsmsConnection.DisposeAsync() has already run. If a delayed event (e.g., T6 timeout) arrives after Dispose(), StartTimer() will attempt to create a new CancellationTokenSource on a disposed object. The _eventChannel is also not guarded against post-Dispose writes. src/SecsGem.Core/Hsms/HsmsConnection.cs:264-307 (StartTimer/CancelTimer) and HsmsConnectionStateMachine.cs:17-42 (delegates) | Add a _disposed flag or use a try-catch in StartTimer/CancelTimer to handle ObjectDisposedException gracefully. Alternatively, have the state machine check a DisposeRequested flag before posting events. Log any post-Dispose operations as warnings. | Inject a 100ms delay in DisposeAsync before CloseConnection(). While that delay is running, post a TimeoutFiredEvent for T6 or T7 from a background thread. Should not crash with ObjectDisposedException; should log a warning. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Missing pending transaction cleanup on TCP disconnect | When HsmsConnection transitions to NotConnected (e.g., TCP socket closed or T6 timeout), SecsSession._pending dictionary retains TaskCompletionSource entries for in-flight transactions. These tasks are never aborted/notified, causing SendAndWaitAsync() callers to wait indefinitely (or until their own CancellationToken fires) rather than immediately failing per SEMI E37 Section 10.5.3. File: /src/SecsGem.Core/Session/SecsSession.cs (lines 20, 64, 79, 123) - the StateChanged event is forwarded but never consumed internally to abort pending. /src/SecsGem.Core/Session/SecsSession.cs:41 - StateChanged event registration does NOT include cleanup handler | Add a handler in SecsSession constructor that listens to StateChanged and aborts all pending transactions when State becomes NotConnected: `_conn.StateChanged += (state, desc) => { if (state == HsmsState.NotConnected) AbortAllPending(); };` Create an AbortAllPending() method that iterates _pending, calls TrySetException() on each TCS with an OperationCanceledException or custom TransactionAbortedException, then clears the dict. | Write a test: (1) create an active/passive pair, (2) start SendAndWaitAsync, (3) before reply arrives, force TCP disconnect via HsmsConnection.Disconnect() or kill socket, (4) assert SendAndWaitAsync raises OperationCanceledException/TransactionAborted (not timeout) within 100ms, not blocking until T3 fires. |
| 錯誤 | high | T3 timeout does not abort transaction on HSMS layer - semantic mismatch | SEMI E37 specifies T3 reply timeout should trigger transaction abort on both sides. Currently, T3 is purely a Session-layer timeout (SecsSession.SendAndWaitAsync, line 69-73) that throws TimeoutException to the caller. However, the HSMS layer (HsmsConnection) has no record of this transaction and does NOT send an abort signal (S-type=7 Reject with reason=3 'Transaction not open') to the remote side per SEMI E37:10.5.3. Remote side continues waiting for or holding the transaction state. File: /src/SecsGem.Core/Session/SecsSession.cs (line 73) throws but never notifies HSMS layer of transaction failure. /src/SecsGem.Core/Session/SecsSession.cs:59-81 (SendAndWaitAsync method) | Consider whether T3 timeout should trigger an HSMS-layer Reject.req (SType=7, reason=3) to signal transaction abort to remote. If yes: (a) refactor SendAndWaitAsync to notify HsmsConnection of T3 failure, or (b) have HsmsConnection track in-flight data-message SystemBytes and auto-send Reject on timeout. If T3 is pure Session-layer concern (no HSMS notification), document this clearly as deviation from SEMI E37, since standard expects bidirectional abort. | Protocol analyzer test: (1) send S1F1 from host to equipment, (2) measure time until host throws TimeoutException, (3) capture network trace on equipment side: verify if/when Reject.req arrives; (4) measure equipment-side _pending cleanup. If no Reject sent, this is the bug; if Reject is sent but equipment doesn't clear pending, that's a separate equipment-side issue. |
| 改善 | medium | No explicit transaction abort state machine in HSMS layer | While HsmsConnectionStateMachine correctly transitions to NotConnected on T6/T7 timeouts (lines 193-197), it does not model or track per-transaction abort semantics. Data messages (SType=0) are delivered directly without a transaction-context wrapper, so the state machine cannot enforce SEMI E37 Section 10.5.3 rule: 'When T3 timeout occurs, transaction shall be aborted on both sides.' The Reject message (SType=7, reason=3) exists in HsmsMessageBuilder.BuildRejectReq() but is only sent reactively (e.g., for unknown SType, entity not selected) not proactively on transaction timeout. /src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:79-108 (HandleDataAsync and HandleTimeoutAsync) | Introduce a lightweight pending-transaction registry in HsmsConnection or HsmsConnectionStateMachine keyed by (SessionBytes) with timeout = T3. On HandleTimeoutAsync or T3 expiry, send Reject.req(reason=3) and remove from registry. Alternatively, push this responsibility back to Session layer but require it to call a new HsmsConnection.AbortTransactionAsync(systemBytes) that sends the Reject. | Create a unit test with mocked timer: (1) simulate Data message arrival with SystemBytes=42, (2) fire T3 timeout for SystemBytes=42, (3) assert Reject.req(systemBytes=42, reason=3) is sent within 100ms and logged. |
| 改善 | medium | T3 timeout cleanup does not distinguish abort reason from normal timeout | SecsSession.SendAndWaitAsync (line 73) throws a generic TimeoutException when T3 expires. However, SEMI E37 specifies that T3 timeout may be caused by: (a) no reply before timeout (normal T3), (b) TCP disconnect during wait (transaction aborted), (c) Reject message from remote. Currently all three cases would only be caught if caught by the caller's finally block. The distinction is lost. File: /src/SecsGem.Core/Session/SecsSession.cs line 79 removes from _pending in all cases without recording abort reason. /src/SecsGem.Core/Session/SecsSession.cs:59-81 | Introduce a custom exception type (e.g., TransactionAbortedException(reason: string)) for non-timeout aborts. In OnData (line 100), when receiving an unsolicited Reject.req with matching SystemBytes, catch and fail the pending TCS with TransactionAbortedException('Reject received: reason X'). In StateChanged handler (when adding pending cleanup), use TransactionAbortedException('Connection lost'). Distinguish these in tests and logs. | Write three scenarios: (1) timeout without reply → TimeoutException, (2) Reject received → TransactionAbortedException('Reject: reason 1'), (3) TCP disconnect → TransactionAbortedException('Connection lost'). Assert each throws the correct exception type. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Silent data corruption for item lengths > 0xFFFFFF | The WriteHeader method in Secs2Codec.cs (line 36-43) accepts an int length parameter and assumes 3-byte length encoding can represent any value up to the max. However, the 3-byte SECS-II length field can only represent values from 0 to 0xFFFFFF (16,777,215). For lengths > 0xFFFFFF, the code silently truncates due to bitwise AND operations ((length >> 16) & 0xFF, etc.), causing incorrect encoded length values and potential data loss during round-trip decode. src/SecsGem.Core/Secs2/Secs2Codec.cs:36-43 | Add validation in WriteHeader to reject invalid lengths: if (length < 0 || length > 0xFFFFFF) throw new InvalidOperationException($"Item length must be between 0 and 0xFFFFFF, got {length}");. Alternatively, change parameter type to uint and validate the upper bound. | Write a test case that creates a SecsItem with a byte array larger than 0xFFFFFF (or simulate by direct MemoryStream write), encode it, then decode and verify the decoded length matches the original. Current test only covers length <= 0xFFFF. |
| 錯誤 | medium | SecsFormat.J (JIS-8) defined but no factory method exists | The SecsFormat enum defines J = 0x11 (JIS-8 text format per SEMI E5), and ElementSize() and ScalarText() methods reference it, but there is no public factory method to create a J format item (e.g., SecsItem.J(string value)). This creates asymmetry: wire data containing J format can be decoded and read via GetAscii(), but cannot be created programmatically or tested via round-trip encoding. The code treats J as ASCII (single-byte elements like A and B), but SEMI E5 specifies JIS-8 encoding which may differ. src/SecsGem.Core/Secs2/SecsItem.cs:45-72 (factories), SecsFormat.cs:10 (J definition) | Either (1) remove SecsFormat.J from the enum if J format is not intended to be supported, or (2) add a factory method: public static SecsItem J(string value) => new(SecsFormat.J, Encoding.ASCII.GetBytes(value ?? string.Empty)); Note: verify whether proper JIS-8 encoding is required or if ASCII-compatible treatment is acceptable per your SEMI E5 compliance requirements. | Add a round-trip test: SecsItem.J("test string") -> encode -> decode -> verify both format and string match. This test currently cannot be written due to missing factory. |
| 改善 | low | Pack/Unpack methods use inefficient intermediate byte arrays | The Pack method (line 74-84) and Unpack method (line 124-136) both allocate a new byte array for each scalar element, copy data to/from it, then dispose. For an array of 1000 U4 values, this creates 1000 temporary 4-byte arrays and 1000 BlockCopy calls. The code is functionally correct but creates unnecessary GC pressure and memory fragmentation. Modern C# Span<T> and MemoryMarshal would allow direct pointer/offset access without allocations. src/SecsGem.Core/Secs2/SecsItem.cs:74-84 and 124-136 | Refactor Pack/Unpack to use Span<byte> instead of byte[]: Pack should write directly to raw[i*size..] span slice; Unpack should read directly from _raw[i*size..] span slice without allocating intermediate arrays. This is a performance optimization that would not change behavior. | Performance regression test: measure encoding/decoding time and GC.GetTotalMemory before/after refactoring for large multi-element arrays (e.g., U4 array of 100,000 elements). No functional test changes needed—existing tests should still pass. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | WriteHeader 未驗證 length 負數與超限 | WriteHeader (Secs2Codec.cs:36-44) 接受 int length 參數，但未檢查負數或超過 0xFFFFFF (3-byte 上限)。負數會導致誤編（例如 -1 被編作 0xFF，解碼時誤讀）；超大值會截斷高位。SEMI E5 規定 list/scalar 最大長度為 3 bytes (16,777,215)。 src/SecsGem.Core/Secs2/Secs2Codec.cs:36-44 | 在 WriteHeader 開頭加入驗證：if (length < 0 || length > 0xFFFFFF) throw new InvalidOperationException(...)。此防衛應於編碼前執行。 | 編寫單元測試：(1) 嘗試編碼 SecsItem.L(-1 個子項)，應拋異常；(2) 編碼 length=0x01000000 (超限 1 byte)，應拋異常；(3) 當前合法範圍 [0, 0xFFFFFF] 應通過。 |
| 改善 | medium | RawData 存取 List 項目缺乏驗證 | SecsItem.RawData (SecsItem.cs:35) 對 List 項目無異常檢查，只回傳空陣列 Array.Empty<byte>()。在 Secs2Codec 編碼路徑 (line 30) 中，誤傳 L 項目會靜默返回空數據，導致邏輯軟故障而非明確拒絕。 src/SecsGem.Core/Secs2/SecsItem.cs:34-35 | 在 RawData 屬性中加入防衛檢查：if (Format == SecsFormat.L) throw new InvalidOperationException("RawData 不適用於 List 項目")。或改為內部專用（非公開 API）。 | 單元測試：建立 SecsItem.L()，呼叫 .RawData，應拋 InvalidOperationException（若改為內部 API，則檢查編譯期無外部呼叫）。 |
| 改善 | low | 缺乏深度邊界測試（depth=64 恰好上限） | Secs2Codec 定義 MaxDepth=64，但 Secs2CodecTests.cs 未測試恰好達上限（深度 64）及超限（深度 65）的情況。現有測試僅含 3 層嵌套。深度檢查邏輯正確（line 59-60），但邊界場景缺乏驗證。 tests/SecsGem.Core.Tests/Secs2CodecTests.cs | 新增測試方法：(1) 建立恰好深度 64 的嵌套結構 (SecsItem.L(SecsItem.L(...(SecsItem.A("x"))...)))，應編解碼成功；(2) 深度 65 應拋 InvalidDataException。使用迴圈動態構建。 | 執行新增測試用例，確認 depth=64 通過、depth=65 拋異常。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Missing maximum length validation (0xFFFFFF overflow) | The WriteHeader method (Secs2Codec.cs:36-44) selects 3-byte length encoding for any value > 0xFFFF, but SEMI E5 specifies that 3-byte encoding can only represent lengths 0x010000 to 0xFFFFFF (65536 to 16777215). Values exceeding 0xFFFFFF will silently truncate during encoding because the code writes only the lower 24 bits. For example, length 0x01000000 (16777216) encodes as three 0x00 bytes instead of rejecting the overflow or throwing an exception. src/SecsGem.Core/Secs2/Secs2Codec.cs:36-44 (WriteHeader method) | Add an explicit guard in WriteHeader: check if length > 0xFFFFFF and throw InvalidOperationException with a message like 'Length exceeds SEMI E5 maximum of 0xFFFFFF (16777215)'. Alternatively, add a constant const int MaxLength = 0xFFFFFF at the class level and reference it in both encoding and as a guard. | Create a test case: encode SecsItem with a scalar containing 16777216 or more bytes, catch the exception, verify the message mentions the limit. Round-trip test that length <= 0xFFFFFF still works correctly. |
| 改善 | medium | Clarify numLenBytes=0 rejection rationale with specification reference | DecodeItem (line 68-69) rejects numLenBytes=0 with a clear error message, which is correct per SEMI E5. However, the code comment or exception message could explicitly reference that SEMI E5 requires at least 1 length byte. The format-byte lower 2 bits encode numLenBytes as {1,2,3}, so 0 is structurally invalid. src/SecsGem.Core/Secs2/Secs2Codec.cs:68-69 | Update the error message to: 'SECS-II numLengthBytes=0 (not allowed by SEMI E5; must be 1-3)' to make the specification compliance explicit. | Decode a manually crafted byte array with formatByte & 0x03 == 0, confirm the exception message mentions SEMI E5. |
| 改善 | low | Add boundary documentation comment to WriteHeader | The length-selection logic (line 38) correctly maps ranges to byte counts: 1-byte for [0, 0xFF], 2-byte for [0x100, 0xFFFF], 3-byte for [0x10000, 0xFFFFFF]. This is not immediately obvious from the conditional expression alone. Adding an inline comment would improve maintainability. src/SecsGem.Core/Secs2/Secs2Codec.cs:38 | Add a comment above line 38: // SEMI E5: 1-byte [0x00-0xFF], 2-byte [0x0100-0xFFFF], 3-byte [0x010000-0xFFFFFF] | Code review: confirm comment matches the conditional branches and SEMI E5 specification. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | JIS-8 Format (0x11) Completely Missing Implementation | SecsFormat.J (JIS-8, 0x11) is defined in SecsFormat.cs:10 but has NO factory method (SecsItem.J()), NO accessor method (GetJis()), and NO proper handling in codec. Only ASCII encoding is supported. SEMI E5 specifies JIS-8 as a mandatory character type for international text exchange. src/SecsGem.Core/Secs2/SecsItem.cs (missing 45-73 range) | Add: public static SecsItem J(string value) factory using Encoding.GetEncoding(50220) or similar for JIS-8; add public string GetJis() accessor; ensure Secs2Codec and SmlJson handle format code 0x11 correctly. | Create test: SecsItem.J("テスト").GetJis() round-trips; encode/decode JIS-8 items; verify Secs2Codec.Encode output format byte for J matches spec. |
| 錯誤 | high | JIS-8 SML Display Uses ASCII Encoding (Wrong Code Page) | SecsItem.cs:167 in ScalarText() method uses Encoding.ASCII.GetString(_raw) for SecsFormat.J. ASCII only covers 0x00-0x7F; JIS-8 bytes 0x80-0xFF will be silently lost or garbled. SEMI E5 specifies JIS X 0208 (Shift JIS variant) for format 0x11. src/SecsGem.Core/Secs2/SecsItem.cs:167 | Replace ASCII with Encoding.GetEncoding(50220) or 'ISO-2022-JP' for proper JIS-8 decoding. Or defer to GetJis() method once factory/accessor are implemented. | Encode JIS-8 string with 0x80-0xFF bytes, call ToSml(), verify output matches original JIS-8 chars (not garbled). |
| 錯誤 | medium | SML JSON Parser Missing JIS-8 Format Key | SmlJson.cs:22-39 (ToItem switch statement) does NOT handle key 'J' or 'JIS'. Attempting to parse {"J":"text"} or {"J":[...]} throws FormatException at line 38. This breaks round-trip JSON ↔ SecsItem for JIS-8 format. src/SecsGem.Scenarios/SmlJson.cs:38 | Add case in switch: `"J" or "JIS" => SecsItem.J(v.GetString() ?? "")` after line 25 (A factory call). Requires SecsItem.J() factory to exist first. | Unit test: SmlJson.ToItem parses {"J":"テスト"}, round-trip JSON → SecsItem → JSON preserves JIS-8 text. |
| 改善 | medium | ElementSize() Returns 1 as Fallback for Unknown Formats | SecsFormat.cs:33 in ElementSize() has default case `_ => 1`. This masks errors: if an invalid/unknown format code slips through validation, it silently assumes 1-byte elements. However, DecodeItem.cs:70 checks format validity before reaching ElementSize, so risk is low—but fallback is non-deterministic. src/SecsGem.Core/Secs2/SecsFormat.cs:33 | Change default to throw: `_ => throw new InvalidOperationException($"Unknown SECS format {f}")` or remove default if enum covers all cases. Validates compile-time completeness. | Confirm all 15 format codes in SecsFormat enum map to explicit cases in ElementSize switch; run tests with exhaustive case checking enabled if available. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 改善 | medium | 浮點數特殊值（±Infinity、NaN）編碼/解碼未驗證 | SecsItem.cs 第 71-72 行使用 BinaryPrimitives.WriteSingleBigEndian/WriteDoubleBigEndian 編碼 F4/F8，測試僅涵蓋常規值 (3.14f, -1.5f, 2.718281828, -0.0)。未測試 IEEE 754 特殊值如 float.PositiveInfinity、float.NegativeInfinity、float.NaN，這些值的位元模式編碼正確但解碼驗證時易失敗（NaN != NaN）。 src/SecsGem.Core/Secs2/SecsItem.cs:71-72 (F4/F8 factory methods) + Secs2CodecTests.cs:23-24 (test coverage) | 在測試中加入 F4/F8 特殊值圓形三角測試：SecsItem.F4(float.PositiveInfinity, float.NaN, float.NegativeInfinity)，驗證解碼時使用 float.IsInfinity/IsNaN() 判斷而非直接相等比較。同時確認 BinaryPrimitives 對特殊值的 big-endian 編碼行為符合 SEMI E5。 | 執行單元測試，驗證: var special = SecsItem.F4(float.PositiveInfinity, float.NaN); var decoded = Secs2Codec.Decode(Secs2Codec.Encode(special))!; Assert.True(float.IsPositiveInfinity(decoded.GetF4s()[0])); Assert.True(float.IsNaN(decoded.GetF4s()[1])); |
| 錯誤 | medium | I1 工廠方法未驗證 sbyte 轉換範圍，邊界值截斷無檢查 | SecsItem.cs 第 58-63 行，I1(params sbyte[] values) 直接執行 raw[i] = (byte)values[i]。若傳入超出 -128~127 範圍的值，會靜默截斷（如 256 → 0、-129 → 127），違反 SEMI E5 對有號整數的要求。U8/U4/U2 等已覆蓋 Unpack 的邊界驗證，但 I1/U1 直接轉換缺乏運行時檢查。 src/SecsGem.Core/Secs2/SecsItem.cs:58-63 | 使用 checked cast: raw[i] = checked((byte)values[i]) 以在溢位時拋出 OverflowException，或在工廠方法文件中明確警告：「必須傳入合法 sbyte 值（-128~127），超範圍值將拋出異常」。建議採用 checked 方案以捕捉上層呼叫端錯誤。 | 編寫測試驗證邊界: var ex = Assert.Throws<OverflowException>(() => SecsItem.I1(unchecked((sbyte)256))); 確保編譯後執行時檢查有效。 |
| 改善 | low | 位元組序假設隱含：1-byte vs multi-byte 型別分組不明確 | SecsItem.cs 第 9 行註釋「純量內部以 big-endian raw bytes 儲存」對 U1/I1/A/J/B/Boolean 實際無關（1-byte 型別無位元組序概念）。Count 屬性（第 37-40 行）統一計算 _raw.Length / ElementSize()，隱含假設所有非 L 型別都遵循相同邏輯，但實際上 1-byte 型別無需轉換。此設計在未來維護時可能造成混淆。 src/SecsGem.Core/Secs2/SecsItem.cs:1-40 (doc + Count property) | 在類別文件頭或 ElementSize 方法上增加明確註釋分組：「1-byte 型別（無位元組序）：U1, I1, A, J, B, Boolean / Multi-byte 型別（big-endian 儲存）：U2, U4, U8, I2, I4, I8, F4, F8」。確保文件清晰區分位元組序相關與無關的型別。 | 閱讀更新後的文件註釋，驗證新開發者無需深入程式碼即可理解位元組序邊界。現有測試 Secs2CodecTests.cs 已充分驗證邏輯，無需新增測試。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Secs2Codec.EncodeItem 無 1MB 長度上限檢查，可能導致超大訊息越界 | Secs2Codec.cs 的 EncodeItem() (第 21-34 行) 與 WriteHeader() (第 36-44 行) 無條件接受任意長度的 raw data 或 list item count。SEMI E37 規定 HSMS 訊息體上限為 1MB (1,048,576 bytes)；但 Secs2Codec 允許產生超過此限的訊息體。
具體問題：
- EncodeItem 直接寫入 item.Items.Count 或 data.Length，無驗證
- WriteHeader 對長度參數無上限檢查，只計算所需 byte 數 (1-3 bytes)
- SECS-II Length field 最大 0xFFFFFF (16,777,215 bytes)，遠超 1MB 限制
對比 HsmsMessageParser.cs 第 13 與 30-31 行已定義 MaxMessageBytes 並在解碼時檢查。編碼端缺少對稱防護。 src/SecsGem.Core/Secs2/Secs2Codec.cs:21-44 | 在 Secs2Codec 中新增常數 MaxSecsItemLength = 1048576，於 EncodeItem() 呼叫 WriteHeader() 前驗證 length 不超過上限。亦可在 SecsItem factory methods (SecsItem.A/B/L 等) 進行驗證。具體: 1. 在 Secs2Codec 類中加入 const int MaxSecsItemLength = 1024 * 1024; 2. 在 EncodeItem() 的純量分支 (第 30 行之前) 加入：if (data.Length > MaxSecsItemLength) throw new InvalidOperationException(...) 3. 在 EncodeItem() 的 List 分支驗證：計算子項目序列化後的總長度（包含 header），確保不超過上限 | 撰寫單元測試：(a) 產生恰好 1MB 的 List/Binary 訊息並驗證可編碼；(b) 產生 1MB+1 的訊息並預期拋 InvalidOperationException；(c) 整合測試：嘗試透過 SecsSession 發送超大訊息，驗證送出端拒絕或接收端拋 InvalidDataException |
| 錯誤 | high | WriteHeader 允許無效 Length 值，無上界檢查可產生惡意訊息 | WriteHeader(ms, format, length) 第 36-44 行無條件接受任意 int length 值，包括負數或超大正數。若呼叫者傳入 length > 0xFFFFFF，writeHeader 會默默截斷至 24-bit，導致訊息長度欄位失真。
具體問題：
- 第 38 行計算 numLenBytes = length <= 0xFF ? 1 : length <= 0xFFFF ? 2 : 3
- 若 length > 0xFFFFFF，仍會寫 3-byte length，但實際編碼的是下 24 bits
- 無檢查 length >= 0，負數亦會被當作超大無符號數
SEMI E5 未明確禁止，但違反了 codec 完整性原則。 src/SecsGem.Core/Secs2/Secs2Codec.cs:36-44 | 在 WriteHeader() 開始加入驗證： if (length < 0 || length > 0xFFFFFF) throw new ArgumentException($"Invalid SECS-II length {length}: must be 0..{0xFFFFFF}"); 或更嚴格（結合 1MB 限制）： if (length < 0 || length > MaxSecsItemLength) throw new ArgumentException(...); 此檢查應置於 WriteHeader() 第一行，保護所有呼叫者。 | 單元測試：(a) 呼叫 WriteHeader() with length=-1, 預期拋例外；(b) with length=0x1000000 (16MB)，預期拋例外；(c) with length=0xFFFFFF，驗證能正確編碼 3-byte header；(d) with length=1048576 (1MB)，驗證能正確編碼 |
| 改善 | medium | SecsItem factory methods 應驗證輸入長度上限，前置防護 | SecsItem.A/B/U4 等 factory methods (第 45-72 行) 直接接收 byte[] 或陣列，無長度驗證。若上層應用直接呼叫 SecsItem.A(largeString) 產生超大項目，再透過 Secs2Codec.Encode() 發送，會因編碼端無檢查而產生無效訊息。
目前缺乏防御層，全依賴 Secs2Codec 端檢查，但使用者可能直接檢視 raw data 而繞過此限。 src/SecsGem.Core/Secs2/SecsItem.cs:45-87 | 在 SecsItem 類中定義 public const int MaxItemLength = 1048576，於各 factory method 的開頭驗證： - SecsItem.A(string value)：檢查 Encoding.ASCII.GetByteCount(value) <= MaxItemLength - SecsItem.B(byte[] data)：檢查 data.Length <= MaxItemLength - SecsItem.L(...)：不需要檢查單一項目（因為 count 在編碼時才計算），但可在 Encode() 時檢查訊息體總長 若超限，拋 ArgumentException with clear message。 | 單元測試：SecsItem.A(new string('X', 1048577)) 應拋 ArgumentException；SecsItem.A(new string('X', 1048576)) 應成功；SecsItem.B(new byte[1048577]) 應拋例外 |
| 改善 | low | 缺少整合測試涵蓋超大訊息邊界案例 | Secs2CodecTests.cs 有 RoundTrip 與邊界測試，但未涵蓋超大訊息。第 70-76 行的 "Two_byte_length_field_for_long_ascii" 只測試 300 bytes，距 1MB 相距甚遠。無測試驗證 1MB、1MB+1、或接近 3-byte limit (16MB) 的訊息。 tests/SecsGem.Core.Tests/Secs2CodecTests.cs:1-115 | 加入新測試方法，例如： - [Fact] public void Encode_exactly_1MB_ascii_succeeds() { var s = new string('A', 1048576); ... } - [Fact] public void Encode_over_1MB_ascii_throws() { var s = new string('A', 1048577); Assert.Throws<...>(...) } - [Fact] public void Encode_list_with_large_children_fails_if_exceeds_1MB() { ... } 此類測試應置於 Secs2CodecTests.cs 或新增 Secs2CodecBoundaryTests.cs 檔案。 | 執行 dotnet test 後，驗證新增的邊界測試全數通過；並確保舊有測試未受破壞 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Missing validation for numLenBytes > 3 | In Secs2Codec.cs:65, numLenBytes is extracted as (formatByte & 0x03), which yields values 0-3. The code validates numLenBytes==0 (line 68-69) but DOES NOT validate numLenBytes==3. However, line 77 unconditionally reads numLenBytes bytes into length. The SEMI E5 spec defines only 1, 2, and 3-byte length fields. A malformed formatByte with lower 2 bits = 0b11 (numLenBytes=3) is technically valid per the bitwise operation, but the code should explicitly validate that only 1, 2, 3 are legal. Currently, numLenBytes=3 is silently accepted without validation against the standard. src/SecsGem.Core/Secs2/Secs2Codec.cs:65-77 | Add explicit validation after line 68 to reject numLenBytes > 3: `if (numLenBytes > 3) throw new InvalidDataException($"SECS-II numLengthBytes={numLenBytes} out of range (1-3), pos={pos-1}");` | Create unit test: byte[] malformed = { 0x03, 0x01, 0x00 }; // formatByte with bits 0-1 = 0b11 (numLenBytes=3). Verify it throws InvalidDataException. Also test: { 0xFF } (formatByte=0xFF yields numLenBytes=3, formatCode=0x3F which is invalid anyway, but should fail on numLenBytes check first). |
| 錯誤 | high | Missing bounds check for list element count reading | In Secs2Codec.cs:82-83, when format==SecsFormat.L, the length value (number of child items) is used directly: `var children = new List<SecsItem>(length); for (var i = 0; i < length; i++) children.Add(DecodeItem(buf, ref pos, depth + 1));`. However, there is NO validation that length >= 0. A malformed length field (e.g., from reading negative-looking bytes in signed interpretation, though length is unsigned int) could cause issues. More critically, a very large length (e.g., 0xFFFFFFFF) would allocate a huge List, causing OOM before the decoding loop even finishes. The code validates buffer boundaries (line 73, 87) but not semantic bounds on list count. src/SecsGem.Core/Secs2/Secs2Codec.cs:76-83 | Add a configurable max-item limit for lists. For example: `const int MaxListItems = 100000; if (length > MaxListItems) throw new InvalidDataException($"SECS-II List item count {length} exceeds max {MaxListItems}");` before line 82. | Test 1: Craft a list with numLenBytes=3 and length=0xFFFFFF (16M items). Verify it throws or gracefully rejects before OOM. Test 2: Verify normal lists with 1-1000 items decode correctly. Test 3: Create a nested list scenario where total item count would explode (e.g., L[L[L[...] x M] x M] to stress depth + width). |
| 錯誤 | medium | Missing validation for length=0 in scalar (non-List) items | In Secs2Codec.cs:90-92, the elemSize check `if (elemSize > 0 && length % elemSize != 0)` correctly validates that length is a multiple of element size. However, there is NO check that length==0 is semantically valid for all format types. According to SEMI E5, empty items (length=0) are allowed for A, J, B, Boolean, U*, I*, F* types. The code accepts length=0 implicitly, which is correct, but there is NO explicit test for it, and NO validation that certain formats might reject empty data in some contexts (e.g., a U4 item with length=0 would yield 0 elements, which is valid by spec but unusual). This is a minor issue but indicates incomplete edge-case handling. src/SecsGem.Core/Secs2/Secs2Codec.cs:87-92 | Add an explicit test case for empty scalar items: e.g., byte[] empty_u4 = EncodeWithLength(U4, 0). Verify Decode returns a U4 with GetU4s().Length==0. Optionally, add a comment or configurable flag to allow/disallow empty items per SEMI E5 compliance level. No code change needed if empty items are intentionally supported; add a test to document this behavior. | Test case: Secs2Codec.Decode(SecsItem.U4().RawData) [0-length U4]. Verify it decodes to a valid U4 item with Count==0. Test GetU4s() returns empty array. Test encoding and re-decoding preserves empty state. |
| 改善 | medium | Missing test coverage for malformed header scenarios | The existing test suite (Secs2CodecTests.cs) covers unknown format codes, truncated data, and non-multiple lengths, but is missing several edge cases: (1) length field boundaries (e.g., numLenBytes=3 with edge values like 0x000000, 0xFFFFFF, 0x00FF00); (2) deeply nested lists approaching MaxDepth=64; (3) mixed valid/invalid items in a list (where first N items decode, then one fails); (4) integer overflow scenarios (e.g., pos + length wrapping); (5) malformed lists with negative or unreasonable child counts. tests/SecsGem.Core.Tests/Secs2CodecTests.cs | Add test methods: TestDecodeMaxDepthExceeded(), TestDecodeListWithOversizeLength(), TestDecodeMixedValidInvalidItems(), TestDecodeEdgeLengthFieldValues(). See comment at line 73-74 (boundary check) and line 90-92 (modulo check) for hints on what to fuzz. | Run new tests with: dotnet test from the project root. Verify all new test cases throw InvalidDataException with appropriate messages. Use property-based fuzzing (e.g., FsCheck) to generate random malformed headers and confirm all are either rejected or decoded correctly. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | System bytes counter overflow causes request/reply mismatch after 2^32 messages | SecsSession.NextSystemBytes() (src/SecsGem.Core/Session/SecsSession.cs:50) increments _sysBytes without bounds. When uint counter wraps from 0xFFFFFFFF to 0, a pending primary message expecting reply with SystemBytes N will match against a completely unrelated reply message that reuses that same SystemBytes value.
Same issue exists in HsmsConnectionStateMachine.NextSystemBytes() (line 44) for control messages (Linktest, Deselect, Separate). The counter starts at 1 but has no overflow protection; after 2^32 calls it cycles back and can collide with old pending requests.
Practical impact: In long-running industrial equipment, after ~4 billion messages, SystemBytes collision causes:
- T3 timeout in SendAndWaitAsync (line 60-75) because unrelated replies satisfy pending TCS
- Broken GEM state machines (S2F35 link event handlers, etc.)
- S9F7 errors delivered to wrong handlers due to stale pending entries SecsSession.cs:20-50, HsmsConnectionStateMachine.cs:14-44 | Implement bounded unique System bytes using a pool or collision detection: Option 1: Wrap counter modulo 0xFFFFFFFE (reserve 0 as 'no transaction') and add ConcurrentDictionary.ContainsKey check before assignment: if (_pending.ContainsKey(sysByte)) { /* skip/retry */ }. Option 2: Use a queue of 'available' SystemBytes (0xFFFFFFFE values) and return used ones to pool when TCS completes. Option 3: Maintain a small bitmap of recent in-flight SystemBytes and only assign if not present. HsmsConnectionStateMachine also needs the same fix for control-plane messages. | Unit test: Create SecsSession, call NextSystemBytes() 0xFFFFFFFF + 10 times, verify all values are unique or within the reserved 'pending' set. Integration test: Inject artificial counter overflow, send primary with W-bit=true, verify either (a) overflow protection prevents collision or (b) exception thrown before sending. |
| 錯誤 | high | Control messages reuse System bytes without tracking pending state | HsmsConnectionStateMachine (line 44) assigns unique System bytes to Linktest.req, Deselect.req, Separate.req via NextSystemBytes(), but HsmsConnectionStateMachine does NOT maintain a _pending dictionary like SecsSession does. When a Linktest.req is sent with SystemBytes=42, and a delayed Linktest.rsp arrives with the same SystemBytes, there's no matching handler—it just updates the timer state (line 177) without verifying the response matches the original request.
Risk: Malicious/network-corrupted Linktest.rsp with mismatched SystemBytes can trigger false state transitions. More critically, if a Linktest.rsp with SystemBytes X arrives out-of-order from a PREVIOUS connection cycle, HandleLinktestRsp() will incorrectly reset T6 timer for the CURRENT Linktest.req, masking a genuinely lost Linktest. HsmsConnectionStateMachine.cs:168-178 (HandleLinktestReqAsync/HandleLinktestRsp), plus Deselect/Separate handlers (lines 150-166, 180-184) | Add _pendingControl: ConcurrentDictionary<uint, TimeSpan?> to track issued control message System bytes with their T6 deadline. In HandleLinktestRsp/HandleDeselectRsp: if (_pendingControl.TryRemove(rsp.SystemBytes, out _)) { /* valid, proceed */ } else { _log($"[WARN] SystemBytes mismatch in {rsp.Label}"); return; } Similarly validate Deselect.rsp and Separate responses. This prevents replay attacks and detects out-of-order arrivals. | Unit test: Call HandleLinktestReqAsync(SystemBytes=1), then simulate HandleLinktestRsp(SystemBytes=2)—verify log warning and timer NOT reset. Integration test: Inject duplicate or reordered Linktest frames, verify state machine ignores mismatched System bytes. |
| 改善 | medium | ReplyAsync does not validate request/reply correspondence for primary W-bit | SecsSession.ReplyAsync (line 84-91) copies SystemBytes from primary to reply without checking primary.ReplyExpected. SEMI E5 mandates that secondary messages are only valid if the corresponding primary had W-bit=1. If a handler accidentally calls ReplyAsync(primaryWithWBit=0, ...), the secondary is sent but violates the protocol.
No test case explicitly validates this condition (SecsSessionTests send only requests with replyExpected=true). SecsSession.cs:84-91, test coverage gap in SecsSessionTests.cs | Add guard in ReplyAsync: if (!primary.ReplyExpected) throw new InvalidOperationException($"Cannot reply to {primary.Sxfy}: W-bit not set (ReplyExpected=false)"); This catches handler bugs early rather than silently sending protocol-violating frames. | Unit test: Call ReplyAsync(primary with ReplyExpected=false, ...) and verify InvalidOperationException. Integration test: Verify GEM handler routes never reply to W-bit=0 messages (audit all _session.ReplyAsync calls in GemEquipmentHandler.cs). |
| 改善 | low | No explicit test for System bytes uniqueness across concurrent SendAndWaitAsync calls | SecsSessionTests.cs has no test verifying that rapid concurrent SendAndWaitAsync calls produce unique System bytes or that one response does not satisfy an unrelated pending request. The test suite uses sequential patterns (send S1F1, wait for S1F2; then send S2F13, wait for reply). In production, GemEquipmentHandler.TriggerEventAsync or host command processing might invoke SendAndWaitAsync from multiple async contexts simultaneously. SecsSessionTests.cs: missing test case | Add test: async Task ConcurrentSendAndWait_each_gets_unique_SystemBytes_and_matching_reply() - Fire 10 parallel SendAndWaitAsync with different S/F pairs - Verify each pending entry has distinct SystemBytes - Wire equipment to echo back replies with matched SystemBytes - Assert all 10 replies complete within T3 without cross-matching | Run test; verify each task receives its own SystemBytes and matching reply within T3. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | SupportedStreams 未同步：定義 {1,2,5,6,9,10} 但實裝缺 S9 函式 | GemEquipmentHandler.cs:17 定義 SupportedStreams = {1,2,5,6,9,10}，但 HandleAsync() 的 switch case 未涵蓋任何 S9 函式（S9F1, S9F3, S9F5, S9F7 皆無）。當收到 S9 primary 時會觸發 UnknownAsync，導致邏輯矛盾：S9 被標記為 supported 卻無實裝。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:17, 34-70, 223-230 | 選項 A（建議）：移除 SupportedStreams 中的 9，改為 {1,2,5,6,10}；S9 訊息應自動視為 Unrecognized。選項 B：實裝 S9 訊息的 dispatch（但設備端一般無需 handle S9，故 A 更符合 SEMI E30）。 | 執行 GemEquipmentTests.cs 中的 Unknown_stream/Unknown_function 單元測試確認邏輯仍正確；補充新測試驗證 S9Fxx 會正確觸發 S9F3（stream unrecognized）。 |
| 錯誤 | high | S1F5, S1F7, S1F9 等 E30 standard S1 函式缺失 | GemEquipmentHandler.cs 的 S1 dispatch 僅實裝 S1F1/F3/F11/F13/F15/F17，但 SEMI E30 GEM 標準定義需支援：S1F5 Recommend Transaction ID (ECID list)、S1F7 Request for Communicate Device ID、S1F9 等。缺失可能導致完整流程場景無法執行。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:41-46 | 審視 SEMI E30 規範，補充 S1 missing messages 的 dispatch case；至少應涵蓋 F1-F19 的 standard pairs；若無對應需求則標記 TODO 或文件化為有意略過。 | 查閱 SEMI E30 文檔確認 E30 GEM 必需的 S1 函式清單；針對未實裝的函式撰寫單元測試，驗證它們回應 S9F5（Unrecognized Function）。 |
| 改善 | medium | dispatch switch 與 SupportedStreams 的維護性：請新增註解或常數映射 | 目前 SupportedStreams (line 17) 是硬編碼的 HashSet<int>，但 HandleAsync switch 的 case 分散在多行。若日後新增或移除 stream，易發生 SupportedStreams 與實際 case 不同步的風險（如此輪 S9 問題所示）。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:17, 34-70 | 建議：(1) 在 switch-case 上方註解 supported streams，如 // Supported: S1, S2, S5, S6, S10；(2) 考慮用字典或 enum 映射替代 HashSet，使 dispatch 更聲明式；(3) 在 UnknownAsync 前新增 debug 日誌，記錄實際收到的 stream/function 組合。 | 手動檢查 HandleAsync 涵蓋的所有 case 與 SupportedStreams 定義對齊；撰寫單元測試對所有 1-10 stream 的 edge function 進行掃描，驗證回應 S9F3 或 S9F5。 |
| 改善 | low | S2F31/32 (TIACK) 與 S2F17/18 (Time Request) 需文件化為設備特性 | GemEquipmentHandler.cs:51-52 實裝了 S2F31→S2F32 TIACK 與 S2F17→S2F18 時間回應，但在類別說明文檔 (line 7-9) 未提及；這可能造成使用者誤認為實裝不完整或無對應 S2 函式。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:7-9, 51-52 | 在 GemEquipmentHandler 類別的 XML 文檔中補充「涵蓋 S1, S2 (含 TIACK/Time), S5, S6, S10」；或在 UnknownAsync 的註解裡列舉完整的 supported SxFy 對，以供使用者快速查詢。 | 更新文檔後，針對 S2F31, S2F17 撰寫單元測試，確認回應 TIACK=0 與正確的時間戳。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | medium | Secondary messages with W-bit=1 not rejected (SEMI E37 violation) | Line 116 in SecsSession.cs creates a SecsMessage from incoming HsmsMessage, preserving the W-bit even for secondary (even-function) messages. According to SEMI E37.1, secondary messages must have W-bit=0. If a secondary message arrives with W-bit=1, it violates the standard and should be rejected with HSMS Reject.req (SType 7, reason 1/2/3).
Current code: var msg = new SecsMessage(hm.Stream, hm.Function, hm.WBit, root, hm.SessionId) at line 116
The WBit is blindly transferred without validating that secondary (even function) messages have WBit=false. src/SecsGem.Core/Session/SecsSession.cs:116 | Add validation before line 116: if (hm.Function % 2 == 0 && hm.WBit) { log warning; reject or discard; return; } Alternatively, normalize: var wbit = (hm.Function % 2 == 1) ? hm.WBit : false; and pass that to SecsMessage constructor. | Create a unit test: send S1F2 (secondary) with W-bit=1 from host to equipment; verify equipment either logs a warning, discards it, or sends HSMS Reject.req. Currently the message is silently accepted. |
| 改善 | medium | Orphaned secondary messages treated as primary and forwarded to handler | Line 123 in SecsSession.cs: if (msg.IsSecondary && _pending.TryRemove(msg.SystemBytes, out var tcs)) { ... return; }
If a secondary message arrives with no matching SystemBytes in _pending (e.g., unsolicited reply, duplicate, or timeout already occurred), control falls through to line 131 and the secondary is forwarded to PrimaryReceived event.
According to SEMI E30 § 11.2, unsolicited secondary messages should be discarded silently or logged as protocol anomalies, not processed as primary. GemEquipmentHandler.OnPrimary (line 29 in GemEquipmentHandler.cs) will then attempt to handle S2F14, S1F2, etc. as if they were requests, which is semantically wrong. src/SecsGem.Core/Session/SecsSession.cs:123-131 | Add explicit discard logic after line 128: ```csharp if (msg.IsSecondary) { Log?.Invoke($"[WARN] Orphaned secondary {msg.Sxfy} (SystemBytes={msg.SystemBytes}) discarded"); return; // Don't forward to PrimaryReceived } ``` This ensures secondaries without pending requests are not handed to application logic. | Send two S1F13 (Establish Comms) requests with W-bit=1 from equipment; discard the first reply; send a duplicate S1F14 manually with old SystemBytes after the timeout. Verify it is logged as orphaned, not forwarded to GemEquipmentHandler. |
| 改善 | low | W-bit semantics not documented in ReplyAsync | Line 83-86 in SecsSession.cs: The ReplyAsync method hardcodes replyExpected=false (correct per SEMI standard), but the docstring does not explicitly state that secondary messages must have W-bit=0. Future maintainers may not understand why the parameter is ignored.
The method signature is: public Task ReplyAsync(SecsMessage primary, int function, SecsItem? body)
There is no way to override the W-bit to true (and rightly so, but this should be explicit in the contract). src/SecsGem.Core/Session/SecsSession.cs:83-86 | Update docstring to clarify: ```csharp /// <summary> /// 以相同 SystemBytes 回覆收到的 primary。 /// W-bit 強制為 false（secondary 訊息不可要求回覆，符合 SEMI E37.1）。 /// </summary> ``` | Code review: verify all callers of ReplyAsync understand that replies are always non-waiting. Check GemEquipmentHandler (lines 41, 76, 85, 94, 100, 107, 115, etc.). |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | S9 訊息使用奇數 function，違反 SEMI E5 規範 | GemEquipmentHandler.cs 第 32、227、229 行發送 S9F3（Unrecognized Stream）、S9F5（Unrecognized Function）、S9F7（Illegal Data）使用奇數 function。根據 SEMI E5，S9 訊息應為 secondary（偶數），不應為 primary（奇數）。SecsMessage.cs 第 7 行文件註解明確指出「奇數 Function = primary（請求）、偶數 Function = secondary（回覆）」，但 S9 應作為對非法/未認可訊息的錯誤回應（secondary）。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:32, 227, 229 | 將 S9 訊息的 function 改為偶數對應：S9F2（Device ID Not Valid）、S9F4（Unrecognized Stream）、S9F6（Unrecognized Function）、S9F8（Illegal Data）。或檢查 SEMI E5 實務慣例，如果 S9 應作為無配對的獨立訊息，須明確文件說明原因。 | 運行 SecsGem.Integration.Tests GemEquipmentTests.cs 的 Unknown_stream_triggers_S9F3() 與 Unknown_function_in_supported_stream_triggers_S9F5() 測試，檢查它們是否預期 S9 作為 primary 或應重構為 secondary。另外檢查 IsPrimary/IsSecondary 邏輯在變更後是否仍正確識別 S9 訊息。 |
| 錯誤 | high | S9 訊息未保留原始 SystemBytes，違反 SEMI E5 配對規則 | GemEquipmentHandler.cs 第 32、227、229 行使用 SendAsync() 發送 S9，導致 SecsSession.cs 第 55 行自動產生新的 SystemBytes。根據 SEMI E5，S9（作為 secondary）應保留觸發錯誤的原始訊息的 SystemBytes，以維持 request/reply 配對。目前實作無法將 S9 與原始 primary 訊息關聯。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:32, 227, 229 與 src/SecsGem.Core/Session/SecsSession.cs:50-57 | 修改 OnPrimary 與 OnDecodeFailed 邏輯：(1) 創建新方法 SendErrorAsync(SecsMessage primary, int errorFunction, SecsItem body)，保留原始 SystemBytes；(2) 在 OnDecodeFailed 中改為 SendErrorAsync(constructedPrimary, 8, header)；(3) 在 UnknownAsync 中傳遞原始訊息的 SystemBytes 以保持配對。 | 增加測試驗證 S9 訊息的 SystemBytes 與觸發錯誤的 primary 訊息相同。利用 SecsSession.MessageLogged 事件記錄 frame 並檢查 system bytes 欄位。 |
| 改善 | medium | IsSecondary 邏輯對 S9 訊息不適用，缺乏文件說明 | SecsMessage.cs 第 37 行定義 IsSecondary = (Function != 0 && Function % 2 == 0)。若 S9 仍保持奇數 function，此邏輯仍認為 S9 是 primary。但如果 S9 改為偶數，SecsSession.cs 第 123 行的分派邏輯（根據 IsSecondary 與 pending 字典匹配）將無法正確處理 S9，因為 S9 可能沒有對應的 pending primary（在 decode 失敗時）。 src/SecsGem.Core/Session/SecsSession.cs:100-132 | 澄清設計：(1) 在 SecsMessage 與 SecsSession 的文件中明確說明 S9 如何分類（作為 primary 還是 secondary）；(2) 在 OnData 邏輯中新增特殊處理：若 msg.Stream == 9，不論 function 奇偶，觸發 ErrorReceived 或其他專用事件；(3) 不依賴 IsPrimary/IsSecondary 來路由 S9 訊息。 | 添加註釋說明 S9 的分派規則，並創建專門測試覆蓋 S9 的所有場景（decode 失敗、unknown stream、unknown function）。檢查 GemHostHandler 第 41 行的 S9 事件捕捉邏輯是否仍有效。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | S9F3/S9F5/S9F7 body format violates SEMI E30 spec | Lines 225, 227, 229, and 32 in GemEquipmentHandler.cs send S9 error messages with body `SecsItem.B(header)` instead of `SecsItem.L(SecsItem.B(header))`. According to SEMI E30 standard, all S9 error messages (Unrecognized Stream, Unrecognized Function, Illegal Data) must wrap the SHEAD in a List container. Current implementation sends bare binary, which violates the protocol schema and will fail validation on compliant Host parsers. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:32, 225-229 | Wrap the binary SHEAD in a List: Replace `SecsItem.B(header)` with `SecsItem.L(SecsItem.B(header))` in three places: (1) line 32 for S9F7, (2) line 227 for S9F3, (3) line 229 for S9F5. This ensures the message body matches SEMI E30 format: L[SHEAD]. | Add assertion in integration tests (GemEquipmentTests.cs lines 80-81 and 99-100) to validate that received S9 messages have body structure: `Assert.Equal(SecsFormat.L, s9.Task.Result.Root.Format)` and `Assert.Equal(1, s9.Task.Result.Root.Count)` and `Assert.Equal(SecsFormat.B, s9.Task.Result.Root[0].Format)`. Run via: `dotnet test` to confirm S9 body is now L[SHEAD]. |
| 改善 | medium | S9 dispatch path missing comprehensive error test coverage | Current tests (GemEquipmentTests.cs lines 66-102) only verify that S9F3/S9F5 are triggered and have correct Stream/Function numbers. They do not validate: (1) SHEAD body content (should be exactly 10 bytes from original HSMS frame at offset 4-14), (2) whether SHEAD extraction handles edge cases (messages shorter than 14 bytes), (3) whether S9 messages are never-reply (W-bit=0 enforced), (4) behavior when receiving malformed S9 responses. tests/SecsGem.Integration.Tests/GemEquipmentTests.cs:66-102 | Extend GemEquipmentTests with additional test cases: (a) Verify S9 body structure matches SEMI E30 (L[SHEAD with exactly 10 bytes]); (b) Send short malformed frames to ensure SHEAD extraction doesn't crash with empty array; (c) Verify that Host S9 handler (GemHostHandler.cs line 41) gracefully handles S9F3/S9F5/S9F7 in ErrorReceived event and can parse the body; (d) Add integration test for S9F7 (Illegal Data) path via decode failure. | Write test: `Assert.NotNull(s9.Task.Result.Root); Assert.Equal(SecsFormat.L, s9.Task.Result.Root.Format); Assert.Equal(10, s9.Task.Result.Root[0].GetBinary().Length);`. Also test malformed case by sending manually constructed short frame. Run: `dotnet test GemEquipmentTests` to confirm all variants pass. |
| 改善 | low | Ambiguous W-bit contract in dispatch path documentation | UnknownAsync (line 223) sends S9F3/S9F5 with `replyExpected=false` (W-bit=0), which is correct per SEMI E30. However, there's no explicit contract documentation explaining WHY S9 errors are never-reply. The SecsSession.ReplyAsync (line 84-90) and SendAsync (line 52-57) methods have XML docs, but UnknownAsync lacks explanation. This ambiguity could lead to future maintainers accidentally changing W-bit to `true` thinking the Host needs to acknowledge S9 errors. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:223 | Add XML documentation to UnknownAsync method: `/// <summary>Dispatch unrecognized SxFy to S9 error (per SEMI E30). S9F3/S9F5/S9F7 are never-reply (W-bit=0) and contain SHEAD (10-byte original message header) wrapped in a List. Equipment must NOT expect S9F4/S9F6/S9F8 secondary replies.</summary>` | Inspect code review: documentation reads clearly and matches implementation. Run `dotnet build` to ensure no warnings. Verify by grep that no other code paths attempt S9 with `replyExpected=true`. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Missing device ID validation in incoming message processing | SecsSession.OnData (line 116) accepts incoming SessionId from HSMS header without validating it matches the configured DeviceId. Per SEMI E37/E30, equipment should reject or warn on mismatched device IDs. Currently hm.SessionId is directly stored in SecsMessage.DeviceId with no validation against Options.DeviceId. src/SecsGem.Core/Session/SecsSession.cs:116 | Add validation: if (hm.SessionId != 0 && hm.SessionId != _conn.Options.DeviceId) { log warning/error and either reject or flag the mismatch }. Consider emitting a validation event for upper layers to decide response (e.g., send S9F1 Invalid Message). | Unit test: send a message with mismatched SessionId and verify it's either rejected, logged, or triggers S9F1. Integration test: simulate host/equipment with different device IDs in a pair. |
| 改善 | high | S9F1 (Invalid Message) not implemented; only S9F3/F5/F7 present | GemEquipmentHandler (lines 226-230) generates S9F3 (Unrecognized Stream) and S9F5 (Unrecognized Function), plus S9F7 (Illegal Data) for decode failures. However, S9F1 (Invalid Message, per SEMI E30) is never generated. S9F1 is the catch-all for protocol violations (bad device ID, invalid stream/function combo in context, etc.). Currently no mechanism exists to send S9F1. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:223-230 | Add S9F1 generation in SecsSession (layer below GEM handler) when device ID mismatch is detected. Alternatively, add an event (like DecodeFailed) and let GemEquipmentHandler send S9F1 in response to device validation failures. Define when S9F1 vs S9F3/F5/F7 should be used in a comment referencing SEMI E30 spec. | Add test case: send message with wrong device ID and assert S9F1 is returned. Verify SHEAD (10-byte header from incoming message) is correctly embedded in S9F1 body. |
| 錯誤 | medium | SHEAD direction not enforced in S9F1 (and other S9 messages) | Per SEMI E30, S9F1 body must contain SHEAD = the received message's 10-byte HSMS header (to identify which request caused the error). Currently, S9F3/F5 errors are sent without body (line 227-229), and S9F7 includes header (line 32) but direction is unclear. S9F1 should always echo the incoming MHEAD as SHEAD. The OnDecodeFailed mechanism (line 31-32) extracts header correctly, but S9F3/F5 do not. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:226-229 | Refactor UnknownAsync to also receive the original message's header and embed it in S9F3/F5 body: await _session.SendAsync(new SecsMessage(9, 3, false, SecsItem.B(m.Header))); (similar to S9F7 at line 32). Add a comment: 'SEMI E30: S9 body contains SHEAD = incoming message header to identify the error source.' | Test: send S1F99 (unknown function) and capture S9F5 response, verify body contains the correct 10-byte SHEAD from the request. Compare against SEMI E30 spec. |
| 改善 | medium | No explicit device ID mismatch handling; ambiguous behavior with mixed device IDs on same session | ToFrame method (line 96) chooses device ID via ternary: m.DeviceId != 0 ? m.DeviceId : Options.DeviceId. This allows creating messages with arbitrary device IDs without validation. Incoming messages accept any device ID. No schema/documentation clarifies whether a multi-device session is intended or if device ID must always match configured value. src/SecsGem.Core/Session/SecsSession.cs:96 | Document the intended device ID model: (a) single device ID per session - enforce incoming device ID matches Options.DeviceId, or (b) multi-device session - define how device routing works. If (a), add strict validation and S9F1 for mismatches. If (b), add device ID mapping/routing layer with explicit documentation. | Code review: define device ID scoping per SecsSession in docstring. Create test case with mismatched device IDs and document expected behavior (reject, warn, accept). Update GemEquipmentHandler docstring to clarify device ID assumptions. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | SystemBytes Overflow & Counter Wraparound Risk | SecsSession.cs line 21 declares `_sysBytes` as `int`, but line 50 casts to `uint`. After ~2.1 billion calls, the int counter wraps to negative, then casts to a large uint (0x80000000 and above). This creates SystemBytes collisions with older pending transactions: a new SendAndWaitAsync might reuse a counter value of an un-acknowledged earlier request, causing the reply to complete the wrong TCS and leave the original waiter hanging indefinitely. src/SecsGem.Core/Session/SecsSession.cs:21, 50 | Change `private int _sysBytes;` to `private uint _sysBytes;` and update `NextSystemBytes()` to return `(uint)Interlocked.Increment(ref _sysBytes);` directly without casting. This ensures the counter never collides and monotonically increases in the uint domain. | Write a unit test that calls Interlocked.Increment on a uint counter 10 times and verify no wraparound occurs before reaching uint.MaxValue. Then add an integration test that simulates rapid SendAndWaitAsync calls and validates each reply matches its corresponding request by SystemBytes. |
| 錯誤 | high | Unsolicited Secondary Messages Silently Treated as Primary | SecsSession.cs lines 123–131: if a secondary message (even Function) arrives with a SystemBytes that is not in `_pending._TryRemove()`, the code falls through and invokes `PrimaryReceived?.Invoke(msg)` at line 131. This is a SEMI E37/E30 violation: unsolicited secondaries (orphaned replies with no matching request) should be logged as errors and optionally trigger an S9F7 response. The current behavior silently dispatches them to GemEquipmentHandler.OnPrimary(), which will likely try to call ReplyAsync() to an already-closed transaction, wasting resources. src/SecsGem.Core/Session/SecsSession.cs:123–131 | After the secondary check (line 123), add an explicit secondary-matched-failure path: if `msg.IsSecondary && !_pending.ContainsKey(msg.SystemBytes)`, log a warning (e.g., 'Orphaned secondary S{Stream}F{Function} SystemBytes={msg.SystemBytes}') and return early without invoking PrimaryReceived. Alternatively, add an event `UnsolicitedSecondaryReceived` for the upper layer to handle (e.g., send S9F3). | Create an integration test: Host A sends S1F1 (SystemBytes=100) to Host B, but Host B's response is lost. Manually inject a spoofed S1F2 with SystemBytes=100 after the first timeout. Verify that the duplicate secondary is logged and not delivered to PrimaryReceived event. |
| 錯誤 | medium | Race Condition: Response May Arrive Before _pending Insertion in SendAndWaitAsync | SecsSession.cs lines 60–81: Line 62 assigns `SystemBytes`, then line 64 inserts into `_pending`. If a very fast local loopback or concurrent test routes a response through OnData() (which calls _pending.TryRemove at line 123) between these two instructions, the TCS will not be in the dictionary, and the response is misclassified as a primary. Although Interlocked.Increment at line 62 is atomic, the subsequent dictionary insertion is not. The HsmsConnection event loop uses a Channel<HsmsEvent> with a single reader (line 56 in HsmsConnection.cs), so messages are serialized, but the ordering of _pending insertion vs. network delivery is not guaranteed if AsyncIO context switches occur. src/SecsGem.Core/Session/SecsSession.cs:60–81 | Wrap the insertion and TCS creation in a helper before awaiting SendDataAsync. Move lines 62–64 into a try-block-start operation that atomically registers the pending transaction. Consider: (a) insert into `_pending` before assigning to `msg.SystemBytes`, so the key exists before the frame is built, or (b) use a lock or ReaderWriterLockSlim to ensure _pending is thread-safe across frame sending and reply reception. The ConcurrentDictionary itself is thread-safe, but the protocol state is not. | Run a loopback stress test with hundreds of concurrent SendAndWaitAsync calls on a single SecsSession pair. Monitor for lost replies (TCS that never complete) or misrouted replies (wrong TCS receives the result). Use a custom test harness to inject delays between NextSystemBytes() and _pending insertion to force the race condition. |
| 改善 | medium | Missing Stream/Function Correspondence Validation | SecsSession.cs and GemEquipmentHandler.cs lack validation that a received secondary message's Stream/Function matches the structure of the sent primary. For example, if a host sends S1F1W (expecting S1F2), but receives S1F4 (an even secondary but different stream) with the same SystemBytes due to a malformed device, the code accepts it as a valid reply. While SEMI E30 defines strict request-reply pairs per transaction, the current code only checks the Function parity (even) and existence in _pending. src/SecsGem.Core/Session/SecsSession.cs:100–132, src/SecsGem.Core/Gem/GemEquipmentHandler.cs:34–70 | Enhance the _pending dictionary to store not just TaskCompletionSource<SecsMessage>, but also metadata: `ConcurrentDictionary<uint, (TaskCompletionSource<SecsMessage> TCS, int ExpectedStream, int ExpectedFunction, DateTime CreatedAt)>`. When a secondary arrives, validate that msg.Stream == expected and msg.Function == expected before resolving the TCS. Log a warning if the secondary function is wrong, and optionally send S9F5 (Unrecognized Function) instead. | Add a unit test: SendAndWaitAsync(S1F1) and inject a malformed reply S2F14 with matching SystemBytes. Verify the reply is rejected and logged, and the original await times out cleanly with T3. Also verify that the correct S1F2 reply, when sent later with the same SystemBytes, does complete the original waiter. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | S1F15 未重置通訊狀態違反 SEMI E30 | GemEquipmentHandler.cs:97-101 S1F15(m) 實現中，僅設定 ControlState = Offline，未重置 CommunicationState 為 NotCommunicating。根據 SEMI E30 狀態轉遷規範，Request OFF-LINE (S1F15/S1F16) 應同時結束通訊狀態，以符合『控制狀態 × 通訊狀態』的二維狀態機。目前遺漏此重置，導致在 Offline 狀態下仍可能有 Communicating 標記，造成狀態不一致。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:97-101 | 在 S1F15 中添加 `_model.CommunicationState = GemCommunicationState.NotCommunicating;` 確保控制與通訊狀態同步轉遷。可參考 S1F13 的實現，在應答前同時更新兩個狀態變數。 | 編寫測試：(1) 執行 S1F13 進入 Communicating，驗證狀態為 Communicating；(2) 執行 S1F15，驗證 ControlState=Offline 且 CommunicationState=NotCommunicating；(3) 嘗試 TriggerEventAsync，應返回 null（無法發送事件報表）。 |
| 錯誤 | medium | S1F13 未驗證先決條件 ControlState | GemEquipmentHandler.cs:73-79 S1F13(m) 無條件接受 Establish Communications 請求。根據 SEMI E30，S1F13 應只在 ControlState 為 OnlineLocal 或 OnlineRemote 時有效，若在 Offline 狀態下不應允許建立通訊。目前實現缺少此前置條件檢查，可能導致不規範的狀態轉遷。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:73-79 | 在 S1F13 開頭添加驗證邏輯。若 ControlState == Offline，回覆 S1F14 with error code（通常為 0x01 或拒絕碼）；僅在 OnlineLocal/OnlineRemote 時進行通訊建立。建議檢視 SEMI E30 Figure 2 狀態轉遷圖確認具體拒絕碼。 | 測試場景：(1) 初始狀態 Offline，發送 S1F13，驗證回覆碼非 0x00；(2) 轉為 OnlineRemote 後發送 S1F13，驗證成功（碼 0x00）；(3) 再次 Offline，驗證 S1F13 被拒。 |
| 改善 | medium | TriggerEventAsync/SendAlarmAsync 缺乏拒絕事件的診斷日誌 | GemEquipmentHandler.cs:235-251 中的 TriggerEventAsync 與 SendAlarmAsync，在檢測到 CommunicationState != Communicating 或 Event/Alarm 不存在時，返回 null。生產環境無法區分『事件被禁用』、『通訊未建立』、『ID 不存在』等失敗原因，影響故障排診效率。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:235-251 | 在返回 null 前加入結構化日誌（Debug.WriteLine 或 ILogger），記錄失敗原因。例：`Debug.WriteLine($"TriggerEvent CEID={ceid} rejected: CommunicationState={_model.CommunicationState}");` 便於設備日誌查閱。或更好地，考慮返回 Result<SecsMessage?> 型別以明確傳達拒絕理由。 | 運行場景：(1) NotCommunicating 狀態呼叫 TriggerEventAsync，觀察日誌輸出；(2) 檢查日誌記錄清晰區分不同拒絕原因；(3) 與生產設備日誌格式比較，確保診斷訊息足夠詳盡。 |
| 改善 | low | HSMS 連線斷線未自動重置 GEM 通訊狀態 | 當底層 HSMS 連線（HsmsConnection）轉入 NotConnected 狀態時，GEM 層的 CommunicationState 與 ControlState 仍保持前值。根據 SEMI E30，連線中斷應自動重置所有狀態回初始值。目前無法察覺 HSMS StateChanged 事件與 GEM 狀態同步的機制。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs (初始化處) | 在 GemEquipmentHandler 建構式中訂閱 _session.StateChanged 事件，當 HsmsState 轉入 NotConnected 時，自動重置：`_model.ControlState = GemControlState.OnlineLocal; _model.CommunicationState = GemCommunicationState.NotCommunicating;` 確保 GEM 狀態與底層連線狀態同步。 | 整合測試：(1) 建立通訊並進入 Communicating；(2) 主動斷線（TCP Close）；(3) 驗證 GEM 狀態自動回到 OnlineLocal + NotCommunicating；(4) 確認後續 S6F11 無法發送（如預期被拒）。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | S1F17 ONLACK byte incorrect when already ONLINE-REMOTE | GemEquipmentHandler.cs line 105: When equipment is already in OnlineRemote state and receives S1F17 (Request ONLINE), the ONLACK should be 0x02 (already online), but the code sets ONLACK=0x00. SEMI E30 requires: ONLACK 0x00 = transition successful, 0x02 = already remote online. Current code checks state BEFORE the transition, then always sets state to OnlineRemote regardless of initial state. This violates compliance when S1F17 is repeated. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:103-108 | Fix line 105 logic: Move the state check AFTER the assignment, or preserve the old state before changing it. Correct order: (a) check if already OnlineRemote, (b) set ONLACK accordingly, (c) update state. Example: `byte onlack = _model.ControlState == GemControlState.OnlineRemote ? (byte)0x02 : (byte)0x00; _model.ControlState = GemControlState.OnlineRemote; return ...` | Write a unit test: (1) Set model.ControlState = OnlineRemote, (2) Send S1F17, (3) Assert response byte == 0x02 (not 0x00), (4) Assert state remains OnlineRemote. Also test S1F17 from Offline/OnlineLocal states expecting ONLACK=0x00. |
| 錯誤 | high | S1F15 no validation: can offline from any state, including already Offline | GemEquipmentHandler.cs line 99: S1F15 (Request OFFLINE) unconditionally sets ControlState=Offline without validating pre-conditions. SEMI E30 defines specific valid transitions (e.g., Offline→Offline is idempotent but should ideally validate equipment safety). More critically, there is no check whether the transition is valid from the current state. Additionally, OFLACK (line 100) is always 0x00; spec may require different codes for illegal transitions or equipment-specific conditions. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:97-101 | Add state validation and error handling: (a) Check if transition from current state is valid (e.g., from OnlineRemote/OnlineLocal to Offline is OK, but may need safety/busy checks), (b) Return OFLACK=0x01 or other code if transition denied, (c) Only set state=Offline on success. Reference SEMI E30 for valid transition matrix and OFLACK code meanings (0x00=success, 0x01=denied, etc.). | Test scenarios: (1) S1F15 from OnlineRemote→Offline expect OFLACK=0x00, state=Offline, (2) S1F15 from OnlineLocal→Offline expect success, (3) S1F15 when Offline expect either success or validated deny-code, (4) Verify no race conditions if S1F15 sent during active communication. |
| 錯誤 | high | ControlState default is OnlineLocal, not Offline per SEMI E30 | EquipmentModel.cs line 72: Default ControlState is set to OnlineLocal, but SEMI E30 Section 5.2 specifies equipment must start in Offline state. This is a compliance violation—every instantiated EquipmentModel is already 'online' without explicit S1F17 acknowledgement from host. src/SecsGem.Core/Gem/EquipmentModel.cs:72 | Change default: `public GemControlState ControlState { get; set; } = GemControlState.Offline;` Equipment transitions to online only after host successfully sends S1F17 and equipment responds with ONLACK=0x00. | Unit test: Create new EquipmentModel, assert ControlState == Offline without any message exchange. Then send S1F17 and verify transition to OnlineRemote. |
| 改善 | medium | No test coverage for state transition idempotency and edge cases | GemEquipmentTests.cs tests happy-path S1F15/S1F17 once (lines 53-61) but does not cover: (1) repeated S1F17 in OnlineRemote state (should return ONLACK=0x02), (2) S1F15 followed immediately by S1F17, (3) S1F17 before S1F13 (communication not established), (4) state transitions during active event reporting, (5) concurrent state change requests. tests/SecsGem.Integration.Tests/GemEquipmentTests.cs:22-62 | Add new test method covering: (a) repeated S1F17 (assert ONLACK=0x02), (b) alternating offline/online cycles, (c) S1F15 during pending event (verify clean queue), (d) concurrent S1F15/S1F17 ordering. Consider state machine diagram in test comments. | Run extended test suite; all state transitions should match SEMI E30 table; response codes (OFLACK/ONLACK) must be correct per spec. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 改善 | medium | S1F3/F4 and S1F11/F12 request format validation | GemEquipmentHandler.cs lines 81-95: The S1F3 and S1F11 handlers call NumericIdList(m.Root) which silently returns an empty list when m.Root is null or not a List format. This causes the handler to return ALL status variables instead of rejecting malformed requests. Per SEMI E30, the request body should be validated to be a List of SVID (U4). Non-List formats may indicate a protocol error that should be handled explicitly. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:275-281 | Add explicit validation in NumericIdList or S1F3/S1F11 to check that m.Root is a List before processing. Consider returning an error response (S9F5 Unrecognized Function or S9F1 Stream/Function error) when request format is invalid, rather than silently treating it as 'return all' request. Alternatively, document this behavior as intentional graceful degradation. | Create a test case sending S1F3 with request body as a non-List type (e.g., U4(1) instead of L[U4(1)]) and verify the response. According to strict SEMI E30, this should either error or return empty; currently it returns all variables. |
| 改善 | low | S1F11/F12 missing SVIDs return empty Name/Units | GemEquipmentHandler.cs lines 90-93: When a requested SVID does not exist in StatusVariables, S1F11 returns L[U4(id), A(""), A("")]. While this is valid SECS-II syntax, SEMI E30 may specify different handling (e.g., skip missing SVIDs entirely, return error flag, or include a 4th field indicating unavailability). The current implementation returns a complete row with empty NAME and UNITS, which could be ambiguous to the Host. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:90-93 | Verify SEMI E30 Section 12.4.3 for exact S1F12 response format when a requested SVID does not exist. Consider options: (1) Filter out non-existent SVIDs from response, (2) Add a 4th status/availability field, or (3) Document current behavior as intentional representation of 'undefined SVID'. | Send S1F11 with a mix of existing and non-existing SVIDs (e.g., L[U4(1), U4(999)]), and verify the response contains both rows with identical structure but empty fields for SVID 999. Compare with reference GEM equipment behavior. |
| 改善 | low | S1F3/F4 missing SVIDs return empty List placeholder | GemEquipmentHandler.cs line 84: When a requested SVID does not exist in StatusVariables, S1F3 returns SecsItem.L() (empty List) as a placeholder value. Per SEMI E30, this is likely correct, but it creates a structural mismatch: defined SVIDs return their actual value (which could be any SECS format), while undefined SVIDs return an empty List. This could confuse Host parsers expecting consistent value types. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:81-85 | Review SEMI E30 specification for S1F4 response format when SVID does not exist. Consider: (1) Only include existing SVIDs in response (ignoring undefined requests), (2) Return a standardized null/error value (e.g., B(0xFF)), or (3) Include a 2-item array [value, status] to indicate availability. Document the chosen approach in code comments. | Send S1F3 requesting a non-existent SVID and verify the response. Type-check the returned item in response to ensure it matches the expected format (should be L, but verify this is intentional per spec). |
| 改善 | low | Empty request to S1F3/F4 silently returns all variables | GemEquipmentHandler.cs line 269: When S1F3 or S1F11 receives an empty List request (L[]), the SelectVars function silently interprets this as 'return all variables'. SEMI E30 may specify different behavior: either return nothing (empty L response), or reject empty requests with an error. Current behavior is non-obvious and could surprise Host implementations. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:267-271 | Document the behavior of empty SVID list requests in code comments. If SEMI E30 specifies different handling (e.g., reject with error or return empty response), implement a check in S1F3/S1F11 handlers before calling SelectVars to handle empty requests explicitly. | Send S1F3 with request body L[] (empty list) and verify the response. Current behavior returns all SVIDs in sorted order; verify this matches SEMI E30 Section 12.4.1 specification for empty request handling. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | S2F15 不驗證 Equipment Constant 值範圍 | GemEquipmentHandler.cs 第 126 行：設定 Equipment Constant 時直接賦值 `ec.Value = pair[1]`，未檢查新值是否落在 Min/Max 範圍內。按 SEMI E30 規範，應驗證數值型態匹配、值在允許範圍內，違反時返回 EAC != 0x00。 src\SecsGem.Core\Gem\GemEquipmentHandler.cs:118-129 | 在 S2F15 S2F16 前插入驗證邏輯：比較 `pair[1]` 與 `ec.Min`/`ec.Max`（需實作 SecsItem 數值比較）；若超出範圍則返回 EAC=0x02（Illegal value）；若型態不合則 EAC=0x02。可新增 EquipmentConstant.ValidateValue(SecsItem) -> bool 方法。 | 編寫單元測試：S2F15 設定超過 Max 的值（如 ECID 1 = 500，而 Max=300），驗證回覆為 S2F16 EAC=0x02；再用 S2F13 確認值未被修改（仍為原值或 Default）。 |
| 錯誤 | high | S2F16 EAC 碼永遠返回 0x00，未區分錯誤 | GemEquipmentHandler.cs 第 128 行：無論 EC 設定成功或失敗，S2F16 都回覆 `SecsItem.B(0x00)`。按 SEMI E30，EAC 應返回不同碼：0x00=成功、0x01=EC 不存在、0x02=非法值（超範圍、型態錯誤）、0x03=裝置忙、0x04=其他。 src\SecsGem.Core\Gem\GemEquipmentHandler.cs:118-129 | 重構 S2F15：對每組 [ECID, ECV]，逐一驗證並蒐集結果（EC存在否、值合法否）；S2F16 傳回分別的 EAC。或改為全量驗證後若任何失敗則整體 EAC=0x02。建議前者更符合 SEMI E30 設計（允許部分成功）。 | 測試場景：(1) 發 S2F15 設定已存在 EC、值在範圍內 → 期待 EAC=0x00；(2) 發 S2F15 設定不存在的 ECID（如 999） → 期待 EAC=0x01；(3) 發 S2F15 值超過 Max → 期待 EAC=0x02。 |
| 改善 | medium | 缺少 S2F29 Equipment Constant Namelist 的測試覆蓋 | S2F29 實作（GemEquipmentHandler.cs 第 131-138 行）格式正確，返回 [ECID, Name, Min, Max, Default, Units]，但測試集（GemEquipmentTests.cs、GemFullFlowTests.cs）皆未覆蓋 S2F29→S2F30 對話。缺乏驗證確保 Min/Max/Default 被正確傳回。 tests\SecsGem.Integration.Tests\GemFullFlowTests.cs | 在 GemFullFlowTests.cs 新增測試 `EquipmentConstant_namelist_S2F29_returns_metadata()` 或在 `EquipmentConstant_set_and_read_back()` 中補充 S2F29 驗證：執行 S2F29 查詢 ECID 1，驗證應返回 Name="MaxTemp"、Min=0、Max=300、Default=200、Units="C"。 | 執行 dotnet test，新測試應通過；手動用 Web UI 或 CLI 發送 S2F29，確認響應包含完整的 Min/Max/Default 欄位，其值與 EquipmentConstant 定義一致。 |
| 改善 | low | EquipmentConstant 缺少型態驗證輔助方法 | EquipmentConstant 類（EquipmentModel.cs 第 20-29 行）未提供 Min/Max/Default 型態一致性校驗或 SecsItem 數值比較方法。S2F15 要驗證輸入值合法性，需能判斷 Value 與 Min/Max 是否相容（數值型態、大小順序）。 src\SecsGem.Core\Gem\EquipmentModel.cs:20-29 | 在 EquipmentConstant 類中新增方法 `bool TryValidate(SecsItem newValue, out string? error)` 或 `bool IsValueInRange(SecsItem value)`，用以比較 SecsItem 的數值大小（U/I/F 型態）；在 S2F15 中調用以決定是否接受新值。 | 單元測試 EquipmentModel 的驗證方法：給定 Min=U2(0)/Max=U2(300)，測 Value=U2(250)→true、Value=U2(350)→false、Value=A("300")→適當型態錯誤処理。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | medium | S2F31 (時間設定) 未實作解析與應用 | GemEquipmentHandler.cs:52 - S2F31 收到 Host 傳來的時間值後，直接回傳 TIACK=0x00（成功），但完全沒有解析 m.Root 中的時間數據或更新設備內部時鐘。SEMI E30 規範要求設備應實際設定其系統時間。目前實作形同虛應，違反 GEM 時間同步協定。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:52 | 實作 S2F31 時間解析邏輯：(1) 在 EquipmentModel 中添加可注入的時鐘源（ISystemClock 抽象或 DateTime offset）；(2) 從 m.Root 提取 YYYYMMDDHHmmss ASCII 或 Binary 時間值；(3) 解析並驗證時間格式；(4) 更新設備內部時鐘引用；(5) 僅在成功時回傳 TIACK=0x00，失敗時回傳非零值（如 0x01）。 | 新增單元測試：Send S2F31 with valid time string → 驗證設備的時鐘已更新；后续的 S2F17 應返回接近該時間的值。整合測試：透過 SimulatorBench 觸發 S2F31，檢查時間是否同步。 |
| 改善 | medium | S2F17 時間精度與時區不明確 | GemEquipmentHandler.cs:273 - Now() 傳回 DateTime.Now.ToString("yyyyMMddHHmmss")，精度僅 14 字元（秒級），無毫秒。且 DateTime.Now 為本地時間（local time），若設備跨時區運作會產生不確定性。SEMI E30 並未強制要求毫秒，但普遍業界期待時間同步時有亞秒精度（至少毫秒），且應明確使用 UTC 或約定時區。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:273 | 評估業界需求後，考慮：(1) 擴展時間格式至 YYYYMMDDHHmmss.fff（17 字元，毫秒級）；(2) 改用 DateTime.UtcNow 代替 DateTime.Now，並在文件說明設備使用 UTC；(3) 若需支援自訂時區或可測試時鐘，導入時鐘抽象層（TimeProvider .NET 8+ 或自訂 ISystemClock）；(4) 於 EquipmentModel 或 GemEquipmentHandler 建構式注入時鐘實例，便於單元測試控制時間。 | 在 EquipmentModel 添加可選的時鐘注入參數。編寫單元測試，注入虛擬時鐘，控制 S2F17 回傳特定時間值。驗證時間格式長度及內容合法性。 |
| 改善 | low | 缺少 S2F31 入力驗證與錯誤碼區分 | 當前 S2F31 實作沒有驗證收到的時間數據格式合法性（如非 14 字元字符串、包含非數字等），也未區分不同的失敗原因（格式錯誤、時間值越界、系統拒絕等）。SEMI E30 允許 TIACK 回傳不同碼值以表達失敗原因，但本實作一律回傳 0x00 或 0x01 沒有細化。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:52 | 在實作 S2F31 解析時，添加輸入驗證：(1) 檢查時間字符串長度（應為 14 或 17）；(2) 驗證字符皆為數字；(3) 檢查時間值合理性（月 01-12、日 01-31、時 00-23、分 00-59、秒 00-59）；(4) 定義並回傳明確的 TIACK 碼值：0x00=成功、0x01=格式錯誤、0x02=時間越界 等，便於 Host 診斷。文件化這些碼值供系統整合參考。 | 撰寫多個 S2F31 測試用例：有效時間、格式錯誤（9字元、包含字母）、越界時間（13月、32日等）。驗證回傳的 TIACK 碼值正確對應。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | medium | S2F33 ignores DATAID field - missing SEMI E30 compliance for report grouping | In GemEquipmentHandler.S2F33() at line 144, the DATAID (first element of S2F33 body) is extracted but never stored or validated. SEMI E30 spec mandates DATAID be echoed back in DRACK (S2F34) to correlate report definitions with their acknowledgment. Current implementation: var defs = m.Root is { Count: >= 2 } ? m.Root[1] : SecsItem.L() - immediately discards Root[0] (DATAID) without using it. S2F34 reply at line 160 returns hardcoded SecsItem.B(0x00) only, missing DATAID echo. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:141-161 | Modify S2F33 to extract DATAID from m.Root[0], store it, and modify the S2F34 response to echo it back: SecsItem.L(SecsItem.U4(dataid), SecsItem.B(0x00)) per SEMI E30 7.5 format. This allows host to correlate bulk report definitions. | Test with scenario that sends multiple S2F33 messages with different DATAIDs and verify each S2F34 echo contains the corresponding DATAID. Add assertion: S2F34.Root[0].GetU4() == dataid_sent. |
| 錯誤 | medium | S2F33 lacks error status code in DRACK when malformed message received | At line 145-160, S2F33 silently returns DRACK=0x00 (success) even when: (1) m.Root has fewer than 2 elements (malformed), causing defs to become empty list and triggering unexpected DeleteAllReports() at line 147. (2) Individual report definitions with bad format are silently skipped (line 153 continue). SEMI E30 GEM spec requires DRACK to return error codes (0x01-0xFF) for invalid message structure. Current code treats malformed input as 'delete all' scenario. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:141-161 | Add validation: if (m.Root?.Count < 2) return error DRACK code (e.g., 0x01 for invalid body structure). Log discrepancy between expected vs actual format. Only return 0x00 when full message is valid. See S2F15 (line 121) for similar pattern - validates m.Root.Format == L before processing. | Send malformed S2F33 with only 1 element (missing definition list) and verify DRACK error code is non-zero. Verify that previous reports remain unchanged after malformed message. |
| 改善 | low | S2F33 delete-all semantics lacks explicit test coverage | At line 145-148, when defs.Count == 0, DeleteAllReports() is called correctly per SEMI E30 spec (empty definition list = delete all reports). However, no test in GemFullFlowTests.cs or GemEquipmentTests.cs validates this edge case. Existing test at line 44-47 only tests normal define scenario. tests/SecsGem.Integration.Tests/GemFullFlowTests.cs:29-74 | Add integration test case 'Report_delete_all_via_empty_S2F33_list' that: (1) Defines multiple reports via S2F33, (2) Sends S2F33 with empty definition list (Root[0]=dataid, Root[1]=empty L), (3) Asserts all reports cleared via S1F12 query returning zero reports. | Run new test and confirm Reports.Count == 0 after empty S2F33. Verify S6F15 event trigger after delete-all returns empty reports list. |
| 改善 | low | Individual report delete semantics could be more explicit with test | Line 156 implements per-report delete correctly (vids.Count == 0 removes RPTID from Reports dict), but no dedicated test validates deleting one report while preserving others. Existing S2F33 test only covers define scenario with non-empty VID lists. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:151-158 | Add test case 'Report_selective_delete_via_empty_VID_list' that defines reports 1,2,3, then sends S2F33 to delete report 2 (with empty VID list), and verify only reports 1,3 remain. This exercises both define and selective-delete in one flow. | Test sends S2F33: [DATAID=1, [[RPTID=2, []]]]. Query via S1F12 should show reports 1,3 exist, report 2 deleted. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | S2F37 (Enable Event) accepts enable without linked reports—violates GEM sequential contract | GemEquipmentHandler.S2F37 (line 177-187) calls _model.EnableEvent(ceid, enable) unconditionally and replies ERACK=0 (success) even if the CEID has no reports linked via prior S2F35 call. When TriggerEventAsync is later called, BuildReports(ce) returns empty (line 258-264) because ce.LinkedReportIds is empty, resulting in S6F11 with empty report list. This violates SEMI E30 GEM state contract: enabling an event without report linkage is semantically invalid and breaks the collection pipeline. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:177-187, 235-242, 255-264 | Add precondition check in S2F37: before EnableEvent() call, verify that if enable==true, the CEID must have at least one linked report (ce.LinkedReportIds.Count > 0). If precondition fails, reply with ERACK!=0 (error code per SEMI E30, typically 0x01 for invalid parameter). Similarly, validate S2F35 that linked reports must be defined (exist in _model.Reports). | Unit test: send S2F37 enable for CEID without prior S2F35 linkage—should get ERACK!=0, not 0x00. Integration test (GemFullFlowTests.cs): after S2F37 without S2F35, trigger event and assert S6F11 is rejected or reports list is non-empty. Scenario test in 02-report-event.json: reverse order (enable before link) should fail. |
| 改善 | medium | S2F35 (Link Event) does not validate report existence—silent linkage to undefined reports | GemEquipmentHandler.S2F35 (line 164-174) calls _model.LinkEvent(ceid, rptids) for any RPTID in the link request without checking if those reports have been defined via prior S2F33. If a RPTID does not exist in _model.Reports, BuildReports() will silently skip it (line 260 continue), creating a hidden linkage contract violation: the host believes reports are linked and will receive them on trigger, but they won't appear if reports are not yet defined or are deleted. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:164-174, 255-264 | Validate S2F35: iterate RPTIDs in the link request and only link those that exist in _model.Reports. Either (a) reply LRACK!=0 if any RPTID is missing, or (b) link only the valid ones and reply LRACK=0 but log a warning. Option (a) is stricter; check SEMI E30 for guidance on partial link failures. | Unit test: S2F35 link a RPTID that doesn't exist in _model.Reports—verify either error reply or warning log. Integration test: define report 1, link CEID to [report 1, report 999], trigger event—verify S6F11 contains report 1 only or entire link is rejected. |
| 改善 | medium | No explicit enforcement of S2F33→S2F35→S2F37 sequence; reports can be deleted mid-chain | The GEM standard flow is S2F33 (define) → S2F35 (link) → S2F37 (enable). However, S2F33 can be called again (or DeleteAllReports at line 147) at any time, clearing all reports without invalidating or unlinking CEIDs. This means a CEID can remain enabled with broken report links. The test GemFullFlowTests.cs follows the correct order, but nothing prevents out-of-order or state-destroying calls. No lifecycle guard exists. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:141-161, 164-174, 177-187 | Consider adding state validation: (a) DeleteAllReports (S2F33 with empty list) should disable all CEIDs that were linked to those reports; or (b) before S2F37 enable, verify linked reports still exist; or (c) document this as an implicit contract that host must manage sequence. Option (a) is most robust. SEMI E30 may specify behavior on report deletion—verify spec. | Scenario test: (1) S2F33 define reports, S2F35 link, S2F37 enable, (2) S2F33 delete all reports, (3) trigger event—verify S6F11 is empty or rejected. Or: add assertion in TriggerEventAsync that reports exist before sending, throw/log if not. |
| 改善 | low | ERACK/LRACK/DRACK responses always 0x00 (success)—no error codes differentiation | S2F34 DRACK (line 160), S2F36 LRACK (line 173), and S2F38 ERACK (line 186) all hardcode SecsItem.B(0x00) for ACK value, with no conditional error codes. SEMI E30 defines error codes (e.g., 0x01 invalid parameter, 0x02 device offline) for these responses. Current implementation cannot signal validation failures gracefully. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:160, 173, 186 | Create a small enum for ACK codes (Success=0x00, InvalidParam=0x01, etc.) and modify S2F33/S2F35/S2F37 to return appropriate codes based on validation results. Update replies to use this enum instead of hardcoded 0x00. Helps downstream GEM implementations detect and handle errors. | Code inspection: check if S2F33/S2F35/S2F37 can conditionally return non-zero ACK in test. Look for SEMI E30 ACK code table and cross-reference implementation. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Non-thread-safe DATAID counter in concurrent event triggers | GemEquipmentHandler._dataId field (line 15) is incremented without synchronization in both S6F15/S6F16 (line 219) and TriggerEventAsync (line 240). When multiple concurrent events are triggered or S6F15 queries coincide with event triggers, DATAID values can be duplicated or out-of-sequence, violating SEMI E30 DATAID uniqueness guarantee. GemEquipmentHandler.cs:15, 219, 240 | Replace 'private uint _dataId' with 'private volatile uint _dataId' and use Interlocked.Increment to ensure atomic increments. Alternatively, use a proper thread-safe counter pattern: 'Interlocked.Increment(ref _dataId)'. | Run concurrent test: trigger multiple events simultaneously via TriggerEventAsync(). Verify each S6F11 message has a unique DATAID by capturing all messages and checking for duplicates. Use: new Task[]{Task.Run(()=>eq.TriggerEventAsync(101)), Task.Run(()=>eq.TriggerEventAsync(101))} with assertion that resulting DATAIDs differ. |
| 錯誤 | high | S2F35 LinkEvent silently ignores invalid CEID without error notification | GemEquipmentHandler.S2F35 (lines 164-173) calls _model.LinkEvent(ceid, rptids) but never checks the bool return value. If ceid doesn't exist in CollectionEvents, LinkEvent returns false and linking fails silently. Host receives LRACK=0 (success) even though the link operation failed, violating SEMI E30 semantics where invalid CEID should cause error handling. GemEquipmentHandler.cs:171 | Check LinkEvent return value and either: (a) return LRACK!=0 indicating link failure, or (b) pre-validate CEID existence before processing. Per SEMI E30, linking to undefined CEID should be detected. Example: 'if (!_model.LinkEvent(ceid, rptids)) sr.LinkResult = 1;' | Test scenario: send S2F35 linking reports to undefined CEID 9999. Verify equipment returns LRACK!=0 (error code). Check that EquipmentModel.CollectionEvents doesn't contain CEID 9999. Current behavior would accept the link without error. |
| 錯誤 | medium | Missing report definition causes silent data loss in S6F11 event reports | BuildReports method (line 260) skips undefined report IDs with 'continue', meaning if a CEID links to RPTID 5 but RPTID 5 wasn't defined via S2F33, the RPTID simply won't appear in the S6F11 body. This creates ambiguity: host cannot distinguish between 'RPTID 5 exists but has no values' vs 'RPTID 5 doesn't exist'. SEMI E30 expects all linked reports to be delivered in every event report. GemEquipmentHandler.cs:260 | Either: (a) prevent linking undefined report IDs by validating in S2F35, (b) always include linked RPTIDs in S6F11 even if undefined (send empty values list), or (c) add validation to reject S2F35 if any linked RPTID is undefined. Recommend option (a): in S2F35, filter NumericIdList(link[1]) to only include report IDs that exist in _model.Reports. | Test: S2F33 defines RPTID 1; S2F35 links CEID 101 to [RPTID 1, RPTID 999]; trigger CEID 101 → S6F11. Verify S6F11 contains both reports (with RPTID 999 having empty values if undefined), not just RPTID 1. Current code only includes RPTID 1. |
| 改善 | medium | Event report documentation lacks clarity on null value handling for missing variables | GemHostHandler.EventReportReceived event (line 16) documentation says '(CEID, reports 根項目)' but doesn't specify: when a variable ID in a report doesn't exist (GetVarValue returns null), the code substitutes SecsItem.L() (empty list). This silent substitution may mask configuration errors or missing status variables. The semantic meaning of an empty list vs. missing data needs clarification per SEMI E30. GemEquipmentHandler.cs:261, GemHostHandler.cs:15-16 | Clarify behavior: either (a) fail the event trigger if any VID is undefined (safest), (b) document that missing VIDs are reported as empty lists in S6F11, or (c) add a warning log when substituting null values. Also update EventReportReceived documentation to specify structure: reports is L[L[RPTID, L[values...]]...]. | Define CEID with linked report containing SVID 1 and undefined DVID 9999. Trigger event → S6F11. Check if S6F11 includes the undefined DVID 9999 (with SecsItem.L()). Verify EquipmentModel has no DVID 9999. Document behavior explicitly. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | S6F15 body structure non-compliant with SEMI E30 | SEMI E30 specifies S6F15 body as L[CEID, ...] (a list with CEID at index 0), but the implementation at GemEquipmentHandler.cs:217 treats m.Root directly as a scalar value: `var ceid = m.Root != null ? ToU4(m.Root) : 0u;`. This assumes m.Root is a single item, not a list. According to SEMI E30, S6F15 is always a list containing the requested CEIDs (possibly multiple). When a host sends S6F15 with the correct L[CEID] structure, the current ToU4() conversion will fail or return garbage. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:214-220 | Change S6F15 handler to parse m.Root as a list: extract CEID from m.Root[0] (or iterate if multiple CEIDs). The loop should handle multiple collection events if the host requests multiple CEIDs. Update: `var ceid = (m.Root?.Format == SecsFormat.L && m.Root.Count > 0) ? ToU4(m.Root[0]) : 0u;` | Write a unit test that sends S6F15 as SecsItem.L(SecsItem.U4(101)) and verify the equipment returns S6F16 with the correct CEID 101 in the response. Check that parsing fails gracefully (e.g., returns empty reports) if CEID is not found, rather than returning a garbage CEID. |
| 錯誤 | medium | S6F16 response missing optional JID/ALID fields for event-linked reports | SEMI E30 E30-0307 defines S6F16 response body as L[DATAID, CEID, L[RPTID, L[item]...], JID, ALID] where JID (Job ID) and ALID (Alarm ID) are optional sentinels that must be present (even if null) if the report data spans multiple items or if the event is job/alarm-related. The current BuildReports() at GemEquipmentHandler.cs:255-264 returns only L[RPTID, L[values]], omitting the JID and ALID sentinels. While these are optional in strict parsing, some hosts expect their presence for compliance-mode operation. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:255-264 and line 219 | Extend CollectionEvent class to optionally store JID and ALID. Modify BuildReports() to append JID and ALID items (or L() if not set) after the reports list. Update S6F16 reply structure from `[DATAID, CEID, reports]` to `[DATAID, CEID, reports, JID, ALID]` or add a parameter to control compliance mode. | Test with a SEMI-compliant parser or real equipment that validates the optional JID/ALID presence. Cross-check against SEMI E30-0307 message definition table for S6F16 mandatory vs. optional fields. |
| 錯誤 | medium | S6F15 handler does not validate CEID existence before sending S6F16 | At GemEquipmentHandler.cs:217-219, if a CEID is not found in CollectionEvents, BuildReports() is called on a missing entry, returning an empty list. The response S6F16 is sent with the requested (non-existent) CEID and empty reports. SEMI E30 specifies that equipment should acknowledge *all* requested CEIDs, but some implementations expect S9F1 (equipment error) if a CEID is unmapped or disabled. The current behavior silently returns empty, which may confuse hosts expecting explicit error signaling for invalid CEIDs. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:214-220 and EquipmentModel.cs:78 | Add an equipment validation option: (1) if CEID is invalid, either return S9F1 with a descriptive error, or (2) document and test the silent-empty-reports behavior as intentional fallback. Consider logging the CEID miss for debugging. If CEID is disabled (Enabled=false), also decide: return empty or error. Update logic to: `if (!_model.CollectionEvents.TryGetValue(ceid, out var ce)) { ce = null; } // then check ce.Enabled separately` | Send S6F15 with a CEID that does not exist in the model. Verify the response: (a) is S6F16 with empty reports, or (b) is S9F1 with error code. Document the chosen behavior in code comments. Write tests for both valid-CEID and invalid-CEID cases. |
| 改善 | low | S6F15/F16 loop does not batch multiple CEID requests | SEMI E30 allows S6F15 to request multiple CEIDs in a single message: L[CEID1, CEID2, ...]. The current implementation assumes a single CEID (m.Root directly). If a host sends S6F15 with multiple CEIDs, the equipment silently ignores all but the first (or treats the list as a scalar). This limits interoperability with hosts that batch requests for efficiency. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:214-220 | Refactor S6F15 to iterate over all CEIDs if m.Root is a list. Either: (a) send one S6F16 per CEID, or (b) accumulate all reports into a single S6F16 response with multiple report sections. SEMI E30 allows both; check your host's expectation. Likely: send separate S6F16 for each CEID (per typical equipment behavior), or send a single aggregated S6F16. Add a flag to EquipmentModel to control this behavior (SingleReportPerRequest vs. AggregatedReports). | Send S6F15 with multiple CEIDs: SecsItem.L(SecsItem.U4(101), SecsItem.U4(102)). Confirm the equipment returns S6F16 messages (one or aggregated) covering all requested CEIDs. Validate that no CEID is lost or silently skipped. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Missing S5F4 Alarm Clear Response Handler | SEMI E30 GEM 規範要求 EQP 必須可接收 S5F4（Host Alarm Clear Response）並根據其內容更新警報狀態。目前 GemEquipmentHandler.cs 僅實作 S5F3（Enable/Disable Alert），未實作 S5F4 的接收與處理。Host 對警報清除的確認訊息被忽視，導致 EQP 端警報狀態與 Host 端失同步。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs，第 38-64 行 switch case 中缺少 (5, 4) 分支；AlarmDefinition.cs 第 60 行 Set 屬性無防護機制 | 新增 S5F4 handler（S5F4Async）：解析 Host 送回的 ALCD+ALID，當 bit7=0（cleared）時，將 AlarmDefinition.Set 設為 false；並檢驗 ALID 與 ALCD 的一致性。在 SendAlarmAsync 後若期待 S5F4 應等候確認後再釋放 al.Set 狀態。 | 新增單元測試：Equipment 送 S5F1（set=true）→ Host 回 S5F4（cleared）→ Equipment 驗證 AlarmDefinition.Set=false；並在 GemFullFlowTests.cs 中加入雙向測試案例。 |
| 錯誤 | high | ALCD Code Mask 不符 SEMI E30 Bit 定義 | SEMI E30 規範定義：ALCD bit7=Set/Clear，bit[6:0]=Code。目前 SendAlarmAsync（行 249）使用 `al.Code & 0x7F` 清 bit7，但未驗證 al.Code ≤ 127。若 AlarmDefinition.Code 初始值 ≥ 128（例如 0x80），`set ? (al.Code | 0x80)` 會產生重複的 1 bits，且 clear 時 `al.Code & 0x7F` 會遺失原始的高 bits。這會導致不同 Code 值的警報在 set/clear 時互相干擾。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs 行 249；EquipmentModel.cs AlarmDefinition 類（行 54-61）無 Code 驗證 | 修正 ALCD 計算邏輯：`var alcd = (byte)((al.Code & 0x7F) | (set ? 0x80 : 0x00))`，確保先掩蓋 Code 的 bit7，再設定 set bit。並在 AlarmDefinition 建立時驗證 Code ≤ 127。 | 單元測試：驗證 SendAlarmAsync 對 Code=127, set=true 產生 ALCD=0xFF，Code=127, set=false 產生 ALCD=0x7F；測試 Code > 127 時是否拋出異常或被正確截斷。 |
| 改善 | medium | ALCD 解析缺少對稱驗證 | GemHostHandler.S5F1（行 62-73）僅單向解析 bit7 以判斷 set/clear，未驗證 ALCD 的 Code 部分是否與預期 ALID 對應的 AlarmDefinition.Code 一致。當 Host 或中間層錯誤映射警報 ID 時，缺乏檢測機制。 src/SecsGem.Core/Gem/GemHostHandler.cs 行 66-67 | 在 S5F1 解析後，若 GemHostHandler 擁有 AlarmDefinition 引用（需從 EquipmentModel 傳入），應驗證 `(alcd & 0x7F) == expectedCode`；若不符應記錄警告或拋出事件讓 UI 顯示異常。 | 新增整合測試：Host 收到 S5F1，驗證 ALCD 的 Code 部分與 Equipment 端定義一致；引入故意錯誤的 ALCD 值，確認驗證能偵測。 |
| 改善 | medium | AlarmDefinition.Set 狀態機不清晰 | 目前 AlarmDefinition.Set 在 SendAlarmAsync 被同步設置，但若網路遺失回覆或 S5F2 遲到，Set 狀態與 Host 端實際狀態失同步。沒有 pending/confirmed 的兩階段狀態管理。 src/SecsGem.Core/Gem/EquipmentModel.cs 行 60；GemEquipmentHandler.cs 行 248 | 考慮引入 SetPending/Confirmed 雙狀態或事件驅動：SendAlarmAsync 後等待 S5F2，僅在收到時才確認 Set 變更；同時錄記 SetChangedAt 時戳以支援逾時偵測。 | 新增場景測試：模擬網路延遲、S5F2 遺失、S5F4 衝突等邊界情況，驗證狀態一致性。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | S2F41 CPVAL 參數值遺漏：應遞迴提取並回傳 command parameters | GemEquipmentHandler.cs:190-198 的 S2F41() 方法只提取 CPNAME（command parameter name），但 SEMI E30 spec 要求 S2F42 回應中的 CPACK 結構應為 L[CPNAME, CPVAL]。目前實作在 line 196 產生 SecsItem.L(cp[0], SecsItem.B(0x00))，其中 0x00 為固定 CPACK code（成功），但完全忽略了 command parameter value (CPVAL)。CPVAL 應從 S2F41 request 的 cp 中至少包含 cp[1] 以保留參數值或者解析其值狀態。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:190-198 | 修改 S2F41() 以保留/提取 CPVAL。建議檢查 cp.Count，若 >= 2，應將 cp[1] 作為 CPVAL 納入 CPACK 回應結構，例如 cpacks.Add(SecsItem.L(cp[0], SecsItem.B(0x00), cp.Count >= 2 ? cp[1] : SecsItem.L()))；或者記錄命令及其參數狀態供後續處理。 | 在 GemFullFlowTests.cs 中添加測試案例：S2F41 包含多個 parameters，驗證 S2F42 回應中的 CPACK 陣列是否正確保留原始 CPVAL；或使用 SecsSessionTests.cs 中 line 68-69 的方式手工驗證回應結構。 |
| 錯誤 | high | S2F41 無狀態檢查：不驗證 OnlineRemote 或命令可執行性 | S2F41() 方法無任何前置條件檢查。SEMI E30 規範要求設備在接收 Host Command 時應驗證 ControlState 是否為 OnlineRemote（見 GemEquipmentHandler.cs line 106 S1F17 的 ControlState 設定邏輯）。目前實作直接回應 HCACK=0（成功），但未檢查設備控制狀態、通訊狀態或命令是否真實可執行，違反 GEM 事件驅動模型。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:190-198 | 在 S2F41() 開頭加入條件：(1) 檢查 _model.ControlState == GemControlState.OnlineRemote；(2) 檢查 _model.CommunicationState == GemCommunicationState.Communicating；若不符，應回傳適當的 HCACK code（例如 0x01 表示 invalid control state）。參考 S5F3 (line 201-211) 的狀態檢查模式。 | 添加集成測試：(a) 設備處於 Offline 狀態時發送 S2F41，驗證回應碼是否為非零；(b) 正確 Online 後再發送，驗證回應為 0x00。可在 GemFullFlowTests.cs 或 SecsSessionTests.cs 中實現。 |
| 錯誤 | medium | S2F42 HCACK 碼表未定義：沒有 error code 的詞彙表或異常處理 | S2F41() 在 line 197 硬編碼 HCACK=0x00（成功）。SEMI E30 spec 定義了多個 HCACK 狀態碼（例如 0x00=success, 0x01=invalid control state, 0x02=undefined command, 0x03=invalid parameters, 等），但代碼中無任何 HCACK enum 或命名常數，也無文檔記錄支援的碼值。這使得異常情況（命令未定義、參數無效等）無法適當回應。 src/SecsGem.Core/Gem/GemEquipmentHandler.cs:190-198, EquipmentModel.cs | 建議在 GEM namespace 中定義一個 HcackCode enum 或常數集合，列舉 SEMI E30 定義的所有 HCACK 碼及其含義。然後在 S2F41() 中按實際情況（如命令是否存在於預定義列表、參數型別檢查）回傳對應的碼。 | 檢查 SEMI E30 spec 文件 (GEM 規範)，對照實作中應支援的 HCACK 碼表；編寫分類測試驗證不同的 HCACK 回應場景（例如製造一個「未定義命令」案例，預期回應 0x02）。 |
| 改善 | low | 建議添加 Remote Command 命令存儲 (CommandDictionary)：支援命令定義與執行邏輯 | 目前設備模型（EquipmentModel.cs）只定義了狀態變數、資料變數、常數、事件、警報等，但沒有 Remote Command 的存儲結構。S2F41 的 RCMD 和後續 CPNAME 應對應到預定義的命令集合，每個命令應有其參數定義、驗證規則、執行邏輯。目前的實作無法區分合法命令和非法命令。 src/SecsGem.Core/Gem/EquipmentModel.cs | 在 EquipmentModel 中添加 Dictionary<string, RemoteCommand>（或類似結構），其中 RemoteCommand 包含：Cmd id、名稱、參數定義清單、驗證函式、執行委託。並在 S2F41() 中先查詢此字典，然後根據查詢結果和參數驗證決定 HCACK 碼。可參考 StatusVariables、EquipmentConstants 等既有的字典模式。 | 添加整合測試：(1) 發送定義過的命令，驗證正確回應；(2) 發送未定義的命令，驗證 HCACK=0x02；(3) 發送參數類型錯誤的命令，驗證 HCACK=0x03。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Missing Equipment-side S10F1 Terminal Send API | GemEquipmentHandler only implements S10F3 (receive) but lacks SendTerminalAsync() method analogous to SendAlarmAsync() and TriggerEventAsync(). SEMI E30 requires equipment to initiate terminal messages (S10F1) to the host. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:245-251 (SendAlarmAsync pattern) vs missing terminal sender | Implement public async Task<SecsMessage?> SendTerminalAsync(uint tid, string text) in GemEquipmentHandler, following the same pattern as SendAlarmAsync: validate model state (CommunicationState), build S10F1 body L[U4(tid), A(text)], and call _session.SendAndWaitAsync() for S10F2 reply. | Create unit test in GemEquipmentTests: send S10F3 from host, verify equipment replies with S10F4; then call eq.SendTerminalAsync(99, 'hello') and verify host's GemHostHandler.TerminalReceived event fires with tid=99, text='hello'. |
| 錯誤 | high | S10F3 handler lacks TEXT validation and error handling | GemEquipmentHandler.S10F3 (line 62) unconditionally replies with ACKC10=0x00, without checking: (1) message structure (Count >= 2, [0] is U4 TID, [1] is A TEXT), (2) TID validity, (3) TEXT length/format per SEMI E30 limits. Malformed requests always succeed, masking protocol violations. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:62 | Add pre-reply validation: check m.Root.Count >= 2 && m.Root[0] numeric && m.Root[1] ascii; log or track invalid requests; reply with ACKC10!=0 on malformed. Optionally store received TEXT in model for UI inspection or test assertions. | Test: send S10F3 with malformed body (empty, missing TEXT, TEXT as binary), verify replies are appropriate (0x00 on valid, non-zero on error); trace logs show validation messages. |
| 改善 | medium | EquipmentModel lacks Terminal state/storage | EquipmentModel has Alarms and Reports collections but no Terminal storage (TID namespace, received TEXT buffer, or TACK history). Cannot track or recall last received terminal message for UI/scenario assertion. src/SecsGem.Core/Gem/EquipmentModel.cs (entire class structure) | Add public ConcurrentDictionary<uint, string> TerminalMessages { get; } and populate on S10F3 receipt. Optionally add DateTime LastTerminalTime for tracking. Allows UI and scenario verifier to assert terminal state post-receipt. | After S10F3 S10F3 receipt in GemEquipmentHandler, add: _model.TerminalMessages[tid] = text. Then in test/scenario, read model.TerminalMessages[99] and assert == 'hello'. |
| 改善 | medium | No explicit support for S10 bidirectional flow in scenario schema | scenarios/01-03-*.json lack S10F1/S10F3 steps; scenario verifier cannot declaratively test equipment-to-host or host-to-equipment terminal exchanges. Verification gaps in headless CI. scenarios/ (all .json files) + src/SecsGem.Scenarios/ schema | Add scenario ops: { "op": "send-terminal", "tid": 99, "text": "..." } (equipment → host S10F1) and { "op": "expect-terminal", "tid": 99, "text": "..." } (host receives). Implement in SecsGem.Scenarios assertion engine. | Create 04-terminal.json scenario, run 'dotnet run --project src/SecsGem.Verify', verify HTML report shows passed S10F1/S10F3 traces with TID/TEXT assertions. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | SO_REUSEADDR not set on TcpListener - port binding race after rapid scenario completion | PassiveConnectionStrategy.cs:23 creates TcpListener without SO_REUSEADDR (ExclusiveAddressUse defaults to true on Windows). When scenarios complete and ports are released, OS transitions to TIME_WAIT state (typically 60s on Windows). Sequential scenario execution on same port risks 'Address already in use' WSAEADDRINUSE (10048) even after Disconnect/Stop, especially under headless rapid succession. src/SecsGem.Core/Hsms/PassiveConnectionStrategy.cs:23 | Set ExclusiveAddressUse=false on TcpListener before Start() to enable SO_REUSEADDR semantics. Change line 23-24 to: `_listener = new TcpListener(bindIp, _options.Port) { ExclusiveAddressUse = false }; _listener.Start();` | Run multiple rapid scenario executions with verify harness on 127.0.0.1. Monitor windows netstat for TIME_WAIT sockets. Confirm first scenario passes, second/third succeed without 'Address already in use' errors in trace. Regression: verify existing test suite (SecsSessionTests, GemFullFlowTests) still passes. |
| 改善 | high | Sequential scenario execution blocks - enable concurrent multi-port orchestration | Program.cs:52-55 executes scenarios serially with `foreach`+`await`. Each scenario pins one port, blocking the next. With 10+ scenarios on basePort 6000, total runtime is sum not concurrent. Headless CI/CD harness gains nothing from loopback parallelism (OS can handle 127.0.0.1 multiplexing). Current: `foreach (_, scenario) in scenarios) await runner.RunAsync(scenario, port++)` takes T1+T2+...+Tn seconds. src/SecsGem.Verify/Program.cs:52-55 | Parallelize scenario execution: replace foreach+await with `var tasks = scenarios.Select((s, i) => runner.RunAsync(s.Scenario, basePort + i)).ToList(); var results = await Task.WhenAll(tasks);`. Each scenario gets dedicated port (6000, 6001, 6002...) concurrently. Verify port allocation doesn't overflow ephemeral range. | Run verify harness with 5-10 scenarios. Measure elapsed time: serial should be Σ(scenario_i.ElapsedMs), parallel should ≈ max(scenario_i.ElapsedMs) + overhead. Confirm all scenarios PASS in HTML report. Check netstat while running: should see 5-10 concurrent 127.0.0.1 ESTABLISHED pairs. Regression: all tests still pass. |
| 錯誤 | medium | Port increment overflow risk - no bounds check on basePort allocation | Program.cs:50 initializes `port = basePort` (default 6000), then `port++` per scenario (line 55). No validation that port + scenario_count < 65536. If basePort=65535 and 2+ scenarios run, second scenario gets port=65536 (invalid). ScenarioRunner.Options() silently passes invalid port to HsmsOptions without validation. src/SecsGem.Verify/Program.cs:50-55 | Validate port range before loop. Add after line 50: `if (basePort + scenarios.Count > 65535) { Console.Error.WriteLine($"Port range overflow: {basePort} + {scenarios.Count} scenarios > 65535"); return 3; }` | Test with --port 65530 and 10 scenarios - should error with clear message. Test with --port 6000 and 100 scenarios - should error. Test with --port 6000 and 5 scenarios - should succeed. Verify HTML report shows all 5 PASS. |
| 改善 | medium | Cleanup order not optimal - passive listener Stop() called inside Finally before DisposeAsync() | ScenarioRunner.cs:89-95 calls Disconnect() then DisposeAsync(). PassiveConnectionStrategy.EstablishAsync() stops listener in finally (line 34), then DisposeAsync() calls Stop() again (line 40 - idempotent but redundant). More critically, if eqp.Connect() spawns listener but host.Connect() fails during HSMS handshake, listener may linger. No timeout guard on AcceptTcpClientAsync. src/SecsGem.Core/Hsms/PassiveConnectionStrategy.cs:16-35 | Add CancellationToken with timeout to AcceptTcpClientAsync guard. Modify EstablishAsync: use `using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); cts.CancelAfter(TimeSpan.FromSeconds(_options.T5Seconds)); await _listener.AcceptTcpClientAsync(cts.Token);` to prevent indefinite listener hang if active side fails to connect within T5. | Inject failure in ActiveConnectionStrategy.EstablishAsync (e.g., wrong port). Run scenario. Verify listener closes within T5 (5s default in ScenarioRunner.Options), not indefinite. Monitor netstat for lingering LISTENING sockets. Verify next scenario on same port succeeds. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Hex string parsing fails on odd-length input in SmlJson.Bytes() | File: src/SecsGem.Scenarios/SmlJson.cs:51. The Bytes() method calls `Convert.ToByte(hex.Substring(i * 2, 2), 16)` without validating hex string length. An odd-length hex string (e.g., `"123"` instead of `"1234"`) causes IndexOutOfRangeException when the loop tries to extract the last 2-character substring beyond the string bounds. SmlJson.cs:45-57 | Validate hex string length before processing: add `if (hex.Length % 2 != 0) throw new FormatException("Hex string must have even length");` after line 49. Alternatively, use `Enumerable.Range(0, hex.Length / 2).Select(...)` to avoid index-based iteration. | Create a test scenario with odd-length hex in a B (Binary) field value, e.g., `{ "B": "12E" }` in step body, and run dotnet run --project src/SecsGem.Verify; should catch FormatException with clear message instead of crashing. |
| 改善 | medium | Inconsistent JSON property naming in AssertDef across scenario files | File: scenarios/03-alarm-ec-command.json:16 uses lowercase `u4` in assertion, while ScenarioModels.cs:113 defines property as `U4`. Although JsonSerializerOptions.PropertyNameCaseInsensitive=true makes this work, scenarios 01 and 02 consistently use proper casing. Scenario 03 deviates, creating maintenance burden and potential confusion. scenarios/03-alarm-ec-command.json:16 vs. src/SecsGem.Scenarios/ScenarioModels.cs:113 | Standardize all AssertDef properties in JSON to match C# casing: use `"U4"`, `"Ascii"`, `"Bin"` consistently across all scenario files. Update scenario 03 line 16: `"u4": 250` → `"U4": 250`. Document expected naming in a SCENARIOS.md schema guide. | Run dotnet run --project src/SecsGem.Verify --scenarios scenarios and verify all scenarios pass. Inspect generated HTML report for any assertion details, then do a case-sensitive grep for all AssertDef properties in *.json to confirm naming consistency. |
| 錯誤 | high | SmlJson.Bytes() crashes on malformed hex input without robust error recovery | SmlJson.Bytes() (line 45-57) attempts hex string parsing via substring indexing without bounds checking. If deserialization accidentally receives a string like `"1"`, `"AB3"`, or other non-even-length values, the conversion fails with cryptic IndexOutOfRangeException rather than a SECS-compliant validation error. No pre-validation or try-catch wrapper exists in SmlJson.ToItem() (line 13-40). SmlJson.cs:13-57 | Wrap hex parsing in try-catch and provide domain-specific error: after line 49, add `if (hex.Length % 2 != 0) throw new FormatException($"Invalid SECS-II B (Binary) hex string: length {hex.Length} is odd. Expected even-length hex string for bytes (e.g. '01FF' not '1FF')");` Also, consider using `Convert.FromHexString(hex)` (available in .NET 5+) if target framework permits—it handles validation automatically. | Manually craft a scenario step with malformed B value: `{ "B": "F" }`, run verify harness, catch FormatException (not IndexOutOfRangeException), and confirm error message mentions 'odd length' or similar clear guidance. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | ASCII (J format) and floating-point formats (F4/F8) lack assertion support | ScenarioRunner.cs lines 180-195 (RunAsserts) only supports 3 assertion properties: Ascii (A format only), U4 (with coercion), and Bin (binary). The AssertDef class (ScenarioModels.cs lines 108-115) has no fields for asserting J (JIS-8), F4, or F8 values. Since SmlJson.cs supports F4/F8 parsing and SecsItem supports GetF4s()/GetF8s() accessors (SecsItem.cs lines 119-122), scenarios cannot validate floating-point message bodies or JIS-8 encoded text. This is a real gap for SEMI E5 compliance where temperature, voltage, and other analog measurements are critical. src/SecsGem.Scenarios/ScenarioRunner.cs:180-195 and src/SecsGem.Scenarios/ScenarioModels.cs:108-115 | Add float4, float8, and jis8 assertion properties to AssertDef. Extend RunAsserts() to call node.GetF4s()[0], node.GetF8s()[0], and a GetJis8() method (or GetAscii() on J format). Validate with scenario: send S1F3 with temperature SVID returning F4 value, assert float4: 25.5. | Run scenario with F4/F8 assertions: { "path": "0.1", "float4": 25.5 } against temperature response, verify assertion passes/fails correctly. Test J format similarly with JIS-8 text. |
| 錯誤 | high | Boolean (U1, I1, I2, I4, I8, U2, U8) array elements cannot be asserted individually | ToU4() helper (ScenarioRunner.cs lines 229-241) only supports coercing Boolean format by returning 0u (line 240). The SECS-II codec (SecsItem.cs) provides GetBooleans() as an array, but the assertion framework provides no way to compare individual boolean array elements or the entire array. For example, S5F1 (alarm report) contains a 1-byte boolean indicating alarm state (on=1, off=0), but a path like "path": "0" cannot assert boolean values. The bin property works around this by checking first byte, but semantically this is type-unsafe and unclear. src/SecsGem.Scenarios/ScenarioRunner.cs:229-241 (ToU4) and 189-191 (RunAsserts) | Add a bool assertion property to AssertDef, and update ToU4()/RunAsserts() to handle SecsFormat.Boolean. Example: { "path": "0", "bool": true }. In scenarios, use this for alarm state assertions instead of brittle bin checks. | Create scenario: send S5F1 (alarm), assert { "path": "0", "bool": true } for the alarm state field. Verify both true and false boolean assertions pass/fail correctly. |
| 錯誤 | medium | Binary (B) format arrays lack granular assertion; only first byte checked | AssertDef.Bin (int? in line 114 of ScenarioModels.cs) and RunAsserts line 191 only verify node.GetBinary()[0] == a.Bin. This cannot assert multi-byte binary sequences or array lengths. For example, equipment model serial numbers or checksums stored as binary cannot be fully validated. A scenario asserting equipment serial ={"B": "AA BB CC"} response has no way to validate all 3 bytes. src/SecsGem.Scenarios/ScenarioRunner.cs:191 and ScenarioModels.cs:114 | Extend AssertDef with a binary property (string, hex-encoded, e.g., "AA BB CC" or "AABBCC"). Update RunAsserts() to compare full GetBinary() result. Reuse SmlJson.Bytes() hex parsing logic. | Scenario: send message with binary response, assert { "path": "0", "binary": "01 FF 80" }, verify assertion fails if any byte differs. |
| 改善 | medium | Path navigation does not report which index was invalid; unhelpful error for nested structures | Navigate() at ScenarioRunner.cs lines 197-207 returns null if any index in the path is out of bounds or format is not L, but sr.Detail only says "ASSERT 路徑 ... 不存在". For a deep path like "1.2.3.0", the user cannot see whether index 1 failed, or 2, or 3. In large nested messages (e.g., multi-report events), this makes debugging slow. src/SecsGem.Scenarios/ScenarioRunner.cs:187-188 | Modify Navigate() to track which segment failed and return (node, failedAt) tuple or a detailed error string. Update sr.Detail to say e.g. "ASSERT 路徑 1.2.3.0：在段 2 處停滯（非 List 或索引超範圍）". | Scenario with invalid nested path, check that error message pinpoints the failing segment index. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | 缺少 T3 超時負向場景 | ScenarioRunner.ExecuteStepAsync() 在 case "send" 中（行 123）呼叫 host.SendAndWaitAsync(msg, Cts()) 等待回覆，使用 T3 超時（4000ms，由 StepReplyTimeoutMs 定義，行 36）。然而現有的 3 個場景檔案（01-connect-status.json, 02-report-event.json, 03-alarm-ec-command.json）均未涵蓋 T3 逾時失敗案例（例如 Host 發送 primary 訊息但 EQP 無回應）。只有集成測試中的 SecsSessionTests.cs 行 36-45 「SendAndWait_times_out_when_no_reply_T3」測試此功能，但場景套件並未提供此用例。 scenarios/ 及 src/SecsGem.Scenarios/ScenarioRunner.cs:123 | 新增場景檔案 04-timeout-t3-no-response.json，包含：(1) Host 送 S1F3 但 EQP 不回覆，預期 TimeoutException；(2) 驗證超時後連線狀態正確。修改 ScenarioRunner 支援 "send-expect-timeout" Op 或在 RunAsserts 中檢查例外拋出。參考 SEMI E37 表 2 T3 定義。 | 執行 dotnet run --project src/SecsGem.Verify -- --scenarios scenarios/04-timeout-t3-no-response.json，確認場景正確拋出 TimeoutException 並正確記錄在報告中。 |
| 錯誤 | high | 缺少連線失敗/拒絕負向場景 | ScenarioRunner 初始化時（行 60-65）等待 Host 和 EQP 都進入 Selected 狀態，逾時則回傳 "連線失敗"。然而現有場景均假設連線成功。未測試的失敗模式包括：(1) Active 端（Host）收到 Select.rsp(1) 忙碌後重試失敗；(2) Select.rsp(2+) 被拒絕；(3) T6/T7 超時導致連線斷開。HsmsConnectionStateMachine 行 125-147 實作了 Select.rsp 處理邏輯，包括 busy 重試上限（SelectBusyMaxRetry=3）和拒絕狀態，但無場景測試。 src/SecsGem.Scenarios/ScenarioRunner.cs:60-65 及 src/SecsGem.Core/Hsms/HsmsConnectionStateMachine.cs:125-147 | 新增場景 05-select-busy-retry.json 和 06-select-reject.json：(1) Select busy 超過重試次數，驗證連線失敗邏輯；(2) Select 被拒 status=2，驗證正確退出。可在場景結構中擴展 Equipment 層級 connectionBehavior 欄位（如 rejectSelectWithStatus: 2），由 ScenarioRunner 在初始化時注入行為。或使用獨立 Listener 模擬此行為。 | 執行場景，確認 ScenarioResult.Error 包含 "連線失敗" 且 Ok=false；驗證 Trace 中含有 Select.rsp 拒絕或 busy 日誌。 |
| 改善 | medium | 缺少格式錯誤/S9F7 異常回覆負向場景 | GemFullFlowTests 行 127-145 已有 "Malformed_body_triggers_S9F7" 集成測試，驗證格式碼不合法時 EQP 自動回 S9F7。然而場景套件未涵蓋此負向流程——無法測試 Host 在集成環境中收到 S9F7 並正確處理。場景執行器目前不支持此類異常注入與驗證。 src/SecsGem.Scenarios/ScenarioRunner.cs:174-176 及 GemFullFlowTests.cs:127-145 | 擴展 StepDef 新增 "send-malformed" Op 或在 Body 層級支援 invalid 標記，讓場景可注入非法 format code；新增 "expect-s9f7" Op 驗證 Host 收到錯誤回覆。參考現有 GemFullFlowTests 中手工構造 HsmsMessageBuilder.BuildDataFrame() 的方式，整合至場景 JSON。 | 新增場景 07-malformed-message-s9f7.json，驗證 Host 收到 S9F7 錯誤訊息並正確斷言。 |
| 改善 | medium | 場景套件缺少邊界與約束檢驗場景 | 現有 3 個場景為主流正向路徑（連線、報表、EC、警報、命令）。SEMI E30/E37 規範還涵蓋多項邊界與約束，目前無場景測試：(1) 設備常數 EC 範圍驗證（Min/Max，行 19-22 定義但未在場景中測試越界情況）；(2) 無效 RPTID/CEID/ECID/ALID（例如不存在的 CEID 123）；(3) 多重報表定義衝突；(4) 未授權訊息（S1F15 Offline 後收到 Host 命令）；(5) 離線/離線狀態轉換。 scenarios/ 及 src/SecsGem.Core/Gem/EquipmentModel.cs | 新增場景 08-ec-bounds-validation.json, 09-invalid-ids.json, 10-unauthorized-offline.json 等，針對邊界與狀態約束驗證。參考 SEMI E30 Part 1 Section 11（變數/常數範圍）與 Section 12（狀態轉換圖）。 | 執行新場景，驗證 EQP 正確拒絕越界/無效操作（例如 EAC 非 0x00），且場景步驟 Ok=false 時詳細記錄。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | CancellationTokenSource leak in ScenarioRunner.Cts() | ScenarioRunner.cs:243 creates a new CancellationTokenSource(StepReplyTimeoutMs) and returns only its Token, but the source itself is never disposed. This leaks resources per send/reply step. Called repeatedly at line 123: 'var reply = await host.SendAndWaitAsync(msg, Cts());' src/SecsGem.Scenarios/ScenarioRunner.cs:243 and 123 | Create and track CancellationTokenSource instances. Either: (1) Store the source in a list and dispose in finally, (2) Refactor SendAndWaitAsync to accept a timeout duration instead of a token, allowing internal CTS creation+cleanup, or (3) Use a static disposable CTS for the harness lifetime. | Run 'dotnet test' on GemFullFlowTests and monitor for CancellationTokenSource finalizer warnings or Task exceptions; check handle count before/after scenario runs using Performance Monitor or native tools. |
| 錯誤 | high | HsmsConnection event channel writer never completed | HsmsConnection.cs creates an unbounded Channel but never calls _eventChannel.Writer.TryComplete(). When DisposeAsync() is called, the channel reader (RunEventLoopAsync) can hang indefinitely waiting for more events, especially if no event is posted after cancellation token fires. The PostEvent method (line 321) only does TryWrite, which succeeds even after completion. src/SecsGem.Core/Hsms/HsmsConnection.cs:55, 321, and missing completion in DisposeAsync (line 323) | Add _eventChannel.Writer.TryComplete() before awaiting _eventLoopTask in DisposeAsync. Ensure proper channel closure to unblock the reader loop. Consider: if (!_eventChannel.Writer.TryComplete()) before the WhenAny await at line 328. | Add delay before DisposeAsync in ScenarioRunner.finally and inspect HsmsConnection lifetimes in unit test. Use xunit TestTimeout or set timeout on test class to catch hangs. Run stress tests with many scenario iterations. |
| 錯誤 | medium | Race condition in delayCts disposal and _pending cleanup timing | SecsSession.SendAndWaitAsync (lines 60-81): delayCts is disposed via using (line 70) immediately after Task.WhenAny returns. If the reply arrives just as the timeout task fires, the timing between 'tcs.Task result' being set and delayCts.Cancel() at line 74 can cause: (1) the delayCts.Dispose() at line 70 end-using to race with Task.Delay continuation, or (2) _pending.TryRemove at line 79 can complete before the TCS is awaited at line 75 if cancellation interleaves. Additionally, if ct is already cancelled, the delayCts.CreateLinkedTokenSource may inherit a cancelled token, causing Task.Delay to throw immediately. src/SecsGem.Core/Session/SecsSession.cs:60-81, especially lines 70-75 | Restructure to eliminate the race: (1) Avoid cancelling delayCts at all—rely on the Task.Delay cancellation token being linked. (2) Await the TCS.Task before exiting the try block so cleanup order is deterministic. (3) Consider moving the T3 timeout management to HsmsConnection level instead of per-SendAndWaitAsync. (4) Ensure ct.IsCancellationRequested is checked before entering the delayCts creation block. | Run GemFullFlowTests.cs tests 100+ times with Chaos.xUnit or random delays injected via reflection. Monitor for TimeoutExceptions or ObjectDisposedExceptions. Add logging to trace exact order of delayCts disposal vs tcs.Task completion. |
| 錯誤 | medium | Potential null reference in SendBytesAsync during concurrent Disconnect/CloseConnection | HsmsConnection.SendBytesAsync (line 225) reads _networkStream into a local variable (line 227), then checks if null. However, CloseConnection (line 309-318) sets _networkStream = null outside any lock. Between the null check and stream.WriteAsync, _networkStream can be disposed by a concurrent CloseConnection() call (e.g., during Disconnect or state machine transition). The null check at line 228 only guards against null, not use-after-dispose of a stream disposed by another thread. src/SecsGem.Core/Hsms/HsmsConnection.cs:225-243 and CloseConnection at 309 | Wrap the stream.WriteAsync in a try-catch for ObjectDisposedException. Or: acquire _sendLock before reading _networkStream to ensure atomic read+write. Better: use a volatile reference to stream and double-check after acquiring the lock. Add a disposed flag to HsmsConnection and check it before SendBytesAsync returns. | Inject rapid Disconnect() calls while messages are being sent (stress test in a loop). Monitor for ObjectDisposedException or silently swallowed errors in SendBytesAsync. Check that error logging at line 237 catches the exception. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | ScenarioRunner sequential execution lacks explicit parallelization protection on scenarios | Program.cs:52-64 uses a sequential foreach loop to run scenarios, with each scenario consuming a unique port. However, ScenarioRunner.RunAsync (ScenarioRunner.cs:38-98) launches EQP/Host sessions with Task.Run internally in HsmsConnection (HsmsConnection.cs:82) without isolating resource cleanup between scenarios. If future code or xunit-managed concurrency parallelizes scenario execution, the event loop tasks spawned by Connect() could cause port binding conflicts and race conditions on the threadpool. src/SecsGem.Verify/Program.cs:52-64; src/SecsGem.Scenarios/ScenarioRunner.cs:38-98 | Document the requirement that ScenarioRunner.RunAsync must not be called concurrently, or add explicit serialization via a SemaphoreSlim(1) wrapping the runner calls in Program.cs. Alternatively, implement collection-level xunit CollectionDefinition to declare scenarios as non-parallelizable (similar to AssemblyInfo.cs:5 in tests). | Run: dotnet test tests/ with XUNIT_CPUCOUNT=4 and verify no intermittent timeouts. Or add a test that spawns 4 scenarios concurrently and observe port binding errors. |
| 改善 | medium | DisposeAsync invoked before all background Task.Run completions are guaranteed | ScenarioRunner.cs:94-95 calls Disconnect() then DisposeAsync() sequentially, but HsmsConnection (HsmsConnection.cs:82, 163) spawns _eventLoopTask and _receiveLoopTask via Task.Run without Task.Wait during teardown. HsmsConnection.DisposeAsync() (HsmsConnection.cs:328) uses Task.WhenAny with DisposeTimeoutMs=3000, which may race if the event loop is processing timer continuations or state transitions. Under high CPU load (parallel scenarios), the timeout may expire prematurely, leaving dangling event-loop tasks. src/SecsGem.Scenarios/ScenarioRunner.cs:89-96; src/SecsGem.Core/Hsms/HsmsConnection.cs:323-336 | Increase DisposeTimeoutMs to 5000 (or make it configurable), or add explicit wait for _eventLoopTask.IsCompleted before returning from DisposeAsync. Alternatively, ensure Disconnect() is awaited before DisposeAsync(): change line 91 to `await Task.Run(() => eqp.Disconnect())` to drain the disconnect event. | Run 10 consecutive scenarios with heavy threadpool load (ulimit -n tests or concurrent test stress), then check for lingering sockets with `netstat -an` or valgrind/ETW tracing. |
| 改善 | medium | Task.Delay hardcoded between Disconnect and DisposeAsync is timing-sensitive | ScenarioRunner.cs:93 uses `await Task.Delay(100)` as a grace period between Disconnect() and DisposeAsync(). This is insufficient under threadpool starvation (visible in TestSupport.cs:14 comment: 'starvation' problem). The 100ms may elapse while the event loop is still processing the disconnect event, causing DisposeAsync() to race on _eventLoopTask cleanup. src/SecsGem.Scenarios/ScenarioRunner.cs:93; src/SecsGem.Integration.Tests/TestSupport.cs:14 | Replace the hardcoded 100ms with a polling loop: `await TestSupport.WaitForAsync(() => eqp.State == HsmsState.NotConnected && host.State == HsmsState.NotConnected, 3000)` before DisposeAsync, or apply the same threadpool minimum-threads pattern from TestSupport.InitThreadPool() to ScenarioRunner initialization. | Add a test that runs ScenarioRunner 20 times in a loop and monitor ThreadPool thread count before/after. If threads don't stabilize at ~64, the 100ms delay is too tight. |
| 改善 | low | Program.cs lacks graceful shutdown of runner and port allocation state | Program.cs:48-64 creates a single ScenarioRunner and reuses it across all scenarios. If an exception occurs during scenario execution or reporting, there is no cleanup of the runner state or reservation of the port counter (_nextPort in TestSupport.cs:64 is module-level and static, shared with all tests). This could cause port collisions if tests are retried or re-run in the same process. src/SecsGem.Verify/Program.cs:48-64; tests/SecsGem.Integration.Tests/TestSupport.cs:64-67 | Wrap the scenario execution loop in a try-finally and add logging for port cleanup. Optionally reset _nextPort or introduce a local port registry per runner instance to avoid cross-test pollution. | Run: dotnet run --project src/SecsGem.Verify -- --scenarios scenarios 2>&1 | grep 'port' and verify each scenario uses a distinct port. Then interrupt and re-run; check that port numbers resume from basePort without collision. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | Unsynchronized property reads in SimulatorBench cause race conditions with UI updates | Properties like EqpState (line 35), HostState (line 36), ControlState (line 37), and CommState (line 38) in SimulatorBench.cs read from mutable reference types (_eqp, _host, _model) without synchronization. These reads happen concurrently: (1) Worker threads in HsmsConnection continuously update _eqp.State and _host.State via the event loop (HsmsConnectionStateMachine.TransitionTo), and (2) UI render thread in Home.razor reads these properties during StateHasChanged. The reads of _eqp and _host object references themselves are not null-safe under concurrent reassignment. Additionally, EquipmentModel.ControlState and .CommunicationState (lines 72-73) are simple auto-properties without volatile/atomic semantics, yet GemEquipmentHandler modifies them from async session handlers (S1F13/S1F15/S1F17 at lines 75, 99, 106 in GemEquipmentHandler.cs). Since these properties use plain field backing without volatile, reads in UI may observe stale values. src/SecsGem.Web/SimulatorBench.cs:35-38 and src/SecsGem.Core/Gem/EquipmentModel.cs:72-73 | Synchronize property reads or use volatile semantics: (1) Mark EquipmentModel.ControlState and .CommunicationState as volatile fields. (2) Wrap SimulatorBench property getters (EqpState, HostState, ControlState, CommState) in a lock or use Interlocked reads for null-safety. (3) Alternatively, snapshot the entire state in a thread-safe immutable record and return that from UI render, avoiding multiple reads. Example: `public string EqpState { get { lock(_lock) return _eqp?.State.ToString() ?? "-"; } }` | Run the simulator under heavy load: spawn multiple concurrent tasks that call TraceSnapshot() and invoke multiple Send/TriggerEvent/FireAlarm buttons rapidly while watching for 'null reference' exceptions or stale state display. Use ThreadSanitizer (if available) or add intrinsic assertions: store reads to local variables and assert no null dereference mid-property-read sequence. |
| 錯誤 | medium | EquipmentModel shared state modifications lack concurrency protection for composite operations | Methods like LinkEvent() (lines 100-106), EnableEvent() (lines 108-113), and EnableAllEvents() (lines 115-118) in EquipmentModel.cs perform TOCTOU (time-of-check-to-time-of-use) sequences: TryGetValue → read item → call Clear() → call AddRange(). These are NOT atomic. If another thread accesses CollectionEvents or modifies ce.LinkedReportIds between the TryGetValue and the Clear() call, the update is corrupted. For example, S2F35 (Link Event Report, line 164 in GemEquipmentHandler.cs) and S2F37 (Enable Event, line 177) both invoke these methods from async task context, potentially racing with TriggerEventAsync (line 237) which reads LinkedReportIds (line 258) and BuildReports (line 255). No synchronization ensures the report list is stable during iteration. src/SecsGem.Core/Gem/EquipmentModel.cs:100-118 and src/SecsGem.Core/Gem/GemEquipmentHandler.cs:164-172 | Introduce a lock (e.g., private readonly object _modelLock) for non-ConcurrentDictionary operations on EquipmentModel. Wrap composite read-modify sequences (TryGetValue + Clear + AddRange) and iteration over LinkedReportIds inside the lock. Example: `public bool LinkEvent(uint ceid, IEnumerable<uint> rptids) { lock(_modelLock) { ... } }`. Alternatively, make LinkedReportIds a ConcurrentBag or use lock-free patterns (atomic snapshot + replace). | Write a stress test: spawn 5+ tasks that concurrently call S2F35 (LinkEvent) and TriggerEventAsync (BuildReports iteration). Assert that BuildReports never skips or duplicates linked report IDs. Run with Task Parallel Library stress patterns or Concurrency Visualizer to detect lock contention. |
| 改善 | medium | SimulatorBench properties should defend against object reference races during startup/disposal | In SimulatorBench.cs, _eqp and _host are assigned in StartAsync() (lines 49-51) and read in property getters (lines 35-36). The reads happen from UI thread, writes from the async caller thread. Although .NET object reference reads are atomic, the pattern is fragile: if StartAsync() is called concurrently with property reads, one thread might see _eqp=null while another starts initialization. Additionally, DisposeAsync() (lines 141-152) can race with concurrent button invocations (Send, TriggerEvent, etc.) because there is no synchronization preventing calls to _host.SendAsync() after _host has been disposed. The try-catch in Send() only masks the error. src/SecsGem.Web/SimulatorBench.cs:43-66, 90-100, 141-152 | Add a _startupLock (SemaphoreSlim or object) to serialize StartAsync and property access: use lock or await _startupLock inside StartAsync, and take it (non-blocking) in property getters to snapshot state. For disposal safety, set _eqp and _host to null after disposal and check null in Send/TriggerEvent before use: `if (_host is null) { AddInfo("已關閉"); return; }`. Alternatively, use a disposed flag (_isDisposed) and throw ObjectDisposedException to fail fast. | Rapid-fire click the start button multiple times while the UI is rendering. Verify no 'Object reference not set' exceptions appear in logs. Call concurrent Send() and StartAsync() from multiple tasks and confirm graceful degradation (logged message rather than crash). |
| 改善 | low | Event handler invocations from multiple threads may cause cascading UI redraws under contention | In SimulatorBench.cs, the MessageLogged event handler (line 54) and the OnChange event (line 113 Raise()) are invoked from both the HSMS worker thread (via _host.MessageLogged and Add()) and the UI event handler thread (via Raise() called from Send, TriggerEvent, etc.). Each Raise() invokes OnChange?.Invoke(), which calls Home.razor's OnBenchChanged → InvokeAsync(StateHasChanged). If Add() is called from two threads in rapid succession, OnChange can fire twice before the first StateHasChanged completes, causing redundant rendering. This is not a correctness bug but impacts UI responsiveness and trace display latency under high message rates. src/SecsGem.Web/SimulatorBench.cs:54, 102-113, 41 and Home.razor:84 | Coalesce UI update notifications: (1) Use a SemaphoreSlim or flag to ensure at most one StateHasChanged is pending at a time. (2) Alternatively, defer the OnChange invocation by queuing it on Blazor's SyncContext, ensuring updates serialize. (3) Or batch trace additions: instead of firing OnChange on every Add(), buffer traces and flush on a timer (e.g., 50ms). Example: `private bool _changeNotified; private void Raise() { if (!_changeNotified) { _changeNotified = true; OnChange?.Invoke(); } }` and reset _changeNotified in OnBenchChanged after StateHasChanged completes. | Monitor UI responsiveness during high-rate message flow (e.g., many S2F13 requests in quick succession). Count OnChange invocations vs actual distinct updates. Measure time between Raise() call and InvokeAsync completion using browser DevTools or Blazor debug trace. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | medium | Channel writer not completed in HsmsConnection.DisposeAsync | In HsmsConnection.DisposeAsync() (src/SecsGem.Core/Hsms/HsmsConnection.cs, line 323-336), the _eventChannel writer is never completed before awaiting the event loop exit. The loop reads via 'await foreach (..._eventChannel.Reader.ReadAllAsync(ct))' (line 112), which should be signaled via _eventChannel.Writer.Complete(). Without completing the writer, the async foreach may not gracefully exit if cancellation timing is tight, potentially causing the 3-second DisposeTimeoutMs to be consumed unnecessarily. src/SecsGem.Core/Hsms/HsmsConnection.cs:323-336 | In DisposeAsync(), call _eventChannel.Writer.Complete() immediately after _workerCts?.Cancel() to signal EOF to ReadAllAsync, ensuring the event loop exits promptly. This follows the Channel dispose pattern: signal completion before cancellation. Example: Insert '_eventChannel.Writer.TryComplete();' after line 326. | Run integration tests (SecsGem.Integration.Tests) with a debugger breakpoint in DisposeAsync to confirm event loop exits before the 3-second timeout under normal conditions. Use ETW tracing to observe Channel<T> completion semantics. |
| 錯誤 | low | SemaphoreSlim may be disposed while event loop still holds _sendLock | In HsmsConnection.DisposeAsync() (line 332), _sendLock.Dispose() is called after Task.WhenAny(..., Task.Delay(DisposeTimeoutMs)), which only waits up to 3 seconds for _eventLoopTask to complete—it does NOT guarantee the event loop has exited. If SendBytesAsync (line 225-242) is still holding the lock when DisposeAsync disposes it, ObjectDisposedException will occur on the next WaitAsync/Release attempt in the event loop (unlikely but possible in race conditions). src/SecsGem.Core/Hsms/HsmsConnection.cs:332 | Move _sendLock.Dispose() to *after* the full event loop await completes, or use a flag-based shutdown: set _isRunning=false and let SendBytesAsync check it before WaitAsync. Alternatively, use try-catch in SendBytesAsync to handle ObjectDisposedException gracefully during shutdown. | Run stress tests with rapid Connect/Disconnect cycles (e.g., 100 iterations in a loop). Monitor for ObjectDisposedException in the debug output. Use dotnet-trace to record lock contention and disposal order. |
| 改善 | low | Ephemeral CancellationTokenSource in ScenarioRunner.Cts() not explicitly disposed | In ScenarioRunner.ExecuteStepAsync (src/SecsGem.Scenarios/ScenarioRunner.cs, line 243), Cts() creates a new CancellationTokenSource(StepReplyTimeoutMs) for each step but never explicitly disposes it. While it is relatively short-lived (4 seconds) and GC will eventually clean it up, this pattern violates IAsyncDisposable best practices and can cause resource accumulation in long-running test suites with many steps. src/SecsGem.Scenarios/ScenarioRunner.cs:243 and 123 | Replace the pattern 'using var delayCts = CancellationTokenSource.CreateLinkedTokenSource(ct)' with a timeout-scoped version, or store the CTS in ExecuteStepAsync scope. For step-level timeouts, use 'using var stepCts = new CancellationTokenSource(StepReplyTimeoutMs); var token = stepCts.Token;' in each case block that needs it, ensuring disposal at step end. | Use dotnet-trace or perfview to profile handle/memory growth across a 100-step scenario run. Confirm CancellationTokenSource finalizer is not being called frequently (indicates premature GC pressure). |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | medium | Silent exception swallowing in async fire-and-forget handlers | GemEquipmentHandler.OnPrimary (line 29) and GemHostHandler.OnPrimary (line 30) use `_ = HandleAsync(m)` to invoke async handlers, catching all exceptions only via Debug.WriteLine in the catch block (lines 68, 46). These debug-only messages are invisible in Release builds and don't propagate to structured logging or UI. For production, silent exceptions in message handlers can cause undetected state corruption or incorrect GEM behavior. src/SecsGem.Core/Gem/GemEquipmentHandler.cs:29, 66-69; GemHostHandler.cs:30, 44-47 | Replace Debug.WriteLine with a proper logging event (e.g., `Handler handler error invoked here`) that can be hooked into Web UI or CLI logs. Consider adding a GemErrorOccurred event to each handler, or use a shared ILogger interface injected via constructor. | Inject a handler error event, trigger S2F41 with malformed body, verify error appears in UI trace log and/or CLI output, not just debug output. |
| 改善 | medium | No centralized ASP.NET logging configuration for protocol events | Program.cs does not wire SecsSession or HsmsConnection Log events to ASP.NET ILogger. The application uses appsettings.json (Logging config) but the Log event (line 40 of HsmsConnection) is only subscribed to in handlers or passed to UI. In production, protocol events (T8 timeout, send errors, connection failures) are not captured in the application's centralized log sink (console, file, Application Insights, etc.). This prevents operations teams from diagnosing production issues. src/SecsGem.Web/Program.cs:1-28; appsettings.json; HsmsConnection.cs:40 | Add `builder.Services.AddLogging()` in Program.cs and inject ILogger<SecsSession> into SimulatorBench. Subscribe HsmsConnection.Log and SecsSession.Log events to ILogger.LogInformation/Warning/Error, routing all protocol diagnostics to the configured logging provider. | Enable File or Console logging in appsettings.json, run Web UI, trigger a T8 timeout or send error, verify error message appears in both stdout and (if configured) .log file. |
| 改善 | low | Bare catch blocks without logging context | SimulatorBench.Send (line 98) catches all exceptions and only logs ex.Message; it does not log the original message (SxFy, direction, timestamp) that failed. Similarly, SimulatorBench.DisposeAsync (line 151) has a bare catch block with no logging. This makes debugging failures harder because you lose context about which operation failed. src/SecsGem.Web/SimulatorBench.cs:93-98, 141-152 | Log the message details (msg.Sxfy, msg.SystemBytes) alongside the exception. For DisposeAsync, at minimum log a warning with ex.Message (currently swallowed entirely). | Send a message that triggers a timeout exception (e.g., increase T3), verify the UI trace log shows both the message label and the error detail. |
| 改善 | low | Missing exception context in scenario runner step failures | ScenarioRunner.ExecuteStepAsync (line 75-79) catches exceptions and only stores `sr.Detail = $"例外：{ex.Message}"` without the exception type or stack trace. For complex scenarios, knowing the exception type (e.g., TimeoutException vs. InvalidDataException) helps distinguish timeout failures from codec errors. src/SecsGem.Scenarios/ScenarioRunner.cs:71-80 | Include ex.GetType().Name in the detail string: `sr.Detail = $"例外 {ex.GetType().Name}：{ex.Message}"`. Optionally capture inner exception chains if relevant to SECS-II codec failure diagnosis. | Write a scenario with a malformed message body, run Verify, check HTML report shows exception type (e.g., "例外 InvalidDataException：..."). |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 改善 | medium | SecsItem.Unpack() allocates intermediate byte arrays per element | In SecsItem.cs lines 124-136, the Unpack<T>() method allocates a new byte[size] for each element in the array. For large multi-element arrays (e.g., U4[1000]), this creates 1000 temporary byte[4] allocations. This pattern occurs in GetU2s(), GetU4s(), GetU8s(), GetI2s(), GetI4s(), GetI8s(), GetF4s(), GetF8s(). src\SecsGem.Core\Secs2\SecsItem.cs:129-133 | Use Span<byte> instead of allocating intermediate arrays. Refactor Unpack() to use `BinaryPrimitives.Read*` methods that accept a Span or ReadOnlySpan directly from the raw buffer offset: `read(_raw.AsSpan(i * size, size))` avoiding the BlockCopy+slice allocation pattern. | Profile decoding of large SECS-II messages with multi-element arrays (e.g., 10000-element U4 list) using dotnet-counters or ETW. Compare Gen2 allocations and total bytes allocated before/after refactoring. |
| 改善 | medium | SecsItem.Pack() allocates temporary byte array per scalar element | In SecsItem.cs lines 74-84, the Pack() factory method allocates a new byte[size] for each element to be written, then BlockCopy's it to the final raw array. This affects U2(), U4(), U8(), I2(), I4(), I8(), F4(), F8() factories when called with multiple values. src\SecsGem.Core\Secs2\SecsItem.cs:77-81 | Write directly to the target raw buffer using Span<byte> slices: `writeAt(_raw.AsSpan(i * size, size), i)` to eliminate the temporary byte[size] allocation and BlockCopy. Adjust the writeAt signature from `Action<byte[], int>` to `Action<Span<byte>, int>`. | Encode a SECS-II message with large multi-element arrays (e.g., SecsItem.U4(Enumerable.Range(0, 50000).Select(x => (uint)x).ToArray())) and measure Gen0/Gen1/Gen2 allocations before/after using dotnet-counters. |
| 改善 | low | Web UI TraceSnapshot() allocates array copy on every render cycle | In SimulatorBench.cs line 29, TraceSnapshot() returns `_trace.ToArray()` which allocates a new array copy every time the UI component renders. Since the Razor component (Home.razor) calls this on every StateHasChanged(), this causes repeated allocations even when the trace content hasn't changed since the last snapshot. src\SecsGem.Web\SimulatorBench.cs:29 | Either: (1) Cache the last snapshot and only update when _trace list actually changes, or (2) Change the UI to iterate directly over a thread-safe enumerator instead of converting to array. Alternatively, use ArrayPool<TraceLine>.Shared to reduce GC pressure from repeated allocations. | Monitor Gen0 GC allocations using dotnet-counters while running the Web UI and triggering GEM messages. Count the rate of TraceLine[] allocations; should decrease after caching. |
| 改善 | low | HsmsMessage.ToHexString() uses ToString() per byte | In HsmsMessage.cs lines 56-66, the ToHexString() method calls `b.ToString("X2")` for each byte in the raw frame, which allocates a string for each byte. These strings are appended to a StringBuilder. For large SECS-II body messages (e.g., 100KB), this means 100K intermediate string allocations. src\SecsGem.Core\Hsms\HsmsMessage.cs:60-64 | Use StringBuilder.Append(byte, format) or AppendFormat directly: `sb.Append(b.ToString("X2"))` → `sb.AppendFormat("{0:X2}", b)` (or use Append with overload that takes format). Even better, use a constant lookup table for hex pairs to avoid string allocation entirely. | Call ToHexString() on a large message (e.g., 1MB frame) and profile string allocations using dotnet-counters. Confirm memory usage drops with the refactored version. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | medium | Hex string length validation missing in JSON B/Binary parsing | In `src/SecsGem.Scenarios/SmlJson.cs:45-52`, the `Bytes()` method accepts hex strings without validating they have an even number of hex digits. Line 49 strips spaces and dashes, then line 50 allocates `hex.Length / 2` bytes. If `hex.Length` is odd after cleanup, `Convert.ToByte(hex.Substring(i * 2, 2), 16)` will access only `i * 2 + 1` characters of an odd-length string, causing either truncation or a silent parsing error when the last character cannot form a valid byte pair. src/SecsGem.Scenarios/SmlJson.cs:45-52 | Add validation before the loop: `if (hex.Length % 2 != 0) throw new FormatException($"Hex string has odd length: {hex.Length}");` Alternatively, pad with a leading '0' or reject as malformed input per SEMI E5 compliance. | Create a test scenario JSON with `{"B": "1 2 3"}` (odd number of hex digits) and verify it throws FormatException rather than silently producing incorrect bytes or an index out of range exception. |
| 錯誤 | high | Unchecked integer overflow in HSMS length field to array allocation | In `src/SecsGem.Core/Hsms/HsmsMessageParser.cs:26`, the HSMS length field is read as `uint` and cast directly to `int` without overflow check. Line 30 validates `bodyLength <= MaxMessageBytes`, but if a malicious or corrupted peer sends a length field > int.MaxValue (0x7FFFFFFF), the cast silently wraps to negative, passing the checks. Line 33 then attempts `new byte[bodyLength]` with a negative size, which throws OutOfMemoryException instead of an explicit validation error. Per SEMI E37, the length should be strictly < 0x80000000. src/SecsGem.Core/Hsms/HsmsMessageParser.cs:26-33 | Add explicit check: `if (bodyLength < 0 || bodyLength > MaxMessageBytes) throw new InvalidDataException($"HSMS Length {bodyLength} out of valid range");` OR cast as `var bodyLength = (long)BinaryPrimitives.ReadUInt32BigEndian(lengthBuf); if (bodyLength > MaxMessageBytes) throw ...;` to prevent silent wraparound. | Unit test: send a raw HSMS message with Length field 0xFFFFFFFF (uint.MaxValue) and verify it throws InvalidDataException with a clear message, not OutOfMemoryException. |
| 改善 | medium | Missing bounds validation on JSON numeric conversions to byte range | In `src/SecsGem.Scenarios/SmlJson.cs:55-56`, when parsing a JSON array for U1/binary, each element is cast to `(byte)x.GetInt32()`. If the JSON contains integers outside [0, 255], the cast silently truncates (e.g., 256 → 0, -1 → 255). No validation checks that numeric values fit their target format. Per SEMI E5, U1 elements should be explicitly validated as unsigned 8-bit. src/SecsGem.Scenarios/SmlJson.cs:45-56 | Add range check in Bytes() and Nums(): `if (x.GetInt32() < 0 || x.GetInt32() > 255) throw new FormatException($"Byte value {x.GetInt32()} out of range [0,255]");` before casting. Apply similar range checks to other numeric formats (U2 [0,65535], I2 [-32768,32767], etc.) | Create a scenario JSON with `{"U1": [0, 256]}` and verify it throws FormatException. Also test negative values and verify the error message is clear. |
| 改善 | low | Scenario JSON format validation could be more defensive | In `src/SecsGem.Scenarios/SmlJson.cs:18`, the code calls `.First()` on `EnumerateObject()` without checking if the object has exactly one key. A malformed JSON like `{"A":"x", "B":"y"}` (multi-key) will silently use only the first key and ignore the second, rather than failing. Similarly, `ScenarioRunner.cs:226` parses S/F values with minimal error context if the format is wrong (e.g., "S1X2" or "99F99"). src/SecsGem.Scenarios/SmlJson.cs:13-20 and src/SecsGem.Scenarios/ScenarioRunner.cs:222-226 | In SmlJson.ToItem(), count the object properties and throw if != 1: `var props = e.EnumerateObject().ToList(); if (props.Count != 1) throw new FormatException($"SECS-II item must have exactly 1 key, got {props.Count}");`. In ParseSxf(), add bounds check: `if (s < 1 || s > 127 || f < 1) throw new FormatException(...)` per SEMI E30. | Test with invalid scenario JSON files (multi-key items, out-of-spec S/F values) and confirm they produce clear FormatException messages rather than silent truncation or unexpected behavior. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 改善 | medium | HSMS control messages (Select/Linktest/Deselect) not logged in UI trace | SimulatorBench.cs line 54 subscribes only to _host.MessageLogged, which fires for SECS-II data messages (SType=0). HSMS control frames (SType=1..9) are parsed and processed by HsmsConnectionStateMachine but never reach the trace table. This hides critical handshake sequences like Select.req/Select.rsp and Linktest keepalive, making it impossible for users to see the full connection lifecycle via the UI. src/SecsGem.Web/SimulatorBench.cs:54-56 and Home.razor:62-76 | Subscribe to both _eqp.MessageLogged and _host.MessageLogged at the HsmsConnection layer (not SecsSession), which emits ALL messages including control frames. Filter by IsControl property and add separate trace rows for control messages (e.g., 'Select.req', 'Linktest.rsp') so users see the complete HSMS handshake sequence before SECS-II messages begin. | Run simulator, click '▶ 啟動模擬器', and verify that the trace shows 'Select.req', 'Select.rsp', 'Linktest' frames in addition to S1F13/S1F14 messages. Compare against raw TCP packet capture to confirm all frames are captured. |
| 改善 | medium | No visibility into scenario operation failures or validation results | Home.razor action buttons (lines 47-55) call Send() methods that catch exceptions and call AddInfo() with generic error text. When a button is clicked while not Connected/Selected, users see '尚未連線，略過' but cannot distinguish between 'connection dropped' vs 'button clicked too early' vs 'GEM state mismatch'. Also, scenario assertion results from docs/verify-report.html are never shown in the Web UI—only headless scenario runs produce detailed pass/fail reports. src/SecsGem.Web/SimulatorBench.cs:92-99 and Home.razor:46-56 | Enrich error messages with context: distinguish 'waiting for connection' (HSMS State != Selected) from 'T3 timeout' from 'S9 error'. For scenario operations, add an optional 'Validate Scenario' button that runs a lightweight assertion check (matching the logic in SecsGem.Scenarios) and displays pass/fail in a collapsible panel below the trace, citing the expected path (e.g., 'Expected S1F14 with JSTS=0, got S1F4'). Reference SecsGem.Scenarios/AssertionEngine.cs to extract assertion logic. | Manually trigger errors (e.g., disconnect mid-transaction, click a button during connection setup). Verify that each error message is distinct and actionable. Run a scenario in the Web UI and confirm assertions are evaluated and reported inline. |
| 改善 | low | Message trace table lacks decoded body content and lacks structured navigation for long sessions | Home.razor line 70-71 displays only Label and truncated hex (60 char limit). For messages like S2F13 (Equipment Constants) or S6F11 (Event Report), users cannot see the actual values (ECID, RPTID, temperature readings, etc.) without decoding the hex manually or running docs/verify-report.html. Also, trace grows unbounded in memory (_trace.Count > 500 keeps only last 500) and has no search/filter to find specific messages. src/SecsGem.Web/Components/Pages/Home.razor:60-76 and SimulatorBench.cs:102-109 | Extend TraceLine record to include an optional DecodedBody field (null for unparseable/control frames, or a string summary like 'SVID=1,2 values'). Populate it by decoding the Root property of SecsMessage using Secs2Codec.Decode(). Add a collapsible detail row per trace entry to show full SML or hex. Add a simple filter input to search by SxFy or direction, and limit trace to last 1000 lines with a 'Clear trace' button. Reference Home.razor line 102 for truncation pattern. | Run a scenario with S1F3 (read status vars) and S6F11 (event report). Verify that the trace shows decoded values inline (e.g., 'SVID 1: IDLE, SVID 2: 25.5°C') and that clicking a row expands full hex/SML body. Search for 'S6F' and verify only event reports are shown. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 改善 | high | Missing Equipment Model Inspector Tab — No visibility into live SV/EC/DV/CE values | Current Home.razor shows only connection state + message trace. Industry SECS simulators (e.g., SECS Spy, SECSGEM.NET) provide a dedicated panel or tab to inspect/edit live equipment model state: Status Variables (SV), Equipment Constants (EC), Data Variables (DV), and Collection Events (CE) with their current values. The EquipmentModel exists in SimulatorBench._model (EquipmentModel.cs) but is never exposed to the UI. Users cannot verify what values the equipment will return in S1F3/S1F4 or S2F13/S2F14 responses, nor interactively modify them before/during a test. src/SecsGem.Web/Components/Pages/Home.razor (lines 1–110) + SimulatorBench.cs (line 22: EquipmentModel _model is private/unexposed) | Add a second Razor component (e.g., EquipmentStateTab.razor) or a collapsible section in Home.razor that displays and allows real-time mutation of: (1) StatusVariables (list of ID/Name/Units/current Value); (2) EquipmentConstants (ID/Name/Min/Max/Default/current Value with input validation); (3) DataVariables (ID/Name/Value); (4) CollectionEvents (ID/Name/Enabled toggle). Expose _model as a public property on SimulatorBench for binding. This mirrors the GEM Data Dictionary inspector in commercial tools. | Launch Web UI, start simulator, check for a new tab/section showing 'DEMO-EQP' SV (ProcessState=IDLE, Temperature=25.5), EC (MaxTemp=200), and ability to click an edit button and change Temperature to 30.0 before sending S1F3 request; verify the updated value in the response message. |
| 改善 | high | No Report Definition / Event Link Visualization — SEMI E30 GEM flow is opaque | The Report Definition Chain (S2F33→S35→S37→S6F11) is critical to SECS/GEM but invisible in the UI. SimulatorBench.DemoModel() pre-defines a Report (lines 133–136) linking CEID 101 to RPTID 1 (containing DVIDs 10, 1), but the UI has no way to display or configure these relationships. Users cannot see: (1) which reports are linked to which events; (2) which data variables (DVIDs) a report will emit; (3) how to define new reports/events for custom scenarios. This gap breaks the ability to understand or demonstrate GEM integration patterns. src/SecsGem.Web/SimulatorBench.cs (lines 121–139: DemoModel()) and Home.razor (line 54: button triggers event without context) | Add a third tab/panel 'GEM Configuration' showing: (1) Collection Events table (CEID / Name / Enabled / Linked Reports); (2) Report Definitions table (RPTID / VID list); (3) Alarms table (ALID / Code / Text / Set status). Include UI controls to add/edit/delete events and reports. When the user clicks 'TriggerEvent', highlight the CEID and show which DVIDs will be sent in the S6F11 payload. Optionally, add an 'S2F33/35/37 Flow Diagram' showing the report definition chain setup. | Launch Web UI, check 'GEM Config' tab, see CEID 101 (ProcessStart) linked to RPTID 1 containing DVID 10 (WaferId) and SVID 1 (ProcessState). Click 'Trigger Event' button, verify highlighted event and the subsequent S6F11 message in trace contains the correct DVIDs. |
| 改善 | medium | Message Detail Inspector Missing — Hex + SxFy table is low-level; no structured SML-JSON view | Home.razor trace table (lines 61–76) shows only: # / Time / Direction / Label (e.g., 'S1F1') / Hex (truncated). Users cannot inspect the decoded SECS-II message body (e.g., see the exact L/U4/A values, structure, nesting) without manually hex-decoding. Industry simulators show a 'Details' panel with the message tree (SML-JSON or similar) on click. This is especially critical for debugging GEM messages with complex nested Lists. src/SecsGem.Web/Components/Pages/Home.razor (lines 59–76) | Add click handlers to trace table rows that expand a details panel below showing the decoded SECS-II item tree in a readable format (nested list, each item showing format code + value + length). Either: (a) store decoded SecsItem in TraceLine and render as collapsible JSON/tree; (b) add a clickable 'Details' column that opens a modal. The trace already has access to messages (line 54: _host.MessageLogged), so capture the decoded SecsItem alongside Hex. | Launch UI, send S1F3 (Read Status) command, click on the S1F4 response row in trace, see a 'Details' panel showing the nested L [ U4(1), A(IDLE), ...] structure; verify the values match the equipment state. |
| 改善 | medium | No Multi-Page Navigation — Single-page layout limits discoverability and feature separation | The Web UI is a single-page Home.razor (lines 1–110) crammed with connection controls + message trace + (future) model inspector + (future) GEM config. For a teaching/demo tool, this risks cognitive overload. Commercial simulators separate: Connection / Equipment State / Message Trace / GEM Config / Help/Docs into distinct views. Blazor supports NavigationManager routing (see Program.cs line 24: MapRazorComponents), but only Home.razor is defined. src/SecsGem.Web/Components/Pages/Home.razor (only page) and Program.cs (line 25: single render mode) | Refactor the single Home page into a navigation-based layout: (1) Dashboard page (Connection + Status Summary); (2) Equipment tab (SV/EC/DV/CE editor); (3) GEM Config tab (Reports/Events/Alarms); (4) Message Trace tab (with filtering/export); (5) Help/Documentation page (link to usage.html). Use a nav bar (MainLayout.razor already has <LayoutComponentBase>) or tab-based UI. This improves usability for longer sessions and aligns with industry UI patterns. | Refactor layout, launch UI, verify a nav bar or tabs appear at the top; click 'Equipment' tab and see the model inspector; click 'Message Trace' and see full-page trace table; verify navigation persists session state. |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | medium | Web UI port documentation uses non-default 5186 without explanation | README.md (line 23) and docs/usage.html (line 47-48) both recommend `dotnet run --project src/SecsGem.Web --urls http://localhost:5186`, but launchSettings.json defines the default HTTP profile applicationUrl as `http://localhost:5003` (line 16). Users following docs without --urls flag will access port 5003, not 5186, creating confusion. README.md line 23, docs/usage.html lines 47-48, src/SecsGem.Web/Properties/launchSettings.json line 16 | Either: (1) Update launchSettings.json HTTP profile to use port 5186 to match documentation, or (2) Update README.md and usage.html to explain that port 5186 is arbitrary and users can omit --urls for default port 5003. Recommend option 1 for consistency. | Run `dotnet run --project src/SecsGem.Web` without --urls flag and verify which port the app binds to (should show in console output and browser navigation) |
| 改善 | low | Quick start commands should clarify working directory requirement | README.md lines 18-26 and docs/usage.html lines 36-40 show build/test commands without explicitly stating these must be run from project root. While this is a common convention, users unfamiliar with .NET might run them from subdirectories and encounter path resolution errors for SecsGemSimulator.sln. README.md lines 18-26, docs/usage.html lines 36-40 | Add a note like 'Run from project root directory' or change command prefix to show: `cd secs-gem-simulator && dotnet build SecsGemSimulator.sln` | Try running commands from src/ subdirectory and verify they fail; confirm root directory is required |
| 改善 | low | Verify CLI default --port value (6000) differs from Web simulator ports (7100+) | docs/usage.html line 60 documents `--port 6000` as default for SecsGem.Verify, and code confirms this in SecsGem.Verify/Program.cs line 10 (basePort=6000). However, SecsGem.Web/SimulatorBench.cs line 10 uses port sequence starting from 7100. This is fine (different components), but could confuse users who see two different port ranges mentioned. docs/usage.html line 60, src/SecsGem.Verify/Program.cs line 10, src/SecsGem.Web/SimulatorBench.cs line 10 | Add clarifying note in docs: 'The --port 6000 applies only to headless scenario verification. The Web UI automatically allocates ports 7100+ for internal EQP/Host communication and is independent.' | Run both `dotnet run --project src/SecsGem.Verify -- --help` and start Web UI, then observe actual port allocations |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 錯誤 | high | HSMS 計時器範圍誤報 | README.md (line 9) 與 intro.html (line 43) 聲稱 HSMS 支援「T3–T8 計時器」，但實際實作（TimeoutKind.cs）及 CHANGELOG.md 確認僅支援 T5/T6/T7/T8。T3 屬於 Session 層的交易層級，非 HSMS 連線層。intro.html 表格（59-66 行）更進一步混淆，將 T3 列為 HSMS 計時器。 README.md:9, docs/intro.html:43 與 intro.html:59-66 | 修正 README.md 與 intro.html 之 HSMS 描述，改為「T5/T6/T7/T8 計時器」；將 T3 的說明移出 HSMS 計時器表，或在表中明確標示 T3 由 Session 層管理。 | 在 intro.html 計時器表檢查 T3 定義：若改為「Reply timeout：由 Session 層交易管理器處理」或移除該列，方為正確。比對 TimeoutKind.cs enum 確認無 T3。 |
| 錯誤 | medium | README 快速開始範例遺漏 --port 參數 | README.md 行 26 的 headless 場景驗證命令省略了 --port 參數：
`dotnet run --project src/SecsGem.Verify -- --scenarios scenarios --report docs/verify-report.html`
但 Program.cs (行 7、18-19) 及 usage.html (行 58-61) 明確列出 --port 為支援參數（預設值 6000）。完整範例應包含：
`dotnet run --project src/SecsGem.Verify -- --scenarios scenarios --port 6000 --report docs/verify-report.html` README.md:26, src/SecsGem.Verify/Program.cs:7 與 docs/usage.html:58-61 | 更新 README.md 行 26，加入 --port 參數，或在範例下方註明「可選 --port 參數指定起始 port（預設 6000）」，與 usage.html 保持一致。 | 執行 `dotnet run --project src/SecsGem.Verify -- --help`，確認 --port 在參數清單中；比對 usage.html 第 58-61 行範例是否列出該參數。 |
| 改善 | medium | README 應明確區分 T3（交易層）與其他計時器（連線層） | README 簡介段落（行 9）聲稱 HSMS 管理「T3–T8 計時器」，但 intro.html 架構說明（行 72）更準確地說「協定引擎」負責計時器，而 Session 層處理 T3。這種籠統敘述可能讓新手誤解層級劃分。 README.md:9 功能段落 | 在 README 的 HSMS 功能點下，改為「T5/T6/T7/T8 連線計時器」；在 Session / GEM 層的說明中補充「T3 交易層 reply 逾時對應」，使讀者能清楚理解分層。 | 讀者閱讀 README 後應能區分：HSMS 管理連線狀態與故障檢測（T5-T8），Session 層管理單筆交易逾時（T3）。對比 intro.html 同樣段落是否表述一致。 |
| 類型 | 嚴重度 | 項目 | 說明 / 位置 | 建議 | 驗證方式 |
|---|---|---|---|---|---|
| 改善 | medium | Missing PackageVersion and LicenseExpression in .csproj files | The project declares [1.0.0] in CHANGELOG.md but does not define <PackageVersion>, <AssemblyVersion>, or <LicenseExpression> in any .csproj files. This is incomplete for downstream packaging (NuGet, GitHub Releases, or container images). src/SecsGem.Core/SecsGem.Core.csproj, src/SecsGem.Web/SecsGem.Web.csproj, src/SecsGem.Scenarios/SecsGem.Scenarios.csproj (lines 1-14 each) | Add <PropertyGroup> block to each library .csproj with: <PackageVersion>1.0.0</PackageVersion>, <LicenseExpression>MIT</LicenseExpression>, <Authors>Lumeon Tech</Authors>, <RepositoryUrl>...</RepositoryUrl>. This enables: (1) dotnet pack to generate accurate NuGet metadata, (2) GitHub Release automation to read semantic version, (3) Docker/container tools to embed versioning. | Run 'dotnet pack' on SecsGem.Core and inspect .nupkg metadata; verify LicenseExpression is 'MIT' in .nuspec; check AssemblyVersion matches 1.0.0 via 'dotnet build && ildasm' or PowerShell Get-Item bin/Debug/*.dll | % VersionInfo |
| 改善 | low | SecsGem.Verify Program.cs lacks early version-check output for CI pipelines | The CLI tool returns correct exit codes (0=all pass, 1=some fail, 2=setup error) per line 75, but does not print '--version' or '--help version' output. CI pipelines often log tool versions at startup for reproducibility. src/SecsGem.Verify/Program.cs, lines 20-23 (help handler) | Add '--version' case to args loop: case "--version": { Console.WriteLine("SecsGem.Verify 1.0.0"); return 0; } This aligns with CI best practices and allows scripts to verify the exact build version during test runs. | Run 'dotnet run --project src/SecsGem.Verify -- --version' and confirm it prints 'SecsGem.Verify 1.0.0' with exit code 0 |
| 錯誤 | medium | docs/verify-report.html is .gitignore'd but no CI automation specified | .gitignore line 14 excludes 'docs/verify-report.html' (correct for ephemeral CI artifacts), but there is no GitHub Actions / CI config (.github/workflows/*.yml, azure-pipelines.yml, or .gitlab-ci.yml) visible. Without CI, the report is never auto-generated, and release artifacts won't include verification evidence. .gitignore line 14; missing .github/workflows/ or equivalent CI definition | Create .github/workflows/ci.yml (or equivalent) with step: 'dotnet run --project src/SecsGem.Verify -- --scenarios scenarios --report docs/verify-report.html'. Conditionally upload docs/verify-report.html to release assets. This ensures every release includes passing scenario verification and is traceable to CI logs. | Push to remote (if GitHub), check for .github/workflows/ folder; or run locally: 'dotnet run --project src/SecsGem.Verify -- --scenarios scenarios --report docs/verify-report.html && file docs/verify-report.html' to confirm it generates valid HTML. |
© Lumeon Tech · 明際資訊 · 自動產出之審核帳本