Walkthrough

The nightly E2E scorecard now derives job status from the Actions API (canonical job set with fallback), includes actor in exported scorecardData, and remaps Slack routing to daily/fullrun/preview while adding conditional actor attribution in messages.

Changes

Nightly E2E Scorecard Actor Attribution & Routing

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Possibly related PRs

• NVIDIA/NemoClaw#4308: Prior Slack/scorecard integration changes; this PR continues the Slack routing and scorecard payload work begun there.

Suggested labels

enhancement: integration, CI/CD, E2E, area: e2e

Suggested reviewers

• jyaunches
• cv

Poem

🐰 I hopped through jobs and fetched their state,
From GitHub's API I tallied fate.
Daily, fullrun, preview — routes anew,
Who hit dispatch now shows in view.
A rabbit nudges CI with a tiny carrot cue.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

E2E Recommendation Advisor

Failed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-raw-output.txt

E2E Scenario Advisor

Failed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-scenario-advisor-raw-output.txt

🛠️ Needs attention

• None.

🔎 Worth checking

• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml canonical job source: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: The workflow documents `Source: listJobsForWorkflowRun (paginated)` and derives `canonicalJobs` from `github.paginate(...)`.
• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml API failure fallback: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: The catch block warns and leaves `canonicalJobs` null; later logic uses `entries` from `needs`.
• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml reusable-workflow suffix normalization: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: `const name = j.name.replace(/ \/ [^/]+$/, '')` strips a final reusable-workflow suffix.
• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml duplicate normalized job dedupe: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: Dedupe now compares `run_attempt` and `completed_at`, but equal or missing metadata still has no stable tie-breaker.
• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml unknown terminal conclusion handling: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: Counts use a catch-all `else failure++;`, but failed job listing still filters only exact `failure` conclusions.
• Guard Slack webhook secrets from branch-controlled scorecard code (.github/workflows/nightly-e2e.yaml:2767): The Slack posting step injects live webhook secrets and then loads `scripts/scorecard/build-slack-blocks.ts` from the checked-out workspace. For workflow_dispatch runs on a non-default workflow ref, that means branch-controlled repo code can execute in the same process as the Slack webhook secrets. This PR changes the Slack secret routing boundary, and there is overlapping security work in PR ci(e2e): withhold live messaging secrets from target refs #4862 about withholding live messaging secrets from target refs.
  • Recommendation: Only expose Slack webhook secrets to trusted workflow refs, or pin the scorecard-builder checkout to a trusted default-branch SHA before loading it. Consider gating Slack posting on trusted refs and coordinating with the live-secret withholding work.
  • Evidence: The scorecard job checks out `scripts/scorecard` without an explicit trusted `ref`, then the Slack step sets `SLACK_WEBHOOK_URL_DAILY`, `SLACK_WEBHOOK_URL_FULLRUN`, and `SLACK_WEBHOOK_URL_PREVIEW` and calls `require(path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard/build-slack-blocks.ts'))`.
• Unknown terminal job conclusions are counted as failures but omitted from failed jobs (.github/workflows/nightly-e2e.yaml:2630): The count loop now correctly maps terminal non-success conclusions such as `timed_out`, `action_required`, `neutral`, `stale`, or future values to `failure`, but the failed job list still includes only exact `j.conclusion === 'failure'`. A timed-out job can therefore produce `❌ 1 failed` without a corresponding failed-job name or hyperlink.
  • Recommendation: Use the same terminal non-success classification for `failedJobs` that is used for counts, or add an explicit anomalous/failed jobs bucket that includes non-success completed conclusions with their URLs.
  • Evidence: Counts use a catch-all `else failure++;`, but `failedJobs` is built with `.filter((j) => j.conclusion === 'failure')`.
• API-derived scorecard construction and fallback still lack runtime coverage (.github/workflows/nightly-e2e.yaml:2571): The highest-risk behavior now lives in inline GitHub Actions script: paginated `listJobsForWorkflowRun`, suffix normalization, duplicate dedupe, conclusion classification, URL preservation, and fallback to `needs`. The added tests exercise only the Slack block builder after `ScorecardData` has already been produced.
  • Recommendation: Extract the scorecard-data construction into a testable helper or add a workflow-script harness that covers the API success path, pagination, fallback branch, normalization, dedupe, non-standard conclusions, and failed-job URL preservation.
  • Evidence: The workflow calls `github.paginate(github.rest.actions.listJobsForWorkflowRun, ...)` and catches API errors by falling back to `entries`; `test/scorecard-blocks.test.ts` imports only `scripts/scorecard/build-slack-blocks.ts`.
• Duplicate job dedupe still has an API-order tie case (.github/workflows/nightly-e2e.yaml:2586): The dedupe logic is much better than last-write-wins because it prefers higher `run_attempt` and later `completed_at`, but if those fields are equal or missing the selected duplicate still depends on the order returned by the Jobs API.
  • Recommendation: Add a stable final tie-breaker such as `started_at` and numeric job `id`, or sort the API jobs before deduping. Add a regression test that shuffles duplicate jobs and proves the same final attempt is selected.
  • Evidence: `isNewer` compares only `(j.run_attempt ?? 0)` and `(j.completed_at ?? '')`; no tie-breaker is applied when both compare equal.
• Prior-day API error text is still rendered into summary and Slack (.github/workflows/nightly-e2e.yaml:2688): The Slack webhook and current-run Jobs API warnings were truncated, but the prior-day trend catch still embeds raw `e.message` in `trendLine`. That value is written to the GitHub summary and included in Slack data, so an external API diagnostic can leak operational details or break display formatting.
  • Recommendation: Sanitize and truncate the prior-day error just like the other external error paths, preferably logging only status and a short safe reason.
  • Evidence: `trendLine = `Trend: ⊘ Could not fetch prior-day data (${e.message})`;` is later placed in `scorecardData` and rendered by the Slack builder.
• Slack webhook secret routing is not tested at the workflow boundary (.github/workflows/nightly-e2e.yaml:2794): Unit tests cover `getSlackChannel`, but they do not verify that the workflow maps the returned tag to the intended secret env var or that selective dispatches skip posting unless explicitly opted in.
  • Recommendation: Add a small extracted helper or workflow-script harness that verifies `daily -> SLACK_WEBHOOK_URL_DAILY`, `fullrun -> SLACK_WEBHOOK_URL_FULLRUN`, `preview -> SLACK_WEBHOOK_URL_PREVIEW`, and that preview returns before posting when `post_to_slack` is not true.
  • Evidence: The workflow defines `envByChannel = { daily: 'SLACK_WEBHOOK_URL_DAILY', fullrun: 'SLACK_WEBHOOK_URL_FULLRUN', preview: 'SLACK_WEBHOOK_URL_PREVIEW' }`; tests only assert `getSlackChannel(...)` return values.
• Slack mrkdwn fields are assembled without escaping (scripts/scorecard/build-slack-blocks.ts:90): Actor, requested job names, failed job names, and failed job URLs are interpolated directly into Slack mrkdwn. Most current sources are GitHub-controlled or maintainer-dispatch-controlled, but the builder accepts arbitrary `ScorecardData`, and malformed values can alter Slack rendering or produce confusing links.
  • Recommendation: Add Slack mrkdwn escaping for text fields and validate/allowlist URLs before rendering `<url|text>` links. Cover actor, requested jobs, failed job names, and failed job URLs in tests.
  • Evidence: `runModeText` includes `*${data.actor}*`; requested jobs are wrapped in backticks; failed jobs render as `<${job.url}|${job.name}>` without escaping.

🌱 Nice ideas

• None.

• **Runtime validation** — Workflow scorecard data builder counts paginated `listJobsForWorkflowRun` results and preserves `html_url` in `failedJobs`.. The changed pure Slack builder has useful unit coverage, but the higher-risk behavior is inline workflow runtime code that talks to GitHub APIs, handles fallbacks, maps secrets, and posts to Slack.
• **Runtime validation** — Workflow scorecard normalizes `cloud-e2e / run` to `cloud-e2e` before dedupe and failed-job rendering.. The changed pure Slack builder has useful unit coverage, but the higher-risk behavior is inline workflow runtime code that talks to GitHub APIs, handles fallbacks, maps secrets, and posts to Slack.
• **Runtime validation** — Workflow scorecard selects duplicate normalized jobs by `run_attempt`, `completed_at`, and a stable tie-breaker independent of API page order.. The changed pure Slack builder has useful unit coverage, but the higher-risk behavior is inline workflow runtime code that talks to GitHub APIs, handles fallbacks, maps secrets, and posts to Slack.
• **Runtime validation** — Workflow scorecard treats `timed_out`, `action_required`, `neutral`, and unknown completed conclusions as non-perfect and includes them in the failed/anomalous jobs list.. The changed pure Slack builder has useful unit coverage, but the higher-risk behavior is inline workflow runtime code that talks to GitHub APIs, handles fallbacks, maps secrets, and posts to Slack.
• **Runtime validation** — Workflow scorecard falls back to `needs` counts and `failedJobs` entries with `url: null` when `listJobsForWorkflowRun` throws.. The changed pure Slack builder has useful unit coverage, but the higher-risk behavior is inline workflow runtime code that talks to GitHub APIs, handles fallbacks, maps secrets, and posts to Slack.
• **API-derived scorecard construction and fallback still lack runtime coverage** — Extract the scorecard-data construction into a testable helper or add a workflow-script harness that covers the API success path, pagination, fallback branch, normalization, dedupe, non-standard conclusions, and failed-job URL preservation.
• **Slack webhook secret routing is not tested at the workflow boundary** — Add a small extracted helper or workflow-script harness that verifies `daily -> SLACK_WEBHOOK_URL_DAILY`, `fullrun -> SLACK_WEBHOOK_URL_FULLRUN`, `preview -> SLACK_WEBHOOK_URL_PREVIEW`, and that preview returns before posting when `post_to_slack` is not true.
• **Acceptance clause:** Rename Slack channel tags + secrets to event-type names (`SITUATION_ROOM` / `CI` → `DAILY` / `FULLRUN`) (due to just pushing notification to a unified channel as requirement) — add test evidence or identify existing coverage. `getSlackChannel` now returns `daily` and `fullrun`, workflow env vars use `SLACK_WEBHOOK_URL_DAILY` and `SLACK_WEBHOOK_URL_FULLRUN`, and tests cover helper return values. The workflow env-var mapping and preview skip gate are not covered by tests.

Current findings:

• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml canonical job source: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: The workflow documents `Source: listJobsForWorkflowRun (paginated)` and derives `canonicalJobs` from `github.paginate(...)`.
• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml API failure fallback: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: The catch block warns and leaves `canonicalJobs` null; later logic uses `entries` from `needs`.
• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml reusable-workflow suffix normalization: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: `const name = j.name.replace(/ \/ [^/]+$/, '')` strips a final reusable-workflow suffix.
• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml duplicate normalized job dedupe: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: Dedupe now compares `run_attempt` and `completed_at`, but equal or missing metadata still has no stable tie-breaker.
• Source-of-truth review needed: .github/workflows/nightly-e2e.yaml unknown terminal conclusion handling: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: Counts use a catch-all `else failure++;`, but failed job listing still filters only exact `failure` conclusions.
• Guard Slack webhook secrets from branch-controlled scorecard code (.github/workflows/nightly-e2e.yaml:2767): The Slack posting step injects live webhook secrets and then loads `scripts/scorecard/build-slack-blocks.ts` from the checked-out workspace. For workflow_dispatch runs on a non-default workflow ref, that means branch-controlled repo code can execute in the same process as the Slack webhook secrets. This PR changes the Slack secret routing boundary, and there is overlapping security work in PR ci(e2e): withhold live messaging secrets from target refs #4862 about withholding live messaging secrets from target refs.
  • Recommendation: Only expose Slack webhook secrets to trusted workflow refs, or pin the scorecard-builder checkout to a trusted default-branch SHA before loading it. Consider gating Slack posting on trusted refs and coordinating with the live-secret withholding work.
  • Evidence: The scorecard job checks out `scripts/scorecard` without an explicit trusted `ref`, then the Slack step sets `SLACK_WEBHOOK_URL_DAILY`, `SLACK_WEBHOOK_URL_FULLRUN`, and `SLACK_WEBHOOK_URL_PREVIEW` and calls `require(path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard/build-slack-blocks.ts'))`.
• Unknown terminal job conclusions are counted as failures but omitted from failed jobs (.github/workflows/nightly-e2e.yaml:2630): The count loop now correctly maps terminal non-success conclusions such as `timed_out`, `action_required`, `neutral`, `stale`, or future values to `failure`, but the failed job list still includes only exact `j.conclusion === 'failure'`. A timed-out job can therefore produce `❌ 1 failed` without a corresponding failed-job name or hyperlink.
  • Recommendation: Use the same terminal non-success classification for `failedJobs` that is used for counts, or add an explicit anomalous/failed jobs bucket that includes non-success completed conclusions with their URLs.
  • Evidence: Counts use a catch-all `else failure++;`, but `failedJobs` is built with `.filter((j) => j.conclusion === 'failure')`.
• API-derived scorecard construction and fallback still lack runtime coverage (.github/workflows/nightly-e2e.yaml:2571): The highest-risk behavior now lives in inline GitHub Actions script: paginated `listJobsForWorkflowRun`, suffix normalization, duplicate dedupe, conclusion classification, URL preservation, and fallback to `needs`. The added tests exercise only the Slack block builder after `ScorecardData` has already been produced.
  • Recommendation: Extract the scorecard-data construction into a testable helper or add a workflow-script harness that covers the API success path, pagination, fallback branch, normalization, dedupe, non-standard conclusions, and failed-job URL preservation.
  • Evidence: The workflow calls `github.paginate(github.rest.actions.listJobsForWorkflowRun, ...)` and catches API errors by falling back to `entries`; `test/scorecard-blocks.test.ts` imports only `scripts/scorecard/build-slack-blocks.ts`.
• Duplicate job dedupe still has an API-order tie case (.github/workflows/nightly-e2e.yaml:2586): The dedupe logic is much better than last-write-wins because it prefers higher `run_attempt` and later `completed_at`, but if those fields are equal or missing the selected duplicate still depends on the order returned by the Jobs API.
  • Recommendation: Add a stable final tie-breaker such as `started_at` and numeric job `id`, or sort the API jobs before deduping. Add a regression test that shuffles duplicate jobs and proves the same final attempt is selected.
  • Evidence: `isNewer` compares only `(j.run_attempt ?? 0)` and `(j.completed_at ?? '')`; no tie-breaker is applied when both compare equal.
• Prior-day API error text is still rendered into summary and Slack (.github/workflows/nightly-e2e.yaml:2688): The Slack webhook and current-run Jobs API warnings were truncated, but the prior-day trend catch still embeds raw `e.message` in `trendLine`. That value is written to the GitHub summary and included in Slack data, so an external API diagnostic can leak operational details or break display formatting.
  • Recommendation: Sanitize and truncate the prior-day error just like the other external error paths, preferably logging only status and a short safe reason.
  • Evidence: `trendLine = `Trend: ⊘ Could not fetch prior-day data (${e.message})`;` is later placed in `scorecardData` and rendered by the Slack builder.
• Slack webhook secret routing is not tested at the workflow boundary (.github/workflows/nightly-e2e.yaml:2794): Unit tests cover `getSlackChannel`, but they do not verify that the workflow maps the returned tag to the intended secret env var or that selective dispatches skip posting unless explicitly opted in.
  • Recommendation: Add a small extracted helper or workflow-script harness that verifies `daily -> SLACK_WEBHOOK_URL_DAILY`, `fullrun -> SLACK_WEBHOOK_URL_FULLRUN`, `preview -> SLACK_WEBHOOK_URL_PREVIEW`, and that preview returns before posting when `post_to_slack` is not true.
  • Evidence: The workflow defines `envByChannel = { daily: 'SLACK_WEBHOOK_URL_DAILY', fullrun: 'SLACK_WEBHOOK_URL_FULLRUN', preview: 'SLACK_WEBHOOK_URL_PREVIEW' }`; tests only assert `getSlackChannel(...)` return values.
• Slack mrkdwn fields are assembled without escaping (scripts/scorecard/build-slack-blocks.ts:90): Actor, requested job names, failed job names, and failed job URLs are interpolated directly into Slack mrkdwn. Most current sources are GitHub-controlled or maintainer-dispatch-controlled, but the builder accepts arbitrary `ScorecardData`, and malformed values can alter Slack rendering or produce confusing links.
  • Recommendation: Add Slack mrkdwn escaping for text fields and validate/allowlist URLs before rendering `<url|text>` links. Cover actor, requested jobs, failed job names, and failed job URLs in tests.
  • Evidence: `runModeText` includes `*${data.actor}*`; requested jobs are wrapped in backticks; failed jobs render as `<${job.url}|${job.name}>` without escaping.