3523dcf93e
plans/ held 58 documents and exactly one said whether it was open. The rest
mixed finished work, reviews of shipped work, parked specs and genuinely
pending ones, with nothing distinguishing them, so "how many plans are in
the queue" had no answer short of reading all 58.
Now `grep -H '^Status:' plans/*.md` is the answer:
50 done 3 in progress 2 planned 2 reference 1 parked
Statuses were derived rather than guessed: CLAUDE.md's own built list and
"What's NOT built yet" section, plus checking the subject exists in the
code. A review of work that shipped counts as done -- it records what was
found, it is not a request for anything. `reference` separates the two docs
that are conventions rather than work items (admin-design-standards,
admin-work-framework), which otherwise read as permanently-open plans.
The vocabulary is deliberately five words. A larger one invites "mostly
done" and "blocked-ish", which is how the directory became unreadable.
test_plans_declare_status.py keeps it from rotting: a new plan without a
marker fails, as does an unknown status, one buried below the eighth line,
or an open status with no reason -- "planned" alone is the state that rots,
since nobody can tell later whether it waits on a decision, a dependency,
or just nobody's turn.
Also updates the sweep plan with what landed and what did not, including
that #9 was not a defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
143 lines
7.2 KiB
Markdown
143 lines
7.2 KiB
Markdown
# Spec + implementation plan: pin down what a shared helper yields on well-typed-but-unparseable input
|
|
|
|
Status: done -- parse_verdict_json returns None rather than a false verdict
|
|
|
|
**Origin.** PR #3 collapsed four independent `messages` → `image_url` part
|
|
traversals into one generator, `capabilities.iter_image_url_values`
|
|
(`plans/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
|
|
`plans/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 `plans/` (or `plans/`) 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
|
|
(`plans/shared-helper-unparseable-input-contract.md`) and fill in the
|
|
contract table from §1 as part of the plan, the same way
|
|
`plans/capability-gate-followups.md` gave concrete before/after code
|
|
for each finding rather than describing it in prose only.
|