Replace brittle EOF string matching with type-safe downcasting

[coderabbitai]

Context

Related to PR #424 which demonstrates the pattern for modularisation by refactoring the connection module.

Request from: @leynos
Source: #424

Objective

Replace brittle string matching for EOF classification in codec-error world steps with type-safe downcasting.

Current situation

• tests/worlds/codec_error/decoder_ops.rs:101 uses brittle string matching to classify EOF errors
• This is fragile and prone to breakage if error messages change

Proposed solution

Since CodecError::Eof becomes an io::Error carrying EofError internally (src/codec/error.rs:298):

1. Downcast via io::Error::get_ref() to EofError
2. Delete the string-matching heuristic
3. Use type-safe error classification instead

Benefits

• Type-safe error classification
• More robust to error message changes
• Eliminates brittle string matching
• Clearer intent and better maintainability

[traycerai]

Warning

Rate limit exceeded. Please try after 518191 seconds.

[leynos]

@coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

If it is still relevant, please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.\n\nOnce a decision has been made, apply the appropriate label.

If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

Please also provide an impact of the required changes.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details of issue `#434`
gh issue view 434 --json title,body,state,labels,createdAt,updatedAt

Length of output: 1153

---

🏁 Script executed:

#!/bin/bash
# Check if the test file still has brittle string matching around line 101
if [ -f tests/worlds/codec_error/decoder_ops.rs ]; then
  echo "=== tests/worlds/codec_error/decoder_ops.rs around line 101 ==="
  cat -n tests/worlds/codec_error/decoder_ops.rs | sed -n '90,115p'
else
  echo "File tests/worlds/codec_error/decoder_ops.rs not found"
fi

echo ""
echo "=== Searching for string matching patterns in the file ==="
if [ -f tests/worlds/codec_error/decoder_ops.rs ]; then
  rg -n "contains|match|Error" tests/worlds/codec_error/decoder_ops.rs | head -20
fi

echo ""
echo "=== Check codec error implementation ==="
if [ -f src/codec/error.rs ]; then
  echo "--- Around line 298 for EofError ---"
  cat -n src/codec/error.rs | sed -n '290,310p'
fi

Length of output: 1071

---

🏁 Script executed:

#!/bin/bash
# Search for codec_error related test files
echo "=== Finding codec_error related files ==="
fd -t f "codec.*error" tests/

echo ""
echo "=== Finding worlds directory structure ==="
fd -t d "worlds" tests/ | head -10

echo ""
echo "=== Search for decoder_ops files ==="
fd -t f "decoder_ops"

echo ""
echo "=== Search for EofError definition ==="
rg -n "struct EofError|enum EofError" src/

echo ""
echo "=== Search for brittle string matching on EOF errors ==="
rg -n "contains.*[Ee]of|[Ee]of.*contains|unexpected end" tests/ | head -20

Length of output: 543

---

🏁 Script executed:

#!/bin/bash
# Get context around the brittle string matching at line 122
echo "=== tests/fixtures/codec_error/decoder_ops.rs around line 122 ==="
cat -n tests/fixtures/codec_error/decoder_ops.rs | sed -n '110,135p'

echo ""
echo "=== Full EofError enum definition ==="
rg -A 10 "pub enum EofError" src/codec/error.rs

echo ""
echo "=== Check how EofError is converted to io::Error ==="
rg -B 5 -A 10 "impl From<CodecError> for io::Error" src/codec/error.rs

echo ""
echo "=== Check the full function containing the brittle string matching ==="
ast-grep --pattern 'fn $FUNC($$$) {
  $$$
  self.detected_eof = Some(if msg.contains("header") {
    $$$
  })
  $$$
}'

Length of output: 2561

---

🏁 Script executed:

#!/bin/bash
# Get complete EofError enum definition with all variants
echo "=== Complete EofError enum definition ==="
rg -A 30 "pub enum EofError" src/codec/error.rs

echo ""
echo "=== Check if there are any uses of io::Error::get_ref or downcasting ==="
rg -n "get_ref|downcast" src/ tests/

echo ""
echo "=== Get more context of the classify_eof_error function ==="
cat -n tests/fixtures/codec_error/decoder_ops.rs | sed -n '100,140p'

echo ""
echo "=== Check what EofError variants are being used ==="
rg -n "EofError::" tests/fixtures/codec_error/decoder_ops.rs

Length of output: 4405

---

🏁 Script executed:

#!/bin/bash
# Apply the "low" criticality label to issue `#434`
gh issue edit 434 --add-label "low"

Length of output: 46

---

@leynos, I've analysed the codebase and issue #434. Here's my assessment:

Issue Status: STILL RELEVANT

The brittle string matching still exists in the codebase, though at a slightly different path than originally noted:

• Original issue location: tests/worlds/codec_error/decoder_ops.rs:101
• Actual location: tests/fixtures/codec_error/decoder_ops.rs:122

The code uses msg.contains("header") to classify EOF errors into EofError::MidHeader vs EofError::MidFrame variants.

Documentation Status

The code includes extensive documentation acknowledging the brittleness:

• Lines 101-114 contain detailed implementation notes explaining the fragility
• Line 115 has a FIXME(Refactor message assembly test API; multiplexing tests #418) comment (note: references #418, not #434)
• Comments explicitly state: "This is fragile: if the upstream error message format changes, this classification will silently produce incorrect results"

The documentation is comprehensive but describes the problem rather than the solution.

Criticality Assessment: LOW

Rationale:

• Scope: Affects test code only, not production code
• Risk of user impact: Indirect—could mask bugs if tests fail incorrectly due to error message changes
• Blast radius: Limited to test fixtures in tests/fixtures/codec_error/
• Acknowledged trade-off: Comments state this is "acceptable for test code where we control the error messages"

Label applied: low ✓

Type: Code Changes Required (NOT documentation-only)

This requires actual code modifications to the test infrastructure.

Impact of Required Changes

Scope:

• Modify classify_eof_error() function in tests/fixtures/codec_error/decoder_ops.rs
• Replace string matching with type-safe downcasting using io::Error::source() or get_ref()
• Remove brittle msg.contains("header") logic
• Update or remove FIXME comment

Implementation approach:
Since CodecError::Eof becomes an io::Error carrying EofError internally (via io::Error::new(io::ErrorKind::UnexpectedEof, e) at src/codec/error.rs:307), the solution would downcast via io::Error::source() and downcast_ref::<EofError>() to extract the variant type-safely.

Risk level: Low—changes are confined to test code with good test coverage to verify the refactor works correctly.

Benefits: Improved test robustness, elimination of fragile string matching, clearer intent, better maintainability.

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Approach

Replace brittle string matching in test code's EOF error classification with type-safe downcasting by first fixing the error conversion to preserve type information, then updating the classification method to use downcasting instead of string inspection.

Observations

The codebase uses a structured error hierarchy with CodecError as the top-level codec error type, containing an Eof(EofError) variant. EofError distinguishes between CleanClose, MidFrame, and MidHeader EOF scenarios.

When CodecError is converted to io::Error (in src/codec/error.rs), most variants preserve their type information, but EofError::CleanClose is currently converted to a string literal which loses type information.

The test fixture in tests/fixtures/codec_error/decoder_ops.rs contains a classify_eof_error method that uses fragile string matching to distinguish EOF types, with an existing FIXME comment acknowledging this technical debt.

Implementation Steps

Phase 1: Preserve EofError Type Information in Error Conversion

This phase fixes the root cause by ensuring all EofError variants preserve their type information when converted to io::Error, enabling downcasting for all EOF types.

Update CodecError to io::Error Conversion

Modify the From<CodecError> for io::Error implementation to preserve EofError::CleanClose type information instead of converting it to a string literal.

• Update src/codec/error.rs in the From<CodecError> for io::Error implementation (around line 304-305)
• Change the EofError::CleanClose case to wrap the actual EofError type instead of the string literal "connection closed"
• This enables io::Error::get_ref() to return the original EofError for downcasting
• The error message will improve from "connection closed" to "connection closed cleanly at frame boundary" (using EofError's Display impl)

🤖 Prompt for AI agents

Fix the error conversion to preserve type information for
`EofError::CleanClose`. This enables downcasting in downstream code.

In `src/codec/error.rs`, locate the `From<CodecError> for io::Error`
implementation (around line 304-305):

- Find the case handling `EofError::CleanClose`
- Currently it converts to a string literal "connection closed", which loses
type information
- Change it to wrap the actual `EofError` type instead of the string literal
- This allows `io::Error::get_ref()` to return the original `EofError` for
downcasting
- The error message will automatically improve from "connection closed" to
"connection closed cleanly at frame boundary" (via EofError's Display
implementation)

Phase 2: Replace String Matching with Type-Safe Downcasting

This phase updates the test code to use the newly available type information through downcasting instead of fragile string inspection.

Refactor classify_eof_error Method

Replace the string-matching logic in the test fixture with type-safe downcasting to EofError.

• Update tests/fixtures/codec_error/decoder_ops.rs method classify_eof_error (lines 117-133)
• Remove the string message inspection (msg.contains("header") pattern)
• Use io::Error::get_ref() to obtain a reference to the inner error
• Downcast the inner error to &EofError using downcast_ref::<EofError>()
• Clone the EofError directly into self.detected_eof when downcasting succeeds
• Remove the FIXME(#418) comment and associated documentation about the fragile approach (lines 101-116)
• Handle the case where downcasting fails (shouldn't occur in normal operation, but should have a fallback)

🤖 Prompt for AI agents

Replace brittle string matching with type-safe downcasting in test fixture. This
uses the type information preserved in Phase 1.

In `tests/fixtures/codec_error/decoder_ops.rs`:

- Locate the `classify_eof_error` method (lines 117-133)
- Remove the current string message inspection logic (the
`msg.contains("header")` pattern)
- Implement type-safe downcasting:
  - Use `io::Error::get_ref()` to obtain a reference to the inner error
  - Downcast to `&EofError` using `downcast_ref::<EofError>()`
- When downcasting succeeds, clone the `EofError` directly into
`self.detected_eof`
- Handle the case where downcasting fails (shouldn't occur in normal operation,
but provide a fallback)
- Remove the FIXME(`#418`) comment and associated documentation about the fragile
string-matching approach (lines 101-116)

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Fix the error conversion to preserve type information for
`EofError::CleanClose`. This enables downcasting in downstream code.

In `src/codec/error.rs`, locate the `From<CodecError> for io::Error`
implementation (around line 304-305):

- Find the case handling `EofError::CleanClose`
- Currently it converts to a string literal "connection closed", which loses
type information
- Change it to wrap the actual `EofError` type instead of the string literal
- This allows `io::Error::get_ref()` to return the original `EofError` for
downcasting
- The error message will automatically improve from "connection closed" to
"connection closed cleanly at frame boundary" (via EofError's Display
implementation)
===============================================================================

Task: 2

Replace brittle string matching with type-safe downcasting in test fixture. This
uses the type information preserved in Phase 1.

In `tests/fixtures/codec_error/decoder_ops.rs`:

- Locate the `classify_eof_error` method (lines 117-133)
- Remove the current string message inspection logic (the
`msg.contains("header")` pattern)
- Implement type-safe downcasting:
  - Use `io::Error::get_ref()` to obtain a reference to the inner error
  - Downcast to `&EofError` using `downcast_ref::<EofError>()`
- When downcasting succeeds, clone the `EofError` directly into
`self.detected_eof`
- Handle the case where downcasting fails (shouldn't occur in normal operation,
but provide a fallback)
- Remove the FIXME(`#418`) comment and associated documentation about the fragile
string-matching approach (lines 101-116)

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For assumption 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!