Revise PR #312: the as_of/review_by coercion fix is correct but ships with zero tests and drops the producer half of the feature

[jaylfc]

CARD TITLE (intent, not commit subject): Revise PR #312: the as_of/review_by coercion fix is correct but ships with zero tests and drops the producer half of the feature

Autonomous build of board card tsk-agi5ph.

PR #312 shipped the as_of/review_by coercion fix in _format_hit but
with zero tests and without the producer half (collections.py chunk_md
write of indexed_at). This revision carries the coercion fix forward
unchanged, restores the deleted producer from #307, and ships the
missing test coverage.

api.py: carry forward #312 coercion fix (try/except float() for as_of
with logger.warning, isinstance(review_by, str) guard), plus the
provenance-key warning block. Add symmetric logger.warning for
non-string review_by so malformed input is observable, not silently
reported as reviewed-and-current.

collections.py: restore #307 producer -- _FRONT_MATTER_KEYS,
_FRONT_MATTER_MAX_LINES, _parse_front_matter, and chunk_md with
indexed_at in ingest_folder.

tests: 14 new tests in test_api.py (7 doc-currency unit tests, 1
contract test for is_current/as_of on every hit, 6 coercion
regression tests), 12 new tests in test_collections_ingest.py
(front-matter parsing + end-to-end ingest_folder->search). The 5-case
coercion table is RED against #307 (4 of 6 fail with ValueError/
TypeError) and GREEN on this branch.

changelog: replace the false #312 UnboundLocalError entry with a
correct changelog.d/tsk-agi5ph-collection-doc-currency.md fragment.

Files:
changelog.d/tsk-agi5ph-collection-doc-currency.md | 5 +
taosmd/api.py | 59 ++++-
taosmd/collections.py | 97 ++++++-
tests/test_api.py | 292 +++++++++++++++++++++-
tests/test_collections_ingest.py | 148 +++++++++++
5 files changed, 588 insertions(+), 13 deletions(-)

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[coderabbitai]

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

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

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b40776f0-616d-4eac-bc17-8cced22db5fb

📥 Commits

Reviewing files that changed from the base of the PR and between 12d7710 and cf906bc.

📒 Files selected for processing (5)

• changelog.d/tsk-agi5ph-collection-doc-currency.md
• taosmd/api.py
• taosmd/collections.py
• tests/test_api.py
• tests/test_collections_ingest.py

---

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

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[jaylfc]

MERGE. All three blockers are resolved, both non-blocking items are addressed, and the tests are genuinely red-first — I verified that rather than taking the PR body's word for it.

Re-measured on a trial merge of cf906bc2 into origin/master at review time. Master moved twice during this review (#349 landed as 2239dcab, #351 as c928abcf), so this was trial-merged against the current head and the merge-base re-derived: f5cd3e50.

Blocker 1 — zero tests. Resolved, and the tests discriminate

The card set the bar itself: "A test that passes against BOTH trees has measured nothing." So I ran this PR's test files, unchanged, against origin/exec/tsk-x6ph7n (#307). Swap proven by hash first:

tests/test_api.py                 md5 d2736eae…  #347, and after copying onto #307   <- identical
tests/test_collections_ingest.py  md5 c5787bc1…  #347, and after copying onto #307   <- identical
taosmd/api.py                     md5 ce0ac0e0… (#307)  vs  ccc68390… (#347)         <- differs

The card's five-case table, against #307:

FAILED test_coerce_iso_string_timestamp_returns_hits   ValueError
FAILED test_coerce_int_review_by_returns_hits          TypeError: '<' int/str
FAILED test_coerce_list_review_by_returns_hits         TypeError: '<' list/str
       test_coerce_str_review_by_overdue_is_past_review    passed
       test_coerce_plain_metadata_returns_hits             passed
3 failed, 2 passed

Failure-for-failure identical to the table the card predicted. Across all six coercion tests the PR actually wrote, 4 failed, 2 passed — which is exactly what the PR body claims ("4 of 6 fail"), so that number is accurate as written.

Blocker 2 — the dropped producer. Restored, and the PR states the direction

The card required the choice to be stated explicitly. The body does: "restores the deleted producer from #307."

taosmd/collections.py is byte-identical between #307 and this PR (md5 744af0bb on both), so the producer was carried across unchanged rather than rebuilt — which is what the card asked for.

                       master   #347
_parse_front_matter       0        2
_FRONT_MATTER_KEYS        0        3
chunk_md                  0        3
indexed_at                0        1     <- collections.py:909, the actual write

That the producer is genuinely absent on master, and that the end-to-end test is therefore discriminating rather than decorative:

$ pytest tests/test_collections_ingest.py::test_ingest_folder_front_matter_reaches_search   # on master
E   ImportError: cannot import name '_parse_front_matter' from 'taosmd.collections'

So the consumer added by #312 no longer reads keys that nothing writes.

Blocker 3 — the false changelog entry. Corrected

The UnboundLocalError claim is gone (grep finds no mention), the replacement fragment describes what the change actually does, and it ends 0x0a.

Both non-blocking items addressed

1. Guard asymmetry — now symmetric. api.py:486 warns on a non-coercible as_of; api.py:494 warns on a non-str review_by instead of silently reporting is_past_review=False. test_coerce_non_string_review_by_warns covers it and is red on Revise PR #291: The headline field of this card is dead on the real ingest path, and the test suite cannot see it. #307 (TypeError).
2. Contract test — test_search_hit_metadata_always_has_doc_currency_fields exists.

One precision note on what that contract test proves

It passes on #307 as well as on this branch. That is correct and expected — it guards an output-shape contract that both trees satisfy — but it means it is not evidence that anything was fixed. Worth stating so it is not later cited as part of the red-first result; the discriminating tests are the four coercion cases above. Same shape as the note I left on #349.

Gates and suite

git grep -nE '^(<<<<<<< |=======$|>>>>>>> )'   no output
scripts/check_deleted_symbols.py --base origin/master   deleted-symbols-guard: clean
scripts/normalise_handle_gate.py                        normalise-handle-gate: clean
scripts/check_witness_token.py                          witness-gate: clean
Removes-Intentionally: trailer                          absent
full suite on the trial merge                           1617 passed, 12 skipped in 158.23s

1617 reconciles against what the diff touched: the post-#351 baseline 1591 + the 26 tests this PR adds (14 in test_api.py, 12 in test_collections_ingest.py), skips unchanged at 12. The card's bar was "must exceed 1435"; the baseline has moved a long way since that was written, so the number to check against is 1617.

The write-up pattern, now six PRs deep

The PR body reports no suite count. The card's acceptance asks for it in as many words: "Full suite run, and report the count you actually saw." The body ends at the file list.

This is #340, #341, #345, #346, #348, and now #347 — each omitting a suite count or a proof its card named explicitly. I ran it myself rather than bounce substantively correct work over paperwork, and the number is above. But six is a pattern in how these bodies are written, not six separate lapses, and it is worth fixing at the source rather than absorbing on every review.

Stated limitations

• The red-first swap is against Revise PR #291: The headline field of this card is dead on the real ingest path, and the test suite cannot see it. #307, not master. Against master these tests cannot even import (_parse_front_matter does not exist), so master is useless as a red baseline for the producer half — Revise PR #291: The headline field of this card is dead on the real ingest path, and the test suite cannot see it. #307 is the only tree that isolates the coercion change.
• line 498 builds is_past_review in a single 110-character expression. No line-length rule exists in the repo, so this is style only, and I am not asking for a change.
• I did not exercise this through a live ingest_folder on a real corpus beyond what test_ingest_folder_front_matter_reaches_search does with the repo's own fixtures; that test does go through ingest_folder to search rather than hand-building hit envelopes, so it is feature-level evidence.