.build_suggestions() fired only when a category returned zero rows in a requested year. Public Welfare is the failure mode that missed: E74/E79 still return rows for legacy years, so no row-absence gap existed, while aggregate-published E67/E68 were dropped by spending_long's NOT is_aggregate filter. LA County FY2011 reported $3,185,943,000 and omitted $2,075,461,000 -- a 39% understatement -- with provenance$suggestions empty.
A recipe now also qualifies when a component code carries dollars the verb's own long view structurally excludes, measured per government by anti-joining the real view. Each suggestion carries trigger, suppressed_amount, suppressed_years and suppressed_codes.
Blast radius measured on the bundled fixture: every fire lands in the wide era, none in 2012/2019/2020, and the leaf-and-classified control families (higher_ed_e18_wide, general_gov_e89_wide) stay silent.
Closes #9.
`.build_suggestions()` fired only when a category returned zero rows in a requested year. Public Welfare is the failure mode that missed: E74/E79 still return rows for legacy years, so no row-absence gap existed, while aggregate-published E67/E68 were dropped by `spending_long`'s `NOT is_aggregate` filter. LA County FY2011 reported $3,185,943,000 and omitted $2,075,461,000 -- a 39% understatement -- with `provenance$suggestions` empty.
A recipe now also qualifies when a component code carries dollars the verb's own long view structurally excludes, measured per government by anti-joining the real view. Each suggestion carries `trigger`, `suppressed_amount`, `suppressed_years` and `suppressed_codes`.
`cog_revenue()` inherits it: Alaska FY2011 Miscellaneous Revenue dropped $1,899,995,000 of `U4-`.
Blast radius measured on the bundled fixture: every fire lands in the wide era, none in 2012/2019/2020, and the leaf-and-classified control families (`higher_ed_e18_wide`, `general_gov_e89_wide`) stay silent.
.inform_suggestions() and cog_explain() now render suppressed_amount /
suppressed_years / suppressed_codes as a continuation line on each
suggestion bullet whenever suppressed_amount > 0 (an empty_year fire can
carry them too, so this keys off the amount, not trigger). Also renames
the cli header from "Coverage gap detected" to "Incomplete coverage" --
a partial-coverage fire is not a gap, the year has rows, they're just
short.
devtools::document() should not change DESCRIPTION when only JSON/Markdown/test files are edited. Restore the original RoxygenNote: 7.3.3 and remove the Config/roxygen2/version auto-generated line that resulted from running devtools::document() locally.
Reviewed the diff across 7 files (+1518 / -37 lines). Here are my findings:
✅ Strengths
Well-scoped fix: The core change to .build_suggestions() cleanly adds a second qualifying path (suppressed_component) alongside the existing empty_year check, without modifying the candidate query or IG-counterpart logic.
SQL safety: .suppressed_components() uses .sql_lit_chr() for all string interpolation and validates long_view against an explicit allowlist (spending_long, spending_long_harmonized, revenue_long, revenue_long_harmonized) before interpolating it as a SQL identifier. Good defense-in-depth.
Schema documentation: provenance-v1.json properly documents the four new fields (trigger, suppressed_amount, suppressed_years, suppressed_codes) with clear descriptions and types.
Test coverage: 201 new test lines in test-recipes.R — solid coverage for the new path.
User-facing messaging: .inform_suggestions() and cog_explain() both render the suppressed dollar amounts clearly. The message change from "Coverage gap" to "Incomplete coverage" is more accurate.
⚠️ Observations
No CI run detected for this PR: I checked the Actions API (last 30 runs) and found no CI run matching this PR. The most recent PR-related runs are for #31 (merged), schema_version 7, and the apt-https fix. Either CI hasn been triggered yet or there is no workflow configured for this branch. Action needed: verify CI triggers on non-main branches.
Plan file included in PR (plans/2026-08-04-partial-coverage-signposting.md, 1061 lines): This is a detailed implementation plan. If this is meant as internal documentation for future reference, it is fine. If it was a working document for the implementation process, consider whether it should be in .gitignore or moved to a wiki.
File size: R/suggestions.R grows from ~251 to ~384 lines (still under the 400-line constraint noted in the plan). Acceptable, but close to the limit.
Summary
The fix is well-architected and addresses a real data integrity issue (39% understatement for LA County Public Welfare). The main concern is the missing CI signal — without a green check, I cannot verify the tests actually pass. Please confirm CI status.
— Kodor 🤖
@jared **Kodor PR Review — #32: fix: signpost partially-suppressed categories**
Reviewed the diff across 7 files (+1518 / -37 lines). Here are my findings:
## ✅ Strengths
1. **Well-scoped fix**: The core change to `.build_suggestions()` cleanly adds a second qualifying path (`suppressed_component`) alongside the existing `empty_year` check, without modifying the candidate query or IG-counterpart logic.
2. **SQL safety**: `.suppressed_components()` uses `.sql_lit_chr()` for all string interpolation and validates `long_view` against an explicit allowlist (`spending_long`, `spending_long_harmonized`, `revenue_long`, `revenue_long_harmonized`) before interpolating it as a SQL identifier. Good defense-in-depth.
3. **Schema documentation**: `provenance-v1.json` properly documents the four new fields (`trigger`, `suppressed_amount`, `suppressed_years`, `suppressed_codes`) with clear descriptions and types.
4. **Test coverage**: 201 new test lines in `test-recipes.R` — solid coverage for the new path.
5. **User-facing messaging**: `.inform_suggestions()` and `cog_explain()` both render the suppressed dollar amounts clearly. The message change from "Coverage gap" to "Incomplete coverage" is more accurate.
## ⚠️ Observations
1. **No CI run detected for this PR**: I checked the Actions API (last 30 runs) and found no CI run matching this PR. The most recent PR-related runs are for #31 (merged), schema_version 7, and the apt-https fix. Either CI hasn been triggered yet or there is no workflow configured for this branch. **Action needed**: verify CI triggers on non-main branches.
2. **Plan file included in PR** (`plans/2026-08-04-partial-coverage-signposting.md`, 1061 lines): This is a detailed implementation plan. If this is meant as internal documentation for future reference, it is fine. If it was a working document for the implementation process, consider whether it should be in `.gitignore` or moved to a wiki.
3. **File size**: `R/suggestions.R` grows from ~251 to ~384 lines (still under the 400-line constraint noted in the plan). Acceptable, but close to the limit.
## Summary
The fix is well-architected and addresses a real data integrity issue (39% understatement for LA County Public Welfare). The main concern is the missing CI signal — without a green check, I cannot verify the tests actually pass. Please confirm CI status.
— Kodor 🤖
Final whole-branch review fix wave for the partial-coverage signposting
feature:
- I1: .suppressed_components() now filters measured recipe components to
the calling verb's own flow_prefixes. Without this, a candidate recipe
from the OTHER flow family was always absent from the verb's own view by
construction and so was always reported as "suppressed" -- fabricating a
dollar claim across flow families (cog_revenue(category = "Corrections")
claimed $3.63B excluded that cog_spending() actually reports in full).
- I2: reworded provenance-v1.json's trigger/suppressed_amount descriptions
to describe what the code actually measures (the verb's underlying long
view, not "the result"), and to note suppressed_amount can be negative.
Added ig_recipe_id to the suggestions items' required list, matching the
key's always-set/nullable runtime behavior.
- I3(a): restated the year/govid literals inside .suppressed_components()'s
NOT EXISTS subquery so DuckDB can partition-prune that side too (verified
via EXPLAIN: Scanning Files 1/4 instead of an unfiltered full scan;
all.equal(old, new) results confirmed unchanged).
- I3(b): added .needs_suppression_query(), a free, exact pre-check reusing
the verb's own already-computed result$codes_included to skip the anti-
join round trip on the common fully-covered path, without weakening the
"suppression can fire with zero gap years" guarantee.
- M4: corrected the overbroad "confines every fire to 2011" scope claim in
R/suggestions.R and NEWS.md -- the suppressed-dollar measurement is now
flow-scoped (post-I1), but the empty_year trigger itself is not, and can
still fire in modern years for a mis-scoped cross-flow-family category.
Added a regression test for I1 plus direct unit-test coverage for the new
flow-family filter and the I3(b) pre-check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scoped re-review measured .needs_suppression_query() against the fixture
and found it doesn't pay for itself: it skips the round trip on ~3% of
healthy candidate-bearing calls, ~0% of the multi-govid batch shape
(cog_geographic_rollup()/cog_peer_compare()) it was meant to help, and
reaching the gate costs an unconditional metadata query that on its own
roughly cancels the expected saving -- net slower on the fixture. The
gate was also correct (0 unsound skips) but left an untested exactness
invariant (result$codes_included and the anti-join sharing the harmonized
item_code space) whose silent violation would kill signposting, which is
the exact failure class uscogdata#9 exists to prevent.
Owner's call: revert it and keep the code simple. A batch-aware
optimization, if warranted, is a separate issue.
Removes .needs_suppression_query() entirely (function, roxygen, call
site, comp_rows/flow_components), restoring .build_suggestions() to call
.suppressed_components() directly -- unchanged from f77adb6 except that
it still threads flow_prefixes through (I1, kept). Also removes the two
tests that existed solely to exercise the gate (the five-branch synthetic
test and the local_mocked_bindings call-counter test); no test asserting
real signposting behavior was touched.
I1 (flow_prefixes filter), I2 (schema wording + ig_recipe_id required),
I3(a) (restated NOT EXISTS literals for partition pruning), and M4
(reworded scope claims) are all untouched by this revert.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
R/suggestions.R crossed the project's 400-line limit (424 lines).
Pure move of .suppressed_components() and its roxygen block per the
plan's Task 5 Step 2 remedy; no logic, SQL, or wording changed.
jared
merged commit 2fc9e7585b into main2026-08-05 08:15:47 -04:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #9.
.build_suggestions()fired only when a category returned zero rows in a requested year. Public Welfare is the failure mode that missed: E74/E79 still return rows for legacy years, so no row-absence gap existed, while aggregate-published E67/E68 were dropped byspending_long'sNOT is_aggregatefilter. LA County FY2011 reported $3,185,943,000 and omitted $2,075,461,000 -- a 39% understatement -- withprovenance$suggestionsempty.A recipe now also qualifies when a component code carries dollars the verb's own long view structurally excludes, measured per government by anti-joining the real view. Each suggestion carries
trigger,suppressed_amount,suppressed_yearsandsuppressed_codes.cog_revenue()inherits it: Alaska FY2011 Miscellaneous Revenue dropped $1,899,995,000 ofU4-.Blast radius measured on the bundled fixture: every fire lands in the wide era, none in 2012/2019/2020, and the leaf-and-classified control families (
higher_ed_e18_wide,general_gov_e89_wide) stay silent.@jared Kodor PR Review — #32: fix: signpost partially-suppressed categories
Reviewed the diff across 7 files (+1518 / -37 lines). Here are my findings:
✅ Strengths
Well-scoped fix: The core change to
.build_suggestions()cleanly adds a second qualifying path (suppressed_component) alongside the existingempty_yearcheck, without modifying the candidate query or IG-counterpart logic.SQL safety:
.suppressed_components()uses.sql_lit_chr()for all string interpolation and validateslong_viewagainst an explicit allowlist (spending_long,spending_long_harmonized,revenue_long,revenue_long_harmonized) before interpolating it as a SQL identifier. Good defense-in-depth.Schema documentation:
provenance-v1.jsonproperly documents the four new fields (trigger,suppressed_amount,suppressed_years,suppressed_codes) with clear descriptions and types.Test coverage: 201 new test lines in
test-recipes.R— solid coverage for the new path.User-facing messaging:
.inform_suggestions()andcog_explain()both render the suppressed dollar amounts clearly. The message change from "Coverage gap" to "Incomplete coverage" is more accurate.⚠️ Observations
No CI run detected for this PR: I checked the Actions API (last 30 runs) and found no CI run matching this PR. The most recent PR-related runs are for #31 (merged), schema_version 7, and the apt-https fix. Either CI hasn been triggered yet or there is no workflow configured for this branch. Action needed: verify CI triggers on non-main branches.
Plan file included in PR (
plans/2026-08-04-partial-coverage-signposting.md, 1061 lines): This is a detailed implementation plan. If this is meant as internal documentation for future reference, it is fine. If it was a working document for the implementation process, consider whether it should be in.gitignoreor moved to a wiki.File size:
R/suggestions.Rgrows from ~251 to ~384 lines (still under the 400-line constraint noted in the plan). Acceptable, but close to the limit.Summary
The fix is well-architected and addresses a real data integrity issue (39% understatement for LA County Public Welfare). The main concern is the missing CI signal — without a green check, I cannot verify the tests actually pass. Please confirm CI status.
— Kodor 🤖