fix(ai-moderator): noop safe output + cache miss hardening#43525
Conversation
…dling - Add explicit `noop:` to safe-outputs frontmatter to guarantee the noop tool is available and intentionally declared - Strengthen cache miss language: the spam log is optional, its absence is normal (first run, 24h expiry, cold cache), and agents must NOT call `missing_data` for a missing spam log - Add explicit "always emit safe output" guideline requiring agents to call `noop` when no moderation action is needed - Recompile ai-moderator.lock.yml Fixes #43203 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the AI Moderator agentic workflow against two observed failure modes: (1) treating an absent ephemeral spam log as missing_data (causing a hard failure), and (2) completing analysis without emitting any safe output.
Changes:
- Declares a
noopsafe output in frontmatter so the workflow can explicitly emit a “no action needed” result. - Strengthens cache-miss guidance by explicitly prohibiting
missing_datawhen the spam log is absent and instructing to proceed with an empty array. - Adds an explicit requirement to always emit at least one safe output (use
noopwhen nothing else is taken), and recompiles the lock file.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/ai-moderator.md | Adds noop safe output and strengthens instructions for spam-log cache misses + mandatory safe output emission. |
| .github/workflows/ai-moderator.lock.yml | Recompiled workflow lock metadata reflecting the frontmatter/body updates. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
There was a problem hiding this comment.
Review: fix(ai-moderator): noop safe output + cache miss hardening
This PR correctly addresses both root causes of the ~86% failure rate. Changes are minimal, focused, and well-reasoned.
What works well
noop:in frontmatter — declaring it explicitly (rather than relying on inference) matches the pattern used across other workflows and is the right fix.- Hard prohibition on
missing_data— upgrading from a soft suggestion to a bold prohibition with justification is appropriate. Explaining the three absence scenarios (first run, 24h expiry, cache miss) removes ambiguity. - Triple-layer noop guarantee — requirement now appears in the frontmatter, Important Guidelines section, and
{{#runtime-import shared/noop-reminder.md}}import. Good reliability layering. - Lock file — only metadata hashes changed; manifest content (secrets, actions, containers, pinned images) is unchanged. Correct recompile.
Non-blocking observation
The systemic root cause — pkg/workflow/prompts/cache_memory_prompt.md unconditionally instructing agents to call missing_data on any cache miss — is still present. This PR correctly overrides it at the workflow level, but the same failure mode could recur in other workflows with optional cache files. A follow-up adding an optional: flag or similar mechanism in cache_memory_prompt.md would prevent recurrence without per-workflow overrides.
No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 39.1 AIC · ⌖ 5.88 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix is solid and directly addresses the root causes. Two observations surfaced.
📋 Key Themes & Highlights
Key Themes
- Systemic risk remains in the template — The per-workflow override in
ai-moderator.mdwins the conflict, butpkg/workflow/prompts/cache_memory_prompt.mdline 12 still unconditionally prescribesmissing_dataon cache miss. Any future workflow with ephemeral cache data will silently inherit the same failure mode. - Noop reminder duplication — The new inline guideline at line 218 overlaps with the existing
{{#runtime-import shared/noop-reminder.md}}at line 226. Harmless now, but they could diverge independently over time.
Positive Highlights
- ✅ Root causes are clearly identified and each addressed with a targeted, minimal change
- ✅ Language upgrade from soft suggestion to hard prohibition with explicit consequence is exactly right
- ✅ Adding
noop:to the frontmattersafe-outputsblock is the correct structural fix — without it the compiler couldn't validate safe-output intent - ✅ Lock file correctly recompiled; the
frontmatter_hashandbody_hashupdates are consistent with the source changes
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 48.4 AIC · ⌖ 6.69 AIC · ⊞ 6.6K
Comment /matt to run again
| ### Reading the Spam Log | ||
|
|
||
| At the start of your analysis, try to read the spam log file at `/tmp/gh-aw/cache-memory/spam-log.json`. This file may not exist (it is absent on the first run or whenever the 24-hour cache has expired) — if it is missing, proceed with an empty array and **do not** call `missing_data`. The file contains an array of spam events: | ||
| At the start of your analysis, try to read the spam log file at `/tmp/gh-aw/cache-memory/spam-log.json`. **This file is optional.** A missing file is completely normal — it will be absent on the first run, after the 24-hour cache expires, or after a cache miss. A missing file is **not** a missing-data error. If the file does not exist, start immediately with an empty array and continue your analysis. **Never call `missing_data` for a missing spam log** — doing so will cause the workflow to fail unnecessarily. The file contains an array of spam events: |
There was a problem hiding this comment.
[/diagnosing-bugs] The per-workflow override is correct, but the upstream system template pkg/workflow/prompts/cache_memory_prompt.md (line 12) still unconditionally instructs agents: "If you look for data in the cache and do not find any, call the missing_data tool". Any other workflow using cache-memory with optional/ephemeral files will face the same 86% failure rate.
💡 Suggested fix
Consider softening the system template so the general case does not prescribe missing_data for absent files:
- **Cache Miss**: If the cached data is **required** for the task to proceed, call the `missing_data` tool with `data_type: "cache_memory"` and `reason: "cache_memory_miss"`. If the file is optional or ephemeral, proceed without it.Without this, the latent conflict remains for any future workflow that uses cache-memory with ephemeral data — they'd need to replicate this per-workflow override.
@copilot please address this.
| - New contributors may have less polished writing - this doesn't necessarily indicate AI generation | ||
| - Provide clear reasoning for each detection in your analysis | ||
| - Only take action if you have high confidence in the detection | ||
| - **You MUST always emit at least one safe output.** After completing your analysis, if no spam was detected and no labels were applied, call the `noop` safe output to confirm that the content was reviewed and no action was required. Never exit without calling a safe output. |
There was a problem hiding this comment.
[/diagnosing-bugs] The new mandatory guideline at line 218 overlaps with the existing {{#runtime-import shared/noop-reminder.md}} at line 226, which already reminds agents to call noop when no action is needed. This duplication is harmless but could diverge over time if one is updated independently.
💡 Suggestion
Consider removing the inline guideline at line 218 and relying solely on the shared noop-reminder.md import at line 226, keeping the canonical reminder in one place. If the shared reminder's wording is insufficient (too soft), strengthen it there rather than overriding inline — this ensures every workflow benefits from the improvement.
If the inline version is intentionally stronger than the shared one (e.g., because this workflow's failure rate justified escalation), document that difference with a brief comment.
@copilot please address this.
🤖 PR Triage — Run §28741077056
Rationale: Fixes ~86% AI Moderator failure rate from 2 root causes (cache_memory_miss hard-fail + missing safe output). 1× APPROVED. Tiny diff (+4/−2). Merge ASAP.
|
|
@copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
The AI Moderator had an ~86% failure rate over 7 days from two distinct failure modes: agents calling
missing_dataon an absent spam log (which the infrastructure treats as a hard failure), and agents completing analysis without emitting any safe output.Root causes
cache_memory_miss: The system-levelcache_memory_prompt.mdtemplate unconditionally instructs agents to callmissing_datawhen cache data is absent. The spam log is intentionally ephemeral (24h TTL, absent on cold start) — the existing soft instruction "do not callmissing_data" lost the conflict against the system prompt.noopwas not declared in the frontmattersafe-outputsblock, and the agent lacked a hard requirement to emit a safe output at all.Changes
safe-outputsfrontmatter — Added explicitnoop:declaration so the intent is clear and not inferred by the compiler.Cache miss language — Replaced the soft suggestion with a hard prohibition and consequence:
Important Guidelines — Added a mandatory safe-output guarantee:
ai-moderator.lock.yml— Recompiled.