Skip to content

Share one gh-aw logs snapshot across activity collectors - #3219

Merged
pelikhan merged 44 commits into
mainfrom
copilot/refactor-activity-yml-download
Sep 6, 2026
Merged

Share one gh-aw logs snapshot across activity collectors#3219
pelikhan merged 44 commits into
mainfrom
copilot/refactor-activity-yml-download

Conversation

Copilot AI commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Activity collection repeatedly scanned workflow history and split related data across caches. This refactor downloads all repository workflow logs once, then derives dashboard data from one persisted snapshot.

Changes

  • Single collection

    • Run one bounded gh aw logs --json invocation.
    • Persist raw JSON and downloaded artifacts.
  • Shared processing

    • Derive AI Credit, security, and operational-value records from the cached logs JSON.
    • Remove per-workflow operational-value history scans and artifact fallbacks.
  • Unified cache

    • Store logs, raw JSON, derived dashboard records, and operational-value history in cao-activity.
    • Preserve prior observations when current evidence is unavailable.
  • Contracts

    • Update workflow assertions and cache documentation for the single-snapshot model.

Copilot AI and others added 7 commits September 6, 2026 04:47
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI requested a review from pelikhan September 6, 2026 04:59
@pelikhan
pelikhan marked this pull request as ready for review September 6, 2026 05:00
Copilot AI lite review requested due to automatic review settings September 6, 2026 05:00
@pelikhan

pelikhan commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

@copilot add core.... logging to debug caching and downloading

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.

🟡 Changes recommended

The new cache/merge logic can unintentionally prevent filling previously “unavailable” operational-value runs from later logs snapshots and can overwrite a usable shared logs snapshot with an empty placeholder on transient collection failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors CAO activity data acquisition to download a single bounded gh aw logs --json snapshot per activity run, persist it in the cao-activity cache, and derive AI Credit, security, and operational-value records from that shared snapshot (removing per-workflow operational-value history scans and artifact fallbacks).

Changes:

  • Switch AI Credit usage collection to one repository-wide gh aw logs --json invocation (no per-workflow targets), persisting the raw logs JSON and artifacts into the activity cache.
  • Refactor operational-value collection to read grader results from the shared logs JSON and merge them with cached observations.
  • Update workflow contracts, documentation, and add focused unit tests for the new shared-snapshot model.
File summaries
File Description
tests/unit/workflow-contract.test.mjs Updates assertions to enforce the single-snapshot model and new cache paths/env vars.
tests/unit/dashboard-operational-values.test.mjs Adds a unit test validating operational-values processing from shared logs JSON.
tests/unit/dashboard-aic-usage.test.mjs Updates AIC usage test to assert persisted shared logs JSON output and new argument behavior.
specs/data-acquisition-audit.md Updates acquisition inventory/audit narrative for the shared snapshot approach.
specs/data-acquisition-audit-history.md Adds a dated entry documenting the shared activity log cache change.
docs/operations.md Updates operational documentation to describe the single gh aw logs --json snapshot and downstream processing.
dashboard/report/operational-values.mjs Replaces report/artifact-based collection with shared logs JSON parsing + cache merge.
dashboard/report/aic-usage.mjs Performs one repository-wide logs download and optionally persists raw logs JSON for reuse.
activity/README.md Documents new cao-activity snapshot contents (gh-aw-logs/, gh-aw-logs.json).
activity/github-telemetry.mjs Logs core quota remaining alongside telemetry observations.
.github/workflows/activity.yml Wires shared logs JSON/artifacts paths into collectors; removes separate operational-value cache steps.
Review details
  • Files reviewed: 11/11 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread dashboard/report/aic-usage.mjs
Comment thread dashboard/report/operational-values.mjs
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 6, 2026 05:06
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI and others added 5 commits September 6, 2026 05:07
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@copilot add core.... logging to debug caching and downloading

Added non-secret diagnostics for activity-cache hit/miss and size, plus gh-aw logs download/cache paths and record counts. Addressed in 603ebba.

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.

🔵 Needs a closer look

The shared gh-aw-logs.json snapshot can be overwritten with an empty placeholder on download failure (or when no runs are selected), discarding previously restored evidence and undermining the stated “preserve prior observations” behavior.

Review details

Suppressed comments (2)

dashboard/report/aic-usage.mjs:594

  • When no workflow runs are selected, this block always rewrites REPORT_GH_AW_LOGS to an empty JSON file. If the shared cache already contains a prior logs snapshot, overwriting it here unnecessarily discards evidence that other collectors (or later steps) might still use. Consider only creating the empty file when it doesn't already exist.
    } else if (logsPath) {
      await mkdir(path.dirname(logsPath), { recursive: true });
      await writeFile(logsPath, '{"runs":[]}\n');
      log.info`Cached empty gh-aw logs JSON at ${logsPath}; no workflow runs were selected`;
    }

dashboard/report/aic-usage.mjs:588

  • When the gh-aw download fails, the code overwrites any previously-restored shared logs snapshot with an empty {"runs":[]} file. This discards the last known-good snapshot from the activity cache and can break the "preserve prior observations when current evidence is unavailable" behavior (and downstream consumers that could have reused the prior logs JSON). Prefer preserving an existing logs file, and only write an empty placeholder if no prior snapshot exists.
      } catch (error) {
        collectionAvailable = false;
        log.warning`AI Credit usage unavailable: ${error.message}`;
        if (logsPath) {
          await mkdir(path.dirname(logsPath), { recursive: true });
          await writeFile(logsPath, '{"runs":[]}\n');
          log.info`Cached fallback empty gh-aw logs JSON at ${logsPath} after download failure`;
        }
  • Files reviewed: 11/11 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 6, 2026 05:09

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.

🟡 Changes recommended

The new stale-placeholder cleanup in mergeOperationalValueRecords can leave duplicate entries if the newer non-placeholder record still has status: "unavailable" (e.g., has an evaluator digest but no result), because the “observed” detection is currently based on status instead of observation/digest presence.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 17/17 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread dashboard/report/operational-value-history.mjs
mergeOperationalValueRecords() previously used status !== "unavailable"
to detect a real (non-placeholder) record for a run. A real evaluator
run that legitimately errors and reports status: "unavailable" (while
still carrying an evaluatorDigest) was then treated as a placeholder,
so the stale placeholder was never dropped and both records could
coexist for the same run. Key the placeholder check on the presence
of evaluatorDigest instead.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 6, 2026 19:42

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.

Copilot was unable to review this pull request because there is no eligible user to bill. An organization owner can allow the organization to be billed for reviews like this by enabling Copilot code review for users without a license.

…selected runs

- selectedRuns previously omitted a top-level runAttempt, so
  operationalValueRunIdentity() always defaulted to attempt 1 for selected
  runs, causing cached attempt>1 records to be reprocessed every run.
- cachedRunKeys previously used status!=="unavailable" to decide whether to
  skip a run; this incorrectly skipped legitimate errored-but-observed
  records without an evaluatorDigest yet, and could fail to reprocess stale
  placeholders that still report status:"unavailable" but hold real
  evidence. Key on evaluatorDigest presence instead, matching the
  placeholder detection already used in mergeOperationalValueRecords().

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Commit pushed: dc47b87

Generated by PR Sous Chef · copilot · auto · 113.4 AIC · ⌖ 9.63 AIC · ⊞ 9.1K

Copilot AI review requested due to automatic review settings September 6, 2026 20:10

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.

Copilot was unable to review this pull request because there is no eligible user to bill. An organization owner can allow the organization to be billed for reviews like this by enabling Copilot code review for users without a license.

@pelikhan

pelikhan commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

@copilot review code and ensure that failed collection does not clear previous data, it is always incremental.

… collections

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 6, 2026 20:15

Copilot AI commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@copilot review code and ensure that failed collection does not clear previous data, it is always incremental.

Reviewed the collectors. Found that aic-usage.mjs unconditionally overwrote aic-usage.json even on a download failure, wiping previously collected AIC/security run data (the raw gh-aw-logs.json snapshot was already preserved, but the derived output wasn't). Fixed by seeding runs/securityRuns from the previously written output before attempting a fresh download, so a failure now falls back to the last observed record per run instead of an empty placeholder. Also made operational-values.mjs default its cache to its own output path when REPORT_VALUE_CACHE is unset, so it's self-incremental even if that env var is misconfigured. Added regression tests for both. Addressed in dc47b87's follow-up commit.

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.

🔵 Needs a closer look

It materially changes the production activity workflow’s collection/caching model and failure modes across multiple collectors, so it should receive final human review despite strong test updates.

Review details
  • Files reviewed: 17/17 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Filter non-object diagnostics entries in definitionFromLogsResult before
accessing .metric, since the gh-aw logs JSON is external input and can
contain schema drift (null/non-object diagnostics entries), which
previously threw a TypeError and crashed the whole operational-value
collector.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 6, 2026 20:39

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.

Copilot was unable to review this pull request because there is no eligible user to bill. An organization owner can allow the organization to be billed for reviews like this by enabling Copilot code review for users without a license.

@pelikhan

pelikhan commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

@copilot add fault injection tests

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 6, 2026 21:05

Copilot AI commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@copilot add fault injection tests

Added injected-download-failure coverage ensuring previously collected AIC and security records, including unselected runs, are retained. Commit: 37a1543.

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.

🟡 Changes recommended

The new incremental AIC/operational-value collectors have correctness issues around completeness/refresh logic that can misreport collection status and prevent later snapshots from filling missing observation data.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

dashboard/report/aic-usage.mjs:653

  • usage.complete/per-repository complete currently compares reportedRuns (derived from all retained runs) to selectedRuns. Because this collector intentionally retains prior unselected runs, reportedRuns can exceed selectedRuns, causing complete to be false even when every selected run is present.
    const reportedRunsByRepository = Object.groupBy([...runs.values()], (run) => run.repository);
    const repositories = [...runIdsByRepository].map(([repository, runIds]) => {
      const reportedRuns = reportedRunsByRepository[repository]?.length || 0;
      const available = runIds.size === 0 || collectionAvailable;
      return {
        repository,
        selectedRuns: runIds.size,
        reportedRuns,
        available,
        complete: available && reportedRuns === runIds.size,
      };

dashboard/report/operational-values.mjs:147

  • Runs are skipped when the cache already has any record with an evaluatorDigest, even if that cached record has observation: null. That prevents a later logs snapshot from filling in the missing observation payload for the same run.
    // Only skip re-processing a run when the cache already holds a non-placeholder
    // record for it (i.e. one with an evaluatorDigest). A prior "unavailable"
    // placeholder (no evaluatorDigest yet) must not block a later logs snapshot
    // from filling in the real observation for that same run.
    const cachedRunKeys = new Set(cachedRecords
      .filter((record) => record.evaluatorDigest)
      .map((record) => operationalValueRunIdentity(record))
      .filter(Boolean));
  • Files reviewed: 17/17 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +505 to +510
try {
const previousUsage = JSON.parse(await readFile(outputPath, "utf8"));
previousRunsByKey = new Map((previousUsage.runs || []).map((run) => [`${run.repository}:${run.runId}`, run]));
previousSecurityRunsByKey = new Map(
(previousUsage.securityRuns || []).map((run) => [`${run.repository}:${run.runId}`, run]),
);
@pelikhan
pelikhan merged commit e44b0cc into main Sep 6, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants