fix: fetch_by_subject returns every source under the subject, reducing nothing

[thedavidmeister]

Refs #215. Not Closes — see "What this does not do" below.

The defect

DotrainSourceV1::fetch_by_subject returned decoded_items[0] of
metabytes[0] — a positional pick out of whatever rows the indexer happened to
hand back, ignoring every other row and every other item. The issue names two
consequences:

1. MetasBySubject carried no orderBy, so which row is "first" is
   indexer-implementation-defined.
2. Nothing checked the returned source against the subject.

The change

fetch_by_subject walks every meta the subject carries and every item in each,
and returns every DotrainSourceV1 it finds. It reduces nothing.

Result<Option<Self>, Error> -> Result<Vec<Self>, Error>.

MetasBySubject gains orderBy: id, orderDirection: asc.

Why it reduces nothing

Nothing says the sources under one subject must agree. IMetaV1_2 documents
subject as the entity the metadata is about — context specific, MAY be an
address, MAY be anything else. Every cardinality in that relation is 1:N: one
sender emits many metas about many subjects, and one subject carries many metas
from many senders. Plurality under a subject is ordinary valid data and needs no
bad actor to produce it.

Any reduction rule — first row, newest row, agreement-or-error, dedup — picks an
axis and decides for the caller which one matters, and destroys information the
caller may want. So this function reports what is there; the choice belongs to
whoever asked.

The first revision of this PR returned Ok(Some) only when every source found
agreed and Err(AmbiguousSubject) when they did not. That is a reduction rule,
and it fails for the reason above: it turns ordinary valid data into an error.
It is gone, along with Error::AmbiguousSubject and its Display arm —
crates/cli/src/error/mod.rs is byte-identical to main again.

The return shape

Result<Vec<Self>, Error>, in scan order: rows in the order the query pins,
items within a blob in the order they were encoded.

• Vec, not Option. Option cannot hold more than one, which is what forced
  the choice in the first place. [] is the old Ok(None) and [x] the old
  Ok(Some(x)), so no case a caller could previously see has disappeared.
• Not deduplicated. Two senders emitting the same source is two facts about the
  subject, and how many there are is the caller's to read.
• Not sorted, not re-keyed. Sorting by content or by sender picks an axis; a
  caller can sort by whatever it wants, and can only do so if what it receives
  is in a stated order rather than one this function invented.
• Not a HashSet or a map: keying by content dedups, keying by sender collapses
  one sender's repeated emissions. Both are the reduction under another name.
• Errors still propagate. A row that fails to CBOR-decode, or an item whose
  payload is not UTF-8, is an Err, not a silently shorter vec. The only
  Ok(vec![]) is "the subject carries no dotrain source".

Two limits of the shape, stated rather than hidden:

• It carries no per-row provenance — no sender, metaHash or meta id — so a
  caller cannot filter by emitter, which is exactly the kind of choice this PR
  is trying to hand back. That is not a reduction chosen here: the client drops
  it first, MetaboardSubgraphClient::get_metabytes_by_subject returning
  Vec<Vec<u8>> and nothing else. Surfacing it is a change to a second public
  API and is not in scope.
• metaV1S defaults to first: 100 and this client does not paginate, so for a
  subject carrying more than 100 metas the vec is every source in the first
  window, not every source. That gap is neither created nor closed here; the
  orderBy pin at least makes which window a function of subgraph state
  rather than of indexer implementation. The doc comment says so.

MetaV1.id is now board address ++ zero-padded counter (#227, fixed on main
and merged into this branch), so orderBy: id asc groups rows by metaboard and
runs chronologically within each.

Callers

There are none to fix outside the tests: a GitHub code search for
fetch_by_subject org:rainlanguage returns exactly this one file. Downstream
consumers of the published crate see a signature change, with [] and [x]
standing in for the old None and Some(x).

What this does not do — why Refs, not Closes

Violated property 2 is verification of the source against the subject, and it is
not implemented. It cannot be settled from inside this PR:

• Must the subject of a DotrainSourceV1 lookup be a hash of the content at
  all? IMetaV1_2 says a subject "MAY be the address of the emitting contract
  … OR anything else", and LibDescribedByMeta.emitForDescribedAddress in this
  same repo emits under an address subject. A content-hash check would reject
  that shape outright.
• If it must be a content hash, which one? Dotrain source subject is derived two ways: generate emits under keccak(cbor item map) while DotrainSourceV1::hash and the subject field docs say keccak(content) #219 and DotrainSourceV1::hash() (raw utf8 keccak) diverges from the metaboard emit subject (cbor document keccak) #158 record that this crate
  derives the dotrain subject two different ways —
  generate_dotrain_source_emit_tx_data emits under keccak256(cbor(item)),
  while DotrainSourceV1::hash() and the DotrainSourceEmitData.subject doc
  say keccak256(content) — and both are explicitly left unadjudicated there.

So a subject carrying an attacker-emitted source still returns it, now
alongside every other source rather than instead of them. That half of #215
stays open, and the collection point this PR adds is where the filter goes once
the convention is ruled.

Relationship to #252 (issue #159)

#252 is open against the same function and also makes it scan every meta and
item, leaving "which meta wins" to this issue. The answer here is that none of
them wins and all are returned, so the two PRs conflict textually in this
function; the resolution is this file's version. Two deliberate differences:
#252 recurses into nested RainMetaDocumentV1 payloads and this does not (and
does not remove that recursion if #252 lands first — #226 tracks the unbounded
recursion in the sibling extractor); and #252 keeps first-item-wins inside one
blob, which is the rule this PR removes.

Tests

Re-pinned:

• test_fetch_by_subject_found / _not_found — same fixtures, asserting a
  one-element and an empty vec.
• test_fetch_by_subject_wrong_magic ->
  test_fetch_by_subject_only_other_meta_types_yields_nothing. This was
  Err(InvalidMetaMagic) whenever the first item happened to be another meta
  type; the NatSpec already promised found/not-found.
• test_fetch_by_subject_takes_first_decoded_item ->
  test_fetch_by_subject_returns_every_item_in_one_meta: two different sources
  in one blob, both returned in encoding order.

New:

• test_fetch_by_subject_returns_divergent_metas_in_row_order — the issue's
  repro, two metas holding two different sources, run in both row orders. Both
  sources come back, and the vec follows the rows: divergence is an expected
  result, not an error, and nothing is reordered.
• test_fetch_by_subject_keeps_duplicate_sources — the same source twice comes
  back twice.
• test_fetch_by_subject_ignores_other_meta_types_alongside — a non-dotrain
  meta ahead of the dotrain one no longer errors and is not counted.
• test_get_metabytes_by_subject_success gains orderBy: id and
  orderDirection: asc to its existing wire-shape pins.

QA

All runs under nix develop -c, on the merge of origin/main into this branch.
Green: cargo test -p rain-metadata --lib source_v1::tests 18 passed 0 failed;
cargo test -p rain-metadata --lib error:: 7 passed 0 failed; cargo test -p rain-metaboard-subgraph --lib 12 passed 0 failed; cargo clippy --workspace --all-targets -- -D warnings clean; cargo fmt --all -- --check clean. The
full suite was NOT run locally — this box is shared by many agents; CI runs it.

• Discriminating tests: the pre-PR implementation cannot be restored under this
  signature, so the discriminators are recorded as mutations of the shipped
  code, including one that restores the old first-wins selection on top of the
  new signature (M7).

• Mutations applied to fetch_by_subject, 9 applied / 9 killed / 0 survivors:

  • for meta_bytes in metabytes -> .into_iter().take(1) — killed by
    ..._returns_divergent_metas_in_row_order, ..._keeps_duplicate_sources,
    ..._ignores_other_meta_types_alongside.
  • for item in cbor_decode(&meta_bytes)? -> .into_iter().take(1) — killed
    by ..._returns_every_item_in_one_meta.
  • if item.magic != KnownMagic::DotrainSourceV1 -> if false — killed by
    ..._only_other_meta_types_yields_nothing,
    ..._ignores_other_meta_types_alongside.
  • sources.dedup_by(|a, b| a.0 == b.0) inserted before the return — killed by
    ..._keeps_duplicate_sources.
  • sources.sort_by(|a, b| a.0.cmp(&b.0)) inserted before the return — killed
    by ..._returns_divergent_metas_in_row_order.
  • the Err(Empty(_)) => Ok(vec![]) arm deleted — killed by ..._not_found.
  • (M7) Ok(sources) -> Ok(sources.into_iter().take(1).collect()), the old
    first-wins rule on the new signature — killed by
    ..._returns_every_item_in_one_meta,
    ..._returns_divergent_metas_in_row_order, ..._keeps_duplicate_sources.
  • orderDirection: asc -> desc — killed by
    test_get_metabytes_by_subject_success.
  • orderBy: id, orderDirection: asc dropped — killed by the same.

• Oracle: IMetaV1_2's definition of subject and the 1:N cardinality it
  implies, plus the issue, not the implementation. Expected order-independence
  of the set and order-faithfulness of the vec are both asserted by running
  one fixture in both row orders. Wire-shape strings are the GraphQL the
  subgraph schema defines (MetaV1_orderBy.id, OrderDirection.asc), pinned
  the way the sibling where: {metaHash: $metahash} pin already is.

• Category check: the issue asks for (A) deterministic ordering on
  MetasBySubject and (B) verification of the returned source against the
  subject. A is covered twice over — the result no longer depends on row order
  for what it contains, and the orderBy/orderDirection the issue names are
  added. B is NOT covered, deliberately, for the reasons above. Hence Refs.

[coderabbitai]

Warning

Review limit reached

Next included review available in 22 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7d475712-5a7c-4cac-b609-ecd176d04759

📥 Commits

Reviewing files that changed from the base of the PR and between abcaa74 and 6befddb.

📒 Files selected for processing (3)

• crates/cli/src/meta/types/dotrain/source_v1.rs
• crates/metaboard/src/metaboard_client.rs
• crates/metaboard/src/types/metas.rs

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}