Repository navigation
Record authoritative zero AIC evidence for pre-flight noops - #70344
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
![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 /diagnosing-bugs and /tdd — approving. This is a well-scoped, fail-closed fix with solid regression coverage.
📋 Key Themes & Highlights
Verification performed
- Traced the new
not_startedevidence write inharness_retry_runner.cjsthrough to the consumer indaily_aic_component_coverage.cjs(provesExecutionNotStarted) — field names, versioning, and therun_id/run_attemptscoping all line up correctly. - Confirmed the
["agent", "detection", "evals"]component allow-list matches the three call sites that emit this evidence protocol from the Go compiler (compiler_yaml_main_job.go,evals_steps.go,threat_detection_steps.go). - Confirmed
Number.isSafeIntegercorrectly rejects the9007199254740992(2^53) boundary test case, and that non-integer/zero/negative run metadata is rejected.
Positive highlights
- ✅ Fail-closed by construction: evidence is only ever written in the exact pre-flight branch that proves no model ran; a post-attempt noop never downgrades
startedevidence (verified againstdaily_aic_component_coverage.cjs's guardrail logic). - ✅ Atomic write:
.tmp+renameavoids partial/corrupt evidence files, and write failures are caught and logged rather than silently manufacturing false zero-use proof. - ✅ Regression coverage is thorough: covers the happy path (
harness_retry_runner.test.cjs), invalid metadata matrix, write-failure-preserves-prior-evidence, post-attempt noop non-downgrade, and the guardrail consumer side (daily_aic_component_coverage.test.cjs) plus an end-to-end harness spawn test (copilot_harness.test.cjs).
Minor, non-blocking note
- Left one inline comment on
harness_retry_runner.cjsabout the hard-coded component allow-list duplicating names already defined on the Go compiler side — worth a shared constant someday, but not a correctness issue today.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 74.6 AIC · ⌖ 15.9 AIC · ⊞ 10.1K
Comment /matt to run again
| const component = env.GH_AW_AWF_EXECUTION_COMPONENT; | ||
| const runId = Number(env.GITHUB_RUN_ID); | ||
| const runAttempt = Number(env.GITHUB_RUN_ATTEMPT); | ||
| if (evidenceFile && ["agent", "detection", "evals"].includes(component || "") && Number.isSafeInteger(runId) && runId > 0 && Number.isSafeInteger(runAttempt) && runAttempt > 0) { |
There was a problem hiding this comment.
[/codebase-design] The ["agent", "detection", "evals"] allow-list duplicates the three component names already encoded via generateComponentExecutionEvidenceStep call sites in pkg/workflow (compiler_yaml_main_job.go, evals_steps.go, threat_detection_steps.go). Not a blocker, but if a fourth component is ever added, this list is an easy place to forget to update.
💡 Suggestion
Consider exporting a shared KNOWN_EXECUTION_COMPONENTS constant (or generating this list from the same source the Go compiler step names come from) so the two sides can't drift silently.
@copilot please address this.
There was a problem hiding this comment.
Removed the fixed component allow-list in favor of generic component-identifier validation, so new compiler components don’t require a duplicate JS list. Added future-component regression coverage in harness_retry_runner.test.cjs (90c37e3).
There was a problem hiding this comment.
🟡 Changes recommended
Execution evidence is not yet reliable across sandbox boundaries and nested harness invocations.
2 open issues
What changed in this PR
Adds run-scoped zero-use evidence so daily AI-credit accounting can recognize pre-flight noops without charging fallback credits.
Changes:
- Atomically records
not_startedevidence after validating execution metadata. - Adds regression coverage for noop accounting, skipped model invocation, and failure cases.
| File | Description |
|---|---|
| actions/setup/js/harness_retry_runner.test.cjs | Tests evidence creation, validation, and preservation. |
| actions/setup/js/harness_retry_runner.cjs | Records execution evidence for pre-flight noops. |
| actions/setup/js/daily_aic_component_coverage.test.cjs | Tests zero-credit accounting and missing-usage rejection. |
| actions/setup/js/copilot_harness.test.cjs | Verifies noop evidence without invoking the engine. |
Review effort: Max
| * @returns {boolean} | ||
| */ | ||
| function shouldSkipForNoopSafeOutputs({ safeOutputsPath, hasNoopInSafeOutputs, log }) { | ||
| function shouldSkipForNoopSafeOutputs({ safeOutputsPath, hasNoopInSafeOutputs, log, env = process.env }) { |
There was a problem hiding this comment.
Captured execution metadata before child-env construction, stripped it from harness/engine children, and isolated it in the safe-outputs test setup. The runner-side preflight now records noops before sandbox execution (90c37e3).
| fs.mkdirSync(path.dirname(evidenceFile), { recursive: true }); | ||
| fs.writeFileSync(`${evidenceFile}.tmp`, JSON.stringify({ version: 1, component, run_id: runId, run_attempt: runAttempt, state: "not_started" }) + "\n"); | ||
| fs.renameSync(`${evidenceFile}.tmp`, evidenceFile); |
There was a problem hiding this comment.
Moved the preflight check to the runner-side AWF wrapper so it records evidence on the host before sandbox execution. Regression coverage verifies this transport and that a noop emitted after execution starts cannot downgrade evidence (90c37e3).
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: b8463dd
|
…t-noops-aic Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Pre-flight noops skip model execution but leave successful agent jobs with empty accounting, causing the daily AIC guardrail to charge the configured
max-ai-creditsfallback.not_startedexecution evidence scoped to the component, run ID, and attempt. Existing artifact collection and guardrail logic consume it.