6krrt/code_plans/shared-helper-unparseable-input-contract.md

# Spec + implementation plan: pin down what a shared helper yields on well-typed-but-unparseable input

**Origin.** PR #3 collapsed four independent `messages` → `image_url` part
traversals into one generator, `capabilities.iter_image_url_values`
(`code_reviews/capability-gate-followups.md`, item #6). The plan specified
the two shapes the four originals agreed on (`image_url` as a dict with
`url`, or as a bare string) but not the case where a part is typed
`image_url` and carries neither — well-typed, but nothing to extract. The
merged helper silently skipped that case instead of yielding a placeholder,
which dropped a degenerate part out of two fail-closed checks at once (see
`code_reviews/pr-3-capability-gate-followups-review.md` for the live
repro and the fix). The follow-up opencode session that implemented the fix
named the general lesson explicitly, which is what this document is
formalizing: **a plan that proposes collapsing N implementations into one
shared helper has to specify the helper's contract on every input shape,
not only the ones the N originals happened to agree on.**

This is a process document, not a bug report — no code changes accompany
it. It exists so the next plan that proposes a shared helper pins this down
up front instead of it being rediscovered in review.

---

## Spec

### The problem, generalized

When several independent implementations of "walk this structure and pull
out X" get unified into one shared helper, each original's handling of its
*own* edge cases has to be re-derived by the person writing the unification
— nothing forces them to enumerate it, because the edge case was never the
reason any of the N implementations were written in the first place. Each
one just happened to fall into some fallback path. The happy path is easy
to unify because all N agree on it by construction (they all worked, on
real traffic, for the common shape). The edge cases are exactly where they
are most likely to *quietly disagree*, and disagreement there is invisible
unless a plan makes each implementation's edge-case behavior explicit
before proposing the replacement.

The specific edge case that bit PR #3 — matches the coarse type tag but the
payload is missing or empty — deserves its own name because it is a
distinct category from "wrong type" or "absent": it is a **well-typed but
unparseable** input. A traversal that filters on `part.get("type") ==
"image_url"` before doing anything else will always let this shape through
the filter; what happens next is the part that has to be decided
deliberately.

### The rule

Any implementation plan that proposes extracting or unifying a shared
traversal, parser, or extractor helper from multiple existing call sites
must include an explicit contract: for every input shape the helper will
see, state what it yields or returns. At minimum, enumerate:

1. **The common/valid shape** — the one all N originals handle the same way.
2. **Every alternate valid shape** any existing call site already handles
   (there may be more than one — PR #3 had three: `image_url` as
   `{"url": ...}`, `image_url` as a bare string, and a bare `url` key on
   the part itself).
3. **The well-typed-but-unparseable shape** — passes the coarse type/shape
   check but the value cannot actually be extracted. State the yielded
   value explicitly (a sentinel like `""`, not "nothing" / "skip") unless
   skipping is a deliberate, stated decision.
4. **The wrong-typed or absent shape** — fails the coarse check. Usually
   "skip", but say so, so it's a decision and not an assumption.

And critically: **for category 3, list what each of the N originals
currently does**, not just what the new helper will do. That list is what
surfaces disagreement before the merge, rather than after. In PR #3, all
four originals agreed by accident (each one's "nothing extracted" path
happened to either count the part or fail closed) — but nothing in the
plan recorded that agreement, so the merge didn't have to preserve it and
didn't.

### Why "skip silently" is the default failure mode to watch for

A `for`/`if`/`continue` traversal skips by construction unless a branch
explicitly appends/yields on every path. Converting four such loops into
one `yield`-per-match generator is a natural, idiomatic simplification —
and it's exactly the transformation that turns "always contributes
*something*, even a fail-closed placeholder" into "contributes nothing,
silently, for a whole category of input." This isn't specific to
generators — the same risk applies to a shared parser that returns `None`
for the unparseable case where callers previously each had their own
`None`-handling that didn't all agree — but generators make it easy to
introduce, because `continue`-without-yielding reads as the most natural
thing to write.

---

## Implementation plan

This is a discipline to apply going forward, not a code change. Concretely:

### 1. Template for future plans

When a `code_plans/` (or `code_reviews/`) entry proposes extracting a
shared helper from multiple call sites, include a table shaped like this
alongside the proposed code:

| Input shape | Original A | Original B | Original C | ... | New shared helper |
|---|---|---|---|---|---|
| common/valid | ... | ... | ... | | ... |
| alternate valid #1 | ... | ... | ... | | ... |
| well-typed, unparseable | ... | ... | ... | | ... (state the sentinel) |
| wrong-typed / absent | skip | skip | skip | | skip |

If any two originals disagree in a row, that disagreement is the thing the
plan has to resolve on purpose (pick one, document why) rather than let the
merge resolve by accident.

### 2. No retroactive audit needed right now, but a watch list for next time

Checked the rest of `dispatcher.py` for other traversals of a message's
multimodal `content` list, since that's the same shape that bit PR #3.
None of these are currently merged/unified helpers, so none are broken —
they're independent, single-purpose traversals, not N-collapsed-into-1. But
if any of them are ever unified (with each other, or with
`iter_image_url_values`), this spec applies:

- `session_fingerprint` (dispatcher.py:1302) — joins text parts to fingerprint a session.
- `session_directory` (dispatcher.py:1321) — joins text parts to find a working directory.
- `estimate_prompt_tokens` (dispatcher.py:1364) — sums text-part lengths for the context estimate.
- `_last_user_text` (dispatcher.py:1391) — joins the last user turn's text parts.

All four already treat a non-dict or non-string part as "contributes
nothing" consistently with each other (each filters with
`isinstance(p, dict)` / `isinstance(p.get("text"), str)` before including
it), so there's no live disagreement to flag today — this is a watch list,
not a finding.

### 3. Where this lives

Keep this document as the reference the next relevant plan cites, rather
than duplicating the rule inline each time. A future plan proposing a
shared-helper extraction should link back to this file
(`code_plans/shared-helper-unparseable-input-contract.md`) and fill in the
contract table from §1 as part of the plan, the same way
`code_reviews/capability-gate-followups.md` gave concrete before/after code
for each finding rather than describing it in prose only.