Repository navigation
discord_analytics: no validation that d90 ≤ total per channel — silently renders a malformed stacked bar chart #452
Description
Activity
Coding Plan
Summary
- Add a per-row invariant check to
load_channels_df()that enforcesd30 <= d90 <= d365 <= totaland raises a descriptiveValueErrornaming the offending channel and values. - Reuse the existing fail-loud philosophy and the
serialization.pydescriptive-error style; iterate rows withdf.itertuples(). - Keep the change local to
discord_analytics.py. Do not add a shared utility, since no existing row-validation helper exists and the check is pipeline-specific. - Add two tests that mirror the existing loader test pattern: one for a valid snapshot and one for a violated row, asserted with
pytest.raises(ValueError, match=...).
Implementation Steps
Phase 1: Add loud per-row validation to the channels loader
This phase adds the invariant check to the channel CSV loader. The check runs after the CSV is read and before the DataFrame is returned. It follows the existing fail-loud philosophy and the codebase's descriptive-error convention.
Task 1: Add a per-row invariant validator for the channels DataFrame
Add validation logic that enforces
d30 <= d90 <= d365 <= totalfor each channel row.- Add a small module-level helper in
src/hiero_analytics/pipelines/discord_analytics.py(nearload_channels_df) that accepts the loaded DataFrame and validates the ordering invariant per row. - Iterate rows with
df.itertuples(), matching the existing codebase idiom. - For any row where
d30 <= d90 <= d365 <= totaldoes not hold, raise aValueError. - Format the error message to name the offending channel and the concrete values, following the
serialization.pyprecedent. Use a message such asf"Channel {row.channel!r} violates d30<=d90<=d365<=total: d30={row.d30}, d90={row.d90}, d365={row.d365}, total={row.total}". - Raise on the first violating row so the failure is immediate and specific.
Task 2: Call the validator inside `load_channels_df()`
Wire the validator into the loader so validation runs on every load.
- In
load_channels_df(), call the new validator afterpd.read_csv(path)and before thechannel_labelandcategoryderivations. - Keep the existing
path.exists()/FileNotFoundErrorguard unchanged; the new check is an additional, later guard. - Do not alter
load_monthly_df(); the ticket scope is the channels invariant only.
🤖 Prompt for AI agents
Implement per-row invariant validation in `src/hiero_analytics/pipelines/discord_analytics.py` so that `load_channels_df()` fails loudly on malformed rows instead of letting them silently produce a negative chart segment. - Add a small module-level helper function near `load_channels_df()` that takes the loaded DataFrame and checks, for every row, that `d30 <= d90 <= d365 <= total` holds. - Iterate rows using `df.itertuples()`, matching the existing codebase idiom for row iteration. - If a row violates the invariant, raise a `ValueError` immediately (on the first violating row) with a descriptive message that names the offending channel and the concrete field values, following the style used in `data_sources/serialization.py::deserialize_record`. Use a message of the form: `f"Channel {row.channel!r} violates d30<=d90<=d365<=total: d30={row.d30}, d90={row.d90}, d365={row.d365}, total={row.total}"`. - In `load_channels_df()`, call this new validator right after `pd.read_csv(path)` and before deriving the `channel_label` and `category` columns. - Keep the existing `path.exists()` / `FileNotFoundError` check unchanged; the new validation is an additional, later guard. - Do not modify `load_monthly_df()`; this change is scoped only to the channels loader.Phase 2: Add tests for the validation behavior
This phase adds test coverage for both the valid snapshot and the violated-row case, following the existing test patterns in the module.
Task 1: Add a passing-snapshot test
Confirm that a valid channels CSV loads without error.
- In
tests/pipelines/test_discord_analytics.py, confirm the existingchannels_csvfixture data satisfiesd30 <= d90 <= d365 <= total. If it does, the existing success test already covers the valid path; add an explicit assertion or a dedicated test that a valid snapshot loads and returns the expected rows. - Follow the existing fixture pattern: write CSV text via
tmp_path, setHIERO_DISCORD_CHANNELS_CSVwithmonkeypatch.setenv, and callrunner.load_channels_df().
Task 2: Add a violated-row failure test
Confirm that a channels CSV with a violating row raises a descriptive
ValueError.- Add a test that writes a CSV containing one row where
d90 > total(or another ordering violation). - Point the loader at the temp CSV via
monkeypatch.setenv, then callrunner.load_channels_df(). - Assert the failure with
pytest.raises(ValueError, match=...), matching a stable substring of the error message, such as the offending channel name or the phrased30<=d90<=d365<=total. - Place the new tests in the existing "Loader tests" comment-banner section to match file organization.
🤖 Prompt for AI agents
Add test coverage in `tests/pipelines/test_discord_analytics.py` for the new per-row validation, following the file's existing test patterns. - For the passing case: confirm the existing `channels_csv` fixture data satisfies `d30 <= d90 <= d365 <= total`. If it already does, the existing success test covers the valid path; otherwise, add an explicit assertion or a dedicated test that a valid snapshot loads and returns the expected rows. Follow the existing fixture pattern: write CSV text via `tmp_path`, set `HIERO_DISCORD_CHANNELS_CSV` with `monkeypatch.setenv`, and call `runner.load_channels_df()`. - For the failing case: add a test that writes a CSV containing one row where `d90 > total` (or another ordering violation). Point the loader at the temp CSV via `monkeypatch.setenv`, then call `runner.load_channels_df()`. Assert the failure using `pytest.raises(ValueError, match=...)`, matching a stable substring of the error message, such as the offending channel name or the phrase `d30<=d90<=d365<=total`. - Place both new tests in the existing "Loader tests" comment-banner section, to match the file's organization.Research
src/hiero_analytics/pipelines/discord_analytics.pyis a self-contained runner. It reads two gitignored CSV snapshots and produces three PNG charts.load_channels_df()resolves the CSV path, checkspath.exists()(raisingFileNotFoundErrorwith a descriptive message), reads the CSV withpd.read_csv, and derives thechannel_labelandcategorycolumns. It does not check columns or values.plot_category_breakdown()groups channels by category, sumstotalandd90, and computesearlier = total - last_90d. A row whered90 > totalproduces a negative segment with no error.The codebase favors loud failures over silent drift.
tests/contracts/test_output_contract.pydocuments this philosophy. The closest existing precedent for a loud, descriptiveValueErrorisdata_sources/serialization.py::deserialize_record, which names the entity, field, and offending value with!r.Row iteration in the codebase uses
df.itertuples().Tests in
tests/pipelines/test_discord_analytics.pyuse module-scope CSV text constants,tmp_pathfixtures,monkeypatch.setenvto point the loader at a temp CSV, andpytest.raises(..., match=...)for failure paths.
🚀 Next Steps
🤖 All AI agent prompts combined
Task: 1 Implement per-row invariant validation in `src/hiero_analytics/pipelines/discord_analytics.py` so that `load_channels_df()` fails loudly on malformed rows instead of letting them silently produce a negative chart segment. - Add a small module-level helper function near `load_channels_df()` that takes the loaded DataFrame and checks, for every row, that `d30 <= d90 <= d365 <= total` holds. - Iterate rows using `df.itertuples()`, matching the existing codebase idiom for row iteration. - If a row violates the invariant, raise a `ValueError` immediately (on the first violating row) with a descriptive message that names the offending channel and the concrete field values, following the style used in `data_sources/serialization.py::deserialize_record`. Use a message of the form: `f"Channel {row.channel!r} violates d30<=d90<=d365<=total: d30={row.d30}, d90={row.d90}, d365={row.d365}, total={row.total}"`. - In `load_channels_df()`, call this new validator right after `pd.read_csv(path)` and before deriving the `channel_label` and `category` columns. - Keep the existing `path.exists()` / `FileNotFoundError` check unchanged; the new validation is an additional, later guard. - Do not modify `load_monthly_df()`; this change is scoped only to the channels loader. =============================================================================== Task: 2 Add test coverage in `tests/pipelines/test_discord_analytics.py` for the new per-row validation, following the file's existing test patterns. - For the passing case: confirm the existing `channels_csv` fixture data satisfies `d30 <= d90 <= d365 <= total`. If it already does, the existing success test covers the valid path; otherwise, add an explicit assertion or a dedicated test that a valid snapshot loads and returns the expected rows. Follow the existing fixture pattern: write CSV text via `tmp_path`, set `HIERO_DISCORD_CHANNELS_CSV` with `monkeypatch.setenv`, and call `runner.load_channels_df()`. - For the failing case: add a test that writes a CSV containing one row where `d90 > total` (or another ordering violation). Point the loader at the temp CSV via `monkeypatch.setenv`, then call `runner.load_channels_df()`. Assert the failure using `pytest.raises(ValueError, match=...)`, matching a stable substring of the error message, such as the offending channel name or the phrase `d30<=d90<=d365<=total`. - Place both new tests in the existing "Loader tests" comment-banner section, to match the file's organization.💡 Iterate on the plan with:
`@coderabbitai` <feedback>Example Feedback - `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2. - `@coderabbitai` For design choice 1 go ahead with option 3 and replan.
- Add a per-row invariant check to
/assign
Thank you @shahd-gaafar
Reacted by shahd-gaafar- added a commit that references this issue
on Sep 27, 2026
The task
The task
Problem:
plot_category_breakdown()insrc/hiero_analytics/pipelines/discord_analytics.pyassumes each channel'stotal(all-time messages) is always ≥ itsd90(last-90-days messages), computingearlier = total - last_90dto build a stacked bar chart. Nothing validates that assumption when the CSV is loaded. Since the source file is a manually-exported snapshot (per the module's own docstring — not committed, columns can be pulled at different times), a single channel where this invariant is violated silently produces a negative "earlier history" value and a malformed chart, with zero errors or warnings anywhere in the pipeline. Confirmed with a real reproduction:What done looks like:
load_channels_df()validatesd30 ≤ d90 ≤ d365 ≤ totalper row before returning the DataFrame, and fails loudly (raises, with a message identifying the offending channel and values) when a row violates it — rather than letting a malformed row reach the chart silently. Add a test intests/pipelines/test_discord_analytics.pycovering both a valid snapshot and one with a violated row.Where to look first:
src/hiero_analytics/pipelines/discord_analytics.py—load_channels_df()already fails loudly on one bad-input case (raise FileNotFoundErrorwhen the CSV is missing); extend that same fail-loud philosophy to a per-row check on the loaded DataFrame.tests/contracts/test_output_contract.py's docstring explains why this codebase prefers loud failures over silent drift — worth reading before deciding whether toraiseor log-and-drop the offending row.How to work on this
/assignand wait to be assigned — unassigned PRs are closed automatically.@coderabbitai planfor a draft implementation plan. Verify it against the code — spotting where it's wrong is the research.🤖 AI: tools are welcome; verified work is required. You ran it, you can explain every line, your tests verify behaviour — see the AI policy. Fully automated bot PRs are closed.
Before opening your PR:
tests/<pkg>/test_<module>.py(testing guide)uv run pytestanduv run ruff check src testspass; scope is limited to this issueStuck? Comment here with what you've tried — see getting help.