Repository navigation
Add repo-memory backend for daily AIC guardrail - #62958
Conversation
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>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const countedRuns = readLedgerEntries({ | ||
| repoMemoryDir: options.repoMemoryDir || process.env.GH_AW_DAILY_AIC_REPO_MEMORY_DIR, | ||
| repository, | ||
| workflowId: process.env.GH_AW_WORKFLOW_ID || workflowName, | ||
| actor: actorLogin, | ||
| }).map(entry => ({ | ||
| id: entry.run_id, | ||
| html_url: entry.run_url || "", | ||
| created_at: entry.timestamp, | ||
| conclusion: "completed", | ||
| aic: entry.aic, | ||
| })); | ||
| const totalAIC = countedRuns.reduce((sum, run) => sum + run.aic, 0); |
There was a problem hiding this comment.
The repo-memory backend derives the entire 24h AI-credit total from JSONL files under the agent-writable repo-memory directory (
/tmp/gh-aw/repo-memory/..., synced from a branch that agentic runs push to). Since agent output is untrusted (prompt-injectable) and entries carry no integrity protection, an attacker can cause the ledger files to be truncated/rewritten so thattotalAICreads as 0, fully bypassing themax-daily-ai-creditsguardrail on every subsequent run. Consider keeping an authoritative, non-agent-writable source (e.g. Actions cache/artifact scan) as a cross-check, or signing/validating ledger files against data the agent cannot modify before trusting them for the guardrail decision.
Best fix without changing core functionality: fail closed for enforcement when using repo-memory backend unless an integrity signal is present, instead of trusting raw ledger contents unconditionally. Given only this file can be edited, the safest minimal remediation is to require an explicit integrity acknowledgement/env gate before honoring repo-memory totals; otherwise mark status as
structural_errorand stop. This preserves existing behavior when operators intentionally enable trusted repo-memory mode, while preventing silent bypass by default in untrusted setups.Concretely, in
actions/setup/js/check_daily_aic_workflow_guardrail.cjs, inside theif (backend === REPO_MEMORY_BACKEND) { ... }block (before callingreadLedgerEntries), add a check for an env var such asGH_AW_TRUST_REPO_MEMORY_LEDGER === "true". If absent, set:
daily_ai_credits_guardrail_status = structural_errordaily_ai_credits_guardrail_errorwith a clear message explaining repo-memory ledger is untrusted unless explicitly trustedcore.setFailed(message)andreturnNo new imports or dependencies are needed.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| const countedRuns = readLedgerEntries({ | ||
| repoMemoryDir: options.repoMemoryDir || process.env.GH_AW_DAILY_AIC_REPO_MEMORY_DIR, | ||
| repository, | ||
| workflowId: process.env.GH_AW_WORKFLOW_ID || workflowName, | ||
| actor: actorLogin, | ||
| }).map(entry => ({ | ||
| id: entry.run_id, | ||
| html_url: entry.run_url || "", | ||
| created_at: entry.timestamp, | ||
| conclusion: "completed", | ||
| aic: entry.aic, | ||
| })); | ||
| const totalAIC = countedRuns.reduce((sum, run) => sum + run.aic, 0); |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Current wiring can skip or undercount ledger writes and exposes trusted guardrail state to agent modification.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 6
Open (6)
What changed in this PR
Adds an opt-in repo-memory ledger backend for the daily AIC guardrail, reducing Actions API and artifact lookups.
Changes:
- Adds backend parsing, validation, schema, and compiler wiring.
- Implements JSONL ledger reads/writes and activation integration.
- Adds focused Go and JavaScript tests.
| File | Description |
|---|---|
pkg/workflow/workflow_data.go |
Stores the selected backend. |
pkg/workflow/daily_aic_workflow.go |
Resolves and validates backend configuration. |
pkg/workflow/daily_aic_workflow_guardrail_test.go |
Tests compilation and validation. |
pkg/workflow/compiler_yaml_post_agent.go |
Invokes ledger persistence. |
pkg/workflow/compiler_string_api.go |
Resolves backend during string parsing. |
pkg/workflow/compiler_orchestrator_workflow.go |
Resolves backend during compilation. |
pkg/workflow/compiler_daily_aic_repo_memory.go |
Generates the ledger append step. |
pkg/workflow/compiler_activation_daily_aic.go |
Generates clone and guardrail wiring. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the backend field. |
pkg/parser/schema_test.go |
Tests schema acceptance. |
actions/setup/js/daily_aic_workflow_helpers.cjs |
Identifies AIC usage files. |
actions/setup/js/daily_aic_repo_memory_ledger.test.cjs |
Tests ledger reads and writes. |
actions/setup/js/daily_aic_repo_memory_ledger.cjs |
Implements ledger storage. |
actions/setup/js/check_daily_aic_workflow_guardrail.cjs |
Reads ledger totals during activation. |
| repoMemoryDir: options.repoMemoryDir || process.env.GH_AW_DAILY_AIC_REPO_MEMORY_DIR, | ||
| repository, | ||
| workflowId: process.env.GH_AW_WORKFLOW_ID || workflowName, | ||
| actor: actorLogin, |
| const { findJSONLFiles, isDailyAICUsageJSONLFile, sumAICFromUsageJSONLFiles } = require("./daily_aic_workflow_helpers.cjs"); | ||
| const { getErrorMessage } = require("./error_helpers.cjs"); | ||
|
|
||
| const LEDGER_SUBDIR = "daily-aic-ledger"; |
|
|
||
| // generateDailyAICRepoMemoryLedgerStep appends the current run's AIC usage to | ||
| // the repo-memory ledger when the repo-memory backend is configured. It must run | ||
| // before the repo-memory artifact upload so the existing push job persists the |
| return | ||
| } | ||
| builder.WriteString(" - name: Append daily AIC repo-memory ledger\n") | ||
| fmt.Fprintf(builder, " if: always() && %s\n", maxDailyAICreditsConfiguredIfExpr) |
| } | ||
| } | ||
|
|
||
| c.generateDailyAICRepoMemoryLedgerStep(yaml, data) |
| if !ok { | ||
| return nil | ||
| } | ||
| backend, hasBackend := extractMaxDailyAICBackend(raw) |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Testing safeoutputs transport availability only; no GitHub action taken in this probe. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
|
🏗️ ADR required — draft added for PR #62958I enforced the design-decision gate for this PR because it adds >100 new lines in business-logic directories ( ResultA draft ADR was added at Evidence used
Inferred architectural decisionUse an opt-in git-backed repo-memory JSONL ledger as an alternative backend for the daily AIC guardrail so high-volume workflows can avoid repeated Actions API scans and artifact downloads. Next actionPlease review and refine the drafted ADR, then keep it with the PR as the decision record for this backend addition.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design to the new repo-memory backend for the daily AIC guardrail. Note: several prior review threads (github-advanced-security[bot], Copilot) already flag security/correctness concerns on the agent-writable ledger, actor-scoping semantics, and push-job gating that remain unresolved — this review focuses on additional design/test-coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Untested repo-memory branch in
main(): the new ledger-read path incheck_daily_aic_workflow_guardrail.cjs(threshold comparison,rateLimit === nullsummary rendering) has no dedicated JS unit test, unlike the well-tested artifact-scan path. - Implicit "first memory wins" fallback:
dailyAICRepoMemoryEntrypicks an arbitrary repo-memory entry when none is nameddefaultand multiple are configured, with no test or documented behavior for that case. - Minor clarity nit:
firstRepoMemoryEntryslicesmemories[:1]and loops instead of directly indexing, which obscures a trivial operation. - Existing unresolved threads on this PR (actor-filtering semantics, agent-writable ledger tamper risk, push-job gating on agent failure, missing
GH_AW_MAX_DAILY_AI_CREDITSenv in imported-config case, and post-agent job timing) remain valid and should be resolved before merge.
Positive Highlights
- ✅ Clear separation between artifact-scan and repo-memory code paths in the activation step builder.
- ✅ Good test coverage for the Go compiler wiring (clone step ordering, env var propagation,
requires tools.repo-memoryvalidation). - ✅ Ledger entry validation (
validEntry) is defensive about malformed/stale data and correctly handles per-run de-duplication by latest timestamp.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
o205451.ingest.us.sentry.io
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 110.4 AIC · ⌖ 14.7 AIC · ⊞ 10.1K
Comment /matt to run again
| return RepoMemoryEntry{}, false | ||
| } | ||
| var first RepoMemoryEntry | ||
| for _, memory := range memories[:1] { |
There was a problem hiding this comment.
[/codebase-design] firstRepoMemoryEntry slices memories[:1] and loops over a single-element slice just to assign first — this obscures intent versus a plain memories[0].
💡 Suggested simplification
func firstRepoMemoryEntry(memories []RepoMemoryEntry) (RepoMemoryEntry, bool) {
if len(memories) == 0 {
return RepoMemoryEntry{}, false
}
return memories[0], true
}A deep, simple helper should read as directly as the operation it performs; the loop-over-one-element pattern makes a reader pause to check whether there is hidden logic (e.g. skipping/merging) that isn't actually there.
@copilot please address this.
| return memory, true | ||
| } | ||
| } | ||
| return firstRepoMemoryEntry(data.RepoMemoryConfig.Memories) |
There was a problem hiding this comment.
[/codebase-design] dailyAICRepoMemoryEntry silently falls back to "the first configured memory" when no memory is named default, but this selection is never exercised by a test with more than one tools.repo-memory entry — so a config with two memories (neither named default) will silently pick whichever one happens to be first in frontmatter order, which is fragile and unannounced to the user.
💡 Suggested improvement
Either:
- Require the
repo-memorybackend to name which memory id it targets (e.g.backend: {repo-memory: {memory: my-id}}), removing the implicit "first wins" behavior, or - Add a compiler test with 2+ memories (none named
default) asserting the documented fallback, and mention the fallback explicitly in the frontmatter schema description so users aren't surprised.
Implicit ordering-dependent selection is the kind of interface leakiness /codebase-design flags — the caller has no way to know which memory was picked without reading the compiler source.
@copilot please address this.
|
@copilot this PR still has substantial outstanding review feedback. Please refresh the branch if needed and run the Outstanding themes to finish:
After pushing fixes, resolve the remaining review threads and summarize focused validation results in-thread.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in Focused fixes:
Validation:
|
|
@copilot enable on a few agentic workflows in this repo |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Enabled |
| const ledgerPath = ledgerPathForDay(root, utcDay(now)); | ||
| try { | ||
| fs.appendFileSync(ledgerPath, `${JSON.stringify(entry)}\n`, "utf8"); |
|
@copilot Please move this PR forward. Open review feedback remains from: github-advanced-security, github-actions, copilot-pull-request-reviewer, a48204179-maker. Please refresh the branch, address the outstanding review comments, and run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
… append Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in the latest push: the The other themes from this thread were already addressed in earlier commits on this branch: read-side trust gate ( |
|
@copilot Ensure the final aic cost is written to the repo backend |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
Add glossary entries for user-facing terms introduced since the last scan (through commit 2b90c3a): - Grouped Audit Findings (`gh aw audit --group`, #63032) - Gateway Steering Events (token_steering/timeout_steering in audit output, #62943) - Custom Safe-Output Job Artifacts (`safe-outputs.jobs.<job>.artifacts`, #63017) - Native Web Search (`tools.web-search` on the Copilot engine, #62957) Reviewed but intentionally skipped as internal-only (no dedicated user-facing docs): repo-memory backend for the daily AIC guardrail (#62958) and container image override propagation to threat-detection jobs (#63014). Confirmed no stale gVisor/Docker sbx glossary entries remain after their removal (#63034). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The daily AIC guardrail currently scales with trailing 24h workflow volume because cache misses require Actions API scans plus artifact downloads per run. This adds an opt-in repo-memory JSONL ledger backend so high-volume workflows can read a rolling total from git-backed state instead.
max-daily-ai-credits.backend.repo-memory.tools.repo-memorywhen selected.Repo-memory ledger
Workflow wiring
Schema and tests
Run: https://github.com/github/gh-aw/actions/runs/35894938867