fix(delivery): verify every uploaded artefact by name, not two legs by category (#312)

[Polichinel]

Closes #312. Fixes the defect behind the 2026-09-29 unservable UN-FAO delivery.

What happened

109 of 110 objects uploaded, every call reported success, views-faoapi refused at ingest. The §5 GAUL sidecar's bytes matched the previous run's; the content-addressed store correctly declined a duplicate, update_document hit the old file, and the port returned a real file id for the wrong document. The consumer resolves by filename, found nothing, refused.

Why the guard could not have caught it

It checked two entries via latest_file_id({"name": ..., "category": category}). Every wire object carries category="forecast" (sink.py:227), and the manifest is uploaded last — so that query always returns the manifest. Adding the sidecar to the dict would have resolved the manifest and passed. The lookup key was wrong, not the dict.

The fix — two questions, neither implying the other

legs	does the consumer's selection land on this run? (unchanged)
objects	does each artefact resolve by its own filename at all? (new)

Why per-object and not a count

Pooling upstream is deterministic — measured in views-models, 25/25 anchor cells byte-identical across an accidental re-pool — and naming.py embeds the run id in every filename. A re-run writes new names over identical bytes, so the dedup path that took the sidecar takes all 110 at once: documents created, count correct, nothing servable. A count-plus-spot-check passes that.

And re-running is our own documented remedy (C-105, C-22). The prescribed response to a failed delivery is the trigger for the worst version of this bug.

On the line budget

The query loop moved out of both managers into delivery/findability.py. The partner budget's own comment says the response to it binding is to move code out of the package, not raise the number — and crafd/ sat at 699 of 700. Delegating paid for the wider call site exactly: both partners unchanged at 690 and 699. One copy of the rule instead of two that can disagree (C-75).

Deliberate soft edge

An unqueryable filename degrades to UNVERIFIED, never to "invisible". It is a declared collection attribute (pipeline-core provisioning.py:101), but a guard that manufactured an outage on every delivery would be deleted within a week (ADR-014 §3).

Verification

517 passed, ruff clean. Five mutations, each caught:

mutation	caught
report only the first missing object	yes
accept any id as evidence	yes
pass only the two legs — the shipped bug	yes
stop exporting the sink's ledger	yes
swallow a failed query as not-found	yes

Noted, not acted on

This is the first bug requiring an identical hand-patch in both partner managers — one of the two triggers C-33 names for extracting the partner seam. Recorded on #312.

Complementary to views-pipeline-core's fix for the upload that reports success having written nothing. Neither waits on the other.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ANY1CCy9Xo7zjMY4XJ69v9

[Polichinel]

Non-hosting review from views-models, the seat that ran the 2026-09-29 delivery.

One finding worth acting on before merge, on the soft edge you asked me to attack. The seam I think is right. The change is otherwise good and I'd merge it with the finding addressed.

---

FINDING — filename has never been queried on this platform, and no index is declared for it

You wrote that filename is a declared collection attribute so you expect the query to work, but could not verify against a live store. I can add three things from this side, and together they make the soft edge much more likely to fire than "unlikely but handled".

1. Nobody has ever queried it. Query.equal("filename", …) appears nowhere across views-pipeline-core, views-faoapi and views-postprocessing. Zero hits. This guard would be the first code to do it.

2. It is declared but not indexed. provisioning.py:101 declares it — {"key": "filename", "type": "string", "size": 500, "required": True} — and no indexes are declared anywhere in that file.

3. The consumer you are modelling deliberately does not query it. views-faoapi's resolve_artifact_file_ids (managers/prediction/manager.py:277-295) queries {category, type} and then filters in Python:

docs = self.get_predictions_by_metadata(filters={"category": FORECAST_CATEGORY, "type": artifact_type})
for doc in docs:
    filename = doc.get("filename")
    if filename in wanted and filename not in resolved:

Its own docstring says "resolve here via a type-scoped metadata query and match on each document's filename field". That shape ran successfully against the real store today, during the incident this PR is about.

Live evidence, stated precisely so I don't overstate it. Querying that deployment today I got:

Error listing files: List files failed: Searching by attribute "name" requires a fulltext index.

That is storage-search, not collection Query.equal — a different call, and I am not claiming it proves your query fails. What it does establish is that this deployment enforces index requirements on attribute lookups, which is exactly the assumption your soft edge rests on being false.

Why it matters more than a degraded-mode annoyance. Your failure direction is right — UNVERIFIED rather than a manufactured outage. But if the query is not supported, the guard returns UNVERIFIED on every delivery, forever. The bug ships, the guard is present, the logs say "could not ask", and the next person reads the guard's existence as coverage. A guard that structurally cannot fire is the exact failure this platform has been finding all week.

Two ways out, and the second needs no provisioning change:

• declare the index in pipeline-core's provisioning (cross-repo, slower), or
• use faoapi's proven shape — type-scoped query, match filename in Python. Same guarantee, no index dependency, and demonstrated against this store today.

I'd take the second and keep the soft edge as genuine belt-and-braces rather than the expected path.

---

THE SEAM — I think it is right, for a reason independent of the budget

You flagged that it was chosen under line-budget pressure, which is the right instinct to be suspicious of. The test I'd apply: would you choose this seam with no budget at all?

I think yes. The logic is identical in both partners, it must not diverge, and findability.py takes a resolve callable rather than a store — which is what makes its refusals testable without Appwrite. That is a good seam on its own merits, and the budget merely forced the question earlier.

One small thing, not blocking: the module lives at delivery/findability.py while the thing it guards is a wire-contract property. contract/wire/ might be the more natural home. Weak opinion, and moving it now for tidiness would be churn.

And I agree with not extracting the partner seam (C-33) in this PR. "Extracting under time pressure on the back of an incident is how the wrong seam gets frozen in" is exactly right.

---

Two things you got right that are worth naming

The wrong branch catches the bug directly. found != exp is the check that specifically catches content-hash dedup, because dedup returns a real id for the wrong document. A pure existence check would pass; this does not. Neither of us named that in the discussion and it is the sharpest part of the implementation.

Your remedy-is-the-trigger line does not overstate it. You asked. I verified independently: contract/wire/naming.py:19-31 embeds the run id in every filename, and C-105/C-22 do prescribe re-running. It is accurate, and it is the single most important sentence in this change — a re-publish was proposed as the fix on the day and was not taken only because we stopped to establish the selection behaviour first.

---

What I cannot verify from here

Whether Query.equal("filename", …) actually succeeds against the live collection. I have the negative evidence above but not the direct test, and I am not going to run write-path experiments against a partner-visible store to find out.

Verdict: approve with the filename-queryability finding addressed. Everything else holds, the mutation coverage is right, and the incident-as-regression-test is the correct way to keep this closed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GG2cY2HaqmUMpwUyR5V82K

[Polichinel]

Re-reviewed. Approve. The finding is addressed and the change is better than the one I suggested.

Verified rather than accepted

• Query.equal("filename", …) on the collection: still zero across all three repos.
• Your fourth fact holds — pipeline-core/file.py:1591 is Query.equal("name", filename) against the storage bucket. Different object, different call. So "zero hits" was not an artefact of my search; every filename lookup on this platform goes to storage and none to the collection attribute. That does make the case stronger than I put it, and it is the kind of correction worth having in the record.
• The regression guard is real: assert "filename" not in filters with the index reasoning in the failure message, plus len(seen) == 3 and the three query shapes named explicitly. M1 catches a revert.

You were right that it is better on its merits, not just a workaround

"It asks the question the consumer literally asks, which is C-94's thesis rather than an approximation of it" — that is the argument I should have made and did not. I proposed it as a way around an index dependency; you took it because it is the correct question. 110 lookups collapsing to 3 is a consequence, not the reason, and framing it that way in the code is right.

One thing I checked and withdraw

I was going to raise that the manifest might not be covered by objects, since the three-query test names only shard, sidecar and historical. It is covered — _upload appends every artefact including the manifest to uploaded, and uploaded_objects is built from all of it. There is also a test carrying sampled_forecast_manifest through the object path. Withdrawn — no action.

On the budget, which you volunteered

Both packages came in under where they started (690→688, 697→695 equivalent) and you rejected moving store_port.py out despite it being the easier win, because six register entries, an anti-drift test and ADR-015 reference it. Recording the reasoning in the commit rather than the conclusion is the right call, and "that blast radius does not belong inside an incident fix" is the same principle as declining the C-33 extraction. Both good.

What this PR does not close, for the record

It makes a silent non-write visible at the postprocessor. It does not stop the non-write — that is views-pipeline-core#551, and the two are complementary as you said. And neither closes #313, which is the only one of the three that would remove the failure class rather than detect it.

Nothing further from me. Good change, and the review genuinely improved it in both directions.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GG2cY2HaqmUMpwUyR5V82K

[Polichinel]

Both of your follow-up questions answered from the code rather than from having watched it. One is clean, one is a real finding.

1. Pagination — no truncation. Your guard is safe.

search_files_by_metadata (views-pipeline-core modules/appwrite/file.py:986+) pages properly, and this was fixed deliberately as register C-241, whose comment describes exactly the failure you feared:

list_documents returns APPWRITE_DEFAULT_PAGE_SIZE rows unless a Query.limit is supplied. This method used to omit one, so a match of more than 25 documents came back silently truncated — and every caller treated the truncation as the whole answer.

The walk is now careful in the two ways that matter, and its own comment says why:

The walk terminates on an EMPTY page rather than on a short one, and advances the offset by what it RECEIVED rather than by what it asked for.

DEFAULT_PAGE_LIMIT = 100, APPWRITE_DEFAULT_PAGE_SIZE = 25. So your 108-shard type-scoped query returns all 108. No false-missing alarm. views-faoapi hit the same default from the other side (their #287) and pages the same way.

2. "First wins" is arbitrary, not newest — this one needs a decision

The walk issues:

queries + [Query.limit(DEFAULT_PAGE_LIMIT), Query.offset(offset)]

No ordering. No Query.orderDesc, no orderAsc. So document order within a page is whatever the backend returns, and your "first fileId per filename" resolves to an unspecified document rather than the newest one.

That matters in exactly the case you were right to flag. If two documents can share a filename, "first wins" is not a tie-break rule — it is a coin flip whose bias you cannot see, and it may differ between runs against an unchanged store.

Three options, and I do not think this blocks the merge:

• Order explicitly — add a descending sort on the timestamp field so "first" genuinely means newest. Most correct, needs the field to be sortable.
• Compare and refuse — if two documents share a filename, raise rather than pick. Consistent with the rest of this PR's philosophy: "cannot tell" resolves to loud, not to a guess.
• Document it as arbitrary — acceptable only if you can argue duplicate filenames should not occur, which is the assumption report.py's note suggests is not safe.

I lean to the second, for the same reason your _dedup_target_is_the_same_artefact returns False on a missing name: on a partner-visible store, an unresolvable ambiguity is better raised than resolved by luck. But it is your call and the current behaviour is not wrong, only underdetermined.

Otherwise: approve, unchanged from my last pass

The six mutations, the regression test carrying the index reasoning in its failure message, and the budget outcome (both packages under where they started) all hold. Nothing else from me.

One thing I should say plainly, since you have taken every correction I have offered today: this PR is now materially better than the change I reviewed this afternoon, and both improvements came from you pushing back on your own first answer rather than from the review. The filename-query shape and the wrong branch were both yours.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GG2cY2HaqmUMpwUyR5V82K

[Polichinel]

Re-reviewed after the /code-review round. Approve. One correction to hand back, because I think it goes the other way from what you were told.

The polarity bug is the best self-catch of the day

The parse sitting outside the try — so that a doc.get failure yields an empty seen, and an empty seen reports every object missing and fires the "nothing in this run is servable" refusal on a healthy delivery — is precisely the failure this module argues against, twice, in C-99's and C-103's words.

A guard that quarantines a good delivery is worse than no guard, because it trains the operator to route around it. Catching it before merge, and mutation-proving the fix by moving the parse back out, is exactly right. So is the summary.get("uploaded_objects", []) → declared key access: a silent fallback that shrinks the guard to one historical object while logging "preflight passed" is the same shape as the bug being fixed, one level up.

Correction — I do not think pipeline-core sorts by $createdAt

You wrote that first-wins is correct because "pipeline-core sorts by $createdAt descending, so first-wins-per-filename reproduces the consumer's own selection". I cannot find that sort, and the evidence points the other way. Stating my working so you can overturn it:

• search_files_by_metadata (modules/appwrite/file.py:986+) builds its query as queries = [] then appends only Query.equal(attribute, value) per filter, and issues queries + [Query.limit(DEFAULT_PAGE_LIMIT), Query.offset(offset)]. No order_desc, no order_asc, no sort field.
• The only ordering calls in the entire module are at file.py:2643-2645, and they live inside list_files(bucket_id, …) — a storage listing, not a collection query — where order_field defaults to None anyway.
• grep '\$createdAt' across the module returns two hits, both in list_files' docstring and example.

So unless your documents() port reaches the collection by some path other than search_files_by_metadata, document order within a page is unspecified and first-wins is a coin flip rather than a tie-break.

This does not change my approval — the behaviour is the same either way, and unspecified-but-usually-fine is not a defect. What I would not want is the rationale going into the code as fact. If it is written down as "reproduces the consumer's own selection", the next person relies on an ordering guarantee that does not exist.

You can settle it faster than I can — you can see which method your port actually calls, and I cannot from outside. If it does route through something that sorts, I am wrong and I would like to know.

The growing-window finding is a good one

That the walk enumerates the collection once per artefact type, that SEARCH_INCOMPLETE surfaces as UNVERIFIED (right polarity), and that the collection grows ~110 documents per run so the window widens monotonically — that is a real property nobody had named, and recording it on #312 for a C-94 note rather than patching it is the right call. Retrying would be guessing at a store semantic neither of us owns.

Everything else

documents() gaining a test at the port, the four-vs-five method docstring, and unverified() no longer hardcoding "leg" are all correct and none of them change the shape.

Verdict: approve. Merge whenever the rest of your ritual is done; the ordering point is a docs/rationale matter, not a blocker, and it is yours to resolve either way.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GG2cY2HaqmUMpwUyR5V82K