Handle string data in Originate_Application

[itg-karthicr]

Summary

• Treat a plain string data value for Originate_Application as one application argument.
• Keep sequence input behavior, so multi-argument data still joins with commas.
• Record the fix in the changelog.

Root Cause

Originate_Application joined every item in data. A string is iterable, so "goodbye" became g,o,o,d,b,y,e.

Validation

• rtk .venv/bin/pytest tests/test_ami_request.py -q
• rtk .venv/bin/pytest -q
• rtk .venv/bin/ruff check .
• rtk .venv/bin/ruff format --check .

Closes #11.

[itg-karthicr]

Review panel findings — 5 reviewers

Scope: `b4014a8...HEAD` | base `master`

The primary fix (treating a plain string `data` as one argument) is correct and the tests for it are solid. Two issues need a code change before merge.

---

Critical

`ami.py:1069` — `Action` header bypasses CRLF validation

`validate_header` is defined and called on every header assembled in the loop, plus `ActionID`. But the first item is constructed before the loop:

```python
items = [(KEY_ACTION, self[KEY_ACTION])]
```

This line does not go through `validate_header`. A `_Request` constructed with a CRLF-bearing action name emits an injected header on the wire with no error raised. Fix:

```python
items = [validate_header(KEY_ACTION, self[KEY_ACTION])]
```

Add a regression test: `_Request("Ping\r\nInjected: x")` should raise `ValueError`.

---

Warning

`core.py:844` — `bytes` data produces silent wire corruption

`isinstance(data, str)` is `False` for `bytes`, so a `bytes` argument falls through to the join path. Iterating `bytes` yields integers (e.g., `b"goodbye"` → 103, 111, 111...), and `str(d)` on each produces `"103,111,111,100,98,121,101"` in the `Data` header. No exception is raised; Asterisk receives corrupted data. Fix:

```python
if isinstance(data, bytes):
raise TypeError("data must be a string or sequence of strings, not bytes")
if isinstance(data, str):
data = (data,)
```

`ami.py` / `send_action` — `ValueError` is undocumented on the public API

`build_request` now raises `ValueError` on CRLF in any header, but neither `build_request` nor `send_action` documents this. Callers that catch only `ManagerError`/`ManagerSocketError` will see unhandled `ValueError` propagate to an outer frame with no warning.

Add a raises note to both docstrings: Raises `ValueError` if any header key or value contains CR or LF.

---

Info

• CHANGELOG documents the string-data fix but not the CRLF validation (a separate behavior change: callers now get `ValueError` for embedded newlines).
• `data=""` is silently dropped by `if data:` — add a test pinning this as intentional, or change the guard to `if data is not None:`.
• `ActionID` CRLF rejection is implemented but has no regression test.

---

5-agent panel: Codex (architecture), internal-consistency, convention/style, silent-failure-hunter (security), test-coverage.

[itg-karthicr]

Addressed the review panel findings in 71feab2.

• Critical: Action now goes through the same CR/LF header validation as other AMI headers, with a regression test for _Request("Ping\r\nInjected: x").
• Warning: Originate_Application now rejects top-level bytes data with TypeError instead of serializing byte values as comma-separated integers, with a regression test.
• Warning: documented the public ValueError behavior on both _Request.build_request and Manager.send_action.
• Info: added CHANGELOG coverage for CR/LF validation, added an ActionID CR/LF regression test, and pinned the current data="" behavior with a test.

Validation run:

• pytest tests/test_ami_request.py -q
• pytest -q
• ruff check .
• ruff format --check .

[itg-karthicr]

Review panel — round 2

All round-1 critical and warning findings are correctly resolved. Codex ran live runtime checks and confirmed no new bugs were introduced.

One new warning

core.py:844 — bytearray and memoryview bypass the bytes guard

isinstance(data, bytes) does not catch bytearray or memoryview. Both are iterable and yield integers, so str(d) on each produces comma-separated ASCII values — the same silent corruption the bytes fix was added to prevent. bytearray is common in Python networking code.

Fix:

if isinstance(data, (bytes, bytearray, memoryview)):
    raise TypeError("data must be a string or sequence of strings, not bytes")

Info (no code change required)

• test_rejects_action_containing_crlf — the test is mechanically correct (the action name is a header value, so the "values" error message is right), but the name implies the key side is being tested. A rename or inline comment would help future readers.
• Multi-value header element CRLF — the code path is guarded but has no regression test. Suggested: request["Variable"] = ("a=1\r\nInjected: x", "b=2") asserting ValueError.
• CHANGELOG CRLF entry has no (#n) — no tracking issue exists for the CRLF hardening, so this is an acceptable gap.

What the panel agrees is clean

• validate_header is airtight across all paths in build_request — Action, ActionID, loop items, and kwargs all go through it.
• b"" is correctly caught before the if data: falsy guard.
• str(self[KEY_ACTION]) wrapping is harmless.
• All 4 new tests are logically correct, including test_rejects_action_containing_crlf which correctly expects the "values" error message.
• TypeError from the bytes guard propagates correctly to callers.

One code fix needed before merge — extend the bytes guard to (bytes, bytearray, memoryview).

---

5-agent panel: Codex (architecture + runtime checks), internal-consistency, convention/style, silent-failure-hunter (security), test-coverage.

[itg-karthicr]

Addressed the round-2 review warning in 67ae4f0.

• Extended the Originate_Application binary-data guard from bytes to (bytes, bytearray, memoryview) so bytes-like inputs cannot be serialized as comma-separated integer values.
• Expanded the regression coverage to include non-empty and empty bytes, bytearray, and memoryview inputs.
• Applied the info-level test cleanups from the review: renamed the action CR/LF test to clarify it validates the header value, and added coverage for CR/LF inside a multi-value header element.
• Updated the changelog wording from bytes to bytes-like data.

Validation run:

• python -m pytest tests/test_ami_request.py -q
• python -m pytest -q
• ruff check .
• ruff format --check .

[itg-karthicr]

Review panel — round 3 (commit 67ae4f0)

One warning confirmed by all 4 reviewers. No critical findings.

Warning

core.py:844 — array.array, ctypes arrays, and mmap bypass the bytes-like guard

isinstance(data, (bytes, bytearray, memoryview)) returns False for all three types. Each implements the buffer protocol and iterates as integers, so passing array.array('b', b'hi') silently produces "104,105" in the AMI Data field — the same silent corruption this commit was added to prevent. Codex confirmed live: memoryview(array.array('b', b'hi')) succeeds, so a buffer-protocol probe catches them all.

Recommended fix:

try:
    v = memoryview(data)
    v.release()
    raise TypeError("data must be a string or sequence of strings, not a bytes-like object")
except TypeError:
    raise
except Exception:
    pass

This also handles any future buffer-protocol type without needing to enumerate them.

Info

core.py:845 — TypeError message says "not bytes" but the guard now rejects three types

Change to "not a bytes-like object" to match CPython's own terminology and the CHANGELOG wording. Update the match= regex in test_originate_application_rejects_binary_data at the same time.

core.py:819 — docstring doesn't mention bytes-like rejection

Worth a one-line addition noting that bytes-like objects raise TypeError.

What is clean

• Guard ordering is correct: isinstance fires before if data:, so empty bytes-like values are caught.
• All 6 parametrized cases pass (empty and non-empty bytes, bytearray, memoryview).
• Test rename and multi-value CRLF test are solid additions.
• CHANGELOG "bytes-like" is the correct CPython term and backtick style is consistent.

---

4-agent panel: Codex (architecture + runtime checks), internal-consistency, silent-failure-hunter (security), convention/style.

[itg-karthicr]

Addressed the round-3 review feedback in cf333ad.

• Replaced the enumerated (bytes, bytearray, memoryview) guard with a buffer-protocol probe using memoryview(data), so array.array, ctypes arrays, mmap, and future bytes-like objects are rejected before they can serialize as comma-separated integers.
• Updated the TypeError wording to not a bytes-like object and adjusted the test regex accordingly.
• Added the docstring note that bytes-like data raises TypeError.
• Expanded regression coverage for array.array, ctypes arrays, and mmap.
• Added fb60fcc to ignore the project-local .worktrees/ directory used for isolated follow-up work.

Validation run:

• RED check: targeted binary-data test failed before implementation for the expected reasons.
• python -m pytest tests/test_ami_request.py::test_originate_application_rejects_binary_data -q
• python -m pytest tests/test_ami_request.py -q
• python -m pytest -q
• ruff check .
• ruff format --check .

[itg-karthicr]

Review panel — round 4 (commits cf333ad, fb60fcc)

No critical findings. Two comments recommended before merge; no logic changes needed.

Warning

core.py:849 — except TypeError: pass needs a comment explaining its intent

The pass branch correctly handles "not a buffer type" — plain list, tuple, and similar sequences all raise TypeError from memoryview() and fall through to the join path. The risk is that TypeError is also raised by broken __buffer__ implementations on third-party objects, and those are silently swallowed with no diagnostic. A comment prevents both silent misuse and future removal:

except TypeError:
    # data does not implement the buffer protocol;
    # treat it as a plain sequence and fall through.
    pass

core.py:857 — isinstance(data, str) at line 857 looks like dead code but is the normal str path

A str skips the buffer probe (if not isinstance(data, str):) then falls through to if data: where line 857 wraps it in a tuple for the join. The check is correct and necessary, but the two-level guard reads as redundant. A comment prevents a future maintainer from deleting it:

# str skips the buffer probe above; wrap it here for the join.
if isinstance(data, str):
    data = (data,)

Info

• array.array("u", ...) / ctypes.c_wchar arrays iterate as strings but export the buffer protocol, so they are rejected by the probe. Contrived edge case; very unlikely in practice.
• data=42 produces a confusing not iterable error from str.join — pre-existing, not a regression.
• ctypes arrays have no close(), so the test finally relies on CPython GC. Harmless in practice.
• mmap.mmap(-1, 2) is portable — Python docs confirm -1 is the standard anonymous mmap form on both Unix and Windows (confirmed by Codex with live checks).
• data_view.release() before raising is correct and necessary: without it, mmap.close() raises BufferError due to a live exported pointer.

What the panel agrees is clean

• Buffer-protocol probe correctly rejects bytes, bytearray, memoryview, array.array, ctypes, and mmap.
• Lambda factories in the parametrize block are idiomatic and the right call for resource scoping.
• Test try/finally cleanup is correct for all 9 parametrize cases.
• .gitignore worktree entry is correct and complete.

---

4-agent panel: Codex (architecture + runtime checks), internal-consistency, silent-failure-hunter (security), convention/style.

[itg-karthicr]

Addressed the round-4 review feedback in cd4bfb7.

• Added an explanatory comment for the memoryview(data) TypeError fallthrough: non-buffer sequences intentionally continue to normal Data joining.
• Added an explanatory comment for the str path: strings skip the buffer-protocol probe and are wrapped before join().
• No logic changes were made.

Validation run:

• python -m pytest tests/test_ami_request.py -q
• python -m pytest -q
• ruff check .
• ruff format --check .

[itg-karthicr]

Review panel — satisfied

Five rounds across all commits. No open critical or warning findings remain. The implementation is correct, the tests are thorough, and the guards are well-documented.

This PR is ready to merge.