Skip to content

Feat: unify report metadata and harden networking#32

Merged
tykeal merged 6 commits into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:feat/report-metadata-and-resilience
Jun 30, 2026
Merged

Feat: unify report metadata and harden networking#32
tykeal merged 6 commits into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:feat/report-metadata-and-resilience

Conversation

@ModeSevenIndustrialSolutions

@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR overhauls how github-security-report builds and renders its
output so that every reporting category shares a single, metadata-driven
representation across all four surfaces (terminal, Slack, Markdown,
HTML), and hardens the tool's network behaviour so an offline run can no
longer produce a falsely reassuring "all clean" report.

The work lands as five focused, signed commits off main (no rebase
required).

What changed

Resilience

  • Hard-fail on unreachable GitHub API. Transport failures to the
    GitHub API now abort the run with a clear NetworkError (exit code 3)
    instead of silently degrading to a green/no-data report. Requests
    funnel through one shared retry/backoff policy defined by module-level
    constants (API_MAX_RETRIES, API_BACKOFF_INITIAL_SECONDS,
    API_BACKOFF_FACTOR, API_MAX_TOTAL_WAIT_SECONDS): exponential
    backoff over three retries, capped at 60 s cumulative wait. The error
    carries a dedicated diagnostics line with the resolved host, IP and
    TCP port to speed up DNS-vs-connectivity triage. The third-party
    Scorecard endpoint still degrades gracefully so a flaky external API
    never aborts the whole report.

Reporting model

  • Unified, metadata-driven rendering. A new categories.py registry
    holds a CategoryMeta per category (stable key, title, pass/fail
    labels, documentation URL, default description, fail_severity). A
    shared report.build_summary() turns count buckets into ordered,
    remediation-first summary lines (failures → disabled → unknown → pass
    → excluded), collapsing the pass line to All <pass> only when nothing
    needs attention. Every surface now renders identically; duplicated
    counts (e.g. All enabled alongside a redundant 86 enabled) are
    gone. Descriptions and doc links are reserved for the rich Markdown/HTML
    surfaces; terminal and Slack stay brevity-first. Excluded repositories
    are surfaced per category. See
    docs/adr/0002-report-metadata-and-footer.md.

  • Informational severity + per-category fail cutoff. Adds an
    Informational rung below Low. SARIF note/none levels (the bulk
    of a tool like Zizmor) normalise to Informational. Each category has a
    fail_severity cutoff (global default Medium; Zizmor lowered to
    Low), overridable via report.categories.<key>.fail_severity. A repo
    is flagged only when it carries a finding at or above the cutoff.

  • Default release-staleness window of 60 days. Repositories
    released/tagged within the window pass and are excluded from the
    Releases / Tagging table. Configurable via the JSON schema.

Configuration

  • Per-category and per-output render toggles. report.categories
    gains a global enabled switch (highest precedence) plus a lower-
    precedence outputs map (cli, slack, markdown, html). Data is
    always collected; toggles govern presentation only. All keys default to
    true and merge key-by-key. The machine-readable report.json is
    deliberately left complete regardless of toggles.

Testing

  • uv run pytest — 316 passed
  • All pre-commit hooks green (ruff, mypy, basedpyright, codespell, …)

Notes

Output content is intentionally near-identical to before for unchanged
categories; the bulk of the diff is structural consolidation behind the
new metadata model.

@ModeSevenIndustrialSolutions @claude
Feat: hard-fail on unreachable GitHub API
7e7185a 
Running the tool offline previously produced a falsely reassuring
report: every GitHub transport failure degraded to an indeterminate
503, so all signals rendered as "no data" or green checkmarks while
no live data had been read at all. An operator could mistake a total
loss of connectivity for a clean estate.

A security report is only meaningful when built from live API data,
so a transport failure to the GitHub API now aborts the run with a
clear network error instead of degrading. Requests funnel through a
single shared retry/backoff policy defined by module-level constants
(API_MAX_RETRIES, API_BACKOFF_INITIAL_SECONDS, API_BACKOFF_FACTOR,
API_MAX_TOTAL_WAIT_SECONDS): exponential backoff over three retries,
capped at sixty seconds of cumulative waiting. Once the budget is
spent, a GitHub transport failure raises NetworkError and the CLI
exits with code 3. The error carries a dedicated diagnostics line
reporting the resolved host, IP address and TCP port to speed up
triage of DNS versus connectivity faults.

Transport failures to the third-party Scorecard endpoint still
degrade that one signal, so a flaky external API never aborts the
whole report. Rate-limit handling reuses the same shared schedule.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions @claude
Feat: default release-staleness window to 60 days
ebf15f1 
The Releases / Tagging table previously defaulted
release_max_age_days to 0, which disables the staleness threshold and
lists every eligible repository ranked by age. In practice that floods
the table with repositories released only days ago, which are plainly
maintained and need no attention.

Default the threshold to 60 days so a repository tagged or released
within the trailing 60-day window counts as recently maintained and
drops out of the table; only repositories quiet for longer than that
(or with no release or tag at all) are flagged. The value stays fully
tunable through the report.release_max_age_days config key and the
matching --release-max-age-days CLI override, with 0 still disabling
the threshold entirely.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions @claude
Refactor: unify report rendering via category metadata
bec83e2 
Introduce a single, render-surface-agnostic category registry and a
shared summary-footer builder so every reporting category presents its
results identically across the terminal, Slack, Markdown and HTML
surfaces. Previously each renderer carried its own heading text, footer
wording and explanatory notes, which drifted apart and duplicated
counts (e.g. "All enabled" alongside a redundant "86 enabled").

categories.py holds a CategoryMeta per category: stable key, title,
pass/fail labels, documentation URL and a default description. The new
report.build_summary() turns normalised count buckets into ordered,
remediation-first SummaryLines (failures, disabled, unknown, pass,
excluded); the pass line collapses to "All <pass>" only when nothing
else needs attention. TableSection now carries CategoryMeta plus
pass/fail/unknown counts instead of pre-rendered note/summary strings.

Descriptions and documentation links are reserved for the rich Markdown
and HTML surfaces; the terminal and Slack stay brevity-first. Excluded
repositories are surfaced per category rather than in a single org-level
banner. The Dependabot enablement table is retitled "Dependabot: Alerts
Enabled" to disambiguate it from the open-alert signal it nests beneath.

See docs/adr/0002-report-metadata-and-footer.md.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions @claude
Feat: add per-category and per-output render toggles
b9c3742 
Let each reporting category be switched on or off, globally and per
render surface, via report.categories in the JSON config. Data is always
collected; the toggles govern presentation only, so a category can be
hidden on noisy surfaces while still publishing to GitHub Pages.

Each category key carries a global "enabled" switch (highest precedence:
off hides it everywhere) and a lower-precedence "outputs" map for the
four surfaces (cli, slack, markdown, html). A category renders on a
surface only when enabled AND that output toggle are both true. All keys
default to true, so an omitted category or key stays fully enabled, and
overrides merge key-by-key across the global and per-org report blocks.

The renderers gain an optional category-visibility predicate; the CLI
builds one per output from the resolved config. The machine-readable
report.json artifact is deliberately left complete regardless of the
toggles.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions @claude
Feat: add informational severity and fail-severity cutoff
b4725bd 
Introduce an Informational severity rung below Low and a per-category
fail_severity cutoff that decides when a repository counts as a failure.
A repository is flagged as an offender only when it carries a finding at
or above the cutoff; sub-threshold findings fold into the clean count.

SARIF note/none levels -- the bulk of a tool like Zizmor -- now normalise
to Informational rather than Low, so a category can treat them as
non-actionable. SeverityCounts gains an informational field and an
at_or_above() helper; the weighted/sort keys make room for the new rung.

The cutoff lives in the category metadata: the global default is Medium
(Low and Informational pass), and Zizmor lowers it to Low (only
Informational passes). It is overridable per category via
report.categories.<key>.fail_severity in the JSON config, resolved in
collection and threaded into classification.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions requested review from a team and Copilot June 30, 2026 12:30
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request refactors github-security-report to use a single metadata-driven category registry and a shared summary-footer builder across all render surfaces (terminal/Slack/Markdown/HTML), while also tightening network behavior so persistent GitHub API transport failures abort the run instead of producing misleading “clean” output.

Changes:

  • Introduces categories.py (CategoryKey/CategoryMeta) and rewires signals/tables/renderers to use metadata consistently across outputs.
  • Replaces ad-hoc per-surface summaries/notes with a shared report.build_summary() model (ordered, remediation-first footer lines + optional name lists).
  • Implements a shared retry/backoff policy and raises NetworkError on exhausted GitHub API transport failures; keeps Scorecard transport failures degradable.

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/test_severity.py Updates SARIF/Code Scanning severity mapping expectations (adds Informational).
tests/test_report.py Adds tests for report.build_summary() ordering/collapse behavior.
tests/test_render_terminal.py Updates terminal rendering expectations to metadata + standard footer.
tests/test_render_slack.py Updates Slack rendering expectations to metadata + standard footer + omission rules.
tests/test_render_markdown.py Updates Markdown rendering expectations for standardized footer + descriptions.
tests/test_render_html.py Updates HTML rendering expectations for standardized footer + descriptions/links.
tests/test_posture.py Updates posture-table expectations to use counts/metadata-driven footer.
tests/test_models.py Extends SeverityCounts with Informational + cutoff counting.
tests/test_config.py Updates defaults (release staleness), adds per-category toggles and fail-severity override tests.
tests/test_collect.py Updates Dependabot table title expectation (metadata-driven).
tests/test_client.py Adds tests for GitHub transport hard-fail (NetworkError) and Scorecard degradation.
tests/test_classify.py Adds tests for per-category fail-severity cutoff behavior in classification.
src/github_security_report/templates/report.html.j2 Reworks HTML template to render description + standardized footer lists.
src/github_security_report/severity.py Adds Severity.INFORMATIONAL; adjusts SARIF fallback and code-scanning defaulting.
src/github_security_report/report.py Adds summary models (SummaryCount/SummaryLine), build_summary(), and count plumbing.
src/github_security_report/render/terminal.py Replaces bespoke totals rendering with standardized footer rendering + show predicate.
src/github_security_report/render/slack.py Replaces bespoke summaries with standardized footer rendering + show predicate.
src/github_security_report/render/markdown.py Replaces bespoke section rendering with metadata + standardized footer + show predicate.
src/github_security_report/render/html.py Replaces bespoke HTML context with metadata + standardized footer + show predicate.
src/github_security_report/posture.py Converts posture tables to metadata-driven TableSection + normalized counts.
src/github_security_report/models.py Connects SignalType to category metadata; extends SeverityCounts for new rung/cutoff.
src/github_security_report/config.py Adds per-category render toggles + per-category fail_severity overrides and schema validation.
src/github_security_report/collect.py Plumbs config fail-severity overrides into classification.
src/github_security_report/client.py Implements shared retry/backoff constants and NetworkError hard-fail logic + diagnostics.
src/github_security_report/cli.py Adds network-abort exit code 3 handling; adds per-output category filtering; updates JSON serialization fields.
src/github_security_report/classify.py Implements fail-severity cutoffs via SeverityCounts.at_or_above() and optional overrides.
src/github_security_report/categories.py New: central metadata registry for all report categories.
README.md Documents standardized footer, release staleness default, category toggles, and fail-severity cutoff.
docs/BRIEF.md Updates architecture/behavior docs for new footer model, defaults, toggles, and network hard-fail.
docs/adr/0002-report-metadata-and-footer.md New ADR for metadata registry + standardized footer decisions.
docs/adr/0001-architecture-and-scope.md Updates architecture ADR for shared retry/backoff + GitHub transport hard-fail behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/github_security_report/render/slack.py Outdated
Comment thread src/github_security_report/render/markdown.py
Comment thread src/github_security_report/templates/report.html.j2
ModeSevenIndustrialSolutions added a commit to modeseven-lfreleng-actions/github-security-report-action that referenced this pull request Jun 30, 2026
@ModeSevenIndustrialSolutions @claude
Fix: add no-data fallback to Slack/Markdown/HTML
1f5a514 
Copilot review of PR lfreleng-actions#32 flagged that the standardised "no data"
fallback was missing or wrong on three render surfaces. A category
with a populated table but empty footer buckets, or with neither rows
nor countable state, rendered inconsistently across surfaces.

Copilot feedback addressed:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary, contradicting the documented
  "no data" behaviour. It now renders a "No data available." note.

Adds regression tests on each surface.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

This comment was marked as resolved.

Sign in to view

ModeSevenIndustrialSolutions added a commit to modeseven-lfreleng-actions/github-security-report-action that referenced this pull request Jun 30, 2026
@ModeSevenIndustrialSolutions @claude
Fix: address Copilot review feedback on PR lfreleng-actions#32
2c27463 
Groups all Copilot review fixes for this PR into a single commit,
amended across review cycles.

Cycle 1 -- standardised "no data" fallback across render surfaces:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary. It now renders a
  "No data available." note.

Cycle 2 -- totals row undercounted informational findings:

- report.offender_column_totals now accumulates the informational rung.
  Each per-row Total cell already includes informational via
  SeverityCounts.total, but the totals helper summed only critical/
  high/medium/low, so the Total column did not sum vertically whenever
  an offender carried informational findings (e.g. zizmor note-level
  results). The hidden informational column is now consistent between
  rows and the totals row.

Adds regression tests on each surface and for the totals helper.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions force-pushed the feat/report-metadata-and-resilience branch from 1f5a514 to 2c27463 Compare June 30, 2026 13:19

This comment was marked as resolved.

Sign in to view

@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions changed the title Unify report metadata and harden network handling Feat: unify report metadata and harden networking Jun 30, 2026
ModeSevenIndustrialSolutions added a commit to modeseven-lfreleng-actions/github-security-report-action that referenced this pull request Jun 30, 2026
@ModeSevenIndustrialSolutions @claude
Fix: address Copilot review feedback on PR lfreleng-actions#32
be61442 
Groups all Copilot review fixes for this PR into a single commit,
amended across review cycles.

Cycle 1 -- standardised "no data" fallback across render surfaces:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary. It now renders a
  "No data available." note.

Cycle 2 -- totals row undercounted informational findings:

- report.offender_column_totals now accumulates the informational rung.
  Each per-row Total cell already includes informational via
  SeverityCounts.total, but the totals helper summed only critical/
  high/medium/low, so the Total column did not sum vertically whenever
  an offender carried informational findings (e.g. zizmor note-level
  results). The hidden informational column is now consistent between
  rows and the totals row.

Cycle 3 -- client retry docstring and log wording:

- client._request docstring claimed retries are capped by
  API_MAX_RETRIES; corrected to reference the configurable max_retries
  constructor argument (which defaults to API_MAX_RETRIES) so an
  override for testing/tuning is not misrepresented.
- the retry warning logged "attempt %d of %d" while printing retry
  counts (attempt + 1 of max_retries); reworded to "retry %d of %d" so
  operators are not misled when diagnosing network issues.

Adds regression tests on each surface and for the totals helper.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions force-pushed the feat/report-metadata-and-resilience branch from 2c27463 to be61442 Compare June 30, 2026 13:36

This comment was marked as resolved.

Sign in to view

ModeSevenIndustrialSolutions added a commit to modeseven-lfreleng-actions/github-security-report-action that referenced this pull request Jun 30, 2026
@ModeSevenIndustrialSolutions @claude
Fix: address Copilot review feedback on PR lfreleng-actions#32
f3d3429 
Groups all Copilot review fixes for this PR into a single commit,
amended across review cycles.

Cycle 1 -- standardised "no data" fallback across render surfaces:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary. It now renders a
  "No data available." note.

Cycle 2 -- totals row undercounted informational findings:

- report.offender_column_totals now accumulates the informational rung.
  Each per-row Total cell already includes informational via
  SeverityCounts.total, but the totals helper summed only critical/
  high/medium/low, so the Total column did not sum vertically whenever
  an offender carried informational findings (e.g. zizmor note-level
  results). The hidden informational column is now consistent between
  rows and the totals row.

Cycle 3 -- client retry docstring and log wording:

- client._request docstring claimed retries are capped by
  API_MAX_RETRIES; corrected to reference the configurable max_retries
  constructor argument (which defaults to API_MAX_RETRIES) so an
  override for testing/tuning is not misrepresented.
- the retry warning logged "attempt %d of %d" while printing retry
  counts (attempt + 1 of max_retries); reworded to "retry %d of %d" so
  operators are not misled when diagnosing network issues.

Cycle 4 -- non-blocking endpoint diagnostics:

- client._endpoint_diagnostics used a synchronous socket.getaddrinfo
  on the asyncio event loop thread. It fires exactly when the network
  is already failing (often DNS itself), so a slow resolve could stall
  the loop and delay the abort. It is now async and resolves through
  the running loop under a bounded timeout
  (API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS, 2s), falling back to
  "unresolved (timed out)" rather than blocking. The call site awaits
  it before raising NetworkError.

Adds regression tests on each surface, for the totals helper, and for
the resolved-address and DNS-timeout diagnostics paths.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions force-pushed the feat/report-metadata-and-resilience branch from be61442 to f3d3429 Compare June 30, 2026 13:46

This comment was marked as resolved.

Sign in to view

ModeSevenIndustrialSolutions added a commit to modeseven-lfreleng-actions/github-security-report-action that referenced this pull request Jun 30, 2026
@ModeSevenIndustrialSolutions @claude
Fix: address Copilot review feedback on PR lfreleng-actions#32
8b8615a 
Groups all Copilot review fixes for this PR into a single commit,
amended across review cycles.

Cycle 1 -- standardised "no data" fallback across render surfaces:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary. It now renders a
  "No data available." note.

Cycle 2 -- totals row undercounted informational findings:

- report.offender_column_totals now accumulates the informational rung.
  Each per-row Total cell already includes informational via
  SeverityCounts.total, but the totals helper summed only critical/
  high/medium/low, so the Total column did not sum vertically whenever
  an offender carried informational findings (e.g. zizmor note-level
  results). The hidden informational column is now consistent between
  rows and the totals row.

Cycle 3 -- client retry docstring and log wording:

- client._request docstring claimed retries are capped by
  API_MAX_RETRIES; corrected to reference the configurable max_retries
  constructor argument (which defaults to API_MAX_RETRIES) so an
  override for testing/tuning is not misrepresented.
- the retry warning logged "attempt %d of %d" while printing retry
  counts (attempt + 1 of max_retries); reworded to "retry %d of %d" so
  operators are not misled when diagnosing network issues.

Cycle 4 -- non-blocking endpoint diagnostics:

- client._endpoint_diagnostics used a synchronous socket.getaddrinfo
  on the asyncio event loop thread. It fires exactly when the network
  is already failing (often DNS itself), so a slow resolve could stall
  the loop and delay the abort. It is now async and resolves through
  the running loop under a bounded timeout
  (API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS, 2s), falling back to
  "unresolved (timed out)" rather than blocking. The call site awaits
  it before raising NetworkError.

Adds regression tests on each surface, for the totals helper, and for
the resolved-address and DNS-timeout diagnostics paths.

Cycle 5 -- Scorecard cutoff, Retry-After dates, Slack docs:

- classify.classify_scorecard: the external-score path computed clean
  via counts.total == 0, ignoring the per-category fail_severity
  cutoff. A perfect score (10.0) carrying only sub-threshold findings
  (e.g. low) was branded an offender, inconsistent with the
  code-scanning fallback path. It now uses not counts.at_or_above(
  cutoff), so sub-threshold findings on a perfect score fold into
  clean while an at-or-above finding still flags the repo.
- client._request: Retry-After may be an HTTP-date as well as
  delta-seconds (RFC 7231); float(retry_after) would raise ValueError
  on a date and crash rate-limit handling. Added _parse_retry_after to
  parse both forms, clamping a past/unparsable date rather than
  raising.
- README.md: documented that orgs sharing one Slack channel union
  their per-org Slack toggles (a category shows if any org enables
  it), so an org-level Slack disable does not suppress a category in a
  shared-channel digest unless every sharing org also disables it.

Adds regression tests for the Scorecard cutoff folding and the
Retry-After delta/HTTP-date parsing.

Cycle 6 -- HTML honours independent Dependabot posture toggles:

- render/html.render_org_html dropped the Dependabot posture
  sub-tables (dependabot_alerts_enabled / _updates_enabled /
  _cooldown) whenever the parent Dependabot Alerts signal was hidden,
  because it continued past the section before attaching extra_tables.
  The posture tables are toggled independently, so a hidden parent now
  promotes any enabled posture table to its own top-level section --
  matching the terminal, Markdown and Slack renderers, which already
  decouple them from the parent signal. When the parent is visible the
  tables stay nested in its card as before (no output change for the
  default all-enabled configuration).

Adds a regression test rendering with the parent hidden and a posture
table enabled.

Cycle 7 -- correct stale severity-normalisation docstring:

- severity.py module docstring still described the SARIF fallback as
  "note -> low"; the implementation maps note/none to INFORMATIONAL
  (the sub-low rung). Updated the prose to match so it no longer
  misleads readers about the new severity rung. Documentation only --
  no behaviour change.

Cycle 8 -- keep the "All <pass>" collapse truthful for signals:

- report.build_summary collapsed the pass bucket to "All <pass>"
  whenever every non-pass bucket was zero. A severity SignalSection
  enumerates its offenders as table rows, not a footer bucket, so a
  section with offenders plus some clean repos (and no disabled/
  unknown/excluded) wrongly rendered "All Clean" beneath a table that
  clearly held findings.
- SummaryCount gains a render flag. SignalSection.summary_counts now
  emits a non-rendered fail bucket carrying the offender count: it
  counts towards the collapse test (so a partially-clean section shows
  "85 Clean", not "All Clean") but emits no visible footer line, since
  severity signals carry fail_label=None and surface offenders in the
  table. Fully-clean sections still collapse to "All Clean".

No output change for the existing fixtures (which all carry an
excluded or nag repo, or are fully clean). Adds regression tests for
the non-rendered bucket and the partially-clean signal section.

Cycle 9 -- Markdown posture-table heading level when parent hidden:

- render/markdown.render_org always emitted the Dependabot posture
  sub-tables at level 3 (###). With the parent Dependabot Alerts
  signal hidden, those became orphaned ### headings nested under the
  previous ## section. They are now promoted to level 2 when the
  parent is hidden (kept at level 3 when nested under a visible
  parent), so the heading structure stays correct and consistent with
  the HTML surface (cycle 6). No output change for the default
  all-enabled configuration.

Adds a regression test asserting the posture table renders as a
level-2 heading when the parent signal is hidden.

Cycle 10 -- HTML column alignment and a stale constant comment:

- templates/report.html.j2: the unified render_table macro forced the
  right-aligned tabular-nums class on every non-first column. That
  regressed the posture/freshness tables, whose columns are textual
  (ecosystem lists, "223 days ago", release/tag strings). The macro
  now takes a per-item numeric flag: severity sections set it true
  (right-aligned counts), posture/release tables set it false
  (left-aligned text), restoring the pre-refactor alignment.
- client.py: the API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS comment said the
  address falls back to "?"; the code reports "ip=unresolved (timed
  out)". Corrected the comment to match.

Adds a regression test asserting severity cells keep class=\"num\" while
textual posture/release cells do not.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions force-pushed the feat/report-metadata-and-resilience branch from c913937 to 8b8615a Compare June 30, 2026 14:49
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Comment thread src/github_security_report/classify.py
ModeSevenIndustrialSolutions added a commit to modeseven-lfreleng-actions/github-security-report-action that referenced this pull request Jun 30, 2026
@ModeSevenIndustrialSolutions @claude
Fix: address Copilot review feedback on PR lfreleng-actions#32
24599e1 
Groups all Copilot review fixes for this PR into a single commit,
amended across review cycles.

Cycle 1 -- standardised "no data" fallback across render surfaces:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary. It now renders a
  "No data available." note.

Cycle 2 -- totals row undercounted informational findings:

- report.offender_column_totals now accumulates the informational rung.
  Each per-row Total cell already includes informational via
  SeverityCounts.total, but the totals helper summed only critical/
  high/medium/low, so the Total column did not sum vertically whenever
  an offender carried informational findings (e.g. zizmor note-level
  results). The hidden informational column is now consistent between
  rows and the totals row.

Cycle 3 -- client retry docstring and log wording:

- client._request docstring claimed retries are capped by
  API_MAX_RETRIES; corrected to reference the configurable max_retries
  constructor argument (which defaults to API_MAX_RETRIES) so an
  override for testing/tuning is not misrepresented.
- the retry warning logged "attempt %d of %d" while printing retry
  counts (attempt + 1 of max_retries); reworded to "retry %d of %d" so
  operators are not misled when diagnosing network issues.

Cycle 4 -- non-blocking endpoint diagnostics:

- client._endpoint_diagnostics used a synchronous socket.getaddrinfo
  on the asyncio event loop thread. It fires exactly when the network
  is already failing (often DNS itself), so a slow resolve could stall
  the loop and delay the abort. It is now async and resolves through
  the running loop under a bounded timeout
  (API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS, 2s), falling back to
  "unresolved (timed out)" rather than blocking. The call site awaits
  it before raising NetworkError.

Adds regression tests on each surface, for the totals helper, and for
the resolved-address and DNS-timeout diagnostics paths.

Cycle 5 -- Scorecard cutoff, Retry-After dates, Slack docs:

- classify.classify_scorecard: the external-score path computed clean
  via counts.total == 0, ignoring the per-category fail_severity
  cutoff. A perfect score (10.0) carrying only sub-threshold findings
  (e.g. low) was branded an offender, inconsistent with the
  code-scanning fallback path. It now uses not counts.at_or_above(
  cutoff), so sub-threshold findings on a perfect score fold into
  clean while an at-or-above finding still flags the repo.
- client._request: Retry-After may be an HTTP-date as well as
  delta-seconds (RFC 7231); float(retry_after) would raise ValueError
  on a date and crash rate-limit handling. Added _parse_retry_after to
  parse both forms, clamping a past/unparsable date rather than
  raising.
- README.md: documented that orgs sharing one Slack channel union
  their per-org Slack toggles (a category shows if any org enables
  it), so an org-level Slack disable does not suppress a category in a
  shared-channel digest unless every sharing org also disables it.

Adds regression tests for the Scorecard cutoff folding and the
Retry-After delta/HTTP-date parsing.

Cycle 6 -- HTML honours independent Dependabot posture toggles:

- render/html.render_org_html dropped the Dependabot posture
  sub-tables (dependabot_alerts_enabled / _updates_enabled /
  _cooldown) whenever the parent Dependabot Alerts signal was hidden,
  because it continued past the section before attaching extra_tables.
  The posture tables are toggled independently, so a hidden parent now
  promotes any enabled posture table to its own top-level section --
  matching the terminal, Markdown and Slack renderers, which already
  decouple them from the parent signal. When the parent is visible the
  tables stay nested in its card as before (no output change for the
  default all-enabled configuration).

Adds a regression test rendering with the parent hidden and a posture
table enabled.

Cycle 7 -- correct stale severity-normalisation docstring:

- severity.py module docstring still described the SARIF fallback as
  "note -> low"; the implementation maps note/none to INFORMATIONAL
  (the sub-low rung). Updated the prose to match so it no longer
  misleads readers about the new severity rung. Documentation only --
  no behaviour change.

Cycle 8 -- keep the "All <pass>" collapse truthful for signals:

- report.build_summary collapsed the pass bucket to "All <pass>"
  whenever every non-pass bucket was zero. A severity SignalSection
  enumerates its offenders as table rows, not a footer bucket, so a
  section with offenders plus some clean repos (and no disabled/
  unknown/excluded) wrongly rendered "All Clean" beneath a table that
  clearly held findings.
- SummaryCount gains a render flag. SignalSection.summary_counts now
  emits a non-rendered fail bucket carrying the offender count: it
  counts towards the collapse test (so a partially-clean section shows
  "85 Clean", not "All Clean") but emits no visible footer line, since
  severity signals carry fail_label=None and surface offenders in the
  table. Fully-clean sections still collapse to "All Clean".

No output change for the existing fixtures (which all carry an
excluded or nag repo, or are fully clean). Adds regression tests for
the non-rendered bucket and the partially-clean signal section.

Cycle 9 -- Markdown posture-table heading level when parent hidden:

- render/markdown.render_org always emitted the Dependabot posture
  sub-tables at level 3 (###). With the parent Dependabot Alerts
  signal hidden, those became orphaned ### headings nested under the
  previous ## section. They are now promoted to level 2 when the
  parent is hidden (kept at level 3 when nested under a visible
  parent), so the heading structure stays correct and consistent with
  the HTML surface (cycle 6). No output change for the default
  all-enabled configuration.

Adds a regression test asserting the posture table renders as a
level-2 heading when the parent signal is hidden.

Cycle 10 -- HTML column alignment and a stale constant comment:

- templates/report.html.j2: the unified render_table macro forced the
  right-aligned tabular-nums class on every non-first column. That
  regressed the posture/freshness tables, whose columns are textual
  (ecosystem lists, "223 days ago", release/tag strings). The macro
  now takes a per-item numeric flag: severity sections set it true
  (right-aligned counts), posture/release tables set it false
  (left-aligned text), restoring the pre-refactor alignment.
- client.py: the API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS comment said the
  address falls back to "?"; the code reports "ip=unresolved (timed
  out)". Corrected the comment to match.

Adds a regression test asserting severity cells keep class=\"num\" while
textual posture/release cells do not.

Cycle 11 -- mark the unused secret-scanning cutoff argument:

- classify_secret_scanning accepted fail_severities only for signature
  uniformity with the other classifiers (dispatched positionally), but
  secret scanning has no per-severity cutoff so it went unused. Renamed
  it to _fail_severities and added a comment, so the intentional
  non-use is clear and unused-argument linters stay quiet. No
  behaviour change.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions force-pushed the feat/report-metadata-and-resilience branch from 8b8615a to 24599e1 Compare June 30, 2026 14:55
ModeSevenIndustrialSolutions added a commit to modeseven-lfreleng-actions/github-security-report-action that referenced this pull request Jun 30, 2026
@ModeSevenIndustrialSolutions @claude
Fix: address Copilot review feedback on PR lfreleng-actions#32
c5e3eb3 
Groups all Copilot review fixes for this PR into a single commit,
amended across review cycles.

Cycle 1 -- standardised "no data" fallback across render surfaces:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary. It now renders a
  "No data available." note.

Cycle 2 -- totals row undercounted informational findings:

- report.offender_column_totals now accumulates the informational rung.
  Each per-row Total cell already includes informational via
  SeverityCounts.total, but the totals helper summed only critical/
  high/medium/low, so the Total column did not sum vertically whenever
  an offender carried informational findings (e.g. zizmor note-level
  results). The hidden informational column is now consistent between
  rows and the totals row.

Cycle 3 -- client retry docstring and log wording:

- client._request docstring claimed retries are capped by
  API_MAX_RETRIES; corrected to reference the configurable max_retries
  constructor argument (which defaults to API_MAX_RETRIES) so an
  override for testing/tuning is not misrepresented.
- the retry warning logged "attempt %d of %d" while printing retry
  counts (attempt + 1 of max_retries); reworded to "retry %d of %d" so
  operators are not misled when diagnosing network issues.

Cycle 4 -- non-blocking endpoint diagnostics:

- client._endpoint_diagnostics used a synchronous socket.getaddrinfo
  on the asyncio event loop thread. It fires exactly when the network
  is already failing (often DNS itself), so a slow resolve could stall
  the loop and delay the abort. It is now async and resolves through
  the running loop under a bounded timeout
  (API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS, 2s), falling back to
  "unresolved (timed out)" rather than blocking. The call site awaits
  it before raising NetworkError.

Adds regression tests on each surface, for the totals helper, and for
the resolved-address and DNS-timeout diagnostics paths.

Cycle 5 -- Scorecard cutoff, Retry-After dates, Slack docs:

- classify.classify_scorecard: the external-score path computed clean
  via counts.total == 0, ignoring the per-category fail_severity
  cutoff. A perfect score (10.0) carrying only sub-threshold findings
  (e.g. low) was branded an offender, inconsistent with the
  code-scanning fallback path. It now uses not counts.at_or_above(
  cutoff), so sub-threshold findings on a perfect score fold into
  clean while an at-or-above finding still flags the repo.
- client._request: Retry-After may be an HTTP-date as well as
  delta-seconds (RFC 7231); float(retry_after) would raise ValueError
  on a date and crash rate-limit handling. Added _parse_retry_after to
  parse both forms, clamping a past/unparsable date rather than
  raising.
- README.md: documented that orgs sharing one Slack channel union
  their per-org Slack toggles (a category shows if any org enables
  it), so an org-level Slack disable does not suppress a category in a
  shared-channel digest unless every sharing org also disables it.

Adds regression tests for the Scorecard cutoff folding and the
Retry-After delta/HTTP-date parsing.

Cycle 6 -- HTML honours independent Dependabot posture toggles:

- render/html.render_org_html dropped the Dependabot posture
  sub-tables (dependabot_alerts_enabled / _updates_enabled /
  _cooldown) whenever the parent Dependabot Alerts signal was hidden,
  because it continued past the section before attaching extra_tables.
  The posture tables are toggled independently, so a hidden parent now
  promotes any enabled posture table to its own top-level section --
  matching the terminal, Markdown and Slack renderers, which already
  decouple them from the parent signal. When the parent is visible the
  tables stay nested in its card as before (no output change for the
  default all-enabled configuration).

Adds a regression test rendering with the parent hidden and a posture
table enabled.

Cycle 7 -- correct stale severity-normalisation docstring:

- severity.py module docstring still described the SARIF fallback as
  "note -> low"; the implementation maps note/none to INFORMATIONAL
  (the sub-low rung). Updated the prose to match so it no longer
  misleads readers about the new severity rung. Documentation only --
  no behaviour change.

Cycle 8 -- keep the "All <pass>" collapse truthful for signals:

- report.build_summary collapsed the pass bucket to "All <pass>"
  whenever every non-pass bucket was zero. A severity SignalSection
  enumerates its offenders as table rows, not a footer bucket, so a
  section with offenders plus some clean repos (and no disabled/
  unknown/excluded) wrongly rendered "All Clean" beneath a table that
  clearly held findings.
- SummaryCount gains a render flag. SignalSection.summary_counts now
  emits a non-rendered fail bucket carrying the offender count: it
  counts towards the collapse test (so a partially-clean section shows
  "85 Clean", not "All Clean") but emits no visible footer line, since
  severity signals carry fail_label=None and surface offenders in the
  table. Fully-clean sections still collapse to "All Clean".

No output change for the existing fixtures (which all carry an
excluded or nag repo, or are fully clean). Adds regression tests for
the non-rendered bucket and the partially-clean signal section.

Cycle 9 -- Markdown posture-table heading level when parent hidden:

- render/markdown.render_org always emitted the Dependabot posture
  sub-tables at level 3 (###). With the parent Dependabot Alerts
  signal hidden, those became orphaned ### headings nested under the
  previous ## section. They are now promoted to level 2 when the
  parent is hidden (kept at level 3 when nested under a visible
  parent), so the heading structure stays correct and consistent with
  the HTML surface (cycle 6). No output change for the default
  all-enabled configuration.

Adds a regression test asserting the posture table renders as a
level-2 heading when the parent signal is hidden.

Cycle 10 -- HTML column alignment and a stale constant comment:

- templates/report.html.j2: the unified render_table macro forced the
  right-aligned tabular-nums class on every non-first column. That
  regressed the posture/freshness tables, whose columns are textual
  (ecosystem lists, "223 days ago", release/tag strings). The macro
  now takes a per-item numeric flag: severity sections set it true
  (right-aligned counts), posture/release tables set it false
  (left-aligned text), restoring the pre-refactor alignment.
- client.py: the API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS comment said the
  address falls back to "?"; the code reports "ip=unresolved (timed
  out)". Corrected the comment to match.

Adds a regression test asserting severity cells keep class=\"num\" while
textual posture/release cells do not.

Cycle 11 -- mark the unused secret-scanning cutoff argument:

- classify_secret_scanning accepted fail_severities only for signature
  uniformity with the other classifiers (dispatched positionally), but
  secret scanning has no per-severity cutoff so it went unused. Renamed
  it to _fail_severities and added a comment, so the intentional
  non-use is clear and unused-argument linters stay quiet. No
  behaviour change.

Cycle 12 -- fix Python 3.10 endpoint-diagnostics timeout test:

- tests/test_client.py: the dns-timeout test fake wait_for raised the
  builtin TimeoutError. On Python 3.11+ asyncio.TimeoutError is an
  alias of the builtin, but on 3.10 they differ (the builtin is an
  OSError subclass), so the fake hit the OSError branch and reported
  "resolution failed" instead of "timed out", failing the assertion on
  the 3.10 CI matrix job. The fake now raises
  client_mod.asyncio.TimeoutError, exactly what asyncio.wait_for raises
  on every supported version. Production code is correct and unchanged;
  the test now matches real wait_for behaviour.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions force-pushed the feat/report-metadata-and-resilience branch from 24599e1 to c5e3eb3 Compare June 30, 2026 14:58
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.

Comment thread src/github_security_report/config.py
Comment thread src/github_security_report/client.py Outdated
Copilot AI review requested due to automatic review settings June 30, 2026 15:02
ModeSevenIndustrialSolutions added a commit to modeseven-lfreleng-actions/github-security-report-action that referenced this pull request Jun 30, 2026
@ModeSevenIndustrialSolutions @claude
Fix: address Copilot review feedback on PR lfreleng-actions#32
63d2dbf 
Groups all Copilot review fixes for this PR into a single commit,
amended across review cycles.

Cycle 1 -- standardised "no data" fallback across render surfaces:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary. It now renders a
  "No data available." note.

Cycle 2 -- totals row undercounted informational findings:

- report.offender_column_totals now accumulates the informational rung.
  Each per-row Total cell already includes informational via
  SeverityCounts.total, but the totals helper summed only critical/
  high/medium/low, so the Total column did not sum vertically whenever
  an offender carried informational findings (e.g. zizmor note-level
  results). The hidden informational column is now consistent between
  rows and the totals row.

Cycle 3 -- client retry docstring and log wording:

- client._request docstring claimed retries are capped by
  API_MAX_RETRIES; corrected to reference the configurable max_retries
  constructor argument (which defaults to API_MAX_RETRIES) so an
  override for testing/tuning is not misrepresented.
- the retry warning logged "attempt %d of %d" while printing retry
  counts (attempt + 1 of max_retries); reworded to "retry %d of %d" so
  operators are not misled when diagnosing network issues.

Cycle 4 -- non-blocking endpoint diagnostics:

- client._endpoint_diagnostics used a synchronous socket.getaddrinfo
  on the asyncio event loop thread. It fires exactly when the network
  is already failing (often DNS itself), so a slow resolve could stall
  the loop and delay the abort. It is now async and resolves through
  the running loop under a bounded timeout
  (API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS, 2s), falling back to
  "unresolved (timed out)" rather than blocking. The call site awaits
  it before raising NetworkError.

Adds regression tests on each surface, for the totals helper, and for
the resolved-address and DNS-timeout diagnostics paths.

Cycle 5 -- Scorecard cutoff, Retry-After dates, Slack docs:

- classify.classify_scorecard: the external-score path computed clean
  via counts.total == 0, ignoring the per-category fail_severity
  cutoff. A perfect score (10.0) carrying only sub-threshold findings
  (e.g. low) was branded an offender, inconsistent with the
  code-scanning fallback path. It now uses not counts.at_or_above(
  cutoff), so sub-threshold findings on a perfect score fold into
  clean while an at-or-above finding still flags the repo.
- client._request: Retry-After may be an HTTP-date as well as
  delta-seconds (RFC 7231); float(retry_after) would raise ValueError
  on a date and crash rate-limit handling. Added _parse_retry_after to
  parse both forms, clamping a past/unparsable date rather than
  raising.
- README.md: documented that orgs sharing one Slack channel union
  their per-org Slack toggles (a category shows if any org enables
  it), so an org-level Slack disable does not suppress a category in a
  shared-channel digest unless every sharing org also disables it.

Adds regression tests for the Scorecard cutoff folding and the
Retry-After delta/HTTP-date parsing.

Cycle 6 -- HTML honours independent Dependabot posture toggles:

- render/html.render_org_html dropped the Dependabot posture
  sub-tables (dependabot_alerts_enabled / _updates_enabled /
  _cooldown) whenever the parent Dependabot Alerts signal was hidden,
  because it continued past the section before attaching extra_tables.
  The posture tables are toggled independently, so a hidden parent now
  promotes any enabled posture table to its own top-level section --
  matching the terminal, Markdown and Slack renderers, which already
  decouple them from the parent signal. When the parent is visible the
  tables stay nested in its card as before (no output change for the
  default all-enabled configuration).

Adds a regression test rendering with the parent hidden and a posture
table enabled.

Cycle 7 -- correct stale severity-normalisation docstring:

- severity.py module docstring still described the SARIF fallback as
  "note -> low"; the implementation maps note/none to INFORMATIONAL
  (the sub-low rung). Updated the prose to match so it no longer
  misleads readers about the new severity rung. Documentation only --
  no behaviour change.

Cycle 8 -- keep the "All <pass>" collapse truthful for signals:

- report.build_summary collapsed the pass bucket to "All <pass>"
  whenever every non-pass bucket was zero. A severity SignalSection
  enumerates its offenders as table rows, not a footer bucket, so a
  section with offenders plus some clean repos (and no disabled/
  unknown/excluded) wrongly rendered "All Clean" beneath a table that
  clearly held findings.
- SummaryCount gains a render flag. SignalSection.summary_counts now
  emits a non-rendered fail bucket carrying the offender count: it
  counts towards the collapse test (so a partially-clean section shows
  "85 Clean", not "All Clean") but emits no visible footer line, since
  severity signals carry fail_label=None and surface offenders in the
  table. Fully-clean sections still collapse to "All Clean".

No output change for the existing fixtures (which all carry an
excluded or nag repo, or are fully clean). Adds regression tests for
the non-rendered bucket and the partially-clean signal section.

Cycle 9 -- Markdown posture-table heading level when parent hidden:

- render/markdown.render_org always emitted the Dependabot posture
  sub-tables at level 3 (###). With the parent Dependabot Alerts
  signal hidden, those became orphaned ### headings nested under the
  previous ## section. They are now promoted to level 2 when the
  parent is hidden (kept at level 3 when nested under a visible
  parent), so the heading structure stays correct and consistent with
  the HTML surface (cycle 6). No output change for the default
  all-enabled configuration.

Adds a regression test asserting the posture table renders as a
level-2 heading when the parent signal is hidden.

Cycle 10 -- HTML column alignment and a stale constant comment:

- templates/report.html.j2: the unified render_table macro forced the
  right-aligned tabular-nums class on every non-first column. That
  regressed the posture/freshness tables, whose columns are textual
  (ecosystem lists, "223 days ago", release/tag strings). The macro
  now takes a per-item numeric flag: severity sections set it true
  (right-aligned counts), posture/release tables set it false
  (left-aligned text), restoring the pre-refactor alignment.
- client.py: the API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS comment said the
  address falls back to "?"; the code reports "ip=unresolved (timed
  out)". Corrected the comment to match.

Adds a regression test asserting severity cells keep class=\"num\" while
textual posture/release cells do not.

Cycle 11 -- mark the unused secret-scanning cutoff argument:

- classify_secret_scanning accepted fail_severities only for signature
  uniformity with the other classifiers (dispatched positionally), but
  secret scanning has no per-severity cutoff so it went unused. Renamed
  it to _fail_severities and added a comment, so the intentional
  non-use is clear and unused-argument linters stay quiet. No
  behaviour change.

Cycle 12 -- fix Python 3.10 endpoint-diagnostics timeout test:

- tests/test_client.py: the dns-timeout test fake wait_for raised the
  builtin TimeoutError. On Python 3.11+ asyncio.TimeoutError is an
  alias of the builtin, but on 3.10 they differ (the builtin is an
  OSError subclass), so the fake hit the OSError branch and reported
  "resolution failed" instead of "timed out", failing the assertion on
  the 3.10 CI matrix job. The fake now raises
  client_mod.asyncio.TimeoutError, exactly what asyncio.wait_for raises
  on every supported version. Production code is correct and unchanged;
  the test now matches real wait_for behaviour.

Cycle 13 -- 429 retry without headers; warn on dead fail_severity:

- client._request: the rate_limited predicate retried only when
  Retry-After parsed or x-ratelimit-remaining was "0". A 429 without
  either header (GitHub or an intermediary) fell through and was
  returned un-retried, despite 429 meaning "Too Many Requests". It now
  treats any 429 as rate limited and backs off on the shared schedule;
  a 403 still needs a header to count as a rate limit (else it is a
  genuine permission error).
- config.build_config: fail_severity only governs the severity-ranked
  signals (their classifier is the sole reader via fail_severity_for).
  Setting it on a binary category (releases, mutable_releases, the
  Dependabot posture toggles) silently did nothing. The build now
  warns, listing the misplaced keys and the categories the override
  actually applies to, so the dead setting is not a quiet footgun.

Adds regression tests for the header-less 429 retry and for the
misplaced/valid fail_severity warning behaviour.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions force-pushed the feat/report-metadata-and-resilience branch from c5e3eb3 to 63d2dbf Compare June 30, 2026 15:08
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Comment thread src/github_security_report/client.py
Copilot AI review requested due to automatic review settings June 30, 2026 15:08
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Comment thread src/github_security_report/categories.py
@ModeSevenIndustrialSolutions @claude
Fix: address Copilot review feedback on PR lfreleng-actions#32
818ad17 
Groups all Copilot review fixes for this PR into a single commit,
amended across review cycles.

Cycle 1 -- standardised "no data" fallback across render surfaces:

- render/slack.py: a SignalSection enumerates failures as table rows,
  not a footer bucket, so an all-offender section (clean/nag/unknown/
  excluded all zero) produced an empty footer and Slack wrongly
  appended "no data" beneath a populated table. Only emit "no data"
  when there are genuinely no offenders.
- render/markdown.py: render_table_section had no "no data" fallback,
  so a table with no rows and all-zero buckets rendered just a bare
  heading. It now matches render_section and emits
  "_No data available._", consistent with the terminal renderer.
- templates/report.html.j2: render_table emitted no placeholder when a
  table had no rows and an empty summary. It now renders a
  "No data available." note.

Cycle 2 -- totals row undercounted informational findings:

- report.offender_column_totals now accumulates the informational rung.
  Each per-row Total cell already includes informational via
  SeverityCounts.total, but the totals helper summed only critical/
  high/medium/low, so the Total column did not sum vertically whenever
  an offender carried informational findings (e.g. zizmor note-level
  results). The hidden informational column is now consistent between
  rows and the totals row.

Cycle 3 -- client retry docstring and log wording:

- client._request docstring claimed retries are capped by
  API_MAX_RETRIES; corrected to reference the configurable max_retries
  constructor argument (which defaults to API_MAX_RETRIES) so an
  override for testing/tuning is not misrepresented.
- the retry warning logged "attempt %d of %d" while printing retry
  counts (attempt + 1 of max_retries); reworded to "retry %d of %d" so
  operators are not misled when diagnosing network issues.

Cycle 4 -- non-blocking endpoint diagnostics:

- client._endpoint_diagnostics used a synchronous socket.getaddrinfo
  on the asyncio event loop thread. It fires exactly when the network
  is already failing (often DNS itself), so a slow resolve could stall
  the loop and delay the abort. It is now async and resolves through
  the running loop under a bounded timeout
  (API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS, 2s), falling back to
  "unresolved (timed out)" rather than blocking. The call site awaits
  it before raising NetworkError.

Adds regression tests on each surface, for the totals helper, and for
the resolved-address and DNS-timeout diagnostics paths.

Cycle 5 -- Scorecard cutoff, Retry-After dates, Slack docs:

- classify.classify_scorecard: the external-score path computed clean
  via counts.total == 0, ignoring the per-category fail_severity
  cutoff. A perfect score (10.0) carrying only sub-threshold findings
  (e.g. low) was branded an offender, inconsistent with the
  code-scanning fallback path. It now uses not counts.at_or_above(
  cutoff), so sub-threshold findings on a perfect score fold into
  clean while an at-or-above finding still flags the repo.
- client._request: Retry-After may be an HTTP-date as well as
  delta-seconds (RFC 7231); float(retry_after) would raise ValueError
  on a date and crash rate-limit handling. Added _parse_retry_after to
  parse both forms, clamping a past/unparsable date rather than
  raising.
- README.md: documented that orgs sharing one Slack channel union
  their per-org Slack toggles (a category shows if any org enables
  it), so an org-level Slack disable does not suppress a category in a
  shared-channel digest unless every sharing org also disables it.

Adds regression tests for the Scorecard cutoff folding and the
Retry-After delta/HTTP-date parsing.

Cycle 6 -- HTML honours independent Dependabot posture toggles:

- render/html.render_org_html dropped the Dependabot posture
  sub-tables (dependabot_alerts_enabled / _updates_enabled /
  _cooldown) whenever the parent Dependabot Alerts signal was hidden,
  because it continued past the section before attaching extra_tables.
  The posture tables are toggled independently, so a hidden parent now
  promotes any enabled posture table to its own top-level section --
  matching the terminal, Markdown and Slack renderers, which already
  decouple them from the parent signal. When the parent is visible the
  tables stay nested in its card as before (no output change for the
  default all-enabled configuration).

Adds a regression test rendering with the parent hidden and a posture
table enabled.

Cycle 7 -- correct stale severity-normalisation docstring:

- severity.py module docstring still described the SARIF fallback as
  "note -> low"; the implementation maps note/none to INFORMATIONAL
  (the sub-low rung). Updated the prose to match so it no longer
  misleads readers about the new severity rung. Documentation only --
  no behaviour change.

Cycle 8 -- keep the "All <pass>" collapse truthful for signals:

- report.build_summary collapsed the pass bucket to "All <pass>"
  whenever every non-pass bucket was zero. A severity SignalSection
  enumerates its offenders as table rows, not a footer bucket, so a
  section with offenders plus some clean repos (and no disabled/
  unknown/excluded) wrongly rendered "All Clean" beneath a table that
  clearly held findings.
- SummaryCount gains a render flag. SignalSection.summary_counts now
  emits a non-rendered fail bucket carrying the offender count: it
  counts towards the collapse test (so a partially-clean section shows
  "85 Clean", not "All Clean") but emits no visible footer line, since
  severity signals carry fail_label=None and surface offenders in the
  table. Fully-clean sections still collapse to "All Clean".

No output change for the existing fixtures (which all carry an
excluded or nag repo, or are fully clean). Adds regression tests for
the non-rendered bucket and the partially-clean signal section.

Cycle 9 -- Markdown posture-table heading level when parent hidden:

- render/markdown.render_org always emitted the Dependabot posture
  sub-tables at level 3 (###). With the parent Dependabot Alerts
  signal hidden, those became orphaned ### headings nested under the
  previous ## section. They are now promoted to level 2 when the
  parent is hidden (kept at level 3 when nested under a visible
  parent), so the heading structure stays correct and consistent with
  the HTML surface (cycle 6). No output change for the default
  all-enabled configuration.

Adds a regression test asserting the posture table renders as a
level-2 heading when the parent signal is hidden.

Cycle 10 -- HTML column alignment and a stale constant comment:

- templates/report.html.j2: the unified render_table macro forced the
  right-aligned tabular-nums class on every non-first column. That
  regressed the posture/freshness tables, whose columns are textual
  (ecosystem lists, "223 days ago", release/tag strings). The macro
  now takes a per-item numeric flag: severity sections set it true
  (right-aligned counts), posture/release tables set it false
  (left-aligned text), restoring the pre-refactor alignment.
- client.py: the API_DIAGNOSTIC_DNS_TIMEOUT_SECONDS comment said the
  address falls back to "?"; the code reports "ip=unresolved (timed
  out)". Corrected the comment to match.

Adds a regression test asserting severity cells keep class=\"num\" while
textual posture/release cells do not.

Cycle 11 -- mark the unused secret-scanning cutoff argument:

- classify_secret_scanning accepted fail_severities only for signature
  uniformity with the other classifiers (dispatched positionally), but
  secret scanning has no per-severity cutoff so it went unused. Renamed
  it to _fail_severities and added a comment, so the intentional
  non-use is clear and unused-argument linters stay quiet. No
  behaviour change.

Cycle 12 -- fix Python 3.10 endpoint-diagnostics timeout test:

- tests/test_client.py: the dns-timeout test fake wait_for raised the
  builtin TimeoutError. On Python 3.11+ asyncio.TimeoutError is an
  alias of the builtin, but on 3.10 they differ (the builtin is an
  OSError subclass), so the fake hit the OSError branch and reported
  "resolution failed" instead of "timed out", failing the assertion on
  the 3.10 CI matrix job. The fake now raises
  client_mod.asyncio.TimeoutError, exactly what asyncio.wait_for raises
  on every supported version. Production code is correct and unchanged;
  the test now matches real wait_for behaviour.

Cycle 13 -- 429 retry without headers; warn on dead fail_severity:

- client._request: the rate_limited predicate retried only when
  Retry-After parsed or x-ratelimit-remaining was "0". A 429 without
  either header (GitHub or an intermediary) fell through and was
  returned un-retried, despite 429 meaning "Too Many Requests". It now
  treats any 429 as rate limited and backs off on the shared schedule;
  a 403 still needs a header to count as a rate limit (else it is a
  genuine permission error).
- config.build_config: fail_severity only governs the severity-ranked
  signals (their classifier is the sole reader via fail_severity_for).
  Setting it on a binary category (releases, mutable_releases, the
  Dependabot posture toggles) silently did nothing. The build now
  warns, listing the misplaced keys and the categories the override
  actually applies to, so the dead setting is not a quiet footgun.

Adds regression tests for the header-less 429 retry and for the
misplaced/valid fail_severity warning behaviour.

Cycle 14 -- presence-based rate-limit detection; docstring fix:

- client._request: a 403 whose Retry-After header is present but
  malformed/unparsable still parsed to None, so the predicate missed
  it and returned the 403 un-retried. Detection now keys on the
  *presence* of the Retry-After header (not a successful parse), so a
  malformed value still triggers a backoff, falling back to the
  exponential schedule when the value cannot be parsed.
- categories.py: the module docstring claimed it "imports nothing from
  the rest of the package", but it imports the leaf severity module.
  Corrected to say it imports nothing except that leaf (which itself
  imports nothing from the package), so the no-cycle guarantee it
  documents stays accurate.

Adds a regression test asserting a 403 with a malformed Retry-After is
retried on the backoff schedule.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions force-pushed the feat/report-metadata-and-resilience branch from 63d2dbf to 818ad17 Compare June 30, 2026 15:15
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

@tykeal tykeal merged commit 266c7fb into lfreleng-actions:main Jun 30, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@tykeal tykeal tykeal approved these changes

Assignees

@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ModeSevenIndustrialSolutions @tykeal