# 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.